Bug 7444

Summary: Use T::T date plugin to display dates omnibus
Product: Koha Reporter: Paul Poulain <paul.poulain>
Component: Architecture, internals, and plumbingAssignee: Ian Walls <koha.sekjal>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart+koha>
Severity: enhancement    
Priority: P5 - low CC: chris, jcamins, kyle.m.hall, martin.renvoize, nengard
Version: 3.10   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8043
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7735, 7737, 7743, 7748, 17821    
Bug Blocks:    
Attachments: Bug 7444: Update circ/circulation.pl to use KohaDates TT Plugin
Bug 7444: Update circ/circulation.pl to use KohaDates TT Plugin
Bug 7444: [REBASED] Update circ/circulation.pl to use KohaDates TT Plugin
10108: Bug 7444: [SIGNED] Update circ/circulation.pl to use KohaDates TT Plugin

Description Paul Poulain 2012-01-13 16:03:34 UTC
All dates can now be displayed through a Template::Toolkit plugin. All existing code must be updated.
Dates can/should/must now be displayed by:
* adding [% USE KohaDates %] at the beginning of the template
* use [% mydate | KohaDates %] for mydate date variable
* remove .pl code that update date format before sending the string to the template.

This entry is an omnibus, as many patches will be necessary.

Note: the plugin has been introduced in Bug 929
Comment 1 Chris Cormack 2012-01-13 18:33:23 UTC
Just a little change it needs to be
[% mydate | $KohaDates %]
Comment 2 Ian Walls 2012-03-09 19:04:06 UTC Comment hidden (obsolete)
Comment 3 Jared Camins-Esakov 2012-03-11 00:32:54 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2012-03-14 15:20:13 UTC
QA comment:
The patch says:
@@ -909,6 +910,7 @@ sub CanBookBeIssued {
 $needsconfirmation{'resborrowernumber'} = $resborrower->{'borrowernumber'};
 $needsconfirmation{'resbranchname'} = $branchname;
 $needsconfirmation{'resreservedate'} = format_date($res->{'reservedate'});
+$needsconfirmation{'resreservedateiso'} = $res->{'reservedate'};

Why don't you remove the resreservedate and his "format_date" sub ? shouldn't it be 
-$needsconfirmation{'resreservedate'} = format_date($res->{'reservedate'});
+$needsconfirmation{'resreservedateiso'} = $res->{'reservedate'};
Comment 5 Jared Camins-Esakov 2012-03-14 15:24:31 UTC
Until KohaDates has replaced pre-formatted dates everywhere in Koha, we need to pass the unformatted date for use with the filter, and the formatted date, for use where it hasn't changed yet.
Comment 6 Jonathan Druart 2012-03-21 14:17:06 UTC
QA comment: same comment as Paul, for me, if it's not currently used, it's useless to keep this line.

Passed QA
Comment 7 Paul Poulain 2012-03-22 09:00:24 UTC
After investigating & thinking of it again, I reject this patch.

Here is my explanation: The patch add a reservedateiso hash entry, and keep the reservedate. It means we add some code that may be useless, or will become. Instead of improving code quality, it lower it.
So, I request
- $needsconfirmation{'resreservedate'} = format_date($res->{'reservedate'});
+ $needsconfirmation{'resreservedate'} = $res->{'reservedate'};

(ie = don't introduce a new variable reservedateiso, and a checking that there is no side effect)

Note that I think there should not be a lot of side effect, as this sub is used in circulation only, so the updated patch would probably be safe. (But it has to be checked, it's just my feeling)
Comment 8 Jared Camins-Esakov 2012-03-22 11:19:58 UTC
Switching bug back to original, un-signed off patch that does what Paul requests, and returning to "Needs signoff" status. I have expressed my concern about the broken display caused by redefining $needsconfirmation{'resreservedate'}, but as the consensus is that the variable should be redefined anyway, a new signoff is required.
Comment 9 Martin Renvoize 2012-06-10 13:44:41 UTC
Needs re-basing against master... taking a look now.
Comment 10 Martin Renvoize 2012-06-10 13:51:45 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2012-06-10 14:10:13 UTC
Created attachment 10114 [details] [review]
10108: Bug 7444: [SIGNED] Update circ/circulation.pl to use KohaDates TT Plugin

Tested as outline in Ian's comment. All working as expected, taking note of the syspref.
Comment 12 Martin Renvoize 2012-06-10 14:13:57 UTC
Related Note: Under "Details" tab for patron record, the checkouts summary at the bottom of the page displays as a Date Time (i.e is followed by 00:00)  This is not consistent with how the table is displayed elsewhere..
Comment 13 Paul Poulain 2012-06-11 16:04:05 UTC
QA comment: cleaning code patch only, no more reason to object !
Comment 14 Kyle M Hall 2012-06-14 12:06:23 UTC
This patch has stripped the hour/minute portion of the date due out on circulation.pl
Comment 15 Nicole C. Engard 2012-06-14 12:10:38 UTC
This patch appears to have removed the time from the list of checkouts - the time is necessary for hourly loans ....
Comment 16 Nicole C. Engard 2012-06-14 12:12:32 UTC
Created attachment 10279 [details]

This is the timestamp that was there in 3.8.1 and needs to be put back ...
Comment 17 Chris Cormack 2012-06-17 08:16:00 UTC
Reverted from 3.8.x, so needs to be fixed before the 3.10.0 release
Comment 18 Paul Poulain 2012-06-29 15:19:10 UTC
(In reply to comment #17)
> Reverted from 3.8.x, so needs to be fixed before the 3.10.0 release

Patch also reverted from master. Switching status to failed QA.