https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #1 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- Created attachment 173260 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173260&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23269 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23269 [Bug 23269] Long hold queues are slowing the service -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173260|0 |1 is obsolete| | --- Comment #2 from Lisette Scheer <lisette@bywatersolutions.com> --- Created attachment 173279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173279&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173279|0 |1 is obsolete| | --- Comment #4 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- Created attachment 173602 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173602&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs Signoff |Failed QA --- Comment #6 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #8 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- 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". -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #11 from David Nind <david@davidnind.com> --- Passed now, and have signed off - thanks for fixing! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173857|0 |1 is obsolete| | --- Comment #12 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 177113 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177113&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #14 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #16 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #18 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- Created attachment 178483 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178483&action=edit Bug 38253: (follow-up) add missing swagger definition This follow-up fixes the missing definition for 400 response. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Happy to treat this as a follow-up - maybe Tomas can have a quick last look. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |25.05.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #21 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #23 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #25 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #27 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Johanna Räisä from comment #26)

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.

It looks better, but what would be the semantics for `false` being passed? It looks like the code still sets the lowest priority? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 --- Comment #29 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #28)

(In reply to Tomás Cohen Arazi (tcohen) from comment #27)

...

(In reply to Johanna Räisä from comment #26)

...

Created attachment 179380 [details] [review] [review] [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.

It looks better, but what would be the semantics for `false` being passed? It looks like the code still sets the lowest priority?

It should restore the priority I'd say.

Sent before finishing my reply.

I think you are right but at the moment the function which is handling this in pl-file is like this. =head2 ToggleLowestPriority ToggleLowestPriority( $borrowernumber, $biblionumber ); This function sets the lowestPriority field to true if is false, and false if it is true. =cut sub ToggleLowestPriority { my ($reserve_id) = @_; my $dbh = C4::Context->dbh; my $sth = $dbh->prepare("UPDATE reserves SET lowestPriority = NOT lowestPriority WHERE reserve_id = ?"); $sth->execute($reserve_id); _FixPriority( { reserve_id => $reserve_id, rank => '999999' } ); } When the toggle is made it will always set rank as 9999. Not sure why it is made like that, maybe that part should be fixed? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #31 from Fridolin Somers <fridolin.somers@biblibre.com> --- additional work needed ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38253 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.