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.
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
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
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?
(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
(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.
Created attachment 140221 [details] Changing the hold date of an existing hold
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
(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.
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.
(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.
(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.
Created attachment 140359 [details] [review] Bug 30661: [FOLLOW-UP] add separate endpoints Here is the fixed follow-up patch.
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