Bug 12197 - Exceeding the maxreserves preference does not prevent librarian from placing the hold
Summary: Exceeding the maxreserves preference does not prevent librarian from placing ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 4045 11250
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-06 15:31 UTC by Kyle M Hall
Modified: 2016-06-21 21:40 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12197 - Exceeding the maxreserves preference does not prevent librarian from placing the hold (1.43 KB, patch)
2014-05-06 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12197: enforce the maxreserves preference when staff members place hold requests (1.57 KB, patch)
2014-05-06 15:56 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 12197: (follow-up) rename variable for greater clarity (3.69 KB, patch)
2014-05-06 15:56 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 12197: enforce the maxreserves preference when staff members place hold requests (9.26 KB, patch)
2014-05-27 14:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12197: enforce the maxreserves preference when staff members place hold requests (8.99 KB, patch)
2015-03-17 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12197: enforce the maxreserves preference when staff members place hold requests (9.01 KB, patch)
2015-09-25 12:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12197: enforce the maxreserves preference when staff members place hold requests (9.51 KB, patch)
2015-09-28 16:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12197: enforce the maxreserves preference when staff members place hold requests (9.58 KB, patch)
2015-10-22 11:01 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 Kyle M Hall 2014-05-06 15:31:49 UTC
Create 3 items to place holds on. Set the circulation rule to allow 50 holds for all items. Set maxreserves to 2. Place a hold on 3 different items. On the third item, it will give a warning, but you can still place the hold. Despite what the circulation rule is set for (which is only a specific case rule), maxreserves is a global rule and should stop this from happening, not just giving a warning.
Comment 1 Kyle M Hall 2014-05-06 15:33:50 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2014-05-06 15:56:02 UTC Comment hidden (obsolete)
Comment 3 Galen Charlton 2014-05-06 15:56:06 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2014-05-06 15:58:09 UTC
Kyle, note that AllowHoldPolicyOverride *does* let the operator override the global maxreserves.   I think this is a reasonable interpretation of AllowHoldPolicyOverride, but if you disagree, that probably belongs on a separate bug.
Comment 5 Galen Charlton 2014-05-06 17:28:23 UTC
Setting status back to signed off.  Please do not change the status to failed QA without providing an explanation.
Comment 6 Christopher Brannon 2014-05-06 17:35:28 UTC
Galen,
I did provide an explanation, but it collided with your post.  :/

Patch works on single holds, but fails when placing multiple holds.

* Create a list or do a search which results in multiple items.
* Following the above test plan, check 3 items to place a hold on and place the hold.
* No warning is issued, and all holds are placed.

Christopher
Comment 7 Kyle M Hall 2014-05-27 14:22:15 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2014-06-16 12:47:25 UTC
Could you rebase the patch please ? I fail to apply it on master:

Applying: Bug 12197: enforce the maxreserves preference when staff members place hold requests
fatal: sha1 information is lacking or useless (reserve/request.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 9 Kyle M Hall 2014-10-27 14:07:32 UTC
Patch applies cleanly for me! Please try again. Maybe you missed the dependency that has now been pushed to master?

Kyle

(In reply to Julian Maurice from comment #8)
> Could you rebase the patch please ? I fail to apply it on master:
> 
> Applying: Bug 12197: enforce the maxreserves preference when staff members
> place hold requests
> fatal: sha1 information is lacking or useless (reserve/request.pl).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
Comment 10 Katrin Fischer 2014-11-17 21:48:58 UTC
Hi Kyle, patch doesn't apply for me either :(

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12197: enforce the maxreserves preference when staff members place hold requests
fatal: sha1 information is lacking or useless (reserve/request.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12197: enforce the maxreserves preference when staff members place hold requests
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.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-12197-enforce-the-maxreserves-preference-when--f87Izr.patch
Comment 11 Kyle M Hall 2015-03-17 16:08:45 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-03-18 11:43:01 UTC
Before the patch, I got

Cannot place hold
    Too many holds: Joubu has too many holds.

After:

Cannot place hold
    Too many holds: Joubu can only place a maximum of 3 total holds.
    No items are available to be placed on hold

I don't understand why the second message appears.
Comment 13 Katrin Fischer 2015-03-22 20:38:20 UTC
Kyle, can you please look into Jonathan's last comment?
Comment 14 Kyle M Hall 2015-09-25 12:19:58 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-09-25 12:21:42 UTC
(In reply to Jonathan Druart from comment #12)
> Before the patch, I got
> 
> Cannot place hold
>     Too many holds: Joubu has too many holds.
> 
> After:
> 
> Cannot place hold
>     Too many holds: Joubu can only place a maximum of 3 total holds.
>     No items are available to be placed on hold
> 
> I don't understand why the second message appears.

Because it is technically correct. The patron cannot place the hold because the patron has too many holds, and there are no items available because the check for item holdability fails each time because the patron has too many holds. This issue is out of scope for this bug. Bug 11250 attempts to deal with this issue.
Comment 16 Jonathan Druart 2015-09-28 13:39:39 UTC
(In reply to Kyle M Hall from comment #15)
> (In reply to Jonathan Druart from comment #12)
> > Before the patch, I got
> > 
> > Cannot place hold
> >     Too many holds: Joubu has too many holds.
> > 
> > After:
> > 
> > Cannot place hold
> >     Too many holds: Joubu can only place a maximum of 3 total holds.
> >     No items are available to be placed on hold
> > 
> > I don't understand why the second message appears.
> 
> Because it is technically correct. The patron cannot place the hold because
> the patron has too many holds, and there are no items available because the
> check for item holdability fails each time because the patron has too many
> holds. This issue is out of scope for this bug. Bug 11250 attempts to deal
> with this issue.

So bug 11250 and this one should be pushed together, that's it?
Comment 17 Kyle M Hall 2015-09-28 13:53:01 UTC
> So bug 11250 and this one should be pushed together, that's it?

Yes, I would agree with that.
Comment 18 Jonathan Druart 2015-09-28 14:01:11 UTC
(In reply to Kyle M Hall from comment #17)
> > So bug 11250 and this one should be pushed together, that's it?
> 
> Yes, I would agree with that.

They are in conflict, could you provide dependent patches?
Comment 19 Kyle M Hall 2015-09-28 16:40:04 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-10-22 11:01:08 UTC
Created attachment 43749 [details] [review]
Bug 12197: enforce the maxreserves preference when staff members place hold requests

This patch ensures that the global maxreserves preference is enforced
when staff members place hold requests.

For example:

Create 3 items to place holds on. Set the circulation rule to allow 50
holds for all items. Set maxreserves to 2. Place a hold on 3 different
items. On the third item, it will give a warning, but you can still
place the hold. Despite what the circulation rule is set for (which is
only a specific case rule), maxreserves is a global rule and
should stop this from happening, not just give a warning.

Test Plan:
1) Reproduce the bug by following the steps above
2) Verify the bug exists
3) Apply this patch
4) Verify the librarian cannot place the hold now
5) Enable AllowHoldPolicyOverride
6) Verify the librarian can forcefully place the hold

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

Bug 12197: (follow-up) rename variable for greater clarity

"maxreserves" was referring both to the system preference and to the
condition of having exceeded the number of hold requests allowed.

This patch renames a variable to remove the ambguity.

Test plan:

* Same as the main patch.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Tomás Cohen Arazi 2015-10-22 13:48:35 UTC
Patch pushed to master.

Thanks Kyle!