Jkmarx/data set owner transfer #2794

jkmarx · 2018-05-30T16:33:48Z

…ry-platform into jkmarx/data-set-owner-transfer

codecov · 2018-05-30T16:51:30Z

Codecov Report

Merging #2794 into develop will decrease coverage by 2.5%.
The diff coverage is 93.54%.

@@             Coverage Diff             @@
##           develop    #2794      +/-   ##
===========================================
- Coverage    59.84%   57.34%   -2.51%     
===========================================
  Files          415      417       +2     
  Lines        28396    26257    -2139     
  Branches      1253     1256       +3     
===========================================
- Hits         16993    15056    -1937     
+ Misses       11403    11201     -202

Impacted Files	Coverage Δ
refinery/core/models.py	`75.14% <100%> (-5.21%)`	⬇️
refinery/core/test_views.py	`100% <100%> (ø)`	⬆️
refinery/core/views.py	`40.37% <100%> (+1.97%)`	⬆️
...js/dashboard/directives/data-set-transfer-modal.js	`100% <100%> (ø)`
...ce/js/dashboard/controllers/data-sets-card-ctrl.js	`67.02% <57.14%> (-0.8%)`	⬇️
...hboard/controllers/data-set-transfer-modal-ctrl.js	`57.5% <57.5%> (ø)`
refinery/tool_manager/views.py	`85.71% <0%> (-1.13%)`	⬇️
refinery/tool_manager/models.py	`96.97% <0%> (-0.61%)`	⬇️
... and 6 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 5a6230c...cb5bc40. Read the comment docs.

jkmarx · 2018-05-30T18:30:37Z

refinery/core/test_views.py

@@ -54,8 +54,8 @@ def setUp(self):
        )

        # Create Datasets
-        self.dataset = create_dataset_with_necessary_models(user=self.user)
-        self.dataset2 = create_dataset_with_necessary_models(user=self.user)
+        self.data_set = create_dataset_with_necessary_models(user=self.user)


Kept making typos so went ahead and updated the file to data_set vs dataset

scottx611x · 2018-05-31T20:44:22Z

refinery/core/views.py

+                perm_groups = self.update_group_perms(new_owner)
+                self.send_tranfer_notification_email(current_owner,
+                                                     new_owner, perm_groups)
+                self.data_set.transfer_ownership(current_owner, new_owner)


I think it would make sense to call transfer_ownership before sending the email notification. That way we could wrap it in a try/except and handle any errors and only send the email if we've deemed the ownership transfer successful.

I'm also curious as to what would happen if anything went wrong after update_group_perms has been called. We should think about how to rollback the unsharing done here: https://github.com/refinery-platform/refinery-platform/pull/2794/files#diff-f20c3884fa8cbcaa080cb9b22e276ad9R915 if send_tranfer_notification_email or transfer_ownership calls fails

Yes, the order is wrong. Must have jumbled it up when refactoring.

scottx611x · 2018-05-31T20:45:12Z

refinery/core/views.py

            )

+    def send_tranfer_notification_email(self, old_owner, new_owner,


tranfer -> transfer

scottx611x · 2018-05-31T20:51:36Z

refinery/core/views.py

+            to=[new_owner.email, old_owner.email]
+        )
+        email.send()
+        return email


Do we need to return the email here?

It's useful for testing.

scottx611x · 2018-05-31T20:53:00Z

refinery/core/views.py

+                        group.extendedgroup.uuid
+                    )
+                })
+            if group.id in new_owner_group_ids:


Could this just be an else?

scottx611x · 2018-05-31T20:56:14Z

refinery/core/views.py

+        )
+        groups_with_access = []
+        groups_without_access = []
+        for group in all_groups_with_ds_access:


Could we do something like:

group_data = { 'name': group.extendedgroup.name, 'profile': 'http://{}/groups/{}'.format(self.current_site, group.extendedgroup.uuid) }

at the beginning of this for, and just append group_data to groups_without_access & groups_with_access below instead of duplicating the dict?

scottx611x · 2018-05-31T21:00:52Z

refinery/core/test_views.py

        self.assertFalse(is_filtered)

+    def test_send_tranfer_notification_email_corrent_users(self):


tranfer -> transfer
corrent -> current

scottx611x · 2018-05-31T21:04:27Z

refinery/core/test_views.py

+        self.data_set.share(ExtendedGroup.objects.public_group())
+        group_union = ExtendedGroup.objects.create(name="Group Union",
+                                                   is_public=True)
+        group_non_union = ExtendedGroup.objects.create(name="Group Non-Union",


I believe in Refinery there is only to be one public group. Maybe this isn't testing an exact scenario we'd encounter

Unsure what you mean. The data set is shared with three groups (one being public) and the new owner only belongs to public and the union group. This test checks to see the data set is unshared with the non-union group.

scottx611x · 2018-05-31T21:05:34Z

refinery/core/test_views.py

+                                             self.password)
+        self.data_set.share(ExtendedGroup.objects.public_group())
+        group_union = ExtendedGroup.objects.create(name="Group Union",
+                                                   is_public=True)


See above public group concern

scottx611x · 2018-05-31T21:06:16Z

refinery/core/test_views.py

+        new_owner_email = 'new_owner@fake.com'
+        new_owner = User.objects.create_user('NewOwner1', new_owner_email,
+                                             self.password)
+        group_union_public = ExtendedGroup.objects.public_group()


