|
Description
Tomás Cohen Arazi (tcohen)
2019-10-21 17:00:13 UTC
Created attachment 114778 [details] [review] Bug 23857: Regression tests Created attachment 114779 [details] [review] Bug 23857: Make adding club holds set the Location header Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Resubmitting, test instructions missing. Created attachment 114782 [details] [review] Bug 23857: Regression tests 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> 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> Created attachment 115789 [details] [review] Bug 23857: Regression tests Signed-off-by: David Nind <david@davidnind.com> 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> 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> 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. 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. 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. |