Bug 40101 - Add `Koha::Patron->can_place_holds`
Summary: Add `Koha::Patron->can_place_holds`
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: PC All
: P1 - high enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords: release-notes-needed
Depends on: 29746
Blocks: 39657 40237 40238
  Show dependency treegraph
 
Reported: 2025-06-09 16:35 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-06-26 17:22 UTC (History)
6 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:


Attachments
Bug 40101: Unit tests (9.66 KB, patch)
2025-06-09 20:39 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40101: Add `Koha::Patron->can_place_holds()` (6.38 KB, patch)
2025-06-09 20:39 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` (3.98 KB, patch)
2025-06-09 20:39 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40101: Unit tests (9.72 KB, patch)
2025-06-12 12:37 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40101: Add `Koha::Patron->can_place_holds()` (6.44 KB, patch)
2025-06-12 12:37 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` (4.04 KB, patch)
2025-06-12 12:37 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 39657: Add more checks and overrides to hold creation endpoint (3.74 KB, patch)
2025-06-12 12:57 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40101: Unit tests (9.72 KB, patch)
2025-06-13 13:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40101: Add `Koha::Patron->can_place_holds()` (6.44 KB, patch)
2025-06-13 13:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` (4.04 KB, patch)
2025-06-13 13:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40101: Unit tests (9.78 KB, patch)
2025-06-25 11:43 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40101: Add `Koha::Patron->can_place_holds()` (6.51 KB, patch)
2025-06-25 11:43 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40101: Make opac-reserve.pl use `$patron->can_place_holds()` (4.10 KB, patch)
2025-06-25 11:43 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2025-06-09 16:35:33 UTC
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.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-06-09 20:39:20 UTC
Created attachment 183104 [details] [review]
Bug 40101: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2025-06-09 20:39:23 UTC
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>
Comment 3 Tomás Cohen Arazi (tcohen) 2025-06-09 20:39:26 UTC
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>
Comment 4 Andrew Fuerste-Henry 2025-06-12 12:37:15 UTC
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>
Comment 5 Andrew Fuerste-Henry 2025-06-12 12:37:17 UTC
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>
Comment 6 Andrew Fuerste-Henry 2025-06-12 12:37:20 UTC
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>
Comment 7 Tomás Cohen Arazi (tcohen) 2025-06-12 12:57:27 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi (tcohen) 2025-06-13 13:43:11 UTC
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>
Comment 9 Tomás Cohen Arazi (tcohen) 2025-06-13 13:43:15 UTC
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>
Comment 10 Tomás Cohen Arazi (tcohen) 2025-06-13 13:43:18 UTC
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>
Comment 11 Martin Renvoize (ashimema) 2025-06-25 11:43:16 UTC
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>
Comment 12 Martin Renvoize (ashimema) 2025-06-25 11:43:18 UTC
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>
Comment 13 Martin Renvoize (ashimema) 2025-06-25 11:43:20 UTC
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>
Comment 14 Martin Renvoize (ashimema) 2025-06-25 11:46:39 UTC
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.
Comment 15 Lucas Gass (lukeg) 2025-06-26 17:12:02 UTC
Nice work everyone!

Pushed to main for 25.11