[ACS-5857] Moment to Date-Fns Migration for adfMomentDate Pipe. #8924

SheenaMalhotra182 · 2023-09-22T15:22:12Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines

Tests for the changes have been added (for bug fixes / features)

Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

Bugfix

Feature

Code style update (formatting, local variables)

Refactoring (no functional changes, no api changes)

Build related changes

Documentation

Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)
Moment Library is being used in adfMomentDate Pipe.

What is the new behaviour?
This PR includes migration of 'lib/core/src/lib/pipes/moment-date.pipe.ts' into date-fns

Created a new Pipe (adfDate) which uses date-fns library instead of Moment.
Replaced occurrences of adfMomentDate with adfDate in 'DateWidgetComponent' and 'DateCloudWidgetComponent' and accordingly changed the ts and spec files
Replaced occurrences of adfMomentDate with adfDate in 'DateTimeWidgetComponent' and accordingly changed the ts and spec files

Does this PR introduce a breaking change? (check one with "x")

Yes

No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:
https://alfresco.atlassian.net/browse/ACS-5857

There's one unit test case which needed the format of test data to be changed in order to validate it with the default format =>
https://github.com/Alfresco/alfresco-ng2-components/pull/8924/files#diff-b52520351ccf61f2557086298ce2a0d0e574463d39d0fa921085bcc2af1c1760R1116.
Rest all unit tests and e2es work fine without needing any changes.

DenysVuika · 2023-09-25T08:09:59Z

lib/core/src/lib/form/services/date-format-translation.service.spec.ts

