Bug 25662 - Create hold route does not check maxreserves syspref
Summary: Create hold route does not check maxreserves syspref
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 13903
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-03 08:34 UTC by Jonathan Druart
Modified: 2020-09-10 04:41 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04


Attachments
Bug 25662: Regression tests (3.59 KB, patch)
2020-06-23 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25662: Make the route for holds restpect maxreserves (1.68 KB, patch)
2020-06-23 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour (1.93 KB, patch)
2020-06-23 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[19.11.x] Bug 25662: Regression tests (3.72 KB, patch)
2020-06-23 17:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[19.11.x] Bug 25662: Make the route for holds restpect maxreserves (1.69 KB, patch)
2020-06-23 17:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[19.11.x] Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour (1.94 KB, patch)
2020-06-23 17:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25662: Regression tests (3.66 KB, patch)
2020-06-25 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25662: Make the route for holds restpect maxreserves (1.75 KB, patch)
2020-06-25 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour (2.00 KB, patch)
2020-06-25 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25662: Regression tests (3.71 KB, patch)
2020-08-22 23:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25662: Make the route for holds restpect maxreserves (1.79 KB, patch)
2020-08-22 23:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour (2.04 KB, patch)
2020-08-22 23:01 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-06-03 08:34:17 UTC
The "maxreserves" syspref is only checked in opac/opac-reserve.pl and reserve/request.pl
Comment 1 Tomás Cohen Arazi 2020-06-03 19:35:26 UTC
(In reply to Jonathan Druart from comment #0)
> The "maxreserves" syspref is only checked in opac/opac-reserve.pl and
> reserve/request.pl

Why isn't maxreserves checked in CanItemBeReserved?
Comment 2 Jonathan Druart 2020-06-04 13:01:17 UTC
It must indeed be moved to CanItemBeReserved (and I think stay in the controller as well, for multi holds).
Comment 3 Tomás Cohen Arazi 2020-06-23 13:39:12 UTC
Created attachment 106204 [details] [review]
Bug 25662: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2020-06-23 13:39:17 UTC
Created attachment 106205 [details] [review]
Bug 25662: Make the route for holds restpect maxreserves

This patch fixes the behaviour for the POST /holds route. It assumed
maxreserves was checked in CanItemBeReserved which is not the case.

Tests are added to check for this behaviour.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2020-06-23 13:39:25 UTC
Created attachment 106206 [details] [review]
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour

This patch adds trivial tests for the new error (400) raised when the
passed patron_id doesn't exist.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2020-06-23 17:41:35 UTC
Created attachment 106212 [details] [review]
[19.11.x] Bug 25662: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2020-06-23 17:41:42 UTC
Created attachment 106213 [details] [review]
[19.11.x] Bug 25662: Make the route for holds restpect maxreserves

This patch fixes the behaviour for the POST /holds route. It assumed
maxreserves was checked in CanItemBeReserved which is not the case.

Tests are added to check for this behaviour.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2020-06-23 17:41:47 UTC
Created attachment 106214 [details] [review]
[19.11.x] Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour

This patch adds trivial tests for the new error (400) raised when the
passed patron_id doesn't exist.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Kyle M Hall 2020-06-25 14:33:45 UTC
Created attachment 106297 [details] [review]
Bug 25662: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2020-06-25 14:34:00 UTC
Created attachment 106298 [details] [review]
Bug 25662: Make the route for holds restpect maxreserves

This patch fixes the behaviour for the POST /holds route. It assumed
maxreserves was checked in CanItemBeReserved which is not the case.

Tests are added to check for this behaviour.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2020-06-25 14:34:03 UTC
Created attachment 106299 [details] [review]
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour

This patch adds trivial tests for the new error (400) raised when the
passed patron_id doesn't exist.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Katrin Fischer 2020-08-22 23:00:36 UTC
Let's pretend we switched - I will sign off and Kyle will count for QA ;)
Comment 13 Katrin Fischer 2020-08-22 23:01:25 UTC
Created attachment 108940 [details] [review]
Bug 25662: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2020-08-22 23:01:29 UTC
Created attachment 108941 [details] [review]
Bug 25662: Make the route for holds restpect maxreserves

This patch fixes the behaviour for the POST /holds route. It assumed
maxreserves was checked in CanItemBeReserved which is not the case.

Tests are added to check for this behaviour.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2020-08-22 23:01:33 UTC
Created attachment 108942 [details] [review]
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour

This patch adds trivial tests for the new error (400) raised when the
passed patron_id doesn't exist.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Jonathan Druart 2020-08-25 13:16:22 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Aleisha Amohia 2020-09-10 04:41:26 UTC
backported to 20.05.x for 20.05.04
Comment 18 Aleisha Amohia 2020-09-10 04:41:48 UTC
Tests fail on 19.11.x. Please fix and let me know when ready to re-backport.

$ prove t/db_dependent/api/v1/holds.t 
t/db_dependent/api/v1/holds.t .. 
    #   Failed test '401 Unauthorized'
    #   at t/db_dependent/api/v1/holds.t line 148.
    #          got: '503'
    #     expected: '401'

    #   Failed test '401 Unauthorized'
    #   at t/db_dependent/api/v1/holds.t line 150.
    #          got: '503'
    #     expected: '401'

    #   Failed test '401 Unauthorized'
    #   at t/db_dependent/api/v1/holds.t line 152.
    #          got: '503'
    #     expected: '401'

    #   Failed test '401 Unauthorized'
    #   at t/db_dependent/api/v1/holds.t line 154.
    #          got: '503'
    #     expected: '401'
    # Looks like you failed 4 tests of 8.
t/db_dependent/api/v1/holds.t .. 1/9 
#   Failed test 'Test endpoints without authentication'
#   at t/db_dependent/api/v1/holds.t line 156.
t/db_dependent/api/v1/holds.t .. 8/9 
    #   Failed test '201 Created'
    #   at t/db_dependent/api/v1/holds.t line 585.
    #          got: '403'
    #     expected: '201'

    #   Failed test '201 Created'
    #   at t/db_dependent/api/v1/holds.t line 595.
    #          got: '403'
    #     expected: '201'
    # Looks like you failed 2 tests of 7.

#   Failed test 'add() tests (maxreserves behaviour)'
#   at t/db_dependent/api/v1/holds.t line 610.
# Looks like you failed 2 tests of 9.
t/db_dependent/api/v1/holds.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/9 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/holds.t (Wstat: 512 Tests: 9 Failed: 2)
  Failed tests:  1, 9
  Non-zero exit status: 2
Files=1, Tests=9,  5 wallclock secs ( 0.03 usr  0.01 sys +  4.50 cusr  0.87 csys =  5.41 CPU)
Result: FAIL