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

Scottx611x/tool manager views cleanup #2991

Merged
merged 13 commits into from Sep 10, 2018

Conversation

scottx611x
Copy link
Member

@scottx611x scottx611x commented Sep 6, 2018

I came across this split (extra logic for handling data_set_uuid vs. dataSetUuid) the other day, and figured it would be good to take care of.

I've also gone ahead and refactored to remove some redundancies and unclear code placement.

@ghost ghost assigned scottx611x Sep 6, 2018
@ghost ghost added the review label Sep 6, 2018
@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #2991 into develop will decrease coverage by 0.06%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2991      +/-   ##
===========================================
- Coverage    59.52%   59.45%   -0.07%     
===========================================
  Files          433      433              
  Lines        27286    27212      -74     
  Branches      1274     1274              
===========================================
- Hits         16241    16180      -61     
+ Misses       11045    11032      -13
Impacted Files Coverage Δ
.../ui/source/js/commons/services/tool-definitions.js 100% <ø> (ø) ⬆️
...rce/js/tool-launch/services/tool-select-service.js 85.71% <ø> (ø) ⬆️
refinery/tool_manager/urls.py 100% <100%> (ø) ⬆️
refinery/tool_manager/tests.py 99.82% <100%> (ø) ⬆️
refinery/tool_manager/views.py 92.66% <93.75%> (+4.13%) ⬆️

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 137a461...5b8bfd5. Read the comment docs.

WorkflowTool.objects.filter(dataset=self.data_set)
)
tool_type = self.request.query_params.get("tool_type")
if tool_type is None:
Copy link
Member

Choose a reason for hiding this comment

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

@scottx611x Any user can hit this endpoint and grab analyses or visualizations with a dataSetUuid. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is just the get_queryset method. The permission logic happens in the list()

Actually looking into if I can use DRF permission_classes here instead.

@@ -14,7 +14,7 @@
query: {
method: 'GET',
isArray: true,
params: { dataSetUuid: '' }
params: { data_set_uuid: '' }
Copy link
Member

Choose a reason for hiding this comment

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

Sure, since it's a url query. Worth noting for json it's camelCase styling. https://google.github.io/styleguide/jsoncstyleguide.xml?showone=Property_Name_Format#Property_Name_Format

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wasn't sold on one way vs. the other, but I can change the backend code to work with dataSetUuid

@scottx611x scottx611x merged commit c96b8f5 into develop Sep 10, 2018
@ghost ghost removed the review label Sep 10, 2018
@scottx611x scottx611x deleted the scottx611x/tool_manager_views_cleanup branch September 10, 2018 19:54
@scottx611x scottx611x added this to Backlog in Scott O. Tasks via automation Oct 24, 2018
@scottx611x scottx611x moved this from Backlog to Done in Scott O. Tasks Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants