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

Jump to bottom

refactor(oneOf validator): Refactor oneOf validator #95

Open
wants to merge 1 commit into
base: feat/formgroup-validator-oneof
Choose a base branch
from
Open

refactor(oneOf validator): Refactor oneOf validator #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
191 changes: 119 additions & 72 deletions projects/ngx-sub-form/src/lib/ngx-sub-form.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ import {
ArrayPropertyValue,
NgxFormWithArrayControls,
OneOfValidatorRequiresMoreThanOneFieldError,
OneOfValidatorUnknownFieldError,
OneOfValidatorUnknownFieldError, TypedValidatorFn,
} from '../public_api';
import { Observable } from 'rxjs';
import { NgxSubFormValidators } from './one-of.validator';

interface Vehicle {
color?: string | null;
Expand Down Expand Up @@ -447,25 +448,29 @@ describe(`NgxSubFormComponent`, () => {
interface DroidForm {
assassinDroid: { type: 'Assassin' };
medicalDroid: { type: 'Medical' };
allianceRebel: { type: 'Rebel' };
allianceImperial: { type: 'Imperial' };
}

class DroidFormComponent extends NgxSubFormComponent<DroidForm> {
protected getFormControls() {
return {
assassinDroid: new FormControl(null),
medicalDroid: new FormControl(null),
allianceRebel: new FormControl(null),
allianceImperial: new FormControl(null),
};
}

public getFormGroupControlOptions(): FormGroupOptions<DroidForm> {
return {
validators: [this.ngxSubFormValidators.oneOf([['assassinDroid', 'medicalDroid']])],
validators: [NgxSubFormValidators.oneOf(['assassinDroid', 'medicalDroid'])],
};
}

// testing utility
public setValidatorOneOf(keysArray: (keyof DroidForm)[][]): void {
this.formGroup.setValidators([(this.ngxSubFormValidators.oneOf(keysArray) as unknown) as ValidatorFn]);
public setValidators(validators: TypedValidatorFn<DroidForm>[]): void {
this.formGroup.setValidators(validators as any);
}
}

Expand Down Expand Up @@ -504,95 +509,137 @@ describe(`NgxSubFormComponent`, () => {
});

describe('ngxSubFormValidators', () => {
it('oneOf should throw an error if no value or only one in the array', () => {
expect(() => droidFormComponent.setValidatorOneOf(undefined as any)).toThrow(
new OneOfValidatorRequiresMoreThanOneFieldError(),
);

expect(() => droidFormComponent.setValidatorOneOf([])).toThrow(
new OneOfValidatorRequiresMoreThanOneFieldError(),
);
describe('oneOf', () => {

expect(() => droidFormComponent.setValidatorOneOf([[]])).toThrow(
new OneOfValidatorRequiresMoreThanOneFieldError(),
);
it('should throw an error if no value or only one in the array', () => {
expect(() => NgxSubFormValidators.oneOf(undefined as any)).toThrow(
new OneOfValidatorRequiresMoreThanOneFieldError(),
);

expect(() => droidFormComponent.setValidatorOneOf([['assassinDroid']])).toThrow(
new OneOfValidatorRequiresMoreThanOneFieldError(),
);
expect(() => NgxSubFormValidators.oneOf([])).toThrow(
new OneOfValidatorRequiresMoreThanOneFieldError(),
);

expect(() => droidFormComponent.setValidatorOneOf([['assassinDroid', 'medicalDroid']])).not.toThrow();
});
expect(() => NgxSubFormValidators.oneOf(['assassinDroid'])).toThrow(
new OneOfValidatorRequiresMoreThanOneFieldError(),
);

it('oneOf should throw an error if there is an unknown key', () => {
droidFormComponent.setValidatorOneOf([['unknown 1' as any, 'unknown 2' as any]]);
expect(() => droidFormComponent.formGroup.updateValueAndValidity()).toThrow(
new OneOfValidatorUnknownFieldError('unknown 1'),
);
expect(() => NgxSubFormValidators.oneOf(['assassinDroid', 'medicalDroid'])).not.toThrow();
});

droidFormComponent.setValidatorOneOf([['assassinDroid', 'unknown 2' as any]]);
expect(() => droidFormComponent.formGroup.updateValueAndValidity()).toThrow(
new OneOfValidatorUnknownFieldError('unknown 2'),
);
});
it('should throw an error if there is an unknown key', () => {
droidFormComponent.setValidators([NgxSubFormValidators.oneOf<DroidForm>(['unknown 1' as any, 'unknown 2' as any])]);
expect(() => droidFormComponent.formGroup.updateValueAndValidity()).toThrow(
new OneOfValidatorUnknownFieldError('unknown 1'),
);

it('oneOf should return an object (representing the error) if all the values are null', (done: () => void) => {
const spyOnChange = jasmine.createSpy();
droidFormComponent.registerOnChange(spyOnChange);
droidFormComponent.setValidators([NgxSubFormValidators.oneOf<DroidForm>(['assassinDroid', 'unknown 2' as any])]);
expect(() => droidFormComponent.formGroup.updateValueAndValidity()).toThrow(
new OneOfValidatorUnknownFieldError('unknown 2'),
);
});

droidFormComponent.formGroup.patchValue({ assassinDroid: null, medicalDroid: null });
it('should return an object (representing the error) if all the values are null', (done: () => void) => {
const spyOnChange = jasmine.createSpy();
droidFormComponent.registerOnChange(spyOnChange);

setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({ formGroup: { oneOf: [['assassinDroid', 'medicalDroid']] } });
expect(droidFormComponent.formGroupErrors).toEqual({
formGroup: { oneOf: [['assassinDroid', 'medicalDroid']] },
});
droidFormComponent.formGroup.patchValue({ assassinDroid: null, medicalDroid: null });

droidFormComponent.formGroup.patchValue({
assassinDroid: { type: 'Assassin' },
medicalDroid: null,
setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({ formGroup: { oneOf: [] } });
expect(droidFormComponent.formGroupErrors).toEqual({
formGroup: { oneOf: [] },
});

droidFormComponent.formGroup.patchValue({
assassinDroid: { type: 'Assassin' },
medicalDroid: null,
});
setTimeout(() => {
expect(droidFormComponent.validate()).toEqual(null);
expect(droidFormComponent.formGroupErrors).toEqual(null);
done();
}, 0);
}, 0);
});
setTimeout(() => {
expect(droidFormComponent.validate()).toEqual(null);
expect(droidFormComponent.formGroupErrors).toEqual(null);
done();
}, 0);
}, 0);
});

it('oneOf should return an object (error) if more than one value are not [null or undefined]', (done: () => void) => {
const spyOnChange = jasmine.createSpy();
droidFormComponent.registerOnChange(spyOnChange);
it('should return an object (error) if more than one value are not [null or undefined]', (done: () => void) => {
const spyOnChange = jasmine.createSpy();
droidFormComponent.registerOnChange(spyOnChange);

droidFormComponent.formGroup.patchValue({ assassinDroid: null, medicalDroid: null });
droidFormComponent.formGroup.patchValue({ assassinDroid: null, medicalDroid: null });

setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({ formGroup: { oneOf: [['assassinDroid', 'medicalDroid']] } });
setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({ formGroup: { oneOf: [] } });

droidFormComponent.formGroup.patchValue({
assassinDroid: { type: 'Assassin' },
medicalDroid: { type: 'Medical' },
});

droidFormComponent.formGroup.patchValue({
assassinDroid: { type: 'Assassin' },
medicalDroid: { type: 'Medical' },
setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({
formGroup: { oneOf: ['assassinDroid', 'medicalDroid'] },
});

droidFormComponent.formGroup.patchValue({
assassinDroid: null,
medicalDroid: { type: 'Medical' },
});

setTimeout(() => {
expect(droidFormComponent.validate()).toEqual(null);

done();
}, 0);
}, 0);
}, 0);
});

setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({
formGroup: { oneOf: [['assassinDroid', 'medicalDroid']] },
});
it('should return an object (error) containing multiple named oneOf errors', (done: () => void) => {
const spyOnChange = jasmine.createSpy();
droidFormComponent.registerOnChange(spyOnChange);

droidFormComponent.formGroup.patchValue({
assassinDroid: null,
medicalDroid: { type: 'Medical' },
});
droidFormComponent.setValidators([
NgxSubFormValidators.oneOf(['assassinDroid', 'medicalDroid'], 'oneOfDroidType'),
NgxSubFormValidators.oneOf(['allianceRebel', 'allianceImperial'], 'oneOfAllianceType'),
]);

setTimeout(() => {
expect(droidFormComponent.validate()).toEqual(null);
droidFormComponent.formGroup.patchValue({ assassinDroid: null, medicalDroid: null, allianceRebel: null, allianceImperial: null });

done();
setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({ formGroup: { oneOfDroidType: [], oneOfAllianceType: [] } });

droidFormComponent.formGroup.patchValue({
assassinDroid: { type: 'Assassin' },
medicalDroid: { type: 'Medical' },
allianceRebel: { type: 'Rebel' },
allianceImperial: { type: 'Imperial' },
});

setTimeout(() => {
expect(droidFormComponent.validate()).toEqual({
formGroup: { oneOfDroidType: ['assassinDroid', 'medicalDroid'], oneOfAllianceType: ['allianceRebel', 'allianceImperial'] },
});

droidFormComponent.formGroup.patchValue({
assassinDroid: null,
medicalDroid: { type: 'Medical' },
allianceRebel: null,
allianceImperial: { type: 'Imperial' },
});

setTimeout(() => {
expect(droidFormComponent.validate()).toEqual(null);

done();
}, 0);
}, 0);
}, 0);
}, 0);
}, 0);
});
});
});
});
})
});
});

