Bug 29750 - Reserves.pm should check patron category hold policies when finding a reserve for an item
Summary: Reserves.pm should check patron category hold policies when finding a reserve...
Status: RESOLVED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-22 04:07 UTC by Alex Buckley
Modified: 2023-10-30 19:32 UTC (History)
5 users (show)

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


Attachments
Bug 29750: Reserves.pm should check if a patron is eligible to reserve and item before confirming the reserve (2.47 KB, patch)
2021-12-22 04:23 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 29750: Unit test fixes for Reserves.pm (5.56 KB, patch)
2021-12-22 04:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 29750: Reserves.pm should check if a patron is eligible to reserve and item before confirming the reserve (1.96 KB, patch)
2023-08-01 23:21 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 29750: Unit test fixes for Reserves.pm (5.57 KB, patch)
2023-08-01 23:21 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 29750: Reserves.pm should check if a patron is eligible to reserve and item before confirming the reserve (2.00 KB, patch)
2023-08-02 14:51 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 29750: Unit test fixes for Reserves.pm (5.61 KB, patch)
2023-08-02 14:52 UTC, Sam Lau
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2021-12-22 04:07:06 UTC
Bug 23172 introduced a check for patron categories hold policies when the holds queue maps items to pending reserves. 

This patchset will introduce a check for patron categories hold policies when finding a reserve for an item. This is useful when the holds queue has not been generated.
Comment 1 Alex Buckley 2021-12-22 04:23:37 UTC
Created attachment 128858 [details] [review]
Bug 29750: Reserves.pm should check if a patron is eligible to reserve and item before confirming the reserve

Test plan:
1. Create two patrons, with the following patron categories:
- Patron A -> Patron
- Patron B -> Student
2. Create two items attached to a single biblio record:
- Book
- Continuing resource
3. Create the following circ rules:
Patron category, item type, Holds allowed (total), Holds allowed
(daily), Holds per record (count)
- Patron, Book, 0, 0, 0
- Student, Book, 1, 1, 1
- All, Continuing resources, 1, 1, 1
4. Place a title level hold on the biblio for patron A
5. Place a title level hold on the biblio for patron B
6. Checkin the Book and notice you are prompted to confirm the hold
by Patron A - even though according to circ rules they cannot reserve Books
7. Apply patch and restart services
9. Checkin the Book, and notice you're prompted to confirm the hold
by Patron B

Sponsored-By: Brimbank Library, Australia
Comment 2 Alex Buckley 2021-12-22 04:25:51 UTC
Created attachment 128859 [details] [review]
Bug 29750: Unit test fixes for Reserves.pm

Test plan:
1. sudo koha-shell <instance>
2.
prove t/db_dependent/Holds.t
prove t/db_dependent/Reserves.t

Sponsored-By: Brimbank Library, Australia
Comment 3 Alex Buckley 2021-12-22 04:27:45 UTC
I have attached the following fixed unit tests:
- t/db_dependent/Holds.t
- t/db_dependent/Reserves.t

I still have to fix these failing unit test files:
- t/db_dependent/Circulation.t
- t/db_dependent/Holds/HoldItemtypeLimit.t
- t/db_dependent/Holds/LocalHoldsPriority.t
Comment 4 Katrin Fischer 2023-06-25 09:37:24 UTC
Hi Alex, should this have a different status?
Comment 5 Alex Buckley 2023-08-01 23:21:18 UTC
Created attachment 154140 [details] [review]
Bug 29750: Reserves.pm should check if a patron is eligible to reserve and item before confirming the reserve

Test plan:
1. Create two patrons, with the following patron categories:
- Patron A -> Patron
- Patron B -> Student
2. Create two items attached to a single biblio record:
- Book
- Continuing resource
3. Create the following circ rules:
Patron category, item type, Holds allowed (total), Holds allowed
(daily), Holds per record (count)
- Patron, Book, 0, 0, 0
- Student, Book, 1, 1, 1
- All, Continuing resources, 1, 1, 1
4. Place a title level hold on the biblio for patron A
5. Place a title level hold on the biblio for patron B
6. Checkin the Book and notice you are prompted to confirm the hold
by Patron A - even though according to circ rules they cannot reserve Books
7. Apply patch and restart services
9. Checkin the Book, and notice you're prompted to confirm the hold
by Patron B

