Issue#48 support field validation constraint #49

crsanti · 2017-03-14T12:13:55Z

This closes #48;

…nc and async functions. Remove unnecessary declaration of functions when adding field validations in sample react/01 SignupForm

…finitelyTyped#15140)

…igned validations. Added test to spy functions for multiple validations added to a field. Updated samples.

brauliodiez · 2017-03-14T12:25:31Z

lib/src/baseFormValidation.ts

    }
  }

+  private addFieldValidationFunctions(fields: { [key: string]: FieldValidationConstraint[] }) {


Should this be addFieldsValidationFunctions ?

To follow SRP could it be better to implement something like (I don't like the naming I'm using):

private addFieldsValidationFunctions(...) { for(let field in fields) { addFieldValidationFunctions(...) } } private addFieldValidationsFunction() { const fieldValidationConstraints = fields[field]; if (fieldValidationConstraints instanceof Array) { addFieldValdationFunction(...) } } (...) }

nasdan · 2017-03-14T12:27:16Z

lib/lcformvalidation.d.ts

+
+export interface FieldValidationConstraint extends Object {
+  validator: FieldValidationFunction;
+  trigger?: { [key: string]: boolean },


You are declaring an interface ValidationFilter below. Do you have to type trigger with this interface?

brauliodiez · 2017-03-14T12:28:23Z

samples/react/01 SignupForm/src/components/sampleSignupForm/validations/signupFormValidation.ts

  return Promise.resolve(fieldValidationResult);
 }

