fix UI defect RAC-4416 #549

kellylu2sym · 2017-11-08T03:52:06Z

BackGround :
when we create a workflow without task definition , rackhd will add api path to the taskName ,when you change the workflow and need save again , it will failed when validation of the task with taskName like "api/2.0/workflows/tasks****"
Solution :
code change in task-graph.js . when validate ,we use actually task name instead of the whole name witch contains api path
Test :
manual test
Jira:
https://rackhd.atlassian.net/browse/RAC-4416
Reviewer :
@anhou @panpan0000 @bbcyyb

houndci-bot · 2017-11-08T03:52:14Z

lib/task-graph.js

-                    return store.getTaskDefinition(taskData.taskName)
+                    var taskName = new String(taskData.taskName);
+                    var contains = taskName.indexOf("\/");
+                    if(contains != null){


Expected '!==' and instead saw '!='.

houndci-bot · 2017-11-08T03:52:14Z

lib/task-graph.js

@@ -360,7 +366,13 @@ function taskGraphFactory(
            assert.arrayOfObject(self.definition.tasks, 'Graph.tasks');
            return Promise.map(self.definition.tasks, function(taskData) {
                if (!_.has(taskData, 'taskDefinition')) {
-                    return store.getTaskDefinition(taskData.taskName)
+                    var taskName = new String(taskData.taskName);


Do not use String as a constructor.

houndci-bot · 2017-11-08T03:52:14Z

lib/task-graph.js

@@ -210,8 +210,14 @@ function taskGraphFactory(
            };

            if (taskData.taskName) {
+                var taskName = new String(taskData.taskName);
+                var contains = taskName.indexOf("\/");
+                if(contains != null){


Expected '!==' and instead saw '!='.

houndci-bot · 2017-11-08T03:52:14Z

lib/task-graph.js

@@ -210,8 +210,14 @@ function taskGraphFactory(
            };

            if (taskData.taskName) {
+                var taskName = new String(taskData.taskName);


Do not use String as a constructor.

anhou · 2017-11-08T09:04:13Z

lib/task-graph.js

@@ -208,10 +208,15 @@ function taskGraphFactory(
                waitingOn: taskData.waitOn,
                ignoreFailure: taskData.ignoreFailure
            };
-
-            if (taskData.taskName) {
+            if (taskData.taskName && typeof(taskData.taskName) !== "undefined") {


the condition never goto 'typeof(taskData.taskName) !== "undefined"' which after '&&' , right?

anhou · 2017-11-08T09:06:41Z

lib/task-graph.js

+            if (taskData.taskName && typeof(taskData.taskName) !== "undefined") {
+                var taskName = taskData.taskName;
+                var contains = taskName.indexOf("\/");
+                if(contains !== -1){


please add annotations for string operations

anhou · 2017-11-08T09:11:43Z

lib/task-graph.js

@@ -360,7 +365,15 @@ function taskGraphFactory(
            assert.arrayOfObject(self.definition.tasks, 'Graph.tasks');
            return Promise.map(self.definition.tasks, function(taskData) {
                if (!_.has(taskData, 'taskDefinition')) {
-                    return store.getTaskDefinition(taskData.taskName)
+                    var taskName = taskData.taskName;


duplicated code with above, please extract a function if possible

done, however this line is in the new function.

anhou · 2017-11-08T09:12:19Z

lib/task-graph.js

-            if (taskData.taskName) {
+            if (taskData.taskName && typeof(taskData.taskName) !== "undefined") {
+                var taskName = taskData.taskName;
+                var contains = taskName.indexOf("\/");


contains what, a descriptive name is better

panpan0000 · 2017-11-08T09:14:24Z

spec/lib/task-graph-spec.js

@@ -130,6 +130,13 @@ describe('Task Graph', function () {
                expect(graph).to.be.an.instanceof(TaskGraph);
            });
        });
+        it('should return this', function() {


please choose another description other than "should return this". it was used in line 127

panpan0000 · 2017-11-08T09:16:10Z

a silly question:
is it possible that there's different path but with same task name ?
ex:
api/workflows/tasks/wsman/Task.test
api/workflows/tasks/ucs/Task.test

houndci-bot · 2017-11-08T10:11:03Z

lib/task-graph.js

+                    taskName = nameArray[nameArray.length - 1];
+                }
+        return taskName;    
+    } 


Missing semicolon.

houndci-bot · 2017-11-08T10:11:03Z

lib/task-graph.js

@@ -229,7 +235,14 @@ function taskGraphFactory(
            return self;
        });
    };
-
+    var getTaskName = function(taskName){


'getTaskName' was used before it was defined.

houndci-bot · 2017-11-08T10:15:48Z

lib/task-graph.js

+            taskName = nameArray[nameArray.length - 1];
+        }
+        return taskName;    
+    } 


Missing semicolon.

houndci-bot · 2017-11-08T10:15:48Z

lib/task-graph.js

+            taskName = nameArray[nameArray.length - 1];
+        }
+        return taskName;    
+    } 


Missing semicolon.

anhou · 2017-11-09T02:32:14Z

lib/task-graph.js

+    var getTaskName = function(taskName){
+        // check if there is "/" in the task name ,if yes 
+        // it may have the api path before task name
+        var contains = taskName.indexOf("\/");


temporary variable name could be slashIndex, or slashOffset. I think this is your purpose, it's more straightforward than a verb 'contain', and it's more readable

anhou · 2017-11-09T02:43:40Z

lib/task-graph.js

-                    return store.getTaskDefinition(taskData.taskName)
+                    var taskName = taskData.taskName;
+                    if(taskName){
+                        taskName = getTaskName(taskName);


taskName in this function have two meanings. former taskName is taskName input, latter is true taskName, it's not so good for clean code. so you could naming former one as taskNameInput, the latter is taskName. Otherwise, it's a little werid that get 'taskName' from 'taskName'

panpan0000 · 2017-11-10T01:17:35Z

can you git commit --amend then git push again, to trigger the Concourse?

houndci-bot reviewed Nov 8, 2017

View reviewed changes

kellylu2sym force-pushed the UIbug branch 4 times, most recently from 92a354b to 50db145 Compare November 8, 2017 08:44

anhou reviewed Nov 8, 2017

View reviewed changes

panpan0000 reviewed Nov 8, 2017

View reviewed changes

kellylu2sym force-pushed the UIbug branch 2 times, most recently from baf0fcb to 5ac99e2 Compare November 8, 2017 09:27

anhou added the run-test label Nov 8, 2017

kellylu2sym force-pushed the UIbug branch from 5ac99e2 to 123af10 Compare November 8, 2017 10:10

houndci-bot reviewed Nov 8, 2017

View reviewed changes

kellylu2sym force-pushed the UIbug branch from 123af10 to ff269dc Compare November 8, 2017 10:15

houndci-bot reviewed Nov 8, 2017

View reviewed changes

lib/task-graph.js Outdated

taskName = nameArray[nameArray.length - 1];

}

return taskName;

}

Copy link

houndci-bot Nov 8, 2017

Choose a reason for hiding this comment

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

Missing semicolon.

houndci-bot reviewed Nov 8, 2017

View reviewed changes

lib/task-graph.js Outdated

taskName = nameArray[nameArray.length - 1];

}

return taskName;

}

Copy link

houndci-bot Nov 8, 2017

Choose a reason for hiding this comment

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

Missing semicolon.

kellylu2sym force-pushed the UIbug branch 2 times, most recently from bcb7197 to 45d0e72 Compare November 9, 2017 01:35

anhou reviewed Nov 9, 2017

View reviewed changes

kellylu2sym force-pushed the UIbug branch from 45d0e72 to 0ffdcd3 Compare November 9, 2017 02:41

anhou reviewed Nov 9, 2017

View reviewed changes

kellylu2sym force-pushed the UIbug branch from d373432 to 434c2c9 Compare November 9, 2017 03:35

kellylu2sym force-pushed the UIbug branch from 434c2c9 to 53115d5 Compare November 10, 2017 03:28

fix UI defect RAC-4416

9d5da55

kellylu2sym force-pushed the UIbug branch from 53115d5 to 9d5da55 Compare November 10, 2017 03:32

anhou approved these changes Nov 10, 2017

View reviewed changes

anhou merged commit b4e1757 into RackHD:master Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix UI defect RAC-4416 #549

fix UI defect RAC-4416 #549

kellylu2sym commented Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

anhou Nov 8, 2017

anhou Nov 8, 2017

anhou Nov 8, 2017

kellylu2sym Nov 9, 2017

anhou Nov 8, 2017

panpan0000 Nov 8, 2017 •

edited

Loading

panpan0000 commented Nov 8, 2017 •

edited

Loading

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

houndci-bot Nov 8, 2017

anhou Nov 9, 2017 •

edited

Loading

anhou Nov 9, 2017

panpan0000 commented Nov 10, 2017

fix UI defect RAC-4416 #549

fix UI defect RAC-4416 #549

Conversation

kellylu2sym commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panpan0000 Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

panpan0000 commented Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anhou Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panpan0000 commented Nov 10, 2017

panpan0000 Nov 8, 2017 •

edited

Loading

panpan0000 commented Nov 8, 2017 •

edited

Loading

anhou Nov 9, 2017 •

edited

Loading