|
Description
Nick Clemens (kidclamp)
2025-03-21 19:40:13 UTC
Aspen bug report: https://aspen-discovery.atlassian.net/browse/DIS-537 Created attachment 185568 [details] [review] Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently This patch adds patron_expiration_date as a new parameter to the Holds API for the add and edit methods. To test: 1. Apply patch and restart_all 2. Make some requests to add holds like POST /api/v1/holds example body: { "biblio_id": "22", "patron_id": "42", "pickup_library_id": "FFL", "expiration_date": "2099-12-31", "patron_expiration_date": "3099-01-01" } 3. Verify that you can set the expiration_date and patron_expiration_date indepdently 4. Confirm that patron_expiration_date is a new field in the API response 5. Make note of the hold_id in the response for the next step 6. Make some requests to edit the hold like PATCH /api/v1/holds/{hold_id} example body: { "patron_expiration_date": "2999-09-19", "expiration_date": "2031-09-19" } 7. Verify that you can set the expiration_date and patron_expiration_date indepdently 8. Make sure making and modifying holds isn't broken in the staff intterface or opac Maybe the changes to Reserves.pm and Hold.pm need tests? There is a bit of extra logic that goes on with expirationdate and patron_expiration_date that is explained on bug 29975. I'm not clear on exactly how the API should work with relation to that though Created attachment 186176 [details] [review] Bug 39419: (follow-up) Unit tests perl t/db_dependent/Reserves.t Created attachment 186177 [details] [review] Bug 39419: (follow-up) Unit tests I added a working test for AddReserve but I had trouble getting the test to work for ModReserve Created attachment 186240 [details] [review] Bug 39419: (follow-up) Unit tests perl t/db_dependent/Reserves.t perl t/db_dependent/api/v1/holds.t Added unit tests for AddReserve(), ModReserve() and for API calls to add() and edit() Thanks Nick! The patch no longer applies 8-(... git bz apply 39419 Bug 39419 - Holds API treats 'expiration_date' as 'patron_expiration_date' 185568 - Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently 186240 - Bug 39419: (follow-up) Unit tests Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently Using index info to reconstruct a base tree... M C4/Reserves.pm M Koha/Hold.pm M Koha/REST/V1/Holds.pm M api/v1/swagger/definitions/hold.yaml M api/v1/swagger/paths/holds.yaml Falling back to patching base and 3-way merge... Auto-merging api/v1/swagger/paths/holds.yaml Auto-merging api/v1/swagger/definitions/hold.yaml Auto-merging Koha/REST/V1/Holds.pm CONFLICT (content): Merge conflict in Koha/REST/V1/Holds.pm Auto-merging Koha/Hold.pm Auto-merging C4/Reserves.pm error: Failed to merge in the changes. Patch failed at 0001 Bug Created attachment 190441 [details] [review] Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently This patch adds patron_expiration_date as a new parameter to the Holds API for the add and edit methods. To test: 1. Apply patch and restart_all 2. Make some requests to add holds like POST /api/v1/holds example body: { "biblio_id": "22", "patron_id": "42", "pickup_library_id": "FFL", "expiration_date": "2099-12-31", "patron_expiration_date": "3099-01-01" } 3. Verify that you can set the expiration_date and patron_expiration_date indepdently 4. Confirm that patron_expiration_date is a new field in the API response 5. Make note of the hold_id in the response for the next step 6. Make some requests to edit the hold like PATCH /api/v1/holds/{hold_id} example body: { "patron_expiration_date": "2999-09-19", "expiration_date": "2031-09-19" } 7. Verify that you can set the expiration_date and patron_expiration_date indepdently 8. Make sure making and modifying holds isn't broken in the staff intterface or opac Created attachment 190442 [details] [review] Bug 39419: (follow-up) Unit tests perl t/db_dependent/Reserves.t perl t/db_dependent/api/v1/holds.t Thanks for testing David! One other note for testing you may get an API error 400 "Properties not allowed: patron_expiration_date." It seems you need both restart_all && yarn build for the API to be updated. Hi Brendan. I've tested, but the tests fail for me for t/db_dependent/Reserves.t [1] Everything with the API worked as expected (I used https://www.usebruno.com/) - I could set expirationdate and patron_exiration_date independently and together. However, I couldn't work out how to get a date in patron_expiration_date using either the staff interface or OPAC (which all seemed to work fine for placing holds). I thought it might have been this system preference, but dates went into expirationdate: DefaultHoldExpirationdate DefaultHoldExpirationdatePeriod DefaultHoldExpirationdateUnitOfTime [1] Failed tests prove t/db_dependent/Reserves.t t/db_dependent/Reserves.t .. 68/71 # Looks like you planned 71 tests but ran 72. t/db_dependent/Reserves.t .. Dubious, test returned 255 (wstat 65280, 0xff00) All 71 subtests passed Test Summary Report ------------------- t/db_dependent/Reserves.t (Wstat: 65280 (exited 255) Tests: 72 Failed: 1) Failed test: 72 Non-zero exit status: 255 Parse errors: Bad plan. You planned 71 tests but ran 72. Files=1, Tests=72, 8 wallclock secs ( 0.03 usr 0.01 sys + 5.71 cusr 2.00 csys = 7.75 CPU) Result: FAIL Created attachment 190443 [details] [review] Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently This patch adds patron_expiration_date as a new parameter to the Holds API for the add and edit methods. To test: 1. Apply patch and restart_all 2. Make some requests to add holds like POST /api/v1/holds example body: { "biblio_id": "22", "patron_id": "42", "pickup_library_id": "FFL", "expiration_date": "2099-12-31", "patron_expiration_date": "3099-01-01" } 3. Verify that you can set the expiration_date and patron_expiration_date indepdently 4. Confirm that patron_expiration_date is a new field in the API response 5. Make note of the hold_id in the response for the next step 6. Make some requests to edit the hold like PATCH /api/v1/holds/{hold_id} example body: { "patron_expiration_date": "2999-09-19", "expiration_date": "2031-09-19" } 7. Verify that you can set the expiration_date and patron_expiration_date indepdently 8. Make sure making and modifying holds isn't broken in the staff intterface or opac Sponsored-by: CLAMS Created attachment 190444 [details] [review] Bug 39419: (follow-up) Unit tests perl t/db_dependent/Reserves.t perl t/db_dependent/api/v1/holds.t (In reply to David Nind from comment #13) > However, I couldn't work out how to get a date in patron_expiration_date > using either the staff interface or OPAC (which all seemed to work fine for > placing holds). The expiration date is mostly set on holds as an automatic process based on system preferences, upon checking the hold into the hold shelf. When it becomes waiting it gets an expiration date accordingly. The patron_expiration_date is set in Koha when the patron places a hold in the OPAC, clicks Show more options and enters a Hold not needed after date. Or in the staff interface when staff places a hold and enters a Hold expires on date. These actions actually set both expirationdate and patron_expiration_date. As far as I can tell in the Koha OPAC and staff interface you can only set the patron_expirtion_date at the time of placing the hold, but there's no way to update it once the hold is placed. I could be wrong about this though :) The terminology and labels are not consistent so it is rather confusing, but you can confirm what field is getting set using ktd --dbshell with a query like: select biblionumber, expirationdate, patron_expiration_date from reserves where borrowernumber=38; Or even better you can confirm using the API with a call like GET /api/v1/holds?patron_id=38 We're using Aspen for our OPAC and this bug affects us because Aspen is currently setting the expiration_date when it should be setting the patron_expiration_date. > [1] Failed tests I fixed the failing test. Not sure but I think I may have made this mistake when rebasing the first time. Thanks for testing again! Thanks Brendan. The tests all now pass. I still couldn't work out how to add a date to the patron_expiration_date field, using either the staff interface or OPAC (the API worked fine). When placing a hold using either the staff interface or OPAC, I selected a date: - staff interface: "Holds expires on date" field - OPAC: show more options, "Hold not needed after" field In both cases, it only updated the expirationdate field in the database. Will leave for someone else more knowledgeable to test. David Thanks for testing again, David. I could be wrong, but I thought that the expiration date is calculated based on system preferences when the hold is checked into the shelf and the patron expiration date is set when the patron requests the hold with a not wanted after date. That's the intent of those fields at least according to the schema: https://schema.koha-community.org/25_05/tables/reserves.html expirationdate the date the hold expires (calculated value) patron_expiration_date the date the hold expires - usually the date entered by the patron to say they don’t need the hold after a certain date I'll have to take a closer look again |