Bug 36594 - Library hours display issues
Summary: Library hours display issues
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: CJ Lynce
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 6796
Blocks: 37662 38043 38081
  Show dependency treegraph
 
Reported: 2024-04-12 12:01 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2024-11-18 22:23 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the display of library hours in the staff interface (Administration > Basic parameters > Libraries): This patch corrects the following issues related to the display of library -Newly created or edited libraries no longer display 'null' for undefined open and close times. -Libraries without any defined hours will state such instead of displaying the hours table. -The CalendarFirstDayOfWeek system preferences is now respected when viewing a library with defined hours. -Time displays and inputs now follow the TimeFormat system preference. -Times are no longer displayed with seconds. -A TT filter, KohaTimes, has been added to handle proper formatting of time strings based on systems preferences.
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 36594: Library hours display issues (7.81 KB, patch)
2024-08-13 20:54 UTC, CJ Lynce
Details | Diff | Splinter Review
Bug 36594: Library hours display issues (7.86 KB, patch)
2024-08-16 20:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences. (13.10 KB, patch)
2024-09-06 20:35 UTC, CJ Lynce
Details | Diff | Splinter Review
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences. (12.94 KB, patch)
2024-09-06 20:53 UTC, CJ Lynce
Details | Diff | Splinter Review
Sql results libraries_hours (20.93 KB, image/png)
2024-09-09 15:46 UTC, Olivier Vezina
Details
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences. (12.94 KB, patch)
2024-09-09 17:05 UTC, CJ Lynce
Details | Diff | Splinter Review
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences. (13.00 KB, patch)
2024-09-09 18:33 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences. (13.61 KB, patch)
2024-09-24 19:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36594: (RM follow-up) Add new filter to filters test (1.02 KB, patch)
2024-09-30 17:22 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2024-04-12 12:01:17 UTC
There are some glitches on the libraries table:

* If not defined, the string literal 'null' is displayed
* If you set 9:00 as the opening hour, 09:00::00 is displayed

The feature authors need to:

* Find a good approach to undefined values here (do we need to display days when the library is not open?)
* Use the formatting tools to display time correctly.
Comment 1 Katrin Fischer 2024-04-13 11:16:53 UTC
+1 thank you for filing.
Comment 2 Martin Renvoize (ashimema) 2024-04-15 17:27:26 UTC
This is one of the things I was getting at when I said we really need to link the closed days and the opening hours functionalities.. For regular closed days we shouldn't display opening hours at all.. just mark the whole day as closed in that row.

The other time output I would be keen to have fixed too and agree.
Comment 3 David Nind 2024-05-30 00:33:41 UTC
I can confirm the display issues.

To replicate:
1. Go to Koha administration > Basic parameters > Libraries
2. Edit any library.
3. Submit (without making any changes).
4. In the library hours column, all days are displayed with null
5. If you select a time for opening or closing, it displays with seconds, for example: 08:00:00

