Bug 23580 - Add C4::Context->yaml_preference
Summary: Add C4::Context->yaml_preference
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2019-09-10 14:40 UTC by Tomás Cohen Arazi
Modified: 2019-09-13 13:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
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


Attachments
Bug 23580: Unit tests (2.09 KB, patch)
2019-09-10 14:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23580: Add C4::Context->yaml_preference (1.56 KB, patch)
2019-09-10 14:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23580: Unit tests (2.14 KB, patch)
2019-09-10 15:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23580: Add C4::Context->yaml_preference (1.62 KB, patch)
2019-09-10 15:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23580: Unit tests (2.18 KB, patch)
2019-09-11 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23580: Add C4::Context->yaml_preference (1.66 KB, patch)
2019-09-11 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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