Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions api/cohorts/migrations/0002_cohort_deletion_requested_at.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 5.2.16 on 2026-08-06 07:03

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("cohorts", "0001_initial"),
]

operations = [
migrations.AddField(
model_name="cohort",
name="deletion_requested_at",
field=models.DateTimeField(blank=True, null=True),
),
]
3 changes: 3 additions & 0 deletions api/cohorts/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ class Cohort(SoftDeleteExportableModel):
)
version = models.PositiveIntegerField(default=0)
created_at = models.DateTimeField(auto_now_add=True)
# Deletion drains memberships from the identity store first; the cohort is
# only soft-deleted once drained. This marks it as awaiting that final step.
deletion_requested_at = models.DateTimeField(null=True, blank=True)

@property
def system_trait_key(self) -> str:
Expand Down
48 changes: 48 additions & 0 deletions api/cohorts/permissions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
from common.environments.permissions import VIEW_ENVIRONMENT
from common.projects.permissions import MANAGE_SEGMENTS
from rest_framework.permissions import BasePermission
from rest_framework.request import Request
from rest_framework.views import APIView

from environments.models import Environment
from organisations.subscriptions.constants import SubscriptionPlanFamily
from organisations.subscriptions.permissions import require_minimum_plan
from users.models import FFAdminUser

_READ_ACTIONS = ("list", "retrieve")

_MinimumStartupPlan = require_minimum_plan(SubscriptionPlanFamily.START_UP)


class CohortPlanPermission(_MinimumStartupPlan): # type: ignore[misc,valid-type]
def has_permission(self, request: Request, view: APIView) -> bool:
try:
environment = Environment.objects.get(
api_key=view.kwargs.get("environment_api_key")
)
except Environment.DoesNotExist:
return False
# The base class reads the organisation from an `organisation` request
# param our URLs don't carry; the project provides it instead.
return bool(super().has_object_permission(request, view, environment.project))

def has_object_permission(
self, request: Request, view: APIView, obj: object
) -> bool:
# DRF hands us a Cohort here, which doesn't carry an organisation;
# re-run the environment-based check instead.
return self.has_permission(request, view)


class CohortPermission(BasePermission):
def has_permission(self, request: Request, view: APIView) -> bool:
try:
environment = Environment.objects.get(
api_key=view.kwargs.get("environment_api_key")
)
except Environment.DoesNotExist:
return False
user: FFAdminUser = request.user # type: ignore[assignment]
if getattr(view, "action", None) in _READ_ACTIONS:
return user.has_environment_permission(VIEW_ENVIRONMENT, environment)
return user.has_project_permission(MANAGE_SEGMENTS, environment.project)
29 changes: 29 additions & 0 deletions api/cohorts/serializers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import typing

from rest_framework import serializers

from cohorts.models import Cohort
from cohorts.services import create_cohort


class CohortSerializer(serializers.ModelSerializer[Cohort]):
name = serializers.CharField(max_length=2000, source="segment.name")

class Meta:
model = Cohort
fields = (
"id",
"uuid",
"name",
"segment",
"source_type",
"version",
"created_at",
)
read_only_fields = ("segment", "source_type", "version", "created_at")

def create(self, validated_data: dict[str, typing.Any]) -> Cohort:
return create_cohort(
environment=validated_data["environment"],
name=validated_data["segment"]["name"],
)
70 changes: 70 additions & 0 deletions api/cohorts/services.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,22 @@
import typing

import structlog
from django.db import transaction
from django.db.models import QuerySet
from django.utils import timezone
from flag_engine.segments.constants import IS_SET

from cohorts.constants import COHORT_MEMBERSHIP_APPLY_BATCH_SIZE
from cohorts.metrics import flagsmith_cohorts_membership_deltas_applied_total
from cohorts.models import Cohort, CohortMembership, CohortMembershipState
from core.dataclasses import AuthorData
from environments.dynamodb import DynamoIdentityWrapper
from segments.models import Condition, Segment, SegmentManagedBy, SegmentRule
from segments.services import delete_segment

if typing.TYPE_CHECKING:
from environments.models import Environment
from projects.models import Project

logger = structlog.get_logger("cohorts")

Expand Down Expand Up @@ -66,3 +77,62 @@ def apply_pending_memberships(cohort: Cohort) -> bool:
removes__count=removed_count,
)
return pending_memberships(cohort).exists()


