Bug 24680 - Hold modification endpoints don't always work properly
Summary: Hold modification endpoints don't always work properly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Ere Maijala
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-19 09:32 UTC by Ere Maijala
Modified: 2020-11-30 21:48 UTC (History)
8 users (show)

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


Attachments
Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided (3.15 KB, patch)
2020-02-19 09:47 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint (3.09 KB, patch)
2020-02-19 09:47 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided (3.64 KB, patch)
2020-02-24 08:40 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint (3.08 KB, patch)
2020-02-24 08:40 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided (3.68 KB, patch)
2020-02-24 09:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint (3.13 KB, patch)
2020-02-24 09:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided (4.18 KB, patch)
2020-02-24 14:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint (3.08 KB, patch)
2020-02-24 14:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided (4.23 KB, patch)
2020-02-24 17:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint (3.13 KB, patch)
2020-02-24 17:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 24680: Fix PUT api/v1/holds/{hold_id} to work also when priority is not provided (3.82 KB, patch)
2020-02-26 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24680: Fix end_date returned from api/v1/holds/{hold_id}/suspension endpoint (3.20 KB, patch)
2020-02-26 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ere Maijala 2020-02-19 09:32:44 UTC
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.
Comment 1 Ere Maijala 2020-02-19 09:47:48 UTC
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.
Comment 2 Ere Maijala 2020-02-19 09:47:51 UTC
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.
Comment 3 David Nind 2020-02-20 19:56:43 UTC
Will test if I could have a test plan.
Comment 4 Ere Maijala 2020-02-21 08:35:37 UTC
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.
Comment 5 Ere Maijala 2020-02-21 08:41:45 UTC
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.
Comment 6 David Nind 2020-02-22 05:35:34 UTC
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.)
Comment 7 Ere Maijala 2020-02-24 08:40:09 UTC
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.
Comment 8 Ere Maijala 2020-02-24 08:40:12 UTC
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.
Comment 9 Ere Maijala 2020-02-24 08:44:23 UTC
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.
Comment 10 David Nind 2020-02-24 09:26:06 UTC
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>
Comment 11 David Nind 2020-02-24 09:26:10 UTC
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>
Comment 12 David Nind 2020-02-24 09:27:28 UTC
(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.
Comment 13 Jonathan Druart 2020-02-24 11:41:56 UTC
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 : () ),
};
Comment 14 Ere Maijala 2020-02-24 11:44:35 UTC
Jonathan, unfortunately ModReserve wants them all, and I think that changing it to make the call easier is far more risky.
Comment 15 Jonathan Druart 2020-02-24 14:23:34 UTC
(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.
Comment 16 Ere Maijala 2020-02-24 14:50:31 UTC
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.
Comment 17 Ere Maijala 2020-02-24 14:50:35 UTC
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.
Comment 18 Ere Maijala 2020-02-24 14:51:56 UTC
(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.
Comment 19 Jonathan Druart 2020-02-24 14:59:29 UTC
I think it also applies to priority as it can be null in DB.
Comment 20 David Nind 2020-02-24 17:08:58 UTC
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>
Comment 21 David Nind 2020-02-24 17:09:01 UTC
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>
Comment 22 Katrin Fischer 2020-02-24 21:37:19 UTC
(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)
Comment 23 Ere Maijala 2020-02-25 06:45:29 UTC
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.
Comment 24 Katrin Fischer 2020-02-25 07:10:41 UTC
(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.
Comment 25 Jonathan Druart 2020-02-25 11:28:19 UTC
(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.
Comment 26 Tomás Cohen Arazi 2020-02-25 12:01:44 UTC
Shouldn't rank be RO?
Comment 27 Ere Maijala 2020-02-25 12:10:57 UTC
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.
Comment 28 Jonathan Druart 2020-02-26 09:28:07 UTC
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..
Comment 29 Ere Maijala 2020-02-26 09:33:14 UTC
(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.
Comment 30 Jonathan Druart 2020-02-26 14:17:03 UTC
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>
Comment 31 Jonathan Druart 2020-02-26 14:17:06 UTC
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>
Comment 32 Jonathan Druart 2020-02-26 14:17:39 UTC
(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!
Comment 33 Jonathan Druart 2020-02-26 14:17:51 UTC
ModReserve I meant ;)
Comment 34 Jonathan Druart 2020-02-26 14:21:26 UTC
Just saying, but I think it would have been better to fix the weird behavior of ModReserve before this patch. Not blocker however.
Comment 35 Martin Renvoize 2020-02-26 20:43:26 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 36 Joy Nelson 2020-04-01 20:01:24 UTC
not backported - does not apply to 19.11.x