|
Description
Eivin Giske Skaaren
2015-09-21 19:23:07 UTC
Created attachment 42751 [details] [review] Bug 14866: Make high holds work with different item types This patch adds the possibility to set decreased loan periods for high holds items in issuingrules via smart-rules.pl. If a rule exist with a value set for a high holds item and the decreaseLoanHighHolds syspref is active then the value of the rule will be used instead of the syspref value. To test: 1. Create testbranch, dump mysql as needed.. 2. Apply patch 3. Update database perl installer/data/mysql/updatedatabase.pl misc/devel/update_dbix_class_files.pl --db_name=koha_instance --db_user=koha_instance --db_passwd=thepassword 4. Enable decreaseLoanHighHolds sysprefs if not already active 5. Create rule for library/item in http://your_staff_client/cgi-bin/koha/admin/smart-rules.pl Set Decrease loan holds to your wanted value. 6. Check out an item with holds and see that the value from the rule is used instead of the syspref - Was this patch ready for signing off? - It does no longer apply on current master. Yes I guess it was ready back in 2015.. I have not looked into what needs to be done now but I will contact Halland county library about if they want a rebase/rewrite now. Are you interested in signing-off for this patch if it is fixed? (In reply to Eivin Giske Skaaren from comment #3) > Yes I guess it was ready back in 2015.. > I have not looked into what needs to be done now but I will contact Halland > county library about if they want a rebase/rewrite now. > > Are you interested in signing-off for this patch if it is fixed? Yes. Created attachment 58324 [details] [review] [FOLLOW-UP] Bug 14866: Make high holds work with different item types This patch fixes merge conflicts, fixes the indentation of the table in smart-rules.tt and uses Koha::IssuingRules->get_effective_issuing_rule. Keep in mind - I am sure that when the original patch for this was written (before get_effective_issuing_rule was used), it all worked fine. I had to add an extra line of code in my patch to get this method to work because it was fetching the WRONG circulation rule - one that did not have decreaseloanholds defined. I don't know if it was my patch that is broken or the get_effective_issuing_rule method that is not fetching the 'most specific/relevant' rule. Sponsored-by: Region Halland Patch no longer applies, can you please rebase? It also seems to be missing tests? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 14866: Make high holds work with different item types .git/rebase-apply/patch:80: trailing whitespace. <td>[% rule.decreaseloanholds %]</td> warning: 1 line adds whitespace errors. Using index info to reconstruct a base tree... M C4/Circulation.pm M admin/smart-rules.pl M koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt .git/rebase-apply/patch:80: trailing whitespace. <td>[% rule.decreaseloanholds %]</td> warning: 1 line applied after fixing whitespace errors. Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt Auto-merging admin/smart-rules.pl CONFLICT (content): Merge conflict in admin/smart-rules.pl Auto-merging C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 14866: Make high holds work with different item types The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-14866-Make-high-holds-work-with-different-item-RnFis7.patch Created attachment 75861 [details] [review] Bug 14866: Make high holds work with different item types This patch adds the possibility to set decreased loan periods for high holds items in issuingrules via smart-rules.pl. If a rule exist with a value set for a high holds item and the decreaseLoanHighHolds syspref is active then the value of the rule will be used instead of the syspref value. To test: 1. Create testbranch, dump mysql as needed.. 2. Apply patch 3. Update database perl installer/data/mysql/updatedatabase.pl misc/devel/update_dbix_class_files.pl --db_name=koha_instance --db_user=koha_instance --db_passwd=thepassword 4. Enable decreaseLoanHighHolds sysprefs if not already active 5. Create rule for library/item in http://your_staff_client/cgi-bin/koha/admin/smart-rules.pl Set Decrease loan holds to your wanted value. 6. Check out an item with holds and see that the value from the rule is used instead of the syspref Created attachment 75862 [details] [review] Bug 14866: Make high holds work with different item types This patch fixes merge conflicts, fixes the indentation of the table in smart-rules.tt and uses Koha::IssuingRules->get_effective_issuing_rule. Keep in mind - I am sure that when the original patch for this was written (before get_effective_issuing_rule was used), it all worked fine. I had to add an extra line of code in my patch to get this method to work because it was fetching the WRONG circulation rule - one that did not have decreaseloanholds defined. I don't know if it was my patch that is broken or the get_effective_issuing_rule method that is not fetching the 'most specific/relevant' rule. Sponsored-by: Region Halland Created attachment 75863 [details] [review] Bug 14866: [FOLLOW-UP] Make high holds work with different item types This patch fixes merge conflicts, fixes the indentation of the table in smart-rules.tt and uses Koha::IssuingRules->get_effective_issuing_rule. Keep in mind - I am sure that when the original patch for this was written (before get_effective_issuing_rule was used), it all worked fine. I had to add an extra line of code in my patch to get this method to work because it was fetching the WRONG circulation rule - one that did not have decreaseloanholds defined. I don't know if it was my patch that is broken or the get_effective_issuing_rule method that is not fetching the 'most specific/relevant' rule. Sponsored-by: Region Halland Patch doesn't apply Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 14866: Make high holds work with different item types .git/rebase-apply/patch:88: space before tab in indent. </tr> .git/rebase-apply/patch:89: space before tab in indent. [% END %] warning: 2 lines add whitespace errors. Using index info to reconstruct a base tree... M C4/Circulation.pm M admin/smart-rules.pl M koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt .git/rebase-apply/patch:88: space before tab in indent. </tr> .git/rebase-apply/patch:89: space before tab in indent. [% END %] warning: 2 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt Auto-merging admin/smart-rules.pl CONFLICT (content): Merge conflict in admin/smart-rules.pl Auto-merging C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 14866: Make high holds work with different item types Created attachment 103323 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT Created attachment 103427 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT It works, but at the top of smart-rules.pl between the library selector and the table there is a bunch of this: <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types <<<<<<< HEAD ======= >>>>>>> I also wasn't able to confirm tests because I was testing on a sandbox. Lisette I've been informed that the <<<<<<< HEAD ======= >>>>>>> Bug 14866: Make high holds work with different item types is a sign that the patch doesn't cleanly apply, so I'm changing the status to patch doesn't apply. Created attachment 110149 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT Created attachment 110224 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Worked great! Once again, I wasn't able to confirm tests because I was testing on a sandbox. Lisette The new rule is named "Decrease loan holds (day)" in the circulation rules. I have some difficulty translating this in my head, even tho I know what is meant. Could we improve the phrasing maybe? Decreased loan period for high holds (day) ? And instead of placing it last, we could place it after the "hard due date" so it's closer to the other loan period parameters. What do you think? Created attachment 110441 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Hi Aleisha, as the requested changes were only "cosmetic" I'd be ok with keeping this signed off, but was hoping for a separate patch as a follow-up. Oops sorry I should've thought about that! Understand if this needs to get signed off again. Hi Aleisha and Katrin, I'm a bit confused by the last two comments. Does this need to be signed off again? If so, can we test and sign off the current patch or should we wait for a new one? (In reply to Henry Bolshaw from comment #22) > Hi Aleisha and Katrin, I'm a bit confused by the last two comments. Does > this need to be signed off again? If so, can we test and sign off the > current patch or should we wait for a new one? Sorry for the confusion, I amended the patch to include the cosmetic changes that Katrin asked for so there is no new patch coming. Since I didn't include it as a follow-up like I should've this will need to be tested and signed off again. Created attachment 111937 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Current status: Signed Off Created attachment 111938 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Thanks for clarifying Aleisha. I've tested and signed this off now. Apologies for the duplicate sign offs, I was trying to sign off using git bz but couldn't work out how to do it properly. Created attachment 112225 [details] [review] Bug 14866: Make high holds work with different item types To test: 1) Apply patch, update database, restart services 2) Enable decreaseLoanHighHolds sysprefs if not already active 3) Go to circulation rules and set a value under 'Decrease loan holds (day)' that is DIFFERENT from decreaseLoanHighHoldsDuration 4) Check out an item with holds and confirm that the value from the rule is used instead of the system preference 5) Confirm tests pass before and after the patch t/db_dependent/DecreaseLoanHighHolds.t Sponsored-by: Catalyst IT Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 112226 [details] [review] Bug 14866: (QA follow-up) Add hint to system preferences Adds a hint about the new configuration option to the existing system preference sectoin. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Please don't forget to describe what the new enhancement is supposed to add when you write the commit message. The title and test plan are, most of the time, not enough. Created attachment 112936 [details] [review] Bug 14866: Trivial code simplification As we are inserting the value into the default circulation rule, why do we keep the syspref? It's a fallback when no other value is set in the circulation conditions:
+ if ( defined($rule) && $rule->rule_value ne '' ){
+ # overrides decreaseLoanHighHoldsDuration syspref
...
Yes, but we insert the rule during the update process. If we create the rule with the value in the syspref I think we could remove the it. Is it worth it? (In reply to Jonathan Druart from comment #33) > Yes, but we insert the rule during the update process. > > If we create the rule with the value in the syspref I think we could remove > the it. Is it worth it? Hm ist could work, but I haven't fully worked out the structure yet, but I think there is a functional difference with your suggestion and the current implementation: The * * * rule shoudl only be applied if there is no rule that is more specific. So, when you have: itemtype a - patroncategory b ... high holds value left empty. Current suggestion: - the pref value will be used as a fallback. Your suggestion: - I am not sure. I believe that it won't fall back but use the "empty" and that it will need to have some meaning, like "0" or "inactive"? Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x |