Bug 25556

Summary: Holds blocked when empty holdallowed value present in circulation_rules
Product: Koha Reporter: Andrii Nugged <nugged>
Component: Hold requestsAssignee: Peter Vashchuk <stalkernoid>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: aleisha, gmcharlt, jonathan.druart, slavashishkin, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18936    
Bug Blocks:    
Attachments: Bug 25556: fix for unnecessary empty name/value pairs creation
Bug 25556: fix for unnecessary empty name/value pairs creation
Bug 25556: fix for unnecessary empty name/value pairs creation

Description Andrii Nugged 2020-05-20 18:20:18 UTC
Holds blocked ("Cannot place hold" yellow notification window) for default rule after onboarding.pl (web installation process). 

That happens because /cgi-bin/koha/installer/onboarding.pl script creates the empty (holdallowed="") key/value pair in the circulation_rules table.

Probably empty ("") holdallowed key assumed to have the same behavior as undef/NULL, because:

  - in the interface (/cgi-bin/koha/admin/smart-rules.pl) both are interpreted
    as "Not set" (undef or ""),

  - if to remove this empty value for holdallowed from circulation_rules
    table at all (so have it undef) holds allowed and not blocked.

  - in other cases if "holdallowed" has correct values from
    the dropdown list (0, 1, 2, 3) - all works as expected.


I.e.:

  1. ="" has behavior as ="0",

  2. value absence (NULL/undef) has behavior as ="2" ("from any library").

  3. but for both values site interface shows "not set".

This makes confusion: in 1. it blocks holds, and in 2. it allows.

I propose we make both "" and undef work similarly as "not set" in "check if holds allowed" code, and both to allow holds like undef now does.

And at the same time remove "" creation but use undef instead in the onboarding process when done first rule creation.
Comment 1 Jonathan Druart 2020-05-22 09:54:56 UTC
No, undef should be the same as no rule, a default value could apply.

"" means unlimited in some case.
Comment 2 Jonathan Druart 2020-05-22 09:56:55 UTC
Example of default values in C4/Circulation.pm

1765     $rules->{holdallowed} = defined $holdallowed_rule
1766         ? $holdallowed_rule->rule_value
1767         : 2;

