Currently, C4::Context->preference('YAMLpref') returns a string that needs to be parsed by any code using the preference. My suggestion is to return a parsed version instead (a hash/array ref or a string) and using the syspref cache with the parsed value. Like this, we could: - centralize parsing of YAML system preferences - optimize use of system preference cache by caching the parsed version instead. Parse once and store in cache; no need to re-parse on each call! We could centralize this functionality into Koha::Config::SysPref->value() (used by C4::Context->preference). Use database column systempreferences.type with value of "yaml" to trigger this functionality in "value()"-method.
Created attachment 76382 [details] [review] Bug 20930: Dump and load system preferences of YAML/JSON type Currently, C4::Context->preference('YAMLpref') returns a string that needs to be parsed by any code using the preference. My suggestion is to return a parsed version instead (a hash/array ref or a string) and using the syspref cache with the parsed value. Like this, we could: - centralize parsing of YAML system preferences - optimize use of system preference cache by caching the parsed version instead. Parse once and store in cache; no need to re-parse on each call! We can centralize this functionality into Koha::Config::SysPref->value() (used by C4::Context->preference). Use database column systempreferences.type with value of "YAML" to trigger this functionality in "value()"-method. To test: 1. prove t/db_dependent/Context.t
Created attachment 76383 [details] [review] Bug 20930: First example - Change OpacHiddenItems to type YAML To test: 1. prove t/db_dependent/Items.t 2. Check that desired items are still hidden in OPAC by using OpacHiddenItems as documented here: https://wiki.koha-community.org/wiki/OpacHiddenItems
Created attachment 76384 [details] [review] Bug 20930: Display human readable errors in preferences page Displays human readable error when YAML/JSON preference has invalid syntax. If multiple preferences are modified at the same time, notifies librarian of the preferences that were saved and the ones that had errors. To test: 1. Apply patches 2. Write invalid YAML syntax to OpacHiddenItems system preference 3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error 4. Write valid YAML syntax to OpacHiddenItems system preference 5. Observe "Saved preference OpacHiddenItems" message 6. Test steps 2-5 for both single preference modification and together with other system preferences. --- invalid: yaml syntax --- valid: yaml: syntax
Changed Bug title from "Handle YAML type system preferences as parsed references" to "Validate and cache parsed YAML/JSON type system preferences" as this now also adds syntax validation for librarians, so that it is no longer possible to store invalid YAML system preferences.
Also fyi, these patches do not automatically change the functionality for all YAML/JSON type system preferences. The type (systempreferences.type) needs to be YAML before this applies for the preference.
Hi Lari, please set the assignee to yourself if you plan to work on a bug or are providing patches for it, thx!
I'm afraid Lari might be unable to work on this, so I'm taking it for any further work.
Hi Ere, any progress with this patch? Can you try to rebase it on the master? Thank you.
Created attachment 91861 [details] [review] Bug 20930: Dump and load system preferences of YAML/JSON type Currently, C4::Context->preference('YAMLpref') returns a string that needs to be parsed by any code using the preference. My suggestion is to return a parsed version instead (a hash/array ref or a string) and using the syspref cache with the parsed value. Like this, we could: - centralize parsing of YAML system preferences - optimize use of system preference cache by caching the parsed version instead. Parse once and store in cache; no need to re-parse on each call! We can centralize this functionality into Koha::Config::SysPref->value() (used by C4::Context->preference). Use database column systempreferences.type with value of "YAML" to trigger this functionality in "value()"-method. To test: 1. Make sure memcached is available 2. Run tests with caching enabled, e.g.: MEMCACHED_SERVERS=localhost:11211 prove t/db_dependent/Context.t 3. Make sure all tests pass and none were skipped
Created attachment 91862 [details] [review] Bug 20930: First example - Change OpacHiddenItems to type YAML To test: 1. prove t/db_dependent/Items.t 2. Check that desired items are still hidden in OPAC by using OpacHiddenItems as documented here: https://wiki.koha-community.org/wiki/OpacHiddenItems
Created attachment 91863 [details] [review] Bug 20930: Display human readable errors in preferences page Displays human readable error when YAML/JSON preference has invalid syntax. If multiple preferences are modified at the same time, notifies librarian of the preferences that were saved and the ones that had errors. To test: 1. Apply patches 2. Write invalid YAML syntax to OpacHiddenItems system preference 3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error 4. Write valid YAML syntax to OpacHiddenItems system preference 5. Observe "Saved preference OpacHiddenItems" message 6. Test steps 2-5 for both single preference modification and together with other system preferences.
I believe I've managed to make this apply and work...
Would be nice to get this signed-off - it makes both sysadmins' and librarians' life easier as it is very simple to make mistakes in YAML syntax. Validations are usually pretty nice, especially in this case.
Created attachment 94412 [details] [review] Bug 20930: Display human readable errors in preferences page Displays human readable error when YAML/JSON preference has invalid syntax. If multiple preferences are modified at the same time, notifies librarian of the preferences that were saved and the ones that had errors. To test: 1. Apply patches 2. Write invalid YAML syntax to OpacHiddenItems system preference 3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error You should also receive a hint on line and column numbers of where the error occured. 4. Write valid YAML syntax to OpacHiddenItems system preference 5. Observe "Saved preference OpacHiddenItems" message 6. Test steps 2-5 for both single preference modification and together with other system preferences.
Created attachment 94413 [details] [review] Bug 20930: Make line and column number alert translatable To test: 1. Apply patch 2. Store invalid YAML syntax into OpacHiddenItems preference 3. Observe error 4. You should also see line and column number of where the error occured
Just one minor thing: I'd like to assume we'll never just tell the user that there's invalid syntax without telling the position, so could the two messages be combined into one, like "Error: Preference '%s' has invalid syntax at line %s, column %s."?
Created attachment 96108 [details] [review] Bug 20930: Dump and load system preferences of YAML/JSON type Currently, C4::Context->preference('YAMLpref') returns a string that needs to be parsed by any code using the preference. My suggestion is to return a parsed version instead (a hash/array ref or a string) and using the syspref cache with the parsed value. Like this, we could: - centralize parsing of YAML system preferences - optimize use of system preference cache by caching the parsed version instead. Parse once and store in cache; no need to re-parse on each call! We can centralize this functionality into Koha::Config::SysPref->value() (used by C4::Context->preference). Use database column systempreferences.type with value of "YAML" to trigger this functionality in "value()"-method. To test: 1. Make sure memcached is available 2. Run tests with caching enabled, e.g.: MEMCACHED_SERVERS=localhost:11211 prove t/db_dependent/Context.t 3. Make sure all tests pass and none were skipped Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 96109 [details] [review] Bug 20930: First example - Change OpacHiddenItems to type YAML To test: 1. prove t/db_dependent/Items.t 2. Check that desired items are still hidden in OPAC by using OpacHiddenItems as documented here: https://wiki.koha-community.org/wiki/OpacHiddenItems Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 96110 [details] [review] Bug 20930: Display human readable errors in preferences page Displays human readable error when YAML/JSON preference has invalid syntax. If multiple preferences are modified at the same time, notifies librarian of the preferences that were saved and the ones that had errors. To test: 1. Apply patches 2. Write invalid YAML syntax to OpacHiddenItems system preference 3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error You should also receive a hint on line and column numbers of where the error occured. 4. Write valid YAML syntax to OpacHiddenItems system preference 5. Observe "Saved preference OpacHiddenItems" message 6. Test steps 2-5 for both single preference modification and together with other system preferences. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 96111 [details] [review] Bug 20930: Make line and column number alert translatable To test: 1. Apply patch 2. Store invalid YAML syntax into OpacHiddenItems preference 3. Observe error 4. You should also see line and column number of where the error occured Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 96112 [details] [review] Bug 20930: (follow-up) Improve formatting of error message This patch makes a few changes to system preferences JavaScript so that the error message for invalid YAML is output on one line. CSS has been modified to improve the readability of multiple error messages. The order of CSS files loaded by preferences.tt has been changed to ensure that CSS in preferences.css overrides plugin-related CSS. Unrelated: An incorrect ";" has been replaced with ":" in preferences.tt. To test, apply the patch and repeat the previous test plan. Confirm that the error messages appear as expected.
Ho Owen, can You rebase this patch on master? Applying: Bug 20930: Dump and load system preferences of YAML/JSON type Using index info to reconstruct a base tree... M C4/Context.pm M Koha/Config/SysPref.pm M Koha/Exceptions/Config.pm M t/db_dependent/Context.t Falling back to patching base and 3-way merge... No changes -- Patch already applied. Applying: Bug 20930: First example - Change OpacHiddenItems to type YAML Using index info to reconstruct a base tree... M C4/Biblio.pm M C4/Items.pm M installer/data/mysql/sysprefs.sql M opac/opac-search.pl M t/db_dependent/Items.t Falling back to patching base and 3-way merge... No changes -- Patch already applied. Applying: Bug 20930: Display human readable errors in preferences page Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt M koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js M svc/config/systempreferences Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt error: Failed to merge in the changes.
Im sorry, my fault. Patched twice.
Created attachment 96817 [details] [review] Bug 20930: Dump and load system preferences of YAML/JSON type Currently, C4::Context->preference('YAMLpref') returns a string that needs to be parsed by any code using the preference. My suggestion is to return a parsed version instead (a hash/array ref or a string) and using the syspref cache with the parsed value. Like this, we could: - centralize parsing of YAML system preferences - optimize use of system preference cache by caching the parsed version instead. Parse once and store in cache; no need to re-parse on each call! We can centralize this functionality into Koha::Config::SysPref->value() (used by C4::Context->preference). Use database column systempreferences.type with value of "YAML" to trigger this functionality in "value()"-method. To test: 1. Make sure memcached is available 2. Run tests with caching enabled, e.g.: MEMCACHED_SERVERS=localhost:11211 prove t/db_dependent/Context.t 3. Make sure all tests pass and none were skipped Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 96818 [details] [review] Bug 20930: First example - Change OpacHiddenItems to type YAML To test: 1. prove t/db_dependent/Items.t 2. Check that desired items are still hidden in OPAC by using OpacHiddenItems as documented here: https://wiki.koha-community.org/wiki/OpacHiddenItems Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 96819 [details] [review] Bug 20930: Display human readable errors in preferences page Displays human readable error when YAML/JSON preference has invalid syntax. If multiple preferences are modified at the same time, notifies librarian of the preferences that were saved and the ones that had errors. To test: 1. Apply patches 2. Write invalid YAML syntax to OpacHiddenItems system preference 3. Observe "Error; Preference OpacHiddenItems has invalid syntax" error You should also receive a hint on line and column numbers of where the error occured. 4. Write valid YAML syntax to OpacHiddenItems system preference 5. Observe "Saved preference OpacHiddenItems" message 6. Test steps 2-5 for both single preference modification and together with other system preferences. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 96820 [details] [review] Bug 20930: Make line and column number alert translatable To test: 1. Apply patch 2. Store invalid YAML syntax into OpacHiddenItems preference 3. Observe error 4. You should also see line and column number of where the error occured Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 96821 [details] [review] Bug 20930: (follow-up) Improve formatting of error message This patch makes a few changes to system preferences JavaScript so that the error message for invalid YAML is output on one line. CSS has been modified to improve the readability of multiple error messages. The order of CSS files loaded by preferences.tt has been changed to ensure that CSS in preferences.css overrides plugin-related CSS. Unrelated: An incorrect ";" has been replaced with ":" in preferences.tt. To test, apply the patch and repeat the previous test plan. Confirm that the error messages appear as expected. Signed-off-by: Michal Denar <black23@gmail.com>
QA comments: 1. You remove the validation from GetHiddenItemnumbers and so assume that the data in the pref is correct, but maybe wrong data exist in the DB prior to this patch. (Also, use YAML in C4::Items that is no longer needed) 2. I do not think we should not use YAML to convert the syspref's value to perl struct. For instance, "field: [value1, value2]" does not work, ie. the ref($syspref_val) return an empty string (means it's a scalar): To recreate, edit OpacHiddenItems with "field: [value1, value2]" Apply: diff --git a/C4/Items.pm b/C4/Items.pm index 519ce41991..283def5e4c 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -1202,6 +1202,8 @@ sub GetHiddenItemnumbers { my @resultitems; my $hidingrules = C4::Context->preference('OpacHiddenItems'); + warn $hidingrules; + warn ref($hidingrules); return () if !$hidingrules or ref($hidingrules) ne 'HASH'; my $dbh = C4::Context->dbh; Finally run: use Koha::Items; use C4::Items; my $items = Koha::Items->search->unblessed; C4::Items::GetHiddenItemnumbers({items => $items}); It displays: field: [value1, value2] at /kohadevbox/koha/C4/Items.pm line 1205. Warning: something's wrong at /kohadevbox/koha/C4/Items.pm line 1206. 3. "Error: your data might not have been saved", it is what I get when I generate an invalid yaml, I do not get info about the line, or what the error is.
4. Update DB entry needed to prevent regression if syspref contains only \s - my $opachiddenitems = $opac - && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); - + my $opachiddenitems = $opac && ( C4::Context->preference('OpacHiddenItems') );