Bug 28480 - GET /patrons missing q parameters on the spec
Summary: GET /patrons missing q parameters on the spec
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 28613
  Show dependency treegraph
 
Reported: 2021-05-28 11:33 UTC by Tomás Cohen Arazi
Modified: 2022-06-06 20:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.02


Attachments
Bug 28480: Add q parameters for GET /patrons (1.21 KB, patch)
2021-05-28 11:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28480: Add q parameters for GET /patrons (1.25 KB, patch)
2021-05-29 00:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 28480: Add q parameters for GET /patrons (1.31 KB, patch)
2021-06-11 18:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28482: (QA follow-up) Add unit test (1.36 KB, patch)
2021-06-11 18:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28613: Add missing parameters to objects.search-based routes (5.00 KB, patch)
2021-06-22 11:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28480: (QA follow-up) Add unit test (1.38 KB, patch)
2021-06-23 13:08 UTC, Jonathan Druart
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 2021-05-28 11:33:00 UTC

    
Comment 1 Tomás Cohen Arazi 2021-05-28 11:41:02 UTC
Created attachment 121475 [details] [review]
Bug 28480: Add q parameters for GET /patrons

This patch adds the q query parameters to the route.

To test:
1. Try the route with the following query parameter:
   q={"patron_id":2}
   i.e. GET /api/v1/patrons?q={"patron_id":2}
=> FAIL: You get a bad request respose
2. Apply this patch
3. Restart all
4. Repeat 1
=> SUCCESS: You get the patron
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 David Nind 2021-05-29 00:21:56 UTC
Created attachment 121503 [details] [review]
Bug 28480: Add q parameters for GET /patrons

This patch adds the q query parameters to the route.

To test:
1. Try the route with the following query parameter:
   q={"patron_id":2}
   i.e. GET /api/v1/patrons?q={"patron_id":2}
=> FAIL: You get a bad request respose
2. Apply this patch
3. Restart all
4. Repeat 1
=> SUCCESS: You get the patron
5. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Nick Clemens 2021-06-11 18:03:02 UTC
Created attachment 121874 [details] [review]
Bug 28480: Add q parameters for GET /patrons

This patch adds the q query parameters to the route.

To test:
1. Try the route with the following query parameter:
   q={"patron_id":2}
   i.e. GET /api/v1/patrons?q={"patron_id":2}
=> FAIL: You get a bad request respose
2. Apply this patch
3. Restart all
4. Repeat 1
=> SUCCESS: You get the patron
5. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens 2021-06-11 18:03:06 UTC
Created attachment 121875 [details] [review]
Bug 28482: (QA follow-up) Add unit test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Jonathan Druart 2021-06-21 09:51:44 UTC
Aren't they missing for other routes as well? Shouldn't we have a test to catch missing occurrences?
Comment 6 Jonathan Druart 2021-06-21 09:54:05 UTC
Second patch has a wrong bug number.
Comment 7 Tomás Cohen Arazi 2021-06-22 11:27:35 UTC
(In reply to Jonathan Druart from comment #5)
> Aren't they missing for other routes as well? Shouldn't we have a test to
> catch missing occurrences?

Do you want a follow-up? Here? Separate bug? I can take care of that.

I think we could have a test on the QA tools raise a warning, as some routes are not yet (maybe never) implemented using our stack (e.g. /ill_backends and /illrequests).
Comment 8 Tomás Cohen Arazi 2021-06-22 11:56:51 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2021-06-23 13:06:45 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 10 Jonathan Druart 2021-06-23 13:08:07 UTC
Created attachment 122332 [details] [review]
Bug 28480: (QA follow-up) Add unit test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Fixed bug number
Comment 11 Kyle M Hall 2021-07-09 13:09:56 UTC
Pushed to 21.05.x for 21.05.02
Comment 12 Fridolin Somers 2021-08-02 20:25:59 UTC
Pushed to 20.11.x for 20.11.09
Comment 13 Fridolin Somers 2021-08-03 02:25:31 UTC
Arf breaks test suite :
 
t/db_dependent/api/v1/patrons.t .. 
        #   Failed test 'exact match for JSON Pointer "/0/cardnumber"'
        #   at t/db_dependent/api/v1/patrons.t line 66.
        #          got: '1'
        #     expected: 'O_HBrhYpvfsZOtvfyC'
        # Looks like you failed 1 test of 16.

    #   Failed test 'librarian access tests'
    #   at t/db_dependent/api/v1/patrons.t line 84.
    # Looks like you failed 1 test of 2.
t/db_dependent/api/v1/patrons.t .. 1/7 
#   Failed test 'list() tests'
#   at t/db_dependent/api/v1/patrons.t line 85.
t/db_dependent/api/v1/patrons.t .. 5/7 # Looks like you failed 1 test of 7.
t/db_dependent/api/v1/patrons.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/7 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/patrons.t (Wstat: 256 Tests: 7 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=7,  7 wallclock secs ( 0.03 usr  0.00 sys +  6.59 cusr  0.54 csys =  7.16 CPU)
Result: FAIL

So I revert from 20.11.x
Comment 14 Victor Grousset/tuxayo 2021-08-12 22:20:04 UTC
Missing dependencies for 20.05.x, it shouldn't be affected, no backport.