Some checks for patron's ability to place holds are baked in * controllers (e.g. opac-reserve.pl, request.pl) * templates (e.g. opac-reserve.tt, request.tt) We should move that logic into a reusable sub, covered with tests. My initial motivation is bug 39657 so we use this on the API, but it is a good opportunity to start cleaning up the codebase in the area as well.
Created attachment 183104 [details] [review] Bug 40101: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 183105 [details] [review] Bug 40101: Add `Koha::Patron->can_place_holds()` This patch adds a new method to the `Koha::Patron` class. As described in the POD, this method makes checks on the patron's current situation and returns a boolean telling if it should be allowed to place holds. This doesn't check specific item/biblio holdability. It only covers patron specific conditions that could prevent holds to be placed. Both in `opac-reserve.pl` and `reserve/request.pl` this checks are all run, and all blocking situations are presented to the UI user. Feedback in community (so far) expressed this is a desirable situation, so that behavior can be retained with the use of the `no_short_circuit` parameter. If `no_short_circuit` is not passed, then the routine will return on the first blocking condition it finds. An `overrides` parameter is added, allowing to be passed a list of strings that match the possible error messages. This way, this method can be asked not to check for patron expiration, for example, and things like that. This is of course designed with the API overrides use case in mind. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patron.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 183106 [details] [review] Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 183197 [details] [review] Bug 40101: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Created attachment 183198 [details] [review] Bug 40101: Add `Koha::Patron->can_place_holds()` This patch adds a new method to the `Koha::Patron` class. As described in the POD, this method makes checks on the patron's current situation and returns a boolean telling if it should be allowed to place holds. This doesn't check specific item/biblio holdability. It only covers patron specific conditions that could prevent holds to be placed. Both in `opac-reserve.pl` and `reserve/request.pl` this checks are all run, and all blocking situations are presented to the UI user. Feedback in community (so far) expressed this is a desirable situation, so that behavior can be retained with the use of the `no_short_circuit` parameter. If `no_short_circuit` is not passed, then the routine will return on the first blocking condition it finds. An `overrides` parameter is added, allowing to be passed a list of strings that match the possible error messages. This way, this method can be asked not to check for patron expiration, for example, and things like that. This is of course designed with the API overrides use case in mind. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patron.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Created attachment 183199 [details] [review] Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Created attachment 183204 [details] [review] Bug 39657: Add more checks and overrides to hold creation endpoint This patch adds new behaviors to the `POST /holds` endpoint. It uses the framework we created in the past for overriding policy rules, and add new options: * expired * debt_limit * bad_address * card_lost * restricted * hold_limit Some status codes are changed from 403 to 409. This should be revisited accross the codebase, as I think we made a wrong choice. Happy to review in this bug. The feature makes use of the newly introduced `$patron->can_place_holds()` method, which accepts to be passed through the overrides. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ yarn api:bundle k$ koha-plack --restart kohadev k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 3. Test the endpoint with the various scenarios using your favourite REST tool (Postman!) 4. Sign off :-D
Created attachment 183242 [details] [review] Bug 40101: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Created attachment 183243 [details] [review] Bug 40101: Add `Koha::Patron->can_place_holds()` This patch adds a new method to the `Koha::Patron` class. As described in the POD, this method makes checks on the patron's current situation and returns a boolean telling if it should be allowed to place holds. This doesn't check specific item/biblio holdability. It only covers patron specific conditions that could prevent holds to be placed. Both in `opac-reserve.pl` and `reserve/request.pl` this checks are all run, and all blocking situations are presented to the UI user. Feedback in community (so far) expressed this is a desirable situation, so that behavior can be retained with the use of the `no_short_circuit` parameter. If `no_short_circuit` is not passed, then the routine will return on the first blocking condition it finds. An `overrides` parameter is added, allowing to be passed a list of strings that match the possible error messages. This way, this method can be asked not to check for patron expiration, for example, and things like that. This is of course designed with the API overrides use case in mind. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patron.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Created attachment 183244 [details] [review] Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Created attachment 183488 [details] [review] Bug 40101: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 183489 [details] [review] Bug 40101: Add `Koha::Patron->can_place_holds()` This patch adds a new method to the `Koha::Patron` class. As described in the POD, this method makes checks on the patron's current situation and returns a boolean telling if it should be allowed to place holds. This doesn't check specific item/biblio holdability. It only covers patron specific conditions that could prevent holds to be placed. Both in `opac-reserve.pl` and `reserve/request.pl` this checks are all run, and all blocking situations are presented to the UI user. Feedback in community (so far) expressed this is a desirable situation, so that behavior can be retained with the use of the `no_short_circuit` parameter. If `no_short_circuit` is not passed, then the routine will return on the first blocking condition it finds. An `overrides` parameter is added, allowing to be passed a list of strings that match the possible error messages. This way, this method can be asked not to check for patron expiration, for example, and things like that. This is of course designed with the API overrides use case in mind. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patron.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 183490 [details] [review] Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
I've added two further follow-up bugs that need to be attended to in due course for the missing refactoring of the codebase to utilise this new method in a wider context. Otherwise, however, I'm happy to PQA on this bug as is.
Nice work everyone! Pushed to main for 25.11