Bug 30794 - 'Default checkout, hold and return policy' overrides Unlimited holds in 'Default checkout, hold policy by patron category'
Summary: 'Default checkout, hold and return policy' overrides Unlimited holds in 'Defa...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords: rel_22_11_candidate
: 29061 (view as bug list)
Depends on:
Blocks: 32262
  Show dependency treegraph
 
Reported: 2022-05-17 22:00 UTC by Sara Brown
Modified: 2023-06-08 22:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 30794: Insert blank rule (unlimited) for max_holds (1.11 KB, patch)
2022-11-17 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30794: Insert blank rule (unlimited) for max_holds (1.17 KB, patch)
2022-11-17 16:17 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30794: Insert blank rule (unlimited) for max_holds (1.26 KB, patch)
2022-11-18 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sara Brown 2022-05-17 22:00:43 UTC
If holds for a patron category are set to Unlimited in 'Default checkout, hold policy by patron category' but a holds limit exists in 'Default checkout, hold and return policy', the latter will apply. To recreate:

-Set max holds at 5 for Patron Category A in 'Default checkout, hold policy by patron category'
-Set max holds at 2 in 'Default checkout, hold and return policy'
-Attempt to place multiple holds for a patron in category A - you will be stopped on the 6th hold.
-Change max holds to Unlimited for Patron Category A in 'Default checkout, hold policy by patron category'
-Keep 'Default checkout, hold and return policy' at 2 max holds
-Attempt to place multiple holds for that patron - you'll be stopped after the 2nd hold

(In the test above, the circulation matrix has an Patron Category A/All item types max of 10 holds.)

This appears to apply in 21.05 and 21.11.
Comment 1 Katrin Fischer 2022-07-16 18:39:15 UTC
This is valid in master as well:

Default checkout, hold and return policy
Max holds: 1

Default checkout, hold policy by patron category:
Staff: Max holds: Unlimited

You can't place a second hold. Only the first rule is applied, although the GUI says that the patron category rule will be used.

This shows that the "Unlimited" rule is actually not seen/ignored.

In circulation rules we have:

| 34 | NULL  | NULL  | NULL     | patron_maxissueqty               |      |
| 35 | NULL  | NULL  | NULL     | patron_maxonsiteissueqty         |      |
| 36 | NULL  | NULL  | NULL     | max_holds                        | 1    |
| 31 | NULL  | S     | NULL     | patron_maxonsiteissueqty         | 80   |
| 33 | NULL  | S     | NULL     | patron_maxissueqty               | 80   |

The line for max_holds and Staff (S) is missing.
Comment 2 Katrin Fischer 2022-07-16 18:41:15 UTC
*** Bug 29061 has been marked as a duplicate of this bug. ***
Comment 3 Andrew Fuerste-Henry 2022-11-16 17:49:16 UTC
I'm not sure if this is a distinct bug or a wrinkle within this one. Sara and Katrin's comments show that when the staff interface says a category has Unlimited holds that rule doesn't really get saved in the database. We have a slightly different scenario.

Our Default checkout, hold and return policy sets unlimited checkouts and 15 holds for all patrons. I want to make our staff category have unlimited checkouts and unlimited holds. That should mean saving an entry in Default checkout, hold policy by patron category setting blank values for both rules. Koha will not save that at all, it just doesn't create a line in that table.

