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.
Created attachment 13535 [details] [review] 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.
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?
Created attachment 13546 [details] [review] 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
Created attachment 13723 [details] [review] 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>
Created attachment 13724 [details] [review] 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>
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.
(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).
(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...)
Created attachment 14189 [details] [review] 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 - Remove "all" occurrences (those I found!) of dateformat_* From now the only way to get the date format is a string comparaison (dateformat == "metric")
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.
(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.
All proposed patches are signed off
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.
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.
This patch has been pushed to master.
Patches do not apply cleanly to 3.10.x or 3.8.x please rebase for those branches if you want it in them.
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
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
3.10.X patches need signoff
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 :/.
(In reply to David Cook from comment #20) No problem, thanks for trying
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.