Bug 38253 - REST API: Toggle holds lowest priority via REST API
Summary: REST API: Toggle holds lowest priority via REST API
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Johanna Räisä
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords: additional_work_needed
Depends on:
Blocks: 23269
  Show dependency treegraph
 
Reported: 2024-10-24 08:53 UTC by Emmi Takkinen
Modified: 2025-03-17 10:17 UTC (History)
4 users (show)

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


Attachments
Bug 38253: Add toggle holds lowest priority via REST API (6.12 KB, patch)
2024-10-24 09:51 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 38253: Add toggle holds lowest priority via REST API (6.18 KB, patch)
2024-10-24 14:45 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 38253: Add lowest priority REST API endpoint for holds (6.06 KB, patch)
2024-10-29 06:21 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 38253: Add lowest priority REST API endpoint for holds (6.06 KB, patch)
2024-11-01 06:35 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 38253: Add lowest priority REST API endpoint for holds (6.10 KB, patch)
2024-11-01 08:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 38253: Add lowest priority REST API endpoint for holds (6.17 KB, patch)
2025-01-24 12:28 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38253: (follow-up) add missing swagger definition (854 bytes, patch)
2025-02-21 12:09 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 38253: (follow-up) remove irrelevant swagger parameter (1003 bytes, patch)
2025-03-03 08:23 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 38253: (follow-up) add boolean value to body parameter (3.04 KB, patch)
2025-03-17 10:17 UTC, Johanna Räisä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2024-10-24 08:53:42 UTC
We need an ability to toggle holds lowest priority via REST API, either from holds edit endpoint or from separate endpoint (e.g. like /holds/{hold_id}/priority).
Comment 1 Johanna Räisä 2024-10-24 09:51:14 UTC
Created attachment 173260 [details] [review]
Bug 38253: Add toggle holds lowest priority via REST API

This patch adds a new endpoint to toggle the lowest priority of a hold via the REST API.

To test:
1) Apply the patch
2) perl build-resources.PL
3) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Comment 2 Lisette Scheer 2024-10-24 14:45:38 UTC
Created attachment 173279 [details] [review]
Bug 38253: Add toggle holds lowest priority via REST API

This patch adds a new endpoint to toggle the lowest priority of a hold via the REST API.

To test:
1) Apply the patch
2) perl build-resources.PL
3) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 3 Tomás Cohen Arazi (tcohen) 2024-10-25 11:16:48 UTC
I don't think having a verb in the resource path is RESTful, and we should avoid it for consistency.

This can be solved by getting rid of the 'toggle' nature of the route and the method used. For example:

```
PUT /holds/{hold_id}/lowest_priority
true
```

or

```
PUT /holds/{hold_id}/lowest_priority
false
```
Comment 4 Johanna Räisä 2024-10-29 06:21:59 UTC
Created attachment 173602 [details] [review]
Bug 38253: Add lowest priority REST API endpoint for holds

This patch adds a new endpoint to toggle the lowest priority of a hold via the REST API.

To test:
1) Apply the patch
2) perl build-resources.PL
3) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Comment 5 Johanna Räisä 2024-10-29 06:24:00 UTC
I changed it to be just PUT /holds/{hold_id}/lowest_priority
Can you test again, please.
Comment 6 David Nind 2024-10-31 22:08:51 UTC
The tests fail for me (tested using koha-testing-docker). Is there anything that needs to be configured first?

prove t/db_dependent/api/v1/holds.t
t/db_dependent/api/v1/holds.t .. 1/15 No reserves HOLD_CANCELLATION letter transported by email at /kohadevbox/koha/C4/Letters.pm line 610.
t/db_dependent/api/v1/holds.t .. 14/15     # Looks like you planned 5 tests but ran 4.

#   Failed test 'PUT /holds/{hold_id}/lowest_priority tests'
#   at t/db_dependent/api/v1/holds.t line 1629.
The method Koha::Hold->lowest_priority is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 1040
Koha::Object::AUTOLOAD('Koha::Hold=HASH(0x5b0544e7cf28)') called at t/db_dependent/api/v1/holds.t line 1626
main::__ANON__ at /usr/share/perl/5.36/Test/Builder.pm line 374
eval {...} at /usr/share/perl/5.36/Test/Builder.pm line 374
Test::Builder::subtest('Test::Builder=HASH(0x5b05389595c0)', 'PUT /holds/{hold_id}/lowest_priority tests', 'CODE(0x5b05447cc908)') called at /usr/share/perl/5.36/Test/More.pm line 809
Test::More::subtest('PUT /holds/{hold_id}/lowest_priority tests', 'CODE(0x5b05447cc908)') called at t/db_dependent/api/v1/holds.t line 1629
# Looks like your test exited with 11 just after 15.
t/db_dependent/api/v1/holds.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 1/15 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/holds.t (Wstat: 2816 (exited 11) Tests: 15 Failed: 1)
  Failed test:  15
  Non-zero exit status: 11
