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.
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
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
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
Hi Alex, should this have a different status?
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
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
(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
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>
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>
(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?
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.
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
(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