Bug 24608

Summary: Allow modification of checkout due date
Product: Koha Reporter: Andrew Isherwood <bugzilla>
Component: CirculationAssignee: Bugs List <koha-bugs>
Status: BLOCKED --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: agustinmoyano, dch, gmcharlt, janet.mcgowan, jonathan.druart, katrin.fischer, kebliss, kyle.m.hall, lucas, marjorie.barry-vila, martin.renvoize, pasi.kallinen, patrick.robitaille, sally.healey, tomascohen
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24850
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25020
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 24609, 24455    
Bug Blocks:    
Attachments: Bug 24608: Add EditDueDates syspref
Bug 24608: Allow due date modification
Bug 24608: Add EditDueDates syspref
Bug 24608: Allow due date modification
Bug 24608: Add EditDueDates syspref
Bug 24608: Allow due date modification
Bug 24608: Add EditDueDates syspref
Bug 24608: Allow due date modification
Bug 24608: (follow-up) Use strings.inc for "Edit"
Bug 24608: (follow-up) Switch to bug 24455 dates
Bug 24608: (follow-up) Buttons should be buttons
Bug 24608: Add EditDueDates syspref
Bug 24608: Allow due date modification
Bug 24608: (follow-up) Use strings.inc for "Edit"
Bug 24608: (follow-up) Switch to bug 24455 dates
Bug 24608: (follow-up) Buttons should be buttons
Bug 24608: Add EditDueDates syspref
Bug 24608: Allow due date modification
Bug 24608: (follow-up) Make "Edit" translatable
Bug 24608: (follow-up) Buttons should be buttons
Bug 24608: (follow-up) Switch to bug 24455 dates

Description Andrew Isherwood 2020-02-07 10:43:23 UTC
Currently it is not possible to modify a checkout's due date. This bug proposes to allow such an action.

It is only proposed to allow modification of a due date *forward* at this stage as moving a due date backwards may involve additional complication that will require further thought.
Comment 1 Katrin Fischer 2020-02-08 11:14:58 UTC
*** Bug 22064 has been marked as a duplicate of this bug. ***
Comment 2 Andrew Isherwood 2020-02-14 08:40:04 UTC
Created attachment 98902 [details] [review]
Bug 24608: Add EditDueDates syspref

This patch adds a EditDueDates syspref which determines whether a button
for editing due dates appears in a patron's checkouts list
Comment 3 Andrew Isherwood 2020-02-14 08:40:07 UTC Comment hidden (obsolete)
Comment 4 Andrew Isherwood 2020-02-14 08:58:55 UTC Comment hidden (obsolete)
Comment 5 Andrew Isherwood 2020-02-14 08:58:58 UTC Comment hidden (obsolete)
Comment 6 Andrew Isherwood 2020-02-14 09:00:54 UTC
Test plan:

- Apply the patch
- View a patron's checkout list
=> TEST: Observe that the due date is displayed as normal
- Enable the EditDueDates syspref
- View a patron's checkout list
=> TEST: Observe that each checkout now has an "Edit" button next to the due date
- Click the edit button next to a checkout's due date
=> TEST: Observe that the date is now editable
=> TEST: The edit button is replaced by a "Save" button
=> TEST: All other checkout "Edit" buttons are disabled
=> TEST: Clicking on the due date opens a calendar widget
=> TEST: It is not possible to select a date prior to the existing due date
- Select a new due date
- Click the "Save" button
=> TEST: The save button is replaced by a progress spinner while the update is performed
=> TEST: Once the update is complete, the displayed date reflects the selected date
=> TEST: Once the update is complete, the display returns to it's initial state
Comment 7 Andrew Isherwood 2020-02-14 09:08:53 UTC Comment hidden (obsolete)
Comment 8 Andrew Isherwood 2020-02-14 09:08:57 UTC Comment hidden (obsolete)
Comment 9 Andrew Isherwood 2020-02-14 11:57:04 UTC
Created attachment 98931 [details] [review]
Bug 24608: Add EditDueDates syspref