Files=1, Tests=15, 11 wallclock secs ( 0.04 usr  0.00 sys +  8.77 cusr  1.94 csys = 10.75 CPU)
Result: FAIL
Comment 7 Johanna Räisä 2024-11-01 06:35:08 UTC
Created attachment 173852 [details] [review]
Bug 38253: Add lowest priority REST API endpoint for holds

This patch adds a new endpoint to toggle the lowest priority of a hold via the REST API.

To test:
1) Apply the patch
2) perl build-resources.PL
3) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Comment 8 Johanna Räisä 2024-11-01 06:40:15 UTC
I fixed the "PUT /holds/{hold_id}/lowest_priority tests" subtest, the name in the object had changed. 

I fetched the latest main and I can't see this error, "No reserves HOLD_CANCELLATION letter transported by email".
Comment 9 Katrin Fischer 2024-11-01 07:40:54 UTC
(In reply to Johanna Räisä from comment #8)
> I fixed the "PUT /holds/{hold_id}/lowest_priority tests" subtest, the name
> in the object had changed. 
> 
> I fetched the latest main and I can't see this error, "No reserves
> HOLD_CANCELLATION letter transported by email".

I think we fixed this fail in the meantime as another patch had broken the test.
Comment 10 David Nind 2024-11-01 08:31:32 UTC
Created attachment 173857 [details] [review]
Bug 38253: Add lowest priority REST API endpoint for holds

This patch adds a new endpoint to toggle the lowest priority of a hold via the REST API.

To test:
1) Apply the patch
2) perl build-resources.PL
3) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2024-11-01 08:32:31 UTC
Passed now, and have signed off - thanks for fixing!
Comment 12 Pedro Amorim 2025-01-24 12:28:21 UTC
Created attachment 177113 [details] [review]
Bug 38253: Add lowest priority REST API endpoint for holds

This patch adds a new endpoint to toggle the lowest priority of a hold via the REST API.

To test:
1) Apply the patch
2) perl build-resources.PL
3) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 13 Katrin Fischer 2025-02-20 16:30:56 UTC
I am sorry, this fails the xt/api.t .. 1/6 
    #   Failed test 'No errors in 400 definitions in the spec'
    #   at xt/api.t line 168.
    #          got: '2'
    #     expected: '0'
