feat: add group owners to features #2908
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2908 +/- ##
==========================================
- Coverage 95.63% 95.62% -0.02%
==========================================
Files 1011 1012 +1
Lines 29006 29122 +116
==========================================
+ Hits 27741 27848 +107
- Misses 1265 1274 +9
☔ View full report in Codecov by Sentry. |
api/features/views.py
Outdated
| if not UserPermissionGroup.objects.filter( | ||
| id__in=data["group_ids"], | ||
| organisation_id=feature.project.organisation_id, | ||
| ).count() == len(data["group_ids"]): | ||
| raise serializers.ValidationError("Some groups not found") | ||
|
|
||
| serializer = FeatureGroupOwnerInputSerializer(data=data) | ||
| serializer.is_valid(raise_exception=True) |
There was a problem hiding this comment.
We should just switch these around since, otherwise, if someone posts an empty body (or any body missing the group_ids key) we would throw a 500 instead of, in the code below, a 400 explaining what the issue is.
| if not UserPermissionGroup.objects.filter( | |
| id__in=data["group_ids"], | |
| organisation_id=feature.project.organisation_id, | |
| ).count() == len(data["group_ids"]): | |
| raise serializers.ValidationError("Some groups not found") | |
| serializer = FeatureGroupOwnerInputSerializer(data=data) | |
| serializer.is_valid(raise_exception=True) | |
| serializer = FeatureGroupOwnerInputSerializer(data=data) | |
| serializer.is_valid(raise_exception=True) | |
| if not UserPermissionGroup.objects.filter( | |
| id__in=serializer.validated_data["group_ids"], | |
| organisation_id=feature.project.organisation_id, | |
| ).count() == len(serializer.validated_data["group_ids"]): | |
| raise serializers.ValidationError("Some groups not found") |
There was a problem hiding this comment.
Apologies, the diff looks bad, but all I've done is switched the serializer validation and the ORM validation around (and used serializer.validated_data instead of data["group_ids"])
There was a problem hiding this comment.
Ok cool, I see what you're going for. I've updated the PR with the changes.
api/features/views.py
Outdated
| if not UserPermissionGroup.objects.filter( | ||
| id__in=serializer.validated_data["group_ids"], | ||
| organisation_id=feature.project.organisation_id, | ||
| ).count() == len(data["group_ids"]): |
There was a problem hiding this comment.
| ).count() == len(data["group_ids"]): | |
| ).count() == len(serializer.validated_data["group_ids"]): |
There was a problem hiding this comment.
Blast! I've committed the fix. (Not sure what black, etc would do if I use the GH interface)
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingChanges
This adds two endpoints to features. The first is to allow the addition of groups as owners of a feature. Think groups like "design department" which could have many users under them. It also allows the removal of groups through a separate, similar endpoint.
This work supports this ticket, though I would like to get feedback before implementing any filtering of features so that part of the ticket is not yet complete.
How did you test this code?
I wrote two new tests that go through the API. I was a little confused as to why the practice was to use a master key for authentication, but I kept with the normal way of doing things since that is how other parts of the codebase around that part did it.