If I cheat and save a line with no value in total checkouts, a (meaningless for us) value in onsite checkouts, and no value in holds, then it saves and tells me that both checkouts and holds will be unlimited (but then I run into what Sara and Katrin have documented, the rule doesn't *really* exist).
Comment 4 Katrin Fischer 2022-11-16 18:08:34 UTC
I am tempted to update severity on this one - circulation rules are complex as is and they should do what they say in the GUI.
Comment 5 Andrew Fuerste-Henry 2022-11-16 18:18:06 UTC
(In reply to Katrin Fischer from comment #4)
> I am tempted to update severity on this one - circulation rules are complex
> as is and they should do what they say in the GUI.

Yes, let's. Making this Major.
Comment 6 Jonathan Druart 2022-11-17 08:28:33 UTC
Related to bug 24669, why didn't we use strip_non_numeric for one occurrence?
Comment 7 Jonathan Druart 2022-11-17 08:30:50 UTC
Created attachment 143984 [details] [review]
Bug 30794: Insert blank rule (unlimited) for max_holds

Related to bug 24669, why didn't we use strip_non_numeric for one occurrence?

Note that there is still a problem, if none of the 3 inputs is filled,
no rule will be created (ie. action ignored)
Comment 8 Jonathan Druart 2022-11-17 08:31:11 UTC
Could you test this patch?
Comment 9 Andrew Fuerste-Henry 2022-11-17 16:16:24 UTC
Test plan:

1: have an All/All rule in the circ matrix that allows unlimited holds
2: set Default checkout, hold, and return policy to maximum 1 total hold allowed
3: set Default checkout, hold policy by patron category to allow Board patrons to have unlimited holds (remember you can't just save a blank set of rules here, so you'll want to put a number into Total Checkouts, nothing in Total Holds, and save that -- you'll end up with "Unlimited" showing in Total Holds).
4: Place a hold for a Board patron. It works!
5: Try to place a second hold for the same Board patron. It tells you you're over the limit, "[patron] can only place a maximum of 1 total holds."
6: In reports, select * from circulation_rules where rule_name='max_holds', see that your Board patron rule didn't get created

APPLY PATCH

7: Back in circ rules, delete and re-create your Default checkout, hold policy by patron category rule for Board patrons
8: Try your second hold again. It works!
9: Run your report again, see you now have a rule where categorycode=B, rule_name=max_holds, rule_value is null
Comment 10 ByWater Sandboxes 2022-11-17 16:17:02 UTC
Created attachment 144011 [details] [review]
Bug 30794: Insert blank rule (unlimited) for max_holds

Related to bug 24669, why didn't we use strip_non_numeric for one occurrence?

Note that there is still a problem, if none of the 3 inputs is filled,
no rule will be created (ie. action ignored)

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 11 Marcel de Rooy 2022-11-18 09:27:00 UTC
QA: Looking here
Comment 12 Marcel de Rooy 2022-11-18 09:40:53 UTC
> Note that there is still a problem, if none of the 3 inputs is filled,
> no rule will be created (ie. action ignored)

|  99 | NULL       | NULL         | NULL     | patron_maxissueqty               |            |
| 100 | NULL       | NULL         | NULL     | patron_maxonsiteissueqty         |            |
| 101 | NULL       | NULL         | NULL     | max_holds                        |            |
| 102 | NULL       | B            | NULL     | patron_maxonsiteissueqty         |            |
| 103 | NULL       | B            | NULL     | patron_maxissueqty               |            |
| 104 | NULL       | B            | NULL     | max_holds                        |            |

Looks like the rule is created though?
Comment 13 Marcel de Rooy 2022-11-18 09:41:48 UTC
(In reply to Marcel de Rooy from comment #12)
> > Note that there is still a problem, if none of the 3 inputs is filled,
> > no rule will be created (ie. action ignored)
> 
> Looks like the rule is created though?

Ah. The rule is there but it is not shown on the interface !
Comment 14 Marcel de Rooy 2022-11-18 09:42:38 UTC
I had something similar the other day with open_article_requests_limit. See bug 27945
Comment 15 Marcel de Rooy 2022-11-18 09:45:20 UTC
Should we open a new bug report for that? Or is it already somewhere?
Comment 16 Marcel de Rooy 2022-11-18 09:46:29 UTC
Created attachment 144050 [details] [review]
Bug 30794: Insert blank rule (unlimited) for max_holds

Related to bug 24669, why didn't we use strip_non_numeric for one occurrence?

Note that there is still a problem, if none of the 3 inputs is filled,
no rule will be created (ie. action ignored)

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Jonathan Druart 2022-11-18 09:59:25 UTC
(In reply to Marcel de Rooy from comment #15)
> Should we open a new bug report for that? Or is it already somewhere?

I haven't created it.
Comment 18 Tomás Cohen Arazi 2022-11-22 12:31:04 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!