NCJ UX with Mode selection by t-anjam · Pull Request #598 · Azure/BatchExplorer

t-anjam · 2017-08-08T18:28:20Z

I added mode selection option to the Market feature in BatchLabs. Now users can create pools, job + old pool, job + new pool submissions.

msftclas · 2017-08-08T18:28:23Z

@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-08T18:30:05Z

src/client/python-process/python-executable.ts

 */
 export function tryMultiplePythons(paths: string[]): Promise<string> {
-    let promise: Promise<string> = Promise.reject(null);
+    //let promise: Promise<string> = Promise.reject(null);


yeah just remove that line

timotheeguerin · 2017-08-08T18:31:25Z

python/controllers/ncj_controller.py

@@ -7,13 +7,13 @@

 @app.procedure("submit-ncj-job")
 def submitNcjJob(request: JsonRpcRequest, template, parameters):


was like that before but should follow python coding style so submit_ncj_job

timotheeguerin · 2017-08-08T18:31:28Z

python/controllers/ncj_controller.py

    return {"what": "it works"}
+
+@app.procedure("get-ncj-pool")
+def getNcjPool(request: JsonRpcRequest, template, parameters):


can you call that expand_ncj_pool instead(and change at id also to be expand-ncj-pool

timotheeguerin · 2017-08-08T18:32:11Z

app/components/market/application-action/choose-action.component.ts


-    constructor(private templateService: NcjTemplateService, private route: ActivatedRoute) { }
+    constructor(private templateService: NcjTemplateService, private route: ActivatedRoute) {
+


remove spaces as it was before

timotheeguerin · 2017-08-08T18:33:32Z

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

+             this.mode_state = "None";
+             this.form = new FormGroup({a: new FormControl()});
+    }
+    private ngOnInit() {


ngOnInit is public

timotheeguerin · 2017-08-08T18:33:53Z

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

-
-    public ngOnInit() {
+        private templateService: NcjTemplateService) {
+             this.mode_state = "None";


Use an enum for this

timotheeguerin · 2017-08-08T18:34:21Z

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

-    public ngOnInit() {
+        private templateService: NcjTemplateService) {
+             this.mode_state = "None";
+             this.form = new FormGroup({a: new FormControl()});


timotheeguerin · 2017-08-08T18:35:04Z

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

-
-    public createForms() {
-        let parameterKeys = Object.keys(this.jobTemplate.parameters);
+    private createForms() {


make private methods start with _

timotheeguerin · 2017-08-08T18:38:18Z

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

+        for (let key of poolParameters) {
+            if ("defaultValue" in this.poolTemplate.parameters[key]) {
+                const defaultValue = String(this.poolTemplate.parameters[key].defaultValue);
+                fg[key] = new FormControl(defaultValue);


I think you should separate the pool and job properties in 2 different file group.

Form should be

this.jobParams = new FormGroup(jobFg) this.poolParams = new FormGroup(poolFg) this.form = formBuilder.group({pool: this.poolParams, job: this.jobParams});

let's wait till UX converges to a final design

This will be much better that merging all params togther and then trying to figure out which ones are belong to the pool and which ones to the job

@t-anjam , how will the UX changes affect this? Is there something that we need to discuss in the UX which will affect our decision to keep the job and pool sections of the template separate?

Was thinking about corner cases if we decided to add special inputs for the different modes, then which category would they fall into? job or pool? So I tried to kept it general by keeping them all in one form object.

all inputs should belong to one or the other. and in the case it doesn't we can add another root param

timotheeguerin · 2017-08-08T18:40:08Z

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

+                            <bl-file-group-picker [formControlName]="param" [label]="param" class="form-element" [hint]="jobTemplate.parameters[param].metadata.description">
+                            </bl-file-group-picker>
+                        </div>
+                        <div *ngSwitchCase="'file-in-file-group'">


There is way to much duplicates here. You should probably create another component called ncj-form-parameter to which you give the param type and it will choose the right value

let's wait till UX converges to a final design

what do you mean? you are going to need that either way. This looks way to hacky this way

I was thinking it is better to wait in refactoring code until a finalized design is agreed upon. I predict there might be more changes to the UX in the nearby future.

yes but this is completely independent from whatever the UI will looks like. You just make a component that display the right input dependending on the type. This is something we should have either way(even if there was no duplicate).

agree, will change

timotheeguerin · 2017-08-08T19:36:07Z

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

+                        </div>
+                        <div *ngSwitchCase="'dropdown'">
+                            {{param}}
+                            <select [formControlName]="param">


use a md-select here instead of simple select

paselem

Added several comments for review. Please take a look.

paselem · 2017-08-10T05:37:00Z

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

-                    log.warn("Invalid form param. Not valid json.", params.formParams as any);
-                }
-            }
+            this.templateService.getTemplates(this.applicationId, this.actionId).subscribe((templates) => {


minor: why is this 'templates' instead of 'template' (note plural vs. singular')?

Didn't name this method, but it's because it returns both job and pool template.

paselem · 2017-08-10T05:38:15Z

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

+        if (this.poolTemplate && this.poolTemplate.parameters){
+            poolParameters = Object.keys(this.poolTemplate.parameters);
+        }
        let fg = {};


not a big fan of abbreviated variables. I assume we can expand to formGroup?

paselem · 2017-08-10T05:38:41Z

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

-        for (let key of parameterKeys) {
+
+        for (let key of jobParameters) {
            if ("defaultValue" in this.jobTemplate.parameters[key]) {


shouldn't "defaultValue" be a const?

this is actually would not be very useful because we do object.defaultValue after to access it.

changed to if (this.jobTemplate.parameters[key].defaultValue)

paselem · 2017-08-10T05:41:36Z

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

+        for (let key of poolParameters) {
+            if ("defaultValue" in this.poolTemplate.parameters[key]) {
+                const defaultValue = String(this.poolTemplate.parameters[key].defaultValue);
+                fg[key] = new FormControl(defaultValue);


@t-anjam , how will the UX changes affect this? Is there something that we need to discuss in the UX which will affect our decision to keep the job and pool sections of the template separate?

paselem · 2017-08-10T05:44:08Z

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

+        delete expandedPoolTemplate.id;
+        this.jobTemplate.job.properties.poolInfo = {
+            autoPoolSpecification: {
+            autoPoolIdPrefix: "jobname",


should the prefix be the actual job name, not the string "jobname"?

Also, consider what kinds of constraints you will run into here. Batch has limitations on what values are allowed for this. It would be worth taking a look at the Batch docs to know what issues you may run into: https://docs.microsoft.com/en-us/rest/api/batchservice/

paselem · 2017-08-10T05:58:18Z

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

+        </bl-complex-form>
+    </div>
+
+    <div *ngIf="mode_state == 'PoolNJob'">


Inconsistent spacing around operators throught the file. In some cases you are using

'a==b'

and in others

'a == b'

There is also an inconsistent use of ' and ".

Is the linter not catching these?

paselem · 2017-08-10T05:59:53Z

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

    </div>
+
+    <button *ngIf="mode_state!='None'" style="width:100px;background-color:#3e81b0;" (click) = "onSubmit()" type="button"  md-raised-button>Submit</button>
+


nitpick: lots of empty lines here.

paselem · 2017-08-10T06:00:13Z

python/controllers/ncj_controller.py


 @app.procedure("submit-ncj-job")
 def submitNcjJob(request: JsonRpcRequest, template, parameters):
+    print("Got template", template)


remove print statement.

paselem · 2017-08-10T06:00:36Z

python/controllers/ncj_controller.py

    print("Job is", job)
    client.job.add(job)
-    return job_json
+    return {"what": "it works"}


Was this just for debugging? Change it back?

paselem · 2017-08-10T06:00:42Z

python/controllers/ncj_controller.py

    job_json = client.job.expand_template(template, parameters)
-    print("Job json", job_json)
    job = client.job.jobparameter_from_json(job_json)
    print("Job is", job)


remove print statement.

timotheeguerin · 2017-08-10T15:08:37Z

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

-import * as inflection from "inflection";
 import "./submit-market-application.scss";

-enum NcjParameterExtendedType {


This enum was there before you should keep it and add more types

timotheeguerin · 2017-08-10T15:08:53Z

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

-}
-
-class NcjParameterWrapper {
-    public type: NcjParameterExtendedType;


Same this class should still be here instead of the function

paselem · 2017-08-10T16:30:02Z

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

-        return obs;
    }

-    private _redirectToJob(data) {


As mentioned above, I think we need to pass a parameter back in here.

Regarding this API, I think passing in a 'data' object is a poor design. The method only requires the jobId, and should not need to know about the structure of some random data structure. If the data structure ever changes, then this method would need to get updated with it, which doesn't make sense. The interface should be kept as clean and simple as possible.

I agree, I didn't create this function. I removed the data parameter for now till I know where to find the jobID/poolID for all three modes.

paselem · 2017-08-10T16:32:10Z

This is a pretty significant change, and I did not see any unit tests for it. I would be wary about letting this go into master without having unit tests come along with it.

Also, our naming schema for feature branches follows the pattern feature/. Please follow that convention in the future. It helps us identify what kind of work is within the branch.

timotheeguerin · 2017-08-18T20:43:57Z

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

+    private _propagateChange: (value: any) => void = null;
+
+    constructor() {
+        console.log("Contructor:", this.parameter, this.type);


this._sub = this.parameterValue.valueChanges.subscribe((value) => { if(propage) {propagate(value)}; });

timotheeguerin · 2017-08-21T17:31:32Z

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

-                            </md-input-container>
-                        </div>
+                    <div *ngFor="let param of poolParametersWrapper">
+                        <bl-parameter-input [formControlName]="param.id" [parameter]="param" [type]="types" [parameterValues]="poolParams.value"> </bl-parameter-input>


Don't pass this

timotheeguerin · 2017-08-28T15:37:47Z

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


    public ngOnInit(): void {
-        this.parameterValue.setValue(this.parameter.defaultValue);
+        // do not need


remove this one if not using

timotheeguerin · 2017-08-28T15:39:40Z

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

-import { NcjParameterRawType } from "app/models";
+import { BatchApplication, NcjParameterRawType } from "app/models";
+import { StorageService } from "app/services";
+import { Subject } from "rxjs";


move the library import in the first group above. i.e @angular/... rxjs ...

timotheeguerin · 2017-08-28T15:42:17Z

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

+
+    describe("text parameter type", () => {
+        let inputEl: DebugElement;
+        const initialInput: string = "initial input value";


don't define the type here is is a bit too verbose. Only do the type if it is not obvious, on a class attribute or is not assigned right away(Like debugElement above).

Same for all variables below being number or string

removed types for simple variables

timotheeguerin · 2017-08-28T15:45:30Z

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

+        });
+    });
+
+    describe("fileinput parameter type", () => {


for this type you alos want to check the containerId given match what's in the formValues

paselem

Overall, looks pretty good! I have added a few comments to the review to consider. Thanks!

paselem · 2017-08-28T18:19:49Z

app/components/market/submit/parameter-input.html

+            <md-option *ngFor="let opt of parameter.allowedValues" [value]="opt"> {{ opt }} </md-option>
+        </md-select>
+    </div>
+    <div *ngSwitchCase="NcjParameterExtendedType.int">


Should we add a specific type for 'bool'?

I think bool scenario can be covered under dropdown case with two options: ['true', 'false'].

paselem · 2017-08-28T18:21:20Z

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

+export enum Modes {
+    None,
+    NewPoolAndJob,
+    OldPoolAndJob,


Nitpick: "ExistingPoolAndJob" instead of "OldPoolAndJob". Thoughts?

paselem · 2017-08-28T18:22:59Z

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

+        }
+        this.jobParametersWrapper = jobTempWrapper;
+        const poolParameters = this.poolTemplate.parameters;
+        const poolTempWrapper: any[] = [];


shouldn't this be 'let' instead of 'const' if you are going to append to it later?

https://medium.com/javascript-scene/javascript-es6-var-let-or-const-ba58b8dcde75

Since the array variable is not being reassigned then it's safe to use const. Even though its contents are changing.

paselem · 2017-08-28T18:26:17Z

test/app/components/market/submit/market-application.model.spec.ts

+            defaultValue: 4,
+        });
+        expect(parameter.type).toBe(NcjParameterExtendedType.int);
+    });


May want to add a unit test here to check for the value as well (since this is an int which is a different case than what you have above for string).

Same goes for cases below.

…jPart2

timotheeguerin · 2017-08-28T19:25:32Z

app/components/market/submit/parameter-input.html

+<div [ngSwitch]="parameter.type">
+    <div *ngSwitchCase="NcjParameterExtendedType.string">
+        <md-input-container class="form-element">
+            <input mdInput type="text" name="parameter.name" [formControl]="parameterValue" [placeholder]="parameter.name">


remove the name= attribute here

ascobie · 2017-08-28T23:01:10Z

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

+    }
+
+    public getContainerFromFileGroup(fileGroup: string) {
+        return fileGroup && `fgrp-${fileGroup}`;


This is in StorageService.ncjFileGroupPrefix

We could move it into Constants if you like. We shouldn't hard code this anywhere.

t-anjam added 14 commits August 1, 2017 11:48

added icon to 2nd page

c5a9db8

added mode selection to form

2190917

created buttons for mode options

ed40fc1

comment out unnecessary code

639ffe2

combined job parameters and pool paramerters in some form group

43bb8f6

uncomment

a873108

form is set to form group

16cb33f

created different forms for different modes

497e783

forms works with different input types but no RPC

496824b

fix dependsOn

f88adeb

market submissions works for all three modes

fa7e908

added tlint changes

763b5f2

added tlint changes

7eeedff

rpc error handling

03d40e9

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

t-anjam requested review from paselem and timotheeguerin August 8, 2017 18:28

timotheeguerin suggested changes Aug 8, 2017

View reviewed changes

timotheeguerin reviewed Aug 8, 2017

View reviewed changes

t-anjam added 2 commits August 9, 2017 11:38

code review changes

ab44549

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

1754785

t-anjam force-pushed the ncjPart2 branch from 97da1e5 to 1754785 Compare August 10, 2017 00:05

paselem suggested changes Aug 10, 2017

View reviewed changes

timotheeguerin reviewed Aug 10, 2017

View reviewed changes

paselem reviewed Aug 10, 2017

View reviewed changes

timotheeguerin modified the milestone: NCJ Aug 10, 2017

timotheeguerin reviewed Aug 18, 2017

View reviewed changes

t-anjam added 2 commits August 18, 2017 17:42

completed parameter input component

1008d3e

spacing

45b644f

timotheeguerin reviewed Aug 21, 2017

View reviewed changes

timotheeguerin and others added 8 commits August 21, 2017 10:48

changes

17c67d5

Missing file

17de835

remove :

2344476

added empty unit test for parameter input component

5ae9cff

remove console logs

ebef609

added unit test for parameter input component

bdc488a

line spacing

36e6e85

added unit tests for market-application model

eda40f8

timotheeguerin reviewed Aug 28, 2017

View reviewed changes

timotheeguerin and others added 2 commits August 28, 2017 08:49

Merge branch 'ncj' into ncjPart2

fada518

code review changes

e274b79

paselem suggested changes Aug 28, 2017

View reviewed changes

t-anjam added 2 commits August 28, 2017 12:03

more code review changes

039c644

Merge branch 'ncjPart2' of https://github.com/Azure/BatchLabs into nc…

f17588e

…jPart2

timotheeguerin reviewed Aug 28, 2017

View reviewed changes

more code review changes

5f166c6

timotheeguerin approved these changes Aug 28, 2017

View reviewed changes

updated unit test name

6ed087c

paselem approved these changes Aug 28, 2017

View reviewed changes

fix multiple imports

a21040d

t-anjam merged commit 581ff55 into ncj Aug 28, 2017

ascobie reviewed Aug 28, 2017

View reviewed changes

timotheeguerin deleted the ncjPart2 branch September 5, 2017 23:46

		@@ -7,13 +7,13 @@

		@app.procedure("submit-ncj-job")
		def submitNcjJob(request: JsonRpcRequest, template, parameters):


		constructor(private templateService: NcjTemplateService, private route: ActivatedRoute) { }
		constructor(private templateService: NcjTemplateService, private route: ActivatedRoute) {

		</div>

		<button *ngIf="mode_state!='None'" style="width:100px;background-color:#3e81b0;" (click) = "onSubmit()" type="button" md-raised-button>Submit</button>

Conversation

t-anjam commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

t-anjam Aug 9, 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!

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

t-anjam Aug 9, 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!

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

paselem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

t-anjam commented Aug 8, 2017 •

edited

Loading

t-anjam Aug 9, 2017 •

edited

Loading

timotheeguerin Aug 8, 2017 •

edited

Loading

t-anjam Aug 9, 2017 •

edited

Loading

timotheeguerin Aug 8, 2017 •

edited

Loading