Bug 11148 - Two routines are useless in Koha::DateUtils
Summary: Two routines are useless in Koha::DateUtils
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 11468
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-28 14:09 UTC by Jonathan Druart
Modified: 2015-06-04 23:30 UTC (History)
6 users (show)

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


Attachments
Bug 11148: Two routines are useless in Koha::DateUtils (5.60 KB, patch)
2013-10-28 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11148: Two routines are useless in Koha::DateUtils (5.78 KB, patch)
2013-11-07 09:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11148: Two routines are useless in Koha::DateUtils (5.85 KB, patch)
2013-11-22 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11148: Followup only display the hours unless "23:59" (3.85 KB, patch)
2013-12-02 16:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11148: Add a as_due_date parameter to the output_pref routine (5.85 KB, patch)
2013-12-03 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11148: Previous patch delete usefull unit test (1.28 KB, patch)
2014-01-13 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11148: Two routines are useless in Koha::DateUtils (5.84 KB, patch)
2014-03-10 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11148: Add a as_due_date parameter to the output_pref routine (5.11 KB, patch)
2014-03-10 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11148: Previous patch delete usefull unit test (1.28 KB, patch)
2014-03-10 08:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11148: Two routines are useless in Koha::DateUtils (5.84 KB, patch)
2014-03-12 17:55 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 11148: Add a as_due_date parameter to the output_pref routine (5.10 KB, patch)
2014-03-12 17:55 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 11148: Previous patch delete usefull unit test (1.28 KB, patch)
2014-03-12 17:56 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 11148: Two routines are useless in Koha::DateUtils (5.94 KB, patch)
2014-03-13 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11148: Add a as_due_date parameter to the output_pref routine (5.21 KB, patch)
2014-03-13 13:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11148: Previous patch delete usefull unit test (1.38 KB, patch)
2014-03-13 13:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-10-28 14:09:44 UTC
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.
Comment 1 Jonathan Druart 2013-10-28 14:13:46 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-11-07 09:01:47 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-11-22 16:43:36 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2013-11-29 15:15:47 UTC
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.
Comment 5 Katrin Fischer 2013-11-29 15:39:56 UTC
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.
Comment 6 Jonathan Druart 2013-12-02 16:06:29 UTC
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.
Comment 7 Jonathan Druart 2013-12-02 16:22:43 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-12-02 16:23:03 UTC
Something like that?
Comment 9 Galen Charlton 2013-12-02 17:08:07 UTC
(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).
Comment 10 Jonathan Druart 2013-12-03 10:19:46 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2014-01-12 04:42:25 UTC
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.
Comment 12 Jonathan Druart 2014-01-13 09:17:41 UTC
(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.
Comment 13 Jonathan Druart 2014-01-13 09:18:26 UTC Comment hidden (obsolete)
Comment 14 Mark Tompsett 2014-02-25 00:13:23 UTC
I vaguely recall someone mentioning given/when being experimental. Is this the case in this patch?
Comment 15 Marcel de Rooy 2014-02-25 07:25:52 UTC
(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
Comment 16 Katrin Fischer 2014-02-25 07:29:07 UTC
While I agree that they should be removed, it looks like the construct was not added by those patches. Possibly only whitespace fixes?
Comment 17 Jonathan Druart 2014-02-25 08:27:58 UTC
(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.
Comment 18 Jonathan Druart 2014-03-10 08:51:52 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-03-10 08:51:57 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-03-10 08:52:04 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-03-10 08:52:59 UTC
Last patches fix conflicts with bug 11468.
Comment 22 Nicolas Legrand 2014-03-12 17:55:40 UTC Comment hidden (obsolete)
Comment 23 Nicolas Legrand 2014-03-12 17:55:58 UTC Comment hidden (obsolete)
Comment 24 Nicolas Legrand 2014-03-12 17:56:12 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2014-03-13 13:55:54 UTC
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
Comment 26 Marcel de Rooy 2014-03-13 13:56:55 UTC
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>
Comment 27 Marcel de Rooy 2014-03-13 13:57:03 UTC
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>
Comment 28 Marcel de Rooy 2014-03-13 13:57:09 UTC
Created attachment 26280 [details] [review]
Bug 11148: Previous patch delete usefull unit test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2014-03-13 13:58:11 UTC
Nicolas: You signed off, but forgot to add a Signed-off-by line. Please add it next time. Thanks.
Comment 30 Galen Charlton 2014-04-18 21:54:25 UTC
Pushed to master, along with a follow-up that adds a couple more test cases.  Thanks, Jonathan!
Comment 31 Fridolin Somers 2014-08-06 10:50:16 UTC
Pushed to 3.14.x, will be in 3.14.10