Bug 14382 - datepicker's dateFormat setting for reports is missing
Summary: datepicker's dateFormat setting for reports is missing
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: M. Tompsett
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 14630 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-06-14 17:28 UTC by Claudio
Modified: 2016-06-21 21:38 UTC (History)
11 users (show)

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


Attachments
Step by step description (163.22 KB, application/pdf)
2015-06-14 18:05 UTC, Claudio
Details
guided_reports_start.tt (16.36 KB, image/png)
2015-06-15 00:56 UTC, Claudio
Details
Step by step reloaded (163.99 KB, application/pdf)
2015-06-15 22:07 UTC, Claudio
Details
Bug 14382 - datepicker's dateFormat setting for reports is missing (1.34 KB, patch)
2015-06-19 09:30 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14382 - datepicker's dateFormat setting for reports is missing (1.35 KB, patch)
2015-06-19 09:35 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14382 - datepicker's dateFormat setting for reports is missing (1.34 KB, patch)
2015-06-19 10:32 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14382: Non-ISO Date parameters generate empty reports. (2.42 KB, patch)
2015-06-24 16:34 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14382: Non-ISO Date parameters generate empty reports. (2.49 KB, patch)
2015-06-24 16:56 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 14382: Non-ISO Date parameters generate empty reports. (2.56 KB, patch)
2015-06-25 03:30 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14382: Non-ISO Date parameters generate empty reports. (2.48 KB, patch)
2015-06-26 17:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14382: Non-ISO Date parameters generate empty reports. (2.55 KB, patch)
2015-06-26 17:53 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14382: Non-ISO Date parameters generate empty reports. (2.67 KB, patch)
2015-07-03 09:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14382: [QA Follow-up] Bad regex alarm (1.47 KB, patch)
2015-07-03 09:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14382: [QA Follow-up] Bad regex alarm (1.52 KB, patch)
2015-07-03 15:41 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 Claudio 2015-06-14 17:28:59 UTC
datepicker's dateformat setting for reports is missing. Reports always run with ISO date format (yyyy-mm-dd) but this setting is missing in /usr/share/koha/intranet/htdocs/intranet-tmpl/prog/LANGUAGE/modules/reports/guided_reports_start.tt.

Because of this date format in datepicker plugin adopts the format defined in dateformat syspreferences variable and this is wrong.

To solve this is needed to include this code below line 169:

  // redefine the datepicker's default because reports require ISO
  // http://jqueryui.com/demos/datepicker/#option-dateFormat
  $( ".datepicker" ).datepicker( "option", "dateFormat", "yy-mm-dd" );


Tested in 3.18.7 release.

In 3.2 is not missing so it works fine.
Comment 1 Katrin Fischer 2015-06-14 17:30:41 UTC
Can you please include a step by step on how to make this bug visible in the GUI?
Comment 2 Claudio 2015-06-14 18:05:10 UTC Comment hidden (obsolete)
Comment 3 Claudio 2015-06-15 00:56:35 UTC
Created attachment 40130 [details]
guided_reports_start.tt

Coded in 3.18.5.1
Comment 4 Claudio 2015-06-15 22:07:53 UTC
Created attachment 40157 [details]
Step by step reloaded
Comment 5 Amit Gupta 2015-06-19 09:20:29 UTC
I have checked feature is broken. Report are coming from SQL so need date in ISO format.
Comment 6 Amit Gupta 2015-06-19 09:30:27 UTC Comment hidden (obsolete)
Comment 7 Amit Gupta 2015-06-19 09:35:07 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-06-19 10:26:07 UTC
When I select a value using the datepicker plugin, the input is filled with 
"20062006-05-31". Don't understand why the year is duplicated.
Anyway, I think the date should be displayed according to the pref. If we need to do some manipulation with it, we can do it in the pl script.
Comment 9 Amit Gupta 2015-06-19 10:32:51 UTC Comment hidden (obsolete)
Comment 10 Amit Gupta 2015-06-19 10:33:30 UTC
Hi Jonathan Druart,

I have fixed it. Please check again.
Comment 11 M. Tompsett 2015-06-23 03:17:05 UTC
(In reply to Jonathan Druart from comment #8)
> When I select a value using the datepicker plugin, the input is filled with 
> "20062006-05-31". Don't understand why the year is duplicated.

Amit fixed this.... HOWEVER...


> Anyway, I think the date should be displayed according to the pref. If we
> need to do some manipulation with it, we can do it in the pl script.

I agree with Jonathan on this. I will probably look at the technicalities of implementation tomorrow.
Comment 12 M. Tompsett 2015-06-24 16:34:42 UTC Comment hidden (obsolete)
Comment 13 Indranil Das Gupta 2015-06-24 16:56:36 UTC Comment hidden (obsolete)
Comment 14 Amit Gupta 2015-06-25 03:30:18 UTC Comment hidden (obsolete)
Comment 15 M. Tompsett 2015-06-26 16:59:09 UTC
Comment on attachment 40611 [details] [review]
Bug 14382: Non-ISO Date parameters generate empty reports.

Review of attachment 40611 [details] [review]:
-----------------------------------------------------------------

::: reports/guided_reports.pl
@@ +755,4 @@
>                  # if there are special regexp chars, we must \ them
>                  $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g;
> +                if ($split[$i*2+1] =~ /|date>>/) {
> +                    $quoted = substr(dt_from_string($quoted),0,10);

I blame Amit Gupta for showing me this mistake.
output_pref({ dt => (dt_from_string() + $days), dateformat => 'iso', dateonly => 1 });
I should have used output_pref!
Comment 16 M. Tompsett 2015-06-26 17:22:42 UTC Comment hidden (obsolete)
Comment 17 Amit Gupta 2015-06-26 17:53:39 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2015-07-03 09:28:46 UTC
Created attachment 40776 [details] [review]
Bug 14382: Non-ISO Date parameters generate empty reports.

The issue is that SQL expects ISO dates, but the user may wish to view dates according to the dateformat system preference.

By detecting a date preference, the non-ISO dates can be converted to ISO dates before being stuffed back into the SQL query to be executed.

TEST PLAN
---------
1) Add a report with date parameters.
   -- I used 'Holds placed in date range' from
      http://wiki.koha-community.org/wiki/SQL_Reports_Library
2) Set your dateformat to YYYY-MM-DD
3) Run the report
   -- Note the SQL reads
      "... BETWEEN '{date formatted in YYYY-MM-DD}'..."
   -- If there is supposed to be data, there is some.
