Bug 14866 - Make high holds work with different item types and number of open days
Summary: Make high holds work with different item types and number of open days
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15523
  Show dependency treegraph
 
Reported: 2015-09-21 19:23 UTC by Eivin Giske Skaaren
Modified: 2021-06-14 21:33 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch adds a new circulation rule - decreaseloanholds - which will override the value set in the decreaseLoanHighHoldsDuration system preference for specific item types, or patron categories, or branches.
Version(s) released in:
20.11.00


Attachments
Bug 14866: Make high holds work with different item types (6.29 KB, patch)
2015-09-21 21:55 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
[FOLLOW-UP] Bug 14866: Make high holds work with different item types (17.39 KB, patch)
2016-12-21 00:31 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (6.50 KB, patch)
2018-06-07 03:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (20.84 KB, patch)
2018-06-07 03:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: [FOLLOW-UP] Make high holds work with different item types (22.14 KB, patch)
2018-06-07 03:10 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (10.92 KB, patch)
2020-04-21 06:07 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (10.91 KB, patch)
2020-04-21 22:55 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (12.37 KB, patch)
2020-09-16 03:51 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (12.43 KB, patch)
2020-09-16 16:54 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (12.33 KB, patch)
2020-09-20 22:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (12.38 KB, patch)
2020-10-16 23:04 UTC, Henry Bolshaw
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (12.38 KB, patch)
2020-10-16 23:05 UTC, Henry Bolshaw
Details | Diff | Splinter Review
Bug 14866: Make high holds work with different item types (12.44 KB, patch)
2020-10-22 23:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14866: (QA follow-up) Add hint to system preferences (1.21 KB, patch)
2020-10-22 23:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14866: Trivial code simplification (1.75 KB, patch)
2020-11-03 16:03 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eivin Giske Skaaren 2015-09-21 19:23:07 UTC
The decreaseLoanHighHolds syspref is global and it would be better to be able to set this per item, perhaps as an override to the global syspref.

Also the syspref does not work as intended when a library has fewer open days per week. With a shortened loan period set to 10 days a library with 2 open days per week gets the loan period set to 10 actual days = 5 weeks.


Sponsored-by: Halland county library
Comment 1 Eivin Giske Skaaren 2015-09-21 21:55:14 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
Comment 2 Marc Véron 2016-09-29 15:19:21 UTC
- Was this patch ready for signing off?

- It does no longer apply on current master.
Comment 3 Eivin Giske Skaaren 2016-09-29 19:27:15 UTC
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?
Comment 4 Marc Véron 2016-09-29 20:32:42 UTC
(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.
Comment 5 Aleisha Amohia 2016-12-21 00:31:13 UTC
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
Comment 6 Katrin Fischer 2017-10-08 09:46:23 UTC
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
Comment 7 Aleisha Amohia 2018-06-07 03:05:25 UTC
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
Comment 8 Aleisha Amohia 2018-06-07 03:05:43 UTC
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
Comment 9 Aleisha Amohia 2018-06-07 03:10:11 UTC
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
Comment 10 Maryse Simard 2019-02-15 23:11:42 UTC
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
Comment 11 Aleisha Amohia 2020-04-21 06:07:06 UTC
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
Comment 12 Aleisha Amohia 2020-04-21 22:55:52 UTC
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
Comment 13 Lisette Scheer 2020-08-18 19:20:50 UTC
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
Comment 14 Lisette Scheer 2020-08-18 20:23:31 UTC
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.
Comment 15 Aleisha Amohia 2020-09-16 03:51:02 UTC
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
Comment 16 ByWater Sandboxes 2020-09-16 16:54:29 UTC
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>
Comment 17 Lisette Scheer 2020-09-16 16:55:34 UTC
Worked great! Once again, I wasn't able to confirm tests because I was testing on a sandbox. 
Lisette
Comment 18 Katrin Fischer 2020-09-20 19:33:54 UTC
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?
Comment 19 Aleisha Amohia 2020-09-20 22:13:38 UTC
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>
Comment 20 Katrin Fischer 2020-09-20 22:19:19 UTC
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.
Comment 21 Aleisha Amohia 2020-09-20 22:23:29 UTC
Oops sorry I should've thought about that! Understand if this needs to get signed off again.
Comment 22 Henry Bolshaw 2020-10-13 19:58:06 UTC
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?
Comment 23 Aleisha Amohia 2020-10-13 20:21:44 UTC
(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.
Comment 24 Henry Bolshaw 2020-10-16 23:04:11 UTC
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
Comment 25 Henry Bolshaw 2020-10-16 23:05:54 UTC
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>
Comment 26 Henry Bolshaw 2020-10-16 23:08:22 UTC
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.
Comment 27 Katrin Fischer 2020-10-22 23:43:04 UTC
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>
Comment 28 Katrin Fischer 2020-10-22 23:43:10 UTC
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>
Comment 29 Jonathan Druart 2020-11-03 15:49:47 UTC
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.
Comment 30 Jonathan Druart 2020-11-03 16:03:43 UTC
Created attachment 112936 [details] [review]
Bug 14866: Trivial code simplification
Comment 31 Jonathan Druart 2020-11-04 08:22:24 UTC
As we are inserting the value into the default circulation rule, why do we keep the syspref?
Comment 32 Katrin Fischer 2020-11-04 08:30:40 UTC
It's a fallback when no other value is set in the circulation conditions:

+        if ( defined($rule) && $rule->rule_value ne '' ){
+            # overrides decreaseLoanHighHoldsDuration syspref
...
Comment 33 Jonathan Druart 2020-11-04 10:11:53 UTC
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?
Comment 34 Katrin Fischer 2020-11-04 11:28:08 UTC
(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"?
Comment 35 Jonathan Druart 2020-11-06 14:55:49 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 36 Lucas Gass 2020-11-16 22:27:32 UTC
enhancement will not be backported to 20.05.x