Bug 9108 - Add uk/euro date sort filter to pages which require it
Summary: Add uk/euro date sort filter to pages which require it
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: 3.12
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 9349 9508
  Show dependency treegraph
 
Reported: 2012-11-19 20:05 UTC by Owen Leonard
Modified: 2014-12-07 20:02 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Large patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 9108 - Add uk/euro date sort filter to pages which require it (8.61 KB, patch)
2012-11-19 20:09 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 9108: Followup: send the dateformat value from C4::Auth (28.86 KB, patch)
2012-11-20 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9108 - Add uk/euro date sort filter to pages which require it (8.75 KB, patch)
2012-11-28 07:23 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9108: Followup: send the dateformat value from C4::Auth (28.91 KB, patch)
2012-11-28 07:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9108: Followup: send the dateformat value from C4::Auth (47.82 KB, patch)
2012-12-18 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9108 - Add uk/euro date sort filter to pages which require it (8.76 KB, patch)
2013-01-14 22:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9108: Followup: send the dateformat value from C4::Auth (30.25 KB, patch)
2013-01-14 22:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[3.10.X] Bug 9108 - Add uk/euro date sort filter to pages which require it (8.88 KB, patch)
2013-11-25 15:44 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[3.10.X] Bug 9108: Followup: send the dateformat value from C4::Auth (30.03 KB, patch)
2013-11-25 15:47 UTC, Fridolin SOMERS
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2012-11-19 20:05:46 UTC
In order for dates in DD/MM/YYYY format to be correctly sorted by the DataTables plugin, a filter must be added in the <script> block. This filter already exists in the DataTables JS but must be explicitly imported.
Comment 1 Owen Leonard 2012-11-19 20:09:38 UTC Comment hidden (obsolete)
Comment 2 Melia Meggs 2012-11-20 01:06:34 UTC
Which affected pages should I be looking at to test this patch?

