Thinking about extending the tests introduced in bug 39657, I noticed we missed to pass the overrides to the `$patron->can_place_holds()` call.
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>