Example of unlimited:

 412     if (defined($maxissueqty_rule) and $maxissueqty_rule->rule_value ne '') {
Comment 3 Jonathan Druart 2020-05-22 10:01:18 UTC
MariaDB [koha_kohadev]> select * from circulation_rules where rule_name="reservesallowed";
+----+------------+--------------+----------+-----------------+------------+
| id | branchcode | categorycode | itemtype | rule_name       | rule_value |
+----+------------+--------------+----------+-----------------+------------+
|  3 | NULL       | NULL         | NULL     | reservesallowed |            |
+----+------------+--------------+----------+-----------------+------------+
1 row in set (0.000 sec)


And I am not blocked from placing holds. On which screen do you see this limitation? Are you running master?
Comment 4 Jonathan Druart 2020-05-22 10:02:24 UTC
Sorry, looking at the wrong rule. You said "holdallowed"!
Comment 5 Jonathan Druart 2020-05-22 10:05:15 UTC
So it's a bug in the onboarding tool, the rule should not be created if "" has been selected.

We could handle that "" value later in the code, but I think we need a quick fix for 20.05. Are you planning to provide a patch?
Comment 6 Andrii Nugged 2020-05-22 10:07:24 UTC
> And I am not blocked from placing holds. On which screen do you see this limitation? Are you running master?

this happens when onboarding creates default rules, so it's a coincidence of a few fields I suspect, after onboarding.pl I have in circulation_rules contents like these:

1.  (clean "reset_all" db)
2. TRUNTACED circulation_rules

3.
INSERT INTO `circulation_rules` (`id`, `branchcode`, `categorycode`, `itemtype`, `ccode`, `shelving_location`, `rule_name`, `rule_value`)
VALUES
 (1, NULL, NULL, NULL, NULL, NULL, 'hardduedate', ''),
 (2, NULL, NULL, NULL, NULL, NULL, 'rentaldiscount', '0'),
 (3, NULL, NULL, NULL, NULL, NULL, 'article_requests', 'no'),
 (4, NULL, NULL, NULL, NULL, NULL, 'firstremind', '0'),
 (5, NULL, NULL, NULL, NULL, NULL, 'issuelength', '14'),
 (6, NULL, NULL, NULL, NULL, NULL, 'maxsuspensiondays', ''),
 (7, NULL, NULL, NULL, NULL, NULL, 'no_auto_renewal_after', ''),
 (8, NULL, NULL, NULL, NULL, NULL, 'cap_fine_to_replacement_price', '0'),
 (9, NULL, NULL, NULL, NULL, NULL, 'auto_renew', '0'),
 (10, NULL, NULL, NULL, NULL, NULL, 'chargeperiod', '0'),
 (11, NULL, NULL, NULL, NULL, NULL, 'fine', '0'),
 (12, NULL, NULL, NULL, NULL, NULL, 'lengthunit', 'days'),
 (13, NULL, NULL, NULL, NULL, NULL, 'holds_per_day', '10'),
 (14, NULL, NULL, NULL, NULL, NULL, 'maxissueqty', '50'),
 (15, NULL, NULL, NULL, NULL, NULL, 'reservesallowed', '10'),
 (16, NULL, NULL, NULL, NULL, NULL, 'overduefinescap', ''),
 (17, NULL, NULL, NULL, NULL, NULL, 'opacitemholds', 'N'),
 (18, NULL, NULL, NULL, NULL, NULL, 'renewalperiod', '14'),
 (19, NULL, NULL, NULL, NULL, NULL, 'norenewalbefore', ''),
 (20, NULL, NULL, NULL, NULL, NULL, 'holds_per_record', '1'),
 (21, NULL, NULL, NULL, NULL, NULL, 'onshelfholds', '1'),
 (22, NULL, NULL, NULL, NULL, NULL, 'finedays', '0'),
 (23, NULL, NULL, NULL, NULL, NULL, 'renewalsallowed', '10'),
 (24, NULL, NULL, NULL, NULL, NULL, 'chargeperiod_charge_at', '0'),
 (25, NULL, NULL, NULL, NULL, NULL, 'hardduedatecompare', '-1'),
 (26, NULL, NULL, NULL, NULL, NULL, 'maxonsiteissueqty', ''),
 (27, NULL, NULL, NULL, NULL, NULL, 'no_auto_renewal_after_hard_limit', ''),
 (28, NULL, NULL, NULL, NULL, NULL, 'patron_maxonsiteissueqty', ''),
 (29, NULL, NULL, NULL, NULL, NULL, 'max_holds', ''),
 (30, NULL, NULL, NULL, NULL, NULL, 'patron_maxissueqty', ''),
 (31, NULL, NULL, NULL, NULL, NULL, 'returnbranch', ''),
 (32, NULL, NULL, NULL, NULL, NULL, 'holdallowed', ''),
 (33, NULL, NULL, NULL, NULL, NULL, 'hold_fulfillment_policy', '');


and it doesn't allow to have holds:

/cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1

but if to remove:

 (32, NULL, NULL, NULL, NULL, NULL, 'holdallowed', ''),

from DB, it allows by the same link.
Comment 7 Andrii Nugged 2020-05-22 10:10:36 UTC
this "INSERT INTO `circulation_rules`" above is an example what contents I will have with truncated circulation_rules + run /cgi-bin/koha/installer/onboarding.pl or do fresh clean we install.
Comment 8 Andrii Nugged 2020-05-22 10:19:51 UTC
So we have two confused-me places:


1. the presence in onboarding.pl of this: 

/opt/n/koha/git/KohaCommunity/installer/onboarding.pl:
  303:                 holdallowed             => "",

makes impossible to plane a hold but even there's not expected to be this block (just re-save "Default checkout, hold policy by patron category" again in .../smart-rules.pl and see that "holds not allowed" gone)

which I propose just to be removed (so not to store "" for all those three items for "Default checkout, hold policy by patron category" rule).



2. Because website (.../smart-rules.pl) shows the same "Not Set" for both, 
undef(=absent) and for "", no matter do we have or not this line in DB:

 (32, NULL, NULL, NULL, NULL, NULL, 'holdallowed', ''),

in DB.

I.e. that dropdown acts like this:

Hold policy:
- "" or absent(undef): "Not Set"
- 0: "No holds allowed"
- 1: "From home library"
- 2: "From any library"
- 3: "From local hold group"

but "" or absence(undef) gives different results in P1.

Here I propose to make both "" and absence(undef) to act similarly if both of them assumed to be "Not Set" = "not limiting at all".
Comment 9 Jonathan Druart 2020-05-22 10:37:22 UTC
At first glance I'd say we should remove $params_2 and $params_3 from installer/onboarding.pl.
Comment 10 Andrii Nugged 2020-05-22 10:38:35 UTC
I agree. That was there because it was aimed for old "issuingrules" table, nowadays we have circulation_rules.
Comment 11 Andrii Nugged 2020-05-22 10:39:29 UTC
Pet(In reply to Jonathan Druart from comment #5)
> We could handle that "" value later in the code, but I think we need a quick
> fix for 20.05. Are you planning to provide a patch?

Yes, NuggedTeam prepping this now ASAP :).
Comment 12 Jonathan Druart 2020-05-22 10:41:07 UTC
(In reply to Jonathan Druart from comment #9)
> At first glance I'd say we should remove $params_2 and $params_3 from
> installer/onboarding.pl.

No, we should keep patron_maxissueqty, patron_maxonsiteissueqty and max_holds from params_2
Comment 13 Andrii Nugged 2020-05-22 12:35:17 UTC
(obviously, I planned to pre-check before cleaning up both rules or to leave what needed, but I got your point)


here's some more observations:


1. On clean (truncated) DB after onboarding tool, I have all as in above message "INSERT..." (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25556#c6)


2. And when I check holds,  I got "Cannot place hold": /cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1


3. now I just go to /cgi-bin/koha/admin/smart-rules.pl to section
"Default checkout, hold policy by patron category": it looks "empty"
("Not Set" for all dropdowns),


4. now I press "Unset" button so it _removes_ from DBj all of these key/val pairs at all:

 (28, NULL, NULL, NULL, NULL, NULL, 'patron_maxonsiteissueqty', ''),
 (29, NULL, NULL, NULL, NULL, NULL, 'max_holds', ''),
 (30, NULL, NULL, NULL, NULL, NULL, 'patron_maxissueqty', ''),
 (31, NULL, NULL, NULL, NULL, NULL, 'returnbranch', ''),
 (32, NULL, NULL, NULL, NULL, NULL, 'holdallowed', ''),
 (33, NULL, NULL, NULL, NULL, NULL, 'hold_fulfillment_policy', '');


5.I check on smart-rules.pl in "Default checkout, hold policy by patron category" looks "empty" in the same way as in p3.


6. And when I check holds, it allows hold. /cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1


7. now I go to smart-rules.pl again to the same section ("Default checkout, hold policy by patron category"): press "Unset" button so it adds back to db but only these lines:

INSERT INTO `circulation_rules` (`id`, `branchcode`, `categorycode`, `itemtype`, `ccode`, `shelving_location`, `rule_name`, `rule_value`)
VALUES
 (35, NULL, NULL, NULL, NULL, NULL, 'hold_fulfillment_policy', ''),
 (36, NULL, NULL, NULL, NULL, NULL, 'returnbranch', ''),
 (37, NULL, NULL, NULL, NULL, NULL, 'patron_maxonsiteissueqty', ''),
 (38, NULL, NULL, NULL, NULL, NULL, 'patron_maxissueqty', ''),
 (39, NULL, NULL, NULL, NULL, NULL, 'max_holds', '');

added, i.e. no that one added back:

 (32, NULL, NULL, NULL, NULL, NULL, 'holdallowed', ''),

so:


8. And when I check holds, it allows hold. /cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1


CONCLUSION: because all SIX keys="" so both from "params_2" and "params_3" removed in p.4 above – and that is "unset" action, I propose to remove both "params_2" and "params_3" from onboarding tool.
Comment 14 Jonathan Druart 2020-05-22 12:58:05 UTC
Checking the code

* max_holds, C4/Reserves.pm
 474     if ( $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) {

=> No rule or '' mean unlimited

* patron_maxissueqty, C4/Circulation.pm
 509     if (defined($branch_borrower_circ_rule->{patron_maxissueqty}) and $branch_borrower_circ_rule->{patron_maxissueqty} ne '') {

=> No rule or '' mean unlimited

* Same for patron_maxonsiteissueqty

* hold_fulfillment_policy, C4/Circulation.pm
1768     $rules->{hold_fulfillment_policy} = defined $hold_fulfillment_policy_rule
1769         ? $hold_fulfillment_policy_rule->rule_value
1770         : 'any';

=> Should not be "" (must be fixed, but later). Must be fixed at onboarding step.

* Same for holdallowed and returnbranch

I am just adding noise here.

tl;dr; yes, remove them!
Comment 15 Peter Vashchuk 2020-05-25 14:43:39 UTC
Created attachment 105325 [details] [review]
Bug 25556: fix for unnecessary empty name/value pairs creation

During the web-install onboarding process, when the default circulation rule gets created, "circulation_rules" table gets populated with empty rule_name/rule_value items. This behavior differs from how the "unset" button on the smart-rules.pl page in "Default checkout, hold and return policy" group acts and leads to unpredictable bug when empty "holdallowed" rule_name prevents holds from any user.
This fix removes creation of these empty name/value pairs.

To reproduce:
    1) Start with an empty installation (or delete all circulation rules and press "unset" in hold and return policy section on /cgi-bin/koha/admin/smart-rules.pl page). This is to have the circulation_rules table empty so onboarding.pl will work.
    2) Go through the onboarding.pl process and create a default circulation rule.
    3) Make sure that circulation_rules table contains will contain empty "patron_maxonsiteissueqty", "max_holds", "patron_maxissueqty", "returnbranch", "holdallowed" and "hold_fulfillment_policy" fields.
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that there are none of six mentioned above name/value pairs appeared.

To reproduce the bug with empty "holdallowed" rule_name:
    1) Repeat steps 1, 2 and 3 that are listed in the previous instruction of reproduction, in order to get empty "holdallowed" field.
    2) Find existing or create a new book with available status and place a holding on it.
    3) Observe the yellow notification window with "Cannot place hold. No items are available to be placed on hold." warning.
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that hold was successfully placed and yellow notification does not appear anymore.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 16 Victor Grousset/tuxayo 2020-05-26 04:20:40 UTC
Created attachment 105342 [details] [review]
Bug 25556: fix for unnecessary empty name/value pairs creation

