Summary: | Holds blocked when empty holdallowed value present in circulation_rules | ||
---|---|---|---|
Product: | Koha | Reporter: | Andrii Nugged <nugged> |
Component: | Hold requests | Assignee: | 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 | ||
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: | 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
No, undef should be the same as no rule, a default value could apply. "" means unlimited in some case. 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 '') { 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? Sorry, looking at the wrong rule. You said "holdallowed"! 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? > 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.
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. 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". At first glance I'd say we should remove $params_2 and $params_3 from installer/onboarding.pl. I agree. That was there because it was aimed for old "issuingrules" table, nowadays we have circulation_rules. 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 :). (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 (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. 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! 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> 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> 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. 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 "". 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> (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 (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 Pushed to master for 20.05 missing dependencies, not backported to 19.11.x |