After applying the patch, with dateformat sys pref set to DD/MM/YYYY, dates seem to be correctly sorted on the patron details tab (http://staff-melia.test.bywatersolutions.com/cgi-bin/koha/members/moremember.pl?borrowernumber=5)

But not on the patron circulation history tab (http://staff-melia.test.bywatersolutions.com/cgi-bin/koha/members/readingrec.pl?borrowernumber=5).  In the date column on the left, it's giving me:
04/10/2012 - Oct 4
05/04/2012 - April 5
07/06/2012 - June 7
12/03/2012 - March 12
Other columns on this screen (checked out on, date due, return date) look wrong as well.

But didn't I test another patch that fixed the patron circulation history tab already?
Comment 3 Jonathan Druart 2012-11-20 10:54:35 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-11-28 07:23:03 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2012-11-28 07:24:14 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2012-12-11 19:16:57 UTC
QA comment:
 * I think the 1st patch is OK (untested)
 * I think the 2nd patch requires more work to remove now useless code.

I explain:
some of the following lines are removed:
-        "dateformat_" . C4::Context->preference('dateformat') => 1,
and it's OK.

However, in C4/Auth.pm, there are still references to such a construction, that was needed with HTML::Template, but can be removed with T::T

The 2nd patch add in all templates the dateformat variable:
in C4/Auth.pm =
+        $template->param( dateformat => C4::Context->preference('dateformat') );

Thus, the lines just after this new one in Auth.pm can be removed:
    if(C4::Context->preference('dateformat')){
        if(C4::Context->preference('dateformat') eq "metric"){
            $template->param(dateformat_metric => 1);
        } elsif(C4::Context->preference('dateformat') eq "us"){
            $template->param(dateformat_us => 1);
        } else {
            $template->param(dateformat_iso => 1);
        }
    } else {
        $template->param(dateformat_iso => 1);
    }

and all references to dateformat_metric, dateformat_us and dateformat_iso should be replaced :
[% IF (dateformat_us ) %]
to replace with:
[% IF (dateformat eq 'us') %]

There is something like a dozen of templates that uses this construction.
Comment 7 Jonathan Druart 2012-12-12 08:15:33 UTC
(In reply to comment #6)
> QA comment:
[...]
> Thus, the lines just after this new one in Auth.pm can be removed:
>     if(C4::Context->preference('dateformat')){
>         if(C4::Context->preference('dateformat') eq "metric"){
>             $template->param(dateformat_metric => 1);
>         } elsif(C4::Context->preference('dateformat') eq "us"){
>             $template->param(dateformat_us => 1);
>         } else {
>             $template->param(dateformat_iso => 1);
>         }
>     } else {
>         $template->param(dateformat_iso => 1);
>     }
> 
> and all references to dateformat_metric, dateformat_us and dateformat_iso
> should be replaced :
> [% IF (dateformat_us ) %]
> to replace with:
> [% IF (dateformat eq 'us') %]
> 
> There is something like a dozen of templates that uses this construction.

Yes but I don't want to introduce any regression and I think my patch is more flexible than your proposition.
Like this anyone is allowed to use the format he wants.
In the same way, the patches still submitted in BZ but not pushed will not be broken (if using one of the two formats).
Comment 8 Paul Poulain 2012-12-17 16:47:25 UTC
(In reply to comment #7)

> Yes but I don't want to introduce any regression and I think my patch is
> more flexible than your proposition.
avoid introducing regression++, but my proposal would have resulted in replacing all code that can be replaced to be. And it will clean the code.

I don't see why your patch is more flexible = my proposal is to go further with your patch, not changing it !

(if another QAer disagree, feel free to jump in)

> Like this anyone is allowed to use the format he wants.
the syspref has only 3 values, and this should not change.

> In the same way, the patches still submitted in BZ but not pushed will not
> be broken (if using one of the two formats).
is there a case ?

I'm still thinking my proposal should be implemented, to avoid piling code (I know there is a lot in Koha already, not a reason to continue...)
Comment 9 Jonathan Druart 2012-12-18 14:36:28 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2012-12-29 09:46:30 UTC
Hmm this follow up is now pretty massive to test. I actually would have preferred the first follow up. And this new code clean up to be on it's own bug.

Perhaps in a series of smaller patches, as it stands to test you have to test about 30 pages to sign off.
Comment 11 Jonathan Druart 2013-01-07 15:08:45 UTC
(In reply to comment #10)
> Hmm this follow up is now pretty massive to test. I actually would have
> preferred the first follow up. And this new code clean up to be on it's own
> bug.
> 
> Perhaps in a series of smaller patches, as it stands to test you have to
> test about 30 pages to sign off.

Hi Chris,
After a discussion with Paul, I mark the last patch as obsolete in order to keep my first one.
Comment 12 Jonathan Druart 2013-01-07 15:09:50 UTC
All proposed patches are signed off
Comment 13 Katrin Fischer 2013-01-14 22:03:30 UTC
Created attachment 14572 [details] [review]
[SIGNED-OFF] Bug 9108 - Add uk/euro date sort filter to pages which require it

This patch adds the JS required to enable correct sorting
of dates in DD/MM/YYYY format to pages which require it.

To test, set your dateformat accordingly and confirm on the
affected pages that dates are sorted correctly.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Resolved conflict in serials/serials-search.tt.
Comment 14 Katrin Fischer 2013-01-14 22:04:19 UTC
Created attachment 14573 [details] [review]
[SIGNED-OFF] Bug 9108: Followup: send the dateformat value from C4::Auth

- the dateformat value is send to all templates (from
  C4::Auth::get_template_and_user)
- remove all assignment of dateformat in all .pl files
- the DHTMLcalendar_dateformat variable is unused

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Fixed conflicts:
 - opac/sco/sco-main.pl
 - reports/acquisitions_stats.pl
 - tools/cleanborrowers.pl

All tests pass, perlcritic problems appeared in some files
before and after these patches were applied.

Checked sorting in following pages:
- acqui/addorderiso2709.tt - list of staged imports in acq
- acqui/histsearch.tt - sorting of dates in acq search result list
- acqui/invoices.tt - billing date in list of invoices in acq
- acqui/lateorders.tt - list of late orders in acq
- acqui/ordered.tt - ordered titles and estimated costs for a fund
- acqui/parcels.tt - receive shipment page
- acqui/spent.tt - received titles and actual costs for a fund
...
- serials-search.tt - subscription search result list
...
- opac/sco/sco-main.tt - due dates in list of checked out items
- reports/acquisitions-stats.tt - date searches, display of dates
- tools/cleanborrowers.tt
- tools.holidays.tt - different views of dates library is closed,
  adding dates

Checked dates display according to system preference everywhere and
searching, entering dates etc. still worked as expected.
Comment 15 Jared Camins-Esakov 2013-01-18 02:59:37 UTC
This patch has been pushed to master.
Comment 16 Chris Cormack 2013-02-01 08:06:45 UTC
Patches do not apply cleanly to 3.10.x or 3.8.x please rebase for those branches if you want it in them.
Comment 17 Fridolin SOMERS 2013-11-25 15:44:18 UTC
Created attachment 23139 [details] [review]
[3.10.X] Bug 9108 - Add uk/euro date sort filter to pages which require it

First patch for old stable 3.10.X branch
Comment 18 Fridolin SOMERS 2013-11-25 15:47:16 UTC
Created attachment 23141 [details] [review]
[3.10.X] Bug 9108: Followup: send the dateformat value from C4::Auth

Second patch for old stable 3.10.X branch
Comment 19 Fridolin SOMERS 2013-11-25 15:49:02 UTC
3.10.X patches need signoff
Comment 20 David Cook 2014-01-20 01:56:33 UTC
I've started trying to test the 3.10.x patches, but it's quite the slog trying to find the pages affected, and then not necessarily having the data to test them and having to create that data.

This one is just too laborious for not enough reward, as it's already in master. 

Sorry :/.
Comment 21 Fridolin SOMERS 2014-01-20 08:39:24 UTC
(In reply to David Cook from comment #20)
No problem, thanks for trying
Comment 22 David Cook 2014-04-24 03:15:48 UTC
I'm closing this one, since it was pushed to master ages ago, and is now out of date in light of some of Owen's latest DataTable patches.