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
Created attachment 173116 [details] [review] Bug 30661: Allow to update more hold parameters via REST API 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 173117 [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
Tests are still failing for me 8-(: prove t/db_dependent/api/v1/holds.t t/db_dependent/api/v1/holds.t .. 9/16 # Failed test '200 OK' # at t/db_dependent/api/v1/holds.t line 1036. # got: '400' # expected: '200' # Failed test 'Hold date changed correctly' # at t/db_dependent/api/v1/holds.t line 1040. # got: '2022-01-01' # expected: '2022-01-02' # Failed test 'Expiration date changed correctly' # at t/db_dependent/api/v1/holds.t line 1041. # got: '2022-03-01' # expected: '2022-03-02' # Failed test '200 OK' # at t/db_dependent/api/v1/holds.t line 1116. # got: '400' # expected: '200' # Failed test 'Hold date changed correctly' # at t/db_dependent/api/v1/holds.t line 1119. # got: '2022-01-01' # expected: '2022-01-02' # Failed test 'Expiration date changed correctly' # at t/db_dependent/api/v1/holds.t line 1120. # got: '2022-03-01' # expected: '2022-03-02' # Looks like you failed 6 tests of 47. t/db_dependent/api/v1/holds.t .. 11/16 # Failed test 'edit() tests' # at t/db_dependent/api/v1/holds.t line 1124. t/db_dependent/api/v1/holds.t .. 14/16 # Failed test '403 Forbidden' # at t/db_dependent/api/v1/holds.t line 1624. # got: '404' # expected: '403' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/holds.t line 1624. # Structures begin differing at: # $got->{errors} = ARRAY(0x60ee6c5f4130) # $expected->{errors} = Does not exist # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/holds.t line 1631. # Structures begin differing at: # $got->{status} = '404' # $expected->{status} = Does not exist # Failed test '200 OK' # at t/db_dependent/api/v1/holds.t line 1634. # got: '404' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/holds.t line 1634. # Structures begin differing at: # $got->{status} = '404' # $expected->{status} = Does not exist # Failed test 'hold date changed correctly' # at t/db_dependent/api/v1/holds.t line 1638. # got: '2024-10-22' # expected: '2022-01-01' # Looks like you failed 6 tests of 10. # Failed test 'PUT /holds/{hold_id}/hold_date tests' # at t/db_dependent/api/v1/holds.t line 1641. # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/holds.t line 1687. # Structures begin differing at: # $got->{errors} = ARRAY(0x60ee6ca3f200) # $expected->{errors} = Does not exist # Failed test '200 OK' # at t/db_dependent/api/v1/holds.t line 1691. # got: '404' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/holds.t line 1691. # Structures begin differing at: # $got->{errors} = ARRAY(0x60ee6c5e0ab8) # $expected->{errors} = Does not exist # Failed test 'expiration date changed correctly' # at t/db_dependent/api/v1/holds.t line 1696. # got: undef # expected: '2022-01-01' # Looks like you failed 4 tests of 7. # Failed test 'PUT /holds/{hold_id}/expiration_date tests' # at t/db_dependent/api/v1/holds.t line 1699. # Looks like you failed 3 tests of 16. t/db_dependent/api/v1/holds.t .. Dubious, test returned 3 (wstat 768, 0x300) Failed 3/16 subtests Test Summary Report ------------------- t/db_dependent/api/v1/holds.t (Wstat: 768 (exited 3) Tests: 16 Failed: 3) Failed tests: 11, 15-16 Non-zero exit status: 3 Files=1, Tests=16, 11 wallclock secs ( 0.04 usr 0.01 sys + 8.69 cusr 2.04 csys = 10.78 CPU) Result: FAIL
(In reply to David Nind from comment #16) > Tests are still failing for me 8-(: > > prove t/db_dependent/api/v1/holds.t > t/db_dependent/api/v1/holds.t .. 9/16 > # Failed test '200 OK' > # at t/db_dependent/api/v1/holds.t line 1036. > # got: '400' > # expected: '200' > > # Failed test 'Hold date changed correctly' > # at t/db_dependent/api/v1/holds.t line 1040. > # got: '2022-01-01' > # expected: '2022-01-02' > > # Failed test 'Expiration date changed correctly' > # at t/db_dependent/api/v1/holds.t line 1041. > # got: '2022-03-01' > # expected: '2022-03-02' > > # Failed test '200 OK' > # at t/db_dependent/api/v1/holds.t line 1116. > # got: '400' > # expected: '200' > > # Failed test 'Hold date changed correctly' > # at t/db_dependent/api/v1/holds.t line 1119. > # got: '2022-01-01' > # expected: '2022-01-02' > > # Failed test 'Expiration date changed correctly' > # at t/db_dependent/api/v1/holds.t line 1120. > # got: '2022-03-01' > # expected: '2022-03-02' > # Looks like you failed 6 tests of 47. > t/db_dependent/api/v1/holds.t .. 11/16 > # Failed test 'edit() tests' > # at t/db_dependent/api/v1/holds.t line 1124. > t/db_dependent/api/v1/holds.t .. 14/16 > # Failed test '403 Forbidden' > # at t/db_dependent/api/v1/holds.t line 1624. > # got: '404' > # expected: '403' > > # Failed test 'exact match for JSON Pointer ""' > # at t/db_dependent/api/v1/holds.t line 1624. > # Structures begin differing at: > # $got->{errors} = ARRAY(0x60ee6c5f4130) > # $expected->{errors} = Does not exist > > # Failed test 'exact match for JSON Pointer ""' > # at t/db_dependent/api/v1/holds.t line 1631. > # Structures begin differing at: > # $got->{status} = '404' > # $expected->{status} = Does not exist > > # Failed test '200 OK' > # at t/db_dependent/api/v1/holds.t line 1634. > # got: '404' > # expected: '200' > > # Failed test 'exact match for JSON Pointer ""' > # at t/db_dependent/api/v1/holds.t line 1634. > # Structures begin differing at: > # $got->{status} = '404' > # $expected->{status} = Does not exist > > # Failed test 'hold date changed correctly' > # at t/db_dependent/api/v1/holds.t line 1638. > # got: '2024-10-22' > # expected: '2022-01-01' > # Looks like you failed 6 tests of 10. > > # Failed test 'PUT /holds/{hold_id}/hold_date tests' > # at t/db_dependent/api/v1/holds.t line 1641. > > # Failed test 'exact match for JSON Pointer ""' > # at t/db_dependent/api/v1/holds.t line 1687. > # Structures begin differing at: > # $got->{errors} = ARRAY(0x60ee6ca3f200) > # $expected->{errors} = Does not exist > > # Failed test '200 OK' > # at t/db_dependent/api/v1/holds.t line 1691. > # got: '404' > # expected: '200' > > # Failed test 'exact match for JSON Pointer ""' > # at t/db_dependent/api/v1/holds.t line 1691. > # Structures begin differing at: > # $got->{errors} = ARRAY(0x60ee6c5e0ab8) > # $expected->{errors} = Does not exist > > # Failed test 'expiration date changed correctly' > # at t/db_dependent/api/v1/holds.t line 1696. > # got: undef > # expected: '2022-01-01' > # Looks like you failed 4 tests of 7. > > # Failed test 'PUT /holds/{hold_id}/expiration_date tests' > # at t/db_dependent/api/v1/holds.t line 1699. > # Looks like you failed 3 tests of 16. > t/db_dependent/api/v1/holds.t .. Dubious, test returned 3 (wstat 768, 0x300) > Failed 3/16 subtests > > Test Summary Report > ------------------- > t/db_dependent/api/v1/holds.t (Wstat: 768 (exited 3) Tests: 16 Failed: 3) > Failed tests: 11, 15-16 > Non-zero exit status: 3 > Files=1, Tests=16, 11 wallclock secs ( 0.04 usr 0.01 sys + 8.69 cusr 2.04 > csys = 10.78 CPU) > Result: FAIL Interesting, I fetched main branch today and added the code on it. My tests go through, are you on main branch?
(In reply to Johanna Räisä from comment #17) > (In reply to David Nind from comment #16) > > Tests are still failing for me 8-(: > > > > prove t/db_dependent/api/v1/holds.t > > t/db_dependent/api/v1/holds.t .. 9/16 ... > Interesting, I fetched main branch today and added the code on it. > My tests go through, are you on main branch? Using KTD - pulled everything (Koha clone, KTD clone, KTD pull). Tests pass before applying the patches.
(In reply to David Nind from comment #18) > (In reply to Johanna Räisä from comment #17) > > (In reply to David Nind from comment #16) > > > Tests are still failing for me 8-(: > > > > > > prove t/db_dependent/api/v1/holds.t > > > t/db_dependent/api/v1/holds.t .. 9/16 > ... > > Interesting, I fetched main branch today and added the code on it. > > My tests go through, are you on main branch? > > Using KTD - pulled everything (Koha clone, KTD clone, KTD pull). Tests pass > before applying the patches. # Failed test '403 Forbidden' # at t/db_dependent/api/v1/holds.t line 1624. # got: '404' # expected: '403' This indicates that the endpoints are not in place, since there is 404 error instead of 403. My koha-dev is installed to my computer and endpoints work after plack restart. Can you restart plack or is it done automatically?
(In reply to Johanna Räisä from comment #19) .. > > Using KTD - pulled everything (Koha clone, KTD clone, KTD pull). Tests pass > > before applying the patches. > > # Failed test '403 Forbidden' > # at t/db_dependent/api/v1/holds.t line 1624. > # got: '404' > # expected: '403' > > This indicates that the endpoints are not in place, since there is 404 error > instead of 403. My koha-dev is installed to my computer and endpoints work > after plack restart. Can you restart plack or is it done automatically? I did a restart (restart_all) (I normally do this as a matter of course after applying patches).
Created attachment 173122 [details] [review] Bug 30661: Allow to update more hold parameters via REST API 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 Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173123 [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 Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using KTD): 1. Run the tests before applying the patches - tests should pass: prove t/db_dependent/api/v1/holds.t 2. Apply the patches. 3. Update the API specification: yarn api:bundle 4. Restart everything: restart_all 5. Re-run the tests - they should still pass.
Hi, I'm looking here. Had a chat with Martin we agreed that due to existing pattern, there's no point in blocking this for adding specific 2 new specific PUT endpoints and we can address that in bug 38229. Though the first patch is also adding the attributes to the edit PATCH endpoint, I'm possibly missing something here but shouldn't we only need one or the other?
(In reply to Pedro Amorim from comment #24) > Hi, I'm looking here. > Had a chat with Martin we agreed that due to existing pattern, there's no > point in blocking this for adding specific 2 new specific PUT endpoints and > we can address that in bug 38229. > > Though the first patch is also adding the attributes to the edit PATCH > endpoint, I'm possibly missing something here but shouldn't we only need one > or the other? There are some time since I started doing this, but I think I have made this similar as the pickup_locations. It has it's own PUT endpoint and it is also included to the edit(). We can skip the follow-up since PATCH should allow updating a singular value.
Created attachment 173209 [details] [review] Bug 30661: Allow to update more hold parameters via REST API 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 Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
(In reply to Johanna Räisä from comment #25) > We can skip the follow-up since PATCH should allow > updating a singular value. Hi Johanna, I've dropped the follow-up patch as discussed.
Pushed for 24.11! Well done everyone, thank you!
Could you have a look if these failures relate to this change and provide a fix? prove t/db_dependent/api/v1/holds.t t/db_dependent/api/v1/holds.t .. 1/14 No reserves HOLD_CANCELLATION letter transported by email at /kohadevbox/koha/C4/Letters.pm line 610. t/db_dependent/api/v1/holds.t .. 12/14 # Looks like you planned 37 tests but ran 3. t/db_dependent/api/v1/holds.t .. 13/14 # Failed test 'PUT /holds/{hold_id}/pickup_location tests' # at t/db_dependent/api/v1/holds.t line 1470. Can't locate object method "branchcode" via package "euRljh" (perhaps you forgot to load "euRljh"?) at t/db_dependent/api/v1/holds.t line 1377. # Looks like your test exited with 11 just after 13. t/db_dependent/api/v1/holds.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 2/14 subtests
(In reply to Katrin Fischer from comment #29) > Could you have a look if these failures relate to this change and provide a > fix? > > prove t/db_dependent/api/v1/holds.t > t/db_dependent/api/v1/holds.t .. 1/14 No reserves HOLD_CANCELLATION letter > transported by email at /kohadevbox/koha/C4/Letters.pm line 610. > t/db_dependent/api/v1/holds.t .. 12/14 # Looks like you planned 37 tests > but ran 3. > t/db_dependent/api/v1/holds.t .. 13/14 > # Failed test 'PUT /holds/{hold_id}/pickup_location tests' > # at t/db_dependent/api/v1/holds.t line 1470. > Can't locate object method "branchcode" via package "euRljh" (perhaps you > forgot to load "euRljh"?) at t/db_dependent/api/v1/holds.t line 1377. > # Looks like your test exited with 11 just after 13. > t/db_dependent/api/v1/holds.t .. Dubious, test returned 11 (wstat 2816, > 0xb00) > Failed 2/14 subtests Not related to this, I think. I didn't get that HOLD_CANCELLATION error after I pulled main repo. The subtest "PUT /holds/{hold_id}/pickup_location tests" fails in holds.t There are some interesting branchcode fetching in "PUT /holds/{hold_id}/pickup_location tests" at least: $hold->discard_changes->branchcode->branchcode If those second branchcode values are removed the tests pass in that subtest.
Thanks for the quick reply, Johanna.
(In reply to Katrin Fischer from comment #29) > Could you have a look if these failures relate to this change and provide a > fix? I think this was caused by bug 38273. I've left a fix patch there.
Enhancement, no 24.05.x backport