Bug 7444 - Use T::T date plugin to display dates omnibus
Summary: Use T::T date plugin to display dates omnibus
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Ian Walls
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 7735 7737 7743 7748 17821
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-13 16:03 UTC by Paul Poulain
Modified: 2016-12-28 14:35 UTC (History)
5 users (show)

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


Attachments
Bug 7444: Update circ/circulation.pl to use KohaDates TT Plugin (15.06 KB, patch)
2012-03-09 19:04 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7444: Update circ/circulation.pl to use KohaDates TT Plugin (16.87 KB, patch)
2012-03-11 00:32 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7444: [REBASED] Update circ/circulation.pl to use KohaDates TT Plugin (14.90 KB, patch)
2012-06-10 13:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
10108: Bug 7444: [SIGNED] Update circ/circulation.pl to use KohaDates TT Plugin (14.97 KB, patch)
2012-06-10 14:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
timestamp (83.56 KB, image/png)
2012-06-14 12:12 UTC, Nicole C. Engard
Details

Note You need to log in before you can comment on or make changes to this bug.
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]
timestamp

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.