feat(Feature Versioning): add multi variate capabilities to the experimental update-flag endpoints#7955
feat(Feature Versioning): add multi variate capabilities to the experimental update-flag endpoints#7955emyller wants to merge 14 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request renames the update-flag endpoints, serializers, and services to Option A and Option B. It adds typed payload schemas and change-set dataclasses for feature values, multivariate options, and segment overrides. Versioning now uses shared helpers for feature-state and multivariate updates. Rollout code, routing, views, tests, OpenAPI schemas, and documentation are updated. Estimated code review effort: 4 (Complex) | ~60 minutes Comment |
420e419 to
d64e257
Compare
d64e257 to
23e4464
Compare
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/features/feature_states/serializers.py (1)
238-254: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winN+1 query in
validate_segment_overrides.One
Segment.objects.filter(...).exists()query per segment override viavalidate_segment_id. Already flagged with a TODO — worth batching into a singlefilter(id__in=segment_ids, project_id=...).count()check once out of experimentation, as noted.♻️ Possible batched check
- for segment_id in segment_ids: - self.validate_segment_id(segment_id) + valid_count = Segment.objects.filter( + id__in=segment_ids, project_id=_environment(self.context).project_id + ).count() + if valid_count != len(segment_ids): + raise serializers.ValidationError( + "One or more segments not found in project" + )Happy to open this as a follow-up if useful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 39f7d2ea-874a-4876-9a95-d80d6825179b
📒 Files selected for processing (18)
api/api/urls/experiments.pyapi/experimentation/services.pyapi/features/feature_states/serializers.pyapi/features/feature_states/types.pyapi/features/feature_states/views.pyapi/features/versioning/dataclasses.pyapi/features/versioning/versioning_service.pyapi/tests/integration/conftest.pyapi/tests/integration/environments/identities/test_integration_identities.pyapi/tests/integration/features/versioning/test_update_flag_endpoints.pyapi/tests/integration/helpers.pyapi/tests/unit/features/feature_states/test_serializers.pyapi/tests/unit/features/feature_states/test_unit_feature_states_views.pyapi/tests/unit/features/versioning/test_unit_versioning_versioning_service.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
| return attrs | ||
|
|
||
|
|
||
| class UpdateFlagOptionASerializer(BaseFeatureUpdateSerializer[FeatureState]): |
There was a problem hiding this comment.
Option A and Option B are becoming the new semantic for v1 and v2 ?
There was a problem hiding this comment.
To the public, they already are. The refactor here happens for:
- Eliminate ambiguity, i.e. flag-update v1/v2 over feature versioning v1/v2.
- Make it consistent with user docs.
- Improve clarity of naming of the backend components.
👻
Only certain spirits are expected to have different names depending on the realm they're in, but these are just code.There was a problem hiding this comment.
Ah ok, I didn't have context over the public documentation. Makes sense, thanks for the clarification. I thought on the opposite this changes were a specific case of renaming versioning v1 versus v2.
Just a NIT then if I may:
It looks like this example of the doc is missing the second request in Option A

If my understanding is correct, the override should be updated in a separate request. If my understanding is wrong please ignore 👍
Feel free to resolve this thread
There was a problem hiding this comment.
Thanks. Option A here is indeed one request, but the docs make it confusing. I've improved things with 42ea717 — also getting rid of the "highest=1" slop.
Please note these are unrelated to the scope of the PR, though I think the improvement warrants piggybacking.
c4e9676 to
9d8dc5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ed23c6d-926a-4bdb-9501-ab0991c865b6
📒 Files selected for processing (18)
api/api/urls/experiments.pyapi/experimentation/services.pyapi/features/feature_states/serializers.pyapi/features/feature_states/types.pyapi/features/feature_states/views.pyapi/features/versioning/dataclasses.pyapi/features/versioning/versioning_service.pyapi/tests/integration/conftest.pyapi/tests/integration/environments/identities/test_integration_identities.pyapi/tests/integration/features/versioning/test_update_flag_endpoints.pyapi/tests/integration/helpers.pyapi/tests/unit/features/feature_states/test_serializers.pyapi/tests/unit/features/feature_states/test_unit_feature_states_views.pyapi/tests/unit/features/versioning/test_unit_versioning_versioning_service.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.md (1)
235-238: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify
multivariate_optionsomission semantics.
State that omitting the field leaves existing options unchanged, while providing the array replaces the current set and deletes any omitted options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f88b0d43-16e9-4175-9878-67217361caf2
📒 Files selected for processing (7)
api/features/feature_states/serializers.pyapi/features/feature_states/types.pyapi/features/models.pyapi/features/versioning/dataclasses.pyapi/features/versioning/versioning_service.pyapi/tests/integration/features/versioning/test_update_flag_endpoints.pydocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.md
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7642
How did you test this code?
Unit tests.
How to review
4/5 raw effort.
3/5 if read commit-by-commit. Each is concern-isolated, and stack nicely.