+const validationConstraints: ValidationConstraints = {


Should we name this: SignupFormValidationConstraints?

nasdan · 2017-03-14T12:34:19Z

lib/src/baseFormValidation.ts

+    return this;
+  }
+
+  validateField(vm: any, key: string, value: any, filter?: ValidationFilter): Promise<FieldValidationResult> {


We need to be consistent when naming trigger or filter. Select one of this and use it always.

I think validationTrigger is better name because you know when fire validations. What do you think?

nasdan · 2017-03-14T12:41:52Z

lib/lcformvalidation.d.ts

 interface FormValidation {
  validateField(vm: any, key: string, value: any, filter?: any): Promise<FieldValidationResult>;
  validateForm(vm: any): Promise<FormValidationResult>;
  isValidationInProgress(): boolean;


Type => validateField(vm: any, key: string, value: any, filter?: ValidationFilter): Promise;

brauliodiez · 2017-03-15T16:22:15Z

lib/src/fieldValidationEventFilter.ts

-  private propertyMatched(item: any, property: any, globalFilter: any): boolean {
-    return (globalFilter.hasOwnProperty(property) &&
-      globalFilter[property] == item.filter[property]);
+  private matchsAnyFilter(fieldValidation: FieldValidation, filters: ValidationFilters) {


matches?

I think matchs is "cerillas"?

nasdan · 2017-03-15T16:41:56Z

lib/src/baseFormValidation.ts

+    }
+  }
+
+  private parseFormValidations(validationFunctions: FormValidationFunction[]) {


I prefer order methods like we are using:

if (global && global instanceof Array) { this.parseFormValidations(global); } if (fields && typeof fields === 'object') { this.parseAllFieldsValidations(fields); } ... private parseFormValidations(validationFunctions: FormValidationFunction[]) { ..... } private parseAllFieldsValidations(fields: { [key: string]: FieldValidationConstraint[] }) { ..... } private parseFieldValidations(constraint: string, fieldValidationConstraints: FieldValidationConstraint[]) { ... }

So, you don't have to scroll down to search what are doing for example parseFormValidations

nasdan · 2017-03-15T16:53:56Z

lib/src/baseFormValidation.ts

-    return this.validationEngine.validateSingleField(vm, key, value, filter);
+  private addFieldValidation(constraint: string, validationConstraint: FieldValidationConstraint): FormValidation {
+    this.validationEngine.addFieldValidation(
+      constraint,


what is this constraint?

Should we rename to field?

nasdan · 2017-03-15T16:56:13Z

lib/src/baseFormValidation.ts

  }

-  private addFormValidationFunctions(validationFunctions: FormValidationFunction[]) {
+  private parseFieldValidations(constraint: string, fieldValidationConstraints: FieldValidationConstraint[]) {


here same as below

nasdan · 2017-03-15T17:00:39Z

lib/src/entities.ts

+  (value: any, vm: any, customParams: any): ValidationResult;
+}
+
+export interface AsyncFieldValidationFunction {


Why do we need defined SyncValidationFunction and AsyncFieldValidationFunction if we define interface like ValidationResult?

We don't have enough if we only define this?

export interface FieldValidationFunction { (value: any, vm: any, customParams: any): ValidationResult; }

nasdan · 2017-03-15T17:10:10Z

lib/src/spec/validationEngineValidateForm.spec.ts


        //Assert
-        expect(promise).to.eventually.be.rejected.notify(done);
+        expect(promise).to.eventually.be.rejected.and.notify(done);


What do you earn with and?

Just expressivity... I expect the promise to eventually be rejected and notify Chai's "done" method. The promise should call "done" when ends, same as test that does not use chai-as-promised.

nasdan · 2017-03-15T17:19:08Z

lib/src/validationEngine.ts

+  isValidationInProgress(): boolean;
  validateForm(vm: any): Promise<FormValidationResult>;
-  triggerFieldValidation(vm: any, key: string, value: any, filter?: any): Promise<FieldValidationResult>;
+  triggerFieldValidation(vm: any, key: string, value: any, filter?: ValidationFilters, customParams?: Object): Promise<FieldValidationResult>;


Rename to validateField?

There is a method called validateSingleField that is called by triggerFieldValidation. Could be a mess rename it validateField (that calls validateSingleField)?

And if we rename like:

triggerFieldValidation ===> validateFiled
validateSingleField ===> triggerFieldValidation

Should we be consistent with fire and trigger?? validateSingleField uses internally validationDispatcher.fireSingleFieldValidations.

I like your proposal. Should triggerFieldValidation be triggerFieldValidations?

Yes, you are right, we have to be consistent. I like fireFieldValidations. What do you think?

nasdan · 2017-03-15T17:19:53Z

lib/src/validationEngine.ts

  }

-  triggerFieldValidation(vm: any, key: string, value: any, filter: any = consts.defaultFilter): Promise<FieldValidationResult> {
+  triggerFieldValidation(vm: any, key: string, value: any, filter: ValidationFilters = consts.defaultFilter): Promise<FieldValidationResult> {


We need to add here customParams?: Object parameter

customParams should be added when you declare your field validation constraint. It is supplied by the validationDispatcher when fires the validation function. Should it be supplied by the action / whatever needs the validation result in runtime?

Ok, sorry but I mean, you have declared triggerFieldValidation as triggerFieldValidation(vm: any, key: string, value: any, filter?: ValidationFilters, customParams?: Object). So we have to remove from declaration.

See here

My bad... that's true! I'll check that.

nasdan · 2017-03-15T17:24:13Z

lib/src/validationsDispatcher.ts

  fireAllFieldsValidations(
-    vm: Object,
+    vm: any,
+    vmKeys: string[],


Why do you need to pass vmKeys as parameter instead of create vmKeys inside this method?

That's because your vm could have more keys/value that keys which needs to be validated, for instance, suppose we have an entity with 12 members which only 5 needs validation. We need an array of fields with validations to iterate (and that is stored by ValidationEngine). I just figured out when i removed the field-vm-mapping object.

Should I rename it to fieldsWithValidations or something like that?

Ok, thanks. It was a bit confusing for me when I read vm and then vmKeys.

Maybe some rename like: vmKeysWithValidation, vmKeysToValidate, fieldsToValidate?

fieldsToValidate sounds really good, I'll change it. 👍

Added some missing semicolon.

nasdan · 2017-03-16T17:46:20Z

lib/src/baseFormValidation.ts

+    if (fieldValidationConstraints instanceof Array) {
+      fieldValidationConstraints.forEach((fieldValidationConstraint) => {
+        if (fieldValidationConstraint && typeof fieldValidationConstraint === 'object') {
+          this.addFieldValidation(constraint, fieldValidationConstraint);


Rename to field

nasdan · 2017-03-16T17:46:37Z

lib/src/entities.ts

+  (value: any, vm: any, customParams: any): Promise<FieldValidationResult> |ValidationResult;
+}
+
+export interface AsyncFieldValidationFunction {


Remove this interface

nasdan · 2017-03-16T17:47:26Z

lib/src/entities.ts


-export interface ValidationConstraints extends Object {
+export interface FieldValidationFunction {
+  (value: any, vm: any, customParams: any): Promise<FieldValidationResult> |ValidationResult;


Change method type to only return ValidationResult

crsanti added 9 commits March 10, 2017 12:02

Updated typings

c23edfe

Removed addFieldValidationAsync and unified its behavior to handle sy…

afe30c5

…nc and async functions. Remove unnecessary declaration of functions when adding field validations in sample react/01 SignupForm

WIP

383f086

WIP

ffa208c

Updated implementation to use an array of FieldValidationConstraints.

51a15f4

Fixed version for @types/core-js. See [this issue](DefinitelyTyped/De…

ee2b9d7

…finitelyTyped#15140)

WIP examples

32a1aba

Fixed Sample 01.

839fc7d

Reimplemented vmKeys in ValidationEngine to loop only fields with ass…

57cd0dd

…igned validations. Added test to spy functions for multiple validations added to a field. Updated samples.

crsanti added the implementation label Mar 14, 2017

crsanti self-assigned this Mar 14, 2017

crsanti requested review from brauliodiez, nasdan and JaimeSalas March 14, 2017 12:13

brauliodiez reviewed Mar 14, 2017

View reviewed changes

Corrected some typings.

a1c1b25

nasdan reviewed Mar 14, 2017

View reviewed changes

brauliodiez reviewed Mar 14, 2017

View reviewed changes

nasdan reviewed Mar 14, 2017

View reviewed changes

crsanti added 7 commits March 14, 2017 15:57

Renamed constraints in samples.

a2177ff

Replaced trigger with filter.

abbba9a

Renamed eventFilter -> eventFilters

39b50f5

WIP

78de057

Removed entitiesMapper

d46af05

Removed reference to entityMapper

206482c

Fixed tests with PhahtomJS (missing Promise)

99cf986

brauliodiez reviewed Mar 15, 2017

View reviewed changes

nasdan reviewed Mar 15, 2017

View reviewed changes

crsanti added 4 commits March 16, 2017 09:45

Renamed some methods and parameters.

0ec9d97

Added some missing semicolon.

Improve some samples

391b56d

Removed unnecessary entity and renamed some vars.

a023482

Reordered methods

151d8da

brauliodiez merged commit 8820a24 into master Mar 16, 2017

brauliodiez deleted the issue#48_support_field_validation_constraint branch March 16, 2017 15:33

nasdan reviewed Mar 16, 2017

View reviewed changes

Issue#48 support field validation constraint #49

Issue#48 support field validation constraint #49

Uh oh!

Conversation

crsanti commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasdan Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasdan Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasdan Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crsanti Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crsanti Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasdan Mar 14, 2017 •

edited

Loading

nasdan Mar 15, 2017 •

edited

Loading

nasdan Mar 15, 2017 •

edited

Loading

crsanti Mar 15, 2017 •

edited

Loading

crsanti Mar 15, 2017 •

edited

Loading