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.
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>
How does that look to you?
Created attachment 101909 [details] [review] [DO NOT PUSH] Bug 24980: This patch is only for test purposes
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>
(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!
Excellent, thanks Agustin.
(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:"
When should we deal with tz when displaying dates? What are the different possibilities offered by js-date-format.inc?
(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?
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.
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?
7. Should not deal with that in js-date-format.inc? (data&&$date(data))||'';
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
(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
(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.
Yes, that was for bug 20936!
Created attachment 102157 [details] [review] Bug 24980: (follow-up) return '' when date is null
How do I test the timezone handling in real life? ie. without using the console.
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 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)
(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.
(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.
(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.
(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.
So basically this first patch is for (maybe) later, and should not block other bug reports? And second patch should be moved somewhere else.
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>
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>
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.
Nice work everyone! Pushed to master for 20.05
does not apply cleanly to 19.11 not backported