Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed to propagate formarray validation status #263

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fixed to propagate formarray validation status #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
907eb61
Fixed to propagate formarray validation status
anschm Jun 20, 2022
078edef
Set emit value to true if form is disabled and enabled to emit valida…
anschm Jun 24, 2022
96afb30
Merge branch 'master' into fix/formarray_length_validation
anschm Jun 27, 2022
8eaf179
Clear formarray
anschm Jun 27, 2022
9fd91e8
Created e2e test for error validation after enbable and disable form
anschm Jun 27, 2022
964cd3c
Merge branch 'master' into fix/formarray_length_validation
anschm Jul 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 90 additions & 66 deletions cypress/e2e/app.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ context(`EJawa demo`, () => {
},
crewMembers: {
required: true,
crewMembers: {
minimumCrewMemberCount: 2
},
},
wingCount: {
required: true,
Expand All @@ -194,80 +197,42 @@ context(`EJawa demo`, () => {

DOM.form.elements.vehicleForm.addCrewMemberButton.click();

if (id === 'old') {
DOM.form.errors.should($el => {
expect(extractErrors($el)).to.eql({
vehicleProduct: {
spaceship: {
color: {
required: true,
},
crewMembers: {
crewMembers: [
{
firstName: {
required: true,
},
lastName: {
required: true,
},
},
],
},
wingCount: {
required: true,
},
DOM.form.errors.should($el => {
expect(extractErrors($el)).to.eql({
vehicleProduct: {
spaceship: {
color: {
required: true,
},
},
title: {
required: true,
},
imageUrl: {
required: true,
},
price: {
required: true,
},
});
});
} else {
DOM.form.errors.should($el => {
expect(extractErrors($el)).to.eql({
vehicleProduct: {
spaceship: {
color: {
required: true,
},
crewMembers: {
crewMembers: {
crewMembers: {
minimumCrewMemberCount: 2,
0: {
firstName: {
required: true,
},
lastName: {
required: true,
},
minimumCrewMemberCount: 2,
0: {
firstName: {
required: true,
},
lastName: {
required: true,
},
},
},
wingCount: {
required: true,
},
},
wingCount: {
required: true,
},
},
title: {
required: true,
},
imageUrl: {
required: true,
},
price: {
required: true,
},
});
},
title: {
required: true,
},
imageUrl: {
required: true,
},
price: {
required: true,
},
});
}
});

DOM.form.elements.selectListingTypeByType(ListingType.DROID);

Expand Down Expand Up @@ -378,6 +343,65 @@ context(`EJawa demo`, () => {
});
});
});

it(`should display the (nested) errors from the form after enable/disable`, () => {
DOM.createNewButton.click();

DOM.form.elements.selectListingTypeByType(ListingType.VEHICLE);

DOM.form.elements.vehicleForm.selectVehicleTypeByType(VehicleType.SPACESHIP);

DOM.form.elements.vehicleForm.addCrewMemberButton.click();

const errorsToExpect = {
vehicleProduct: {
spaceship: {
color: {
required: true,
},
crewMembers: {
crewMembers: {
minimumCrewMemberCount: 2,
0: {
firstName: {
required: true,
},
lastName: {
required: true,
},
},
},
},
wingCount: {
required: true,
},
},
},
title: {
required: true,
},
imageUrl: {
required: true,
},
price: {
required: true,
},
};

DOM.form.errors.should($el => {
expect(extractErrors($el)).to.eql(errorsToExpect);
});

DOM.readonlyToggle.click();

DOM.form.errors.should('not.exist');

DOM.readonlyToggle.click();

DOM.form.errors.should($el => {
expect(extractErrors($el)).to.eql(errorsToExpect);
});
});
});
});
});
5 changes: 3 additions & 2 deletions projects/ngx-sub-form/src/lib/create-form.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,13 @@ export function createForm<ControlInterface, FormInterface>(
),
setDisabledState$: setDisabledState$.pipe(
tap((shouldDisable: boolean) => {
shouldDisable ? formGroup.disable({ emitEvent: false }) : formGroup.enable({ emitEvent: false });
// We have to emit to update and validate the value and propagate it to the parent
shouldDisable ? formGroup.disable({ emitEvent: true }) : formGroup.enable({ emitEvent: true });
}),
),
updateValue$: updateValueAndValidity$.pipe(
tap(() => {
formGroup.updateValueAndValidity({ emitEvent: false });
formGroup.updateValueAndValidity({ emitEvent: true });
}),
),
bindTouched$: combineLatest([componentHooks.registerOnTouched$, options.touched$ ?? EMPTY]).pipe(
Expand Down
57 changes: 37 additions & 20 deletions projects/ngx-sub-form/src/lib/deprecated/ngx-sub-form.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,20 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont
}
}

