Add API views/urls/serializers/tests for OSF Groups on Nodes #8818

erinspace · 2018-11-19T21:46:53Z

Purpose

API work for groups attached to node detail. Includes listing groups, group detail, attaching a group to a node, changing details of an OSF group's permission on a node, and removing an OSF Group from a node.

Changes

API Routes, Serializers, Views, Tests
Add generic CompoundIDField for the ID field consisting of the node _id and the group _id
Add NODE_OSF_GROUPS_READ/WRITE core scope and add that to NODE_ACCESS_READ/WRITE

QA Notes

This is part of the larger guardian feature, so is pretty untestable on its own, and testing I imagine will take place once all the pieces are in place.

Documentation

In progress!

Side Effects

This PR will not be effective until #8817 is merged, as the relationships referenced here are defined in this PR (also tests are not expected to pass for this reason, as well as a reason listed below in "Questions."

Ticket

https://openscience.atlassian.net/browse/PLAT-1184

Questions

One of the currently failing tests on travis is in the TestNodeRegistrationSerializer class in particular because groups isn't in the should_not_relate_to_registrations and also isn't yet on the Registration serializer. Wasn't sure how to rectify this, as I know OSF Groups have a tenuous relationship with Registrations, so wasn't sure if they should indeed show up or not, or if this decision has been made.
The other one is very similar and also involves a decision on weather or not the groups relationship is visible on Registrations/Withdrawls

EDIT: Not showing groups on registrations, since we won't be copying groups over to registrations when a registration is created.

