| Summary: | POST /holds override logic problem | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Component: | REST API | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Status: | RESOLVED FIXED | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | david, fridolin.somers, lucas, martin.renvoize, tomascohen |
| Version: | Main | Keywords: | release-notes-needed |
| Hardware: | All | ||
| OS: | All | ||
| GIT URL: | Initiative type: | --- | |
| Sponsorship status: | --- | Comma delimited list of Sponsors: | |
| Crowdfunding goal: | 0 | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: |
25.11.00,25.05.02
|
|
| Circulation function: | |||
| Bug Depends on: | 39657 | ||
| Bug Blocks: | |||
| Attachments: |
Bug 40254: Regression tests
Bug 40254: Fix error in overrides check allowing too much Bug 40254: Fix error in overrides check allowing too much Bug 40254: Regression tests Bug 40254: Fix error in overrides check allowing too much Bug 40254: Regression tests |
||
|
Description
Tomás Cohen Arazi (tcohen)
2025-06-26 19:03:17 UTC
The code smelled because of the overrides not being passed to the method, and only the first 'message' being checked for overrides. I extended the tests and made them full-stack (as opposed to mocked) and caught the bug. Created attachment 183557 [details] [review] Bug 40254: Regression tests This patch makes the tests related to `can_place_holds` cover more use cases, and highlight an underlying issue in the controller. Created attachment 183558 [details] [review] Bug 40254: Fix error in overrides check allowing too much This patch fixes a subtle (yet naive) error making the controller not care about blocking conditions if they were more than one and the first one was overridden. To test: 1. Apply the regression tests 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail! Some tests expect a 409 (denied for a conflict) but return a 201! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Created attachment 183560 [details] [review] Bug 40254: Fix error in overrides check allowing too much This patch fixes a subtle (yet naive) error making the controller not care about blocking conditions if they were more than one and the first one was overridden. To test: 1. Apply the regression tests 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail! Some tests expect a 409 (denied for a conflict) but return a 201! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Created attachment 183561 [details] [review] Bug 40254: Regression tests This patch makes the tests related to `can_place_holds` cover more use cases, and highlight an underlying issue in the controller. Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Created attachment 183603 [details] [review] Bug 40254: Fix error in overrides check allowing too much This patch fixes a subtle (yet naive) error making the controller not care about blocking conditions if they were more than one and the first one was overridden. To test: 1. Apply the regression tests 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail! Some tests expect a 409 (denied for a conflict) but return a 201! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 183604 [details] [review] Bug 40254: Regression tests This patch makes the tests related to `can_place_holds` cover more use cases, and highlight an underlying issue in the controller. Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Nice work everyone! Pushed to main for 25.11 Nice work everyone! Pushed to 25.05.x Depends on Bug 39657 not in 24.11.x API-related change, no changes required to the manual. |