Bug 39657 - Block holds placed via the API when patron would be blocked from placing OPAC hold
Summary: Block holds placed via the API when patron would be blocked from placing OPAC...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
: 24841 35703 (view as bug list)
Depends on: 40101
Blocks: 40130 40131 40254 40257
  Show dependency treegraph
 
Reported: 2025-04-16 15:41 UTC by Andrew Fuerste-Henry
Modified: 2025-07-03 17:46 UTC (History)
9 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development adds more holdability checks to the `POST /holds` endpoint. Overrides are added for all of them: * bad_address * card_lost * debt_limit * expired * hold_limit * restricted Before this development, only `any` could be passed as an override. It will now have more granularity.
Version(s) released in:
Circulation function:


Attachments
Bug 39657: Unit tests (3.38 KB, patch)
2025-06-09 22:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39657: Spec changes (1.35 KB, patch)
2025-06-09 22:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39657: Add more checks and overrides to hold creation endpoint (3.68 KB, patch)
2025-06-09 22:51 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39657: Unit tests (3.44 KB, patch)
2025-06-12 12:40 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 39657: Spec changes (1.41 KB, patch)
2025-06-12 12:40 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 39657: Add more checks and overrides to hold creation endpoint (3.75 KB, patch)
2025-06-12 12:40 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 39657: Unit tests (3.50 KB, patch)
2025-06-25 12:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39657: Spec changes (1.47 KB, patch)
2025-06-25 12:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39657: Add more checks and overrides to hold creation endpoint (3.81 KB, patch)
2025-06-25 12:26 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 Andrew Fuerste-Henry 2025-04-16 15:41:20 UTC
When the API is used to perform actions on behalf of patrons, we should be able to block those actions when the patron is expired. This is similar to the BlockExpiredPatronOpacActions system preference, which gives the option to block holds, renewals, and/or ILL requests for expired patrons.
Comment 1 Wally DesChamps 2025-05-09 13:59:27 UTC
This is causing confusion and bad PR for us and needs to be corrected ASAP.  This has been reported in Aspen (Jira).  All restrictions and circ rules should be honored across the board.
Comment 2 Katrin Fischer 2025-05-09 14:46:04 UTC
I believe these are different things.

A) Make the API another option in BlockExpiredPatronOpacActions.

B) Make a BlockExpiredPatronOpacActions for API use.

c) Update our APIs to work like the staff interface - enforcing what is configured in BlockExpiredPatronOpacActions.

BlockExpiredPatronOpacActions = meaning the system preference and the patron category specific overrride.
Comment 3 Tomás Cohen Arazi (tcohen) 2025-06-09 11:36:33 UTC
Our API policy override schema is designed so we can add another checks that can block placing holds, and we can add individual overrides for them.