let value = undefined;
if (control && filterControl(control, key, false)) {
value = {
...mapControl(control, key)
};
}

if (values.length > 0 && values.some(x => !isNullOrUndefined(x))) {
controls[key] = values;
value = {
...value,
...values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what the above is supposed to do.

values is of type MapValue[]
value is of type MapValue | undefined

So when you do ...value it'll explose the MapValue object and when you do ...values it'll explode an array of map value which is different from value.

Can you please specifiy the type on let value I think that'd help to clarify what's the intent and potentially TS would error here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about to go away as well (the deprecated folder entirely).

It's been there for long enough. I know that having the fix here may be nice but I don't think it's necessary tbh so if that becomes a pain to maintain that bit of code I'd say don't bother and lets just make the fix for the latest version ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't spend to much investigation into the old implementation because they will be removed. This works into my fork and project. I think we should not care about this.

}
}
controls[key] = value;
} else if (control && filterControl(control, key, false)) {
controls[key] = mapControl(control, key);
}
Expand Down Expand Up @@ -287,28 +298,33 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont

private handleFormArrayControls(obj: any) {
Object.entries(obj).forEach(([key, value]) => {
if (this.formGroup.get(key) instanceof UntypedFormArray && Array.isArray(value)) {
if (
this.formGroup.get(key) instanceof UntypedFormArray &&
Array.isArray(value)
) {
const formArray: UntypedFormArray = this.formGroup.get(key) as UntypedFormArray;

// instead of creating a new array every time and push a new FormControl
// we just remove or add what is necessary so that:
// - it is as efficient as possible and do not create unnecessary FormControl every time
// - validators are not destroyed/created again and eventually fire again for no reason
while (formArray.length > value.length) {
formArray.removeAt(formArray.length - 1);
}

for (let i = formArray.length; i < value.length; i++) {
if (this.formIsFormWithArrayControls()) {
formArray.insert(i, this.createFormArrayControl(key as ArrayPropertyKey<FormInterface>, value[i]));
} else {
formArray.insert(i, new UntypedFormControl(value[i]));
}
}
formArray.clear();
this.addAdditionalObjects(formArray, value, key);
}
});
}

private addAdditionalObjects(
formArray: UntypedFormArray,
value: Array<any>,
key: string
) {
for (let i = formArray.length; i < value.length; i++) {
const control = this.formIsFormWithArrayControls()
? this.createFormArrayControl(
key as ArrayPropertyKey<FormInterface>,
value[i]
)
: new UntypedFormControl(value[i]);
formArray.insert(i, control);
}
}

private formIsFormWithArrayControls(): this is NgxFormWithArrayControls<FormInterface> {
return typeof (this as unknown as NgxFormWithArrayControls<FormInterface>).createFormArrayControl === 'function';
}
Expand Down Expand Up @@ -423,10 +439,11 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont
return;
}

// We have to emit to update and validate the value and propagate it to the parent
if (shouldDisable) {
this.formGroup.disable({ emitEvent: false });
this.formGroup.disable({ emitEvent: true });
} else {
this.formGroup.enable({ emitEvent: false });
this.formGroup.enable({ emitEvent: true });
}
}
}
Expand Down
9 changes: 1 addition & 8 deletions projects/ngx-sub-form/src/lib/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,7 @@ export const handleFormArrays = <FormInterface>(
return;
}

// instead of creating a new array every time and push a new FormControl
// we just remove or add what is necessary so that:
// - it is as efficient as possible and do not create unnecessary FormControl every time
// - validators are not destroyed/created again and eventually fire again for no reason
while (control.length > value.length) {
control.removeAt(control.length - 1);
}

control.clear();
for (let i = control.length; i < value.length; i++) {
const newControl = createFormArrayControl(key as ArrayPropertyKey<FormInterface>, value[i]);
if (control.disabled) {
Expand Down
8 changes: 6 additions & 2 deletions src/app/app.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { NgModule } from '@angular/core';
import { registerLocaleData } from '@angular/common';
import { LOCALE_ID, NgModule } from '@angular/core';
import { BrowserModule } from '@angular/platform-browser';
import { BrowserAnimationsModule } from '@angular/platform-browser/animations';
import { RouterModule } from '@angular/router';
import { AppComponent } from './app.component';
import { SharedModule } from './shared/shared.module';
import localeDe from '@angular/common/locales/de';

registerLocaleData(localeDe, 'de');

@NgModule({
declarations: [AppComponent],
Expand All @@ -25,7 +29,7 @@ import { SharedModule } from './shared/shared.module';
),
SharedModule,
],
providers: [],
providers: [{provide: LOCALE_ID, useValue: 'de' }],
bootstrap: [AppComponent],
})
export class AppModule {}