In addition, I have these suggestions (maybe separate bugs?):
1. Column headings: maybe change 'Open time' and 'Close time' to 'Opens' and 'Closes'. Or maybe 'Opening time' and 'Closing time' - realising that column width is at a premium, and not sure how this affects translation.
2. Option to select that the library is closed for the day, and displays in the table as 'Closed'
3. Option to organise the order of the day column - displays as Sunday to Monday, which I think is an American convention. Here in New Zealand, the first day of the week is Monday.
Comment 4 Caroline Cyr La Rose 2024-07-24 16:19:38 UTC
(In reply to David Nind from comment #3)

> 3. Option to organise the order of the day column - displays as Sunday to
> Monday, which I think is an American convention. Here in New Zealand, the
> first day of the week is Monday.

There is a system preference, CalendarFirstDayOfWeek, that determines the first day of the week. I tried changing it to Wednesday and the days are rearranged in the library display. So I think this part is OK.

I agree with all the rest and would add (although that might be Tomás' first point) to display the text "Library hours not set" if everything is null. In KTD, and I assume in other existing systems, all the libraries currently show as "Library hours not set" in the main branches table. But if you create a new library, or edit an existing library, the table with 'null' everywhere appears.
Comment 5 Caroline Cyr La Rose 2024-07-24 16:23:05 UTC
(In reply to Caroline Cyr La Rose from comment #4)
> (In reply to David Nind from comment #3)
> 
> > 3. Option to organise the order of the day column - displays as Sunday to
> > Monday, which I think is an American convention. Here in New Zealand, the
> > first day of the week is Monday.
> 
> There is a system preference, CalendarFirstDayOfWeek, that determines the
> first day of the week. I tried changing it to Wednesday and the days are
> rearranged in the library display. So I think this part is OK.

I retract that statement, it is not 100% OK. The hours table in the main library table (the first one when you go to Administration > Libraries) is subject to CalendarFirstDayOfWeek. The one in the form (new library or edit existing) is also OK.
BUT if you click on the name of the library, the days table is displayed with Sunday first regardless of CalendarFirstDayOfWeek.
So it's 66% OK ;)
Comment 6 David Nind 2024-07-29 01:51:31 UTC
Thanks Caroline! I didn't know about the CalendarFirstDayOfWeek system preference.
Comment 7 CJ Lynce 2024-08-13 20:54:05 UTC
Created attachment 170298 [details] [review]
Bug 36594: Library hours display issues

The following issues existing when displays library hours in
	koha/admin/branches.pl
-For newly created or edited libraries, 'null' is displayed when that
	time is undefined.
-Time is displayed with seconds, which are not necessary.
-The system preference CalendarFirstDayOfWeek is not respected when
	viewing a specific library with hours defined.

This patch corrects these issues:
-Undefined values are properly handled. All open & close times are
	undefined, the library is considered as not having hours set.
-If open and close times for a day are both undefined, the library
	is considered closed.
-Time is now displayed in the format HH:MM
-CalendarFirstDayOfWeek is respected when viewing a library with
	defined hours.

To Test:

1. Login to staff interface
2. Go to Koha administration > Basic parameters > Libraries
3. Edit any library or create a new library. Do not set hours. Save.
4. On the library list, in the hours column, all days are displayed
	with null.
5. Edit or create a 2nd library, set some hours, leaving at least one
	day without hours. Save.
6. In the library list, in the hours column, defined times are in the
	format HH:MM:SS.
7. Edit the system preference CalendarFirstDayOfWeek to set a day other
	than Sunday as the first day of the week.
8. Return to Go to Koha administration > Basic parameters > Libraries
9. Click on the name of a specific library with hours to view details.
10. The order of the calendar week still starts with Sunday.

11. Apply Patch.
12. Return to Go to Koha administration > Basic parameters > Libraries
13. Check that your library with all undefined times shows "Library
	hours not set".
14. Check that your library with defined hours is showing times in
	HH:MM format, and days without defined hours are "Closed".
15. Click on the name of a library with hours to view details. Verify
	that the calendar week starts respects CalendarFirstDayOfWeek.
16. Click on the name of another library without hours to view details.
	Verify these libraries show as "No opening hours have been set"
	in the branch detail view.

Sponsored-by: Westlake Porter Public Library
Comment 8 CJ Lynce 2024-08-13 21:06:01 UTC
Please take a look at a first try at addressing the issues here.

The following assumptions/decisions apply:
A) Days with both open and close times undefined are considered 'CLOSED'.
B) It might be possible to have a library open one day and close the next? This would mean that an opening time is defined, but a closing time is not for one day, and a closing time defined for the next day, but no opening time.

Regarding B) The more I think about this, I don't think this is going to work well. If a library opened on a Monday, and remained opened straight through to Wednesday, Tuesday would shows as closed since both times are undefined.

I also don't have a great understanding yet of all the places these times are used (e.g. calculating due dates/times, or displaying in the OPAC?), but I would imagine allowing and opening time, and no closing time for a day would cause problems?

I think a better way would be to require BOTH open and close times for a day for it to be considered valid. If a library is opening spanning days, do it 'close' at 23:59, and open at 0:00 the next day?

I welcome any comments on these thoughts.
Comment 9 Katrin Fischer 2024-08-15 09:54:54 UTC
Hi CJ, I feel these are all great questions, but I wonder if we should not split them to a separate bug as this was mostly to deal with the display issues, not affecting the functionality. Do you agree?
Comment 10 CJ Lynce 2024-08-16 15:52:33 UTC
Katrin, that makes sense to keep this bug focused on getting the currently collected data to display correctly. In that case, the submitted the patch should cover it.

I'll do some additional digging to better understand how open and closed hours are used/not used, and open another bug for functionality changes. Bug 36079 will likely tie into an ideal solution with this.
Comment 11 David Nind 2024-08-16 20:05:21 UTC
Created attachment 170460 [details] [review]
Bug 36594: Library hours display issues

The following issues existing when displays library hours in
	koha/admin/branches.pl
-For newly created or edited libraries, 'null' is displayed when that
	time is undefined.
-Time is displayed with seconds, which are not necessary.
-The system preference CalendarFirstDayOfWeek is not respected when
	viewing a specific library with hours defined.

This patch corrects these issues:
-Undefined values are properly handled. All open & close times are
	undefined, the library is considered as not having hours set.
-If open and close times for a day are both undefined, the library
	is considered closed.
-Time is now displayed in the format HH:MM
-CalendarFirstDayOfWeek is respected when viewing a library with
	defined hours.

To Test:

1. Login to staff interface
2. Go to Koha administration > Basic parameters > Libraries
3. Edit any library or create a new library. Do not set hours. Save.
4. On the library list, in the hours column, all days are displayed
	with null.
5. Edit or create a 2nd library, set some hours, leaving at least one
	day without hours. Save.
6. In the library list, in the hours column, defined times are in the
	format HH:MM:SS.
7. Edit the system preference CalendarFirstDayOfWeek to set a day other
	than Sunday as the first day of the week.
8. Return to Go to Koha administration > Basic parameters > Libraries
9. Click on the name of a specific library with hours to view details.
10. The order of the calendar week still starts with Sunday.

11. Apply Patch.
12. Return to Go to Koha administration > Basic parameters > Libraries
13. Check that your library with all undefined times shows "Library
	hours not set".
14. Check that your library with defined hours is showing times in
	HH:MM format, and days without defined hours are "Closed".
15. Click on the name of a library with hours to view details. Verify
	that the calendar week starts respects CalendarFirstDayOfWeek.
16. Click on the name of another library without hours to view details.
	Verify these libraries show as "No opening hours have been set"
	in the branch detail view.

Sponsored-by: Westlake Porter Public Library
Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2024-08-16 21:29:08 UTC
Thank you for providing a patch to fix this!

I've added a draft release note - feel free to update it if I haven't got it right.

Some other things I noted - for fixing on other bugs:

1. Entering the time manually: you can only manually enter the time up to 12 in either of the open or close time input fields (irrespective of the TimeFormat system preference) - for example, for the close time column you can't manually enter 14:00 (you can select it from the input control)

2. When editing the opening and closing times, if you select the X it then displays the date picker.

3. Using the TimeFormat system preference: the selector takes this system preference into account, but then displays the time using 24 hours (for example: entering 4:00 PM as the closing time displays as 16:00).

4. Text inconsistencies - I have created bug 37662 for these: 
   4.1 If no library opening and closing times are set, it displays in the library hours column as "Library hours not set". However, if you view the library details (selecting the library name in the name column), the text is "No opening hours have been set."
   4.2 The column name in the list of libraries is "Library hours". However, if you view the library details (selecting the library name in the name column), the field label is "Opening hours". "Opening hours" is also used as the field label when adding and editing library information.
Comment 13 Katrin Fischer 2024-08-19 08:03:16 UTC
I wonder if 1. might also depend on some local settings. Would you mind filing these separately?
Comment 14 CJ Lynce 2024-08-19 11:08:04 UTC
I suspect 1. & 2. are due to config issues with the use of flatpickr for time only. Manually typing in a time formats as a date when you try specify minutes or an hour larger than 12. This appears to be the only place flatpickr is configured to just do time and not date only or date&time. 

3. may or may not be flatpickr, as it looks like the current code (koha-tmpl/intranet-tmpl/prog/en/includes/calendar.inc) is trying to honor the 24 or 12 hour system variable.

I'm going to try to get new bugs opened for this later today, unless someone beats me to them.
Comment 15 Martin Renvoize (ashimema) 2024-08-21 07:59:01 UTC
This is a good start.. though I feel it can be misleading right now.

I'd really like to see calendar taken into account so we're not marking 24 hour opening days as Closed and rather actually using the regular closed days for marking Closed.

I also see we're not formatting the times as per the system preference yet.

I'm afraid it's a fail for me.. whilst I'm keen to see this fixed I think we need to go further.
Comment 16 Martin Renvoize (ashimema) 2024-08-21 08:07:52 UTC
To get the time format correct we probably want a Koha/Template/Plugin/KohaTimes.pm template filter adding.
Comment 17 CJ Lynce 2024-08-28 19:11:02 UTC
Thanks for the feedback. I agree that better integration with the calendar is ultimately needed, and I'll list my thoughts on that over on bug_36079.

Limiting the fix for this bug to resolving the 'null' display issue that appears when you edit or add a library is a fairly basic fix compared to appropriately pulling in calendar data so that it relates to 'regular hours'. (The 'null matrix that shows when no hours are defined for the library is a massive space waster for systems with many libraries).

I will look at respecting the time format when displaying, though if going the route of creating a KohaTimes.pm filter, that is a bit above my skills wheelhouse still, but I'm willing to give it a shot with some guidance?

Related to dates without hours defined, leaving them blank rather than 'Closed' is probably a better option, and again focuses on display/formatting issues.

I'd posit taking this small bite out here, then taking the larger work of integration on bug_36079.

Thoughts?
Comment 18 CJ Lynce 2024-09-06 20:35:34 UTC
Created attachment 171152 [details] [review]
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences.

-Newly created or edited libraries no longer display 'null' for undefined
	open and close times.
-Libraries without any defined hours will state such instead of displaying the
	hours table.
-The CalendarFirstDayOfWeek system preferences is now respected when viewing
	a library with defined hours.
-Time displays and inputs now follow the TimeFormat system preference.
-Times are no longer displayed with seconds.

As part of this patch, a new Template Filter, KohaTimes, has been added to
	allow for proper formatting of time strings based on systems preferences.

To Test:
1. Login to staff interface
2. Go to Koha administration > Basic parameters > Libraries
3. Edit any library or create a new library. Do not set hours.
4. Save the library.
	*Note that on the library list, in the hours column, all days are displayed
	with 'null'.
5. Edit or create a 2nd library.
6. Try to manually enter a time (not using the flatpickr modal)
	*Note that the mask wants a date format, not a time format.
7. Set some hours for the library, leaving at least one
	day without hours. Save.
	*Note that in the library list, in the hours column, defined times are in
	the format HH:MM:SS.
7. Edit the system preference CalendarFirstDayOfWeek to set a day other
	than Sunday as the first day of the week.
8. Return to Koha administration > Basic parameters > Libraries
9. Click on the name of a specific library with hours to view details.
	*Note that the order of the calendar weeek still starts with Sunday.
10. The order of the calendar week still starts with Sunday.
11. Edit the system preference TimeFormat and set to 12-hours.
12. Return to Koha administration > Basic parameters > Libraries.
	*Note that the time format on the branches list is not in 12-hour format.
13. Click on the name of a specific library with hours to view details.
	*Note that the time format in the branch details screen is not in 12-hour
	format.

14. Apply Patch.

15. Return to Koha administration > Basic parameters > Libraries
16. Verify that your library with all undefined times shows "Library hours
	not set".
17. Verify that your library with defined hours is showing times in	HH:MM
	format, and days without defined hours are blank.
18. Verify that your library with defined hours is respecting the TimeFormat
	preferences.
19. Click on the name of a library with hours to view details.
20. Verify that the hours respect the CalendarFirstDayOfWeek and TimeFormat
	preferences.
21. Click on the name of another library without hours to view details.
	Verify these libraries show as "No opening hours have been set"
	in the branch detail view.
22. Edit a library's hours. When inputing, try to manually enter a time (not
	using the flatpickr modal). Verify that you can enter time in the proper
	(12 or 24 hours) format.

Sponsored-by: Westlake Porter Public Library
Comment 19 CJ Lynce 2024-09-06 20:43:17 UTC
Here's and update patch that addresses a myriad of display issues brought up in Comment 1, 12, and 15, with the exception of the topics in bug_36079 (integrating the calendar and hours better) and bug_37662 (string changes)

I added a new KohaTimes filter for TT to handle the 12/24 hour formatting (KohaDates does not handle time-only inputs)
Comment 20 CJ Lynce 2024-09-06 20:43:58 UTC
Here's and update patch that addresses a myriad of display issues brought up in Comment 1, 12, and 15, with the exception of the topics in bug_36079 (integrating the calendar and hours better) and bug_37662 (string changes)

I added a new KohaTimes filter for TT to handle the 12/24 hour formatting (KohaDates does not handle time-only inputs)
Comment 21 CJ Lynce 2024-09-06 20:53:03 UTC
Created attachment 171153 [details] [review]
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences.

-Newly created or edited libraries no longer display 'null' for undefined
	open and close times.
-Libraries without any defined hours will state such instead of displaying the
	hours table.
-The CalendarFirstDayOfWeek system preferences is now respected when viewing
	a library with defined hours.
-Time displays and inputs now follow the TimeFormat system preference.
-Times are no longer displayed with seconds.

As part of this patch, a new Template Filter, KohaTimes, has been added to
	allow for proper formatting of time strings based on systems preferences.

To Test:
1. Login to staff interface
2. Go to Koha administration > Basic parameters > Libraries
3. Edit any library or create a new library. Do not set hours.
4. Save the library.
	*Note that on the library list, in the hours column, all days are displayed
	with 'null'.
5. Edit or create a 2nd library.
6. Try to manually enter a time (not using the flatpickr modal)
	*Note that the mask wants a date format, not a time format.
7. Set some hours for the library, leaving at least one
	day without hours. Save.
	*Note that in the library list, in the hours column, defined times are in
	the format HH:MM:SS.
7. Edit the system preference CalendarFirstDayOfWeek to set a day other
	than Sunday as the first day of the week.
8. Return to Koha administration > Basic parameters > Libraries
9. Click on the name of a specific library with hours to view details.
	*Note that the order of the calendar weeek still starts with Sunday.
10. The order of the calendar week still starts with Sunday.
11. Edit the system preference TimeFormat and set to 12-hours.
12. Return to Koha administration > Basic parameters > Libraries.
	*Note that the time format on the branches list is not in 12-hour format.
13. Click on the name of a specific library with hours to view details.
	*Note that the time format in the branch details screen is not in 12-hour
	format.

14. Apply Patch.

15. Return to Koha administration > Basic parameters > Libraries
16. Verify that your library with all undefined times shows "Library hours
	not set".
17. Verify that your library with defined hours is showing times in	HH:MM
	format, and days without defined hours are blank.
18. Verify that your library with defined hours is respecting the TimeFormat
	preferences.
19. Click on the name of a library with hours to view details.
20. Verify that the hours respect the CalendarFirstDayOfWeek and TimeFormat
	preferences.
21. Click on the name of another library without hours to view details.
	Verify these libraries show as "No opening hours have been set"
	in the branch detail view.
22. Edit a library's hours. When inputing, try to manually enter a time (not
	using the flatpickr modal). Verify that you can enter time in the proper
	(12 or 24 hours) format.