I'll give this a shot.
Comment 4 Tomás Cohen Arazi (tcohen) 2025-06-09 22:51:32 UTC
Created attachment 183108 [details] [review]
Bug 39657: Unit tests
Comment 5 Tomás Cohen Arazi (tcohen) 2025-06-09 22:51:40 UTC
Created attachment 183109 [details] [review]
Bug 39657: Spec changes
Comment 6 Tomás Cohen Arazi (tcohen) 2025-06-09 22:51:47 UTC
Created attachment 183110 [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
Comment 7 Andrew Fuerste-Henry 2025-06-11 18:45:38 UTC
These patches pursue Katrin's option C, making the API behavior match what the patron would be able to do via the OPAC.

Expanding this test plan a little for more detail:

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. Have an expired patron; confirm BlockExpiredPatronOpacActions syspref set to allow patrons to place hold via opac; confirm your patron can have holds placed for them via API

5a. Set your expired patron's category to block holds for expired patrons; confirm your patron cannot have a hold placed for them via the API

5b. Switch BlockExpiredPatronOpacActions syspref to block OPAC holds for all expired patrons; switch your expired patron's category back to following the syspref; confirm your patron still cannot have a hold placed for them via the API


6. Have a patron flagged "Gone no address." Confirm they cannot have a hold placed for them via the API
7. Repeat for a patron flagged "Lost card"
8. and a patron with debt greater than the maxoutstanding syspref
9. and a patron with a restriction
10. and a patron who already has as many holds as they're allowed by policy


11. Sign off :-D
Comment 8 Andrew Fuerste-Henry 2025-06-11 18:46:23 UTC
*** Bug 35703 has been marked as a duplicate of this bug. ***
Comment 9 Tomás Cohen Arazi (tcohen) 2025-06-11 18:57:00 UTC
(In reply to Andrew Fuerste-Henry from comment #7)
> These patches pursue Katrin's option C, making the API behavior match what
> the patron would be able to do via the OPAC.
> 
> Expanding this test plan a little for more detail:

I would also add to the test plan, that all checks that block the hold from happening can be overridden with the `x-koha-override` header, and can be tested. The valid overrides are:

* any
* bad_address
* card_lost
* debt_limit
* expired
* hold_limit
* restricted

This is well documented on the spec.
Comment 10 Andrew Fuerste-Henry 2025-06-12 12:40:50 UTC
Created attachment 183200 [details] [review]
Bug 39657: Unit tests

Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Comment 11 Andrew Fuerste-Henry 2025-06-12 12:40:53 UTC
Created attachment 183201 [details] [review]
Bug 39657: Spec changes

Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Comment 12 Andrew Fuerste-Henry 2025-06-12 12:40:55 UTC
Created attachment 183202 [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

Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Comment 13 Martin Renvoize (ashimema) 2025-06-25 12:26:50 UTC
Created attachment 183491 [details] [review]
Bug 39657: Unit tests

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 12:26:53 UTC
Created attachment 183492 [details] [review]
Bug 39657: Spec changes

Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 15 Martin Renvoize (ashimema) 2025-06-25 12:26:55 UTC
Created attachment 183493 [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

Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 16 Martin Renvoize (ashimema) 2025-06-25 12:29:02 UTC
OK, I'm passing QA here.. but I do wonder a little about when we should be just allowing 'x-koha-override' headers and when we should actually require a confirmation as the checkouts endpoint does.

With this case, the API consumer could just always opt to pass x-koha-override=any or list all the overrides with any request and as such, we're not actually blocking anything.
Comment 17 Tomás Cohen Arazi (tcohen) 2025-06-25 19:32:25 UTC
(In reply to Martin Renvoize (ashimema) from comment #16)
> OK, I'm passing QA here.. but I do wonder a little about when we should be
> just allowing 'x-koha-override' headers and when we should actually require
> a confirmation as the checkouts endpoint does.
> 
> With this case, the API consumer could just always opt to pass
> x-koha-override=any or list all the overrides with any request and as such,
> we're not actually blocking anything.

Just FTR, as we already chatted about this: I think in its current design this endpoint could be extended with the `confirmation flow` in a pretty straight-forward way.

That said, the system we are calling the API from (Aspen) is asking Koha to actually *block* the requests. We are adding the individual overrides just so other API consumers can recreate the previous behavior if they find the need for that.

Martin: if you think the confirmation flow is desirable for holds, please file a follow-up bug and I will be more than happy to give it a shot.
Comment 18 Lucas Gass (lukeg) 2025-06-26 16:55:38 UTC
Tomas,

Tests are failing here ( t/db_dependent/api/v1/holds.t ), can you have a look?
Comment 19 Tomás Cohen Arazi (tcohen) 2025-06-26 17:53:09 UTC
I don't reproduce the failures.

Remember the API always returns 503 if there are pending upgrades on the DB, and as this patch changes the spec, it needs a `yarn api:bundle` and `koha-plack --restart kohadev` in KTD.

If you share more details about the failure, I'll fix it ASAP.
Comment 20 Lucas Gass (lukeg) 2025-06-26 18:50:27 UTC
Sorry Tomas, it was my mistake. 

Pushed to main for 25.11

Thanks everyone.
Comment 21 Tomás Cohen Arazi (tcohen) 2025-06-26 19:01:46 UTC
I noticed a bug when looking at the tests. Will file a follow-up bug as it will need regression tests and stuff.
Comment 22 Tomás Cohen Arazi (tcohen) 2025-07-01 17:55:49 UTC
*** Bug 24841 has been marked as a duplicate of this bug. ***