Bug 39419

Summary: Holds API treats 'expiration_date' as 'patron_expiration_date'
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Hold requestsAssignee: Brendan Lawlor <blawlor>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: blawlor, chlee, david, esther.melander, gmcharlt, mspinney, piia.semenoff, rcoert
Version: Main   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently
Bug 39419: (follow-up) Unit tests
Bug 39419: (follow-up) Unit tests
Bug 39419: (follow-up) Unit tests
Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently
Bug 39419: (follow-up) Unit tests
Bug 39419: Enable Holds API to set expiration_date and patron_expiration_date independently
Bug 39419: (follow-up) Unit tests

Description Nick Clemens (kidclamp) 2025-03-21 19:40:13 UTC
This is going to have a related issue in Aspen, but I think the confusion starts here

Koha has two fields for hold expiration:
expirationdate - (usually) auto generated date set based no when holds on the shelf expire
patron_exiration_date  - the day the patron says they don't need the hold after

The API takes "expiration_date" and maps it to "patron_expiration_date"

This can be unclear for outside systems.

Aspen has a 'Default Not Needed After Days' setting which is assumed to be setting the automatic expiraton date, but in fact sets the patron date

We should allow either field to be set via the API
Comment 1 Nick Clemens (kidclamp) 2025-03-21 19:43:32 UTC
Aspen bug report: https://aspen-discovery.atlassian.net/browse/DIS-537
Comment 2 Brendan Lawlor 2025-08-19 19:37:24 UTC
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
Comment 3 Brendan Lawlor 2025-08-20 15:26:39 UTC
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
Comment 4 Brendan Lawlor 2025-09-04 18:25:11 UTC
Created attachment 186176 [details] [review]
Bug 39419: (follow-up) Unit tests

perl t/db_dependent/Reserves.t
Comment 5 Brendan Lawlor 2025-09-04 18:25:14 UTC
Created attachment 186177 [details] [review]
Bug 39419: (follow-up) Unit tests
Comment 6 Brendan Lawlor 2025-09-04 18:26:38 UTC
I added a working test for AddReserve but I had trouble getting the test to work for ModReserve
Comment 7 Brendan Lawlor 2025-09-05 16:58:21 UTC
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
Comment 8 Brendan Lawlor 2025-09-05 17:01:22 UTC
Added unit tests for AddReserve(), ModReserve() and for API calls to add() and edit()

Thanks Nick!
Comment 9 David Nind 2025-11-28 19:08:53 UTC
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
Comment 10 Brendan Lawlor 2025-12-11 16:32:07 UTC
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
Comment 11 Brendan Lawlor 2025-12-11 16:32:10 UTC
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
Comment 12 Brendan Lawlor 2025-12-11 16:42:50 UTC
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.
Comment 13 David Nind 2025-12-11 20:18:51 UTC
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
Comment 14 Brendan Lawlor 2025-12-11 20:42:29 UTC
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
Comment 15 Brendan Lawlor 2025-12-11 20:42:32 UTC
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
Comment 16 Brendan Lawlor 2025-12-11 21:07:04 UTC
(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!
Comment 17 David Nind 2025-12-13 02:35:31 UTC
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
Comment 18 Brendan Lawlor 2025-12-13 02:58:27 UTC
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