Created attachment 93172 [details] [review] Bug 23677: OpenAPI spec for GET /biblios/{biblio_id}
Created attachment 93173 [details] [review] Bug 23677: Schema fix
Created attachment 93174 [details] [review] Bug 23677: Controller method and dependencies tweak
Created attachment 93175 [details] [review] Bug 23677: Unit tests This patch introduces tests for the GET /biblios/{biblio_id} endpoint. It tries to cover all the use cases. To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! - Use Postman (or your favourite tool) to test the API => SUCCESS: Works as expected! - Sign off :-D
I submit this work to gather opinions. I've already thought about extending this with plugins for handling special serialization formats and schemas. This is just the start
I think it would be wise to submit support for MARC in JSON format in a separate patch after this is merged. This is because MARC::Record::MiJ needs to be first packaged for Debian and that might take a while.
(In reply to Joonas Kylmälä from comment #6) > I think it would be wise to submit support for MARC in JSON format in a > separate patch after this is merged. This is because MARC::Record::MiJ needs > to be first packaged for Debian and that might take a while. It is already packaged. https://packages.debian.org/stretch/libmarc-file-mij-perl
(In reply to Tomás Cohen Arazi from comment #7) > It is already packaged. > > https://packages.debian.org/stretch/libmarc-file-mij-perl Sorry about that, I just searched for MARC::Record::MiJ and couldn't find it.
It might not be pulled by default. As Catmandu is pulled when you install koha-elasticsearch I recall. But this patch adds as a core dependency so next koha-common builds would pull it! Thanks for looking at my patches
Hi Tomas, Here are the results for the qa-tools, over than that everything (unit test and manual testing) working perfectly thanks! curl http://localhost:8080/api/v1/biblios/4 \ -H 'Accept: application/json' > Gives back expected results :) testing 4 commit(s) (applied to 98e4b5c '4a Bug 23537: Overdrive won't show co') Processing files before patches |========================>| 4 / 4 (100.00%) Processing files after patches |========================>| 4 / 4 (100.00%) OK C4/Installer/PerlDependencies.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage OK spelling OK valid FAIL Koha/REST/V1/Biblios.pm FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). OK forbidden patterns OK git manipulation OK pod OK pod coverage OK spelling OK valid OK Koha/Schema/Result/Biblio.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage OK spelling OK valid OK t/db_dependent/api/v1/biblios.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid
Created attachment 93622 [details] [review] Bug 23677: Controller method and dependencies tweak
Created attachment 93623 [details] [review] Bug 23677: Unit tests This patch introduces tests for the GET /biblios/{biblio_id} endpoint. It tries to cover all the use cases. To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! - Use Postman (or your favourite tool) to test the API => SUCCESS: Works as expected! - Sign off :-D
(In reply to Arthur Suzuki from comment #10) > Hi Tomas, > Here are the results for the qa-tools, over than that everything (unit test > and manual testing) working perfectly thanks! > > curl http://localhost:8080/api/v1/biblios/4 \ -H 'Accept: application/json' > > > Gives back expected results :) Awesome! Thanks! > FAIL Koha/REST/V1/Biblios.pm > FAIL critic > # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). Fixed!
Created attachment 93679 [details] [review] Bug 23677: OpenAPI spec for GET /biblios/{biblio_id} Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Created attachment 93680 [details] [review] Bug 23677: Schema fix Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Created attachment 93681 [details] [review] Bug 23677: Controller method and dependencies tweak Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Created attachment 93682 [details] [review] Bug 23677: Unit tests This patch introduces tests for the GET /biblios/{biblio_id} endpoint. It tries to cover all the use cases. To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! - Use Postman (or your favourite tool) to test the API => SUCCESS: Works as expected! - Sign off :-D Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Created attachment 93750 [details] [review] Bug 23677: OpenAPI spec for GET /biblios/{biblio_id} Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 93751 [details] [review] Bug 23677: Schema fix Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 93752 [details] [review] Bug 23677: Controller method and dependencies tweak Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 93753 [details] [review] Bug 23677: Unit tests This patch introduces tests for the GET /biblios/{biblio_id} endpoint. It tries to cover all the use cases. To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => SUCCESS: Tests pass! - Use Postman (or your favourite tool) to test the API => SUCCESS: Works as expected! - Sign off :-D Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work! Pushed to master for 19.11.00
I used this new route on bug 21232. I am if it is ok to request the accept in the header (by default it is */*). Should not we default to application/json?
+1
(In reply to Jonathan Druart from comment #23) > I used this new route on bug 21232. I am if it is ok to request the accept > in the header (by default it is */*). > Should not we default to application/json? I didn't implement it like that, because I had plans of making HTML the default (i.e. the normal, tt based view). But it's a fair request. I can do it if you file a bug.
It seems that other routes (all? I did not check) are already defaulting to json. Is it correct? If so it would make sense to continue that way.