https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #1 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 99241 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99241&action=edit Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided Before this fix the endpoint would accept the request but fail to actually update the hold if the request does not contain a priority parameter. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi, | |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #4 from Ere Maijala <ere.maijala@helsinki.fi> --- Test plan for no priority provided: 1. Create a hold (you can use the UI or API for it, doesn't matter) and take note of its id and pickup location. 2. Find branchcode for another pickup location. 3. Without the patch, do a PUT request to /api/v1/holds/{id} with the following JSON: { "pickup_library_id":"<another pickup location>" } 4. Check the response. It still has the original location pickup_library_id. Check the hold in the UI, it still has the original pickup location. 5. Apply the patch and do the PUT request again. 6. Check the response. It should now reflect the new location in pickup_library_id. Check the hold in the UI, it should also have the new pickup location. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #6 from David Nind <david@davidnind.com> --- The first patch no longer applies: Bug 24680 - Hold modification endpoints don't always work properly 99241 - Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided 99242 - Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided error: sha1 information is lacking or useless (t/db_dependent/api/v1/holds.t). error: could not build fake ancestor Patch failed at 0001 Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided The second patch works as per the test plan. (For my information - Figured out how to test: installed Insomnia REST client, enabled RESTPublicAPI and RESTBasicAuth system preferences. This is all new to me, so any suggestions on the appropriate tools to use would be appreciated.) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99241|0 |1 is obsolete| | --- Comment #7 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 99476 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99476&action=edit Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided Before this fix the endpoint would accept the request but fail to actually update the hold if the request does not contain a priority parameter. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #9 from Ere Maijala <ere.maijala@helsinki.fi> --- Oops, sorry. I had some remnants of another fix there. Should apply properly now. For tools, I use RESTer plugin for Firefox (it's mentioned in bug 20402). It supports also OAuth2 with Koha. For this bug it doesn't really matter, though, so whatever client that can send a proper request is fine. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99476|0 |1 is obsolete| | --- Comment #10 from David Nind <david@davidnind.com> --- Created attachment 99481 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99481&action=edit Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided Before this fix the endpoint would accept the request but fail to actually update the hold if the request does not contain a priority parameter. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #12 from David Nind <david@davidnind.com> --- (In reply to Ere Maijala from comment #9)

For tools, I use RESTer plugin for Firefox (it's mentioned in bug 20402). It supports also OAuth2 with Koha. For this bug it doesn't really matter, though, so whatever client that can send a proper request is fine.

Thanks! All tested and signed-off. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Ere, do not you think we should set the values only if passed, instead? Like: my $params = { id => $id, ( defined $foo ? foo => $foo : () ), ( defined $bar ? bar => $bar : () ), }; -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Ere Maijala from comment #14)

Jonathan, unfortunately ModReserve wants them all, and I think that changing it to make the call easier is far more risky.

Indeed! However, should not we test for "exists" instead of "defined" (//)? As it, it will be impossible to set suspend_until to NULL, which I think is something we want to. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99481|0 |1 is obsolete| | --- Comment #16 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 99531 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99531&action=edit Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided Before this fix the endpoint would accept the request but fail to actually update the hold if the request does not contain a priority parameter. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #18 from Ere Maijala <ere.maijala@helsinki.fi> --- (In reply to Jonathan Druart from comment #15)

(In reply to Ere Maijala from comment #14)

...

Jonathan, unfortunately ModReserve wants them all, and I think that changing it to make the call easier is far more risky.

Indeed!

However, should not we test for "exists" instead of "defined" (//)? As it, it will be impossible to set suspend_until to NULL, which I think is something we want to.

Right, that was indeed broken (in a couple of ways). I don't think this applies to the other parameters, so I didn't change them. I also added a test to verify setting suspended_until to null. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 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=24680 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99532|0 |1 is obsolete| | --- Comment #21 from David Nind <david@davidnind.com> --- Created attachment 99535 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99535&action=edit Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint Before this patch the response would return current date as the suspension end date for a hold that is suspended with no end date. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #23 from Ere Maijala <ere.maijala@helsinki.fi> --- The API spec does not allow null value, and ModReserve would not do anything if rank is not set to a number > 0. Thus I believe this is how it should be. Unfortunately I can't set this back to signed off, the option is not available for me. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=24722 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Ere Maijala from comment #23)

The API spec does not allow null value, and ModReserve would not do anything if rank is not set to a number > 0. Thus I believe this is how it should be. Unfortunately I can't set this back to signed off, the option is not available for me.

Indeed, the constraint is wrong at DB level. I opened bug 24722. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #27 from Ere Maijala <ere.maijala@helsinki.fi> --- It's not in ModReserve or the API spec. I can't really say if it should be. Regardless of that, it must be passed to ModReserve for it to do _any_ modifications at all. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #29 from Ere Maijala <ere.maijala@helsinki.fi> --- (In reply to Jonathan Druart from comment #28)

About suspend_until:

+ my $suspended_until = exists $body->{suspended_until} ? $body->{suspended_until} : $hold->suspend_until;

then

+ suspend_until => $suspended_until ? output_pref(dt_from_string($suspended_until, 'rfc3339')) : '',

last '' must be undef I'd say.

No, ModReserve needs it to be defined, otherwise it won't clear out any old value. It is a peculiar little function indeed. The test block starting with comment "Reset suspended_until, everything else should remain" tests this. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99534|0 |1 is obsolete| | Attachment #99535|0 |1 is obsolete| | --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 99644 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99644&action=edit Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided Before this fix the endpoint would accept the request but fail to actually update the hold if the request does not contain a priority parameter. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Ere Maijala from comment #29)

(In reply to Jonathan Druart from comment #28)

...

About suspend_until:

+ my $suspended_until = exists $body->{suspended_until} ? $body->{suspended_until} : $hold->suspend_until;

then

+ suspend_until => $suspended_until ? output_pref(dt_from_string($suspended_until, 'rfc3339')) : '',

last '' must be undef I'd say.

No, ModReserve needs it to be defined, otherwise it won't clear out any old value. It is a peculiar little function indeed. The test block starting with comment "Reset suspended_until, everything else should remain" tests this.

Erk yes, indeed. That's pretty ugly! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Just saying, but I think it would have been better to fix the weird behavior of ModReserve before this patch. Not blocker however. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24680 --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.