def create_cohort(*, environment: "Environment", name: str) -> Cohort:
with transaction.atomic():
segment = Segment.objects.create(
name=name,
project=environment.project,
managed_by=SegmentManagedBy.COHORT,
)
rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE)
cohort: Cohort = Cohort.objects.create(environment=environment, segment=segment)
Condition.objects.create(
rule=rule,
operator=IS_SET,
property=cohort.system_trait_key,
created_with_segment=True,
)
logger.info(
"cohort.created",
cohort__id=cohort.id,
segment__id=segment.id,
environment__id=environment.id,
project__id=environment.project_id,
organisation__id=environment.project.organisation_id,
)
return cohort


def edge_sync_enabled(project: "Project") -> bool:
return bool(project.enable_dynamo_db and DynamoIdentityWrapper().is_enabled)


def delete_cohort(cohort: Cohort) -> None:
from cohorts.tasks import apply_cohort_membership_deltas

with transaction.atomic():
cohort.deletion_requested_at = timezone.now()
cohort.save(update_fields=["deletion_requested_at"])
logger.info(
"cohort.deletion_requested",
cohort__id=cohort.id,
environment__id=cohort.environment_id,
)
CohortMembership.objects.filter(cohort=cohort).update(
state=CohortMembershipState.PENDING_REMOVE, updated_at=timezone.now()
)
apply_cohort_membership_deltas.delay(kwargs={"cohort_id": cohort.id})


def finalise_cohort_deletion(cohort: Cohort) -> None:
segment = cohort.segment
with transaction.atomic():
cohort.delete()
delete_segment(segment, AuthorData())
logger.info(
"cohort.deleted",
cohort__id=cohort.id,
environment__id=cohort.environment_id,
)
Comment on lines +129 to +138

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

For Django 5.0, after Model.delete() succeeds, is the deleted model instance's primary key set to None?

💡 Result:

Yes, for Django 5.0, after a model instance's delete method succeeds, the instance's primary key is set to None [1]. According to the official Django documentation, while the Python instance continues to exist after the delete method is called and retains the data in its other fields, the primary key attribute is explicitly set to None [1]. This behavior is implemented in the Django source code to reflect that the object no longer exists in the database [2][3]. Note that this can cause issues if your model's str method or other logic assumes the primary key remains populated after deletion [4].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate files =="
fd -a 'services.py|test.*cohort|cohort.*test' . | sed 's#^\./##' | head -100

echo
echo "== api/cohorts/services.py outline and relevant lines =="
ast-grep outline api/cohorts/services.py --view compact || true
sed -n '1,180p' api/cohorts/services.py | cat -n

echo
echo "== search cohort.deleted and finalise_cohort_deletion tests/usages =="
rg -n "cohort\.deleted|finalise_cohort_deletion|cohort__id|Cohort\.delete|delete_cohort" .

Repository: Flagsmith/flagsmith

Length of output: 10979


🌐 Web query:

documentation docs.djangoproject.com reference model instances after delete primary key set to None

💡 Result:

In Django, when the delete method is called on a model instance, the object is deleted from the database, but the Python instance itself remains in memory [1][2]. As a side effect of this operation, Django sets the instance's primary key attribute (and the pk proxy attribute) to None [1][3]. This behavior is explicitly documented in the official Django Model instance reference [1][4]. The documentation states that after calling delete, the Python instance will continue to exist and retain its field data, with the exception that its primary key is set to None [1][5]. This behavior ensures that the instance is no longer treated as an existing record in the database, preventing accidental attempts to update a non-existent row [1][4]. For developers, this means that any logic relying on the primary key—such as custom str methods or code that checks if an instance is "deleted"—must account for the possibility that the primary key may be None [6]. Top results: [1][7][3]

Citations:


Keep the cohort identifier before deletion.

Model.delete() leaves the Python instance in memory but sets its primary key to None; line 132 therefore emits cohort.deleted with cohort__id=None. Capture the identifier before deletion and use it for the event. The tests compare cohort.deleted against the deleted instance primary key, so they also do not catch this.

Proposed fix
 def finalise_cohort_deletion(cohort: Cohort) -> None:
+    cohort_id = cohort.id
     segment = cohort.segment
     with transaction.atomic():
         cohort.delete()
         delete_segment(segment, AuthorData())
     logger.info(
         "cohort.deleted",
-        cohort__id=cohort.id,
+        cohort__id=cohort_id,
         environment__id=cohort.environment_id,
     )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def finalise_cohort_deletion(cohort: Cohort) -> None:
