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: | Passed QA --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | martin.renvoize, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | 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> |