fix: Made timestamp consistant #46

atiqueahmedziad · 2020-08-17T14:54:17Z

Fixes #41

log.timeStamp contains timestamp (number).
While viewing logs, the timestamp is being formatted into human-readable format i.e. 8:49:31 PM (hh:mm:ss am/pm)
When hover the timestamp in log-view, it shows tooltip in the format 17-8-2020 8:49:31 PM
The timestamp filter texts also show the same format as tooltip i.e. 17-8-2020 8:49:31 PM

Screenshot

atiqueahmedziad · 2020-08-17T14:57:53Z

src/lib/web-component/filter-timestamp/filter-timestamp-element.js

-    const selectedRow = selectedEl.closest('tr');
+    if (!selectedEl) {
+      return;
+    }


~~When we have multiple activity log page opened at a time, multiple context menu event listeners are being assigned but we get only one selectedEl (from where the context event has been called)~~

Update: opened another PR #47 , as this change is unrelated to this PR. Removed this change from here.

Rob--W · 2020-08-17T16:19:45Z

src/lib/ext-listen.js

+export function dateTimeFormat(timestamp) {
+  const date = new Date(timestamp);
+  const month = date.getMonth() + 1;
+  return `${date.getDate()}-${month}-${date.getFullYear()} ${date.toLocaleTimeString()}`;


dd-MM-YYYY looks reasonable to me because the format is the same as where I am from (The Netherlands), but it is not universally true.

Since you're trying to have locale-aware time strings, I suggest to be consistent.

Use the Intl.DateTimeFormat API to format the date in a locale-aware way - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat

Rob--W · 2020-08-17T16:24:41Z

src/lib/web-component/filter-timestamp/filter-timestamp-element.js

@@ -33,14 +35,15 @@ export class FilterTimestamp extends HTMLElement {

    const timeStamp = this.timeStamp || {};

-    const chosenTimestamp = selectedRow.querySelector('.timestamp').textContent;
+    const chosenTimestamp = selectedRow.querySelector('.timestamp')?._timestamp;


Use selectedRow._log.timeStamp instead.

I would rather not have an external class using an internal _log property, but it is preferable to your current approach, of adding a _timeStamp property to an unrelated DOM element.

To reduce the risk of future regressions, it would be nice to have a unit test that verifies that the time labels are rendered as expected.

Rob--W · 2020-08-20T12:42:16Z

src/lib/ext-listen.js

+
+export function dateTimeFormat(timestamp, options) {
+  const dateTime = new Date(timestamp);
+  const time = dateTime.toLocaleTimeString();


DateTimeFormat also supports parameters to include the time in the generated string. Do not use toLocaleTimeString, but only DateTimeFormat.

Rob--W · 2020-08-20T12:42:52Z

tests/ext-activitylog.test.js

+  });
+
+  document.body.innerHTML = activityLogBody;
+  const dateTimeFormatFn = jest.spyOn(ExtListen, 'dateTimeFormat');


You shouldn't mock your function, but the API that you're using. The only changes should be:

fix 'en-US' as the locale instead of undefined.

fix the timeZone option.

By doing both, the generated time strings become fully deterministic / predictable, and you can put the expected times in strings below.

done :-) Thanks

Rob--W · 2020-08-20T12:47:45Z

tests/ext-activitylog.test.js

+  expect(dateTimeFormatFn).toHaveBeenCalled();
+
+  const dateTime = new Date(logTimestamp);
+  const expectedTime = dateTime.toLocaleTimeString();


Hard-code the expected strings instead of generating it based on the dynamic objects.

Rob--W · 2020-08-20T12:50:13Z

src/lib/ext-listen.js

@@ -31,3 +31,19 @@ export function openActivityLogPage() {
    url: getActivityLogPageURL(),
  });
 }
+
+export function dateTimeFormat(timestamp, options) {


Why did you put this function in ext-listen.js instead of a new file called, say, formatters.js?

yes, made a new file formatters.js

Rob--W · 2020-08-21T10:31:59Z

src/lib/formatters.js

+    dateTime
+  );
+
+  return `${date} ${time}`;


Don't create two DateTimeFormat instances followed by string concatenation. Let the value of timeFormatOptions be conditional based on options?.timeOnly.