segment = cohort.segment
with transaction.atomic():
cohort.delete()
delete_segment(segment, AuthorData())
logger.info(
"cohort.deleted",
cohort__id=cohort.id,
environment__id=cohort.environment_id,
)
def finalise_cohort_deletion(cohort: Cohort) -> None:
cohort_id = cohort.id
segment = cohort.segment
with transaction.atomic():
cohort.delete()
delete_segment(segment, AuthorData())
logger.info(
"cohort.deleted",
cohort__id=cohort_id,
environment__id=cohort.environment_id,
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Cohort is a SoftDeleteObject, so .delete() here soft-deletes (sets deleted_at and saves) — the pk survives. test_apply_cohort_membership_deltas__deletion_requested__finalises_after_drain asserts the emitted cohort__id against an instance the task never loaded, which would fail if the pk were cleared.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Cohort model =="
ast-grep outline api/cohorts/models.py --view expanded || true
sed -n '1,220p' api/cohorts/models.py | cat -n

echo
echo "== SoftDeleteObject definitions and delete overrides =="
rg -n -A20 -B8 'class\s+SoftDeleteObject\b|def\s+delete\s*\(' api --glob '*.py'

Length of output: 15299


@gagantrivedi, confirmed. Cohort inherits from SoftDeleteExportableModel, so the hard-delete assumption was incorrect. The cohort.deleted event can use cohort.id after cohort.delete().


✏️ Learnings added
Learnt from: gagantrivedi
Repo: Flagsmith/flagsmith PR: 8248
File: api/cohorts/services.py:124-133
Timestamp: 2026-08-11T04:59:37.878Z
Learning: In `api/cohorts/models.py`, `Cohort` inherits from `core.models.SoftDeleteExportableModel`. Its `delete()` operation soft-deletes the cohort by setting `deleted_at` and saving the model, so `cohort.id` remains available after deletion. Reviews of `api/cohorts/services.py:finalise_cohort_deletion` must not apply Django hard-delete primary-key-clearing behaviour to `Cohort`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

8 changes: 3 additions & 5 deletions api/cohorts/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
DYNAMODB_THROTTLING_ERROR_CODES,
)
from cohorts.models import Cohort
from environments.dynamodb import DynamoIdentityWrapper

logger = structlog.get_logger("cohorts")

Expand All @@ -22,15 +21,14 @@ def apply_cohort_membership_deltas(cohort_id: int) -> None:
if (cohort := Cohort.objects.filter(id=cohort_id).first()) is None:
log.info("membership.apply.skipped", reason="cohort_missing")
return
if not (
cohort.environment.project.enable_dynamo_db
and DynamoIdentityWrapper().is_enabled
):
if not services.edge_sync_enabled(cohort.environment.project):
log.info("membership.apply.skipped", reason="not_edge")
return
try:
for _ in range(COHORT_MEMBERSHIP_APPLY_MAX_BATCHES_PER_RUN):
if not services.apply_pending_memberships(cohort):
if cohort.deletion_requested_at is not None:
services.finalise_cohort_deletion(cohort)
return
except ClientError as exc:
if exc.response["Error"]["Code"] in DYNAMODB_THROTTLING_ERROR_CODES:
Expand Down
10 changes: 10 additions & 0 deletions api/cohorts/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from rest_framework.routers import DefaultRouter

from cohorts.views import CohortViewSet

app_name = "cohorts"

router = DefaultRouter()
router.register(r"", CohortViewSet, basename="cohorts")

urlpatterns = router.urls
64 changes: 64 additions & 0 deletions api/cohorts/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
from django.db.models import QuerySet
from drf_spectacular.utils import extend_schema, extend_schema_view
from rest_framework import mixins, status
from rest_framework.permissions import IsAuthenticated
from rest_framework.request import Request
from rest_framework.response import Response

from cohorts import services
from cohorts.models import Cohort
from cohorts.permissions import CohortPermission, CohortPlanPermission
from cohorts.serializers import CohortSerializer
from environments.views import NestedEnvironmentViewSet
from projects.exceptions import DynamoNotEnabledError


