From 493f0e5a09bb92dab38e13419e7b5c320e9779dd Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 6 Nov 2023 14:47:49 -0500 Subject: [PATCH] feat: add group owners to features (#2908) --- .../migrations/0060_feature_group_owners.py | 19 ++ api/features/models.py | 4 + api/features/permissions.py | 2 + api/features/serializers.py | 20 ++- api/features/views.py | 39 ++++- api/import_export/export.py | 2 +- api/poetry.lock | 96 ++++++++++- api/pyproject.toml | 1 + .../unit/features/test_unit_features_views.py | 163 +++++++++++++++++- api/users/serializers.py | 2 +- api/users/views.py | 4 +- 11 files changed, 339 insertions(+), 13 deletions(-) create mode 100644 api/features/migrations/0060_feature_group_owners.py diff --git a/api/features/migrations/0060_feature_group_owners.py b/api/features/migrations/0060_feature_group_owners.py new file mode 100644 index 000000000000..a39017866ba1 --- /dev/null +++ b/api/features/migrations/0060_feature_group_owners.py @@ -0,0 +1,19 @@ +# Generated by Django 3.2.20 on 2023-10-30 20:15 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0034_add_user_permission_group_membership_through_model'), + ('features', '0059_fix_feature_type'), + ] + + operations = [ + migrations.AddField( + model_name='feature', + name='group_owners', + field=models.ManyToManyField(blank=True, related_name='owned_features', to='users.UserPermissionGroup'), + ), + ] diff --git a/api/features/models.py b/api/features/models.py index 6091eded2f86..b9d3e7c406d9 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -113,6 +113,10 @@ class Feature( owners = models.ManyToManyField( "users.FFAdminUser", related_name="owned_features", blank=True ) + group_owners = models.ManyToManyField( + "users.UserPermissionGroup", related_name="owned_features", blank=True + ) + is_server_key_only = models.BooleanField(default=False) history_record_class_path = "features.models.HistoricalFeature" diff --git a/api/features/permissions.py b/api/features/permissions.py index a0a49dbf0667..8d84b426829d 100644 --- a/api/features/permissions.py +++ b/api/features/permissions.py @@ -26,6 +26,8 @@ "create": CREATE_FEATURE, "add_owners": CREATE_FEATURE, "remove_owners": CREATE_FEATURE, + "add_group_owners": CREATE_FEATURE, + "remove_group_owners": CREATE_FEATURE, "update": CREATE_FEATURE, "partial_update": CREATE_FEATURE, } diff --git a/api/features/serializers.py b/api/features/serializers.py index a9ddff5e70ea..509e1f02f4ef 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -11,7 +11,11 @@ HideSensitiveFieldsSerializerMixin, ) from projects.models import Project -from users.serializers import UserIdsSerializer, UserListSerializer +from users.serializers import ( + UserIdsSerializer, + UserListSerializer, + UserPermissionGroupSummarySerializer, +) from util.drf_writable_nested.serializers import ( DeleteBeforeUpdateWritableNestedModelSerializer, ) @@ -36,8 +40,21 @@ def remove_users(self, feature: Feature): feature.owners.remove(*user_ids) +class FeatureGroupOwnerInputSerializer(serializers.Serializer): + group_ids = serializers.ListField(child=serializers.IntegerField()) + + def add_group_owners(self, feature: Feature): + group_ids = self.validated_data["group_ids"] + feature.group_owners.add(*group_ids) + + def remove_group_owners(self, feature: Feature): + group_ids = self.validated_data["group_ids"] + feature.group_owners.remove(*group_ids) + + class ProjectFeatureSerializer(serializers.ModelSerializer): owners = UserListSerializer(many=True, read_only=True) + group_owners = UserPermissionGroupSummarySerializer(many=True, read_only=True) class Meta: model = Feature @@ -50,6 +67,7 @@ class Meta: "default_enabled", "type", "owners", + "group_owners", "is_server_key_only", ) writeonly_fields = ("initial_value", "default_enabled") diff --git a/api/features/views.py b/api/features/views.py index 4d8831b20e6d..cbfe227f9f0b 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -13,7 +13,7 @@ from django.views.decorators.cache import cache_page from drf_yasg import openapi from drf_yasg.utils import swagger_auto_schema -from rest_framework import mixins, status, viewsets +from rest_framework import mixins, serializers, status, viewsets from rest_framework.decorators import action, api_view, permission_classes from rest_framework.exceptions import NotFound, ValidationError from rest_framework.generics import GenericAPIView, get_object_or_404 @@ -35,6 +35,7 @@ ) from projects.models import Project from projects.permissions import VIEW_PROJECT +from users.models import UserPermissionGroup from webhooks.webhooks import WebhookEventType from .models import Feature, FeatureState @@ -48,6 +49,7 @@ from .serializers import ( CreateSegmentOverrideFeatureStateSerializer, FeatureEvaluationDataSerializer, + FeatureGroupOwnerInputSerializer, FeatureInfluxDataSerializer, FeatureOwnerInputSerializer, FeatureQuerySerializer, @@ -157,6 +159,41 @@ def get_serializer_context(self): return context + @swagger_auto_schema( + request_body=FeatureGroupOwnerInputSerializer, + responses={200: ProjectFeatureSerializer}, + ) + @action(detail=True, methods=["POST"], url_path="add-group-owners") + def add_group_owners(self, request, *args, **kwargs): + feature = self.get_object() + data = request.data + + 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") + + serializer.add_group_owners(feature) + response = Response(self.get_serializer(instance=feature).data) + return response + + @swagger_auto_schema( + request_body=FeatureGroupOwnerInputSerializer, + responses={200: ProjectFeatureSerializer}, + ) + @action(detail=True, methods=["POST"], url_path="remove-group-owners") + def remove_group_owners(self, request, *args, **kwargs): + feature = self.get_object() + serializer = FeatureGroupOwnerInputSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + serializer.remove_group_owners(feature) + response = Response(self.get_serializer(instance=feature).data) + return response + @swagger_auto_schema( request_body=FeatureOwnerInputSerializer, responses={200: ProjectFeatureSerializer}, diff --git a/api/import_export/export.py b/api/import_export/export.py index 86f9938ff3f4..0aa106b2e25b 100644 --- a/api/import_export/export.py +++ b/api/import_export/export.py @@ -187,7 +187,7 @@ def export_features(organisation_id: int) -> typing.List[dict]: _EntityExportConfig( Feature, Q(project__organisation__id=organisation_id), - exclude_fields=["owners"], + exclude_fields=["owners", "group_owners"], ), _EntityExportConfig( MultivariateFeatureOption, diff --git a/api/poetry.lock b/api/poetry.lock index df2a317141ca..b0a2529c7d51 100644 --- a/api/poetry.lock +++ b/api/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 1.6.1 and should not be changed by hand. +# This file is automatically @generated by Poetry 1.5.1 and should not be changed by hand. [[package]] name = "aiohttp" @@ -211,8 +211,8 @@ files = [ lazy-object-proxy = ">=1.4.0" typing-extensions = {version = ">=4.0.0", markers = "python_version < \"3.11\""} wrapt = [ - {version = ">=1.11,<2", markers = "python_version < \"3.11\""}, {version = ">=1.14,<2", markers = "python_version >= \"3.11\""}, + {version = ">=1.11,<2", markers = "python_version < \"3.11\""}, ] [[package]] @@ -825,8 +825,8 @@ openapi-spec-validator = ">=0.2.8,<=0.5.7" packaging = "*" prance = ">=0.18.2" pydantic = [ - {version = ">=1.9.0,<3.0", extras = ["email"], markers = "python_version >= \"3.10\" and python_version < \"3.11\""}, {version = ">=1.10.0,<3.0", extras = ["email"], markers = "python_version >= \"3.11\" and python_version < \"4.0\""}, + {version = ">=1.9.0,<3.0", extras = ["email"], markers = "python_version >= \"3.10\" and python_version < \"3.11\""}, ] PySnooper = ">=0.4.1,<2.0.0" toml = ">=0.10.0,<1.0.0" @@ -1395,6 +1395,21 @@ files = [ [package.extras] testing = ["hatch", "pre-commit", "pytest", "tox"] +[[package]] +name = "fancycompleter" +version = "0.9.1" +description = "colorful TAB completion for Python prompt" +optional = false +python-versions = "*" +files = [ + {file = "fancycompleter-0.9.1-py3-none-any.whl", hash = "sha256:dd076bca7d9d524cc7f25ec8f35ef95388ffef9ef46def4d3d25e9b044ad7080"}, + {file = "fancycompleter-0.9.1.tar.gz", hash = "sha256:09e0feb8ae242abdfd7ef2ba55069a46f011814a80fe5476be48f51b00247272"}, +] + +[package.dependencies] +pyreadline = {version = "*", markers = "platform_system == \"Windows\""} +pyrepl = ">=0.8.2" + [[package]] name = "filelock" version = "3.12.2" @@ -2451,6 +2466,26 @@ files = [ {file = "pathspec-0.11.2.tar.gz", hash = "sha256:e0d8d0ac2f12da61956eb2306b69f9469b42f4deb0f3cb6ed47b9cce9996ced3"}, ] +[[package]] +name = "pdbpp" +version = "0.10.3" +description = "pdb++, a drop-in replacement for pdb" +optional = false +python-versions = "*" +files = [ + {file = "pdbpp-0.10.3-py2.py3-none-any.whl", hash = "sha256:79580568e33eb3d6f6b462b1187f53e10cd8e4538f7d31495c9181e2cf9665d1"}, + {file = "pdbpp-0.10.3.tar.gz", hash = "sha256:d9e43f4fda388eeb365f2887f4e7b66ac09dce9b6236b76f63616530e2f669f5"}, +] + +[package.dependencies] +fancycompleter = ">=0.8" +pygments = "*" +wmctrl = "*" + +[package.extras] +funcsigs = ["funcsigs"] +testing = ["funcsigs", "pytest"] + [[package]] name = "pep8" version = "1.7.1" @@ -2830,6 +2865,20 @@ files = [ {file = "pyflakes-3.0.1.tar.gz", hash = "sha256:ec8b276a6b60bd80defed25add7e439881c19e64850afd9b346283d4165fd0fd"}, ] +[[package]] +name = "pygments" +version = "2.16.1" +description = "Pygments is a syntax highlighting package written in Python." +optional = false +python-versions = ">=3.7" +files = [ + {file = "Pygments-2.16.1-py3-none-any.whl", hash = "sha256:13fc09fa63bc8d8671a6d247e1eb303c4b343eaee81d861f3404db2935653692"}, + {file = "Pygments-2.16.1.tar.gz", hash = "sha256:1daff0494820c69bc8941e407aa20f577374ee88364ee10a98fdbe0aece96e29"}, +] + +[package.extras] +plugins = ["importlib-metadata"] + [[package]] name = "pyjwt" version = "2.8.0" @@ -2865,8 +2914,8 @@ files = [ astroid = ">=2.14.2,<=2.16.0-dev0" colorama = {version = ">=0.4.5", markers = "sys_platform == \"win32\""} dill = [ - {version = ">=0.2", markers = "python_version < \"3.11\""}, {version = ">=0.3.6", markers = "python_version >= \"3.11\""}, + {version = ">=0.2", markers = "python_version < \"3.11\""}, ] isort = ">=4.2.5,<6" mccabe = ">=0.6,<0.8" @@ -2966,6 +3015,26 @@ files = [ [package.dependencies] tomli = {version = ">=1.1.0", markers = "python_version < \"3.11\""} +[[package]] +name = "pyreadline" +version = "2.1" +description = "A python implmementation of GNU readline." +optional = false +python-versions = "*" +files = [ + {file = "pyreadline-2.1.zip", hash = "sha256:4530592fc2e85b25b1a9f79664433da09237c1a270e4d78ea5aa3a2c7229e2d1"}, +] + +[[package]] +name = "pyrepl" +version = "0.9.0" +description = "A library for building flexible command line interfaces" +optional = false +python-versions = "*" +files = [ + {file = "pyrepl-0.9.0.tar.gz", hash = "sha256:292570f34b5502e871bbb966d639474f2b57fbfcd3373c2d6a2f3d56e681a775"}, +] + [[package]] name = "pyrsistent" version = "0.19.3" @@ -4023,6 +4092,23 @@ files = [ [package.extras] brotli = ["Brotli"] +[[package]] +name = "wmctrl" +version = "0.5" +description = "A tool to programmatically control windows inside X" +optional = false +python-versions = ">=2.7" +files = [ + {file = "wmctrl-0.5-py2.py3-none-any.whl", hash = "sha256:ae695c1863a314c899e7cf113f07c0da02a394b968c4772e1936219d9234ddd7"}, + {file = "wmctrl-0.5.tar.gz", hash = "sha256:7839a36b6fe9e2d6fd22304e5dc372dbced2116ba41283ea938b2da57f53e962"}, +] + +[package.dependencies] +attrs = "*" + +[package.extras] +test = ["pytest"] + [[package]] name = "wrapt" version = "1.15.0" @@ -4241,4 +4327,4 @@ requests = ">=2.7,<3.0" [metadata] lock-version = "2.0" python-versions = "^3.10" -content-hash = "2f018f20e63331505030b0ff9692d32328b2e3bbade5fd7809e957fd4e126ffc" +content-hash = "0953761dca78786c8a5f4d553cd12dcb562f0c62aa39dbe250744c9746085c93" diff --git a/api/pyproject.toml b/api/pyproject.toml index c94e5e153567..7121d9747b8f 100644 --- a/api/pyproject.toml +++ b/api/pyproject.toml @@ -129,6 +129,7 @@ pip-tools = "~6.13.0" pytest-cov = "~4.1.0" datamodel-code-generator = "~0.22" requests-mock = "^1.11.0" +pdbpp = "^0.10.3" [build-system] requires = ["poetry-core>=1.5.0"] diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 4214dc3434b1..e51813c9f45f 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -14,10 +14,10 @@ from features.feature_types import MULTIVARIATE from features.models import Feature, FeatureSegment, FeatureState from features.multivariate.models import MultivariateFeatureOption -from organisations.models import Organisation +from organisations.models import Organisation, OrganisationRole from projects.models import Project, UserProjectPermission from segments.models import Segment -from users.models import FFAdminUser +from users.models import FFAdminUser, UserPermissionGroup def test_list_feature_states_from_simple_view_set( @@ -563,6 +563,165 @@ def test_add_owners_adds_owner(client, project): } +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_add_group_owners_adds_group_owner(client, project): + # Given + feature = Feature.objects.create(name="Test Feature", project=project) + user_1 = FFAdminUser.objects.create_user(email="user1@mail.com") + organisation = project.organisation + group_1 = UserPermissionGroup.objects.create( + name="Test Group", organisation=organisation + ) + group_2 = UserPermissionGroup.objects.create( + name="Second Group", organisation=organisation + ) + user_1.add_organisation(organisation, OrganisationRole.ADMIN) + group_1.users.add(user_1) + group_2.users.add(user_1) + + url = reverse( + "api-v1:projects:project-features-add-group-owners", + args=[project.id, feature.id], + ) + + data = {"group_ids": [group_1.id, group_2.id]} + + # When + json_response = client.post( + url, data=json.dumps(data), content_type="application/json" + ).json() + + # Then + assert len(json_response["group_owners"]) == 2 + assert json_response["group_owners"][0] == { + "id": group_1.id, + "name": group_1.name, + } + assert json_response["group_owners"][1] == { + "id": group_2.id, + "name": group_2.name, + } + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_remove_group_owners_removes_group_owner(client, project): + # Given + feature = Feature.objects.create(name="Test Feature", project=project) + user_1 = FFAdminUser.objects.create_user(email="user1@mail.com") + organisation = project.organisation + group_1 = UserPermissionGroup.objects.create( + name="To be removed group", organisation=organisation + ) + group_2 = UserPermissionGroup.objects.create( + name="To be kept group", organisation=organisation + ) + user_1.add_organisation(organisation, OrganisationRole.ADMIN) + group_1.users.add(user_1) + group_2.users.add(user_1) + + feature.group_owners.add(group_1.id, group_2.id) + + url = reverse( + "api-v1:projects:project-features-remove-group-owners", + args=[project.id, feature.id], + ) + + # Note that only group_1 is set to be removed. + data = {"group_ids": [group_1.id]} + + # When + json_response = client.post( + url, data=json.dumps(data), content_type="application/json" + ).json() + + # Then + assert len(json_response["group_owners"]) == 1 + assert json_response["group_owners"][0] == { + "id": group_2.id, + "name": group_2.name, + } + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_remove_group_owners_when_nonexistent(client, project): + # Given + feature = Feature.objects.create(name="Test Feature", project=project) + user_1 = FFAdminUser.objects.create_user(email="user1@mail.com") + organisation = project.organisation + group_1 = UserPermissionGroup.objects.create( + name="To be removed group", organisation=organisation + ) + user_1.add_organisation(organisation, OrganisationRole.ADMIN) + group_1.users.add(user_1) + + assert feature.group_owners.count() == 0 + + url = reverse( + "api-v1:projects:project-features-remove-group-owners", + args=[project.id, feature.id], + ) + + # Note that group_1 is not present, but it should work + # anyway since there may have been a double request or two + # users working at the same time. + data = {"group_ids": [group_1.id]} + + # When + json_response = client.post( + url, data=json.dumps(data), content_type="application/json" + ).json() + + # Then + assert len(json_response["group_owners"]) == 0 + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_add_group_owners_with_wrong_org_group(client, project): + # Given + feature = Feature.objects.create(name="Test Feature", project=project) + user_1 = FFAdminUser.objects.create_user(email="user1@mail.com") + user_2 = FFAdminUser.objects.create_user(email="user2@mail.com") + organisation = project.organisation + other_organisation = Organisation.objects.create(name="Orgy") + + group_1 = UserPermissionGroup.objects.create( + name="Valid Group", organisation=organisation + ) + group_2 = UserPermissionGroup.objects.create( + name="Invalid Group", organisation=other_organisation + ) + user_1.add_organisation(organisation, OrganisationRole.ADMIN) + user_2.add_organisation(other_organisation, OrganisationRole.ADMIN) + group_1.users.add(user_1) + group_2.users.add(user_2) + + url = reverse( + "api-v1:projects:project-features-add-group-owners", + args=[project.id, feature.id], + ) + + data = {"group_ids": [group_1.id, group_2.id]} + + # When + response = client.post(url, data=json.dumps(data), content_type="application/json") + + # Then + assert response.status_code == 400 + response.json() == {"non_field_errors": ["Some groups not found"]} + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], diff --git a/api/users/serializers.py b/api/users/serializers.py index 038cb5751e68..789b1b1412f5 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -95,7 +95,7 @@ class Meta: read_only_fields = ("id",) -class MyUserPermissionGroupsSerializer(serializers.ModelSerializer): +class UserPermissionGroupSummarySerializer(serializers.ModelSerializer): class Meta: model = UserPermissionGroup fields = ("id", "name") diff --git a/api/users/views.py b/api/users/views.py index 3e79ebab8db8..aa3b0ef1b95e 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -33,10 +33,10 @@ from users.serializers import ( ListUserPermissionGroupSerializer, ListUsersQuerySerializer, - MyUserPermissionGroupsSerializer, UserIdsSerializer, UserListSerializer, UserPermissionGroupSerializerDetail, + UserPermissionGroupSummarySerializer, ) from .forms import InitConfigForm @@ -179,7 +179,7 @@ def get_serializer_class(self): if self.action == "retrieve": return UserPermissionGroupSerializerDetail elif self.action == "my_groups": - return MyUserPermissionGroupsSerializer + return UserPermissionGroupSummarySerializer return ListUserPermissionGroupSerializer def get_serializer_context(self):