Sponsored-by: Westlake Porter Public Library
Comment 22 Olivier Vezina 2024-09-09 12:56:06 UTC
Everything works as intended except in the libraries table where the dates comes out as "invalid Date" while in the specific view of the library the date appear in the right format and time.
Comment 23 CJ Lynce 2024-09-09 14:16:42 UTC
Hi Oliver, thanks for testing! I'm Having trouble tracking that one down. Could you share the following for your test case:
A) Your open and close times inputs for your library.
B) Your TimeFormat system pref setting (12 or 24 hours)
C) Your region's date format (dd/mm/yyyy, yyyy-dd-mm, etc.)
D) If possible, create a SQL report 'SELECT* FROM library_hours' and attach the results (or feel free to email it to me)
Comment 24 Olivier Vezina 2024-09-09 15:46:55 UTC
Created attachment 171213 [details]
Sql results libraries_hours
Comment 25 Olivier Vezina 2024-09-09 15:48:54 UTC
(In reply to Olivier V from comment #24)
> Created attachment 171213 [details]
> Sql results libraries_hours

The problem seems to be coming from the firefox browser since it'S a bit picky on date format. I tried the fix once more but with the chrome browser this time and everything worked fine. 

For firefox i tested with every dateformat and timeformat and i still got the same result.
Comment 26 CJ Lynce 2024-09-09 15:51:51 UTC
Thanks Oliver, that's helpful and makes sense (the library list was originally done in JavaScript, while the individual view uses template toolkit). Let me work on correction.
Comment 27 CJ Lynce 2024-09-09 17:05:44 UTC
Created attachment 171220 [details] [review]
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences.

-Newly created or edited libraries no longer display 'null' for undefined
	open and close times.
-Libraries without any defined hours will state such instead of displaying the
	hours table.
-The CalendarFirstDayOfWeek system preferences is now respected when viewing
	a library with defined hours.
-Time displays and inputs now follow the TimeFormat system preference.
-Times are no longer displayed with seconds.

As part of this patch, a new Template Filter, KohaTimes, has been added to
	allow for proper formatting of time strings based on systems preferences.

To Test:
1. Login to staff interface
2. Go to Koha administration > Basic parameters > Libraries
3. Edit any library or create a new library. Do not set hours.
4. Save the library.
	*Note that on the library list, in the hours column, all days are displayed
	with 'null'.
5. Edit or create a 2nd library.
6. Try to manually enter a time (not using the flatpickr modal)
	*Note that the mask wants a date format, not a time format.
7. Set some hours for the library, leaving at least one
	day without hours. Save.
	*Note that in the library list, in the hours column, defined times are in
	the format HH:MM:SS.
7. Edit the system preference CalendarFirstDayOfWeek to set a day other
	than Sunday as the first day of the week.
8. Return to Koha administration > Basic parameters > Libraries
9. Click on the name of a specific library with hours to view details.
	*Note that the order of the calendar weeek still starts with Sunday.
10. The order of the calendar week still starts with Sunday.
11. Edit the system preference TimeFormat and set to 12-hours.
12. Return to Koha administration > Basic parameters > Libraries.
	*Note that the time format on the branches list is not in 12-hour format.
13. Click on the name of a specific library with hours to view details.
	*Note that the time format in the branch details screen is not in 12-hour
	format.

14. Apply Patch.

15. Return to Koha administration > Basic parameters > Libraries
16. Verify that your library with all undefined times shows "Library hours
	not set".
17. Verify that your library with defined hours is showing times in	HH:MM
	format, and days without defined hours are blank.
18. Verify that your library with defined hours is respecting the TimeFormat
	preferences.
19. Click on the name of a library with hours to view details.
20. Verify that the hours respect the CalendarFirstDayOfWeek and TimeFormat
	preferences.
21. Click on the name of another library without hours to view details.
	Verify these libraries show as "No opening hours have been set"
	in the branch detail view.
22. Edit a library's hours. When inputing, try to manually enter a time (not
	using the flatpickr modal). Verify that you can enter time in the proper
	(12 or 24 hours) format.

