Bug 36495 - Add render_resource_not_found() and render_resource_deleted() helpers
Summary: Add render_resource_not_found() and render_resource_deleted() helpers
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 36482 36483
Blocks: 36505
  Show dependency treegraph
 
Reported: 2024-04-02 18:17 UTC by Tomás Cohen Arazi
Modified: 2024-04-26 20:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 36495: Add resource_not_found() and resource_deleted() helpers (7.10 KB, patch)
2024-04-02 18:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Use resource_not_found() helper everywhere (90.96 KB, patch)
2024-04-02 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Use resource_deleted() helper everywhere (23.34 KB, patch)
2024-04-02 19:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Use resource_not_found() helper everywhere (92.13 KB, patch)
2024-04-03 12:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Use resource_deleted() helper everywhere (23.34 KB, patch)
2024-04-03 12:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Trivial tests fixes (3.42 KB, patch)
2024-04-03 13:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Add render_resource_not_found() and render_resource_deleted() helpers (7.19 KB, patch)
2024-04-03 15:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Use render_resource_not_found() helper everywhere (94.66 KB, patch)
2024-04-03 15:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Use render_resource_deleted() helper everywhere (23.16 KB, patch)
2024-04-03 15:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36495: Add render_resource_not_found() and render_resource_deleted() helpers (7.27 KB, patch)
2024-04-17 06:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36495: Use render_resource_not_found() helper everywhere (94.74 KB, patch)
2024-04-17 06:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36495: Use render_resource_deleted() helper everywhere (23.23 KB, patch)
2024-04-17 06:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36495: Add render_resource_not_found() and render_resource_deleted() helpers (7.33 KB, patch)
2024-04-17 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36495: Use render_resource_not_found() helper everywhere (94.80 KB, patch)
2024-04-17 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36495: Use render_resource_deleted() helper everywhere (23.30 KB, patch)
2024-04-17 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36495: Fix erm_counter_files.t (973 bytes, patch)
2024-04-26 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2024-04-02 18:17:26 UTC
In order to get consistent in our response structures I propose to introduce a couple Mojolicious helpers.
Comment 1 Tomás Cohen Arazi 2024-04-02 18:21:32 UTC
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
Comment 2 Tomás Cohen Arazi 2024-04-02 18:35:58 UTC
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
Comment 3 Tomás Cohen Arazi 2024-04-02 19:15:51 UTC
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
Comment 4 Martin Renvoize 2024-04-03 06:06:56 UTC
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.
Comment 5 Martin Renvoize 2024-04-03 06:07:27 UTC
Or... Did we want to go the other way and make them all consistently "Object" instead.. ?
Comment 6 Jonathan Druart 2024-04-03 07:31:02 UTC
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".
Comment 7 Tomás Cohen Arazi 2024-04-03 11:13:57 UTC
(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.
Comment 8 Tomás Cohen Arazi 2024-04-03 12:59:39 UTC
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
Comment 9 Tomás Cohen Arazi 2024-04-03 12:59:41 UTC
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
Comment 10 Tomás Cohen Arazi 2024-04-03 13:11:04 UTC
Created attachment 164380 [details] [review]
Bug 36495: Trivial tests fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2024-04-03 13:12:32 UTC
(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.
Comment 12 Jonathan Druart 2024-04-03 13:19:21 UTC
Sorry for the late idea but wouldn't it make sense to prefix those generic render helper? Like 'render_resource_deleted'.
Comment 13 Tomás Cohen Arazi 2024-04-03 13:34:30 UTC
(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.
Comment 14 Tomás Cohen Arazi 2024-04-03 15:41:07 UTC
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
Comment 15 Tomás Cohen Arazi 2024-04-03 15:41:10 UTC
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>
Comment 16 Tomás Cohen Arazi 2024-04-03 15:41:13 UTC
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
Comment 17 Pedro Amorim 2024-04-04 11:31:22 UTC
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?
Comment 18 Tomás Cohen Arazi 2024-04-10 12:23:36 UTC
(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.
Comment 19 Tomás Cohen Arazi 2024-04-10 12:24:22 UTC
(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?
Comment 20 Martin Renvoize 2024-04-15 17:13:22 UTC
I'm happy with this now as it stands.. as soon as I can get the dependencies all applying I'll add my signature.
Comment 21 Jonathan Druart 2024-04-16 07:40:47 UTC
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.
Comment 22 Tomás Cohen Arazi 2024-04-16 17:43:47 UTC
(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.
Comment 23 Jonathan Druart 2024-04-17 06:44:02 UTC
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>
Comment 24 Jonathan Druart 2024-04-17 06:44:04 UTC
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>
Comment 25 Jonathan Druart 2024-04-17 06:44:07 UTC
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>
Comment 26 Jonathan Druart 2024-04-17 06:44:18 UTC
All good to me, but we need to plan to prevent regressions.
Comment 27 Martin Renvoize 2024-04-17 09:40:11 UTC
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>
Comment 28 Martin Renvoize 2024-04-17 09:40:14 UTC
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>
Comment 29 Martin Renvoize 2024-04-17 09:40:17 UTC
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>
Comment 30 Martin Renvoize 2024-04-17 09:42:42 UTC
Passing QA, this is a great improvement.

We're working on the QA scripts in the background.. discussion already taking place on mattermost.
Comment 31 Katrin Fischer 2024-04-26 16:07:31 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 32 Tomás Cohen Arazi 2024-04-26 20:19:41 UTC
Created attachment 165696 [details] [review]
Bug 36495: Fix erm_counter_files.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2024-04-26 20:23:51 UTC
@RM: I pushed the last follow-up to fix a follow-up :-D