Sponsored-By: Catalyst IT
Comment 6 Alex Buckley 2023-08-01 23:21:34 UTC
Created attachment 154141 [details] [review]
Bug 29750: Unit test fixes for Reserves.pm

Test plan:
1. sudo koha-shell <instance>
2.
prove t/db_dependent/Holds.t
prove t/db_dependent/Reserves.t

Sponsored-By: Catalyst IT
Comment 7 Alex Buckley 2023-08-01 23:24:20 UTC
(In reply to Katrin Fischer from comment #4)
> Hi Alex, should this have a different status?

Hi Katrin, 

My apologies for the late reply. Yes you're right this should be 'Needs signoff'

As this is an old bug report I've just checked upstream and this bug can still be replicated. 

I've tidied up the patches a bit, they apply cleanly, and they actually don't depend on bug 23172 so I've removed that dependency. 

Ready for testing. 

Kind regards
Alex
Comment 8 Sam Lau 2023-08-02 14:51:59 UTC
Created attachment 154159 [details] [review]
Bug 29750: Reserves.pm should check if a patron is eligible to reserve and item before confirming the reserve

Test plan:
1. Create two patrons, with the following patron categories:
- Patron A -> Patron
- Patron B -> Student
2. Create two items attached to a single biblio record:
- Book
- Continuing resource
3. Create the following circ rules:
Patron category, item type, Holds allowed (total), Holds allowed
(daily), Holds per record (count)
- Patron, Book, 0, 0, 0
- Student, Book, 1, 1, 1
- All, Continuing resources, 1, 1, 1
4. Place a title level hold on the biblio for patron A
5. Place a title level hold on the biblio for patron B
6. Checkin the Book and notice you are prompted to confirm the hold
by Patron A - even though according to circ rules they cannot reserve Books
7. Apply patch and restart services
9. Checkin the Book, and notice you're prompted to confirm the hold
by Patron B

Sponsored-By: Catalyst IT
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 9 Sam Lau 2023-08-02 14:52:01 UTC
Created attachment 154160 [details] [review]
Bug 29750: Unit test fixes for Reserves.pm

Test plan:
1. sudo koha-shell <instance>
2.
prove t/db_dependent/Holds.t
prove t/db_dependent/Reserves.t

Sponsored-By: Catalyst IT
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 10 Marcel de Rooy 2023-09-29 09:29:26 UTC
(In reply to Alex Buckley from comment #0)
> Bug 23172 introduced a check for patron categories hold policies when the
> holds queue maps items to pending reserves. 

Just noting that 23172 is still In discussion and not moving.
What impact does that have on this report?
Comment 11 Marcel de Rooy 2023-09-29 09:37:14 UTC
One of the tests is failing now.

Another question: How does this do in terms of performance? Looks like we are checking in CanItemBeReserved lots of things that we already checked.
Do you have a specific check in mind that we did not yet catch? Or could we eliminate some checks now since they will be done in CanItemBeReserved now ?

Needs some attention.
Comment 12 Nick Clemens 2023-09-29 13:06:02 UTC
I don't know if I agree with these patches.

Rules for placing a hold and rules for filling a hold are not the same.

Being allowed to place 0 holds is different than these holds are not allowed.

It's subtle, and confusing, but its designed this way and libraries use overrides to place holds that a patron can't place, but they expect to fill
Comment 13 Alex Buckley 2023-10-30 19:32:37 UTC
(In reply to Nick Clemens from comment #12)
> I don't know if I agree with these patches.
> 
> Rules for placing a hold and rules for filling a hold are not the same.
> 
> Being allowed to place 0 holds is different than these holds are not allowed.
> 
> It's subtle, and confusing, but its designed this way and libraries use
> overrides to place holds that a patron can't place, but they expect to fill

Hi Nick, 

It was nice to meet you in person at KohaCon23 :)

Thank you for your feedback.

Looking at these patches in retrospect, I agree. 
I have conflated placing and filling holds which could cause unintended consequences with respect to overrides. 

That being the case I am going to close this bug report.

Alex