Bug 37398 - Initialize a datepicker on all date/datetime fields when adding/editing items
Summary: Initialize a datepicker on all date/datetime fields when adding/editing items
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Lucas Gass (lukeg)
QA Contact: Testopia
URL:
Keywords:
: 27768 (view as bug list)
Depends on: 38027
Blocks: 38001
  Show dependency treegraph
 
Reported: 2024-07-17 21:56 UTC by Eric Swenson
Modified: 2024-09-27 22:11 UTC (History)
7 users (show)

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


Attachments
Bug 37398: Make date and date time fields have date picker by default (3.01 KB, patch)
2024-09-12 16:09 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Adjust CSS for wrapped flatpickr (987 bytes, patch)
2024-09-12 16:24 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Make sure subfield and subfield_date are defined (1.86 KB, patch)
2024-09-12 16:45 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Add a unit test (2.18 KB, patch)
2024-09-12 17:36 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Make date and date time fields have date picker by default (4.23 KB, patch)
2024-09-12 17:49 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Adjust CSS for wrapped flatpickr (987 bytes, patch)
2024-09-12 17:49 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Make sure subfield and subfield_date are defined (1.86 KB, patch)
2024-09-12 17:49 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Add a unit test (2.18 KB, patch)
2024-09-12 17:49 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Add set to today function to calendar.inc (3.19 KB, patch)
2024-09-23 16:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Adjust CSS for flatpickr (1008 bytes, patch)
2024-09-23 16:34 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Do not rely on the columns names but data type instead (2.09 KB, patch)
2024-09-24 07:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37398: Adjust tests (1.19 KB, patch)
2024-09-26 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37398: Add CSS for the important class (785 bytes, patch)
2024-09-27 22:06 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 37398: Make date and date time fields have date picker by default (4.29 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Adjust CSS for wrapped flatpickr (1.02 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Make sure subfield and subfield_date are defined (1.91 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Add a unit test (2.23 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Add set to today function to calendar.inc (3.24 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Adjust CSS for flatpickr (1.04 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Do not rely on the columns names but data type instead (2.14 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Adjust tests (1.24 KB, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37398: Add CSS for the important class (838 bytes, patch)
2024-09-27 22:11 UTC, Phil Ringnalda
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Swenson 2024-07-17 21:56:11 UTC
A typed in value that follows the dateFormat system preference (mm/dd/yyyy) is changed to 0000-00-00 when the item is saved. This was found in 952$w before enabling the dateaccessioned.pl plugin for the subfield.

To recreate:
-set system preference dateFormat to mm/dd/yyyy
-Go to an existing bib record
-Edit items
-input a date following the specified format  (07-17-2024)
-save the item
-check the item and notice the date has been changed to 0000-00-00
-click back to the bib record
-Error 500: Internal server error - Month out of range
Comment 1 Katrin Fischer 2024-07-18 07:26:49 UTC
Without the dateaccessioned.pl, you had no datepicker widget, right?
Comment 2 Lucas Gass (lukeg) 2024-07-18 13:51:34 UTC
I can't reproduce this in main. I did the following in k-t-d:

1. Remove the dateaccessioned.pl plugin for the 952$w in the framework I am using.
2. Add/edit item and enter a date in the 'w - Price effective from' in the format specified in my dateFormat system preference. I get:

Invalid value passed, items.replacementpricedate=07/17/2024 expected type is date at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 3 Eric Swenson 2024-07-18 14:38:08 UTC
(In reply to Katrin Fischer from comment #1)
> Without the dateaccessioned.pl, you had no datepicker widget, right?

Correct. It was just an empty input box.
Comment 4 Nick Clemens (kidclamp) 2024-08-20 12:28:15 UTC
Recreated on main - you need to disable 'strict_sql_modes' in the koha-conf - then it happens as described
Comment 5 Katrin Fischer 2024-08-20 12:31:39 UTC
(In reply to Nick Clemens (kidclamp) from comment #4)
> Recreated on main - you need to disable 'strict_sql_modes' in the koha-conf
> - then it happens as described

That's interesting. But doesn't that break a lot of other things like circulation reports too?
Comment 6 Nick Clemens (kidclamp) 2024-08-20 12:55:34 UTC
I think what we should do is:
 - Make a list of all 'date' fields in the items table
 - If a subfield in the items form is mapped to one of the fields above then we use a datepicker explicitly - whether or not plugin is enabled
Comment 7 Nick Clemens (kidclamp) 2024-08-20 12:56:49 UTC
(In reply to Katrin Fischer from comment #5)
> (In reply to Nick Clemens (kidclamp) from comment #4)
> > Recreated on main - you need to disable 'strict_sql_modes' in the koha-conf
> > - then it happens as described
> 
> That's interesting. But doesn't that break a lot of other things like
> circulation reports too?

Disabling strict mode? I don't think so - enabling it makes things stricter - it's on by default in KTD so we don't break the rules further - but I don't believe most run it in production, we don't
Comment 8 Katrin Fischer 2024-08-20 13:00:17 UTC
I think I read it the wrong way around. I thought you said "make it stricter". So things here break if you make things less strict.
Comment 9 Lucas Gass (lukeg) 2024-08-21 14:59:24 UTC
My opinion, for what it is worth, is that the dateaccessioned.pl plugin should be automatically set on date and datetime fields with no ability to remove via frameworks.
Comment 10 Katrin Fischer 2024-08-21 15:04:34 UTC
(In reply to Lucas Gass from comment #9)
> My opinion, for what it is worth, is that the dateaccessioned.pl plugin
> should be automatically set on date and datetime fields with no ability to
> remove via frameworks.

Just a suggestion: can we rename it, if we do that? Otherwise we will probably be stuck forever. I also thought we already had it added to other fields as well. 

Does it more than just adding the date widget? Because than we might just do that without a plugin?

What about timestamp? The time part might pose a problem?
Comment 11 Lucas Gass (lukeg) 2024-08-21 16:02:05 UTC
(In reply to Katrin Fischer from comment #10)
> (In reply to Lucas Gass from comment #9)
> > My opinion, for what it is worth, is that the dateaccessioned.pl plugin
> > should be automatically set on date and datetime fields with no ability to
> > remove via frameworks.
> 
> Just a suggestion: can we rename it, if we do that? Otherwise we will
> probably be stuck forever. I also thought we already had it added to other
> fields as well. 
> 
> Does it more than just adding the date widget? Because than we might just do
> that without a plugin?
> 
> What about timestamp? The time part might pose a problem?

The other thing it does is set_to_today when you click on field. 

We can deal with date and datetime, I think. We already do this in other places with Flatpickr/Maskito.

I think renaming the plugin is a good idea.

The last question I have is, should it continue to be a plugin? Or should we just make it not a plugin?
Comment 12 Katrin Fischer 2024-08-21 16:19:31 UTC
I think if you save empty, it also saves today's date. But we might need to verify if that is enough. 

I am open to it being not a plugin.
Comment 13 Lucas Gass (lukeg) 2024-09-12 16:09:14 UTC
Created attachment 171416 [details] [review]
Bug 37398: Make date and date time fields have date picker by default
Comment 14 Lucas Gass (lukeg) 2024-09-12 16:24:32 UTC
Created attachment 171417 [details] [review]
Bug 37398: Adjust CSS for wrapped flatpickr
Comment 15 Lucas Gass (lukeg) 2024-09-12 16:45:02 UTC
Created attachment 171419 [details] [review]
Bug 37398: Make sure subfield and subfield_date are defined
Comment 16 Lucas Gass (lukeg) 2024-09-12 17:36:21 UTC
Created attachment 171420 [details] [review]
Bug 37398: Add a unit test
Comment 17 Lucas Gass (lukeg) 2024-09-12 17:49:26 UTC
Created attachment 171421 [details] [review]
Bug 37398: Make date and date time fields have date picker by default

To test:
1. APPLY PATCH, restart_all
2. Pick a framework, I choose BKS in k-t-d
3. Make sure that a few date and datetime fields are exposed in the editor.
4. On additem.tt a date picker should be instantiated on the date or datetime fields.
5. Make sure the date pickers work on each field.
6. If it is a datetime field you should have the option to add hours and minutes. ( datelastseen )
7. Edit the framework and remove the dateaccessioned.pl plugin from any of these date fields.
8. Even without the dateaccessioned.pl plugin a date picker should be present on the field.
9. Add the dateaccessioned.pl plugin to a non date/datetime field.
10. Make sure the dateaccessioned.pl plugin works properly on the non date/datetime field.
11. Mark one of the date/datetime fields as mandatory, make sure it still acts as mandatory.
12. Hide date/datetime from the editor, make sure it hides properly.

+----------------------+-------------+
| COLUMN_NAME          | COLUMN_TYPE |
+----------------------+-------------+
| dateaccessioned      | date        |
| replacementpricedate | date        |
| datelastborrowed     | date        |
| datelastseen         | datetime    |
| onloan               | date        |
+----------------------+-------------+
Comment 18 Lucas Gass (lukeg) 2024-09-12 17:49:34 UTC
Created attachment 171422 [details] [review]
Bug 37398: Adjust CSS for wrapped flatpickr
Comment 19 Lucas Gass (lukeg) 2024-09-12 17:49:40 UTC
Created attachment 171423 [details] [review]
Bug 37398: Make sure subfield and subfield_date are defined
Comment 20 Lucas Gass (lukeg) 2024-09-12 17:49:45 UTC
Created attachment 171424 [details] [review]
Bug 37398: Add a unit test
Comment 21 Phil Ringnalda 2024-09-13 17:27:49 UTC
*** Bug 27768 has been marked as a duplicate of this bug. ***
Comment 22 Phil Ringnalda 2024-09-13 19:03:53 UTC
Snag that becomes very visible at step 11:

Without the patch, a subfield using dateaccessioned.pl has a "..." at the end of the input, clicking it fills in Today without opening Flatpickr, and if the subfield is mandatory, the "..." pushes the "Required" label over to the right.

With the patch, the "..." is replaced by a big red "X" to clear the field, which overlaps the Required label instead of pushing it over to the right, making it visually a sort of "℞equired" only with more X

I'm guessing, without being sure, that it's an acceptable keyboard accessibility change that instead of actively changing the date to today by tabbing past Flatpickr (not thinking about what a screenreader does as it opens) to the "..." and hitting enter, you instead tab past it to something labelled "Clear date" and hit enter on it, trusting that it will become today when you save with it empty?
Comment 23 Lucas Gass (lukeg) 2024-09-23 16:25:17 UTC
Created attachment 171881 [details] [review]
Bug 37398: Add set to today function to calendar.inc

To test:
1. Try editing some items with a empty value in the flatpickr.
2. Make sure the empty date fields get a date set to today when you click on the empty field.
3. Try a datetime field, making sure the datetime is correctly set.
Comment 24 Lucas Gass (lukeg) 2024-09-23 16:34:33 UTC
Created attachment 171882 [details] [review]
Bug 37398: Adjust CSS for flatpickr
Comment 25 Jonathan Druart 2024-09-24 07:47:28 UTC
Created attachment 171919 [details] [review]
Bug 37398: Do not rely on the columns names but data type instead
Comment 26 Jonathan Druart 2024-09-24 07:47:58 UTC
+    if ($subfield->{kohafield} =~ /items\.(replacementpricedate|dateaccessioned|datelastborrowed|onloan)/) {
+        $subfield_data{datetype} = 'date';
+    }
+
+    if ($subfield->{kohafield} =~ /items\.(datelastseen|damaged_on|itemlost_on|withdrawn_on|deleted_on)/) {
+        $subfield_data{datetype} = 'datetime';
+    }

I think I would much prefer to see something generic that this list of fields.

See the last patch, for discussion.
Comment 27 Lucas Gass (lukeg) 2024-09-24 13:40:35 UTC
(In reply to Jonathan Druart from comment #26)
> +    if ($subfield->{kohafield} =~
> /items\.(replacementpricedate|dateaccessioned|datelastborrowed|onloan)/) {
> +        $subfield_data{datetype} = 'date';
> +    }
> +
> +    if ($subfield->{kohafield} =~
> /items\.(datelastseen|damaged_on|itemlost_on|withdrawn_on|deleted_on)/) {
> +        $subfield_data{datetype} = 'datetime';
> +    }
> 
> I think I would much prefer to see something generic that this list of
> fields.
> 
> See the last patch, for discussion.

This had crossed my mind. I agree with the change, Jonathan. Thanks.
Comment 28 Phil Ringnalda 2024-09-24 20:42:08 UTC
Either need a different unit test, or none, then. That breaks attachment 171424 [details] [review]
Comment 29 Jonathan Druart 2024-09-26 12:19:34 UTC
Created attachment 171963 [details] [review]
Bug 37398: Adjust tests
Comment 30 Phil Ringnalda 2024-09-26 22:43:39 UTC
Test plan now fails at

6. If it is a datetime field you should have the option to add hours and minutes. ( datelastseen )
Comment 31 Lucas Gass (lukeg) 2024-09-27 13:46:52 UTC
(In reply to Phil Ringnalda from comment #30)
> Test plan now fails at
> 
> 6. If it is a datetime field you should have the option to add hours and
> minutes. ( datelastseen )

What item field and framework are you testing? This works for me.
Comment 32 Lucas Gass (lukeg) 2024-09-27 13:51:07 UTC
(In reply to Lucas Gass from comment #31)
> (In reply to Phil Ringnalda from comment #30)
> > Test plan now fails at
> > 
> > 6. If it is a datetime field you should have the option to add hours and
> > minutes. ( datelastseen )
> 
> What item field and framework are you testing? This works for me.

The only item field to really test this on is datelastseen. I do get a flatpickr with the data-flatpickr-enable-time attribute. I am going to reset this to 'Needs sign-off'
Comment 33 Phil Ringnalda 2024-09-27 15:29:49 UTC
Oh, that's what I was doing, still looking at the keyboard user wanting to set things to today without having the former plugin-provided "..."

Steps to reproduce failure:
1. Set 952$r to be visible in Editor
2. Edit an item, existing or new
3. Click the red X to clear the value from 952$r
4. Click in the input for 952$r

You now have a date picker, not a datetime picker.
Comment 34 Lucas Gass (lukeg) 2024-09-27 15:57:29 UTC
(In reply to Phil Ringnalda from comment #33)
> Oh, that's what I was doing, still looking at the keyboard user wanting to
> set things to today without having the former plugin-provided "..."
> 
> Steps to reproduce failure:
> 1. Set 952$r to be visible in Editor
> 2. Edit an item, existing or new
> 3. Click the red X to clear the value from 952$r
> 4. Click in the input for 952$r
> 
> You now have a date picker, not a datetime picker.

OK, I see what you mean now. Thanks for clarifying. Feel free to set back to FQA, I'll follow-up asap.
Comment 35 Lucas Gass (lukeg) 2024-09-27 20:07:00 UTC
(In reply to Phil Ringnalda from comment #33)
> Oh, that's what I was doing, still looking at the keyboard user wanting to
> set things to today without having the former plugin-provided "..."
> 
> Steps to reproduce failure:
> 1. Set 952$r to be visible in Editor
> 2. Edit an item, existing or new
> 3. Click the red X to clear the value from 952$r
> 4. Click in the input for 952$r
> 
> You now have a date picker, not a datetime picker.

After testing I notice this is happening on all datetime pickers, with or without this patch. I filed this separately: 

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38027
Comment 36 Phil Ringnalda 2024-09-27 21:34:13 UTC
I'm very carefully not noticing that saving an item with a blank datelastseen sets it to the current time in the database's timezone, and clicking in a blank datelastseeen field to trigger flatpickr and saving sets it to the current time in the browser's timezone, because if I notice timezone conversion failures I have to go outside and wander around until I forget what I saw.
Comment 37 Phil Ringnalda 2024-09-27 21:37:49 UTC
The CSS change for .item_edit_form span.required needs to also apply to .item_edit_form span.important
Comment 38 Lucas Gass (lukeg) 2024-09-27 22:06:10 UTC
Created attachment 172187 [details] [review]
Bug 37398: Add CSS for the important class
Comment 39 Phil Ringnalda 2024-09-27 22:11:24 UTC
Created attachment 172188 [details] [review]
Bug 37398: Make date and date time fields have date picker by default

To test:
1. APPLY PATCH, restart_all
2. Pick a framework, I choose BKS in k-t-d
3. Make sure that a few date and datetime fields are exposed in the editor.
4. On additem.tt a date picker should be instantiated on the date or datetime fields.
5. Make sure the date pickers work on each field.
6. If it is a datetime field you should have the option to add hours and minutes. ( datelastseen )
7. Edit the framework and remove the dateaccessioned.pl plugin from any of these date fields.
8. Even without the dateaccessioned.pl plugin a date picker should be present on the field.
9. Add the dateaccessioned.pl plugin to a non date/datetime field.
10. Make sure the dateaccessioned.pl plugin works properly on the non date/datetime field.
11. Mark one of the date/datetime fields as mandatory, make sure it still acts as mandatory.
12. Hide date/datetime from the editor, make sure it hides properly.

+----------------------+-------------+
| COLUMN_NAME          | COLUMN_TYPE |
+----------------------+-------------+
| dateaccessioned      | date        |
| replacementpricedate | date        |
| datelastborrowed     | date        |
| datelastseen         | datetime    |
| onloan               | date        |
+----------------------+-------------+

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 40 Phil Ringnalda 2024-09-27 22:11:27 UTC
Created attachment 172189 [details] [review]
Bug 37398: Adjust CSS for wrapped flatpickr

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 41 Phil Ringnalda 2024-09-27 22:11:30 UTC
Created attachment 172190 [details] [review]
Bug 37398: Make sure subfield and subfield_date are defined

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 42 Phil Ringnalda 2024-09-27 22:11:33 UTC
Created attachment 172191 [details] [review]
Bug 37398: Add a unit test

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 43 Phil Ringnalda 2024-09-27 22:11:36 UTC
Created attachment 172192 [details] [review]
Bug 37398: Add set to today function to calendar.inc

To test:
1. Try editing some items with a empty value in the flatpickr.
2. Make sure the empty date fields get a date set to today when you click on the empty field.
3. Try a datetime field, making sure the datetime is correctly set.

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 44 Phil Ringnalda 2024-09-27 22:11:39 UTC
Created attachment 172193 [details] [review]
Bug 37398: Adjust CSS for flatpickr

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 45 Phil Ringnalda 2024-09-27 22:11:42 UTC
Created attachment 172194 [details] [review]
Bug 37398: Do not rely on the columns names but data type instead

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 46 Phil Ringnalda 2024-09-27 22:11:45 UTC
Created attachment 172195 [details] [review]
Bug 37398: Adjust tests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 47 Phil Ringnalda 2024-09-27 22:11:49 UTC
Created attachment 172196 [details] [review]
Bug 37398: Add CSS for the important class

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>