Bug 17480 - UpdateNotForLoanStatusOnCheckin preference requires space after colon
Summary: UpdateNotForLoanStatusOnCheckin preference requires space after colon
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 11629
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-21 14:51 UTC by Owen Leonard
Modified: 2023-03-22 10:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2016-10-21 14:51:35 UTC
YAML requires that there be a space after a colon, so the UpdateNotForLoanStatusOnCheckin preference does too:

-1:0 will not work

-1:<space>0 will work

We should either handle this gracefully for the user (preferable, since this preference will only use numeric values), or update the preference's description to highlight the requirement.
Comment 1 Jonathan Druart 2016-11-14 13:00:33 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.
Comment 2 jpl 2023-02-20 12:28:54 UTC
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
Comment 3 Katrin Fischer 2023-02-20 19:11:16 UTC
Should we deactivate the feature somehow if misconfigured so it doesn't break things?
Comment 4 jpl 2023-03-17 16:24:12 UTC
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.
Comment 5 Jonathan Druart 2023-03-22 09:50:27 UTC
(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.
Comment 6 Jonathan Druart 2023-03-22 10:02:55 UTC
($@ ? " : $@" || "");
should be
($@ ? " : $@" : "");
Comment 7 Katrin Fischer 2023-03-22 10:04:37 UTC
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.