erp5_web_renderjs_ui: datetime field formats date in the interface language

Also hide seconds as it weights down the interface, and to follow the behavior of XML style

bt5/erp5_web_renderjs_ui/PathTemplateItem/web_page_module/rjs_gadget_erp5_datetimefield_js.js

@@ -32,7 +32,7 @@
   ];
 
   rJS(window)
-
+    .declareAcquiredMethod('getSelectedLanguage', 'getSelectedLanguage')
     .declareMethod('render', function (options) {
       var field_json = options.field_json || {},
         state_dict = {
@@ -228,24 +228,52 @@
       } else {
         queue
           .push(function (gadget_list) {
-            var text_content = "",
+            return RSVP.all([
+              gadget.getSelectedLanguage(),
+              gadget_list
+            ]);
+          })
+          .push(function (result_list) {
+            var language = result_list[0],
+              gadget_list = result_list[1],
+              text_content = "",
               state_date,
+              locale_formatted_state_date,
               offset_time_zone;
             if (gadget.state.value) {
               state_date = new Date(gadget.state.value);
+              /* Ideally we would like to use {timeStyle: "short"} as option
+               * to hide seconds. Unfortunately it doesn't work in older
+               * versions of firefox. Luckily, by using
+               *   {hour: "numeric", minute: "numeric"}
+               * it hides seconds, and still respects the locale.
+               * >> date = new Date(2019, 1, 1, 1, 1)
+               * >> date.toLocaleTimeString(
+               *      'en', {hour: "numeric", minute: "numeric"}
+               *    )
+               *    "1:01 AM"
+               * >> date.toLocaleTimeString(
+               *      'fr', {hour: "numeric", minute: "numeric"}
+               *    )
+               *    "01:01"
+               */
+              locale_formatted_state_date = state_date.toLocaleTimeString(
+                language,
+                {hour: "numeric", minute: "numeric"}
+              );
               if (gadget.state.timezone_style) {
-                text_content = state_date.toLocaleDateString();
+                text_content = state_date.toLocaleDateString(language);
                 if (!gadget.state.date_only) {
-                  text_content += " " + state_date.toLocaleTimeString();
+                  text_content += " " + locale_formatted_state_date;
                 }
               } else {
                 //get timezone difference between server and local browser
                 offset_time_zone = timezone + (state_date.getTimezoneOffset() / 60);
                 //adjust hour in order to get correct date time string
                 state_date.setUTCHours(state_date.getUTCHours() + offset_time_zone);
-                text_content = state_date.toLocaleDateString();
+                text_content = state_date.toLocaleDateString(language);
                 if (!gadget.state.date_only) {
-                  text_content += " " + state_date.toLocaleTimeString();
+                  text_content += " " + locale_formatted_state_date;
                 }
               }
             }

bt5/erp5_web_renderjs_ui/PathTemplateItem/web_page_module/rjs_gadget_erp5_datetimefield_js.xml

@@ -142,11 +142,13 @@
   </record>
   <record id="3" aka="AAAAAAAAAAM=">
     <pickle>
-      <global name="WorkflowHistoryList" module="Products.ERP5Type.patches.WorkflowTool"/>
+      <global name="WorkflowHistoryList" module="Products.ERP5Type.Workflow"/>
     </pickle>
     <pickle>
-      <tuple>
-        <none/>
+      <dictionary>
+        <item>
+            <key> <string>_log</string> </key>
+            <value>
               <list>
                 <dictionary>
                   <item>
@@ -190,16 +192,20 @@
                   </item>
                 </dictionary>
               </list>
-      </tuple>
+            </value>
+        </item>
+      </dictionary>
     </pickle>
   </record>
   <record id="4" aka="AAAAAAAAAAQ=">
     <pickle>
-      <global name="WorkflowHistoryList" module="Products.ERP5Type.patches.WorkflowTool"/>
+      <global name="WorkflowHistoryList" module="Products.ERP5Type.Workflow"/>
     </pickle>
     <pickle>
-      <tuple>
-        <none/>
+      <dictionary>
+        <item>
+            <key> <string>_log</string> </key>
+            <value>
               <list>
                 <dictionary>
                   <item>
@@ -222,7 +228,7 @@
                   </item>
                   <item>
                       <key> <string>serial</string> </key>
-                <value> <string>965.12118.35525.1655</string> </value>
+                      <value> <string>980.55051.50282.19404</string> </value>
                   </item>
                   <item>
                       <key> <string>state</string> </key>
@@ -240,7 +246,7 @@
                           </tuple>
                           <state>
                             <tuple>
-                        <float>1517930509.55</float>
+                              <float>1577775957.85</float>
                               <string>UTC</string>
                             </tuple>
                           </state>
@@ -249,7 +255,9 @@
                   </item>
                 </dictionary>
               </list>
-      </tuple>
+            </value>
+        </item>
+      </dictionary>
     </pickle>
   </record>
 </ZopeData>

bt5/erp5_web_renderjs_ui_test/PathTemplateItem/portal_tests/renderjs_ui_date_time_field_zuite/testDateTimeDefaultToNow.zpt

@@ -153,6 +153,19 @@
   <td>glob:${now}*</td>
 </tr>
 
+<!-- Seconds should also be invisible in non-editable mode,
+     and that output follows locale (here "en") -->
+
+<tal:block metal:use-macro="here/Zuite_CommonTemplateForRenderjsUi/macros/toggle_editable_mode" />
+<tal:block metal:use-macro="here/Zuite_CommonTemplateForRenderjsUi/macros/verify_is_non_editable_mode" />
+<tal:block metal:use-macro="here/Zuite_CommonTemplateForRenderjsUi/macros/wait_for_content_loaded" />
+
+<tr>
+  <td>assertText</td>
+  <td>//div[@data-gadget-scope='field_my_start_date']//p</td>
+  <td>regexp:1?\d/[123]?\d/20\d\d \d\d?:\d\d (AM|PM)</td>
+</tr>
+
 </tbody></table>
 </body>
 </html>
\ No newline at end of file

bt5/erp5_web_renderjs_ui_test/PathTemplateItem/portal_tests/renderjs_ui_date_time_field_zuite/testDisplayWithAndWithoutTimeZone.zpt

@@ -82,7 +82,7 @@
 
 <tr>
   <td>assertElementPresent</td>
-  <td>//div[@data-gadget-scope='field_my_start_date']//div[@data-gadget-scope='field']//p[contains(text(), '11:00:00')]</td>
+  <td>//div[@data-gadget-scope='field_my_start_date']//div[@data-gadget-scope='field']//p[contains(text(), '11:00')]</td>
   <td></td>
 </tr>
 

bt5/erp5_web_renderjs_ui_test/PathTemplateItem/portal_tests/renderjs_ui_date_time_field_zuite/testSetDateTimeZone.zpt

@@ -82,7 +82,7 @@
 
 <tr>
   <td>assertElementPresent</td>
-  <td>//div[@data-gadget-scope='field_my_start_date']//div[@data-gadget-scope='field']//p[contains(text(), '11:00:00')]</td>
+  <td>//div[@data-gadget-scope='field_my_start_date']//div[@data-gadget-scope='field']//p[contains(text(), '11:00')]</td>
   <td></td>
 </tr>
 

[Jérome Perrin]

@Nicolas @xiaowu.zhang in this branch, we use locale_formatted_state_date, but state_date was modified just above ( state_date.setUTCHours(state_date.getUTCHours() + offset_time_zone); ). I think we should call toLocaleTimeString again on the modified date.

Maybe move this

              locale_formatted_state_date = state_date.toLocaleTimeString(
                language,
                {hour: "numeric", minute: "numeric"}
              );

in a function and call it in both branches of this if ?

/cc @romain

[Jérome Perrin]

also, this seems not correct:

                state_date.setUTCHours(state_date.getUTCHours() + offset_time_zone);

what if local timezone is before UTC and the local time should be another day ?

what if timezone is something like +5:30 ?

This is not something from this change, the code before looks incorrect.

[Nicolas Wavrant]

@jerome , @romain

I have a fix here : e589ded7

Tests pass locally. Is it OK to push ?

[Jérome Perrin]

great, thank you !

We can fix the cases of timezones before UTC or where the offset is not a multiple of one hour later, that commit already fixes the regression for most of us.

I'm wondering how we can get a test failing before the fix. Things like https://github.com/plaa/TimeShift-js seems hard to integrate reliably because it would mean that from Zelenium we have to patch Date in the gadget namespace. Maybe we could run firefox with a different timezone using an approach like 7a91ddf5 ? that would not work when running on remote selenium (like we did with saucelabs), but if this approach works, maybe we could make a test class which runs timezone functional tests in various timezones (for example: Europe/Paris, America/Sao_Paulo, Asia/Colombo)

[Jérome Perrin]

that commit already fixes the regression for most of us.

That's not exact. This commit fixes the regression. It will still be wrong in some cases, but same as before, so it's definitely an improvement and I believe we can push this commit on master.

[Jérome Perrin]

What test* is supposed to fail ? erp5_web_renderjs_ui_test:testFunctionalRJSDateTimeField ?

[Jérome Perrin]

run firefox with a different timezone using an approach like 7a91ddf5 ?

there was a mistake in this change, I'm trying again as 74126d27

[Romain Courteaud]

without adding a test, the issue will come back next time one update the field code.

@Nicolas please add a test reproducing the expected behaviour you want to achieve.

[Jérome Perrin]

The proof of concept from 74126d27 seem to work. If we run firefox with TZ=Europe/Paris, then erp5_web_renderjs_ui_test:testFunctionalRJSDateTimeField fail as expected:

...

In this test, zope runs as UTC and browser as Europe/Paris. This never failed on master because both zope and browser are on UTC.

How do you think about this approach ? We could run this test and other tests depending on the browser timezone on multiple timezones like this ( before UTC, after UTC etc ) ?

[Nicolas Wavrant]

Wouldn't it possible to write a test where I set a date with a timezone ?

I tried in my instance to set this date on a document :

In the new interface (without this commit), it renders as :

But this seems super buggy to me, as if I hide the timezone in the field definition I get :

[Nicolas Wavrant]

(So we assert on a buggy behavior of the old interface, but at least the test is easy to write)

[Jérome Perrin]

Wouldn't it possible to write a test where I set a date with a timezone ?

Isn't it another case ?

But this seems super buggy to me, as if I hide the timezone in the field definition I get :

"hide timezone" just does not show timezone, in a sense it's correct.

[Nicolas Wavrant]

Wouldn't it possible to write a test where I set a date with a timezone ?

Isn't it another case ?

If the date is in a timezone different from server and from client, then I think it will be testing the behavior we want.

[Jérome Perrin]

Ah yes, if we can consider that this specific problem is purely a client side bug that is independant of the server timezone, then it's probably enough to test it with a date that is different from client timezone.

That said, now that we have timezone related code in javascript, it could be nice one day to be able to test this under different timezones, but you are right, let's first fix this bug and later we can consider improving this part of our tests.

[Nicolas Wavrant]

@romain : my first plan to test was to create a bar and calling on it .setStartDate(some_date_in_a_timezone_different_from_server_and_client). I notice that the portal type Bar doesn't have property sheets. Is it on purpose ?

[Romain Courteaud]

No idea. Try with a Foo maybe