Bug 25662

Summary: Create hold route does not check maxreserves syspref
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: aleisha, aspencatteam, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25643
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24466
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04
Bug Depends on: 13903    
Bug Blocks:    
Attachments: Bug 25662: Regression tests
Bug 25662: Make the route for holds restpect maxreserves
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour
[19.11.x] Bug 25662: Regression tests
[19.11.x] Bug 25662: Make the route for holds restpect maxreserves
[19.11.x] Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour
Bug 25662: Regression tests
Bug 25662: Make the route for holds restpect maxreserves
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour
Bug 25662: Regression tests
Bug 25662: Make the route for holds restpect maxreserves
Bug 25662: (follow-up) Add tests for the wrong patron_id added behaviour

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