group_union_public -> public_group ?

* Add tranfer modal. * Error handling for invalid email. * Add group notice text. * Disable button after successful transfer. * Add data set title to modal. * Add unit tests. * Add unit test for view method. * Update email text. * Additional modal text. * Fix css. * Relocate data set name to minimize clutter on form. * Fix all transfer typos. * Remove console. * Uneccessary string.

jkmarx · 2018-06-02T00:45:30Z

refinery/core/test_views.py

+
+    @mock.patch('core.views.DataSetsViewSet.update_group_perms',
+                side_effect=RuntimeError)
+    def test_dataset_patch_fails_and_rollback_owner(self, mock_update):


@scottx611x These are the additional tests

I did confirm the unsharing

jkmarx · 2018-06-02T00:47:06Z

refinery/core/views.py

+                except Exception:
+                    return Response(uuid, status=status.HTTP_404_NOT_FOUND)
+
+                try:


@scottx611x Error handling and rollback

scottx611x · 2018-06-06T13:55:07Z

refinery/core/views.py

+                        perm_groups = self.update_group_perms(new_owner)
+                except Exception as e:
+                    return Response(
+                        e, status=status.HTTP_500_INTERNAL_SERVER_ERROR


Thinking again, this would be more appropriate as an error in the 400's. Its weird to explicitly send back a 500

@scottx611x Suggestions?
http://www.django-rest-framework.org/api-guide/status-codes/#client-error-4xx

It's not really a bad request is it?

Maybe HTTP_412_PRECONDITION_FAILED ?

Good reference for HTTP response status codes: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

jkmarx · 2018-06-06T18:33:29Z

@scottx611x After reading @hackdna posts, I think the 412 is appropriate. Were there any other changes?

scottx611x · 2018-06-06T18:42:14Z

@jkmarx Its been a while since I looked at the whole PR. I'll take a peek and let you know

jkmarx added 7 commits May 24, 2018 14:32

Add tranfer_ownership method to model.

1c50cdd

Remove share for non-unioned groups.

fd73d7c

Add email template to notify users of transfer.

d322818

Update text for clarity.

672dcf2

Move email to helper method.

0062fad

Merge branch 'develop' of https://github.com/refinery-platform/refine…

61f087d

…ry-platform into jkmarx/data-set-owner-transfer

Add unit tests for view and email helper method.

48e86d6

Add unit tests for update_group_perms.

e8bfa00

jkmarx commented May 30, 2018

View reviewed changes

jkmarx mentioned this pull request May 30, 2018

Collaboration Enhancements #2332

Open

jkmarx requested a review from scottx611x May 30, 2018 18:53

jkmarx self-assigned this May 30, 2018

jkmarx added this to the Release 1.6.5 milestone May 30, 2018

scottx611x requested changes May 31, 2018

View reviewed changes

jkmarx and others added 7 commits May 31, 2018 18:31

Fix incorrect order.

a7f542d

Minimize verbose name.

a1c79f6

Refactor to remove other if.

ff281c6

Add exception handling.

c83665b

Refactor for DRY.

1183d58

Add unit test to check rollback.

3b9dccf

jkmarx commented Jun 2, 2018

View reviewed changes

scottx611x reviewed Jun 6, 2018

View reviewed changes

Update response status for error.

cb5bc40

scottx611x approved these changes Jun 6, 2018

View reviewed changes

jkmarx merged commit a488e68 into develop Jun 6, 2018

jkmarx deleted the jkmarx/data-set-owner-transfer branch June 6, 2018 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jkmarx/data set owner transfer #2794

Jkmarx/data set owner transfer #2794

jkmarx commented May 30, 2018 •

edited

codecov bot commented May 30, 2018 •

edited

jkmarx May 30, 2018

scottx611x May 31, 2018

jkmarx May 31, 2018

scottx611x May 31, 2018

scottx611x May 31, 2018

jkmarx May 31, 2018

scottx611x May 31, 2018

scottx611x May 31, 2018 •

edited

scottx611x May 31, 2018

scottx611x May 31, 2018

jkmarx May 31, 2018

scottx611x May 31, 2018

scottx611x May 31, 2018

jkmarx Jun 2, 2018

jkmarx Jun 2, 2018

jkmarx Jun 2, 2018

scottx611x Jun 6, 2018

jkmarx Jun 6, 2018

jkmarx Jun 6, 2018

scottx611x Jun 6, 2018

hackdna Jun 6, 2018

jkmarx commented Jun 6, 2018

scottx611x commented Jun 6, 2018

		)

		def send_tranfer_notification_email(self, old_owner, new_owner,

		self.assertFalse(is_filtered)

		def test_send_tranfer_notification_email_corrent_users(self):

Jkmarx/data set owner transfer #2794

Jkmarx/data set owner transfer #2794

Conversation

jkmarx commented May 30, 2018 • edited

codecov bot commented May 30, 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

scottx611x May 31, 2018 • edited

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

Choose a reason for hiding this comment

jkmarx commented Jun 6, 2018

scottx611x commented Jun 6, 2018

jkmarx commented May 30, 2018 •

edited

codecov bot commented May 30, 2018 •

edited

scottx611x May 31, 2018 •

edited