In order to get consistent in our response structures I propose to introduce a couple Mojolicious helpers.
Created attachment 164291 [details] [review] Bug 36495: Add resource_not_found() and resource_deleted() helpers This patch introduces two helpers to be used in controllers. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/responses.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 164297 [details] [review] Bug 36495: Use resource_not_found() helper everywhere This patch makes the existing controllers use the new helpers for (most) 404 situations. The remaining ones are subject to discussion. I proposed to discuss on the next meeting, how to make this more consistent. For reference: ```shell git grep resource_not_found ``` and see the different use cases in the codebase. Once we settle on the matter, I will provide a follow-up to make them all the same. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/ => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 164301 [details] [review] Bug 36495: Use resource_deleted() helper everywhere This patch makes the API controllers use the resource_deleted() helper. No behavior changes are expected. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Nothing broke 3. Sign off :-D
I'd be tempted to "fix" the "Object" cases here too.. currently we're just using the helper as is, but there are a few obvious cases to be where we could upgrade the generic "Object not found" to be more specific and return"Vendor not found" for example.
Or... Did we want to go the other way and make them all consistently "Object" instead.. ?
Yes I agree with Martin, we should adjust "Object". I think we shouldn't use the generic "Object" term. For GET /patrons/{patron_id}/holds/{hold_id} we certainly want to see the difference between "Patron not found" and "Hold not found".
(In reply to Jonathan Druart from comment #6) > Yes I agree with Martin, we should adjust "Object". > I think we shouldn't use the generic "Object" term. For GET > /patrons/{patron_id}/holds/{hold_id} we certainly want to see the difference > between "Patron not found" and "Hold not found". I didn't do it right away so we could discuss it on the dev meeting. I agree we should make them specific. Not sure if it would be an issue that we expose the non existence of a (patron?) id this way. But as we are using ->search_limited, that should be covered (i.e. the consumer wouldn't know what makes is 'not exist'). Side note: I picked 'Resource' instead of object for the fallback case :-P.
Created attachment 164377 [details] [review] Bug 36495: Use resource_not_found() helper everywhere This patch makes the existing controllers use the new helpers for (most) 404 situations. The remaining ones are subject to discussion. I proposed to discuss on the next meeting, how to make this more consistent. For reference: ```shell git grep resource_not_found ``` and see the different use cases in the codebase. Once we settle on the matter, I will provide a follow-up to make them all the same. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/ => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 164378 [details] [review] Bug 36495: Use resource_deleted() helper everywhere This patch makes the API controllers use the resource_deleted() helper. No behavior changes are expected. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Nothing broke 3. Sign off :-D
Created attachment 164380 [details] [review] Bug 36495: Trivial tests fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #6) > Yes I agree with Martin, we should adjust "Object". > I think we shouldn't use the generic "Object" term. For GET > /patrons/{patron_id}/holds/{hold_id} we certainly want to see the difference > between "Patron not found" and "Hold not found". I went ahead and submitted an updated patch, that goes specific on resource descriptions. Please share your thoughts.
Sorry for the late idea but wouldn't it make sense to prefix those generic render helper? Like 'render_resource_deleted'.
(In reply to Jonathan Druart from comment #12) > Sorry for the late idea but wouldn't it make sense to prefix those generic > render helper? Like 'render_resource_deleted'. I would agree.
Created attachment 164382 [details] [review] Bug 36495: Add render_resource_not_found() and render_resource_deleted() helpers This patch introduces two helpers to be used in controllers. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/responses.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 164383 [details] [review] Bug 36495: Use render_resource_not_found() helper everywhere This patch makes the existing controllers use the new helpers for (most) 404 situations. The remaining ones are subject to discussion. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/ => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 164384 [details] [review] Bug 36495: Use render_resource_deleted() helper everywhere This patch makes the API controllers use the render_resource_deleted() helper. No behavior changes are expected. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Nothing broke 3. Sign off :-D
There a few occurrences not updated by the patch: grep --color=always -B2 -rnw Koha/REST/V1 -e "status" | grep 404 examples: Koha/REST/V1/Illbackends.pm Koha/REST/V1/Preservation/Trains.pm Koha/REST/V1/Holds.pm Koha/REST/V1/Biblios/ItemGroups.pm and others We'd need a qa script update for this also, correct?
(In reply to Pedro Amorim from comment #17) > There a few occurrences not updated by the patch: > > grep --color=always -B2 -rnw Koha/REST/V1 -e "status" | grep 404 > > examples: > Koha/REST/V1/Illbackends.pm > Koha/REST/V1/Preservation/Trains.pm > Koha/REST/V1/Holds.pm > Koha/REST/V1/Biblios/ItemGroups.pm > > and others > > We'd need a qa script update for this also, correct? Maybe. Not all 404 cases need to be handled the same way. Some I left out, like the ItemGroups one is because they were returning more useful information than with this helper. Not sure how relevant it is. Can we please move forward with this? There seems to be a bunch of new endpoint patches submitted and it would be great if they were built on top of this.
(In reply to Jonathan Druart from comment #12) > Sorry for the late idea but wouldn't it make sense to prefix those generic > render helper? Like 'render_resource_deleted'. Done, can we move forward?
I'm happy with this now as it stands.. as soon as I can get the dependencies all applying I'll add my signature.
What's the pattern to reject? "status\s*=>\s*404" in Koha/REST/V1? We need a QA script check + unit test in Koha for this change.
(In reply to Jonathan Druart from comment #21) > What's the pattern to reject? "status\s*=>\s*404" in Koha/REST/V1? > > We need a QA script check + unit test in Koha for this change. I had doubts because of this you can read below: (In reply to Tomás Cohen Arazi from comment #18) > > Maybe. Not all 404 cases need to be handled the same way. Some I left out, > like the ItemGroups one is because they were returning more useful > information than with this helper. Not sure how relevant it is.
Created attachment 164971 [details] [review] Bug 36495: Add render_resource_not_found() and render_resource_deleted() helpers This patch introduces two helpers to be used in controllers. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/responses.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164972 [details] [review] Bug 36495: Use render_resource_not_found() helper everywhere This patch makes the existing controllers use the new helpers for (most) 404 situations. The remaining ones are subject to discussion. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/ => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164973 [details] [review] Bug 36495: Use render_resource_deleted() helper everywhere This patch makes the API controllers use the render_resource_deleted() helper. No behavior changes are expected. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Nothing broke 3. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
All good to me, but we need to plan to prevent regressions.
Created attachment 164983 [details] [review] Bug 36495: Add render_resource_not_found() and render_resource_deleted() helpers This patch introduces two helpers to be used in controllers. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/responses.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 164984 [details] [review] Bug 36495: Use render_resource_not_found() helper everywhere This patch makes the existing controllers use the new helpers for (most) 404 situations. The remaining ones are subject to discussion. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/ => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 164985 [details] [review] Bug 36495: Use render_resource_deleted() helper everywhere This patch makes the API controllers use the render_resource_deleted() helper. No behavior changes are expected. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Nothing broke 3. Sign off :-D Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Passing QA, this is a great improvement. We're working on the QA scripts in the background.. discussion already taking place on mattermost.
Pushed for 24.05! Well done everyone, thank you!
Created attachment 165696 [details] [review] Bug 36495: Fix erm_counter_files.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
@RM: I pushed the last follow-up to fix a follow-up :-D
Not backported to 23.11.x
Architecture change, nothing to add/edit in the Koha manual.