During the web-install onboarding process, when the default circulation rule gets created, "circulation_rules" table gets populated with empty rule_name/rule_value items. This behavior differs from how the "unset" button on the smart-rules.pl page in "Default checkout, hold and return policy" group acts and leads to unpredictable bug when empty "holdallowed" rule_name prevents holds from any user.
This fix removes creation of these empty name/value pairs.

To reproduce:
    1) Start with an empty installation (or delete all circulation rules and press "unset" in hold and return policy section on /cgi-bin/koha/admin/smart-rules.pl page). This is to have the circulation_rules table empty so onboarding.pl will work.
    2) Go through the onboarding.pl process and create a default circulation rule.
    3) Make sure that circulation_rules table contains will contain empty "patron_maxonsiteissueqty", "max_holds", "patron_maxissueqty", "returnbranch", "holdallowed" and "hold_fulfillment_policy" fields.
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that there are none of six mentioned above name/value pairs appeared.

To reproduce the bug with empty "holdallowed" rule_name:
    1) Repeat steps 1, 2 and 3 that are listed in the previous instruction of reproduction, in order to get empty "holdallowed" field.
    2) Find existing or create a new book with available status and place a holding on it.
    3) Observe the yellow notification window with "Cannot place hold. No items are available to be placed on hold." warning.
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that hold was successfully placed and yellow notification does not appear anymore.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Victor Grousset/tuxayo 2020-05-26 04:22:26 UTC
It works! :D

