Bug 30794

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.

*** Bug 29061 has been marked as a duplicate of this bug. ***

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).

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.

(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.

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

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)

Could you test this patch?

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

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>

QA: Looking here

> 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?

(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 !

I had something similar the other day with open_article_requests_limit. See bug 27945

Should we open a new bug report for that? Or is it already somewhere?

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>

(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.

Pushed to master for 22.11.

Nice work everyone, thanks!