To recreate: 1 - Visit Admin->Circulation rules 2 - Go to a specific branch 3 - Clear all the boxes in Default checkout, hold and return policy and save 4 - No visible indication that anything is set 5 - Check the DB, (SELECT * FROM circulation_rules WHERE branchcode='BRANCH') 6 - Note there are rules 7 - Unset the rules, no visible change 8 - Check the D, rules are cleared
Nick, they ("" or nothing in DB) means the same, right? If so we should not save them if "".
(In reply to Jonathan Druart from comment #1) > Nick, they ("" or nothing in DB) means the same, right? > > If so we should not save them if "". Not exist in the DB means fall back to other rules - exist in DB as "" means 'Unlimited' Reason to allow: Default for all libraries is 10 Library A wants default as unlimited Library A can simply save the blank rule
Created attachment 119767 [details] [review] Bug 24154: Get rules rather than values to determine if set or not Template plugin CirculationRules.Search returns "" if a rule doesn't exist or if the rule exists but is set to "" To get more info, we add a 'want_rule' parameter and use that to test if defined but blank To test: 1 - In circulation rules set the 'Default checkout, hold and return policy' without setting values 2 - Unset it 3 - Notice there is no visual difference 4 - Apply patch 5 - Reload the page 6 - note there is no 'Unset button' and first column says 'Not set' 7 - Save the rule with no values 8 - Now 'Defaults' is displayed and the unset button exists
Created attachment 119950 [details] [review] Bug 24154: Get rules rather than values to determine if set or not Template plugin CirculationRules.Search returns "" if a rule doesn't exist or if the rule exists but is set to "" To get more info, we add a 'want_rule' parameter and use that to test if defined but blank To test: 1 - In circulation rules set the 'Default checkout, hold and return policy' without setting values 2 - Unset it 3 - Notice there is no visual difference 4 - Apply patch 5 - Reload the page 6 - note there is no 'Unset button' and first column says 'Not set' 7 - Save the rule with no values 8 - Now 'Defaults' is displayed and the unset button exists Signed-off-by: Owen Leonard <oleonard@myacpl.org>
QA: Looking here
I would add a separate method for that. The additional parameter feels hacky.
+ [% SET patron_maxissueqty = CirculationRules.Search( current_branch, undef, undef, 'patron_maxissueqty', 1, 1 ) %] - [% SET patron_maxonsiteissueqty = CirculationRules.Search( current_branch, undef, undef, 'patron_maxonsiteissueqty' ) %] Apart from comment6, we do not need the two 1s. You only added want_rule..
(In reply to Jonathan Druart from comment #6) > I would add a separate method for that. The additional parameter feels hacky. Or I would propose at least something like: additional_parameters instead of want_rule and check for additional_parameters->{want_rule} and pass { want_rule => 1 } instead of only 1 at the end Which is far more explanatory
[% SET dcharp = ( patron_maxissueqty || patron_maxonsiteissueqty || rule_value || holdallowed || hold_fulfillment_policy || returnbranch ) %] What means dcharp ? Since it is not clear rightaway, I would suggest a better variable name here. Maybe it's just me ?
We need a few tests too indeed. Moving to FQA for now
Created attachment 120047 [details] [review] Bug 24154: (follow-up) Make parameter and variable more explicit, add tests
+ [% SET patron_maxonsiteissueqty = CirculationRules.Search( current_branch, undef, undef, 'patron_maxonsiteissueqty', { want_rule = 1 } ) %] You probably mean: want_rule => 1 ?
Created attachment 120561 [details] [review] Bug 24154: Adjust assignment The code seems to work either way We have a mix in our code: git grep "}) %]" Using '=>' over '=' does look nicer
Created attachment 120741 [details] [review] Bug 24154: Get rules rather than values to determine if set or not Template plugin CirculationRules.Search returns "" if a rule doesn't exist or if the rule exists but is set to "" To get more info, we add a 'want_rule' parameter and use that to test if defined but blank To test: 1 - In circulation rules set the 'Default checkout, hold and return policy' without setting values 2 - Unset it 3 - Notice there is no visual difference 4 - Apply patch 5 - Reload the page 6 - note there is no 'Unset button' and first column says 'Not set' 7 - Save the rule with no values 8 - Now 'Defaults' is displayed and the unset button exists Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120742 [details] [review] Bug 24154: (follow-up) Make parameter and variable more explicit, add tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120743 [details] [review] Bug 24154: Adjust assignment The code seems to work either way We have a mix in our code: git grep "}) %]" Using '=>' over '=' does look nicer Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 21.05, thanks to everybody involved!
Does not apply on 20.11.x. It depends on Bug 27069 no ?