Bug 23857 - Club holds endpoint not setting Location header
Summary: Club holds endpoint not setting Location header
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on: 19618 27330
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-21 17:00 UTC by Tomás Cohen Arazi
Modified: 2021-01-27 12:41 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23857: Regression tests (2.91 KB, patch)
2021-01-04 13:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23857: Make adding club holds set the Location header (818 bytes, patch)
2021-01-04 13:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23857: Regression tests (2.91 KB, patch)
2021-01-04 13:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23857: Make adding club holds set the Location header (1.12 KB, patch)
2021-01-04 13:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23857: (QA follow-up) Use Basic auth in tests (8.63 KB, patch)
2021-01-04 13:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23857: Regression tests (2.96 KB, patch)
2021-01-25 21:02 UTC, David Nind
Details | Diff | Splinter Review
Bug 23857: Make adding club holds set the Location header (1.17 KB, patch)
2021-01-25 21:02 UTC, David Nind
Details | Diff | Splinter Review
Bug 23857: (QA follow-up) Use Basic auth in tests (8.69 KB, patch)
2021-01-25 21:02 UTC, David Nind
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 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.