Bug 40254

Summary: POST /holds override logic problem
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: REST APIAssignee: 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
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>