Bug 23857

Summary: Club holds endpoint not setting Location header
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED INVALID QA Contact:
Severity: minor    
Priority: P5 - low CC: agustinmoyano, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER3.4.1_POST
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19618, 27330    
Bug Blocks:    
Attachments: Bug 23857: Regression tests
Bug 23857: Make adding club holds set the Location header
Bug 23857: Regression tests
Bug 23857: Make adding club holds set the Location header
Bug 23857: (QA follow-up) Use Basic auth in tests
Bug 23857: Regression tests
Bug 23857: Make adding club holds set the Location header
Bug 23857: (QA follow-up) Use Basic auth in tests

Description Tomás Cohen Arazi 2019-10-21 17:00:13 UTC
It is not being set
Comment 1 Tomás Cohen Arazi 2021-01-04 13:01:25 UTC
Created attachment 114778 [details] [review]
Bug 23857: Regression tests
Comment 2 Tomás Cohen Arazi 2021-01-04 13:01:28 UTC
Created attachment 114779 [details] [review]
Bug 23857: Make adding club holds set the Location header

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-01-04 13:20:03 UTC
Resubmitting, test instructions missing.
Comment 4 Tomás Cohen Arazi 2021-01-04 13:22:20 UTC
Created attachment 114782 [details] [review]
Bug 23857: Regression tests
Comment 5 Tomás Cohen Arazi 2021-01-04 13:22:24 UTC
Created attachment 114783 [details] [review]
Bug 23857: Make adding club holds set the Location header

This patch makes the club holds route add the Location header to the
response as specified in the API coding guidelines.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-01-04 13:22:28 UTC
Created attachment 114784 [details] [review]
Bug 23857: (QA follow-up) Use Basic auth in tests

This patch puts the tests in line with the current codebase. Some bits
are passed through perltidy to aid readability...

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
=> SUCCESS: Tests look more like the rest of the API tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 David Nind 2021-01-25 21:02:47 UTC
Created attachment 115789 [details] [review]
Bug 23857: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2021-01-25 21:02:50 UTC
Created attachment 115790 [details] [review]
Bug 23857: Make adding club holds set the Location header

This patch makes the club holds route add the Location header to the
response as specified in the API coding guidelines.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests don't pass!
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: David Nind <david@davidnind.com>
Comment 9 David Nind 2021-01-25 21:02:53 UTC
Created attachment 115791 [details] [review]
Bug 23857: (QA follow-up) Use Basic auth in tests

This patch puts the tests in line with the current codebase. Some bits
are passed through perltidy to aid readability...

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
=> SUCCESS: Tests look more like the rest of the API tests

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Martin Renvoize 2021-01-26 15:07:05 UTC
This one is a bit weird to me...

Whilst it adds a semi-sensible Location header.. I would have thought that header should point to the created resource (rather than the generic 'list all resources' route).  But, more than that, neither the `list` or `get` routes actually exist.. so the Location header will point to a 404.

I think we aught to add the `get` endpoint to get the individual created resource and update this patch to point to that specific resource rather than the list.
Comment 11 Tomás Cohen Arazi 2021-01-27 11:38:42 UTC
I'm setting this one 'in discussion'. Until we have a real need for the GET routes, I'm not sure this is relevant. The UI itself doesn't provide a way to list or view the club holds, so... if there's a request for such a feature, we could reopen this bug.
Comment 12 Martin Renvoize 2021-01-27 12:41:32 UTC
Thanks Tomas, sorry to have thrown a spanner into the works here..  with any luck we'll get there in the end down the line.. but I think your right.. right now 'In discussion' is the right move.