4) Set your dateformat to MM/DD/YYYY
5) Run the report
   -- Note the SQL reads
      "... BETWEEN '{date formatted in MM/DD/YYYY}'..."
   -- If there is supposed to be data, there is none.
6) Apply patch
7) Repeat steps 2-5
   -- The SQL will always read YYYY-MM-DD (ISO) format.
   -- The report will have data, if there is some.
8) koha qa test tools.

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Adding a QA follow-up.
Comment 19 Marcel de Rooy 2015-07-03 09:28:50 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2015-07-03 09:29:59 UTC
$a= "<<Between (yyyy-mm-dd)|date>>";
$b= "test";
if ($a  =~ /|date>>/) {
    print "a true";
}
if ($b  =~ /|date>>/) {
    print "b true";
}
Comment 21 M. Tompsett 2015-07-03 13:05:00 UTC
Comment on attachment 40777 [details] [review]
Bug 14382: [QA Follow-up] Bad regex alarm

Review of attachment 40777 [details] [review]:
-----------------------------------------------------------------

::: reports/guided_reports.pl
@@ +754,4 @@
>                  my $quoted = $sql_params[$i];
>                  # if there are special regexp chars, we must \ them
>                  $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g;
> +                if ($split[$i*2+1] =~ /\|date$/) {

While I agree with the escaping (I really should have caught that, DOH!), the $ does not work. Why not leave >>? >'s are not special characters, are they?
I was thinking it may be better to put:
/\|\s*date\s*>>/
Comment 22 Marcel de Rooy 2015-07-03 15:12:10 UTC
(In reply to M. Tompsett from comment #21)
> Comment on attachment 40777 [details] [review] [review]
> Bug 14382: [QA Follow-up] Bad regex alarm
> 
> Review of attachment 40777 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: reports/guided_reports.pl
> @@ +754,4 @@
> >                  my $quoted = $sql_params[$i];
> >                  # if there are special regexp chars, we must \ them
> >                  $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g;
> > +                if ($split[$i*2+1] =~ /\|date$/) {
> 
> While I agree with the escaping (I really should have caught that, DOH!),
> the $ does not work. Why not leave >>? >'s are not special characters, are
> they?
> I was thinking it may be better to put:
> /\|\s*date\s*>>/

Please explain why the $ should not work. I agree btw that \s* could be put in additionally. But the >> should be removed, because of the split /<<|>>/,$sql.
When I tested this, it worked.
Comment 23 M. Tompsett 2015-07-03 15:27:37 UTC
I wrote:
>> I was thinking it may be better to put:
>> /\|\s*date\s*>>/


Marcel wrote:
> Please explain why the $ should not work. ...
>  But the >> should be removed, because of the split
> /<<|>>/,$sql.

DOH! Larger code context. You are right.


> I agree btw that \s* could be put
> in additionally.
> When I tested this, it worked.

So we agree that:
/\|\s*date\s*$/
would be better. :)
Comment 24 Marcel de Rooy 2015-07-03 15:41:29 UTC
Created attachment 40781 [details] [review]
Bug 14382: [QA Follow-up] Bad regex alarm

The regex /|date>>/ will match much more than you like :)
The unescaped pipe is bad, but you also need to remove the >> because
the split a few lines above it removes them already.

This allows you to recover from an error like this one, running another
report with a string parameter:
The given date (india%) does not match the date format (us) at
Koha/DateUtils.pm line 144.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended for possible spaces around the word date.
Comment 25 Tomás Cohen Arazi 2015-07-07 18:30:20 UTC
Patches pushed to master.

Thanks Mark and Marcel!
Comment 26 Chris Cormack 2015-07-14 07:32:12 UTC
Pushed to 3.20.x will be in 3.20.2
Comment 27 Liz Rea 2015-07-20 22:14:50 UTC
Pushed to 3.18.x will be in 3.18.09
Comment 28 Katrin Fischer 2015-08-03 05:40:00 UTC
*** Bug 14630 has been marked as a duplicate of this bug. ***
Comment 29 Mason James 2015-09-17 20:57:08 UTC
Pushed to 3.16.x, will be in 3.16.15