Jkmarx/add group member api #3309

jkmarx · 2019-04-03T11:24:02Z

Add groupMember resource to promote/demote/remove users
Add unit tests

* Refactor other view sets to use helper. * Fix typo. * Jkmarx/update client edit perms (#3287) * Update client to use new group api. * Update unit tests. * Remove unused service. * Add other unit tests. * Remove dataSetResource (#3288) * Remove dataSetResource * Remove unused shareable api resource and unused helpers. (#3289) * Remove unused shareable api resource and unused helpers. * Remove helper tests.

…ry-platform into jkmarx/extend-group-api-to-add

…ry-platform into jkmarx/extend-group-api-return-all-user-groups

codecov · 2019-04-03T14:03:23Z

Codecov Report

Merging #3309 into develop will decrease coverage by 0.46%.
The diff coverage is 90.9%.

@@             Coverage Diff             @@
##           develop    #3309      +/-   ##
===========================================
- Coverage    67.33%   66.86%   -0.47%     
===========================================
  Files          375      375              
  Lines        25420    24659     -761     
  Branches       939      939              
===========================================
- Hits         17116    16488     -628     
+ Misses        8304     8171     -133

Impacted Files	Coverage Δ
...nery/ui/source/js/commons/services/group-member.js	`100% <ø> (ø)`	⬆️
refinery/core/urls.py	`100% <ø> (ø)`	⬆️
...e/js/commons/ctrls/group-member-edit-modal-ctrl.js	`43.63% <ø> (ø)`	⬆️
...i/source/js/commons/ctrls/group-edit-modal-ctrl.js	`55.17% <ø> (ø)`	⬆️
refinery/core/models.py	`81.26% <100%> (-0.59%)`	⬇️
refinery/core/test_views.py	`100% <100%> (ø)`	⬆️
refinery/core/serializers.py	`87.42% <100%> (-4.17%)`	⬇️
refinery/core/test_models.py	`99.85% <100%> (-0.03%)`	⬇️
refinery/core/api.py	`39.13% <100%> (+11.11%)`	⬆️
refinery/core/views.py	`60.63% <70%> (-2.36%)`	⬇️
... and 7 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 e9ec411...be9d803. Read the comment docs.

hackdna · 2019-04-03T14:15:31Z

refinery/core/models.py

@@ -1863,6 +1863,12 @@ def get_managed_group(self):
        except:
            return None

+    def is_user_a_group_manager(self, user):


Sounds like it should be a method on the User model.

hackdna · 2019-04-03T14:18:47Z

refinery/core/serializers.py

@@ -128,6 +129,11 @@ class ExtendedGroupSerializer(serializers.ModelSerializer):
        validators=[UniqueValidator(queryset=ExtendedGroup.objects.all())]
    )

+    def get_manager_group_uuid(self, group):


Seems redundant. Doesn't appear to be used anywhere.

It's a field the client expects.

hackdna · 2019-04-03T14:24:26Z

refinery/core/views.py

+            return Response(uuid)
+        return HttpResponseBadRequest(content="No users left in group.")
+
+    def is_user_authorized_to_edit(self, group, request_user, edit_user):


Sounds like it should be a method either on the Group or the User model. This would help with reuse and simplify the function signature.

It's only used for this delete request which is dependent on both the member and request user. I don't imagine it being used anywhere else, so it's fine here.

hackdna · 2019-04-03T15:24:03Z

refinery/core/views.py

+            logger.error(e)
+            raise APIException("Multiple groups returned for this request.")
+
+    def get_user(self, id):


Why use Django internal ID instead of UUID like everywhere else?

Legacy of previous api. url /group/:uuid/member/:id
I can update it to both use uuid after merging the other two branches into here.

Yes, it would be great to fix this.

@hackdna Users don't have UUID. User profiles have UUIDs. Simplicity wise, it would make sense to use IDs verses having to grab users through their profiles.

