Bug #12881

[Composer] Drag and drop of tools into graph view doesn't work

Added by Peter Amstutz almost 4 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
01/08/2018
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Subtasks

Task #12898: Review 12881-fix-dndResolvedPeter Amstutz

History

#1 Updated by Peter Amstutz almost 4 years ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz almost 4 years ago

  • Status changed from In Progress to New

#3 Updated by Peter Amstutz almost 4 years ago

  • Assigned To set to Peter Amstutz

#5 Updated by Peter Amstutz almost 4 years ago

  • Status changed from New to In Progress

#6 Updated by Peter Amstutz almost 4 years ago

12881-fix-dnd partial fix, works for adding files in the same repository. Needs to handle drag-and-drop when the file is from a different repository from the target.

#7 Updated by Lucas Di Pentima almost 4 years ago

  • File src/app/core/panels/my-apps-panel/arvados-apps-panel.service.ts:L155 - missing colon
  • Tested adding a cwl file into the visual editor of another cwl file, both on the same, and on different repos (using a couple of repos of my own @ 4xphq) and it seems to have worked ok on both cases.
  • Adding a cwl file into itself seems to be allowed (it displays an error message, but maybe it would be better to not allow it entirely?)

#8 Updated by Peter Amstutz almost 4 years ago

  • Target version changed from 2018-01-17 Sprint to 2018-01-31 Sprint

#9 Updated by Peter Amstutz over 3 years ago

Lucas Di Pentima wrote:

  • File src/app/core/panels/my-apps-panel/arvados-apps-panel.service.ts:L155 - missing colon

Fixed.

  • Tested adding a cwl file into the visual editor of another cwl file, both on the same, and on different repos (using a couple of repos of my own @ 4xphq) and it seems to have worked ok on both cases.

This should now report an error.

  • Adding a cwl file into itself seems to be allowed (it displays an error message, but maybe it would be better to not allow it entirely?)

This should now report an error.

12881-fix-dnd @ commit:dccb9a63eea9196524a9cb1a53306a69f4b8197f

#10 Updated by Peter Amstutz over 3 years ago

Oops, try 12881-fix-dnd @ commit:e40623aafba27928a7f7c1624da0c801ed617a24 this updates the commit hash for the js-git dependency.

#11 Updated by Lucas Di Pentima over 3 years ago

Drag & drop behavior works as intended on my end, so I just have some questions about readability/code formatting:

  • File src/app/workflow-editor/graph-editor/graph-editor/workflow-graph-editor.component.ts
    • Lines 336-349’s indentation is a little off making the code less readable
    • else block at lines 342-346 seems to be superfluous.
    • Line 336: if isLocal is always true, there are at least 3 places where it's being checked for truthfulness that may get simpler.

Other than the above, this LGTM.

#12 Updated by Peter Amstutz over 3 years ago

Lucas Di Pentima wrote:

Drag & drop behavior works as intended on my end, so I just have some questions about readability/code formatting:

  • File src/app/workflow-editor/graph-editor/graph-editor/workflow-graph-editor.component.ts
    • Lines 336-349’s indentation is a little off making the code less readable

Ah yea, there's a tabs vs spaces thing going on there.

  • else block at lines 342-346 seems to be superfluous.
  • Line 336: if isLocal is always true, there are at least 3 places where it's being checked for truthfulness that may get simpler.

The isLocal flag is meaningful in the original code, which we eventually want to merge with, so removing that code would mainly have the effect of creating future merge conflicts.

Other than the above, this LGTM.

Thanks.

#13 Updated by Peter Amstutz over 3 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF