Bug 23580

Summary: Add C4::Context->yaml_preference
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, m.de.rooy, martin.renvoize, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22996
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This trivial patch adds a convenient way to retrieve YAML-based system preferences in the code, avoiding the need to handle the decoding in each place they are used.
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 33431    
Attachments: Bug 23580: Unit tests
Bug 23580: Add C4::Context->yaml_preference
Bug 23580: Unit tests
Bug 23580: Add C4::Context->yaml_preference
Bug 23580: Unit tests
Bug 23580: Add C4::Context->yaml_preference

Description Tomás Cohen Arazi 2019-09-10 14:40:24 UTC
I propose to add a way to retrieve the decoded YAML from (YAML) system preferences. This way we don't need to write code in vain for decoding them in-place.

Also, I propose we move pipe-separated sysprefs into YAML lists. This raised on reviewing 22996.
Comment 1 Tomás Cohen Arazi 2019-09-10 14:49:16 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2019-09-10 14:49:20 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2019-09-10 15:55:19 UTC
Created attachment 92696 [details] [review]
Bug 23580: Unit tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Owen Leonard 2019-09-10 15:55:22 UTC
Created attachment 92697 [details] [review]
Bug 23580: Add C4::Context->yaml_preference

This patch adds a new methods to be used on YAML-based sysprefs. The
idea is that we would use it instead of handling the YAML decoding
in-place everywhere we use YAML syprefs.

Ideally, we could even move pipe separated values sysprefs into YAML,
etc.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Context.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Marcel de Rooy 2019-09-11 11:01:36 UTC
Picked for QA
Comment 6 Marcel de Rooy 2019-09-11 11:24:08 UTC
Created attachment 92707 [details] [review]
Bug 23580: Unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2019-09-11 11:24:12 UTC
Created attachment 92708 [details] [review]
Bug 23580: Add C4::Context->yaml_preference

This patch adds a new methods to be used on YAML-based sysprefs. The
idea is that we would use it instead of handling the YAML decoding
in-place everywhere we use YAML syprefs.

Ideally, we could even move pipe separated values sysprefs into YAML,
etc.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Context.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2019-09-11 11:27:33 UTC
One point of attention:
C4/Circulation.pm:    my $update_loc_rules = get_yaml_pref_hash('UpdateItemLocationOnCheckin');
C4/Circulation.pm:    my $denyingrules = Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash();

This routine comes from Koha/Util/SystemPreferences.pm.
It is parsing (probably) a small subset of YAML constructions, and is meant to always return a hashref. Surely, we could better use YAML to parse and expect both arrayrefs and hashrefs.

So, I would recommend to remove get_yaml_pref_hash now too.
Comment 9 Tomás Cohen Arazi 2019-09-11 14:50:01 UTC
(In reply to Marcel de Rooy from comment #8)
> One point of attention:
> C4/Circulation.pm:    my $update_loc_rules =
> get_yaml_pref_hash('UpdateItemLocationOnCheckin');
> C4/Circulation.pm:    my $denyingrules =
> Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash();
> 
> This routine comes from Koha/Util/SystemPreferences.pm.
> It is parsing (probably) a small subset of YAML constructions, and is meant
> to always return a hashref. Surely, we could better use YAML to parse and
> expect both arrayrefs and hashrefs.
> 
> So, I would recommend to remove get_yaml_pref_hash now too.

Excellent point. I personally mistrust *Utils libraries in general :-D I understand the use of course.

I agree the implementation on this bugis far more flexible, and should be preferred.

One thing I fancied about this was: shouldn't we add a new type of syspref or column so we can make ->preference actually return the decoded syspref if required?
Comment 10 Marcel de Rooy 2019-09-11 14:57:47 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> One thing I fancied about this was: shouldn't we add a new type of syspref
> or column so we can make ->preference actually return the decoded syspref if
> required?

I like the idea, although the need is not so high for now.
Comment 11 Martin Renvoize 2019-09-12 15:44:50 UTC
Nice work!

Pushed to master for 19.11.00
Comment 12 Fridolin Somers 2019-09-13 13:16:38 UTC
Enhancement not pushed to 19.05.x