Bug 30661 - Able to update more hold parameters via REST API
Summary: Able to update more hold parameters via REST API
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Johanna Räisä
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 23269
  Show dependency treegraph
 
Reported: 2022-05-02 07:29 UTC by Johanna Räisä
Modified: 2023-06-30 00:07 UTC (History)
6 users (show)

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


Attachments
Bug 30661: Allow to update more hold parameters via REST (4.61 KB, patch)
2022-08-30 09:15 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 30661: [FOLLOW-UP] add separate endpoints (12.24 KB, patch)
2022-08-30 09:16 UTC, Johanna Räisä
Details | Diff | Splinter Review
Changing the hold date of an existing hold (176.83 KB, image/png)
2022-09-06 11:12 UTC, Owen Leonard
Details
Bug 30661: [FOLLOW-UP] add separate endpoints (12.48 KB, patch)
2022-09-09 05:33 UTC, Johanna Räisä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Johanna Räisä 2022-05-02 07:29:36 UTC
If the holds edit endpoint is wanted to use more effectively then it needs more parameters than priority, pickup_library and suspend_until. 

Is there a plan for PUT action since it has acted as PATCH and is now deprecated? There definitely should be also a real PUT endpoint as documented here https://wiki.koha-community.org/wiki/Holds_endpoint_RFC, now the situation is quite misleading.
Comment 1 Johanna Räisä 2022-08-30 09:15:46 UTC
Created attachment 139973 [details] [review]
Bug 30661: Allow to update more hold parameters via REST

This patch adds hold_date and expiration_date to holds edit endpoint

Test plan:

1) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Comment 2 Johanna Räisä 2022-08-30 09:16:52 UTC
Created attachment 139974 [details] [review]
Bug 30661: [FOLLOW-UP] add separate endpoints

This patch adds separate endpoints for updating hold_date and expiration_date.

Test plan:
1) prove t/db_dependent/api/v1/holds.t