Sponsored-by: Westlake Porter Public Library
Comment 28 CJ Lynce 2024-09-09 17:06:44 UTC
Please try this with Firefox- and Chrome-based browsers. Both should work properly.
Comment 29 Olivier Vezina 2024-09-09 18:33:43 UTC
Created attachment 171229 [details] [review]
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences.

-Newly created or edited libraries no longer display 'null' for undefined
	open and close times.
-Libraries without any defined hours will state such instead of displaying the
	hours table.
-The CalendarFirstDayOfWeek system preferences is now respected when viewing
	a library with defined hours.
-Time displays and inputs now follow the TimeFormat system preference.
-Times are no longer displayed with seconds.

As part of this patch, a new Template Filter, KohaTimes, has been added to
	allow for proper formatting of time strings based on systems preferences.

To Test:
1. Login to staff interface
2. Go to Koha administration > Basic parameters > Libraries
3. Edit any library or create a new library. Do not set hours.
4. Save the library.
	*Note that on the library list, in the hours column, all days are displayed
	with 'null'.
5. Edit or create a 2nd library.
6. Try to manually enter a time (not using the flatpickr modal)
	*Note that the mask wants a date format, not a time format.
7. Set some hours for the library, leaving at least one
	day without hours. Save.
	*Note that in the library list, in the hours column, defined times are in
	the format HH:MM:SS.
