Jkmarx/remove unused igv stuff #2844

jkmarx · 2018-07-05T18:48:03Z

Feel free to test using swagger.

@hackdna Unsure the status of the remove link, but this api should be able to support it.

@scottx611x Unit test will come in the next pull request, if this works for Ilya. If it doesn't, then we'll just remove the Node V2 API.

codecov · 2018-07-06T16:04:20Z

Codecov Report

Merging #2844 into develop will decrease coverage by 0.77%.
The diff coverage is 36.36%.

@@             Coverage Diff             @@
##           develop    #2844      +/-   ##
===========================================
- Coverage    58.77%   57.99%   -0.78%     
===========================================
  Files          431      431              
  Lines        27054    26643     -411     
  Branches      1261     1261              
===========================================
- Hits         15901    15452     -449     
- Misses       11153    11191      +38

Impacted Files	Coverage Δ
refinery/data_set_manager/test_views.py	`100% <ø> (ø)`	⬆️
refinery/core/urls.py	`100% <ø> (ø)`	⬆️
...js/file-browser/ctrls/data-file-edit-modal-ctrl.js	`60.71% <0%> (-28.76%)`	⬇️
...ource/js/file-browser/services/nodes-v2-service.js	`100% <100%> (ø)`
...e/js/file-browser/ctrls/data-file-dropdown-ctrl.js	`78.94% <100%> (+9.71%)`	⬆️
refinery/core/views.py	`40.41% <22.72%> (-1.93%)`	⬇️
refinery/core/serializers.py	`78.04% <45.45%> (-6.47%)`	⬇️
refinery/data_set_manager/models.py	`83.59% <0%> (-0.89%)`	⬇️
refinery/core/models.py	`74.59% <0%> (-0.71%)`	⬇️
... and 4 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 0c25f5b...09c74d3. Read the comment docs.

scottx611x · 2018-07-06T18:54:14Z

refinery/core/views.py

+
+                try:
+                    FileStoreItem.objects\
+                        .get(uuid=old_file_uuid).delete_datafile()


I don't think we would want to delete the datafile on every patch, correct? I would think that would only be necessary if the old file_uuid differed from the new one

Yes, correct. I'll fix that.

FYI, this api is only supporting what we need which right now is removing file_uuid. It does not support adding or changing.

scottx611x · 2018-07-06T19:00:06Z

refinery/core/views.py

@@ -693,13 +696,79 @@ def graph(self, request, *args, **kwargs):
        )


-class NodeViewSet(viewsets.ModelViewSet):


What was the need for the ModelViewSet to APIView change?

https://stackoverflow.com/questions/41379654/difference-between-apiview-class-and-viewsets-class/41380941

I was more so curious about the underlying need to switch from one to the other

scottx611x · 2018-07-06T19:03:50Z

refinery/core/serializers.py

+        fields = ['uuid', 'file_uuid']
+
+    def validate_file_uuid(self, file_uuid):
+        if file_uuid != 'None':


Are we setting Node.file_uuid to the string 'None' by default? That seems off to me.
Maybe you're looking for: if file_uuid is not None:?

* Add service for removing file. * Add method to view. * Seperate the add and remove features. * Add file name. * Add remove for owners only. * Fix download for nonowners.

hackdna

It'd be great to add unit tests to this PR before it is merged to make sure Node API access controls are working correctly.

hackdna · 2018-07-10T14:49:08Z

refinery/core/serializers.py

-            'ready_for_igv_detail_view',
-            'file_uuid'
-        ]
+        fields = ['uuid', 'file_uuid']