Note for next person testing: the two test plans are doable at the same time. One shows the bug and fix from the data perspective and the other one from the user's.
Comment 18 Jonathan Druart 2020-05-26 07:45:01 UTC
We did not notice the problem before because we have the following code in misc4dev:

        my $params_3 = {
            branchcode => undef,
            itemtype   => undef,
            rules      => {
                holdallowed             => undef,
                hold_fulfillment_policy => undef,
                returnbranch            => undef,
            }
        };

The rules was inserted with undef instead of "".
Comment 19 Jonathan Druart 2020-05-26 07:46:26 UTC
Created attachment 105347 [details] [review]
Bug 25556: fix for unnecessary empty name/value pairs creation

During the web-install onboarding process, when the default circulation rule gets created, "circulation_rules" table gets populated with empty rule_name/rule_value items. This behavior differs from how the "unset" button on the smart-rules.pl page in "Default checkout, hold and return policy" group acts and leads to unpredictable bug when empty "holdallowed" rule_name prevents holds from any user.
This fix removes creation of these empty name/value pairs.

To reproduce:
    1) Start with an empty installation (or delete all circulation rules and press "unset" in hold and return policy section on /cgi-bin/koha/admin/smart-rules.pl page). This is to have the circulation_rules table empty so onboarding.pl will work.
    2) Go through the onboarding.pl process and create a default circulation rule.
    3) Make sure that circulation_rules table contains will contain empty "patron_maxonsiteissueqty", "max_holds", "patron_maxissueqty", "returnbranch", "holdallowed" and "hold_fulfillment_policy" fields.
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that there are none of six mentioned above name/value pairs appeared.