put /holds/{hold_id}/lowest_priority -> 'description' does not start with 'Bad request': (Default response.)
put /holds/{hold_id}/lowest_priority -> '$ref' is not '#/definitions/error': (#/definitions/DefaultResponse)
    # Looks like you failed 1 test of 1.

Please check.
Comment 14 Johanna Räisä 2025-02-21 10:44:12 UTC
(In reply to Katrin Fischer from comment #13)
> I am sorry, this fails the xt/api.t .. 1/6 
>     #   Failed test 'No errors in 400 definitions in the spec'
>     #   at xt/api.t line 168.
>     #          got: '2'
>     #     expected: '0'
> put /holds/{hold_id}/lowest_priority -> 'description' does not start with
> 'Bad request': (Default response.)
> put /holds/{hold_id}/lowest_priority -> '$ref' is not '#/definitions/error':
> (#/definitions/DefaultResponse)
>     # Looks like you failed 1 test of 1.
> 
> Please check.

I added this on top of main and I didn't get this, the holds.t passes and that xt/api.t. Could this be a build issue?
Comment 15 Katrin Fischer 2025-02-21 11:37:53 UTC
(In reply to Johanna Räisä from comment #14)
> (In reply to Katrin Fischer from comment #13)
> > I am sorry, this fails the xt/api.t .. 1/6 
> >     #   Failed test 'No errors in 400 definitions in the spec'
> >     #   at xt/api.t line 168.
> >     #          got: '2'
> >     #     expected: '0'
> > put /holds/{hold_id}/lowest_priority -> 'description' does not start with
> > 'Bad request': (Default response.)
> > put /holds/{hold_id}/lowest_priority -> '$ref' is not '#/definitions/error':
> > (#/definitions/DefaultResponse)
> >     # Looks like you failed 1 test of 1.
> > 
> > Please check.
> 
> I added this on top of main and I didn't get this, the holds.t passes and
> that xt/api.t. Could this be a build issue?

Did you run yarn build? It will update the specification. Then the test should fail.
Comment 16 Johanna Räisä 2025-02-21 11:41:15 UTC
(In reply to Katrin Fischer from comment #15)
> (In reply to Johanna Räisä from comment #14)
> > (In reply to Katrin Fischer from comment #13)
> > > I am sorry, this fails the xt/api.t .. 1/6 
> > >     #   Failed test 'No errors in 400 definitions in the spec'
> > >     #   at xt/api.t line 168.
> > >     #          got: '2'
> > >     #     expected: '0'
> > > put /holds/{hold_id}/lowest_priority -> 'description' does not start with
> > > 'Bad request': (Default response.)
> > > put /holds/{hold_id}/lowest_priority -> '$ref' is not '#/definitions/error':
> > > (#/definitions/DefaultResponse)
> > >     # Looks like you failed 1 test of 1.
> > > 
> > > Please check.
> > 
> > I added this on top of main and I didn't get this, the holds.t passes and
> > that xt/api.t. Could this be a build issue?
> 
> Did you run yarn build? It will update the specification. Then the test
> should fail.

I did perl build-resources.PL. Should I do something else?
Comment 17 Katrin Fischer 2025-02-21 11:45:52 UTC
(In reply to Johanna Räisä from comment #16)
> (In reply to Katrin Fischer from comment #15)
> > (In reply to Johanna Räisä from comment #14)
> > > (In reply to Katrin Fischer from comment #13)
> > > > I am sorry, this fails the xt/api.t .. 1/6 
> > > >     #   Failed test 'No errors in 400 definitions in the spec'
> > > >     #   at xt/api.t line 168.
> > > >     #          got: '2'
> > > >     #     expected: '0'
> > > > put /holds/{hold_id}/lowest_priority -> 'description' does not start with
> > > > 'Bad request': (Default response.)
> > > > put /holds/{hold_id}/lowest_priority -> '$ref' is not '#/definitions/error':
> > > > (#/definitions/DefaultResponse)
> > > >     # Looks like you failed 1 test of 1.
> > > > 
> > > > Please check.
> > > 
> > > I added this on top of main and I didn't get this, the holds.t passes and
> > > that xt/api.t. Could this be a build issue?
> > 
> > Did you run yarn build? It will update the specification. Then the test
> > should fail.
> 
> I did perl build-resources.PL. Should I do something else?

I have never used build-resources.PL - I doubt it does the same. Please use yarn build (there is a yarn... something for the API only, but this should take care of all eventualities).
Comment 18 Johanna Räisä 2025-02-21 12:09:49 UTC
Created attachment 178483 [details] [review]
Bug 38253: (follow-up) add missing swagger definition

This follow-up fixes the missing definition for 400 response.
Comment 19 Katrin Fischer 2025-02-21 13:48:47 UTC
Happy to treat this as a follow-up - maybe Tomas can have a quick last look.
Comment 20 Katrin Fischer 2025-02-26 12:19:45 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 21 Katrin Fischer 2025-02-26 12:24:06 UTC
Hi Johanna,

now it's pushed (seconds after...) I was alerted of another thing that needs fixing: This should not be an Integer, but a Boolean. Can you please check? 

Also adding Martin as he can maybe explain better and for another set of eyes.
Comment 22 Johanna Räisä 2025-03-03 08:23:05 UTC
Created attachment 178890 [details] [review]
Bug 38253: (follow-up) remove irrelevant swagger parameter

Thanks for informing, that part was actually irrelevant! The toggle is made only with hold_id, so no additional parameters are needed in this endpoint.

I added a follow up to remove the parameter from swagger.
Comment 23 Tomás Cohen Arazi (tcohen) 2025-03-05 13:42:56 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #3)
> I don't think having a verb in the resource path is RESTful, and we should
> avoid it for consistency.
> 
> This can be solved by getting rid of the 'toggle' nature of the route and
> the method used. For example:
> 
> ```
> PUT /holds/{hold_id}/lowest_priority
> true
> ```
> 
> or
> 
> ```
> PUT /holds/{hold_id}/lowest_priority
> false
> ```

This is not what's been implemented. And it should.
Comment 24 Katrin Fischer 2025-03-05 14:31:08 UTC
I'll hold off with pushing the follow-up for now until the open questions have been resolved.
Comment 25 Johanna Räisä 2025-03-06 06:05:05 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #23)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #3)
> > I don't think having a verb in the resource path is RESTful, and we should
> > avoid it for consistency.
> > 
> > This can be solved by getting rid of the 'toggle' nature of the route and
> > the method used. For example:
> > 
> > ```
> > PUT /holds/{hold_id}/lowest_priority
> > true
> > ```
> > 
> > or
> > 
> > ```
> > PUT /holds/{hold_id}/lowest_priority
> > false
> > ```
> 
> This is not what's been implemented. And it should.

I made it similar as in request.pl. It uses C4::Reserves::ToggleLowestPriority($hold_id), so only the hold_id is needed. 

I can change it so the boolean value is required if that is the case.
Comment 26 Johanna Räisä 2025-03-17 10:17:59 UTC
Created attachment 179380 [details] [review]
Bug 38253: (follow-up) add boolean value to body parameter

This patch adds the boolean value to the body parameter to change the status of the lowest priority hold.