feat: add feature flag for campaigns #2944
Conversation
WalkthroughAdds a new "campaigns" feature flag across frontend and backend: UI form rendering, feature-flag schema and loader, sidebar visibility mapping, serializer exposure via GlobalSettings, and a serializer-factory/resolution change in the global settings view. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FeatureFlagsViewSet
participant GlobalSettingsSerializerFactory as Factory
participant FeatureFlagsSerializer as Serializer
participant GlobalSettingsDB as GlobalSettings
Client->>FeatureFlagsViewSet: GET /global-settings/feature-flags
FeatureFlagsViewSet->>Factory: get_serializer("<BaseName>", action)
Factory-->>FeatureFlagsViewSet: FeatureFlagsSerializer class
FeatureFlagsViewSet->>Serializer: instantiate(serializer_class, instance)
Serializer->>GlobalSettings: read value (e.g., value.campaigns)
GlobalSettings-->>Serializer: feature flags payload
Serializer-->>FeatureFlagsViewSet: serialized data
FeatureFlagsViewSet-->>Client: 200 OK + serialized feature flags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
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 |
nas-tabchiche
left a comment
nas-tabchiche
left a comment
There was a problem hiding this comment.
Campaigns are a pro-only feature, the feature flag must not be visible in community
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/global_settings/views.py (2)
7-8: SimplifyGlobalSettingsSerializerFactoryand avoid unused parameters
GlobalSettingsSerializerFactorymostly duplicatesSerializerFactory:
__init__is identical to the base implementation; you can simply delegate tosuper().__init__(*modules)and avoid repeating the logic/comment.get_serializer(self, base_name: str, action: str = "default")ignores theactionargument entirely, which makes the signature misleading.You could reduce surface area and future confusion with something like:
class GlobalSettingsSerializerFactory(SerializerFactory): def __init__(self, *modules: str): super().__init__(*modules) def get_serializer(self, base_name: str) -> type: return self._get_serializer_class(f"{base_name}Serializer")or even drop this subclass and use
SerializerFactory+ a small helper where you need it.Also applies to: 18-21, 23-35
64-80: Use DRF’sget_serializer/context and guard against invalid module defaultsA few points around
FeatureFlagsViewSet:
In
retrieve/updateyou’re doingself.get_serializer_class()(instance, ...)directly. This bypasses DRF’sget_serializer, so:
- No
context(e.g.{"request": request, "view": self}) is passed.- You also skip
perform_update()inupdate.It’s safer and more idiomatic to keep DRF’s flow and only customize which class is used. For example:
def get_serializer_class(self, **kwargs): factory = GlobalSettingsSerializerFactory( self.serializers_module, settings.MODULE_PATHS.get("serializers"), ) return factory.get_serializer("FeatureFlags") def retrieve(self, request, *args, **kwargs): instance = self.get_object() serializer = self.get_serializer(instance) return Response(serializer.data) def update(self, request, *args, **kwargs): instance = self.get_object() serializer = self.get_serializer(instance, data=request.data) serializer.is_valid(raise_exception=True) self.perform_update(serializer) return Response(serializer.data)
settings.MODULE_PATHS.get("serializers", [])uses[]as a default, which is not a valid module path. If the key were ever missing,GlobalSettingsSerializerFactorywould receive a non‑string module andimportlib.import_modulewould fail. Either ensure"serializers"is always defined inMODULE_PATHSor filter out falsy/non‑string values before passing them into the factory.Since your
GlobalSettingsSerializerFactory.get_serializerignores itsactionargument, passingkwargs.get("action", "default")is unnecessary noise; consider dropping that argument for clarity.These changes would keep the dynamic enterprise/community resolution while staying aligned with DRF conventions and making the behavior more robust.
Also applies to: 84-85, 89-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/global_settings/views.py(3 hunks)enterprise/backend/enterprise_core/serializers.py(2 hunks)frontend/src/lib/components/Forms/ModelForm/FeatureFlagsSettingForm.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/Forms/ModelForm/FeatureFlagsSettingForm.svelte
🧰 Additional context used
🧬 Code graph analysis (1)
backend/global_settings/views.py (3)
backend/core/serializers.py (9)
SerializerFactory(30-62)get_serializer(41-49)_get_serializer_class(51-62)update(75-83)update(306-315)update(455-467)update(836-852)update(1254-1268)update(1505-1512)backend/global_settings/models.py (1)
GlobalSettings(7-29)backend/core/views.py (10)
get_serializer_class(498-513)get_serializer_class(8545-8546)get(669-676)get(6247-6248)retrieve(838-839)retrieve(5106-5107)update(578-606)update(4679-4691)update(4939-4952)update(8440-8443)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
enterprise/backend/enterprise_core/serializers.py (2)
10-14: Enterprise feature-flag imports are wired correctlyImporting
GlobalSettingsand aliasing the communityFeatureFlagsSerializerfor extension in the enterprise layer looks consistent with the new usage below.
137-177: I'll help you verify the concerns raised in this review comment. Let me start by examining the codebase to understand the context better.
<function_calls>
#!/bin/bashFirst, let's find the file and examine the serializer class
find . -name "serializers.py" -path "/enterprise_core/" | head -5
</function_calls>
#!/bin/bashGet line count and view the relevant section of the file
wc -l enterprise/backend/enterprise_core/serializers.py
</function_calls>
<function_calls>
#!/bin/bashView the FeatureFlagsSerializer and CommunityFeatureFlagSerializer
sed -n '100,200p' enterprise/backend/enterprise_core/serializers.py
</function_calls>
<function_calls>
#!/bin/bashFind CommunityFeatureFlagSerializer definition
rg -n "class CommunityFeatureFlagSerializer" enterprise/backend/
</function_calls>Let me search for the parent serializer and understand the full context:
<function_calls>
#!/bin/bashSearch for CommunityFeatureFlagSerializer in the entire repo
rg -n "CommunityFeatureFlagSerializer" --type py -A 20 | head -80
</function_calls>Now let me check the GlobalSettings model to understand if "name" is actually a field:
<function_calls>
#!/bin/bashFind GlobalSettings model definition
rg -n "class GlobalSettings" --type py -A 30 | head -60
</function_calls>Let me also check how DRF handles the read_only_fields configuration:
<function_calls>
#!/bin/bashSearch for any tests or usages of FeatureFlagsSerializer
rg -n "FeatureFlagsSerializer" --type py -B 2 -A 5
</function_calls>Let me get a clearer view of the complete serializers file around this area:
<function_calls>
#!/bin/bashView lines 1-50 to see imports and parent class
sed -n '1,50p' enterprise/backend/enterprise_core/serializers.py
</function_calls><function_calls>
#!/bin/bashView more of the file to find CommunityFeatureFlagSerializer
sed -n '50,140p' enterprise/backend/enterprise_core/serializers.py
</function_calls>Now let me check what fields are actually on the GlobalSettings model:
<function_calls>
#!/bin/bashFind GlobalSettings model
find . -name "models.py" -path "/enterprise_core/" | xargs grep -l "GlobalSettings"
</function_calls><function_calls>
#!/bin/bashView the GlobalSettings model
rg -n "class GlobalSettings" --type py -A 50 | head -100
</function_calls>Let me check if there are any actual import-time errors or test failures related to this:
<function_calls>
#!/bin/bashSearch for test files that might test this serializer
find . -path "/test" -name "*.py" -type f | xargs grep -l "FeatureFlagsSerializer|GlobalSettings" 2>/dev/null | head -5
</function_calls>Let me view the complete serializer class definition more carefully:
<function_calls>
#!/bin/bashGet the full context around lines 137-177
sed -n '130,185p' enterprise/backend/enterprise_core/serializers.py
</function_calls>Now let me verify the exact field definitions on GlobalSettings:
<function_calls>
#!/bin/bashSearch for GlobalSettings definition more thoroughly
find . -type f -name "*.py" | xargs grep -n "class GlobalSettings" 2>/dev/null
</function_calls><function_calls>
#!/bin/bashView the models.py file containing GlobalSettings
rg -n "class GlobalSettings" -A 60 --type py | head -100
</function_calls>Now let me check the CommunityFeatureFlagSerializer to understand the parent class:
<function_calls>
#!/bin/bashGet full CommunityFeatureFlagSerializer
rg -n "class CommunityFeatureFlagSerializer" -A 80 --type py
</function_calls>Let me search more specifically in the enterprise_core directory:
<function_calls>
#!/bin/bashList all python files in enterprise_core
find enterprise/backend/enterprise_core -name "*.py" -type f
</function_calls><function_calls>
#!/bin/bashView models.py to find GlobalSettings
wc -l enterprise/backend/enterprise_core/models.py
sed -n '1,50p' enterprise/backend/enterprise_core/models.py
</function_calls><function_calls>
#!/bin/bashSearch for GlobalSettings in models
grep -n "class GlobalSettings" enterprise/backend/enterprise_core/models.py
</function_calls><function_calls>
#!/bin/bashGet GlobalSettings class definition
sed -n '1,100p' enterprise/backend/enterprise_core/models.py | grep -A 50 "class GlobalSettings"
</function_calls><function_calls>
#!/bin/bashLet's get the full serializers file to understand the complete context
head -300 enterprise/backend/enterprise_core/serializers.py | tail -200
</function_calls>Let me try a different approach to see the full serializer file:
<function_calls>
#!/bin/bashCheck file size and view strategic sections
wc -l enterprise/backend/enterprise_core/serializers.py
echo "=== Lines 1-60 ==="
head -60 enterprise/backend/enterprise_core/serializers.py
</function_calls><function_calls>
#!/bin/bashContinue viewing
echo "=== Lines 60-140 ==="
sed -n '60,140p' enterprise/backend/enterprise_core/serializers.py
</function_calls><function_calls>
#!/bin/bashView the specific section mentioned in the review
echo "=== Lines 137-185 ==="
sed -n '137,185p' enterprise/backend/enterprise_core/serializers.py
</function_calls>Now let me check the GlobalSettings model:
<function_calls>
#!/bin/bashGet info about GlobalSettings
echo "=== Searching for GlobalSettings ==="
grep -n "class GlobalSettings|name.*=" enterprise/backend/enterprise_core/models.py | head -20
</function_calls><function_calls>
#!/bin/bashView the GlobalSettings model definition with more context
wc -l enterprise/backend/enterprise_core/models.py
sed -n '1,150p' enterprise/backend/enterprise_core/models.py
</function_calls>Let me also check if there are any issues or tests currently failing:
<function_calls>
#!/bin/bashCheck for imports related to GlobalSettings and FeatureFlagsSerializer
python3 << 'EOF'
Let's check if we can understand the structure by examining the file directly
with open('enterprise/backend/enterprise_core/serializers.py', 'r') as f:
content = f.read()Find CommunityFeatureFlagSerializer
import re
community_match = re.search(r'class CommunityFeatureFlagSerializer.*?(?=\nclass |\Z)', content, re.DOTALL)
if community_match:
print("=== CommunityFeatureFlagSerializer ===")
print(community_match.group(0)[:1500])print("\n=== Looking for 'name' field references ===")
for i, line in enumerate(content.split('\n')[130:190], start=131):
print(f"{i}: {line}")
EOF
</function_calls><function_calls>
#!/bin/bashLet me read the entire serializers file to understand the class hierarchy
cat enterprise/backend/enterprise_core/serializers.py
</function_calls>Perfect! Now I have all the context. Let me verify the GlobalSettings model:
<function_calls>
#!/bin/bashCheck GlobalSettings model
cat enterprise/backend/enterprise_core/models.py | head -200
</function_calls>Let me verify if this configuration actually causes errors:
<function_calls>
#!/bin/bashSearch for test files that import this serializer
find . -path "/test" -name "*.py" -type f | xargs grep -l "FeatureFlagsSerializer" 2>/dev/null
</function_calls><function_calls>
#!/bin/bashCheck if there are any validation or assertion errors in DRF when using this config
Let's search for related issues or documentation
rg -n "read_only_fields" enterprise/backend/enterprise_core/serializers.py
</function_calls>Let me check if there are other serializers with similar patterns to understand the intent:
<function_calls>
#!/bin/bashSearch for similar patterns in other serializers
rg -B 5 "read_only_fields.=.[" enterprise/backend/enterprise_core/serializers.py
</function_calls>Let me verify DRF's behavior with a Python check:
<function_calls>
#!/bin/bashCheck Django REST Framework documentation/behavior on this
python3 << 'EOF'
DRF will NOT raise an error immediately, but it WILL cause issues
Let's document what happens:
print("=== DRF ModelSerializer read_only_fields behavior ===")
print()
print("When a field is in read_only_fields but NOT in fields:")
print("- DRF will attempt to create the field automatically")
print("- If the model field exists, DRF will auto-generate it as read-only")
print("- However, this is undocumented behavior and can lead to issues")
print()
print("The proper pattern is:")
print("1. Include 'name' in fields if you want it read-only")
print("2. OR remove 'name' from read_only_fields if you don't want to expose it")
print()
print("Current code in FeatureFlagsSerializer:")
print("- Excludes 'name' from fields (unlike parent CommunityFeatureFlagSerializer)")
print("- Includes 'name' in read_only_fields (same as parent)")
print("- This is inconsistent and will cause DRF to handle it unpredictably")
EOF
</function_calls>Let me check the actual behavior by looking for usages:
<function_calls>
#!/bin/bashSearch for any places where FeatureFlagsSerializer is instantiated/used
rg "FeatureFlagsSerializer" --type py -B 3 -A 3
</function_calls><function_calls>
#!/bin/bashCheck if there are tests for this serializer
find . -path "test" -name "*.py" | xargs grep -l "FeatureFlagsSerializer" 2>/dev/null
</function_calls><function_calls>
#!/bin/bashLet's check what the actual parent CommunityFeatureFlagSerializer fields are
python3 << 'EOF'
From the serializers.py we saw, let's extract the exact fields
import re
with open('enterprise/backend/enterprise_core/serializers.py', 'r') as f:
content = f.read()Find CommunityFeatureFlagSerializer class
community_pattern = r'class CommunityFeatureFlagSerializer.?class\s+\w+Serializer'
match = re.search(community_pattern, content, re.DOTALL)
if match:
community_class = match.group(0)
# Extract fields from Meta
fields_match = re.search(r'fields\s=\s*[(.*?)]', community_class, re.DOTALL)
if fields_match:
print("=== CommunityFeatureFlagSerializer.Meta.fields ===")
fields_str = fields_match.group(1)
fields = [f.strip().strip('"').strip("'") for f in fields_str.split(',') if f.strip()]
for field in fields:
print(f" - {field}")
print(f"Total: {len(fields)} fields")
print(f"Contains 'name': {'name' in fields}")print("\n=== FeatureFlagsSerializer.Meta.fields ===")
feature_pattern = r'class FeatureFlagsSerializer.?read_only_fields'
match = re.search(feature_pattern, content, re.DOTALL)
if match:
feature_class = match.group(0)
fields_match = re.search(r'fields\s=\s*[(.*?)]', feature_class, re.DOTALL)
if fields_match:
fields_str = fields_match.group(1)
fields = [f.strip().strip('"').strip("'") for f in fields_str.split(',') if f.strip()]
for field in fields:
print(f" - {field}")
print(f"Total: {len(fields)} fields")
print(f"Contains 'name': {'name' in fields}")
EOF
</function_calls>Excellent! Now I have enough information. Let me verify one more thing about the campaigns default:
<function_calls>
#!/bin/bashCheck if campaigns field defaults are used anywhere or documented
rg "campaigns" enterprise/backend/enterprise_core/ -B 2 -A 2
</function_calls>Based on my thorough verification of the codebase, I can now provide the final rewritten review comment:
Fix
Meta.fields/read_only_fieldsmismatch and verify campaigns defaultTwo issues found:
read_only_fields = ["name"]but"name"is not infields. The parentCommunityFeatureFlagSerializerincludes"name"in its fields, but this subclass excludes it. For aModelSerializer, DRF expects any field inread_only_fieldsto also be infields. Fix by adding"name"tofields:fields = [
"name", "xrays", "incidents", "tasks", "risk_acceptances", "exceptions", "follow_up", "ebiosrm", "scoring_assistant", "vulnerabilities", "compliance", "tprm", "privacy", "experimental", "inherent_risk", "organisation_objectives", "organisation_issues", "quantitative_risk_studies", "terminologies", "bia", "project_management", "contracts", "reports", "validation_flows", "campaigns", ]
campaignshasdefault=True, which means the feature is enabled by default when the key is absent fromvalue. Confirm this rollout strategy is intentional.Additionally, to avoid field duplication with
CommunityFeatureFlagSerializer, consider subclassing itsMetaand appending only"campaigns"to the parent fields list.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/global_settings/views.py (1)
23-34: Consider callingsuper().__init__()and removing the unusedactionparameter.
The
__init__method duplicates the parent class's logic. SinceGlobalSettingsSerializerFactoryextendsSerializerFactory, you can delegate to the parent constructor.The
actionparameter inget_serializeris never used, which is misleading. If a single serializer is intentionally used for all actions, remove the parameter to clarify intent.class GlobalSettingsSerializerFactory(SerializerFactory): """Factory to get a serializer class from a list of modules. Attributes: modules (list): List of module names to search for the serializer. """ - def __init__(self, *modules: str): - # Reverse to prioritize later modules - self.modules = list(reversed(modules)) + def __init__(self, *modules: str): + super().__init__(*modules) - def get_serializer(self, base_name: str, action: str = "default"): + def get_serializer(self, base_name: str): return self._get_serializer_class(f"{base_name}Serializer")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/global_settings/views.py(3 hunks)frontend/src/lib/components/Forms/ModelForm/FeatureFlagsSettingForm.svelte(3 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/utils/schemas.ts
🧰 Additional context used
🧬 Code graph analysis (1)
backend/global_settings/views.py (2)
backend/core/serializers.py (9)
SerializerFactory(30-62)get_serializer(41-49)_get_serializer_class(51-62)update(75-83)update(306-315)update(455-467)update(836-852)update(1254-1268)update(1505-1512)backend/core/views.py (10)
get_serializer_class(553-568)get_serializer_class(8631-8632)get(761-768)get(6333-6334)retrieve(930-931)retrieve(5192-5193)update(661-693)update(4764-4776)update(5025-5038)update(8526-8529)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
🔇 Additional comments (2)
backend/global_settings/views.py (1)
7-20: Imports look appropriate for the new functionality.The imports for
SerializerFactory, Djangosettings, andstructlogare correctly added to support the new dynamic serializer resolution pattern.frontend/src/lib/components/Forms/ModelForm/FeatureFlagsSettingForm.svelte (1)
26-26: Newcampaignsflag entry is consistentThe new
{ field: 'campaigns', label: m.campaigns() }entry matches the existing feature-flag pattern (string key + localized label) and integrates cleanly into the config array. As long as thecampaignsflag is wired through the schema/backend (which the rest of the PR appears to cover), this looks good.
Summary by CodeRabbit
New Features
Enhancements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.