Expand Down
36 changes: 36 additions & 0 deletions projects/ngx-sub-form/src/lib/one-of.validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { AbstractControl } from '@angular/forms';
import { TypedFormGroup, TypedValidatorFn } from './ngx-sub-form.types';
import {
isNullOrUndefined,
OneOfValidatorRequiresMoreThanOneFieldError,
OneOfValidatorUnknownFieldError,
} from './ngx-sub-form-utils';

export namespace NgxSubFormValidators {

export function oneOf<FormInterface>(keysArray: Array<keyof FormInterface>, errorKey = 'oneOf'): TypedValidatorFn<FormInterface> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want to be able to change the error key?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nvm I just saw why:

NgxSubFormValidators.oneOf(['allianceRebel', 'allianceImperial'], 'oneOfAllianceType')

Few things here, the key should probably be at the beginning (just a preference) but something that might lead to weird results:

When using 2 oneOf (or more), as the key is optional it would override the previous result if someone forgets to pass it.

I think it'd probably be better to have the definition of oneOf to be :

interface ControlInterface {
  randomKey: any;
  polyTypeA: string;
  polyTypeB: string;
}

interface FormInterface {
  subTypeA1: string;
  subTypeA2: string;
  subTypeB1: string;
  subTypeB2: string;
}

export function oneOf<ControlInterfaceType, FormInterfaceType>(
  keys: Partial<Record<keyof ControlInterface, FormInterface>>
) {
  // ...
}

oneOf<ControlInterface, FormInterface>({
  polyTypeA: ['subTypeA1', 'subTypeA2'],
  polyTypeB: ['subTypeB1', 'subTypeB2'],
});

This way, we only need to use oneOf once, it'll loop over the properties of the form once too (not X times where X is the number of times you call oneOf)

Also, in the above we'd be using as key what's defined in the original object and mapping that to keys of the sub form

The above doesn't fully work though...

I get Type 'string[]' is not assignable to type 'FormInterface' but that should be doable :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent here, and what I’m optimising for is the common case. It strikes me as incredibly rare that you’d want to have multiple oneOfs in a single form, so I think we should not be making the api awkward for that one case.

I was also struggling with the problem of the error clobbering if you forgot to set the key name for differentiation, but I think you’ve got a great idea with having the argument be an object. However, I think we should support both options simultaneously, again to avoid an awkward interface for the extremely rare case. This could be handled by just checking if the first arg is array or object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another possibly simpler option is to have NgxSubFormValidators.oneOf and NgxSubFormValidators.oneOfMultiple

if (!keysArray || keysArray.length < 2) {
throw new OneOfValidatorRequiresMoreThanOneFieldError();
}

return (formGroup: TypedFormGroup<FormInterface>) => {

const notNullKeys: Array<keyof FormInterface> = keysArray.filter((key) => {
const control: AbstractControl | null = formGroup.get(key as string);

if (!control) {
throw new OneOfValidatorUnknownFieldError(key as string);
}

return !isNullOrUndefined(control.value);
});

if (notNullKeys.length === 1) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't take into account if multiple properties are defined.

From the example above, if assassinDroid AND medicalDroid are defined there should be an error because we don't know which one to pick up.

That error should be optional when I think about it, and turned on by default.
But if someone prefers not to reset to null a value when the component is destroyed to restore previous values if the user come back to it, then he might turn it off. In that case, if he has a select to choose a given type, within the remap function he might just pick the corresponding value, according to the select so he'd be fine.

But I believe that by default we should have an error if more than one value is defined with a oneOf condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mentioned this in the other PR but I’m not getting what you mean? This code works and passes the unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the example above, if assassinDroid AND medicalDroid are defined, notNullKeys.length is 2, not 1; there will be an error


return { [errorKey]: notNullKeys };
}
}

}