@extend_schema_view(
list=extend_schema(description="List the environment's cohorts."),
create=extend_schema(
description="Create a cohort and the managed segment that targets it."
),
retrieve=extend_schema(description="Retrieve a cohort."),
destroy=extend_schema(
description=(
"Request cohort deletion. Memberships are drained from identity "
"data first; the cohort and its segment are deleted once drained."
),
responses={202: None},
),
)
class CohortViewSet(
NestedEnvironmentViewSet[Cohort],
mixins.ListModelMixin,
mixins.CreateModelMixin,
mixins.RetrieveModelMixin,
mixins.DestroyModelMixin,
):
Comment thread
coderabbitai[bot] marked this conversation as resolved.
serializer_class = CohortSerializer
pagination_class = None
permission_classes = [IsAuthenticated, CohortPlanPermission, CohortPermission]
model_class = Cohort
lookup_field = "id"
lookup_url_kwarg = "cohort_id"

def initial(self, request: Request, *args: object, **kwargs: object) -> None:
super().initial(request, *args, **kwargs)
# Cohorts only sync to edge identities for now; core (Postgres
# identities) support comes later.
if not services.edge_sync_enabled(self._get_environment().project):
raise DynamoNotEnabledError()
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def get_queryset(self) -> QuerySet[Cohort]:
# A cohort awaiting drain-then-delete is already gone from the
# user's point of view.
return (
super()
.get_queryset()
.filter(deletion_requested_at__isnull=True)
.select_related("segment")
.order_by("id")
)

def destroy(self, request: Request, *args: object, **kwargs: object) -> Response:
services.delete_cohort(self.get_object())
return Response(status=status.HTTP_202_ACCEPTED)
4 changes: 4 additions & 0 deletions api/environments/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@
"<str:environment_api_key>/warehouse-connections/",
include("experimentation.urls"),
),
path(
"<str:environment_api_key>/cohorts/",
include("cohorts.urls"),
),
path(
"<str:environment_api_key>/experiments/",
include("experimentation.experiment_urls"),
Expand Down
27 changes: 27 additions & 0 deletions api/segments/migrations/0031_segment_managed_by.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Generated by Django 5.2.16 on 2026-08-11 07:43

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("segments", "0030_add_default_to_segment_version"),
]

operations = [
migrations.AddField(
model_name="historicalsegment",
name="managed_by",
field=models.CharField(
blank=True, choices=[("cohort", "Cohort")], default="", max_length=50
),
),
migrations.AddField(
model_name="segment",
name="managed_by",
field=models.CharField(
blank=True, choices=[("cohort", "Cohort")], default="", max_length=50
),
),
]
10 changes: 10 additions & 0 deletions api/segments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ class SegmentConditionManager(ConfiguredOrderManager["Condition"]):
setting_name = "SEGMENT_CONDITIONS_EXPLICIT_ORDERING_ENABLED"


class SegmentManagedBy(models.TextChoices):
COHORT = "cohort", "Cohort"


class Segment(
LifecycleModelMixin, # type: ignore[misc]
SoftDeleteExportableModel,
Expand Down Expand Up @@ -121,6 +125,12 @@ class Segment(
created_at = models.DateTimeField(null=True, auto_now_add=True)
updated_at = models.DateTimeField(null=True, auto_now=True)
is_system_segment = models.BooleanField(default=False)
# A managed segment is created and maintained by another feature (e.g. a
# cohort). Unlike system segments it stays visible in the API, but the
# dashboard renders it differently and cannot edit it.
managed_by = models.CharField(
max_length=50, choices=SegmentManagedBy.choices, default="", blank=True
)

objects = SegmentManager() # type: ignore[misc]

Expand Down
3 changes: 2 additions & 1 deletion api/segments/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,9 @@ class Meta:
"rules",
"metadata",
"membership_counts",
"managed_by",
]
read_only_fields = ["membership_counts"]
read_only_fields = ["membership_counts", "managed_by"]

def validate(self, attrs: dict[str, Any]) -> dict[str, Any]:
attrs = super().validate(attrs)
Expand Down
11 changes: 11 additions & 0 deletions api/segments/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,17 @@ def members(self, request: Request, *args: Any, **kwargs: Any) -> Response:
next_cursor = members[-1]["identifier"] if has_more else None
return Response({"results": members, "next_cursor": next_cursor})

def check_object_permissions(self, request: Request, obj: Segment) -> None:
super().check_object_permissions(request, obj)
if (
self.action in ("update", "partial_update", "destroy", "clone")
and obj.cohorts.exists()
):
raise PermissionDenied(
"This segment is managed by a cohort and cannot be edited "
"or cloned directly."
)

def destroy(self, request: Request, *args: Any, **kwargs: Any) -> Response:
segment = self.get_object()
author = AuthorData.from_request(request)
Expand Down
Loading
Loading