7. Edit the system preference CalendarFirstDayOfWeek to set a day other
	than Sunday as the first day of the week.
8. Return to Koha administration > Basic parameters > Libraries
9. Click on the name of a specific library with hours to view details.
	*Note that the order of the calendar weeek still starts with Sunday.
10. The order of the calendar week still starts with Sunday.
11. Edit the system preference TimeFormat and set to 12-hours.
12. Return to Koha administration > Basic parameters > Libraries.
	*Note that the time format on the branches list is not in 12-hour format.
13. Click on the name of a specific library with hours to view details.
	*Note that the time format in the branch details screen is not in 12-hour
	format.

14. Apply Patch.

15. Return to Koha administration > Basic parameters > Libraries
16. Verify that your library with all undefined times shows "Library hours
	not set".
17. Verify that your library with defined hours is showing times in	HH:MM
	format, and days without defined hours are blank.
18. Verify that your library with defined hours is respecting the TimeFormat
	preferences.
19. Click on the name of a library with hours to view details.
20. Verify that the hours respect the CalendarFirstDayOfWeek and TimeFormat
	preferences.
21. Click on the name of another library without hours to view details.
	Verify these libraries show as "No opening hours have been set"
	in the branch detail view.
22. Edit a library's hours. When inputing, try to manually enter a time (not
	using the flatpickr modal). Verify that you can enter time in the proper
	(12 or 24 hours) format.

