Bug 40254 - POST /holds override logic problem
Summary: POST /holds override logic problem
Status: Passed QA
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:
Depends on: 39657
Blocks:
  Show dependency treegraph
 
Reported: 2025-06-26 19:03 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-06-27 13:17 UTC (History)
2 users (show)

See Also:
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:


Attachments
Bug 40254: Regression tests (11.75 KB, patch)
2025-06-26 20:14 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40254: Fix error in overrides check allowing too much (1.62 KB, patch)
2025-06-26 20:14 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40254: Fix error in overrides check allowing too much (1.69 KB, patch)
2025-06-26 20:51 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 40254: Regression tests (11.81 KB, patch)
2025-06-26 20:52 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 40254: Fix error in overrides check allowing too much (1.75 KB, patch)
2025-06-27 13:17 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40254: Regression tests (11.87 KB, patch)
2025-06-27 13:17 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-26 19:03:17 UTC
Thinking about extending the tests introduced in bug 39657, I noticed we missed to pass the overrides to the `$patron->can_place_holds()` call.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-06-26 20:07:28 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.
Comment 2 Tomás Cohen Arazi (tcohen) 2025-06-26 20:14:41 UTC
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.
Comment 3 Tomás Cohen Arazi (tcohen) 2025-06-26 20:14:44 UTC
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
Comment 4 Lisette Scheer 2025-06-26 20:51:49 UTC
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>
Comment 5 Lisette Scheer 2025-06-26 20:52:52 UTC
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>
Comment 6 Martin Renvoize (ashimema) 2025-06-27 13:17:27 UTC
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>
Comment 7 Martin Renvoize (ashimema) 2025-06-27 13:17:29 UTC
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>