PUT request to /api/v1/holds/{hold_id} requires the priority parameter (non-mandatory in the API spec) for the request to actually do anything and POST request to /api/v1/holds/{hold_id}/suspension without end_date improperly returns current date as the date. Patch coming up.
Created attachment 99241 [details] [review] 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.
Created attachment 99242 [details] [review] 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.
Will test if I could have a test plan.
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.
Test plan for suspension endpoint: 1. Create a hold and take note of its id 2. Without the patch, do a POST request to /api/v1/holds/{id}/suspension with no body 3. Take note that it returns the current date as end_date. Check in the database that reserves.suspend_until is actually null. 4. Apply the patch and do the POST request again. 5. Verify that the response now contains null as end_date. 6. Make another POST request to /api/v1/holds/{id}/suspension with the following JSON body (use tomorrow as the date): { "end_date": "2020-xx-yy" } 7. Verify that the response reflects the given date. Verify that the database also has the given date in reserves.suspend_until field.
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.)
Created attachment 99476 [details] [review] 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.
Created attachment 99477 [details] [review] 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.
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.
Created attachment 99481 [details] [review] 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>
Created attachment 99482 [details] [review] 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>
(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.
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 : () ), };
Jonathan, unfortunately ModReserve wants them all, and I think that changing it to make the call easier is far more risky.
(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.
Created attachment 99531 [details] [review] 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.
Created attachment 99532 [details] [review] 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.
(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.
I think it also applies to priority as it can be null in DB.
Created attachment 99534 [details] [review] 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>
Created attachment 99535 [details] [review] 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>
(In reply to Jonathan Druart from comment #19) > I think it also applies to priority as it can be null in DB. Setting to Assigned - Ere please check, can go back to Signed off after (with or without follow-up)
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.
(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. There is some workflow stuff in how the status setting works. You need to go to 'needs signoff' first, then you will have 'signed off' in the pull down (assuming noone would need to do the change directly, but sometimes assumptions are wrong.
(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.
Shouldn't rank be RO?
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.
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. I wanted to confirm that with the tests, but then realised that there is no test for PUT and suspend_until..
(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.
Created attachment 99644 [details] [review] 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>
Created attachment 99645 [details] [review] 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> Signed-off-by: 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!
ModReserve I meant ;)
Just saying, but I think it would have been better to fix the weird behavior of ModReserve before this patch. Not blocker however.
Nice work everyone! Pushed to master for 20.05
not backported - does not apply to 19.11.x