Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jkmarx/add tools list landing page #3122

Merged
merged 23 commits into from Nov 29, 2018
Merged

Conversation

jkmarx
Copy link
Member

@jkmarx jkmarx commented Nov 28, 2018

Ref #2680

  • Added tools list
    Note: Just getting the components in and will focus on styling once all the pieces are in place.

screen shot 2018-11-28 at 2 17 40 pm

@jkmarx jkmarx self-assigned this Nov 28, 2018
@jkmarx jkmarx added this to the Release 1.6.8 milestone Nov 28, 2018
@codecov
Copy link

codecov bot commented Nov 28, 2018

Codecov Report

Merging #3122 into develop will decrease coverage by 3.09%.
The diff coverage is 97.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #3122     +/-   ##
==========================================
- Coverage    64.15%   61.06%   -3.1%     
==========================================
  Files          435      429      -6     
  Lines        30698    27483   -3215     
  Branches      1293     1272     -21     
==========================================
- Hits         19694    16782   -2912     
+ Misses       11003    10701    -302     
+ Partials         1        0      -1
Impacted Files Coverage Δ
refinery/ui/source/js/home/directives/tool-list.js 100% <100%> (ø) ⬆️
refinery/tool_manager/tests.py 99.77% <100%> (-0.17%) ⬇️
...ry/ui/source/js/home/services/tool-list-service.js 100% <100%> (ø) ⬆️
refinery/tool_manager/serializers.py 100% <100%> (ø) ⬆️
refinery/tool_manager/views.py 92.3% <100%> (-2.38%) ⬇️
refinery/ui/source/js/home/ctrls/tool-list-ctrl.js 90.9% <90.9%> (+7.57%) ⬆️
refinery/core/views.py 47.62% <0%> (-3.55%) ⬇️
refinery/core/models.py 78.29% <0%> (-3.39%) ⬇️
refinery/ui/source/js/file-browser/ctrls/ctrl.js 30.95% <0%> (-3.05%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66a3996...a9ed7ad. Read the comment docs.

@@ -1,24 +0,0 @@
(function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

unused

Copy link
Member

@scottx611x scottx611x left a comment

Choose a reason for hiding this comment

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

Looks good. One question about ToolDefinitionSerializer.get_workflow

def get_workflow(self, tool):
if tool.workflow:
return tool.workflow.uuid
return tool.workflow
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the 2nd return tool.workflow always be Nonehere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, am I being redundant?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a little. How about something like

def get_workflow(self, tool):
    return tool.workflow.uuid if tool.is_workflow() else None

or

def get_workflow(self, tool):
    if tool.is_visualization():
        return None  # Vis Tools have no associated Workflows
    return tool.workflow.uuid

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how that improves anything.

Copy link
Member

Choose a reason for hiding this comment

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

It's good to be explicit if we're going to be returning a None. Otherwise, someone could be left wondering why we're returning tool.workflow after already running the if tool.workflow.

Another path could be something like:

def get_workflow(self, tool):
    if tool.workflow is not None:  # Vis Tools have no associated Workflows
        return tool.workflow.uuid

@jkmarx jkmarx merged commit 3a2cb79 into develop Nov 29, 2018
@jkmarx jkmarx deleted the jkmarx/add-tools-list-landing-page branch November 29, 2018 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants