Skip to content

feat: enable feature versioning for environments in newly created projects #5108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions api/app/settings/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1242,6 +1242,7 @@
FLAGSMITH_ON_FLAGSMITH_SERVER_API_URL = env(
"FLAGSMITH_ON_FLAGSMITH_SERVER_API_URL", default=FLAGSMITH_ON_FLAGSMITH_API_URL
)
USE_GLOBAL_FLAGSMITH_CLIENTS = True

FLAGSMITH_ON_FLAGSMITH_FEATURE_EXPORT_ENVIRONMENT_ID = env.int(
"FLAGSMITH_ON_FLAGSMITH_FEATURE_EXPORT_ENVIRONMENT_ID",
Expand Down
2 changes: 2 additions & 0 deletions api/app/settings/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,5 @@
ENABLE_POSTPONE_DECORATOR = False

DEBUG = True

USE_GLOBAL_FLAGSMITH_CLIENTS = False
19 changes: 19 additions & 0 deletions api/environments/models.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import datetime
import logging
import typing
import uuid
Expand All @@ -15,6 +16,7 @@
AFTER_DELETE,
AFTER_SAVE,
AFTER_UPDATE,
BEFORE_CREATE,
LifecycleModel,
hook,
)
Expand Down Expand Up @@ -49,6 +51,7 @@
)
from features.models import Feature, FeatureSegment, FeatureState
from features.multivariate.models import MultivariateFeatureStateValue
from integrations.flagsmith.client import get_client
from metadata.models import Metadata
from projects.models import Project
from segments.models import Segment
Expand Down Expand Up @@ -195,6 +198,22 @@ def delete_environment_document_from_cache(self) -> None:
):
environment_document_cache.delete(self.api_key)

@hook(BEFORE_CREATE) # type: ignore[misc]
def enable_v2_versioning(self) -> None:
flagsmith_client = get_client("local", local_eval=True)
organisation = self.project.organisation
flag = flagsmith_client.get_identity_flags(
organisation.flagsmith_identifier,
traits={"organisation_id": organisation.id},
).get_flag("enable_feature_versioning_for_new_projects")

if (
flag.enabled
and self.project.created_date.date()
>= datetime.date.fromisoformat(str(flag.value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't think I clearly understand the reason to grandfather in existing projects vs. just scheduling a change request for this flag. What are the implications of having versioned and non-versioned environments in the same project? If we want to avoid it, why not have a project-level setting to enable v2 versioning?
  2. The flag value usage here is questionable. Enabling the flag without setting a correct value will basically break Flagsmith. Can we not make FoF more fragile please?

Side note: to me, this code is a good example of current limitations in Flagsmith's segmenting; if the engine supported datetime comparisons, this could've been

        flag = flagsmith_client.get_identity_flags(
            organisation.flagsmith_identifier,
            traits={
                "organisation_id": organisation.id,
                "project_created_date": {
                    "value": self.project.created_date,
                    "transient": True,
                 }
            },
        ).get_flag("enable_feature_versioning_for_new_projects")

or, if we had multi-context support, an even better

        flag = flagsmith_client.get_identity_flags(
            organisation.flagsmith_identifier,
            contexts={
                "default": {"organisation_id": organisation.id},
                "project": {
                    "created_date": self.project.created_date,
                },
            },
        ).get_flag("enable_feature_versioning_for_new_projects")

— both of the above are much safer and clearer than the code I'm reviewing.

):
self.use_v2_feature_versioning = True

def __str__(self): # type: ignore[no-untyped-def]
return "Project %s - Environment %s" % (self.project.name, self.name)

Expand Down
16 changes: 9 additions & 7 deletions api/integrations/flagsmith/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@
def get_client(name: str = "default", local_eval: bool = False) -> Flagsmith:
global _flagsmith_clients

try:
_flagsmith_client = _flagsmith_clients[name]
except (KeyError, TypeError):
kwargs = _get_client_kwargs()
kwargs["enable_local_evaluation"] = local_eval
_flagsmith_client = Flagsmith(**kwargs)
_flagsmith_clients[name] = _flagsmith_client
if settings.USE_GLOBAL_FLAGSMITH_CLIENTS and (
client := _flagsmith_clients.get(name)
):
Comment on lines +28 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use global Flagsmith clients? If it's just for tests, I'd prefer a fixture mocking them than a Django setting. This will also solve the coverage issue.

Copy link
Contributor Author

@matthewelwell matthewelwell Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, TBH, I have no idea why this logic exists. I wrote it far too long ago to remember the context. When I came back to it yesterday, I also hated it :)

return client

kwargs = _get_client_kwargs()
kwargs["enable_local_evaluation"] = local_eval
_flagsmith_client = Flagsmith(**kwargs)
_flagsmith_clients[name] = _flagsmith_client

return _flagsmith_client

Expand Down
7 changes: 7 additions & 0 deletions api/tests/types.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import typing
from typing import Callable, Literal

from environments.permissions.models import UserEnvironmentPermission
Expand All @@ -17,3 +18,9 @@
]

