Bug 24980 - Date formatting from JS - use timezone only with dates with offset
Summary: Date formatting from JS - use timezone only with dates with offset
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Agustín Moyano
QA Contact: Testopia
URL:
Keywords:
Depends on: 24455
Blocks: 24999 25328
  Show dependency treegraph
 
Reported: 2020-03-25 19:35 UTC by Agustín Moyano
Modified: 2020-05-08 22:46 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24980: (RM follow-up) Only add timezone if passed a date-time (2.59 KB, patch)
2020-03-26 09:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 24980: This patch is only for test purposes (924 bytes, patch)
2020-03-26 20:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24980: (RM follow-up) Only add timezone if passed a date-time (3.06 KB, patch)
2020-03-26 20:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24980: (follow-up) return '' when date is null (2.21 KB, patch)
2020-03-31 20:57 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24980: (RM follow-up) Only add timezone if passed a date-time (3.13 KB, patch)
2020-04-30 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24980: (follow-up) return '' when date is null (2.29 KB, patch)
2020-04-30 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2020-03-25 19:35:40 UTC
Bug 24455 uses timezone to format any date that comes.. even dates like '2020-04-04' that specifies no offset.

That date gets translated to '2020-04-04T00:00:00Z', and if you've got a timezone with -NN:NN if you ask for the date, you may get '2020-04-03' which is wrong.

This bug will try to fix that.
Comment 1 Martin Renvoize 2020-03-26 09:57:13 UTC
Created attachment 101794 [details] [review]
Bug 24980: (RM follow-up) Only add timezone if passed a date-time

This patch prevents a timezone from being assigned to the js moment if
the input string is only at the full-date and not date-time resolution

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Martin Renvoize 2020-03-26 09:57:55 UTC
How does that look to you?
Comment 3 Agustín Moyano 2020-03-26 20:43:25 UTC
Created attachment 101909 [details] [review]
[DO NOT PUSH] Bug 24980: This patch is only for test purposes
Comment 4 Agustín Moyano 2020-03-26 20:43:29 UTC
Created attachment 101910 [details] [review]
Bug 24980: (RM follow-up) Only add timezone if passed a date-time

This patch prevents a timezone from being assigned to the js moment if
the input string is only at the full-date and not date-time resolution