[#PLAT-1184] - Add serializers, views, urls, tests - Add generic CompoundIDField for the ID field consisting of the node _id and the group _id

erinspace · 2018-11-19T21:47:33Z

api/nodes/serializers.py

+
+class OSFGroupsRelationshipField(RelationshipField):
+
+    def get_value(self, data):


What on earth am I doing wrong that made me need to add this? Is it somehow because of the CompoundIDField? Without this, the relationship for the OSF Group (the id) disappears from validated_data when trying to create a new relationship between a node and a group. I tried forever to figure this out but got so stuck figured someone else could look at it and it'd be more obvious.

I don't think it's the CompoundIDField because that is for the Node-OSF Group Relationship and the id being passed in the payload references the Group id. What am I missing?

You don't need a special RelationshipField at all here! You just need a regular RelationshipsField and the id needs to be writeable on 'create'.

More info:
The relationships portion of the JSONAPIParser was originally written around this use case, something that looked like a NodeContributor. Where your request was creating a single to-one relationship, plus additional attributes (Contributor = User object + permission/bibliographic). (The node wouldn't have to be specified as a relationship on write, because this would be a request to the NodeContributor endpoint). NodeOSFGroups are the same! (NodeOSFGroup = OSFGroup object + permission)

The JSONAPIParser will flatten your attributes and relationships and put it all on the same level. So your permission and osf_groups data will end up looking like this: {'id'': abcde_osfgroupid', 'target_type': 'osf_groups', 'permission': 'write', 'type': 'node-groups'}. When run_validation is called on that, fields that aren't writable fields will be dropped, so you will have {'_id': 'abcde_osfgroupid', 'permission': 'write', 'type': 'node-groups'}. Because we just need to write one relationship, the relationship to the osf_group, the parser will retain the osf_group_id, and because _id will be writeable, it won't get dropped.

We end up using other parsers or creating special RelationshipFields for where we need to update multiple relationships in a request, or where the single relationship is a to-many.

erinspace · 2018-11-19T21:48:55Z

api/nodes/views.py

+        if field_name == 'permission':
+            node = self.get_node()
+            osf_groups = node.osf_groups
+            group_ids = [


This logic felt necessarily complicated, and I spent awhile trying to make it nicer. Can anyone point out a better way to get all of the groups granted a given permission on the specified node?

In response to your question ^, I just added a method to the node model in my linked PR, still seems like it's doing a lot. There are many tables involved.

def get_osf_groups_with_perms(self, permission): """Returns a queryset of OSF Groups whose members have the specified permission to the node """ from osf.models.osf_group import OSFGroup from osf.models.node import NodeGroupObjectPermission perm_id = Permission.objects.get(codename=permission + '_node').id member_groups = NodeGroupObjectPermission.objects.filter( permission_id=perm_id, content_object_id=self.id ).filter( group__name__icontains='osfgroup' ).values_list( 'group_id', flat=True ) return OSFGroup.objects.filter(osfgroupgroupobjectpermission__group_id__in=member_groups)

erinspace · 2018-11-19T21:50:51Z

api/nodes/serializers.py

@@ -1012,27 +1018,27 @@ def create(self, validated_data):
        return fork


-class ContributorIDField(IDField):
-    """ID field to use with the contributor resource. Contributor IDs have the form "<node-id>-<user-id>"."""
+class CompoundIDField(IDField):


It didn't seem like this ContributorIDField was actually being used anymore, instead the _id was being stored in that format on the Contributor model. I decided to use this and generalize it anyway incase we ever wanted to use it in the future, but I'd also understand if we thought this was too broad and un-specific that it wouldn't be useful in other cases.

Nice. Looks like we haven't needed the ContributorIdField in awhile, since we added the _id property on the Contributor object. I wasn't 100% sure we needed the id in this format (node-group) for the NodeGroupSerializer, but if I recall correctly, I think Ember got confused without it.

@property def _id(self): return '{}-{}'.format(self.node._id, self.user._id)

pattisdr

❗️ PR here with my suggested changes

Nice work Erin; you had some especially thorough tests that covered some scenarios I hadn't thought of. Also, I should have made #8817 come first instead of having these two work in parallel - sorry for the extra work that this caused.

Since I had to dig into your code to figure some of this out, I ended up committing my changes. Everything suggested below except for the casing of the 'node-groups' type. I put this up as a PR against #8817, changes specifically outlined here pattisdr@7973e71. I think we should merge OSF Groups API work and Node Groups API work instead of having them reviewed at the next stage separately since they're linked. Feel free to push back against any of the changes I made.

pattisdr · 2018-11-21T15:56:41Z

api/nodes/serializers.py

+
+class OSFGroupsRelationshipField(RelationshipField):
+
+    def get_value(self, data):


You don't need a special RelationshipField at all here! You just need a regular RelationshipsField and the id needs to be writeable on 'create'.

More info:
The relationships portion of the JSONAPIParser was originally written around this use case, something that looked like a NodeContributor. Where your request was creating a single to-one relationship, plus additional attributes (Contributor = User object + permission/bibliographic). (The node wouldn't have to be specified as a relationship on write, because this would be a request to the NodeContributor endpoint). NodeOSFGroups are the same! (NodeOSFGroup = OSFGroup object + permission)

The JSONAPIParser will flatten your attributes and relationships and put it all on the same level. So your permission and osf_groups data will end up looking like this: {'id'': abcde_osfgroupid', 'target_type': 'osf_groups', 'permission': 'write', 'type': 'node-groups'}. When run_validation is called on that, fields that aren't writable fields will be dropped, so you will have {'_id': 'abcde_osfgroupid', 'permission': 'write', 'type': 'node-groups'}. Because we just need to write one relationship, the relationship to the osf_group, the parser will retain the osf_group_id, and because _id will be writeable, it won't get dropped.

We end up using other parsers or creating special RelationshipFields for where we need to update multiple relationships in a request, or where the single relationship is a to-many.

pattisdr · 2018-11-21T15:57:44Z

api/nodes/serializers.py

+        return {'group_id': osf_group_id}
+
+
+class NodeGroupsSerializer(JSONAPISerializer):


Would split NodeGroupsSerializer into three -adding NodeGroupsCreateSerializer and NodeGroupsDetailSerializer.

pattisdr · 2018-11-21T15:58:36Z

api/nodes/serializers.py

+    id = CompoundIDField(source='_id', read_only=True)
+    permission = ser.SerializerMethodField()
+    type = TypeField()
+    name = ser.CharField(read_only=True)


-Unsure if we should expose name here since it's on the osf_group serializer and not specific to NodeGroup.-

EDIT: Oh, you're filtering on it, ok, good, I could see a use-case for this.

pattisdr · 2018-11-21T15:58:57Z

api/nodes/serializers.py

+    osf_groups = OSFGroupsRelationshipField(
+        related_view='osf_groups:group-detail',
+        related_view_kwargs={'group_id': '<_id>'},
+        read_only=False,


Can just be a regular RelationshipField.

pattisdr · 2018-11-21T15:59:47Z

api/nodes/serializers.py

+            )
+        return osf_permissions.reduce_permissions(permissions)
+
+    def update(self, obj, validated_data):


If NodeGroupsDetailSerializer is made, would move update to that serializer.

pattisdr · 2018-11-21T20:07:44Z

api/nodes/views.py

+        node = self.get_node()
+        auth = get_user_auth(self.request)
+        if instance not in node.osf_groups:
+            raise ValidationError('The OSF group {} has not been added to the node {}'.format(instance.name, node._id))


If get_object is handling the checking for whether the group specified belongs to the node's osf group, then we can remove this.

pattisdr · 2018-11-21T20:27:34Z

api_tests/nodes/views/test_node_groups.py

+        public_project.add_osf_group(osf_group, 'admin')
+        res = app.delete_json_api(public_detail_url, payload, auth=member.auth)
+        assert res.status_code == 204
+        assert osf_group not in public_project.osf_groups


This last case was something I hadn't considered, but it fits the "you either need admin permissions to the node" OR "be a manager of the group you're trying to remove". If your OSF Group has admin permissions to the node, then anyone in the group can remove it.

Also any other osf group that has admin perms to the node can also remove anyone else's osf group. Is this fine? Just looking for a sanity check on this one!

pattisdr · 2018-11-21T20:33:16Z

api_tests/nodes/views/test_node_groups.py

+        assert res.status_code == 403
+
+        # test manager on group can remove group
+        res = app.delete_json_api(public_detail_url, payload, auth=manager.auth)


Since the manager is also an admin contributor, this doesn't quite cover the scenario where even if a manager's perms have been downgraded, they are still a manager on the group and can remove themselves (similar to the fact that a read contributor can remove themselves).
The node model actually currently prevents this and needs to be changed.

pattisdr · 2018-11-21T23:38:16Z

api/nodes/views.py

+
+    # overrides FilterMixin
+    def build_query_from_field(self, field_name, operation):
+        if field_name == 'permission':


We might need to double check permission is read, write, or admin ourselves since permission is not a ChoiceField or similar?

pattisdr · 2018-11-21T23:52:56Z

api/nodes/views.py

+        if field_name == 'permission':
+            node = self.get_node()
+            osf_groups = node.osf_groups
+            group_ids = [


In response to your question ^, I just added a method to the node model in my linked PR, still seems like it's doing a lot. There are many tables involved.

def get_osf_groups_with_perms(self, permission): """Returns a queryset of OSF Groups whose members have the specified permission to the node """ from osf.models.osf_group import OSFGroup from osf.models.node import NodeGroupObjectPermission perm_id = Permission.objects.get(codename=permission + '_node').id member_groups = NodeGroupObjectPermission.objects.filter( permission_id=perm_id, content_object_id=self.id ).filter( group__name__icontains='osfgroup' ).values_list( 'group_id', flat=True ) return OSFGroup.objects.filter(osfgroupgroupobjectpermission__group_id__in=member_groups)

erinspace · 2018-11-26T16:19:52Z

@pattisdr thanks for the changes! I went ahead and made a PR to your PR for the type change! Should I just close this PR then?

pattisdr · 2018-11-26T16:31:22Z

thanks @erinspace - yeah, I would go ahead and close, and then on the other PR, I'll point out that the Node OSF Groups portion has had one pass of CR.

erinspace · 2018-11-26T16:39:51Z

Closing in favor of pattisdr#22

API routes, views, serializers, tests for OSF Groups on Nodes

86a7011

[#PLAT-1184] - Add serializers, views, urls, tests - Add generic CompoundIDField for the ID field consisting of the node _id and the group _id

erinspace commented Nov 19, 2018

View reviewed changes

pattisdr mentioned this pull request Nov 22, 2018

Feature/Node Groups API [PLAT-1184] pattisdr/osf.io#22

Merged

pattisdr reviewed Nov 22, 2018

View reviewed changes

erinspace closed this Nov 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API views/urls/serializers/tests for OSF Groups on Nodes #8818

Add API views/urls/serializers/tests for OSF Groups on Nodes #8818

erinspace commented Nov 19, 2018 •

edited by pattisdr

Loading

erinspace Nov 19, 2018 •

edited

Loading

pattisdr Nov 21, 2018

erinspace Nov 19, 2018

pattisdr Nov 21, 2018 •

edited

Loading

erinspace Nov 19, 2018

pattisdr Nov 21, 2018

pattisdr left a comment •

edited

Loading

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018

pattisdr Nov 21, 2018 •

edited

Loading

erinspace commented Nov 26, 2018

pattisdr commented Nov 26, 2018

erinspace commented Nov 26, 2018


		class OSFGroupsRelationshipField(RelationshipField):

		def get_value(self, data):

		return {'group_id': osf_group_id}


		class NodeGroupsSerializer(JSONAPISerializer):

Add API views/urls/serializers/tests for OSF Groups on Nodes #8818

Add API views/urls/serializers/tests for OSF Groups on Nodes #8818

Conversation

erinspace commented Nov 19, 2018 • edited by pattisdr Loading

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

Questions

erinspace Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr left a comment • edited Loading

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

pattisdr Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

erinspace commented Nov 26, 2018

pattisdr commented Nov 26, 2018

erinspace commented Nov 26, 2018

erinspace commented Nov 19, 2018 •

edited by pattisdr

Loading

erinspace Nov 19, 2018 •

edited

Loading

pattisdr Nov 21, 2018 •

edited

Loading

pattisdr left a comment •

edited

Loading

pattisdr Nov 21, 2018 •

edited

Loading