Sponsored-by: Westlake Porter Public Library
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Comment 30 Martin Renvoize (ashimema) 2024-09-24 19:44:38 UTC
Created attachment 171952 [details] [review]
Bug 36594: Library hours in koha/admin/branches.pl now adhere to selected time and calendar system preferences.

-Newly created or edited libraries no longer display 'null' for undefined
	open and close times.
-Libraries without any defined hours will state such instead of displaying the
	hours table.
-The CalendarFirstDayOfWeek system preferences is now respected when viewing
	a library with defined hours.
-Time displays and inputs now follow the TimeFormat system preference.
-Times are no longer displayed with seconds.

As part of this patch, a new Template Filter, KohaTimes, has been added to
	allow for proper formatting of time strings based on systems preferences.

To Test:
1. Login to staff interface
2. Go to Koha administration > Basic parameters > Libraries
3. Edit any library or create a new library. Do not set hours.
4. Save the library.
	*Note that on the library list, in the hours column, all days are displayed
	with 'null'.
5. Edit or create a 2nd library.
6. Try to manually enter a time (not using the flatpickr modal)
	*Note that the mask wants a date format, not a time format.
7. Set some hours for the library, leaving at least one
	day without hours. Save.
	*Note that in the library list, in the hours column, defined times are in
	the format HH:MM:SS.