I see. It would be great to switch to a custom User model as Two Scoops and Django docs suggest but it's out of the scope of this PR.

hackdna · 2019-04-03T16:33:06Z

refinery/core/views.py

+        group = self.get_object(uuid)
+        edit_user = self.get_user(id)
+        if self.is_user_authorized_to_edit(group, request.user, edit_user):
+            return Response(uuid, status=status.HTTP_403_FORBIDDEN)


If user is authorized to edit then return HTTP 403?

Forgot the un, will update the method name.

I'd rather keep the name and simplify the logic.

* Update client to group memebers api v2. * Rename for clarity. * Remove unused group api v1 resource. (#3311)

jkmarx and others added 28 commits March 26, 2019 11:29

Extend group api to update group perms and retrieve all group info.

6553d89

Add unit tests.

e8d769e

Refactor and add utils method.

397603e

Fix method name and comments for clarity.

621bd48

Resolve merge conflict.

65722cf

Refactor email address in tests.

964bd37

Update return statuses for clarity

8e52d31

Limit group returns by user membership.

4c5aabd

Remove unused groupManagementResource.

b987154

Extend api to create groups.

694872c

Resolve merge conflict.

faa52d1

Add unit tests.

8dc2e47

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

30e4eca

…ry-platform into jkmarx/extend-group-api-to-add

Extend api to return all member groups.

88446c8

Add unit tests.

e423606

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

9deb3ed

…ry-platform into jkmarx/extend-group-api-return-all-user-groups

Add unit tests.

c40d450

Extend group API to destroy.

b5774a5

Add unit tests.

cc3ed54

Fix typo.

41d9a0a

Update client.

0ec7315

Extend api to return can_edit field.

9dde810

Add group unit tests.

27f9405

Avoid unneccessary api call.

b260db1

Adjust fields to accomodate current client.

8c1d0e5

Add unit tests.

b9d12f2

Resolve merge conflicts.

34c4f56

jkmarx self-assigned this Apr 3, 2019

jkmarx added this to the Release 1.6.9 milestone Apr 3, 2019

jkmarx added api technical debt labels Apr 3, 2019

jkmarx added 2 commits April 3, 2019 09:19

Move helper to model method.

1688a71

Add unit tests.

c07b901

jkmarx requested a review from hackdna April 3, 2019 13:39

Fix flake issue with line break.

55f0969

hackdna requested changes Apr 3, 2019

View reviewed changes

hackdna reviewed Apr 3, 2019

View reviewed changes

jkmarx and others added 2 commits April 3, 2019 13:14

Update client to group memebers api v2. (#3310)

b03f733

* Update client to group memebers api v2. * Rename for clarity. * Remove unused group api v1 resource. (#3311)

Fix method name.

be9d803

jkmarx merged commit 690342f into develop Apr 4, 2019

jkmarx deleted the jkmarx/add-group-member-api branch April 4, 2019 13:46

hackdna mentioned this pull request Apr 4, 2019

Replace UserProfile model with a custom user model #3313

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jkmarx/add group member api #3309

Jkmarx/add group member api #3309

jkmarx commented Apr 3, 2019

codecov bot commented Apr 3, 2019 •

edited

hackdna Apr 3, 2019

hackdna Apr 3, 2019

jkmarx Apr 3, 2019

hackdna Apr 3, 2019

jkmarx Apr 3, 2019

hackdna Apr 3, 2019

jkmarx Apr 3, 2019

hackdna Apr 3, 2019

jkmarx Apr 3, 2019

hackdna Apr 3, 2019

hackdna Apr 3, 2019

jkmarx Apr 3, 2019

hackdna Apr 3, 2019

Jkmarx/add group member api #3309

Jkmarx/add group member api #3309

Conversation

jkmarx commented Apr 3, 2019

codecov bot commented Apr 3, 2019 • 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

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

codecov bot commented Apr 3, 2019 •

edited