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.
*** Bug 22064 has been marked as a duplicate of this bug. ***
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
Created attachment 98903 [details] [review] Bug 24608: Allow due date modification This patch adds the ability to modify due dates from a patron's checkouts list.
Created attachment 98908 [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
Created attachment 98909 [details] [review] Bug 24608: Allow due date modification This patch adds the ability to modify due dates from a patron's checkouts list.
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
Created attachment 98914 [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
Created attachment 98915 [details] [review] Bug 24608: Allow due date modification This patch adds the ability to modify due dates from a patron's checkouts list.
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
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.
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 on attachment 98932 [details] [review] Bug 24608: Allow due date modification Is the "Edit" in checkouts.js untranslatable?
(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.
(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.
Created attachment 99240 [details] [review] Bug 24608: (follow-up) Use strings.inc for "Edit" As per comment #14
I'm getting an error when I save an edited date, "ReferenceError: ISO_to_syspref is not defined checkouts_19.1200030.js:1135:45"
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?
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
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
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.
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>
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>
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>
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>
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>
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.
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.
(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.
(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?
(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".
OK, if we can assume the time should be 23:59, then I agree :)
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
(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?
I think the timezone issues are actually to do with a lack of offset handling in dt_from_string: See bug 24850
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?
As an aside, I still find it strange that we allow time granularly on loans that are measured in days rather than hours.
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>
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>
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>
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>
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>
Rebased
This will likely need updating to flatpickr now too..
A note is needed: This is only affecting the UI, not the REST API routes.
Why isn't this a permission actually?
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.
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
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}
*** Bug 17581 has been marked as a duplicate of this bug. ***