Summary: | Block holds placed via the API when patron would be blocked from placing OPAC hold | ||
---|---|---|---|
Product: | Koha | Reporter: | Andrew Fuerste-Henry <andrew> |
Component: | REST API | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Status: | Pushed to main --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | carthur, catrina, kkrueger, lucas, magnus, martin.renvoize, nick, tomascohen, wdeschamps |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
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: | ||
Bug Depends on: | 40101 | ||
Bug Blocks: | 40130, 40131, 40254, 40257 | ||
Attachments: |
Bug 39657: Unit tests
Bug 39657: Spec changes Bug 39657: Add more checks and overrides to hold creation endpoint Bug 39657: Unit tests Bug 39657: Spec changes Bug 39657: Add more checks and overrides to hold creation endpoint Bug 39657: Unit tests Bug 39657: Spec changes Bug 39657: Add more checks and overrides to hold creation endpoint |
Description
Andrew Fuerste-Henry
2025-04-16 15:41:20 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. 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. 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. Created attachment 183108 [details] [review] Bug 39657: Unit tests Created attachment 183109 [details] [review] Bug 39657: Spec changes 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 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 *** Bug 35703 has been marked as a duplicate of this bug. *** (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. Created attachment 183200 [details] [review] Bug 39657: Unit tests Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org> Created attachment 183201 [details] [review] Bug 39657: Spec changes Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org> 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> 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> 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> 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> 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. (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. Tomas, Tests are failing here ( t/db_dependent/api/v1/holds.t ), can you have a look? 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. Sorry Tomas, it was my mistake. Pushed to main for 25.11 Thanks everyone. I noticed a bug when looking at the tests. Will file a follow-up bug as it will need regression tests and stuff. *** Bug 24841 has been marked as a duplicate of this bug. *** |