-
Notifications
You must be signed in to change notification settings - Fork 556
feat(cohorts): add environment cohort CRUD API #8248
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
Open
gagantrivedi
wants to merge
16
commits into
main
Choose a base branch
from
feat/cohort-crud
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
fd5ac62
feat(cohorts): add environment cohort CRUD API
gagantrivedi a6c9ff0
chore: Update documentation artefacts
flagsmith-engineering[bot] 134630d
refactor(cohorts): remove audit logs for now
gagantrivedi 08e4f78
docs(cohorts): make deletion drain comment store-agnostic
gagantrivedi fab0b11
feat(cohorts): require startup plan for cohort API
gagantrivedi 96a3e9f
refactor(cohorts): declare plan gate on the viewset
gagantrivedi b9daea5
chore: Update documentation artefacts
flagsmith-engineering[bot] 58481a0
refactor(cohorts): drop uninformative viewset docstring
gagantrivedi d4038e2
chore: Update documentation artefacts
flagsmith-engineering[bot] ee03574
feat(cohorts): reject non-edge projects with DynamoNotEnabledError
gagantrivedi b750237
fix(cohorts): log deletion request before enqueue and describe API sc…
gagantrivedi 34b34ce
chore: Update documentation artefacts
flagsmith-engineering[bot] 18ce5c8
fix(cohorts): make deletion state change and task enqueue one transac…
gagantrivedi a81436e
feat(segments): mark cohort-managed segments with managed_by
gagantrivedi 35eefe2
chore: Update documentation artefacts
flagsmith-engineering[bot] 42e69f5
feat(segments): block cloning cohort-managed segments
gagantrivedi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
18 changes: 18 additions & 0 deletions
18
api/cohorts/migrations/0002_cohort_deletion_requested_at.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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), | ||
| ), | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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"], | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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, | ||
| ): | ||
|
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() | ||
|
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) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
| ), | ||
| ), | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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:
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 toNone; line 132 therefore emitscohort.deletedwithcohort__id=None. Capture the identifier before deletion and use it for the event. The tests comparecohort.deletedagainst the deleted instance primary key, so they also do not catch this.Proposed fix
📝 Committable suggestion
There was a problem hiding this comment.
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 (setsdeleted_atand saves) — the pk survives.test_apply_cohort_membership_deltas__deletion_requested__finalises_after_drainasserts the emittedcohort__idagainst an instance the task never loaded, which would fail if the pk were cleared.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 15299
@gagantrivedi, confirmed.Cohortinherits fromSoftDeleteExportableModel, so the hard-delete assumption was incorrect. Thecohort.deletedevent can usecohort.idaftercohort.delete().✏️ Learnings added
You are interacting with an AI system.