There are 2 useless routines in the Koha::DateUtils module:output_pref_due and format_sqlduedatetime. We can call output_pref and format_datetime with dateonly = 0.
Created attachment 22470 [details] [review] Bug 11148: Two routines are useless in Koha::DateUtils There are 2 useless routines in the Koha::DateUtils module:output_pref_due and format_sqlduedatetime. We can call output_pref and format_datetime with dateonly = 0. format_sqlduedatetime is only used in one place: opac-reserve.pl Test plan: 1/ Verify on the opac-reserve.pl page that the date is correctly displayed for for onloan items (you should use the "specific copy" feature). 2/ Launch prove t/DateUtils.t UT file and verify all UT pass.
Created attachment 22759 [details] [review] Bug 11148: Two routines are useless in Koha::DateUtils There are 2 useless routines in the Koha::DateUtils module:output_pref_due and format_sqlduedatetime. We can call output_pref and format_datetime with dateonly = 0. format_sqlduedatetime is only used in one place: opac-reserve.pl Test plan: 1/ Verify on the opac-reserve.pl page that the date is correctly displayed for for onloan items (you should use the "specific copy" feature). 2/ Launch prove t/DateUtils.t UT file and verify all UT pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Due date on opac-reserve shown correctly. Unit tests pass. Did a grep on both function names.
Created attachment 23092 [details] [review] Bug 11148: Two routines are useless in Koha::DateUtils There are 2 useless routines in the Koha::DateUtils module:output_pref_due and format_sqlduedatetime. We can call output_pref and format_datetime with dateonly = 0. format_sqlduedatetime is only used in one place: opac-reserve.pl Test plan: 1/ Verify on the opac-reserve.pl page that the date is correctly displayed for for onloan items (you should use the "specific copy" feature). 2/ Launch prove t/DateUtils.t UT file and verify all UT pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Due date on opac-reserve shown correctly. Unit tests pass. Did a grep on both function names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> No references to subs found. Passes koha-qa.pl, t and xt
Sure about this? dateonly => 1 is not exactly equivalent to what format_sqlduedatetime and output_pref_due output. This is most evident for hourly loans: Due date/time Current display in opac-reserves New display ----------------- -------------------------------- ----------- 2013-12-01 23:59 2013-12-01 2013-12-01 2013-12-01 13:25 2013-12-01 13:25 2013-12-01 IOW, this patch would cause a (minor) net loss of displayed information for patrons if hourly loans are being used. Changing status to have a discussion of the desired display of due times for hourly loans. I will note that I *do* think that output_pref_due and format_sqlduedatetime can be removed, but it may be advisable to add a display_as_due_date flag or similarly to output_pref.
In response to Galen's email to koha-devel: I think it would be less confusing to not display the 23:59 but just the date. Especially in libraries not using hourly loans the new display caused some confusion.
Galen, I suspect you had a mistake on updating the status :) It is not pushed to master but in discussion. Maybe should we never display the hour/minutes if it's 23:59? So we remove the dateonly flag and we don't need a new routine.
Created attachment 23239 [details] [review] Bug 11148: Followup only display the hours unless "23:59"
Something like that?
(In reply to Jonathan Druart from comment #8) > Something like that? Not as a default behavior -- I'd prefer that the caller explicitly ask for (say) date_only or as_due_date, not have the behavior of stripping off "23:59" be hardcoded. (Also, stripping off "23:59" doesn't work if the timeformat is 12hr).
Created attachment 23253 [details] [review] Bug 11148: Add a as_due_date parameter to the output_pref routine This parameter is a boolean, if true, the hours won't be displayed if the time is 23:59 (24hr format) or 11:59 PM (12hr format).
Comment on attachment 23092 [details] [review] Bug 11148: Two routines are useless in Koha::DateUtils Review of attachment 23092 [details] [review]: ----------------------------------------------------------------- ::: t/DateUtils.t @@ -68,5 @@ > cmp_ok $date_string, 'eq', '16/06/2011', 'metric output (date only)'; > > -$date_string = output_pref({ dt => $dt, dateformat => 'metric', timeformat => '24hr' }); > -cmp_ok $date_string, 'eq', '16/06/2011 12:00', > - 'output_pref_due preserves non midnight HH:SS'; Why did you cut this test? The text is wrong, but the function call is output_pref, which is still used.
(In reply to M. Tompsett from comment #11) > Why did you cut this test? The text is wrong, but the function call is > output_pref, which is still used. I really don't know! Maybe because there was the 'output_pref_due' in the unit test description and I read too quickly! I will submit a new followup to reintroduce it.
Created attachment 24219 [details] [review] Bug 11148: Previous patch delete usefull unit test
I vaguely recall someone mentioning given/when being experimental. Is this the case in this patch?
(In reply to M. Tompsett from comment #14) > I vaguely recall someone mentioning given/when being experimental. Is this > the case in this patch? Yeah. Have a look at e.g. bug 11474. Please remove them. Failed QA
While I agree that they should be removed, it looks like the construct was not added by those patches. Possibly only whitespace fixes?
(In reply to Katrin Fischer from comment #16) > While I agree that they should be removed, it looks like the construct was > not added by those patches. Possibly only whitespace fixes? Yes, this patch does not introduce them.
Created attachment 25973 [details] [review] Bug 11148: Two routines are useless in Koha::DateUtils There are 2 useless routines in the Koha::DateUtils module:output_pref_due and format_sqlduedatetime. We can call output_pref and format_datetime with dateonly = 0. format_sqlduedatetime is only used in one place: opac-reserve.pl Test plan: 1/ Verify on the opac-reserve.pl page that the date is correctly displayed for for onloan items (you should use the "specific copy" feature). 2/ Launch prove t/DateUtils.t UT file and verify all UT pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Due date on opac-reserve shown correctly. Unit tests pass. Did a grep on both function names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> No references to subs found. Passes koha-qa.pl, t and xt
Created attachment 25974 [details] [review] Bug 11148: Add a as_due_date parameter to the output_pref routine This parameter is a boolean, if true, the hours won't be displayed if the time is 23:59 (24hr format) or 11:59 PM (12hr format).
Created attachment 25975 [details] [review] Bug 11148: Previous patch delete usefull unit test
Last patches fix conflicts with bug 11468.
Created attachment 26212 [details] [review] Bug 11148: Two routines are useless in Koha::DateUtils There are 2 useless routines in the Koha::DateUtils module:output_pref_due and format_sqlduedatetime. We can call output_pref and format_datetime with dateonly = 0. format_sqlduedatetime is only used in one place: opac-reserve.pl Test plan: 1/ Verify on the opac-reserve.pl page that the date is correctly displayed for for onloan items (you should use the "specific copy" feature). 2/ Launch prove t/DateUtils.t UT file and verify all UT pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Due date on opac-reserve shown correctly. Unit tests pass. Did a grep on both function names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> No references to subs found. Passes koha-qa.pl, t and xt
Created attachment 26213 [details] [review] Bug 11148: Add a as_due_date parameter to the output_pref routine This parameter is a boolean, if true, the hours won't be displayed if the time is 23:59 (24hr format) or 11:59 PM (12hr format).
Created attachment 26214 [details] [review] Bug 11148: Previous patch delete usefull unit test
QA Comment: These patches look good to me. Just noting one very minor issue: if hourly loans by incident should be due on exactly 23:59 or 11:59 PM, the hours are gone too.. Passed QA
Created attachment 26278 [details] [review] Bug 11148: Two routines are useless in Koha::DateUtils There are 2 useless routines in the Koha::DateUtils module:output_pref_due and format_sqlduedatetime. We can call output_pref and format_datetime with dateonly = 0. format_sqlduedatetime is only used in one place: opac-reserve.pl Test plan: 1/ Verify on the opac-reserve.pl page that the date is correctly displayed for for onloan items (you should use the "specific copy" feature). 2/ Launch prove t/DateUtils.t UT file and verify all UT pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Due date on opac-reserve shown correctly. Unit tests pass. Did a grep on both function names. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> No references to subs found. Passes koha-qa.pl, t and xt Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 26279 [details] [review] Bug 11148: Add a as_due_date parameter to the output_pref routine This parameter is a boolean, if true, the hours won't be displayed if the time is 23:59 (24hr format) or 11:59 PM (12hr format). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 26280 [details] [review] Bug 11148: Previous patch delete usefull unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nicolas: You signed off, but forgot to add a Signed-off-by line. Please add it next time. Thanks.
Pushed to master, along with a follow-up that adds a couple more test cases. Thanks, Jonathan!
Pushed to 3.14.x, will be in 3.14.10