For some reasons I POSTed an invalid biblio to /api/v1/biblios it returned {id: null} The biblio record is not created, but the response is 200. 707 my ( $biblio_id ) = AddBiblio( $record, $frameworkcode, { record_source_id => $record_source_id } ); 708 709 $c->render( 710 status => 200, 711 openapi => { id => $biblio_id } 712 ); Obviously we should test $biblio_id
Created attachment 176795 [details] [review] Bug 38926: Regression tests
Created attachment 176796 [details] [review] Bug 38926: Make POST /biblios return 400 if AddBiblio fails We don't have proper exceptions in the `C4::Biblio::AddBiblio` method, but we at least know `$biblio_id` will be `undef` in the even of an error processing the call. This patch makes the controller handle this situation so (at least) it is obvious that something bad happened. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail! The endpoint returns 200 even on error! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable message! 5. Sign off :-D
should not we list the error_code in the spec?
Created attachment 176835 [details] [review] Bug 38926: Make POST /biblios return 400 if AddBiblio fails We don't have proper exceptions in the `C4::Biblio::AddBiblio` method, but we at least know `$biblio_id` will be `undef` in the even of an error processing the call. This patch makes the controller handle this situation so (at least) it is obvious that something bad happened. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail! The endpoint returns 200 even on error! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable message! 5. Sign off :-D
(In reply to Jonathan Druart from comment #3) > should not we list the error_code in the spec? Good catch! Fixed. Thanks!
Created attachment 176847 [details] [review] Bug 38926: Regression tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 176848 [details] [review] Bug 38926: Make POST /biblios return 400 if AddBiblio fails We don't have proper exceptions in the `C4::Biblio::AddBiblio` method, but we at least know `$biblio_id` will be `undef` in the even of an error processing the call. This patch makes the controller handle this situation so (at least) it is obvious that something bad happened. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail! The endpoint returns 200 even on error! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable message! 5. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 176890 [details] [review] Bug 38926: Regression tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 176891 [details] [review] Bug 38926: Make POST /biblios return 400 if AddBiblio fails We don't have proper exceptions in the `C4::Biblio::AddBiblio` method, but we at least know `$biblio_id` will be `undef` in the even of an error processing the call. This patch makes the controller handle this situation so (at least) it is obvious that something bad happened. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail! The endpoint returns 200 even on error! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! The endpoint returns a 400 with a reasonable message! 5. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: David Nind <david@davidnind.com>
Have added my signoff, so maybe Jonathan's can count as QA.
Pushed for 25.05! Well done everyone, thank you!