AdminClientAuthType = Literal["user", "master_api_key"]


class TestFlagData(typing.NamedTuple):
feature_name: str
enabled: bool
value: typing.Any
48 changes: 48 additions & 0 deletions api/tests/unit/conftest.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,24 @@
import typing
from unittest.mock import MagicMock

import pytest
from django.core.cache import BaseCache
from flag_engine.environments.models import EnvironmentModel
from flag_engine.features.models import FeatureModel, FeatureStateModel
from flag_engine.organisations.models import OrganisationModel
from flag_engine.projects.models import ProjectModel
from flagsmith.offline_handlers import BaseOfflineHandler
from pytest_django.fixtures import SettingsWrapper
from pytest_mock import MockerFixture

from environments.enums import EnvironmentDocumentCacheMode
from environments.models import Environment
from features.feature_types import STANDARD
from features.models import Feature
from organisations.models import Organisation, OrganisationRole
from projects.models import Project
from projects.tags.models import Tag
from tests.types import TestFlagData
from users.models import FFAdminUser
from util.mappers import map_environment_to_environment_document

Expand Down Expand Up @@ -232,3 +240,43 @@ def populate_environment_document_cache(
persistent_environment_document_cache.get.return_value = (
map_environment_to_environment_document(environment)
)


@pytest.fixture()
def set_flagsmith_client_flags(
mocker: MockerFixture,
) -> typing.Callable[[list[TestFlagData]], None]:
Comment on lines +246 to +248
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I'm curious to see this fixture used and evolved. IMO we can include it as a pytest plugin along with the Python client when it's mature enough.

class TestOfflineHandler(BaseOfflineHandler):
def __init__(self) -> None:
_organisation_model = OrganisationModel(id=1, name="flagsmith-organisation")
_project_model = ProjectModel(
id=1, name="flagsmith-project", organisation=_organisation_model
)
self.environment = EnvironmentModel(
id=1, api_key="test-key", project=_project_model
)

def set_flags(self, flags: list[TestFlagData]) -> None:
self.environment.feature_states = [
FeatureStateModel(
feature=FeatureModel(
id=i, name=flag_data.feature_name, type=STANDARD
),
enabled=flag_data.enabled,
feature_state_value=flag_data.value,
)
for i, flag_data in enumerate(flags)
]

def get_environment(self) -> EnvironmentModel:
return self.environment

offline_handler = TestOfflineHandler()
mocker.patch(
"integrations.flagsmith.client.LocalFileHandler", return_value=offline_handler
)

def _setter(flags: list[TestFlagData]) -> None:
offline_handler.set_flags(flags)

return _setter
64 changes: 64 additions & 0 deletions api/tests/unit/environments/test_unit_environments_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from organisations.models import Organisation, OrganisationRole
from projects.models import EdgeV2MigrationStatus, Project
from segments.models import Segment
from tests.types import TestFlagData
from users.models import FFAdminUser
from util.mappers import map_environment_to_environment_document

Expand Down Expand Up @@ -1208,3 +1209,66 @@ def test_environment_metric_query_helpers_match_expected_counts(
assert change_request_count_result == change_request_count
assert scheduled_count_result == scheduled_change_count
assert identity_override_count == 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'd like to see

a) a test with project with created_date < flag value
b) a test with an empty/non-date flag value


def test_environment_create_with_use_v2_feature_versioning_true(
project: Project,
environment_v2_versioning: Environment,
feature: Feature,
set_flagsmith_client_flags: typing.Callable[[list[TestFlagData]], None],
) -> None:
# Given
set_flagsmith_client_flags(
[TestFlagData("enable_feature_versioning_for_new_projects", True, "2025-02-17")]
)

# When
new_environment = Environment.objects.create(
name="new-environment",
project=project,
)

# Then
assert EnvironmentFeatureVersion.objects.filter(
environment=new_environment, feature=feature
).exists()


def test_environment_clone_from_versioned_environment_with_use_v2_feature_versioning_true(
project: Project,
environment_v2_versioning: Environment,
feature: Feature,
set_flagsmith_client_flags: typing.Callable[[list[TestFlagData]], None],
) -> None:
# Given
set_flagsmith_client_flags(
[TestFlagData("enable_feature_versioning_for_new_projects", True, "2025-02-17")]
)

# When
new_environment = environment_v2_versioning.clone(name="new-environment")

# Then
assert EnvironmentFeatureVersion.objects.filter(
environment=new_environment, feature=feature
).exists()


def test_environment_clone_from_non_versioned_environment_with_use_v2_feature_versioning_true(
project: Project,
environment: Environment,
feature: Feature,
set_flagsmith_client_flags: typing.Callable[[list[TestFlagData]], None],
) -> None:
# Given
set_flagsmith_client_flags(
[TestFlagData("enable_feature_versioning_for_new_projects", True, "2025-02-17")]
)

# When
new_environment = environment.clone(name="new-environment")

# Then
assert not EnvironmentFeatureVersion.objects.filter(
environment=new_environment, feature=feature
).exists()
Loading