Rob--W · 2020-08-21T10:34:06Z

tests/ext-activitylog.test.js

+  // To have consistant date time format, we choose "en-US" date time formatting and UTC timezone.
+  IntlDateTimeFormatFn.mockImplementation(function (zone, options) {
+    Object.assign(options, timeZone);
+    this.format = (date) =>


Don't replace the format method; just let the constructor return new originalIntlDateTimeFormat('en-US', options);.

Rob--W · 2020-08-21T12:09:28Z

tests/ext-activitylog.test.js

+  const { activityLog } = new ActivityLog();
+  // To have consistant date time format, we choose "en-US" date time formatting and UTC timezone.
+  IntlDateTimeFormatFn.mockImplementation((zone, options) => {
+    Object.assign(options, timeZone);


Mocks should not modify the input object, as that may have side effects.
Do this instead:

options = {...options, timeZone: 'UTC'}

Rob--W · 2020-08-21T12:18:05Z

src/lib/ext-monitor.js

@@ -34,6 +34,9 @@ export default class ExtensionMonitor {

  createLogListener() {
    return async (details) => {
+      // set a timestamp(number) as the value of timeStamp property
+      details.timeStamp = Date.parse(details.timeStamp);


Could you add the next comment:

// TODO: Stop using `Date.parse` when `details.timeStamp` is a numeric timestamp. // See https://bugzilla.mozilla.org/show_bug.cgi?id=1660460

atiqueahmedziad commented Aug 17, 2020

View reviewed changes

fix: Made timestamp consistant

ba05db8

atiqueahmedziad force-pushed the fix-timestamp branch from dbbf38d to ba05db8 Compare August 17, 2020 15:17

Rob--W requested changes Aug 17, 2020

View reviewed changes

comments are addressed

5f02cc4

atiqueahmedziad force-pushed the fix-timestamp branch from 561655c to bcf8de9 Compare August 17, 2020 18:05

test added

44fa52e

atiqueahmedziad force-pushed the fix-timestamp branch 2 times, most recently from 105def4 to 511fd3f Compare August 18, 2020 14:24

test updated

f52a0f0

atiqueahmedziad force-pushed the fix-timestamp branch from 511fd3f to f52a0f0 Compare August 18, 2020 15:03

Rob--W reviewed Aug 20, 2020

View reviewed changes

atiqueahmedziad added 2 commits August 21, 2020 15:22

update

ec1fc3c

update

48cb2e2

Rob--W reviewed Aug 21, 2020

View reviewed changes

comments are addressed

b613cef

atiqueahmedziad force-pushed the fix-timestamp branch from 0f67e05 to b613cef Compare August 21, 2020 10:54

atiqueahmedziad added 2 commits August 21, 2020 16:54

typo

4a2f23d

typo

dea2d66

Rob--W approved these changes Aug 21, 2020

View reviewed changes

Rob--W reviewed Aug 21, 2020

View reviewed changes

update

0a24856

Rob--W approved these changes Aug 21, 2020

View reviewed changes

atiqueahmedziad merged commit 68d5194 into mozilla:master Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Made timestamp consistant #46

fix: Made timestamp consistant #46

atiqueahmedziad commented Aug 17, 2020

atiqueahmedziad Aug 17, 2020 •

edited

Rob--W Aug 17, 2020

Rob--W Aug 17, 2020

Rob--W Aug 20, 2020

atiqueahmedziad Aug 21, 2020

Rob--W Aug 20, 2020

atiqueahmedziad Aug 21, 2020

Rob--W Aug 20, 2020

atiqueahmedziad Aug 21, 2020

Rob--W Aug 20, 2020

atiqueahmedziad Aug 21, 2020

Rob--W Aug 21, 2020

atiqueahmedziad Aug 21, 2020

Rob--W Aug 21, 2020

atiqueahmedziad Aug 21, 2020

Rob--W Aug 21, 2020

Rob--W Aug 21, 2020

fix: Made timestamp consistant #46

fix: Made timestamp consistant #46

Conversation

atiqueahmedziad commented Aug 17, 2020

Screenshot

atiqueahmedziad Aug 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiqueahmedziad Aug 17, 2020 •

edited