To test:
1. Do not apply this patch
2. Set your system time zone to CET
3. Apply previous patch (It's just for test)
4. In your browser go to koha main page, open a console and type $date('2020-03-25', {tz:
   'UTC'})
CHECK => you sould get '03/24/2020' (notice day is 24 instead of 25)
5. Apply this patch and reload the browser
6. repeat step 4
SUCCESS => now you get '03/25/2020' (day is correct)
7. Sing off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 5 Agustín Moyano 2020-03-26 20:45:25 UTC
(In reply to Martin Renvoize from comment #2)
> How does that look to you?

Great, it was just what I was looking for.. it just lacked the actual execution of creationData function.. too much Perl I guess, hehe

Thanks!
Comment 6 Martin Renvoize 2020-03-26 20:47:59 UTC
Excellent, thanks Agustin.
Comment 7 Agustín Moyano 2020-03-26 20:49:12 UTC
(In reply to Agustín Moyano from comment #4)
> 4. In your browser go to koha main page, open a console and type
> $date('2020-03-25', {tz:
>    'UTC'})

Don't know why it added an enter.. in console you should type like this

$date(''2020-03-25', {tz:'UTC'})

without that enter after "tz:"
Comment 8 Jonathan Druart 2020-03-27 07:25:58 UTC
When should we deal with tz when displaying dates?

What are the different possibilities offered by js-date-format.inc?
Comment 9 Agustín Moyano 2020-03-27 12:39:56 UTC
(In reply to Jonathan Druart from comment #8)
> When should we deal with tz when displaying dates?
> 
> What are the different possibilities offered by js-date-format.inc?

Hi Jonathan, with this patch, js-date-format can detect if incoming date is just date only, or something else.. 

So, this functions should be used to transform date and datetime to Koha's defined format. If the first, it's just transformed to the corresponding format, if the later it will also be transformed to the corresponding timezone.

With Martin's bug 24850, datetimes from database has timezone now, so I don't believe we will have more cases, but maybe I'm not seeing the big picture.

Do you have any other case in mind?
Comment 10 Martin Renvoize 2020-03-27 12:48:10 UTC
OK.. so my understanding of why we need to support timezone in this js function at all is that we are effectively replacing the native toLocalDateString().  That function, in my understanding, does too things.. it converts Date prototypes to a locale based formatted string and also ensures the output is in the local timezone (if the original Date prototype has a timezone associated with it upon creation).

Our function takes an RFC3339 formatted string, which is what the API returns. These strings have timezones attached, if we used toLocalDateString to output them, then we would get the timezone of the browser applied.. which may differ from our koha instances configured timezone.. and thus the one that all our templates currently output in.
Comment 11 Jonathan Druart 2020-03-30 12:55:09 UTC
0. Dates look great now! :)

1. Why not having current holds in the table (behavior intranet-side, members/holdshistory.pl)

2. Is there a plan to make existing intranet-side view using the REST API?
ie. only 1 code to maintain.

3. Why only 10 results displayed by default? Usually 20

4. Why no way to modify the number of results displayed? Basically, why DataTable does not display as other table? (like no page number, search box, etc.)
Is that the OPAC style?

5. Not translatable strings:
187                         if(row.cancelation_date) return 'Canceled';
188                         if (data == 'F') return 'Fulfilled';
189                         if (data == 'W') return 'Waiting';
190                         if (data == 'T') return 'In Transit';
191                         return 'Pending';

6. opac-holdsrecord.pl vs members/holdshistory.pl
Should not we use the same script names?
Comment 12 Jonathan Druart 2020-03-30 12:59:04 UTC
7. Should not deal with that in js-date-format.inc?
  (data&&$date(data))||'';
Comment 13 Agustín Moyano 2020-03-30 15:25:40 UTC
Hi Jonathan,

(In reply to Jonathan Druart from comment #11)
> 0. Dates look great now! :)
> 
> 1. Why not having current holds in the table (behavior intranet-side,
> members/holdshistory.pl)

Controller uses object.search, and places order by in dbic.. to replicate current behaviour in intranet I should one object.search for both current and old holds, merge and sort both arrays depending on search column... I decided to wait till requests and old_requests tables were merged to have current intranet behaviour 

> 
> 2. Is there a plan to make existing intranet-side view using the REST API?
> ie. only 1 code to maintain.
> 

Once we can replicate current intranet behaviour, we can think on changing intranet-side

> 3. Why only 10 results displayed by default? Usually 20
> 

ok, will change that

> 4. Why no way to modify the number of results displayed? Basically, why
> DataTable does not display as other table? (like no page number, search box,
> etc.)
> Is that the OPAC style?
> 

OPAC has no style for DataTables.. only intranet has. What you see in other tables in OPAC was coded outside DataTables. I thought it best to have a search box and page number, but I'm open to suggestions.

> 5. Not translatable strings:
> 187                         if(row.cancelation_date) return 'Canceled';
> 188                         if (data == 'F') return 'Fulfilled';
> 189                         if (data == 'W') return 'Waiting';
> 190                         if (data == 'T') return 'In Transit';
> 191                         return 'Pending';
> 

Good catch.. will fix that

> 6. opac-holdsrecord.pl vs members/holdshistory.pl
> Should not we use the same script names?

copied the name from opac-readingrecord, but it can be changed
Comment 14 Agustín Moyano 2020-03-30 15:53:20 UTC
(In reply to Jonathan Druart from comment #12)
> 7. Should not deal with that in js-date-format.inc?
>   (data&&$date(data))||'';

ok, it can be done
Comment 15 Agustín Moyano 2020-03-31 16:10:37 UTC
(In reply to Agustín Moyano from comment #13)
> Hi Jonathan,
> 
> (In reply to Jonathan Druart from comment #11)
> > 0. Dates look great now! :)
> > 
> > 1. Why not having current holds in the table (behavior intranet-side,
> > members/holdshistory.pl)
> 
> Controller uses object.search, and places order by in dbic.. to replicate
> current behaviour in intranet I should one object.search for both current
> and old holds, merge and sort both arrays depending on search column... I
> decided to wait till requests and old_requests tables were merged to have
> current intranet behaviour 
> 
> > 
> > 2. Is there a plan to make existing intranet-side view using the REST API?
> > ie. only 1 code to maintain.
> > 
> 
> Once we can replicate current intranet behaviour, we can think on changing
> intranet-side
> 
> > 3. Why only 10 results displayed by default? Usually 20
> > 
> 
> ok, will change that
> 
> > 4. Why no way to modify the number of results displayed? Basically, why
> > DataTable does not display as other table? (like no page number, search box,
> > etc.)
> > Is that the OPAC style?
> > 
> 
> OPAC has no style for DataTables.. only intranet has. What you see in other
> tables in OPAC was coded outside DataTables. I thought it best to have a
> search box and page number, but I'm open to suggestions.
> 
> > 5. Not translatable strings:
> > 187                         if(row.cancelation_date) return 'Canceled';
> > 188                         if (data == 'F') return 'Fulfilled';
> > 189                         if (data == 'W') return 'Waiting';
> > 190                         if (data == 'T') return 'In Transit';
> > 191                         return 'Pending';
> > 
> 
> Good catch.. will fix that
> 
> > 6. opac-holdsrecord.pl vs members/holdshistory.pl
> > Should not we use the same script names?
> 
> copied the name from opac-readingrecord, but it can be changed

Jonathan, I just realized this are comments mostly for bug 20936, not this one.. I'll change what was suggested in point 7, and put this one in SO again.
Comment 16 Jonathan Druart 2020-03-31 16:29:50 UTC
Yes, that was for bug 20936!
Comment 17 Agustín Moyano 2020-03-31 20:57:07 UTC
Created attachment 102157 [details] [review]
Bug 24980: (follow-up) return '' when date is null
Comment 18 Jonathan Druart 2020-04-01 08:25:50 UTC
How do I test the timezone handling in real life? ie. without using the console.
Comment 19 Martin Renvoize 2020-04-01 09:49:41 UTC
I initially liked that final patch.. but do we believe it's correct consistency wise given that dt_from_string (which we're using in KohaDates which with function is emulating) will return 'now' when passed 'undef'. ?
Comment 20 Agustín Moyano 2020-04-01 19:12:40 UTC
(In reply to Jonathan Druart from comment #18)
> How do I test the timezone handling in real life? ie. without using the
> console.

Actually there is no place (even bug 20936) that brings timestamp or datetime. If you want to try to see if it works with koha's configured timezone you should use the console without the tz option, then format a datetime with timezone. Later change koha's timezone, restart plack and reload the page and try to format the same datetime, it should change according to the new timestamp

If it works in the console, it should work on real life (it's the same function)
Comment 21 Agustín Moyano 2020-04-01 19:14:06 UTC
(In reply to Martin Renvoize from comment #19)
> I initially liked that final patch.. but do we believe it's correct
> consistency wise given that dt_from_string (which we're using in KohaDates
> which with function is emulating) will return 'now' when passed 'undef'. ?

In my opinion, if the API's date is null, we shouldn't return current date.
Comment 22 Jonathan Druart 2020-04-02 08:40:58 UTC
(In reply to Agustín Moyano from comment #20)
> (In reply to Jonathan Druart from comment #18)
> > How do I test the timezone handling in real life? ie. without using the
> > console.
> 
> Actually there is no place (even bug 20936) that brings timestamp or
> datetime. If you want to try to see if it works with koha's configured
> timezone you should use the console without the tz option, then format a
> datetime with timezone. Later change koha's timezone, restart plack and
> reload the page and try to format the same datetime, it should change
> according to the new timestamp
> 
> If it works in the console, it should work on real life (it's the same
> function)

Do we need it then?

(In reply to Agustín Moyano from comment #21)
> (In reply to Martin Renvoize from comment #19)
> > I initially liked that final patch.. but do we believe it's correct
> > consistency wise given that dt_from_string (which we're using in KohaDates
> > which with function is emulating) will return 'now' when passed 'undef'. ?
> 
> In my opinion, if the API's date is null, we shouldn't return current date.

I agree, this patch looks good to me.
Comment 23 Jonathan Druart 2020-04-08 10:49:55 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Agustín Moyano from comment #20)
> > (In reply to Jonathan Druart from comment #18)
> > > How do I test the timezone handling in real life? ie. without using the
> > > console.
> > 
> > Actually there is no place (even bug 20936) that brings timestamp or
> > datetime. If you want to try to see if it works with koha's configured
> > timezone you should use the console without the tz option, then format a
> > datetime with timezone. Later change koha's timezone, restart plack and
> > reload the page and try to format the same datetime, it should change
> > according to the new timestamp
> > 
> > If it works in the console, it should work on real life (it's the same
> > function)
> 
> Do we need it then?

Changing status as we are waiting for an answer.
Comment 24 Agustín Moyano 2020-04-08 20:20:29 UTC
(In reply to Jonathan Druart from comment #23)
> (In reply to Jonathan Druart from comment #22)
> > (In reply to Agustín Moyano from comment #20)
> > > (In reply to Jonathan Druart from comment #18)
> > > > How do I test the timezone handling in real life? ie. without using the
> > > > console.
> > > 
> > > Actually there is no place (even bug 20936) that brings timestamp or
> > > datetime. If you want to try to see if it works with koha's configured
> > > timezone you should use the console without the tz option, then format a
> > > datetime with timezone. Later change koha's timezone, restart plack and
> > > reload the page and try to format the same datetime, it should change
> > > according to the new timestamp
> > > 
> > > If it works in the console, it should work on real life (it's the same
> > > function)
> > 
> > Do we need it then?
> 
> Changing status as we are waiting for an answer.

Yes, I think we do.. I'm sure at some point there's going to be a datatable that brings a datetime or timestamp from the database.

If you'd like to test without console, you can locally modify the table in bug 20936 and display the timestamp column, that it's a timestamp as it's name suggests.
Comment 25 Jonathan Druart 2020-04-09 10:41:34 UTC
So basically this first patch is for (maybe) later, and should not block other bug reports?
And second patch should be moved somewhere else.
Comment 26 Jonathan Druart 2020-04-30 12:09:17 UTC
Created attachment 104025 [details] [review]
Bug 24980: (RM follow-up) Only add timezone if passed a date-time

This patch prevents a timezone from being assigned to the js moment if
the input string is only at the full-date and not date-time resolution

To test:
1. Do not apply this patch
2. Set your system time zone to CET
3. Apply previous patch (It's just for test)
4. In your browser go to koha main page, open a console and type $date('2020-03-25', {tz:
   'UTC'})
CHECK => you sould get '03/24/2020' (notice day is 24 instead of 25)
5. Apply this patch and reload the browser
6. repeat step 4
SUCCESS => now you get '03/25/2020' (day is correct)
7. Sing off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2020-04-30 12:09:20 UTC
Created attachment 104026 [details] [review]
Bug 24980: (follow-up) return '' when date is null

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2020-04-30 12:11:08 UTC
I still think we are doing messy stuff with the timezone. I may be wrong (don't have the situation clear in mind right now) be I feel like the whole timezone handling should be removed from the JS code.

If you think I am wrong, you should prove it to me with trivial and easy to follow examples.
Comment 29 Martin Renvoize 2020-05-01 07:20:13 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 30 Joy Nelson 2020-05-08 22:46:34 UTC
does not apply cleanly to 19.11 not backported