Bug 23858 - Vendors endpoint not setting the Location header
Summary: Vendors endpoint not setting the Location header
Status: Pushed to master
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: 23843
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-21 17:16 UTC by Tomás Cohen Arazi
Modified: 2019-10-28 12:39 UTC (History)
2 users (show)

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


Attachments
Bug 23858: Regression tests (1.27 KB, patch)
2019-10-21 17:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23858: Make POST add the Location header on vendors endpoint (1.14 KB, patch)
2019-10-21 17:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23858: (follow-up) Return code fix (2.27 KB, patch)
2019-10-21 17:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23858: Regression tests (1.42 KB, patch)
2019-10-22 12:02 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23858: Make POST add the Location header on vendors endpoint (1.29 KB, patch)
2019-10-22 12:02 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23858: (follow-up) Return code fix (2.41 KB, patch)
2019-10-22 12:02 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23858: Regression tests (1.47 KB, patch)
2019-10-25 18:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23858: Make POST add the Location header on vendors endpoint (1.34 KB, patch)
2019-10-25 18:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23858: (follow-up) Return code fix (2.47 KB, patch)
2019-10-25 18:52 UTC, Nick Clemens
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:16:08 UTC
It is not being set when creating a new vendor.
Comment 1 Tomás Cohen Arazi 2019-10-21 17:25:23 UTC
Created attachment 94480 [details] [review]
Bug 23858: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2019-10-21 17:25:29 UTC
Created attachment 94481 [details] [review]
Bug 23858: Make POST add the Location header on vendors endpoint

According to the coding guidelines, this endpoint should be adding the
Location header on POST actions.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2019-10-21 17:32:51 UTC
Created attachment 94482 [details] [review]
Bug 23858: (follow-up) Return code fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Joonas Kylmälä 2019-10-22 08:22:01 UTC
Comment on attachment 94481 [details] [review]
Bug 23858: Make POST add the Location header on vendors endpoint

Review of attachment 94481 [details] [review]:
-----------------------------------------------------------------

In the commit message "prove t/db_dependent/api/v1/acquisition_vendors.t" should be "prove t/db_dependent/api/v1/acquisitions_vendors.t"
Comment 5 Joonas Kylmälä 2019-10-22 08:23:05 UTC
Instead of making a follow-up to your own commit please squash the 201 regression tests to the first patch "Bug 23858: Regression tests" and make the fix separate
Comment 6 Tomás Cohen Arazi 2019-10-22 10:52:01 UTC
(In reply to Joonas Kylmälä from comment #5)
> Instead of making a follow-up to your own commit please squash the 201
> regression tests to the first patch "Bug 23858: Regression tests" and make
> the fix separate

Well, it is not a 'fix'. I actually wanted to honor the rule that refactoring patches should be separate from those introducing the behavior change so we can keep track of things better. The refactoring could have happened on a separate bug I reckon.

Yesterday I've finished the patrons one as well. Submitting today
Comment 7 Joonas Kylmälä 2019-10-22 10:59:21 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> (In reply to Joonas Kylmälä from comment #5)
> > Instead of making a follow-up to your own commit please squash the 201
> > regression tests to the first patch "Bug 23858: Regression tests" and make
> > the fix separate
> 
> Well, it is not a 'fix'. I actually wanted to honor the rule that
> refactoring patches should be separate from those introducing the behavior
> change so we can keep track of things better. The refactoring could have
> happened on a separate bug I reckon.
> 
> Yesterday I've finished the patrons one as well. Submitting today

Oh, I misread the patch earlier, I thought it was adding that 201 check as new test but I now see it tested it earlier already for 200 return code. Sorry for this confusion, will sign-off this soon.
Comment 8 Joonas Kylmälä 2019-10-22 11:44:13 UTC
When trying to apply the patch I get:

Applying: Bug 23858: Make POST add the Location header on vendors endpoint
error: sha1 information is lacking or useless (Koha/REST/V1/Acquisitions/Vendors.pm).

Can you resubmit rebased version?
Comment 9 Joonas Kylmälä 2019-10-22 11:45:11 UTC
Sorry, I forgot that this was depending on the other bug...
Comment 10 Joonas Kylmälä 2019-10-22 12:02:13 UTC
Created attachment 94534 [details] [review]
Bug 23858: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 11 Joonas Kylmälä 2019-10-22 12:02:17 UTC
Created attachment 94535 [details] [review]
Bug 23858: Make POST add the Location header on vendors endpoint

According to the coding guidelines, this endpoint should be adding the
Location header on POST actions.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 12 Joonas Kylmälä 2019-10-22 12:02:20 UTC
Created attachment 94536 [details] [review]
Bug 23858: (follow-up) Return code fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 13 Joonas Kylmälä 2019-10-22 12:03:54 UTC
(In reply to Joonas Kylmälä from comment #4)
> Comment on attachment 94481 [details] [review] [review]
> Bug 23858: Make POST add the Location header on vendors endpoint
> 
> Review of attachment 94481 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> In the commit message "prove t/db_dependent/api/v1/acquisition_vendors.t"
> should be "prove t/db_dependent/api/v1/acquisitions_vendors.t"

This was just a minor nitpick and I was easily able to figure it out so I will let it slide this time.
Comment 14 Nick Clemens 2019-10-25 18:52:25 UTC
Created attachment 94796 [details] [review]
Bug 23858: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2019-10-25 18:52:28 UTC
Created attachment 94797 [details] [review]
Bug 23858: Make POST add the Location header on vendors endpoint

According to the coding guidelines, this endpoint should be adding the
Location header on POST actions.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2019-10-25 18:52:30 UTC
Created attachment 94798 [details] [review]
Bug 23858: (follow-up) Return code fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Martin Renvoize 2019-10-28 12:39:14 UTC
Nice work!

Pushed to master for 19.11.00