Feature/ncj validation by t-anjam · Pull Request #644 · Azure/BatchExplorer

t-anjam · 2017-08-29T21:55:21Z

Added validation to the NCJ form. Able to validate for required, minValue, maxValue, minLength, and maxLength. Run button only turns on when form is valid. Also added tool tip to run button. Added unit tests.

msftclas · 2017-08-29T21:55:25Z

@t-anjam,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

timotheeguerin · 2017-08-29T22:34:25Z

app/components/market/submit/parameter-input.component.ts

+       return {
+            validJsonConfig: {
+                valid: false,
+                message: "FormControl is invalid",


Change the message here to contain what the error is(Look into the parameterValue.errors)

timotheeguerin · 2017-08-29T22:34:42Z

app/components/market/submit/parameter-input.component.ts

+       const valid = this.parameterValue.valid;
+       if (valid) { return null; }
+       return {
+            validJsonConfig: {


Also change this key

timotheeguerin · 2017-08-29T22:36:05Z

app/components/market/submit/submit-market-application.component.ts

+    }
+
+    public isFormValid() {
+        return (this.modeState ===  Modes.NewPoolAndJob && this.jobParams.valid && this.poolParams.valid) ||


you should just be able to have this.form.valid instead of this

Different mode will have different conditions for submit

timotheeguerin · 2017-08-29T22:38:02Z

app/components/market/submit/submit-market-application.component.ts

+        for (let key of templateParameters) {
+            if (template.parameters[key].defaultValue) {
+                const defaultValue = String(template.parameters[key].defaultValue);
+                templateFormGroup[key] = new FormControl(defaultValue, Validators.required );


don't create the control twice here, do just set the default value to null if needed

timotheeguerin · 2017-08-29T22:39:39Z

app/components/market/submit/submit-market-application.html

+    <div [title]="getToolTip()" class="submit-box" *ngIf="modeState !== Modes.None">
        <bl-server-error [error]="error"> </bl-server-error>
-        <bl-button [action]="submit" type="wide" class="submit-job" color="success">
+        <bl-button [disabled]="!isFormValid()" [action]="submit" type="wide" class="submit-job" color="success">


make the tooltip here instead of being on the outer div. also make it a getter public get submitTooltip() instead of a function

timotheeguerin · 2017-08-29T22:42:04Z

test/app/components/market/submit/parameter-input.component.spec.ts


+        it("should validate minimum value constraint", () => {
+            const input = 5;
+            testComponent.param = new NcjParameterWrapper("frameEnd", {


for those test add the param restriction in the beforeEach() so you have less duplicate and just include both restriction at the same time.

timotheeguerin · 2017-08-29T22:42:57Z

test/app/components/market/submit/parameter-input.component.spec.ts

            expect(inputEl.nativeElement.value).toBe(updatedInput);
        });
+
+        it("should validate minimum length constraint", () => {


You should also check for the error message

timotheeguerin · 2017-08-31T18:39:07Z

app/components/market/submit/parameter-input.component.ts

+        } else {
+            let messageText = "unknown error";
+            if (this.parameterValue.errors.minlength) {
+                messageText = "FormControl minLength error";


use the message provided don't rewrite it. Here you are losing information.

will add more detail to error message

timotheeguerin · 2017-08-31T22:19:14Z

app/components/market/submit/parameter-input.component.ts

+            let messageText = "unknown error";
+            if (this.parameterValue.errors.minlength) {
+                messageText = "FormControl minLength error:";
+                messageText = messageText + " minlength: " +  this.parameterValue.errors.minlength.requiredLength;


Save this.parameterValue.errors in a local variable so you don't have to repeat all that.
Also make the message look nicer like "Should be at least x characters`

And try to use string interpolation ${} instead of +

timotheeguerin · 2017-09-05T17:33:23Z

app/components/market/submit/parameter-input.component.ts

+            const error = this.parameterValue.errors;
+            if (error.minlength) {
+                const minLength = String(error.minlength.requiredLength);
+                messageText = `Should be atleast ${minLength} characters`;


at least and at most below

paselem · 2017-09-05T18:18:03Z

app/components/market/submit/parameter-input.component.ts

+                messageText = `Should be atmost ${maxLength} characters`;
+            } else if (error.min) {
+                const minValue = String(error.min.min);
+                messageText = `Should be greater than ${minValue}`;


Greater than or equal to

paselem · 2017-09-05T18:18:12Z

app/components/market/submit/parameter-input.component.ts

+                messageText = `Should be greater than ${minValue}`;
+            } else if (this.parameterValue.errors.max) {
+                const maxValue = String(error.max.max);
+                messageText = `Should be less than ${maxValue}`;


Less than or equal to

paselem · 2017-09-05T18:18:50Z

app/components/market/submit/submit-market-application.component.ts


+    public get submitToolTip(): string {
+        if (this.isFormValid()) {
+            return "Click to submit form";


Change to "Click to submit job"

This tooltip text will also be displayed when user is in pool only mode.

paselem · 2017-09-05T18:20:24Z

app/components/market/submit/parameter-input.component.ts

+        if (valid) {
+            return null;
+        } else {
+            let messageText = `unknown error`;


Nitpick: I noticed below you are using "" instead of ``. Any reason in particular?

Example:

return "Click to submit form";

should actually use `` only if using interpolation. Use "" otherwise

…/ncj-validation

t-anjam added 4 commits August 28, 2017 16:58

added validation logic and refactored

c29ae69

submit button only activates when form is valid

1618aed

added return type

456c649

added unit tests and completed form validation

daa5328

t-anjam requested review from ascobie, paselem and timotheeguerin August 29, 2017 21:55

msftclas added the cla-not-required label Aug 29, 2017

timotheeguerin reviewed Aug 29, 2017

View reviewed changes

timotheeguerin suggested changes Aug 29, 2017

View reviewed changes

t-anjam added 2 commits August 30, 2017 14:52

code review

f132f48

added unit test for validation

e7c8d24

timotheeguerin reviewed Aug 31, 2017

View reviewed changes

updated error messages

cef26e7

timotheeguerin reviewed Aug 31, 2017

View reviewed changes

made error message more user friendly

383dea2

timotheeguerin reviewed Sep 5, 2017

View reviewed changes

paselem reviewed Sep 5, 2017

View reviewed changes

made error message even more friendlier

f0cd6b7

timotheeguerin approved these changes Sep 6, 2017

View reviewed changes

t-anjam added 2 commits September 6, 2017 10:27

error message spelling

7b5eb8d

Merge branch 'ncj' of https://github.com/Azure/BatchLabs into feature…

adcfa8d

…/ncj-validation

t-anjam merged commit 39e84b5 into ncj Sep 6, 2017

timotheeguerin deleted the feature/ncj-validation branch September 6, 2017 17:42

Conversation

t-anjam commented Aug 29, 2017

Uh oh!

msftclas commented Aug 29, 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!

timotheeguerin Aug 29, 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!

timotheeguerin Aug 29, 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!

timotheeguerin Aug 29, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timotheeguerin Aug 29, 2017 •

edited

Loading

timotheeguerin Aug 29, 2017 •

edited

Loading

timotheeguerin Aug 29, 2017 •

edited

Loading