To reproduce the bug with empty "holdallowed" rule_name:
    1) Repeat steps 1, 2 and 3 that are listed in the previous instruction of reproduction, in order to get empty "holdallowed" field.
    2) Find existing or create a new book with available status and place a holding on it.
    3) Observe the yellow notification window with "Cannot place hold. No items are available to be placed on hold." warning.
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that hold was successfully placed and yellow notification does not appear anymore.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Andrii Nugged 2020-05-26 07:50:12 UTC
(In reply to Jonathan Druart from comment #18)
> We did not notice the problem before because we have the following code in
> misc4dev:
>   ...
> The rules was inserted with undef instead of "".

should Peter roll back those erased strings "params_2/3" from the file and assign these with "undef" like in misc4dev so approaches will match for the future?

Or let's leave without "param_3" creation at all as it now (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=25556&attachment=105347)? 

... ah, Passed QA :)... So ok then :P
Comment 21 Jonathan Druart 2020-05-26 07:56:23 UTC
(In reply to Andrew Nugged from comment #20)
> (In reply to Jonathan Druart from comment #18)
> > We did not notice the problem before because we have the following code in
> > misc4dev:
> >   ...
> > The rules was inserted with undef instead of "".
> 
> should Peter roll back those erased strings "params_2/3" from the file and
> assign these with "undef" like in misc4dev so approaches will match for the
> future?
> 
> Or let's leave without "param_3" creation at all as it now
> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.
> html&bug=25556&attachment=105347)? 
> 
> ... ah, Passed QA :)... So ok then :P

Yes, it was just a note :)

I removed them from misc4dev as well.

https://gitlab.com/koha-community/koha-misc4dev/-/commit/ea5655e33cb77928ae0f0fa2c53b4ce594a5b697
Comment 22 Jonathan Druart 2020-05-26 12:45:04 UTC
Pushed to master for 20.05
Comment 23 Aleisha Amohia 2020-06-12 02:05:56 UTC
missing dependencies, not backported to 19.11.x