7. Edit the system preference CalendarFirstDayOfWeek to set a day other
	than Sunday as the first day of the week.
8. Return to Koha administration > Basic parameters > Libraries
9. Click on the name of a specific library with hours to view details.
	*Note that the order of the calendar weeek still starts with Sunday.
10. The order of the calendar week still starts with Sunday.
11. Edit the system preference TimeFormat and set to 12-hours.
12. Return to Koha administration > Basic parameters > Libraries.
	*Note that the time format on the branches list is not in 12-hour format.
13. Click on the name of a specific library with hours to view details.
	*Note that the time format in the branch details screen is not in 12-hour
	format.

14. Apply Patch.

15. Return to Koha administration > Basic parameters > Libraries
16. Verify that your library with all undefined times shows "Library hours
	not set".
17. Verify that your library with defined hours is showing times in	HH:MM
	format, and days without defined hours are blank.
18. Verify that your library with defined hours is respecting the TimeFormat
	preferences.
19. Click on the name of a library with hours to view details.
20. Verify that the hours respect the CalendarFirstDayOfWeek and TimeFormat
	preferences.
21. Click on the name of another library without hours to view details.
	Verify these libraries show as "No opening hours have been set"
	in the branch detail view.
22. Edit a library's hours. When inputing, try to manually enter a time (not
	using the flatpickr modal). Verify that you can enter time in the proper
	(12 or 24 hours) format.

Sponsored-by: Westlake Porter Public Library
Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize (ashimema) 2024-09-24 19:46:29 UTC
Thanks for all the work here and for breaking up the requirements into management steps.

This is great, no regressions, so really great improvement.

Passing QA

Side note: I'm going to add merge request for the QA tools to add the new TT filter into the expected filters list.
Comment 32 Martin Renvoize (ashimema) 2024-09-24 19:49:21 UTC
Interesting.. I'm not seeing any catch for KohaDates.. so perhsp my assumption of a need to add KohaTimes into the QA scripts was wrong.. we can figure that out in the QA team.