This patch adds a EditDueDates syspref which determines whether a button
for editing due dates appears in a patron's checkouts list
Comment 10 Andrew Isherwood 2020-02-14 11:57:07 UTC
Created attachment 98932 [details] [review]
Bug 24608: Allow due date modification

This patch adds the ability to modify due dates from a patron's
checkouts list.
Comment 11 Daphne Hoolahan 2020-02-14 16:05:19 UTC
I applied the patch on Sandbox 5, set the syspref to allow, but I'm not seeing the new button beside the due date on the checkouts.
Comment 12 paxed 2020-02-19 06:09:51 UTC
Comment on attachment 98932 [details] [review]
Bug 24608: Allow due date modification

Is the "Edit" in checkouts.js untranslatable?
Comment 13 Katrin Fischer 2020-02-19 08:31:03 UTC
(In reply to paxed from comment #12)
> Comment on attachment 98932 [details] [review] [review]
> Bug 24608: Allow due date modification
> 
> Is the "Edit" in checkouts.js untranslatable?

In the way it's done now it is not - it could be made translatable with bug 21156.
Comment 14 Andrew Isherwood 2020-02-19 08:36:36 UTC
(In reply to paxed from comment #12)
> Comment on attachment 98932 [details] [review] [review]
> Bug 24608: Allow due date modification
> 
> Is the "Edit" in checkouts.js untranslatable?

This is an oversight on my part, I should be using strings.inc as per the "Save" button. I'll create a patch to address this.
Comment 15 Andrew Isherwood 2020-02-19 08:46:58 UTC
Created attachment 99240 [details] [review]
Bug 24608: (follow-up) Use strings.inc for "Edit"

As per comment #14
Comment 16 Owen Leonard 2020-02-29 16:21:52 UTC
I'm getting an error when I save an edited date, "ReferenceError: ISO_to_syspref is not defined checkouts_19.1200030.js:1135:45"
Comment 17 Andrew Isherwood 2020-03-02 09:15:36 UTC
Hi Owen - Thanks for taking a look at this, I appreciate it.

The reason for the error you're seeing is that this bug is dependent on bug 24650. That bug has been moved to "Blocked" status since another bug, bug 24455 is a possible duplicate, but is still awaiting sign-off. But since bug 24455 has not yet been signed off, I don't particularly want to make this bug dependent on it.

Bug 24650 being blocked seems to stop git bz picking it up as a dependency. So can I suggest manually applying that first and then applying this bug?
Comment 18 Andrew Isherwood 2020-03-02 10:47:13 UTC
Created attachment 99880 [details] [review]
Bug 24608: (follow-up) Switch to bug 24455 dates

Bug 24455 does a far more comprehensive job of date parsing than my
single use case patch in bug 24650, therefore this bug has now switched
to using it
Comment 19 Andrew Isherwood 2020-03-02 10:49:01 UTC
Hi (again) - I've now switched to depending on bug 24455 rather than my puny bug 24650 as 24455 brings much more comprehensive date handling to JS in Koha through use of moment.js
Comment 20 Andrew Isherwood 2020-03-03 09:36:56 UTC
Created attachment 99979 [details] [review]
Bug 24608: (follow-up) Buttons should be buttons

No idea why I used <a> elements for the Edit buttons. But the 'disabled'
attribute has no effect on <a> elements, which leads to all sorts of
problems when you have disabled buttons that are not disabled.

Switched to <button> elements.
Comment 21 PTFS Europe Sandboxes 2020-03-03 11:55:43 UTC
Created attachment 99986 [details] [review]
Bug 24608: Add EditDueDates syspref

This patch adds a EditDueDates syspref which determines whether a button
for editing due dates appears in a patron's checkouts list

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 22 PTFS Europe Sandboxes 2020-03-03 11:55:46 UTC
Created attachment 99987 [details] [review]
Bug 24608: Allow due date modification

This patch adds the ability to modify due dates from a patron's
checkouts list.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 23 PTFS Europe Sandboxes 2020-03-03 11:55:49 UTC
Created attachment 99988 [details] [review]
Bug 24608: (follow-up) Use strings.inc for "Edit"

As per comment #14

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 24 PTFS Europe Sandboxes 2020-03-03 11:55:52 UTC
Created attachment 99989 [details] [review]
Bug 24608: (follow-up) Switch to bug 24455 dates

Bug 24455 does a far more comprehensive job of date parsing than my
single use case patch in bug 24650, therefore this bug has now switched
to using it

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 25 PTFS Europe Sandboxes 2020-03-03 11:55:55 UTC
Created attachment 99990 [details] [review]
Bug 24608: (follow-up) Buttons should be buttons

No idea why I used <a> elements for the Edit buttons. But the 'disabled'
attribute has no effect on <a> elements, which leads to all sorts of
problems when you have disabled buttons that are not disabled.

Switched to <button> elements.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 26 Jonathan Druart 2020-03-04 13:06:57 UTC
When I edit I pick 23:59 but 22:59 is saved. Then edit again, save without modifying anything, 21:59 is saved, and so on.
Comment 27 Jonathan Druart 2020-03-04 13:08:24 UTC
If I edit the date and remove the time page, save, it's loading endlessly. Which means 1. errors are not handled correctly, and 2. date only formatted dates are not taken into account.
Comment 28 Jonathan Druart 2020-03-04 13:09:10 UTC
(In reply to Jonathan Druart from comment #27)
> If I edit the date and remove the time part, save, it's loading endlessly.
> Which means 1. errors are not handled correctly, and 2. date only formatted
> dates are not taken into account.
Comment 29 Andrew Isherwood 2020-03-05 11:59:41 UTC
(In reply to Jonathan Druart from comment #26)

Hi Jonathan

> When I edit I pick 23:59 but 22:59 is saved. Then edit again, save without
> modifying anything, 21:59 is saved, and so on.

I can't replicate this. So, you're selecting a date, default time is 23:59, but when you save, 22:59 is saved? Is that the value sent by the API call, or the value that's returned by the API? As I say, I can't replicate this, it works fine for me.

> If I edit the date and remove the time page, save, it's loading endlessly.
> Which means 1. errors are not handled correctly, and 2. date only formatted
> dates are not taken into account.

The "loading endlessly" is actually the JS falling over because the call to DateTime_from_syspref is failing because DateTime_from_syspref doesn't check what it's got before trying to split it. So it splits the received string by " ", then tries to split the second part of that by ":" which of course fails because the second part is null.

Having considered this for a while, I'm not clear of the best way forward, it potentially opens a can of worms. We could say that DateTime_from_syspref should validate what it's got before working with it, and if it fails validation, it should error. We have date validation with accompanying errors in the form of calendar.inc's validate_date, so it would be reasonable to say we need similar time validation. However, if we're doing that, should we not be adding that to the upcoming moment.js work in Bug 24455 since that uses moment.js for date formatting, it should probably be leveraging moment.js' date & time validation. But if we do that with the time component, should we not also switch the date validation that's currently in calendar.inc to using moment.js.

Maybe I'm overcomplicating, but that was my thought process. I don't want to just put a hack in, but at the same time, doing it "properly" could be a pain in the ass.

Any thoughts?
Comment 30 Jonathan Druart 2020-03-05 14:51:11 UTC
(In reply to Andrew Isherwood from comment #29)
> (In reply to Jonathan Druart from comment #26)
> 
> Hi Jonathan
> 
> > When I edit I pick 23:59 but 22:59 is saved. Then edit again, save without
> > modifying anything, 21:59 is saved, and so on.
> 
> I can't replicate this. So, you're selecting a date, default time is 23:59,
> but when you save, 22:59 is saved? Is that the value sent by the API call,
> or the value that's returned by the API? As I say, I can't replicate this,
> it works fine for me.

This is sent when I pick 23:59
{"date_due":"2020-03-18T22:59:00.000Z"}

My host and the server do not have the same timezone, that's why I see something different.
What your patch does (and so the JS lib from bug 24455 I guess) is the expected behavior, in an ideal world.
But it cannot be the only place in Koha where it works like that.
If such behavior is really needed, we need to redesign how we store and display date/datetime.

> > If I edit the date and remove the time page, save, it's loading endlessly.
> > Which means 1. errors are not handled correctly, and 2. date only formatted
> > dates are not taken into account.
> 
> The "loading endlessly" is actually the JS falling over because the call to
> DateTime_from_syspref is failing because DateTime_from_syspref doesn't check
> what it's got before trying to split it. So it splits the received string by
> " ", then tries to split the second part of that by ":" which of course
> fails because the second part is null.
> 
> Having considered this for a while, I'm not clear of the best way forward,
> it potentially opens a can of worms. We could say that DateTime_from_syspref
> should validate what it's got before working with it, and if it fails
> validation, it should error. We have date validation with accompanying
> errors in the form of calendar.inc's validate_date, so it would be
> reasonable to say we need similar time validation. However, if we're doing
> that, should we not be adding that to the upcoming moment.js work in Bug
> 24455 since that uses moment.js for date formatting, it should probably be
> leveraging moment.js' date & time validation. But if we do that with the
> time component, should we not also switch the date validation that's
> currently in calendar.inc to using moment.js.
> 
> Maybe I'm overcomplicating, but that was my thought process. I don't want to
> just put a hack in, but at the same time, doing it "properly" could be a
> pain in the ass.
> 
> Any thoughts?

Looks like a quick change to DateTime_from_syspref could do the trick.
If the time part does not exist then add it with "23:59".
Comment 31 Andrew Isherwood 2020-03-05 15:00:15 UTC
OK, if we can assume the time should be 23:59, then I agree :)
Comment 32 Agustín Moyano 2020-03-06 17:52:57 UTC
seems timezone is not saved in date_due column, because it's a datetime rather than timestamp

https://stackoverflow.com/questions/19843203/how-to-store-a-datetime-in-mysql-with-timezone-info

Maybe we can alter that column to timestamp, so timezone is saved, or I can add a flag to the JS function in 24455 to use timezone or not
Comment 33 Andrew Isherwood 2020-03-09 09:19:49 UTC
(In reply to Agustín Moyano from comment #32)
> seems timezone is not saved in date_due column, because it's a datetime
> rather than timestamp

Good spot Agustín! Since we're wanting to take into account the timezone during storage and display, I think you're right, a timestamp column would make more sense.

> Maybe we can alter that column to timestamp, so timezone is saved, or I can
> add a flag to the JS function in 24455 to use timezone or not

Modifying the column seems the proper way to do this. Since the data stored in a datetime / timestamp column is the same (it's just whether it's considered to be UTC or not that varies).

However, it's not clear to me how the migration would work though. Currently everything in date_due is stored in whatever the client's timezone was and displayed as-is. However, if we convert it to timestamp, it will then be considered to be UTC and adjusted as appropriate for display at the client. But because it wasn't stored as UTC, this will lead to incorrect times being displayed. Since we've no way of knowing the timezone of the client storing the value, I'm not sure how we can compensate for this.

Any ideas?
Comment 34 Martin Renvoize 2020-03-11 16:44:02 UTC
I think the timezone issues are actually to do with a lack of offset handling in dt_from_string: See bug 24850
Comment 35 Martin Renvoize 2020-03-17 15:04:53 UTC
I've been digging here..

dt_from_string did indeed ignore offsets which is bad.. but it also assumed a timezone that matches the configured instance timezone rather than respecting the 'Z' or '+0000' to mean 'UTC'.

As such.. whatever you passed in via the API would be stored 'as is', as a string with an assumed offset matching your instances configured timezone.

This patchset uses the js date function .toISOString which always returns a zero offset UTC string (and compensates for local timezone of the browser so our widget which returns a string of local time results in a date object with the local offset and as such when the ISO call is called above the offset is applied to get UTC time).

I've now updated bug 24850 to handle rfc3339 dates correctly and to always localise as to the instance config time for the output datetime object.. this will mean that if we pass in a rfc3339 string with or without an offset the resulting datetime will get properly localised to the instance time with the difference in offsets applied.

I believe this will resolve the issue face by Jonathan but would like to see some tests using hourly loans.  I think for a realistic case we should be setting instance time and browser locale to match.. I don't believe it's a fair case work with multiple timezones for a single library branch.. or is it?
Comment 36 Martin Renvoize 2020-03-26 10:16:30 UTC
As an aside, I still find it strange that we allow time granularly on loans that are measured in days rather than hours.
Comment 37 Martin Renvoize 2021-10-13 13:30:31 UTC
Created attachment 126195 [details] [review]
Bug 24608: Add EditDueDates syspref

This patch adds a EditDueDates syspref which determines whether a button
for editing due dates appears in a patron's checkouts list

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 38 Martin Renvoize 2021-10-13 13:30:35 UTC
Created attachment 126196 [details] [review]
Bug 24608: Allow due date modification

This patch adds the ability to modify due dates from a patron's
checkouts list.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 39 Martin Renvoize 2021-10-13 13:30:40 UTC
Created attachment 126197 [details] [review]
Bug 24608: (follow-up) Make "Edit" translatable

As per comment #14

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 40 Martin Renvoize 2021-10-13 13:30:45 UTC
Created attachment 126198 [details] [review]
Bug 24608: (follow-up) Buttons should be buttons

No idea why I used <a> elements for the Edit buttons. But the 'disabled'
attribute has no effect on <a> elements, which leads to all sorts of
problems when you have disabled buttons that are not disabled.

Switched to <button> elements.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 41 Martin Renvoize 2021-10-13 13:51:48 UTC
Created attachment 126201 [details] [review]
Bug 24608: (follow-up) Switch to bug 24455 dates

Bug 24455 does a far more comprehensive job of date parsing than my
single use case patch in bug 24650, therefore this bug has now switched
to using it

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 42 Martin Renvoize 2021-10-13 13:52:39 UTC
Rebased
Comment 43 Martin Renvoize 2021-10-13 13:54:25 UTC
This will likely need updating to flatpickr now too..
Comment 44 Jonathan Druart 2021-10-22 14:08:08 UTC
A note is needed: This is only affecting the UI, not the REST API routes.
Comment 45 Jonathan Druart 2021-10-22 14:09:53 UTC
Why isn't this a permission actually?
Comment 46 Katrin Fischer 2021-11-01 12:31:17 UTC
I am sorry, but this is currently blocked by bug 24609 being Failed QA. Please reset status as soon as the dependency has cleared up.
Comment 47 Katrin Fischer 2021-11-01 12:35:46 UTC
There is actually quite a long dependency tree above this one we'll need to work through first.

I still ran the qa tools here and here is a little glitch:

 FAIL	koha-tmpl/intranet-tmpl/prog/js/checkouts.js
   FAIL	  ES template literals
		ES template literals found (                    url: `/api/v1/checkouts/${checkoutId}`,) - See bug 24625
   OK	  git manipulation
Comment 48 Katrin Fischer 2022-01-31 20:36:53 UTC
Sorry, this depends on bug 24609, that is currently Failed QA... and no longer applies.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24609: Create PUT /checkouts/{checkout_id}
Using index info to reconstruct a base tree...
A	api/v1/swagger/definitions/checkout.json
A	api/v1/swagger/paths/checkouts.json
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): api/v1/swagger/paths/checkouts.json deleted in HEAD and modified in Bug 24609: Create PUT /checkouts/{checkout_id}. Version Bug 24609: Create PUT /checkouts/{checkout_id} of api/v1/swagger/paths/checkouts.json left in tree.
CONFLICT (modify/delete): api/v1/swagger/definitions/checkout.json deleted in HEAD and modified in Bug 24609: Create PUT /checkouts/{checkout_id}. Version Bug 24609: Create PUT /checkouts/{checkout_id} of api/v1/swagger/definitions/checkout.json left in tree.
error: Failed to merge in the changes.
Patch failed at 0001 Bug 24609: Create PUT /checkouts/{checkout_id}
Comment 49 Katrin Fischer 2023-06-25 14:37:57 UTC
*** Bug 17581 has been marked as a duplicate of this bug. ***