('uuid, 'file_uuid')

example

hackdna · 2018-07-10T15:16:35Z

refinery/core/views.py

+              required: false
+    ...
+    """
+    http_method_names = ['get', 'patch']


The recommended approach to restrict API methods is to inherit from GenericViewSet and add appropriate mixins.

Not a viewset.

hackdna · 2018-07-10T15:24:18Z

refinery/core/views.py

+    http_method_names = ['get', 'patch']
+
+    def update_file_store(self, old_uuid, new_uuid):
+        if new_uuid is None:


This check should not be needed since validate_file_uuid() in serializer already handles validation.

Even though the api is only supporting file_uuid deletion, this is needed to avoid future issues when the file_uuid is not changed and other fields are.

hackdna · 2018-07-10T15:25:25Z

refinery/core/views.py

+    """
+    http_method_names = ['get', 'patch']
+
+    def update_file_store(self, old_uuid, new_uuid):


This method contains very simple logic and is used only once, so does not need to be factored out.

hackdna · 2018-07-10T15:27:39Z

refinery/core/views.py

+    def update_file_store(self, old_uuid, new_uuid):
+        if new_uuid is None:
+            try:
+                FileStoreItem.objects.get(uuid=old_uuid).delete_datafile()


Try block should be as simple as possible, so delete_datafile() should be moved out.

hackdna · 2018-07-10T16:11:49Z

refinery/core/views.py

+        public_group = ExtendedGroup.objects.public_group()
+
+        if request.user.has_perm('core.read_dataset', data_set) or \
+                'read_dataset' in get_perms(public_group, data_set):


DRF docs recommend using a custom permission class

hackdna · 2018-07-10T16:15:04Z

refinery/core/views.py

+            serializer = NodeSerializer(node)
+            return Response(serializer.data)
+
+        return Response(node, status=status.HTTP_401_UNAUTHORIZED)


Returning a Node instance contradicts HTTP 401 status code.

hackdna · 2018-07-10T16:17:02Z

refinery/core/views.py

+        if data_set_owner == request.user:
+            serializer = NodeSerializer(node, data=request.data, partial=True)
+            if serializer.is_valid():
+                serializer.save()


This should probably be the last action contingent on success of data file deletion and Solr index updates.

hackdna · 2018-07-10T16:23:47Z

refinery/core/views.py

-    lookup_field = 'uuid'
-    http_method_names = ['get']
-    # permission_classes = (IsAuthenticated,)
+class NodeViewSet(APIView):


Missing authentication_classes and permission_classes attributes.

hackdna · 2018-07-10T16:24:47Z

refinery/core/views.py

+        old_file_uuid = node.file_uuid
+        data_set_owner = node.study.get_dataset().get_owner()
+
+        if data_set_owner == request.user:


DRF docs recommend using a custom permission class

jkmarx added 5 commits July 5, 2018 14:20

Remove unused igv template.

15db617

Revise node api to remove igv related features.

1f6d9bc

Fix typo

e80358d

Add get method.

eca401a

Add patch.

0355e4d

jkmarx added this to the Release 1.6.5 milestone Jul 5, 2018

jkmarx added 6 commits July 5, 2018 14:54

Add patch

da9c18a

Refactor duplicate code to seperate method.

e42f9cc

Add auth.

0187953

Fix node object.

00e4bf8

Remove irrelevant unit tests.

a89939b

Fix yaml for swagger.

a44ddef

jkmarx requested review from scottx611x and hackdna July 6, 2018 15:50

Fix spacing.

9ee29bb

scottx611x approved these changes Jul 6, 2018

View reviewed changes

jkmarx added 2 commits July 6, 2018 14:07

Remove datafile and update index.

c7ec459

Remove unneccessary variable.

1bc9818

scottx611x reviewed Jul 6, 2018

View reviewed changes

jkmarx added 2 commits July 6, 2018 18:01

Avoid string conversion.

2c20a34

Specify None.

2910083

jkmarx self-assigned this Jul 6, 2018

Jkmarx/remove file ui (#2845)

09c74d3

* Add service for removing file. * Add method to view. * Seperate the add and remove features. * Add file name. * Add remove for owners only. * Fix download for nonowners.

jkmarx merged commit 2f968bf into develop Jul 9, 2018

jkmarx deleted the jkmarx/remove-unused-igv-stuff branch July 9, 2018 14:37

hackdna reviewed Jul 10, 2018

View reviewed changes

jkmarx mentioned this pull request Jul 10, 2018

Implement support for removing files from a data set #2817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jkmarx/remove unused igv stuff #2844

Jkmarx/remove unused igv stuff #2844

jkmarx commented Jul 5, 2018 •

edited

codecov bot commented Jul 6, 2018 •

edited

scottx611x Jul 6, 2018

jkmarx Jul 6, 2018

jkmarx Jul 6, 2018

scottx611x Jul 6, 2018

jkmarx Jul 6, 2018

scottx611x Jul 9, 2018

scottx611x Jul 6, 2018

hackdna left a comment

hackdna Jul 10, 2018

hackdna Jul 10, 2018

jkmarx Jul 10, 2018

hackdna Jul 10, 2018

jkmarx Jul 10, 2018

hackdna Jul 10, 2018

hackdna Jul 10, 2018

hackdna Jul 10, 2018

hackdna Jul 10, 2018

hackdna Jul 10, 2018

hackdna Jul 10, 2018

hackdna Jul 10, 2018

		@@ -693,13 +696,79 @@ def graph(self, request, args, *kwargs):
		)


		class NodeViewSet(viewsets.ModelViewSet):

Jkmarx/remove unused igv stuff #2844

Jkmarx/remove unused igv stuff #2844

Conversation

jkmarx commented Jul 5, 2018 • edited

codecov bot commented Jul 6, 2018 • edited

Codecov Report

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

hackdna left a 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

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

jkmarx commented Jul 5, 2018 •

edited

codecov bot commented Jul 6, 2018 •

edited