[Bug 24154] New: No indication that Default checkout, hold and return policy are set if values are blank
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Bug ID: 24154 Summary: No indication that Default checkout, hold and return policy are set if values are blank Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kelly@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Nick, they ("" or nothing in DB) means the same, right? If so we should not save them if "". -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 119767 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119767&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119767|0 |1 is obsolete| | --- Comment #4 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 119950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119950&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED CC| |m.de.rooy@rijksmuseum.nl --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I would add a separate method for that. The additional parameter feels hacky. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- + [% 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- [% 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- We need a few tests too indeed. Moving to FQA for now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 120047 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120047&action=edit Bug 24154: (follow-up) Make parameter and variable more explicit, add tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- + [% SET patron_maxonsiteissueqty = CirculationRules.Search( current_branch, undef, undef, 'patron_maxonsiteissueqty', { want_rule = 1 } ) %] You probably mean: want_rule => 1 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 120561 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120561&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119950|0 |1 is obsolete| | --- Comment #14 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 120741 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120741&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120047|0 |1 is obsolete| | --- Comment #15 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 120742 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120742&action=edit Bug 24154: (follow-up) Make parameter and variable more explicit, add tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #120561|0 |1 is obsolete| | --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 120743 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120743&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24154 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #18 from Fridolin Somers <fridolin.somers@biblibre.com> --- Does not apply on 20.11.x. It depends on Bug 27069 no ? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org