+    let service: DateFormatTranslationService;
+
+    beforeEach(() => {
+        TestBed.configureTestingModule({


you don't need this block, the service does not use Translation or Root, and it has providedIn: root, so you don't need setting the "providers" section too

DenysVuika · 2023-09-25T08:11:29Z

lib/core/src/lib/form/services/date-format-translation.service.ts

+        a: 'A'
+    };
+
+    convertMomentToDateFnsFormat(dateDisplayFormat: string): string {


provide code comments for the public methods so that it gets to the generated documentation

added comments

DenysVuika · 2023-09-25T08:13:17Z

lib/core/src/lib/pipes/date.pipe.ts

+import { DateFormatTranslationService } from '../form/services/date-format-translation.service';
+
+@Pipe({ name: 'adfDate' })
+export class DatePipe implements PipeTransform {


the naming of class is going to clash with Angular DatePipe: https://angular.io/api/common/DatePipe
please use AdfDatePipe as the pipe name suggests

lib/core/src/lib/form/components/widgets/core/form-field.model.ts

DenysVuika · 2023-09-25T08:27:47Z

lib/core/src/lib/form/components/widgets/core/form-field.model.spec.ts

@@ -19,9 +19,16 @@ import moment from 'moment';
 import { FormFieldTypes } from './form-field-types';
 import { FormFieldModel } from './form-field.model';
 import { FormModel } from './form.model';
+import { DateFormatTranslationService } from '../../../public-api';


incorrect import path

fixed import path

lib/core/src/lib/form/components/widgets/core/form-field.model.ts

DenysVuika · 2023-09-25T08:42:47Z

lib/core/src/lib/form/services/date-format-translation.service.ts

+        return format(date, this.convertMomentToDateFnsFormat(dateFormat));
+    }
+
+    parse(value: string, dateFormat: string, date = new Date()): Date {


this function is confusing, why it takes date parameter and why it sets it to new date() ?

if there are no cases when you pass the date here, please use "new Date()" inside the "parse" call instead

updated to call new Date() inside parse itself.

DenysVuika · 2023-09-25T08:45:14Z

lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.ts

-        const date = moment(newDateValue, this.field.dateDisplayFormat, true);
-        if (date.isValid()) {
-            this.field.value = date.format(this.field.dateDisplayFormat);
+        const date = this.dateFormatTranslationService.parse(newDateValue, this.field.dateDisplayFormat, new Date());


why do you pass new date there as third param? what is the reason for that?

DenysVuika · 2023-09-25T08:57:22Z

lib/core/src/lib/form/components/widgets/date/date.widget.html

@@ -1,6 +1,6 @@
 <div class="{{field.className}}" id="data-widget" [class.adf-invalid]="!field.isValid && isTouched()">
    <mat-form-field class="adf-date-widget" [hideRequiredMarker]="true">
-        <label class="adf-label" [attr.for]="field.id">{{field.name | translate }} ({{field.dateDisplayFormat}})<span class="adf-asterisk"
+        <label class="adf-label" [attr.for]="field.id">{{field.name | translate }} ({{dateFormatTranslationService.convertDateFnsToMomentFormat(field.dateDisplayFormat)}})<span class="adf-asterisk"


please extract to a separate function so that template does not deal with services conversion directly:

formatLabel(field: FormField): string { const displayName = this.translationService.instant(field.name); const displayFormat = this.dateFormatTranslationService.convertDateFnsToMomentFormat(field.dateDisplayFormat); return `${displayName} (${displayFormat})`; }

then in HTML:

<label>{formatLabel(field)}</label>

also provide unit tests for the formatLabel function as it becomes easier

implemented

DenysVuika · 2023-09-25T08:58:11Z

lib/core/src/lib/pipes/date.pipe.spec.ts

+
+    beforeEach(() => {
+        TestBed.configureTestingModule({
+            imports: [TranslateModule.forRoot(), CoreTestingModule],


check if you need those imports, probably not

removed unnecessary imports

DenysVuika · 2023-09-25T08:58:21Z

lib/core/src/lib/pipes/date.pipe.spec.ts

+import { CoreTestingModule } from '../testing/core.testing.module';
+import { TranslateModule } from '@ngx-translate/core';
+import { DatePipe } from './date.pipe';
+import { DateFormatTranslationService } from '../form/public-api';


incorrect import path

DenysVuika · 2023-09-25T08:58:56Z

lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.html

@@ -1,11 +1,11 @@
 <div class="{{field.className}}" id="data-widget" [class.adf-invalid]="!field.isValid && isTouched()" [class.adf-left-label-input-container]="field.leftLabels">
    <div *ngIf="field.leftLabels">
-        <label class="adf-label adf-left-label" [attr.for]="field.id">{{field.name | translate }} ({{field.dateDisplayFormat}})<span class="adf-asterisk"
+        <label class="adf-label adf-left-label" [attr.for]="field.id">{{field.name | translate }} ({{dateFormatTranslationService.convertDateFnsToMomentFormat(field.dateDisplayFormat)}})<span class="adf-asterisk"


this needs to be extracted to a function, UI templates should not deal with services directly, see https://github.com/Alfresco/alfresco-ng2-components/pull/8924/files#r1335592062

DenysVuika · 2023-09-25T08:59:02Z

lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.html

            *ngIf="isRequired()">*</span></label>
     </div>
    <div>
        <mat-form-field class="adf-date-widget" [class.adf-left-label-input-datepicker]="field.leftLabels" [hideRequiredMarker]="true">
-            <label class="adf-label" *ngIf="!field.leftLabels" [attr.for]="field.id">{{field.name | translate }} ({{field.dateDisplayFormat}})<span class="adf-asterisk"
+            <label class="adf-label" *ngIf="!field.leftLabels" [attr.for]="field.id">{{field.name | translate }} ({{dateFormatTranslationService.convertDateFnsToMomentFormat(field.dateDisplayFormat)}})<span class="adf-asterisk"


this needs to be extracted to a function, UI templates should not deal with services directly, see https://github.com/Alfresco/alfresco-ng2-components/pull/8924/files#r1335592062

DenysVuika · 2023-09-25T09:02:28Z

lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.ts

                }
            } else {
                if (this.field.minValue) {
-                    this.minDate = moment(this.field.minValue, DATE_FORMAT_CLOUD);
+                    this.minDate = this.dateFormatTranslationService.format(new Date(this.field.minValue), DATE_FORMAT_CLOUD);


why do you need translation if you already have new Date() and export const DATE_FORMAT_CLOUD = 'yyyy-MM-dd'; ? you can probably use datefns directly?

after dome changes now, DATE_FORMAT_CLOUD is 'YYYY-MM-DD' like before.
It also helps to maintain uniformity across all places if we use format or parse methods from DateFnsUtils.

DenysVuika · 2023-09-25T09:03:08Z

lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.ts


-        const momentDateAdapter = this.dateAdapter as MomentDateAdapter;
-        momentDateAdapter.overrideDisplayFormat = this.field.dateDisplayFormat;
+        this.dateFormatConfig.display.dateInput = this.field.dateDisplayFormat;

        if (this.field) {
            if (this.field.dynamicDateRangeSelection) {


review this block as you probably don't need format translation service as today is a Date object

DenysVuika · 2023-09-25T09:06:43Z

lib/core/src/lib/pipes/date.pipe.spec.ts

+    it('should return the input value when given an invalid date object', () => {
+        const inputDate = new Date('invalid');
+        const dateFormat = 'DD-MM-YYYY';
+        const expectedOutput = inputDate.toString();


your expected output should not be based on .toString() conversion, as whatever happens there the test will be just green. Add exact value it expects instead.

DenysVuika · 2023-09-25T09:11:34Z

lib/core/src/lib/form/components/widgets/core/form-field-validator.ts

@@ -583,3 +584,39 @@ export const FORM_FIELD_VALIDATORS = [
    new MinDateTimeFieldValidator(),
    new MaxDateTimeFieldValidator()
 ];
+
+export const momentToDateFnsMap = {


you are using this maps in 2 places, second is translation service; extract to a separate class and use same import in both cases

moved all logic to DateFnsUtils

DenysVuika · 2023-09-25T09:11:51Z

lib/core/src/lib/form/components/widgets/core/form-field-validator.ts

+    a: 'A'
+};
+
+export function convertMomentToDateFnsFormat(dateDisplayFormat: string): string {


provide documentation comments for public functions

added documentation

DenysVuika · 2023-09-25T09:12:38Z

lib/core/src/lib/form/services/date-format-translation.service.ts

+    providedIn: 'root'
+})
+export class DateFormatTranslationService {
+    private momentToDateFnsMap = {


you are using this maps in 2 places, extract to a separate class and use same import in both cases

you already have DateFnsUtils which can be a good place

moved all logic to DateFnsUtils

DenysVuika · 2023-09-25T09:14:04Z

lib/core/src/lib/pipes/date.pipe.spec.ts

+import { DatePipe } from './date.pipe';
+import { DateFormatTranslationService } from '../form/public-api';
+
+describe('DatePipe', () => {


please provide tests with different variations of the date format, i.e.

YYYY-MM-DD

YYYY-DD-MM

MM-DD-YY

added steps to check wth different date formats

DenysVuika · 2023-09-25T09:14:51Z

lib/core/src/lib/pipes/pipe.module.ts

@@ -36,6 +36,7 @@ import { MomentDateTimePipe } from './moment-datetime.pipe';
 import { FilterStringPipe } from './filter-string.pipe';
 import { FilterOutArrayObjectsByPropPipe } from './filter-out-every-object-by-prop.pipe';
 import { DateTimePipe } from './date-time.pipe';
+import { DatePipe } from './date.pipe';


should be AdfDatePipe not to clash with Angular DatePipe

DenysVuika · 2023-09-25T09:16:35Z

lib/core/src/lib/form/components/widgets/core/form-field-validator.ts

+    return dateDisplayFormat;
+}
+
+export function convertDateFnsToMomentFormat(dateDisplayFormat: string): string {


move all these functions to DateFnsUtils class to have a single place for all date/time code

moved all logic to DateFnsUtils

DenysVuika · 2023-09-25T09:23:27Z

lib/core/src/lib/form/components/widgets/core/form-field-validator.ts

+    return dateDisplayFormat;
+}
+
+export function formatUpdate(date: number|Date, dateFormat: string): string {


move to DateFnsUtils class and use better naming, like formatDate, it is already implied that some update is going to happen

DenysVuika · 2023-09-25T09:23:50Z

lib/core/src/lib/form/components/widgets/core/form-field-validator.ts

+    return format(date, convertMomentToDateFnsFormat(dateFormat));
+}
+
+export function parseUpdate(value: string, dateFormat: string, date = new Date()): Date {


move to DateFnsUtils class and use better naming, like parseDate, it is already implied that some update is going to happen

DenysVuika · 2023-09-25T09:27:10Z

lib/core/src/lib/form/services/date-format-translation.service.ts

+@Injectable({
+    providedIn: 'root'
+})
+export class DateFormatTranslationService {


This service has a lot of duplicated code with Field Validator funcitons.
you probably don't need this service at all, if you move all the field validator functions to DateFnsUtils, you can use those utils without this service

DenysVuika · 2023-10-05T17:10:43Z

lib/core/src/lib/form/components/widgets/date/date.widget.spec.ts

@@ -170,23 +171,23 @@ describe('DateWidgetComponent', () => {
                readOnly: 'false'
            });
            widget.field.isVisible = true;
-            widget.field.dateDisplayFormat = 'MM-DD-YYYY';


modified test data

DenysVuika · 2023-10-05T17:14:33Z

lib/core/src/lib/form/components/widgets/date/date.widget.spec.ts


            fixture.detectChanges();

            let dateElement: any = element.querySelector('#date-field-id');
            expect(dateElement.value).toContain('12-30-9999');

            widget.field.value = '5-6-2019 00:00';
-            widget.field.dateDisplayFormat = 'D-M-YYYY HH:mm';


this is data coming from the backend

DenysVuika · 2023-10-09T14:27:04Z

lib/core/src/lib/form/components/widgets/date/date.widget.ts


    private onDestroy$ = new Subject<boolean>();

    constructor(public formService: FormService,
-                private dateAdapter: DateAdapter<Moment>,
-                private userPreferencesService: UserPreferencesService) {
+                private dateAdapter: DateAdapter<DateFnsAdapter>,


incorrect parameter, should be the value type, not another adapter,
valid cases are DateAdapter<Moment | Date | string> etc

…te-fns methods

…dating

sonarqubecloud · 2023-10-09T18:55:55Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
4.7% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

DenysVuika · 2023-10-10T16:13:34Z

closing as too many issues

SheenaMalhotra182 requested a review from rbahirsheth September 22, 2023 15:34

DenysVuika reviewed Sep 25, 2023

View reviewed changes

lib/core/src/lib/form/components/widgets/core/form-field.model.ts Outdated Show resolved Hide resolved

DenysVuika reviewed Sep 25, 2023

View reviewed changes

lib/core/src/lib/form/components/widgets/core/form-field.model.ts Outdated Show resolved Hide resolved

DenysVuika reviewed Sep 25, 2023

View reviewed changes

SheenaMalhotra182 mentioned this pull request Sep 26, 2023

[APPS-2202] Added custom utils for syncing moment and date-fns formats #8934

Merged

DenysVuika added the A/N BDU label Sep 26, 2023

SheenaMalhotra182 added PR: draft do not merge 🙅🏻‍♂️ labels Sep 29, 2023

DenysVuika reviewed Oct 5, 2023

View reviewed changes

SheenaMalhotra182 added PR: draft do not review do not merge 🙅🏻‍♂️ labels Oct 9, 2023

SheenaMalhotra182 marked this pull request as draft October 9, 2023 13:45

DenysVuika reviewed Oct 9, 2023

View reviewed changes

SheenaMalhotra182 added 17 commits October 9, 2023 23:44

[ACS-5857] migrated adfMomentDate Pipe to date-fns equivalent

b388cb5

[ACS-5857] fixed date parsing and linting

9fc4c97

migrated adfMomentDate Pipe to date-fns equivalent and used custom da…

b94591e

…te-fns methods

[ACS-5857] fixed unit test for adding seconds

8c1e5c9

[ACS-5857] fixed imports

ec2d09a

[ACS-5857] linting fixes

621b05e

[ACS-5857] added unit tests for formatLabel

5a25ea8

[ACS-5857] fixed paths and minDate maxDate logic in date-time-widget

55baa94

[ACS-5857] linting fixes

d176691

[ACS-5857] e2e fixes - converted dateFormat back to moment while vali…

2dd0e0d

…dating

[ACS-5857] cleanup

6877adb

[ACS-5857] modified code to reduce duplication

61f8e12

[ACS-5857] fixed code smell by creating a method

b0b0ffb

[ACS-5857] fixed linting

0dffbe4

[ACS-5857] addressed review comments

6404481

[ACS-5857] addressed review comments

2cc6f6c

[ACS-5857] reverted moment migration for DateWidget and DateCloudWidget

27dbd03

SheenaMalhotra182 force-pushed the moment-migration-poc branch from cb8b9dd to 27dbd03 Compare October 9, 2023 18:22

SheenaMalhotra182 added 2 commits October 10, 2023 00:22

[ACS-5857] reverted date field back to moment in form-field-model.ts

f41a090

[ci:force]

4b8dd3e

DenysVuika closed this Oct 10, 2023

[ACS-5857] Moment to Date-Fns Migration for adfMomentDate Pipe. #8924

[ACS-5857] Moment to Date-Fns Migration for adfMomentDate Pipe. #8924

Conversation

SheenaMalhotra182 commented Sep 22, 2023 • 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

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

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

DenysVuika Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

DenysVuika Sep 25, 2023 • 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

sonarqubecloud bot commented Oct 9, 2023

DenysVuika commented Oct 10, 2023

SheenaMalhotra182 commented Sep 22, 2023 •

edited

Loading

DenysVuika Sep 25, 2023 •

edited

Loading

DenysVuika Sep 25, 2023 •

edited

Loading