Summary: | UpdateNotForLoanStatusOnCheckin preference requires space after colon | ||
---|---|---|---|
Product: | Koha | Reporter: | Owen Leonard <oleonard> |
Component: | System Administration | Assignee: | Bugs List <koha-bugs> |
Status: | NEW --- | QA Contact: | Testopia <testopia> |
Severity: | minor | ||
Priority: | P5 - low | CC: | data-27FC05A3CB97, gmcharlt, jonathan.druart |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20930 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 11629 | ||
Bug Blocks: |
Description
Owen Leonard
2016-10-21 14:51:35 UTC
I would be good to have a syspref integrity check (in the about page for instance). A first step could be to have a validation on prefs expecting YAML. This bug is still around, but it now also prevents any item from being checked in if there is no space after the colon in UpdateNotForLoanStatusOnCheckin. All checkins will fail with a 500 error, even for items that do not have any specific NOT_LOAN authorized value. This can be very frustrating for users as it is hard to spot. The following appears in plack-error.log: Can't use string ("-4:0") as a HASH ref while "strict refs" in use at /usr/share/koha/lib/C4/Circulation.pm line 2156. Tested with Koha 22.11.00 Should we deactivate the feature somehow if misconfigured so it doesn't break things? I think that would be the best option. If the code that parses UpdateNotForLoanStatusOnCheckin can't make sense of it, then it should be treated as if it were empty. (In reply to Katrin Fischer from comment #3) > Should we deactivate the feature somehow if misconfigured so it doesn't > break things? diff --git a/C4/Circulation.pm b/C4/Circulation.pm index e190019ae1a..b4437331027 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2150,8 +2150,8 @@ sub AddReturn { $yaml = "$yaml\n\n"; # YAML is anal on ending \n. Surplus does not hurt my $rules; eval { $rules = YAML::XS::Load(Encode::encode_utf8($yaml)); }; - if ($@) { - warn "Unable to parse UpdateNotForLoanStatusOnCheckin syspref : $@"; + if ($@ || ref($rules) ne 'HASH') { + warn "Unable to parse UpdateNotForLoanStatusOnCheckin syspref " . ($@ ? " : $@" || ""); } else { foreach my $key ( keys %$rules ) { This patch does what you want, but I don't think it's a good idea. If something is misconfigured it should not be ignored. Only the warn in the logs is certainly very easy to missed. ($@ ? " : $@" || ""); should be ($@ ? " : $@" : ""); Maybe we could put something on the about page or an alert on top of the system preferences? We could also warn on saving that it was saved but is incorrect. Not sure what would be the best/easiest to do. OpacHiddenItems can also cause some serious crashing when misconfigured, so we have some candidates here. |