Fixing time zone problems #1624

lancewf · 2019-09-16T21:11:09Z

🔩 Description: What code changed, and why?

Fixing a bug where the compliance reporting page would not allow the user in UTC+1 view the current day. The problem was the reportQuery state was being changed unintentionally. There was also some sections of code that when converting from moment to date was switching the current hour because of time zones.

👍 Definition of Done

When in timezone UTC+1 the current date can be selected.

👟 How to Build and Test the Change

Switch your local computer to London time (UTC+1)
build components/automate-ui && start_all_services
Open the https://a2-dev.test/compliance/reports/overview
Ensure today's UTC date is selected.
Ensure the date can be switched to yesterday back to today.

✅ Checklist

Tests added/updated?
Docs added/updated?

📷 Screenshots, if applicable

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

lancewf · 2019-09-16T21:57:50Z

components/automate-ui/src/app/pages/+compliance/shared/reporting/report-query.service.ts

+      endDate: reportQuery.endDate.clone(),
+      interval: reportQuery.interval,
+      filters: Object.assign([], reportQuery.filters)
+    };


Returning a clone of the current state to avoid the state changing unintentionally

lancewf · 2019-09-16T21:58:44Z

components/automate-ui/src/app/pages/+compliance/shared/reporting/stats.service.ts


      apiFilters.push({type: 'start_time', values: [value.format()]});
    }

    if (reportQuery.endDate) {
-      const now = moment();
-      const value = reportQuery.endDate


This is the real bug. The hour for the state was being updated here.

lancewf · 2019-09-16T22:00:28Z

components/automate-ui/src/app/pages/+compliance/+reporting/reporting.component.ts

@@ -445,7 +446,7 @@ export class ReportingComponent implements OnInit, OnDestroy {
    const foundFilter = urlFilters.find( (filter: Chicklet) => filter.type === 'end_time');

    if (foundFilter !== undefined) {
-      const endDate = moment(foundFilter.text, 'YYYY-MM-DD');
+      const endDate = moment(foundFilter.text + 'GMT+00:00', 'YYYY-MM-DDZ');


parsing the date as UTC. Before the date would come in as the local users timezone.

lancewf · 2019-09-16T22:01:37Z

components/automate-ui/src/app/pages/+compliance/+reporting/reporting.component.ts

@@ -180,7 +180,8 @@ export class ReportingComponent implements OnInit, OnDestroy {
    const allUrlParameters$ = this.getAllUrlParameters();

    this.endDate$ = this.reportQuery.state.pipe(map((reportQuery: ReportQuery) =>
-      reportQuery.endDate.toDate()));
+      new Date(reportQuery.endDate.year(), reportQuery.endDate.month(),


Converting the moment to a date object without any time zone conversion. Where before the date's hours would be offset by the user's local timezone.

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

lancewf · 2019-09-16T23:13:22Z

components/automate-ui/src/app/pages/+compliance/+reporting/reporting.component.spec.ts

-      component.applyParamFilters([
-        {type: 'chef_tags', text: '123'},
-        {type: 'end_time', text: '2019-09-05'}]);
-      expect(reportQueryService.setState).toHaveBeenCalledWith(


This test would not pass anymore even though the same objects were displayed in the error. The 'getEndDate' test covers the same test case.

msorens · 2019-09-17T14:08:28Z

components/automate-ui/src/app/pages/+compliance/shared/reporting/report-query.service.ts

+      startDate: reportQuery.startDate.clone(),
+      endDate: reportQuery.endDate.clone(),
+      interval: reportQuery.interval,
+      filters: Object.assign([], reportQuery.filters)


Cleaner pattern for cloning that we (auth team) have been introducing in recent months--use the spread operator:

Suggested change

filters: Object.assign([], reportQuery.filters)

filters: [...portQuery.filters]

msorens · 2019-09-17T14:12:49Z

components/automate-ui/src/app/pages/+compliance/+reporting/reporting.component.spec.ts

@@ -68,6 +68,18 @@ describe('ReportingComponent', () => {
    expect(qe).not.toBeNull();
  });

+  it('convertMomentToDateWithoutTimezone', () => {


For consistency should be a verb-phrase rather than a noun, perhaps for this test:

Suggested change

it('convertMomentToDateWithoutTimezone', () => {

it('maps hour correctly without timezone', () => {

and for the next test, looks like with timezone...?

msorens · 2019-09-17T14:22:24Z

components/automate-ui/src/app/pages/+compliance/+reporting/reporting.component.spec.ts

@@ -68,6 +68,18 @@ describe('ReportingComponent', () => {
    expect(qe).not.toBeNull();
  });

+  it('convertMomentToDateWithoutTimezone', () => {


These new tests are, I understand, specifically to cover the bug. But consider expanding them with a few other key values, typically around boundaries. There's a test helper using in spec-helpers.ts, that makes it easy to test all equivalence classes:

using([ [23, 'an hour before end of day'], [0, 'exactly end of day'], [1, 'an hour after end of day'] ], function (hour: number, description: string) { it(`maps hour correctly without timezone for ${description}`, () => { const dateBefore = moment(`20191023-${hour}00`, 'YYYYMMDD-HHMM'); const dateAfter = component.convertMomentToDateWithoutTimezone(dateBefore); expect(dateBefore.hour()).toEqual(dateAfter.getHours()); }); });

Thank you. The using block is a nice tool.

msorens · 2019-09-17T14:25:53Z

components/automate-ui/src/app/pages/+compliance/+reporting/reporting.component.ts

@@ -253,7 +253,7 @@ export class ReportingComponent implements OnInit, OnDestroy {
    this.downloadOptsVisible = false;

    const reportQuery = this.reportQuery.getReportQuery();
-    const filename = `${moment(reportQuery.endDate).format('YYYY-M-D')}.${format}`;
+    const filename = `${reportQuery.endDate.format('YYYY-M-D')}.${format}`;


Suggest constant width for easy filename sorting at the command line:

Suggested change

const filename = `${reportQuery.endDate.format('YYYY-M-D')}.${format}`;

const filename = `${reportQuery.endDate.format('YYYY-MM-DD')}.${format}`;

I don't feel comfortable making this change with this PR.

msorens · 2019-09-17T14:53:10Z

components/automate-ui/src/app/pages/+compliance/shared/reporting/stats.service.ts

-          second: now.get('second')
-        }).utc().endOf('day');
-
+      const value = reportQuery.endDate.clone().utc().endOf('day');


Pretty sure you do not need the clone() there as you are evaluating a functional sequence.

That is what I thought from my original PR, but the endOf('day') function changes the original object. https://momentjs.com/docs/#/manipulating/end-of/

vjeffrey

pulled the change down locally and ensure the correct date was selectable for my timezone as well as those across the pond

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

Lance Finfrock added 2 commits September 16, 2019 14:10

Fixing time zone problems

999723c

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

Adding tests

dafa588

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

lancewf commented Sep 16, 2019

View reviewed changes

Lance Finfrock added 2 commits September 16, 2019 15:29

Adding tests

0f75ae0

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

fixing broken test

2424591

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

lancewf commented Sep 16, 2019

View reviewed changes

lancewf added automate-ui bug 🐛 Something isn't working ui labels Sep 16, 2019

lancewf marked this pull request as ready for review September 16, 2019 23:40

lancewf requested a review from a team September 16, 2019 23:40

msorens approved these changes Sep 17, 2019

View reviewed changes

vjeffrey approved these changes Sep 17, 2019

View reviewed changes

Refactor

a65e261

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>

lancewf merged commit f74a640 into master Sep 17, 2019

chef-expeditor bot deleted the lancewf/compliance_reporting_fix_timezone branch September 17, 2019 17:08

lancewf mentioned this pull request Sep 17, 2019

Fixing wrong trend graph date with time zone + UTC #1638

Merged

2 tasks

SEAjamieD mentioned this pull request Jul 2, 2020

Jamie/aus utc bug #4030

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing time zone problems #1624

Fixing time zone problems #1624

lancewf commented Sep 16, 2019 •

edited

Loading

lancewf Sep 16, 2019

lancewf Sep 16, 2019

lancewf Sep 16, 2019

lancewf Sep 16, 2019

lancewf Sep 16, 2019 •

edited

Loading

msorens Sep 17, 2019

msorens Sep 17, 2019

msorens Sep 17, 2019

lancewf Sep 17, 2019

msorens Sep 17, 2019

lancewf Sep 17, 2019

msorens Sep 17, 2019

lancewf Sep 17, 2019

vjeffrey left a comment

	filters: Object.assign([], reportQuery.filters)
	filters: [...portQuery.filters]

	it('convertMomentToDateWithoutTimezone', () => {
	it('maps hour correctly without timezone', () => {

	const filename = `${reportQuery.endDate.format('YYYY-M-D')}.${format}`;
	const filename = `${reportQuery.endDate.format('YYYY-MM-DD')}.${format}`;

Fixing time zone problems #1624

Fixing time zone problems #1624

Conversation

lancewf commented Sep 16, 2019 • edited Loading

🔩 Description: What code changed, and why?

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

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

lancewf Sep 16, 2019 • edited Loading

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

vjeffrey left a comment

Choose a reason for hiding this comment

lancewf commented Sep 16, 2019 •

edited

Loading

lancewf Sep 16, 2019 •

edited

Loading