Bug 35559 - Can't change the pickup date of holds on the last day of expiration
Summary: Can't change the pickup date of holds on the last day of expiration
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement with 50 votes (vote)
Assignee: Emily Lamancusa
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-12-13 13:55 UTC by Peter Vashchuk
Modified: 2024-04-08 19:44 UTC (History)
10 users (show)

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


Attachments
Bug 35559 - Allow the change of expiration date on the last day of expiration (1.84 KB, patch)
2023-12-13 15:10 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 35559: Revert changes from bug 34634 (2.30 KB, patch)
2024-03-22 20:28 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 35559: Correctly parse previous flatpickr date (2.64 KB, patch)
2024-03-22 20:29 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2023-12-13 13:55:37 UTC
Our libraries complained that the expiration date of holds awaiting pickup can no longer be changed on the last day of the pick-up period. In the previous version, it was possible.

To reproduce the problem:
1) Create or use existing hold that is awaiting pickup.
2) Set it's expiration date to today.
3) Check that you can't change the expiration date anymore as it says "Expired: %date%"
Comment 1 Peter Vashchuk 2023-12-13 15:10:26 UTC
Created attachment 159807 [details] [review]
Bug 35559 - Allow the change of expiration date on the last day of expiration

The result of this comparison will almost never be 0 as it requires the dates to be same down to a second:
DateTime->compare( dt_from_string( $res->expirationdate ), dt_from_string() ) == -1
This patch fixes it.

To reproduce:
1) Create or use existing hold that is awaiting pickup.
2) Set it's expiration date to today. (if you can't do it through koha interface, edit it in the database or wait for tomorrow to come)
3) Check that you can't change the expiration date anymore as it says "Expired: %date%"
4) Apply the patch
5) Check that you can change the expiration date
Comment 2 Lucas Gass 2023-12-15 21:54:42 UTC
This creates a bit of a problem on reserve/request.pl with your patched applied.

1. Have a hold that expires today and go to reserve/request.pl for that item. 
2. Look at the expiration date column and notice it will be blank.
3. We use data-flatpickr-futuredate on this input and today is not a futuredate. 


Furthermore, I am not sure I agree with this change. If the expiration date is today I would not considered it "the last day of the pick-up period". I would consider that to be the day before the expirationdate.

 

Also, your bug title is not not correct. Have a look at the QA tool:

Processing additional checks

	* Commit title does not start with 'Bug XXXXX: ' - c2d44aa429
Comment 3 Lisette Scheer 2023-12-18 15:26:04 UTC
> Furthermore, I am not sure I agree with this change. If the expiration date
> is today I would not considered it "the last day of the pick-up period". I
> would consider that to be the day before the expirationdate.

Many libraries that don't expire waiting holds with the cron seem to hold through the expiration date and would like the option to change the date at that time.
Comment 4 Owen Leonard 2023-12-21 12:45:00 UTC
(In reply to Lisette Scheer from comment #3)

> Many libraries that don't expire waiting holds with the cron seem to hold
> through the expiration date and would like the option to change the date at
> that time.

Agreed. We don't use the cron and would like the option to revise the expiration date even if it has passed.
Comment 5 Emily Lamancusa 2024-02-13 21:58:41 UTC
I've gotten feedback from our staff about this as well - we also don't use the cron for waiting holds, and it is common for staff to grant last-minute extensions if a customer called and asked for one.

Incidentally, I also noticed an inconsistency the other day - the expiration dates becomes un-editable on the expiration date of the hold, but holds don't appear on the Holds Over tab until the day after the expiration date. Not sure off the top of my head which way the cron goes, but those three things should be consistent.
Comment 6 Keith Dembek 2024-02-14 14:33:21 UTC
Our circulation staff have also requested this feature. They are used to being able to edit the expiration date on the final date listed.
Comment 7 Emily Lamancusa 2024-02-27 22:16:04 UTC
It looks like this is a regression (or the indirect result of one), and I think I've tracked down how we got here, but I'm not familiar enough with flatpickr to submit an alternate patch. Hopefully this will be helpful to someone who is:

1. We had a bug in the past, where a flatpickr with futuredate wouldn't display the previously input date (e.g. the expiration date on an expired hold) if that date was in the past. We fixed that in Bug 29241 and its follow-up, Bug 29500.

2. Bug 30718 (Use flatpickr's altInput option everywhere) caused a regression, and the date field on an expired hold (or any other futuredate field with a preexisting date in the past) would show as blank again.

3. Bug 34634 fixed the regression by removing the input field and simply displaying the date if the hold was expired. However, some library staff are accustomed to extending holds on or after the expiration date is reached, so this less than ideal for those workflows.

Also, the regression caused by bug 30718 (presumably) still exists for all other futuredate inputs, besides the holds page. So it would be ideal if we can fix that underlying regression and allow editing of the dates on expired holds.
Comment 8 Emily Lamancusa 2024-03-22 20:28:57 UTC
Created attachment 163742 [details] [review]
Bug 35559: Revert changes from bug 34634

This reverts commit fad3af0005c3a4234634ee477fafc85902540843
Comment 9 Emily Lamancusa 2024-03-22 20:29:00 UTC
Created attachment 163743 [details] [review]
Bug 35559: Correctly parse previous flatpickr date

The Flatpickr configuration for futuredate and futuredateinclusive is
meant to allow the preexisting date, even if it is in the past, to avoid
data loss. As of Bug 30718 - Use flatpickr's alt option everywhere,
that incoming date is in yyyy-mm-dd format, not the configured
human-readable format, and needs to be parsed accordingly.

To test:
1. Place 2 holds on the same bib, both with an expiration date set
2. Set the expiration date for one of the holds to a date in the past
   (e.g., in Koha Testing Docker, use the commands:
   ktd --shell
   koha-mysql kohadev
   to access the database directly)
3. Reload the holds tab for that bib
--> Note that the future expiration date will be editable, and the past
    expiration date will not be editable
4. In a new tab, go to Administration > Patron Categories
5. Edit one patron category to have an enrolment period date in the future
6. Edit another patron category to have an enrolment period date in the
   past (e.g. by accessing the database directly, as above)
7. Reload the Edit pages for each of the above categories (in new tabs)
--> Note that the future enrolment period date will be retained in the
    date field, but the past enrolment period date will be blanked out
8. Apply this patchset
9. Refresh the holds tab from step 3
--> Note that both expiration dates are now editable
10. Refresh the 2 patron category tabs from step 7
--> Note that both enrolment dates are now retained correctly
11. Open the date picker on one of the date fields that has a past date
--> Note that other past dates, besides the existing date, are prevented