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).
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
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>
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 ```
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
I changed it to be just PUT /holds/{hold_id}/lowest_priority Can you test again, please.
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
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
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".
(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.
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>
Passed now, and have signed off - thanks for fixing!
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>
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.
(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?
(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.
(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?
(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).
Created attachment 178483 [details] [review] Bug 38253: (follow-up) add missing swagger definition This follow-up fixes the missing definition for 400 response.
Happy to treat this as a follow-up - maybe Tomas can have a quick last look.
Pushed for 25.05! Well done everyone, thank you!
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.
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.
(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'll hold off with pushing the follow-up for now until the open questions have been resolved.
(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.
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.