|
Description
Tomás Cohen Arazi (tcohen)
2021-01-04 13:42:55 UTC
Created attachment 114798 [details] [review] Bug 27330: Regression tests Created attachment 114799 [details] [review] Bug 27330: Return 403 if club has no enrollments This patch makes the controller script return a 403 as it should [1] instead of a plain 500. To test: 1. Apply the regressions tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D [1] https://www.restapitutorial.com/httpstatuscodes.html Created attachment 114800 [details] [review] Bug 27330: (follow-up) Cleanup missed on bug 23843 This patch makes some cleanup missed on bug 23843. The catch condition on the controller was copied and pasted from other controller and should be avoided unless there's a known case that needs special handling. Otherwise it will be catched by the $c->unhandled_exception call. All the old mappings were inadvertedly kept in the controller as well as the (unused) _to_api method. The base class is also cleaned from unnecessary mappings. Only mapped things need to be added. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/Koha/Club/Hold.t => SUCCESS: Tests pass! 3. Sign off :-D This feels like it could be a `409 Conflict` response.. but I'm not really sure if it wins us much doing that rather than a `403 Forbidden`... Either way, I think the 4XX is correct, this is a client pre-condition issue.. as in patrons should have been added to a club prior to a hold being placed. Hmm, tests conflict with dependencies so this doesn't apply nicely.. I'll wait for those to get pushed before returning to this one.. After re-reading the original tutorial I rely on for HTTP status codes [1] I think I now read it the same way you do: the user can add the patrons and THEN re-submit. So 409 makes perfect sense. My original interpretation was that some change could be made to the request for resubmitting it. I agree with 409! [1] https://www.restapitutorial.com/httpstatuscodes.html Created attachment 115863 [details] [review] Bug 27330: 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 115864 [details] [review] Bug 27330: Regression tests Created attachment 115865 [details] [review] Bug 27330: Return 403 if club has no enrollments This patch makes the controller script return a 403 as it should [1] instead of a plain 500. To test: 1. Apply the regressions tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D [1] https://www.restapitutorial.com/httpstatuscodes.html Created attachment 115866 [details] [review] Bug 27330: (follow-up) Cleanup missed on bug 23843 This patch makes some cleanup missed on bug 23843. The catch condition on the controller was copied and pasted from other controller and should be avoided unless there's a known case that needs special handling. Otherwise it will be catched by the $c->unhandled_exception call. All the old mappings were inadvertedly kept in the controller as well as the (unused) _to_api method. The base class is also cleaned from unnecessary mappings. Only mapped things need to be added. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/Koha/Club/Hold.t => SUCCESS: Tests pass! 3. Sign off :-D Created attachment 115867 [details] [review] Bug 27330: (QA follow-up) Return 409 instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Martin Renvoize from comment #5) > Hmm, tests conflict with dependencies so this doesn't apply nicely.. I'll > wait for those to get pushed before returning to this one.. This was missing a dependency (bug 23857) that changed how the tests for this feature are written. As that bug will take a while, I moved the (needed) refactoring here, and added a follow-up for returning 409 instead. Thanks for the valuable feedback. Created attachment 115968 [details] [review] Bug 27330: 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: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 115969 [details] [review] Bug 27330: Regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 115970 [details] [review] Bug 27330: Return 403 if club has no enrollments This patch makes the controller script return a 403 as it should [1] instead of a plain 500. To test: 1. Apply the regressions tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D [1] https://www.restapitutorial.com/httpstatuscodes.html Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 115971 [details] [review] Bug 27330: (follow-up) Cleanup missed on bug 23843 This patch makes some cleanup missed on bug 23843. The catch condition on the controller was copied and pasted from other controller and should be avoided unless there's a known case that needs special handling. Otherwise it will be catched by the $c->unhandled_exception call. All the old mappings were inadvertedly kept in the controller as well as the (unused) _to_api method. The base class is also cleaned from unnecessary mappings. Only mapped things need to be added. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/Koha/Club/Hold.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 115972 [details] [review] Bug 27330: (QA follow-up) Return 409 instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Works as expected, thanks Tomas :) Signing off Created attachment 116109 [details] [review] Bug 27330: 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: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 116110 [details] [review] Bug 27330: Regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 116111 [details] [review] Bug 27330: Return 403 if club has no enrollments This patch makes the controller script return a 403 as it should [1] instead of a plain 500. To test: 1. Apply the regressions tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D [1] https://www.restapitutorial.com/httpstatuscodes.html Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 116112 [details] [review] Bug 27330: (follow-up) Cleanup missed on bug 23843 This patch makes some cleanup missed on bug 23843. The catch condition on the controller was copied and pasted from other controller and should be avoided unless there's a known case that needs special handling. Otherwise it will be catched by the $c->unhandled_exception call. All the old mappings were inadvertedly kept in the controller as well as the (unused) _to_api method. The base class is also cleaned from unnecessary mappings. Only mapped things need to be added. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/clubs_holds.t \ t/db_dependent/Koha/Club/Hold.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 116113 [details] [review] Bug 27330: (QA follow-up) Return 409 instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I've verified the tests pass and taken a look at the code, nothing obvious (to me) stood out. Trusting Martin with the functional testing here. I've verified the tests pass and taken a look at the code, nothing obvious (to me) stood out. Trusting Martin with the functional testing here. (In reply to Katrin Fischer from comment #19) > Created attachment 116109 [details] [review] [review] > Bug 27330: Use Basic auth in tests > > This patch puts the tests in line with the current codebase. Can we fix all the other tests as well please? (On its own bug report of course) (In reply to Jonathan Druart from comment #26) > (In reply to Katrin Fischer from comment #19) > > Created attachment 116109 [details] [review] [review] [review] > > Bug 27330: Use Basic auth in tests > > > > This patch puts the tests in line with the current codebase. > > Can we fix all the other tests as well please? I think it is only: - acquisitions_vendors.t - patrons_accounts.t - patrons_password.t - stockrotationstage.t + unless ($item) {
+ return $c->render(
+ status => 404,
+ openapi => { error => "item_id not found." }
+ );
+ }
[...]
+ elsif ($biblio_id) {
+ $biblio = Koha::Biblios->find($biblio_id);
+ }
+ else {
+ return $c->render(
+ status => 400,
+ openapi => { error => "At least one of biblio_id, item_id should be given" }
+ );
Is that correct? (404 vs 400)
(In reply to Tomás Cohen Arazi from comment #28) > (In reply to Jonathan Druart from comment #26) > > (In reply to Katrin Fischer from comment #19) > > > Created attachment 116109 [details] [review] [review] [review] [review] > > > Bug 27330: Use Basic auth in tests > > > > > > This patch puts the tests in line with the current codebase. > > > > Can we fix all the other tests as well please? > > I think it is only: > > - acquisitions_vendors.t > - patrons_accounts.t > - patrons_password.t > - stockrotationstage.t "only", so you prefer to fix them here? Pushed to master for 21.05, thanks to everybody involved! (In reply to Jonathan Druart from comment #29) > + unless ($item) { > + return $c->render( > + status => 404, > + openapi => { error => "item_id not found." } > + ); > + } > > [...] > > + elsif ($biblio_id) { > + $biblio = Koha::Biblios->find($biblio_id); > + } > + else { > + return $c->render( > + status => 400, > + openapi => { error => "At least one of biblio_id, item_id > should be given" } > + ); > > > Is that correct? (404 vs 400) Wrong line, but good catch :-D Fixed on bug 27593. (In reply to Jonathan Druart from comment #30) > (In reply to Tomás Cohen Arazi from comment #28) > > (In reply to Jonathan Druart from comment #26) > > > (In reply to Katrin Fischer from comment #19) > > > > Created attachment 116109 [details] [review] [review] [review] [review] [review] > > > > Bug 27330: Use Basic auth in tests > > > > > > > > This patch puts the tests in line with the current codebase. > > > > > > Can we fix all the other tests as well please? > > > > I think it is only: > > > > - acquisitions_vendors.t > > - patrons_accounts.t > > - patrons_password.t > > - stockrotationstage.t > > "only", so you prefer to fix them here? Patches for this are a bit lengthy. Handled on bug 27330. Can this be backported to 20.11.x ? (In reply to Fridolin Somers from comment #34) > Can this be backported to 20.11.x ? Yes. Pushed to 20.11.x for 20.11.04 Pushed to 20.05.x for 20.05.10 Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed. |