Anyway, great work here.
Comment 33 Jonathan Druart 2024-09-25 08:47:31 UTC
(In reply to Martin Renvoize from comment #32)
> Interesting.. I'm not seeing any catch for KohaDates.. so perhsp my
> assumption of a need to add KohaTimes into the QA scripts was wrong.. we can
> figure that out in the QA team.

See t/lib/QA/TemplateFilters.pm
146         or $tt_block =~ m{\s?\|\s?\$KohaDates[^\|]*$}
Comment 34 Jonathan Druart 2024-09-25 08:50:31 UTC
Wouldn't it be better to use KohaDates and add a time-only flag (same as flatpickr)?
I haven't had a full look at the patches however.
Can do later if needed.
Comment 35 CJ Lynce 2024-09-25 15:59:43 UTC
I had thought about incorporating a flag for time-only in KohaDates, but opted for adding the suggested KohaTimes filter since KohaDate expects, at minimum, a date component starting with ####-# to work, including when using the with_hours option.

Doing a time_only option for KohaDates would have essentially branched the filter's code due to the Library Hours are only stored as HH:MM:SS, without a date component. Thus, KohaTimes keeps the differing code bases separate, each with their different expected minimum input. That said, KohaTimes is fairly simplistic, so folding it into a KohaDates option would be trivial.

Similar rationale for the JS equivalent of KohaDates in koha-tmpl/intranet-tmpl/prog/en/includes/js-date-format.inc - the existing related functions all required a date component to be passed, even when handling time. The added equivalent of KohaTimes handles inputs without a date component.
Comment 36 Martin Renvoize (ashimema) 2024-09-30 14:34:04 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 37 Tomás Cohen Arazi (tcohen) 2024-09-30 17:01:16 UTC
Hi all, this broke some tests. It looks like a false positive. Please take a look:

```
$ prove xt/find-missing-filters.t
xt/find-missing-filters.t .. 1/1
#   Failed test 'Template variables should be correctly escaped'
#   at xt/find-missing-filters.t line 42.
#          got: '1'
#     expected: '0'
# $VAR1 = {
#           'file' => 'koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt',
#           'errors' => [
#                         {
#                           'error' => 'missing_filter',
#                           'line_number' => 588,
#                           'line' => '                                            <span>[% IF hr.open_time != null %][% hr.open_time | $KohaTimes %][% END %]</span>'
#                         },
#                         {
#                           'error' => 'missing_filter',
#                           'line_number' => 591,
#                           'line' => '                                            <span>[% IF hr.close_time != null %][% hr.close_time | $KohaTimes %][% END %]</span>'
#                         }
#                       ]
#         };
# Looks like you failed 1 test of 1.
```
Comment 38 Martin Renvoize (ashimema) 2024-09-30 17:22:24 UTC
Created attachment 172233 [details] [review]
Bug 36594: (RM follow-up) Add new filter to filters test

This is the patch I meant to write before pushing but brain fade failed
me.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize (ashimema) 2024-09-30 17:23:14 UTC
Follow-up pushed, my mistake.

And I did also ask myself about the KohaTimes vs KohaDates with_time option and came to the conclusion that CJ has made clear :).
Comment 40 Jonathan Druart 2024-10-03 11:52:02 UTC
I am seeing a lot of errors in the console:
  TypeError: items is undefined

On the library edit form (admin/branches.pl?op=add_form&branchcode=CPL)
Comment 41 Jonathan Druart 2024-10-03 11:53:03 UTC
(In reply to Jonathan Druart from comment #40)
> I am seeing a lot of errors in the console:
>   TypeError: items is undefined
> 
> On the library edit form (admin/branches.pl?op=add_form&branchcode=CPL)

Maybe not related to this change?
Comment 42 CJ Lynce 2024-10-03 18:31:07 UTC
Jonathan: Maybe its related? I am seeing a 'TypeError: undefined is not iterable' (but no reference to 'items is undefiend')

I submitted a patch for the error I'm seeing on bug 38081, does this resolve the error you are getting?
Comment 43 Jonathan Druart 2024-10-07 09:41:51 UTC
(In reply to CJ Lynce from comment #42)
> Jonathan: Maybe its related? I am seeing a 'TypeError: undefined is not
> iterable' (but no reference to 'items is undefiend')
> 
> I submitted a patch for the error I'm seeing on bug 38081, does this resolve
> the error you are getting?

Yes, confirmed, it fixes the error I got.