From 18abb1f79192f25be8cb99ba01b85e86101025e5 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 2 Aug 2021 22:58:29 +0530 Subject: [PATCH 01/18] wip: Make objects cloneable --- api/environments/identities/models.py | 12 ++++++++++-- api/environments/models.py | 7 ++++--- api/features/models.py | 28 +++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/api/environments/identities/models.py b/api/environments/identities/models.py index 24acef1d9dfb..cd4d465cb4c1 100644 --- a/api/environments/identities/models.py +++ b/api/environments/identities/models.py @@ -1,12 +1,13 @@ import hashlib import typing +from copy import deepcopy from django.db import models -from django.db.models import Q, Prefetch +from django.db.models import Prefetch, Q from django.utils.encoding import python_2_unicode_compatible -from environments.models import Environment from environments.identities.traits.models import Trait +from environments.models import Environment from features.models import FeatureState from features.multivariate.models import MultivariateFeatureStateValue @@ -184,3 +185,10 @@ def update_traits(self, trait_data_items): # return the full list of traits for this identity by refreshing from the db # TODO: handle this in the above logic to avoid a second hit to the DB return self.get_all_user_traits() + + def clone(self, environment: Environment) -> "Identity": + clone = deepcopy(self) + clone.id = None + clone.environment = environment + clone.save() + return clone diff --git a/api/environments/models.py b/api/environments/models.py index 4d3d48d05866..7f112d96c7ae 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -65,14 +65,15 @@ def __str__(self): return "Project %s - Environment %s" % (self.project.name, self.name) def clone(self, name: str, api_key: str = None) -> "Environment": + # NOTE: clone will not trigger create hooks # Creates a copy/clone of the object clone = deepcopy(self) - # update the state to let django know that this object is not coming from database - # ref: https://docs.djangoproject.com/en/3.2/topics/db/queries/#copying-model-instances - clone._state.adding = True clone.id = None clone.name = name clone.api_key = api_key if api_key else create_hash() + clone.save() + for fs in self.feature_states.all(): + fs.clone(clone) return clone @staticmethod diff --git a/api/features/models.py b/api/features/models.py index fc56fb2779c2..8c73bac880d6 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -2,6 +2,7 @@ import logging import typing +from copy import deepcopy from django.core.exceptions import ( NON_FIELD_ERRORS, @@ -170,6 +171,13 @@ class FeatureSegment(OrderedModelBase): # used for audit purposes history = HistoricalRecords() + def clone(environment) -> "FeatureSegment": + clone = deepcopy(self) + clone.id = None + clone.environment = environment + clone.save() + return clone + class Meta: unique_together = ("feature", "environment", "segment") ordering = ("priority",) @@ -199,6 +207,7 @@ class FeatureState(LifecycleModel, models.Model): feature = models.ForeignKey( Feature, related_name="feature_states", on_delete=models.CASCADE ) + environment = models.ForeignKey( "environments.Environment", related_name="feature_states", @@ -225,6 +234,18 @@ class FeatureState(LifecycleModel, models.Model): enabled = models.BooleanField(default=False) history = HistoricalRecords() + def clone(self, env) -> "FeatureState": + clone = deepcopy(self) + clone.id = None + clone.identity = self.identity.clone(env) if self.identity else None + clone.feature_segment = ( + self.feature_segment.clone(env) if self.feature_segment else None + ) + clone.environment = env + clone.save() + self.feature_state_value.clone(clone) + return clone + class Meta: # Note: this is manually overridden in the migrations for Oracle DBs to include # all 4 unique fields in each of these constraints. See migration 0025. @@ -435,3 +456,10 @@ class FeatureStateValue(AbstractBaseFeatureValueModel): string_value = models.CharField(null=True, max_length=20000, blank=True) history = HistoricalRecords() + + def clone(self, feature_state: FeatureState) -> "FeatureStateValue": + clone = deepcopy(self) + clone.id = None + clone.feature_state = feature_state + clone.save() + return clone From e9f128c670d485c15529f0a744df3e54bbfaaad3 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Aug 2021 10:51:39 +0530 Subject: [PATCH 02/18] Add test cases for FeatureSegment.clone --- .../feature_segments/tests/test_models.py | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/api/features/feature_segments/tests/test_models.py b/api/features/feature_segments/tests/test_models.py index 1fd2057f6697..3bb5aa521cf7 100644 --- a/api/features/feature_segments/tests/test_models.py +++ b/api/features/feature_segments/tests/test_models.py @@ -3,12 +3,17 @@ from environments.identities.models import Identity from environments.identities.traits.models import Trait -from environments.models import Environment, STRING -from features.models import Feature, FeatureSegment, FeatureState, FeatureStateValue -from features.utils import INTEGER, BOOLEAN +from environments.models import STRING, Environment +from features.models import ( + Feature, + FeatureSegment, + FeatureState, + FeatureStateValue, +) +from features.utils import BOOLEAN, INTEGER from organisations.models import Organisation from projects.models import Project -from segments.models import Segment, SegmentRule, Condition, EQUAL +from segments.models import EQUAL, Condition, Segment, SegmentRule @pytest.mark.django_db @@ -135,3 +140,22 @@ def test_feature_segments_are_created_with_correct_priority(self): assert feature_segment_3.priority == 0 assert feature_segment_4.priority == 0 assert feature_segment_5.priority == 0 + + def test_clone_creates_a_new_object(self): + # Given + feature_segment = FeatureSegment.objects.create( + feature=self.remote_config, + segment=self.segment, + environment=self.environment, + priority=1, + ) + new_env = Environment.objects.create( + name="Test environment New", project=self.project + ) + + # When + feature_segment_clone = feature_segment.clone() + + # Then + assert feature_segment_clone.id != feature_segment.id + assert feature_segment_clone.priority != feature_segment.priority From 79a5ce4c53e07153d97b6c2de405c087fc5aee0b Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Aug 2021 11:45:32 +0530 Subject: [PATCH 03/18] refactor(fixtures): move some fixutres to global conftest This commit creates a global conftest that can be used by tests who exists outside the integration directory. --- api/conftest.py | 69 +++++++++++++++++++++++++++++++ api/tests/integration/conftest.py | 66 ----------------------------- 2 files changed, 69 insertions(+), 66 deletions(-) create mode 100644 api/conftest.py diff --git a/api/conftest.py b/api/conftest.py new file mode 100644 index 000000000000..d50914f0551f --- /dev/null +++ b/api/conftest.py @@ -0,0 +1,69 @@ +import uuid + +import pytest +from django.test import Client as DjangoClient +from django.urls import reverse +from rest_framework.test import APIClient + +from app.utils import create_hash + + +@pytest.fixture() +def admin_user(django_user_model): + return django_user_model.objects.create(email="test@example.com") + + +@pytest.fixture() +def admin_client(admin_user): + client = APIClient() + client.force_authenticate(user=admin_user) + return client + + +@pytest.fixture() +def organisation(admin_client): + organisation_data = {"name": "Test org"} + url = reverse("api-v1:organisations:organisation-list") + response = admin_client.post(url, data=organisation_data) + return response.json()["id"] + + +@pytest.fixture() +def project(admin_client, organisation): + project_data = {"name": "Test Project", "organisation": organisation} + url = reverse("api-v1:projects:project-list") + response = admin_client.post(url, data=project_data) + return response.json()["id"] + + +@pytest.fixture() +def environment_api_key(): + return create_hash() + + +@pytest.fixture() +def environment(admin_client, project, environment_api_key) -> int: + environment_data = { + "name": "Test Environment", + "api_key": environment_api_key, + "project": project, + } + url = reverse("api-v1:environments:environment-list") + + response = admin_client.post(url, data=environment_data) + return response.json()["id"] + + +@pytest.fixture() +def identity_identifier(): + return uuid.uuid4() + + +@pytest.fixture() +def identity(admin_client, identity_identifier, environment, environment_api_key): + identity_data = {"identifier": identity_identifier} + url = reverse( + "api-v1:environments:environment-identities-list", args=[environment_api_key] + ) + response = admin_client.post(url, data=identity_data) + return response.json()["id"] diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index 300b1ab1a662..c858b10e073c 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -1,79 +1,13 @@ -import uuid - import pytest from django.test import Client as DjangoClient -from django.urls import reverse from rest_framework.test import APIClient -from app.utils import create_hash - - -@pytest.fixture() -def admin_user(django_user_model): - return django_user_model.objects.create(email="test@example.com") - @pytest.fixture() def django_client(): return DjangoClient() -@pytest.fixture() -def admin_client(admin_user): - client = APIClient() - client.force_authenticate(user=admin_user) - return client - - -@pytest.fixture() -def organisation(admin_client): - organisation_data = {"name": "Test org"} - url = reverse("api-v1:organisations:organisation-list") - response = admin_client.post(url, data=organisation_data) - return response.json()["id"] - - -@pytest.fixture() -def project(admin_client, organisation): - project_data = {"name": "Test Project", "organisation": organisation} - url = reverse("api-v1:projects:project-list") - response = admin_client.post(url, data=project_data) - return response.json()["id"] - - -@pytest.fixture() -def environment_api_key(): - return create_hash() - - -@pytest.fixture() -def environment(admin_client, project, environment_api_key) -> int: - environment_data = { - "name": "Test Environment", - "api_key": environment_api_key, - "project": project, - } - url = reverse("api-v1:environments:environment-list") - - response = admin_client.post(url, data=environment_data) - return response.json()["id"] - - -@pytest.fixture() -def identity_identifier(): - return uuid.uuid4() - - -@pytest.fixture() -def identity(admin_client, identity_identifier, environment, environment_api_key): - identity_data = {"identifier": identity_identifier} - url = reverse( - "api-v1:environments:environment-identities-list", args=[environment_api_key] - ) - response = admin_client.post(url, data=identity_data) - return response.json()["id"] - - @pytest.fixture() def sdk_client(environment_api_key): client = APIClient() From 284a8c1078cb1c9de44e2d0844857eb1f4d1f73c Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Aug 2021 12:06:57 +0530 Subject: [PATCH 04/18] Add clone on Trait --- api/environments/identities/traits/models.py | 14 +++++++++++++- .../identities/traits/tests/test_models.py | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/api/environments/identities/traits/models.py b/api/environments/identities/traits/models.py index b42acfca6033..c770f5c7e970 100644 --- a/api/environments/identities/traits/models.py +++ b/api/environments/identities/traits/models.py @@ -1,4 +1,6 @@ import typing +from copy import deepcopy +from typing import TYPE_CHECKING from django.core.exceptions import ObjectDoesNotExist from django.db import models @@ -6,7 +8,10 @@ from simple_history.models import HistoricalRecords from environments.identities.traits.exceptions import TraitPersistenceError -from environments.models import INTEGER, STRING, BOOLEAN, FLOAT +from environments.models import BOOLEAN, FLOAT, INTEGER, STRING + +if TYPE_CHECKING: + from environments.identities.models import Identity @python_2_unicode_compatible @@ -100,6 +105,13 @@ def generate_trait_value_data(value: typing.Any): def __str__(self): return "Identity: %s - %s" % (self.identity.identifier, self.trait_key) + def clone(self, identity: "Identity") -> "Trait": + clone = deepcopy(self) + clone.id = None + clone.identity = identity + clone.save() + return clone + def save(self, *args, **kwargs): if not self.identity.environment.project.organisation.persist_trait_data: # this is a final line of defense to ensure that traits are never saved diff --git a/api/environments/identities/traits/tests/test_models.py b/api/environments/identities/traits/tests/test_models.py index 7f5e69b87580..5ed4b2e25079 100644 --- a/api/environments/identities/traits/tests/test_models.py +++ b/api/environments/identities/traits/tests/test_models.py @@ -1,5 +1,6 @@ import pytest +from environments.identities.models import Identity from environments.identities.traits.models import Trait @@ -45,3 +46,20 @@ def test_generate_trait_value_data_for_deserialized_data( deserialized_data, expected_data ): assert Trait.generate_trait_value_data(deserialized_data) == expected_data + + +def test_clone_creates_a_new_object(db, environment, identity): + # Given + trait = Trait.objects.create( + trait_key="test-key-one", string_value="testing trait", identity_id=identity + ) + new_identity = Identity.objects.create( + identifier="test-identity", environment_id=environment + ) + # When + cloned_trait = trait.clone(new_identity) + + # Then + assert cloned_trait.id != trait.id + assert cloned_trait.trait_key == trait.trait_key + assert cloned_trait.trait_value == trait.trait_value From 2c9e295b0609bd6e601226e2ec21c8037647c271 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Aug 2021 12:07:35 +0530 Subject: [PATCH 05/18] fix(clone:Identity): update clone to include traits --- api/environments/identities/models.py | 15 +++++--- .../identities/tests/test_models.py | 34 +++++++++++++++++-- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/api/environments/identities/models.py b/api/environments/identities/models.py index cd4d465cb4c1..83e0c58f1a2d 100644 --- a/api/environments/identities/models.py +++ b/api/environments/identities/models.py @@ -187,8 +187,13 @@ def update_traits(self, trait_data_items): return self.get_all_user_traits() def clone(self, environment: Environment) -> "Identity": - clone = deepcopy(self) - clone.id = None - clone.environment = environment - clone.save() - return clone + identity_clone = deepcopy(self) + identity_clone.id = None + identity_clone.environment = environment + identity_clone.save() + + if self.environment.project.organisation.persist_trait_data: + for trait in self.identity_traits.all(): + trait.clone(identity_clone) + + return identity_clone diff --git a/api/environments/identities/tests/test_models.py b/api/environments/identities/tests/test_models.py index e8214a2dec83..b3f056002610 100644 --- a/api/environments/identities/tests/test_models.py +++ b/api/environments/identities/tests/test_models.py @@ -3,8 +3,13 @@ from environments.identities.models import Identity from environments.identities.traits.models import Trait from environments.models import FLOAT, Environment -from features.models import Feature, FeatureSegment, FeatureState, FeatureStateValue -from features.value_types import INTEGER, STRING, BOOLEAN +from features.models import ( + Feature, + FeatureSegment, + FeatureState, + FeatureStateValue, +) +from features.value_types import BOOLEAN, INTEGER, STRING from organisations.models import Organisation from projects.models import Project from segments.models import ( @@ -819,3 +824,28 @@ def test_get_segments(self): # Then # the number of queries are what we expect (see above context manager) and the segment is returned assert len(segments) == 1 and segments[0] == segment + + def test_clone_creates_a_new_object_with_same_traits(self): + # Given + identity = Identity.objects.create( + identifier="test-identity", environment=self.environment + ) + Trait.objects.create( + trait_key="test-key-one", string_value="testing trait", identity=identity + ) + new_env = Environment.objects.create( + name="Test Environment New", project=self.project + ) + # When + identity_clone = identity.clone(new_env) + + # Then + assert identity.id != identity_clone.id + assert ( + identity.identity_traits.first().trait_key + == identity_clone.identity_traits.first().trait_key + ) + assert ( + identity.identity_traits.first().string_value + == identity_clone.identity_traits.first().string_value + ) From bc16a994109a824eb4741304efea14f5116679a8 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Aug 2021 12:24:57 +0530 Subject: [PATCH 06/18] squash! minor changes --- api/environments/models.py | 1 + api/features/feature_segments/tests/test_models.py | 4 ++-- api/features/models.py | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/api/environments/models.py b/api/environments/models.py index 7f112d96c7ae..a95860c51f56 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -72,6 +72,7 @@ def clone(self, name: str, api_key: str = None) -> "Environment": clone.name = name clone.api_key = api_key if api_key else create_hash() clone.save() + # Clone the related objects for fs in self.feature_states.all(): fs.clone(clone) return clone diff --git a/api/features/feature_segments/tests/test_models.py b/api/features/feature_segments/tests/test_models.py index 3bb5aa521cf7..60ad30e49ca0 100644 --- a/api/features/feature_segments/tests/test_models.py +++ b/api/features/feature_segments/tests/test_models.py @@ -154,8 +154,8 @@ def test_clone_creates_a_new_object(self): ) # When - feature_segment_clone = feature_segment.clone() + feature_segment_clone = feature_segment.clone(new_env) # Then assert feature_segment_clone.id != feature_segment.id - assert feature_segment_clone.priority != feature_segment.priority + assert feature_segment_clone.priority == feature_segment.priority diff --git a/api/features/models.py b/api/features/models.py index 8c73bac880d6..2a00d25ef842 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -49,6 +49,7 @@ logger = logging.getLogger(__name__) if typing.TYPE_CHECKING: + from environments.models import Environment from environments.identities.models import Identity @@ -171,7 +172,7 @@ class FeatureSegment(OrderedModelBase): # used for audit purposes history = HistoricalRecords() - def clone(environment) -> "FeatureSegment": + def clone(self, environment: "Environment") -> "FeatureSegment": clone = deepcopy(self) clone.id = None clone.environment = environment From 7a6508fc138f7e4ef08d55ec59a01d10064e2921 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Aug 2021 19:06:25 +0530 Subject: [PATCH 07/18] squash! remove clone from identity and trait --- api/environments/identities/models.py | 13 ---------- .../identities/tests/test_models.py | 25 ------------------- api/environments/identities/traits/models.py | 12 --------- .../identities/traits/tests/test_models.py | 18 ------------- 4 files changed, 68 deletions(-) diff --git a/api/environments/identities/models.py b/api/environments/identities/models.py index 83e0c58f1a2d..1996659a07d1 100644 --- a/api/environments/identities/models.py +++ b/api/environments/identities/models.py @@ -1,6 +1,5 @@ import hashlib import typing -from copy import deepcopy from django.db import models from django.db.models import Prefetch, Q @@ -185,15 +184,3 @@ def update_traits(self, trait_data_items): # return the full list of traits for this identity by refreshing from the db # TODO: handle this in the above logic to avoid a second hit to the DB return self.get_all_user_traits() - - def clone(self, environment: Environment) -> "Identity": - identity_clone = deepcopy(self) - identity_clone.id = None - identity_clone.environment = environment - identity_clone.save() - - if self.environment.project.organisation.persist_trait_data: - for trait in self.identity_traits.all(): - trait.clone(identity_clone) - - return identity_clone diff --git a/api/environments/identities/tests/test_models.py b/api/environments/identities/tests/test_models.py index b3f056002610..7c2d297cb0ac 100644 --- a/api/environments/identities/tests/test_models.py +++ b/api/environments/identities/tests/test_models.py @@ -824,28 +824,3 @@ def test_get_segments(self): # Then # the number of queries are what we expect (see above context manager) and the segment is returned assert len(segments) == 1 and segments[0] == segment - - def test_clone_creates_a_new_object_with_same_traits(self): - # Given - identity = Identity.objects.create( - identifier="test-identity", environment=self.environment - ) - Trait.objects.create( - trait_key="test-key-one", string_value="testing trait", identity=identity - ) - new_env = Environment.objects.create( - name="Test Environment New", project=self.project - ) - # When - identity_clone = identity.clone(new_env) - - # Then - assert identity.id != identity_clone.id - assert ( - identity.identity_traits.first().trait_key - == identity_clone.identity_traits.first().trait_key - ) - assert ( - identity.identity_traits.first().string_value - == identity_clone.identity_traits.first().string_value - ) diff --git a/api/environments/identities/traits/models.py b/api/environments/identities/traits/models.py index c770f5c7e970..0a4884c8715a 100644 --- a/api/environments/identities/traits/models.py +++ b/api/environments/identities/traits/models.py @@ -1,6 +1,4 @@ import typing -from copy import deepcopy -from typing import TYPE_CHECKING from django.core.exceptions import ObjectDoesNotExist from django.db import models @@ -10,9 +8,6 @@ from environments.identities.traits.exceptions import TraitPersistenceError from environments.models import BOOLEAN, FLOAT, INTEGER, STRING -if TYPE_CHECKING: - from environments.identities.models import Identity - @python_2_unicode_compatible class Trait(models.Model): @@ -105,13 +100,6 @@ def generate_trait_value_data(value: typing.Any): def __str__(self): return "Identity: %s - %s" % (self.identity.identifier, self.trait_key) - def clone(self, identity: "Identity") -> "Trait": - clone = deepcopy(self) - clone.id = None - clone.identity = identity - clone.save() - return clone - def save(self, *args, **kwargs): if not self.identity.environment.project.organisation.persist_trait_data: # this is a final line of defense to ensure that traits are never saved diff --git a/api/environments/identities/traits/tests/test_models.py b/api/environments/identities/traits/tests/test_models.py index 5ed4b2e25079..7f5e69b87580 100644 --- a/api/environments/identities/traits/tests/test_models.py +++ b/api/environments/identities/traits/tests/test_models.py @@ -1,6 +1,5 @@ import pytest -from environments.identities.models import Identity from environments.identities.traits.models import Trait @@ -46,20 +45,3 @@ def test_generate_trait_value_data_for_deserialized_data( deserialized_data, expected_data ): assert Trait.generate_trait_value_data(deserialized_data) == expected_data - - -def test_clone_creates_a_new_object(db, environment, identity): - # Given - trait = Trait.objects.create( - trait_key="test-key-one", string_value="testing trait", identity_id=identity - ) - new_identity = Identity.objects.create( - identifier="test-identity", environment_id=environment - ) - # When - cloned_trait = trait.clone(new_identity) - - # Then - assert cloned_trait.id != trait.id - assert cloned_trait.trait_key == trait.trait_key - assert cloned_trait.trait_value == trait.trait_value From c8b87902b535a2a96e204170bc456c11c5d8f9e7 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Aug 2021 19:08:39 +0530 Subject: [PATCH 08/18] Revert "refactor(fixtures): move some fixutres to global conftest" This reverts commit 79a5ce4c53e07153d97b6c2de405c087fc5aee0b. --- api/conftest.py | 69 ------------------------------- api/tests/integration/conftest.py | 66 +++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 69 deletions(-) delete mode 100644 api/conftest.py diff --git a/api/conftest.py b/api/conftest.py deleted file mode 100644 index d50914f0551f..000000000000 --- a/api/conftest.py +++ /dev/null @@ -1,69 +0,0 @@ -import uuid - -import pytest -from django.test import Client as DjangoClient -from django.urls import reverse -from rest_framework.test import APIClient - -from app.utils import create_hash - - -@pytest.fixture() -def admin_user(django_user_model): - return django_user_model.objects.create(email="test@example.com") - - -@pytest.fixture() -def admin_client(admin_user): - client = APIClient() - client.force_authenticate(user=admin_user) - return client - - -@pytest.fixture() -def organisation(admin_client): - organisation_data = {"name": "Test org"} - url = reverse("api-v1:organisations:organisation-list") - response = admin_client.post(url, data=organisation_data) - return response.json()["id"] - - -@pytest.fixture() -def project(admin_client, organisation): - project_data = {"name": "Test Project", "organisation": organisation} - url = reverse("api-v1:projects:project-list") - response = admin_client.post(url, data=project_data) - return response.json()["id"] - - -@pytest.fixture() -def environment_api_key(): - return create_hash() - - -@pytest.fixture() -def environment(admin_client, project, environment_api_key) -> int: - environment_data = { - "name": "Test Environment", - "api_key": environment_api_key, - "project": project, - } - url = reverse("api-v1:environments:environment-list") - - response = admin_client.post(url, data=environment_data) - return response.json()["id"] - - -@pytest.fixture() -def identity_identifier(): - return uuid.uuid4() - - -@pytest.fixture() -def identity(admin_client, identity_identifier, environment, environment_api_key): - identity_data = {"identifier": identity_identifier} - url = reverse( - "api-v1:environments:environment-identities-list", args=[environment_api_key] - ) - response = admin_client.post(url, data=identity_data) - return response.json()["id"] diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index c858b10e073c..300b1ab1a662 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -1,13 +1,79 @@ +import uuid + import pytest from django.test import Client as DjangoClient +from django.urls import reverse from rest_framework.test import APIClient +from app.utils import create_hash + + +@pytest.fixture() +def admin_user(django_user_model): + return django_user_model.objects.create(email="test@example.com") + @pytest.fixture() def django_client(): return DjangoClient() +@pytest.fixture() +def admin_client(admin_user): + client = APIClient() + client.force_authenticate(user=admin_user) + return client + + +@pytest.fixture() +def organisation(admin_client): + organisation_data = {"name": "Test org"} + url = reverse("api-v1:organisations:organisation-list") + response = admin_client.post(url, data=organisation_data) + return response.json()["id"] + + +@pytest.fixture() +def project(admin_client, organisation): + project_data = {"name": "Test Project", "organisation": organisation} + url = reverse("api-v1:projects:project-list") + response = admin_client.post(url, data=project_data) + return response.json()["id"] + + +@pytest.fixture() +def environment_api_key(): + return create_hash() + + +@pytest.fixture() +def environment(admin_client, project, environment_api_key) -> int: + environment_data = { + "name": "Test Environment", + "api_key": environment_api_key, + "project": project, + } + url = reverse("api-v1:environments:environment-list") + + response = admin_client.post(url, data=environment_data) + return response.json()["id"] + + +@pytest.fixture() +def identity_identifier(): + return uuid.uuid4() + + +@pytest.fixture() +def identity(admin_client, identity_identifier, environment, environment_api_key): + identity_data = {"identifier": identity_identifier} + url = reverse( + "api-v1:environments:environment-identities-list", args=[environment_api_key] + ) + response = admin_client.post(url, data=identity_data) + return response.json()["id"] + + @pytest.fixture() def sdk_client(environment_api_key): client = APIClient() From 77d758029ce52d7f804699be69fb28a044a897f6 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Wed, 4 Aug 2021 16:01:02 +0530 Subject: [PATCH 09/18] fix: don't clone feature state with identity and other minor fixes --- api/environments/models.py | 2 +- api/environments/serializers.py | 1 - api/environments/tests/test_models.py | 18 ++++++++++++++---- api/environments/views.py | 5 ++++- api/features/models.py | 2 +- 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/api/environments/models.py b/api/environments/models.py index a95860c51f56..3e86c80df0e6 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -73,7 +73,7 @@ def clone(self, name: str, api_key: str = None) -> "Environment": clone.api_key = api_key if api_key else create_hash() clone.save() # Clone the related objects - for fs in self.feature_states.all(): + for fs in self.feature_states.filter(identity=None): fs.clone(clone) return clone diff --git a/api/environments/serializers.py b/api/environments/serializers.py index cb89b4c241a2..072f951e3c4e 100644 --- a/api/environments/serializers.py +++ b/api/environments/serializers.py @@ -62,7 +62,6 @@ def create(self, validated_data): name = validated_data.get("name") source_env = validated_data.get("source_env") clone = source_env.clone(name) - clone.save() self._create_audit_log(clone, True) return clone diff --git a/api/environments/tests/test_models.py b/api/environments/tests/test_models.py index ae5334838135..08ab185911b4 100644 --- a/api/environments/tests/test_models.py +++ b/api/environments/tests/test_models.py @@ -61,7 +61,6 @@ def test_clone_does_not_modify_the_original_instance(self): # When clone = self.environment.clone(name="Cloned env") - clone.save() # Then self.assertNotEqual(clone.name, self.environment.name) @@ -73,7 +72,6 @@ def test_clone_save_creates_feature_states(self): # When clone = self.environment.clone(name="Cloned env") - clone.save() # Then feature_states = FeatureState.objects.filter(environment=clone) @@ -87,8 +85,7 @@ def test_clone_does_not_modify_source_feature_state(self): ).first() # When - clone = self.environment.clone(name="Cloned env") - clone.save() + self.environment.clone(name="Cloned env") source_feature_state_after_clone = FeatureState.objects.filter( environment=self.environment ).first() @@ -96,6 +93,19 @@ def test_clone_does_not_modify_source_feature_state(self): # Then assert source_feature_state_before_clone == source_feature_state_after_clone + def test_clone_clones_the_feature_states(self): + # Given + self.environment.save() + + # Enable the feature in the source environment + self.environment.feature_states.update(enabled=True) + + # When + clone = self.environment.clone(name="Cloned env") + + # Then + assert clone.feature_states.first().enabled is True + @mock.patch("environments.models.environment_cache") def test_get_from_cache_stores_environment_in_cache_on_success(self, mock_cache): # Given diff --git a/api/environments/views.py b/api/environments/views.py index 9f0601f4cd52..b80e9c75caa5 100644 --- a/api/environments/views.py +++ b/api/environments/views.py @@ -115,7 +115,10 @@ def trait_keys(self, request, *args, **kwargs): def clone(self, request, *args, **kwargs): serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) - serializer.save(source_env=self.get_object()) + clone = serializer.save(source_env=self.get_object()) + UserEnvironmentPermission.objects.create( + user=self.request.user, environment=clone, admin=True + ) return Response(serializer.data, status=status.HTTP_200_OK) @action(detail=True, methods=["POST"], url_path="delete-traits") diff --git a/api/features/models.py b/api/features/models.py index 2a00d25ef842..138150fbeac8 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -238,7 +238,7 @@ class FeatureState(LifecycleModel, models.Model): def clone(self, env) -> "FeatureState": clone = deepcopy(self) clone.id = None - clone.identity = self.identity.clone(env) if self.identity else None + clone.identity = None # Clonning the Identity is not supported clone.feature_segment = ( self.feature_segment.clone(env) if self.feature_segment else None ) From 45366a5315ff0b2863224b1de2935e147b553d7a Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 5 Aug 2021 08:29:30 +0530 Subject: [PATCH 10/18] test(clone): Add test to make sure we don't clone identity --- api/environments/tests/test_models.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/api/environments/tests/test_models.py b/api/environments/tests/test_models.py index 08ab185911b4..bee31ce6349d 100644 --- a/api/environments/tests/test_models.py +++ b/api/environments/tests/test_models.py @@ -93,6 +93,18 @@ def test_clone_does_not_modify_source_feature_state(self): # Then assert source_feature_state_before_clone == source_feature_state_after_clone + def test_clone_does_not_create_idetntity(self): + # Given + self.environment.save() + Identity.objects.create( + environment=self.environment, identifier="test_identity" + ) + # When + clone = self.environment.clone(name="Cloned env") + + # Then + assert clone.identities.count() == 0 + def test_clone_clones_the_feature_states(self): # Given self.environment.save() From e5045c18a8852ea7ee1713cff0e4faeb60f6e4f2 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 5 Aug 2021 08:41:46 +0530 Subject: [PATCH 11/18] fixup! minor refactoring --- api/features/models.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/api/features/models.py b/api/features/models.py index 138150fbeac8..8d43b4cc350c 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -235,10 +235,12 @@ class FeatureState(LifecycleModel, models.Model): enabled = models.BooleanField(default=False) history = HistoricalRecords() - def clone(self, env) -> "FeatureState": + def clone(self, env: "Environment") -> "FeatureState": + # Clonning the Identity is not allowed + assert self.identity is None + clone = deepcopy(self) clone.id = None - clone.identity = None # Clonning the Identity is not supported clone.feature_segment = ( self.feature_segment.clone(env) if self.feature_segment else None ) From 8534426d267701241691bc7c7c36882ccd39f5ed Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 5 Aug 2021 16:05:30 +0530 Subject: [PATCH 12/18] doc(clone-env): Add docstring to clone method on env model --- api/environments/models.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/api/environments/models.py b/api/environments/models.py index 3e86c80df0e6..81fb81e86b4b 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -65,14 +65,19 @@ def __str__(self): return "Project %s - Environment %s" % (self.project.name, self.name) def clone(self, name: str, api_key: str = None) -> "Environment": + """ + Creates a clone of the environment, related objects and returns the + cloned object after saving it to the database. # NOTE: clone will not trigger create hooks - # Creates a copy/clone of the object + """ clone = deepcopy(self) clone.id = None clone.name = name clone.api_key = api_key if api_key else create_hash() clone.save() - # Clone the related objects + # Since identities are closely tied to the enviroment + # it does not make much sense to clone them, hence + # only clone feature states without identities for fs in self.feature_states.filter(identity=None): fs.clone(clone) return clone From be48cbcfbfba52ac3b090d4281621e4370a3c56c Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Thu, 5 Aug 2021 17:18:19 +0530 Subject: [PATCH 13/18] minor fixes --- api/environments/tests/test_models.py | 2 +- api/features/feature_segments/tests/test_models.py | 1 + api/features/models.py | 2 ++ api/tests/integration/environments/test_environments.py | 1 + 4 files changed, 5 insertions(+), 1 deletion(-) diff --git a/api/environments/tests/test_models.py b/api/environments/tests/test_models.py index bee31ce6349d..8a2e4eeb1f31 100644 --- a/api/environments/tests/test_models.py +++ b/api/environments/tests/test_models.py @@ -93,7 +93,7 @@ def test_clone_does_not_modify_source_feature_state(self): # Then assert source_feature_state_before_clone == source_feature_state_after_clone - def test_clone_does_not_create_idetntity(self): + def test_clone_does_not_create_identity(self): # Given self.environment.save() Identity.objects.create( diff --git a/api/features/feature_segments/tests/test_models.py b/api/features/feature_segments/tests/test_models.py index 60ad30e49ca0..06c2b2503b54 100644 --- a/api/features/feature_segments/tests/test_models.py +++ b/api/features/feature_segments/tests/test_models.py @@ -159,3 +159,4 @@ def test_clone_creates_a_new_object(self): # Then assert feature_segment_clone.id != feature_segment.id assert feature_segment_clone.priority == feature_segment.priority + assert feature_segment_clone.environment.id == new_env.id diff --git a/api/features/models.py b/api/features/models.py index 8d43b4cc350c..5c3c3a34f5ee 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -173,6 +173,8 @@ class FeatureSegment(OrderedModelBase): history = HistoricalRecords() def clone(self, environment: "Environment") -> "FeatureSegment": + assert self.environment.id != environment.id + clone = deepcopy(self) clone.id = None clone.environment = environment diff --git a/api/tests/integration/environments/test_environments.py b/api/tests/integration/environments/test_environments.py index 286d81c84ea1..bb3c77152211 100644 --- a/api/tests/integration/environments/test_environments.py +++ b/api/tests/integration/environments/test_environments.py @@ -8,6 +8,7 @@ def test_clone_environment_returns_200( # Given env_name = "Cloned env" url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) + # When res = admin_client.post(url, {"name": env_name}) From 81fbba2e94ff16d5d850c7d426f8de02ef05266e Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 9 Aug 2021 11:45:19 +0530 Subject: [PATCH 14/18] test(clone)Add integration test cases for env clone --- api/environments/models.py | 8 +- api/features/models.py | 10 +- api/tests/integration/conftest.py | 47 ++++ .../environments/test_clone_environment.py | 214 ++++++++++++++++++ .../environments/test_environments.py | 17 -- api/tests/integration/helpers.py | 20 +- 6 files changed, 293 insertions(+), 23 deletions(-) create mode 100644 api/tests/integration/environments/test_clone_environment.py delete mode 100644 api/tests/integration/environments/test_environments.py diff --git a/api/environments/models.py b/api/environments/models.py index 81fb81e86b4b..31b2803ef161 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -75,11 +75,15 @@ def clone(self, name: str, api_key: str = None) -> "Environment": clone.name = name clone.api_key = api_key if api_key else create_hash() clone.save() + for feature_segment in self.feature_segments.all(): + feature_segment.clone(clone) + # Since identities are closely tied to the enviroment # it does not make much sense to clone them, hence # only clone feature states without identities - for fs in self.feature_states.filter(identity=None): - fs.clone(clone) + for feature_state in self.feature_states.filter(identity=None): + feature_state.clone(clone) + return clone @staticmethod diff --git a/api/features/models.py b/api/features/models.py index 5c3c3a34f5ee..706fb48f6478 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -174,7 +174,6 @@ class FeatureSegment(OrderedModelBase): def clone(self, environment: "Environment") -> "FeatureSegment": assert self.environment.id != environment.id - clone = deepcopy(self) clone.id = None clone.environment = environment @@ -240,11 +239,16 @@ class FeatureState(LifecycleModel, models.Model): def clone(self, env: "Environment") -> "FeatureState": # Clonning the Identity is not allowed assert self.identity is None - clone = deepcopy(self) clone.id = None clone.feature_segment = ( - self.feature_segment.clone(env) if self.feature_segment else None + FeatureSegment.objects.get( + environment=env, + feature=clone.feature, + segment=self.feature_segment.segment, + ) + if self.feature_segment + else None ) clone.environment = env clone.save() diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index 300b1ab1a662..fcfb7940912f 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -1,3 +1,4 @@ +import json import uuid import pytest @@ -79,3 +80,49 @@ def sdk_client(environment_api_key): client = APIClient() client.credentials(HTTP_X_ENVIRONMENT_KEY=environment_api_key) return client + + +@pytest.fixture() +def feature(admin_client, project): + default_value = "This is a value" + data = { + "name": "test feature", + "initial_value": default_value, + "project": project, + } + url = reverse("api-v1:projects:project-features-list", args=[project]) + + # When + response = admin_client.post(url, data=data) + return response.json()["id"] + + +@pytest.fixture() +def segment(admin_client, project): + url = reverse("api-v1:projects:project-segments-list", args=[project]) + data = { + "name": "Test Segment", + "project": project, + "rules": [{"type": "ALL", "rules": [], "conditions": []}], + } + + # When + response = admin_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + return response.json()["id"] + + +@pytest.fixture() +def feature_segment(admin_client, segment, feature, environment): + data = { + "feature": feature, + "segment": segment, + "environment": environment, + } + url = reverse("api-v1:features:feature-segment-list") + + response = admin_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + return response.json()["id"] diff --git a/api/tests/integration/environments/test_clone_environment.py b/api/tests/integration/environments/test_clone_environment.py new file mode 100644 index 000000000000..5feb404b69ca --- /dev/null +++ b/api/tests/integration/environments/test_clone_environment.py @@ -0,0 +1,214 @@ +import json + +from django.urls import reverse +from rest_framework import status +from tests.integration.helpers import get_env_feature_states_list_with_api + +from environments.permissions.models import UserEnvironmentPermission +from features.models import ( + Feature, + FeatureSegment, + FeatureState, + FeatureStateValue, +) + + +def test_clone_environment_returns_200( + admin_client, project, environment, environment_api_key +): + # Given + env_name = "Cloned env" + url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) + + # When + res = admin_client.post(url, {"name": env_name}) + + # Then + assert res.status_code == status.HTTP_200_OK + assert res.json()["name"] == env_name + + +def test_clone_environment_clones_feature_states_with_value( + admin_client, project, environment, environment_api_key, feature +): + + # Firstly, create an environment and update it's feature state + + # Fetch the feature state id to update + feature_state = FeatureState.objects.get( + environment_id=environment, feature=feature + ) + fs_update_url = reverse( + "api-v1:features:featurestates-detail", args=[feature_state.id] + ) + data = { + "id": feature_state.id, + "feature_state_value": "new_value", + "enabled": False, + "feature": feature, + "environment": environment, + "identity": None, + "feature_segment": None, + } + admin_client.put( + fs_update_url, data=json.dumps(data), content_type="application/json" + ) + + # Now, clone the environment + env_name = "Cloned env" + url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) + res = admin_client.post(url, {"name": env_name}) + + # Then, check that the clone was successful + assert res.status_code == status.HTTP_200_OK + + # Now, fetch the feature states of the source environment + source_env_feature_states = get_env_feature_states_list_with_api( + admin_client, {"environment": environment} + ) + + # Now, fetch the feature states of the clone enviroment + clone_env_feature_states = get_env_feature_states_list_with_api( + admin_client, {"environment": res.json()["id"]} + ) + + # Finaly, compare the responses + assert source_env_feature_states["count"] == 1 + assert ( + source_env_feature_states["results"][0]["id"] + != clone_env_feature_states["results"][0]["id"] + ) + assert ( + source_env_feature_states["results"][0]["environment"] + != clone_env_feature_states["results"][0]["environment"] + ) + + assert ( + source_env_feature_states["results"][0]["feature_state_value"] + == clone_env_feature_states["results"][0]["feature_state_value"] + ) + assert ( + source_env_feature_states["results"][0]["enabled"] + == clone_env_feature_states["results"][0]["enabled"] + ) + + +def test_clone_environment_creates_permission_with_the_current_user( + admin_user, admin_client, environment, environment_api_key +): + # Given + env_name = "Cloned env" + url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) + + # When + res = admin_client.post(url, {"name": env_name}) + + # Then + assert res.status_code == status.HTTP_200_OK + assert ( + UserEnvironmentPermission.objects.filter( + user=admin_user, environment_id=environment + ).count() + == 1 + ) + + +def test_env_clone_creates_feature_segment( + admin_client, environment, environment_api_key, db, feature, feature_segment +): + # Firstly, let's clone the environment + env_name = "Cloned env" + url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) + response = admin_client.post(url, {"name": env_name}) + + clone_env_id = response.json()["id"] + + # Then, let's fetch feature_segment list of the clone environment + base_url = reverse("api-v1:features:feature-segment-list") + url = f"{base_url}?environment={clone_env_id}&feature={feature}" + + response = admin_client.get(url) + json_response = response.json() + + # Finally, verify the structure of feature_segment + assert json_response["count"] == 1 + assert json_response["results"][0]["environment"] == clone_env_id + assert json_response["results"][0]["id"] != feature_segment + + +def test_clone_clones_segments_overrides( + admin_client, environment, environment_api_key, feature, feature_segment, segment +): + # Firstly, Let's override the segment in source environment + create_url = reverse("api-v1:features:featurestates-list") + data = { + "feature_state_value": { + "type": "unicode", + "boolean_value": None, + "integer_value": None, + "string_value": "dumb", + }, + "multivariate_feature_state_values": [], + "enabled": False, + "feature": feature, + "environment": environment, + "identity": None, + "feature_segment": feature_segment, + } + seg_override_response = admin_client.post( + create_url, data=json.dumps(data), content_type="application/json" + ) + # Make sure that override was a success + assert seg_override_response.status_code == status.HTTP_201_CREATED + + # Now, clone the environment + env_name = "Cloned env" + url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) + res = admin_client.post(url, {"name": env_name}) + + clone_env_id = res.json()["id"] + + # Then, fetch the feature state of source environment for the feature segement + source_env_feature_states = get_env_feature_states_list_with_api( + admin_client, + { + "environment": environment, + "feature": feature, + "feature_segment": feature_segment, + }, + ) + + # Then fetch the feature state of clone environment for the feature segement + clone_feature_segment_id = FeatureSegment.objects.get( + feature_id=feature, environment_id=clone_env_id, segment=segment + ).id + clone_env_feature_states = get_env_feature_states_list_with_api( + admin_client, + { + "environment": clone_env_id, + "feature": feature, + "feature_segment": clone_feature_segment_id, + }, + ) + + assert source_env_feature_states["count"] == 1 + assert ( + source_env_feature_states["results"][0]["id"] + != clone_env_feature_states["results"][0]["id"] + ) + assert ( + source_env_feature_states["results"][0]["environment"] + != clone_env_feature_states["results"][0]["environment"] + ) + assert ( + source_env_feature_states["results"][0]["feature_state_value"] + == clone_env_feature_states["results"][0]["feature_state_value"] + ) + assert ( + source_env_feature_states["results"][0]["enabled"] + == clone_env_feature_states["results"][0]["enabled"] + ) + assert ( + clone_env_feature_states["results"][0]["feature_segment"] + == clone_feature_segment_id + ) diff --git a/api/tests/integration/environments/test_environments.py b/api/tests/integration/environments/test_environments.py deleted file mode 100644 index bb3c77152211..000000000000 --- a/api/tests/integration/environments/test_environments.py +++ /dev/null @@ -1,17 +0,0 @@ -from django.urls import reverse -from rest_framework import status - - -def test_clone_environment_returns_200( - admin_client, project, environment, environment_api_key -): - # Given - env_name = "Cloned env" - url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) - - # When - res = admin_client.post(url, {"name": env_name}) - - # Then - assert res.status_code == status.HTTP_200_OK - assert res.json()["name"] == env_name diff --git a/api/tests/integration/helpers.py b/api/tests/integration/helpers.py index 9b1523bdd94d..8dd662a17888 100644 --- a/api/tests/integration/helpers.py +++ b/api/tests/integration/helpers.py @@ -2,9 +2,10 @@ import typing from django.urls import reverse +from django.utils.http import urlencode from rest_framework.test import APIClient -from features.feature_types import STANDARD, MULTIVARIATE +from features.feature_types import MULTIVARIATE, STANDARD from features.value_types import STRING @@ -49,3 +50,20 @@ def create_feature_with_api( content_type="application/json", ) return create_standard_feature_response.json()["id"] + + +def get_env_feature_states_list_with_api(client: APIClient, query_params: dict) -> dict: + """ + Return the feature states using the provided test client. + + :param client: DRF api client to use to make the request + :param query_params: A Mapping object used as query params for filtering + + """ + url = reverse( + "api-v1:features:featurestates-list", + ) + if query_params: + url = f"{url}?{urlencode(query_params)}" + response = client.get(url) + return response.json() From be1ee3326b62298f5ee6c9ce09714250f67ac9ac Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 9 Aug 2021 13:21:23 +0530 Subject: [PATCH 15/18] Minor refactoring --- api/features/models.py | 63 ++++++++++--------- api/tests/integration/conftest.py | 5 +- .../environments/test_clone_environment.py | 13 ++-- 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/api/features/models.py b/api/features/models.py index 706fb48f6478..653b5ada55a7 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -172,14 +172,6 @@ class FeatureSegment(OrderedModelBase): # used for audit purposes history = HistoricalRecords() - def clone(self, environment: "Environment") -> "FeatureSegment": - assert self.environment.id != environment.id - clone = deepcopy(self) - clone.id = None - clone.environment = environment - clone.save() - return clone - class Meta: unique_together = ("feature", "environment", "segment") ordering = ("priority",) @@ -192,10 +184,6 @@ def __str__(self): + str(self.priority) ) - # noinspection PyTypeChecker - def get_value(self): - return get_correctly_typed_value(self.value_type, self.value) - def __lt__(self, other): """ Kind of counter intuitive but since priority 1 is highest, we want to check if priority is GREATER than the @@ -203,6 +191,17 @@ def __lt__(self, other): """ return other and self.priority > other.priority + def clone(self, environment: "Environment") -> "FeatureSegment": + clone = deepcopy(self) + clone.id = None + clone.environment = environment + clone.save() + return clone + + # noinspection PyTypeChecker + def get_value(self): + return get_correctly_typed_value(self.value_type, self.value) + @python_2_unicode_compatible class FeatureState(LifecycleModel, models.Model): @@ -236,25 +235,6 @@ class FeatureState(LifecycleModel, models.Model): enabled = models.BooleanField(default=False) history = HistoricalRecords() - def clone(self, env: "Environment") -> "FeatureState": - # Clonning the Identity is not allowed - assert self.identity is None - clone = deepcopy(self) - clone.id = None - clone.feature_segment = ( - FeatureSegment.objects.get( - environment=env, - feature=clone.feature, - segment=self.feature_segment.segment, - ) - if self.feature_segment - else None - ) - clone.environment = env - clone.save() - self.feature_state_value.clone(clone) - return clone - class Meta: # Note: this is manually overridden in the migrations for Oracle DBs to include # all 4 unique fields in each of these constraints. See migration 0025. @@ -311,6 +291,27 @@ def __gt__(self, other): # it has a feature_segment or an identity return not (other.feature_segment or other.identity) + def clone(self, env: "Environment") -> "FeatureState": + # Clonning the Identity is not allowed because they are closely tied + # to the enviroment + assert self.identity is None + clone = deepcopy(self) + clone.id = None + clone.feature_segment = ( + FeatureSegment.objects.get( + environment=env, + feature=clone.feature, + segment=self.feature_segment.segment, + ) + if self.feature_segment + else None + ) + clone.environment = env + clone.save() + # clone the related objects + self.feature_state_value.clone(clone) + return clone + def get_feature_state_value(self, identity: "Identity" = None) -> typing.Any: feature_state_value = ( self.get_multivariate_feature_state_value(identity) diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index fcfb7940912f..9ef7f87b1361 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -84,15 +84,13 @@ def sdk_client(environment_api_key): @pytest.fixture() def feature(admin_client, project): - default_value = "This is a value" data = { "name": "test feature", - "initial_value": default_value, + "initial_value": "default_value", "project": project, } url = reverse("api-v1:projects:project-features-list", args=[project]) - # When response = admin_client.post(url, data=data) return response.json()["id"] @@ -106,7 +104,6 @@ def segment(admin_client, project): "rules": [{"type": "ALL", "rules": [], "conditions": []}], } - # When response = admin_client.post( url, data=json.dumps(data), content_type="application/json" ) diff --git a/api/tests/integration/environments/test_clone_environment.py b/api/tests/integration/environments/test_clone_environment.py index 5feb404b69ca..5dd59fb88287 100644 --- a/api/tests/integration/environments/test_clone_environment.py +++ b/api/tests/integration/environments/test_clone_environment.py @@ -5,12 +5,7 @@ from tests.integration.helpers import get_env_feature_states_list_with_api from environments.permissions.models import UserEnvironmentPermission -from features.models import ( - Feature, - FeatureSegment, - FeatureState, - FeatureStateValue, -) +from features.models import FeatureSegment, FeatureState def test_clone_environment_returns_200( @@ -93,7 +88,7 @@ def test_clone_environment_clones_feature_states_with_value( ) -def test_clone_environment_creates_permission_with_the_current_user( +def test_clone_environment_creates_permission_object_with_the_current_user( admin_user, admin_client, environment, environment_api_key ): # Given @@ -107,7 +102,7 @@ def test_clone_environment_creates_permission_with_the_current_user( assert res.status_code == status.HTTP_200_OK assert ( UserEnvironmentPermission.objects.filter( - user=admin_user, environment_id=environment + user=admin_user, environment_id=environment, admin=True ).count() == 1 ) @@ -178,7 +173,7 @@ def test_clone_clones_segments_overrides( }, ) - # Then fetch the feature state of clone environment for the feature segement + # Then, fetch the feature state of clone environment for the feature segement clone_feature_segment_id = FeatureSegment.objects.get( feature_id=feature, environment_id=clone_env_id, segment=segment ).id From 5862fc7aa6b62e4f41c09cfc9d9edcc4c7454b77 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 9 Aug 2021 18:34:12 +0530 Subject: [PATCH 16/18] tests(env_clone): remove orm calls with api calls --- .../environments/test_clone_environment.py | 69 ++++++++----------- api/tests/integration/helpers.py | 22 +++++- 2 files changed, 47 insertions(+), 44 deletions(-) diff --git a/api/tests/integration/environments/test_clone_environment.py b/api/tests/integration/environments/test_clone_environment.py index 5dd59fb88287..dfa486ac08bc 100644 --- a/api/tests/integration/environments/test_clone_environment.py +++ b/api/tests/integration/environments/test_clone_environment.py @@ -2,42 +2,27 @@ from django.urls import reverse from rest_framework import status -from tests.integration.helpers import get_env_feature_states_list_with_api - -from environments.permissions.models import UserEnvironmentPermission -from features.models import FeatureSegment, FeatureState - - -def test_clone_environment_returns_200( - admin_client, project, environment, environment_api_key -): - # Given - env_name = "Cloned env" - url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) - - # When - res = admin_client.post(url, {"name": env_name}) - - # Then - assert res.status_code == status.HTTP_200_OK - assert res.json()["name"] == env_name +from tests.integration.helpers import ( + get_env_feature_states_list_with_api, + get_feature_segement_list_with_api, +) def test_clone_environment_clones_feature_states_with_value( admin_client, project, environment, environment_api_key, feature ): - # Firstly, create an environment and update it's feature state + # Firstly, Update feature state value of the source enviroment + # fetch the feature state id to update + feature_state = get_env_feature_states_list_with_api( + admin_client, {"enviroment": environment, "feature": feature} + )["results"][0]["id"] - # Fetch the feature state id to update - feature_state = FeatureState.objects.get( - environment_id=environment, feature=feature - ) fs_update_url = reverse( - "api-v1:features:featurestates-detail", args=[feature_state.id] + "api-v1:features:featurestates-detail", args=[feature_state] ) data = { - "id": feature_state.id, + "id": feature_state, "feature_state_value": "new_value", "enabled": False, "feature": feature, @@ -45,6 +30,7 @@ def test_clone_environment_clones_feature_states_with_value( "identity": None, "feature_segment": None, } + # Update the feature state admin_client.put( fs_update_url, data=json.dumps(data), content_type="application/json" ) @@ -88,24 +74,24 @@ def test_clone_environment_clones_feature_states_with_value( ) -def test_clone_environment_creates_permission_object_with_the_current_user( +def test_clone_environment_creates_admin_permission_with_the_current_user( admin_user, admin_client, environment, environment_api_key ): - # Given + # Firstly, let's create the clone of the enviroment env_name = "Cloned env" url = reverse("api-v1:environments:environment-clone", args=[environment_api_key]) - - # When res = admin_client.post(url, {"name": env_name}) + clone_env_api_key = res.json()["api_key"] - # Then - assert res.status_code == status.HTTP_200_OK - assert ( - UserEnvironmentPermission.objects.filter( - user=admin_user, environment_id=environment, admin=True - ).count() - == 1 + # Now, fetch the permission of the newly creatd enviroment + perm_url = reverse( + "api-v1:environments:environment-user-permissions-list", + args=[clone_env_api_key], ) + response = admin_client.get(perm_url) + + # Then, assert that current user is admin + assert response.json()[0]["admin"] is True def test_env_clone_creates_feature_segment( @@ -131,7 +117,7 @@ def test_env_clone_creates_feature_segment( assert json_response["results"][0]["id"] != feature_segment -def test_clone_clones_segments_overrides( +def test_env_clone_clones_segments_overrides( admin_client, environment, environment_api_key, feature, feature_segment, segment ): # Firstly, Let's override the segment in source environment @@ -174,9 +160,10 @@ def test_clone_clones_segments_overrides( ) # Then, fetch the feature state of clone environment for the feature segement - clone_feature_segment_id = FeatureSegment.objects.get( - feature_id=feature, environment_id=clone_env_id, segment=segment - ).id + clone_feature_segment_id = get_feature_segement_list_with_api( + admin_client, + {"environment": res.json()["id"], "feature": feature, "segment": segment}, + )["results"][0]["id"] clone_env_feature_states = get_env_feature_states_list_with_api( admin_client, { diff --git a/api/tests/integration/helpers.py b/api/tests/integration/helpers.py index 8dd662a17888..66e3b76dcf38 100644 --- a/api/tests/integration/helpers.py +++ b/api/tests/integration/helpers.py @@ -54,7 +54,7 @@ def create_feature_with_api( def get_env_feature_states_list_with_api(client: APIClient, query_params: dict) -> dict: """ - Return the feature states using the provided test client. + Returns feature states using the provided test client. :param client: DRF api client to use to make the request :param query_params: A Mapping object used as query params for filtering @@ -63,7 +63,23 @@ def get_env_feature_states_list_with_api(client: APIClient, query_params: dict) url = reverse( "api-v1:features:featurestates-list", ) + return get_json_response(client, url, query_params) + + +def get_feature_segement_list_with_api(client: APIClient, query_params: dict) -> dict: + """ + Return feature segments using the provided test client. + + :param client: DRF api client to use to make the request + :param query_params: A Mapping object used as query params for filtering + + """ + + url = reverse("api-v1:features:feature-segment-list") + return get_json_response(client, url, query_params) + + +def get_json_response(client: APIClient, url: str, query_params: dict) -> dict: if query_params: url = f"{url}?{urlencode(query_params)}" - response = client.get(url) - return response.json() + return client.get(url).json() From 763ce8b8774e76f8357bf3c45821049d06b7176a Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 9 Aug 2021 18:45:12 +0530 Subject: [PATCH 17/18] squash! tests(env_clone): remove orm calls with api calls --- .../environments/test_clone_environment.py | 11 +++++++---- api/tests/integration/helpers.py | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/api/tests/integration/environments/test_clone_environment.py b/api/tests/integration/environments/test_clone_environment.py index dfa486ac08bc..ce38c29c6f79 100644 --- a/api/tests/integration/environments/test_clone_environment.py +++ b/api/tests/integration/environments/test_clone_environment.py @@ -12,7 +12,7 @@ def test_clone_environment_clones_feature_states_with_value( admin_client, project, environment, environment_api_key, feature ): - # Firstly, Update feature state value of the source enviroment + # Firstly, let's update feature state value of the source enviroment # fetch the feature state id to update feature_state = get_env_feature_states_list_with_api( admin_client, {"enviroment": environment, "feature": feature} @@ -120,7 +120,7 @@ def test_env_clone_creates_feature_segment( def test_env_clone_clones_segments_overrides( admin_client, environment, environment_api_key, feature, feature_segment, segment ): - # Firstly, Let's override the segment in source environment + # Firstly, let's override the segment in source environment create_url = reverse("api-v1:features:featurestates-list") data = { "feature_state_value": { @@ -149,7 +149,7 @@ def test_env_clone_clones_segments_overrides( clone_env_id = res.json()["id"] - # Then, fetch the feature state of source environment for the feature segement + # Then, fetch the feature state of source environment source_env_feature_states = get_env_feature_states_list_with_api( admin_client, { @@ -159,11 +159,14 @@ def test_env_clone_clones_segments_overrides( }, ) - # Then, fetch the feature state of clone environment for the feature segement + # Then, fetch the feature state of clone environment + + # fetch the feature segment id to filter feature states clone_feature_segment_id = get_feature_segement_list_with_api( admin_client, {"environment": res.json()["id"], "feature": feature, "segment": segment}, )["results"][0]["id"] + clone_env_feature_states = get_env_feature_states_list_with_api( admin_client, { diff --git a/api/tests/integration/helpers.py b/api/tests/integration/helpers.py index 66e3b76dcf38..702ff0e913b5 100644 --- a/api/tests/integration/helpers.py +++ b/api/tests/integration/helpers.py @@ -68,7 +68,7 @@ def get_env_feature_states_list_with_api(client: APIClient, query_params: dict) def get_feature_segement_list_with_api(client: APIClient, query_params: dict) -> dict: """ - Return feature segments using the provided test client. + Returns feature segments using the provided test client. :param client: DRF api client to use to make the request :param query_params: A Mapping object used as query params for filtering From 87c0803ecf342fddb464cd2c2c9b927eb4a53ff1 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 10 Aug 2021 08:18:07 +0530 Subject: [PATCH 18/18] minor style fixes --- api/tests/integration/environments/test_clone_environment.py | 5 ++--- api/tests/integration/helpers.py | 4 +--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/api/tests/integration/environments/test_clone_environment.py b/api/tests/integration/environments/test_clone_environment.py index ce38c29c6f79..a302e70a6cfa 100644 --- a/api/tests/integration/environments/test_clone_environment.py +++ b/api/tests/integration/environments/test_clone_environment.py @@ -159,14 +159,13 @@ def test_env_clone_clones_segments_overrides( }, ) - # Then, fetch the feature state of clone environment - - # fetch the feature segment id to filter feature states + # (fetch the feature segment id to filter feature states) clone_feature_segment_id = get_feature_segement_list_with_api( admin_client, {"environment": res.json()["id"], "feature": feature, "segment": segment}, )["results"][0]["id"] + # Then, fetch the feature state of clone environment clone_env_feature_states = get_env_feature_states_list_with_api( admin_client, { diff --git a/api/tests/integration/helpers.py b/api/tests/integration/helpers.py index 702ff0e913b5..57b0aa71d9a1 100644 --- a/api/tests/integration/helpers.py +++ b/api/tests/integration/helpers.py @@ -60,9 +60,7 @@ def get_env_feature_states_list_with_api(client: APIClient, query_params: dict) :param query_params: A Mapping object used as query params for filtering """ - url = reverse( - "api-v1:features:featurestates-list", - ) + url = reverse("api-v1:features:featurestates-list") return get_json_response(client, url, query_params)