Without the dateaccessioned.pl, you had no datepicker widget, right?

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

(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.

Recreated on main - you need to disable 'strict_sql_modes' in the koha-conf - then it happens as described

(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?

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

(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

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.

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.

(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?

(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?

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.

Created attachment 171416 [details] [review]
Bug 37398: Make date and date time fields have date picker by default

Created attachment 171417 [details] [review]
Bug 37398: Adjust CSS for wrapped flatpickr

Created attachment 171419 [details] [review]
Bug 37398: Make sure subfield and subfield_date are defined

Created attachment 171420 [details] [review]
Bug 37398: Add a unit test

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        |
+----------------------+-------------+

Created attachment 171422 [details] [review]
Bug 37398: Adjust CSS for wrapped flatpickr

Created attachment 171423 [details] [review]
Bug 37398: Make sure subfield and subfield_date are defined

Created attachment 171424 [details] [review]
Bug 37398: Add a unit test

*** Bug 27768 has been marked as a duplicate of this bug. ***

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?

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.

Created attachment 171882 [details] [review]
Bug 37398: Adjust CSS for flatpickr

Created attachment 171919 [details] [review]
Bug 37398: Do not rely on the columns names but data type instead

+    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.

(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.

Either need a different unit test, or none, then. That breaks attachment 171424 [details] [review]

Created attachment 171963 [details] [review]
Bug 37398: Adjust tests

Test plan now fails at

6. If it is a datetime field you should have the option to add hours and minutes. ( datelastseen )

(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.

(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'

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.

(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.

(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

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.

The CSS change for .item_edit_form span.required needs to also apply to .item_edit_form span.important

Created attachment 172187 [details] [review]
Bug 37398: Add CSS for the important class

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>

Created attachment 172189 [details] [review]
Bug 37398: Adjust CSS for wrapped flatpickr

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

Created attachment 172190 [details] [review]
Bug 37398: Make sure subfield and subfield_date are defined

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

Created attachment 172191 [details] [review]
Bug 37398: Add a unit test

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

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>

Created attachment 172193 [details] [review]
Bug 37398: Adjust CSS for flatpickr

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

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>

Created attachment 172195 [details] [review]
Bug 37398: Adjust tests

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

Created attachment 172196 [details] [review]
Bug 37398: Add CSS for the important class

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

Created attachment 174558 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174559 [details] [review]
Bug 37398: Adjust CSS for wrapped flatpickr

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174560 [details] [review]
Bug 37398: Make sure subfield and subfield_date are defined

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174561 [details] [review]
Bug 37398: Add a unit test

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174562 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174563 [details] [review]
Bug 37398: Adjust CSS for flatpickr

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174564 [details] [review]
Bug 37398: Do not rely on the columns names but data type instead

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174565 [details] [review]
Bug 37398: Adjust tests

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 174566 [details] [review]
Bug 37398: Add CSS for the important class

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Great job, few remarks.

 WARN   Koha/UI/Form/Builder/Item.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 144, now: 154)
 WARN   t/db_dependent/Koha/UI/Form/Builder/Item.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 116, now: 127)
Ignoring the spaces here.

+    my $columns = Koha::Items->_resultset->result_source->columns_info;
+    $subfield_data{data_type} =
+        defined $subfield->{kohafield} ? $columns->{ $subfield->{kohafield} =~ s|^items\.||r }->{data_type} : undef;
Actually, is it possible to have an item subfield without a kohafield ? (No blocker)
Seems that the r modifier was added in Perl 5.14. So that looks okay to me. Felt new, but wasnt.

Feels like an enh btw.

> Feels like an enh btw.

I agree, will keep this for 25.05