feat(cohorts): add environment cohort CRUD API - #8248
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds environment-scoped cohort CRUD endpoints with Startup plan and environment permission checks. Adds cohort creation with linked segments, rules, and conditions. Adds deferred deletion that drains memberships before removing the cohort and segment. Excludes deletion-pending cohorts from listings. Prevents changes to cohort-managed segments. Adds database migration, OpenAPI definitions, event catalogue entries, and unit tests. Estimated code review effort: 4 (Complex) | ~60 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8248 +/- ##
==========================================
- Coverage 98.72% 98.67% -0.05%
==========================================
Files 1543 1565 +22
Lines 61683 62167 +484
==========================================
+ Hits 60895 61344 +449
- Misses 788 823 +35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
feature
github-actions
Bot
added
docs
github-actions
Bot
added
docs
github-actions
Bot
added
docs
github-actions
Bot
removed
feature
Docker builds report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 42822bd2-7789-4d4b-a766-f0fcf54c1e7b
📒 Files selected for processing (17)
api/cohorts/migrations/0002_cohort_deletion_requested_at.pyapi/cohorts/models.pyapi/cohorts/permissions.pyapi/cohorts/serializers.pyapi/cohorts/services.pyapi/cohorts/tasks.pyapi/cohorts/urls.pyapi/cohorts/views.pyapi/environments/urls.pyapi/segments/views.pyapi/tests/unit/cohorts/test_permissions.pyapi/tests/unit/cohorts/test_services.pyapi/tests/unit/cohorts/test_tasks.pyapi/tests/unit/cohorts/test_views.pyapi/tests/unit/segments/test_unit_segments_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdopenapi.yaml
| 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, | ||
| ) |
There was a problem hiding this comment.
🗄️ 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:
- 1: https://docs.djangoproject.com/en/5.0/ref/models/instances/
- 2: https://code.djangoproject.com/ticket/34242
- 3: https://docs.djangoproject.com/en/5.0/_modules/django/db/models/deletion/
- 4: https://code.djangoproject.com/ticket/22089
🏁 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:
- 1: https://docs.djangoproject.com/en/6.0/ref/models/instances/
- 2: https://docs.djangoproject.com/en/dev/ref/models/instances/
- 3: https://docs.djangoproject.com/en/6.0/ref/models/fields/
- 4: https://docs.djangoproject.com/en/5.1/ref/models/instances/
- 5: https://django.readthedocs.io/en/latest/ref/models/instances.html
- 6: https://code.djangoproject.com/ticket/22089
- 7: https://code.djangoproject.com/ticket/34242
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.
| 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, | |
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🧩 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.
| responses: | ||
| '204': | ||
| description: No response body |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document the actual delete response status.
CohortViewSet.destroy returns HTTP 202, but this contract documents HTTP 204. Update the specification so generated clients accept the deletion-request response.
Proposed fix
- '204':
- description: No response body
+ '202':
+ description: Deletion requested📝 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.
| responses: | |
| '204': | |
| description: No response body | |
| responses: | |
| '202': | |
| description: Deletion requested |
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19147 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19147 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19147 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19147 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19145 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19145 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19145 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19145 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7019dcdd-b1c1-4707-9d27-04040f0be7c2
📒 Files selected for processing (7)
api/cohorts/services.pyapi/cohorts/tasks.pyapi/cohorts/views.pyapi/tests/unit/cohorts/test_services.pyapi/tests/unit/cohorts/test_tasks.pyapi/tests/unit/cohorts/test_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
github-actions
Bot
added
feature
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to cohort sync (follow-up to #8211, #8212, #8213).
Adds the environment-scoped cohort CRUD API:
POST/GET/DELETEon/api/v1/environments/<api_key>/cohorts/; create builds a managed segment with a singleIS_SETcondition on the cohort's system trait key.pending_removeand the applier finishes the soft delete once drained (deletion_requested_atmarks the wait).DynamoNotEnabledError(edge-identities precedent) — core/Postgres identity support branches out later.require_minimum_plan; self-hosted deployments are exempt as usual), plusMANAGE_SEGMENTSto create/delete.cohort.created/cohort.deletion_requested/cohort.deletedevents.How did you test this code?
Unit tests for the create/delete services, applier task deletion branch, views, permissions (including plan gating via
saas_mode), and segment edit protection; the full segments view suite still passes.