Sponsored-by: Koha-Suomi Oy
Comment 3 Joonas Kylmälä 2022-08-30 17:32:58 UTC
Is it right to allow changing the reservedate after it has been set as you cannot do that now either through staff or opac as far as I know? Did you intend to use this to change the reservedate for a hold with reservedate in the future? Other than that I cannot think of any use case for this. We would have to probably add some checks to make sure the API user is only able to edit reservedate in the future, not one that has already passed?
Comment 4 Owen Leonard 2022-09-02 16:37:06 UTC
(In reply to Joonas Kylmälä from comment #3)
> Is it right to allow changing the reservedate after it has been set as you
> cannot do that now either through staff or opac as far as I know?

You can update reservedate from the hold details page, e.g. /cgi-bin/koha/reserve/request.pl?biblionumber=XXX
Comment 5 Joonas Kylmälä 2022-09-02 16:51:38 UTC
(In reply to Owen Leonard from comment #4)
> (In reply to Joonas Kylmälä from comment #3)
> > Is it right to allow changing the reservedate after it has been set as you
> > cannot do that now either through staff or opac as far as I know?
> 
> You can update reservedate from the hold details page, e.g.
> /cgi-bin/koha/reserve/request.pl?biblionumber=XXX

How? I don't see such option.
Comment 6 Owen Leonard 2022-09-06 11:12:17 UTC
Created attachment 140221 [details]
Changing the hold date of an existing hold
Comment 7 Johanna Räisä 2022-09-06 12:15:30 UTC
If we want to start using API in the frontend then we need these endpoints. Creating a pagination to holds is impossible, so I will use the endpoints for this bug: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23269
Comment 8 Joonas Kylmälä 2022-09-06 16:37:53 UTC
(In reply to Owen Leonard from comment #6)
> Created attachment 140221 [details]
> Changing the hold date of an existing hold

Thanks, I figured it out, need to enable AllowHoldDateInFuture syspref in order for the date to be editable. It seems to be possible also to change the date back in the past through staff interface so having it being possible through API is not a problem. Although I would consider it a bug but it can be handled in a separate report.
Comment 9 Joonas Kylmälä 2022-09-06 20:52:43 UTC
1. The API seems to break after applying these patches:

[ERROR] Warning: Could not load REST API spec bundle: Unable to load schema "file:///kohadevbox/koha/api/v1/swagger/parameters.yaml". at /usr/share/perl5/JSON/Validator/Store.pm line 53.
 Koha::REST::V1::catch {...}  /kohadevbox/koha/Koha/REST/V1.pm (130)

Cannot do any API requests.

2. Going back to the reservedate modification through API. There needs to be a check to disallow changing reservedate if the reserve has been already found. Actually reading the ModReserve code it says:

>    # The only column that can be updated for a found hold is the expiration date
>     $hold->expirationdate($date)->store();

The current API for editing hold already does these checks as it calls ModReserve. So I'm thinking either the new API endpoints need to call ModReserve too or the checks from ModReserve need to be duplicated to to the API functions. I think it would be better to call ModReserve as then we don't risk only changing the conditions only either in the API side or ModReserve side.
Comment 10 Johanna Räisä 2022-09-07 07:03:02 UTC
(In reply to Joonas Kylmälä from comment #9)
> 1. The API seems to break after applying these patches:
> 
> [ERROR] Warning: Could not load REST API spec bundle: Unable to load schema
> "file:///kohadevbox/koha/api/v1/swagger/parameters.yaml". at
> /usr/share/perl5/JSON/Validator/Store.pm line 53.
>  Koha::REST::V1::catch {...}  /kohadevbox/koha/Koha/REST/V1.pm (130)

Is your test environment updated? The parameters.yaml doesn't exist anymore, it is replaced with swagger.yaml.

> 
> Cannot do any API requests.
> 
> 2. Going back to the reservedate modification through API. There needs to be
> a check to disallow changing reservedate if the reserve has been already
> found. Actually reading the ModReserve code it says:
> 
> >    # The only column that can be updated for a found hold is the expiration date
> >     $hold->expirationdate($date)->store();
> 
> The current API for editing hold already does these checks as it calls
> ModReserve. So I'm thinking either the new API endpoints need to call
> ModReserve too or the checks from ModReserve need to be duplicated to to the
> API functions. I think it would be better to call ModReserve as then we
> don't risk only changing the conditions only either in the API side or
> ModReserve side.

I will check this out.
Comment 11 Joonas Kylmälä 2022-09-07 15:58:49 UTC
(In reply to Johanna Räisä from comment #10)
> Is your test environment updated? The parameters.yaml doesn't exist anymore,
> it is replaced with swagger.yaml.

It is updated. The second patch refers to parameters.yaml.
Comment 12 Johanna Räisä 2022-09-09 05:33:37 UTC
Created attachment 140359 [details] [review]
Bug 30661: [FOLLOW-UP] add separate endpoints

Here is the fixed follow-up patch.
Comment 13 David Nind 2022-10-31 00:57:02 UTC
The tests fail.

Also, I think the format for the follow-up in the bug title is Bug XXXX: (follow-up) ...

prove t/db_dependent/api/v1/holds.t
t/db_dependent/api/v1/holds.t .. 15/16 Use of uninitialized value $rank in string eq at /kohadevbox/koha/C4/Reserves.pm line 1115.
Use of uninitialized value $rank in pattern match (m//) at /kohadevbox/koha/C4/Reserves.pm line 1125.

    #   Failed test 'expiration date changed correctly'
    #   at t/db_dependent/api/v1/holds.t line 1597.
    #          got: undef
    #     expected: '2022-01-01'
    # Looks like you failed 1 test of 7.

#   Failed test 'PUT /holds/{hold_id}/expiration_date tests'
#   at t/db_dependent/api/v1/holds.t line 1600.
# Looks like you failed 1 test of 16.
t/db_dependent/api/v1/holds.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/16 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/holds.t (Wstat: 256 Tests: 16 Failed: 1)
  Failed test:  16
  Non-zero exit status: 1
Files=1, Tests=16, 25 wallclock secs ( 0.07 usr  0.01 sys + 21.83 cusr  1.58 csys = 23.49 CPU)
Result: FAIL