[SILO-699] chore: add check for feature enabled for module and cycle create #8146
+9
−1
Conversation
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
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
Contributor
WalkthroughTwo API serializers are enhanced with feature flag validation gates. CycleCreateSerializer now enforces project.cycle_view before allowing cycle creation, and ModuleCreateSerializer enforces project.module_view before allowing module operations. Additionally, cycle start_date conversion includes an is_start_date parameter flag. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Serializer
participant ProjectDB as Project DB
Client->>Serializer: POST create cycle/module
Serializer->>Serializer: validate() called
Serializer->>ProjectDB: Fetch project
ProjectDB-->>Serializer: project data
alt feature flag enabled
rect rgb(200, 220, 255)
Note over Serializer: Continue validation
Serializer-->>Client: Success
end
else feature flag disabled
rect rgb(255, 200, 200)
Note over Serializer: Raise ValidationError
Serializer-->>Client: Error: Feature not enabled
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/api/serializers/cycle.py(2 hunks)apps/api/plane/api/serializers/module.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dheeru0198
Repo: makeplane/plane PR: 7625
File: apps/api/plane/bgtasks/workspace_seed_task.py:95-98
Timestamp: 2025-09-12T07:29:36.083Z
Learning: In the Plane codebase, workspace seed data should have project features (cycle_view, module_view, issue_views_view) enabled by default, even when regular project creation has these features disabled. This provides users with a complete demo experience in the seeded workspace.
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7969
File: apps/api/plane/api/views/cycle.py:1218-1228
Timestamp: 2025-10-17T10:46:13.825Z
Learning: In the cycle transfer functionality in apps/api/plane/api/views/cycle.py, transfers should only be allowed from completed cycles (end_date < timezone.now()), not from active or draft cycles. The guard should block when end_date is None or end_date >= timezone.now().
📚 Learning: 2025-10-17T10:46:13.825Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7969
File: apps/api/plane/api/views/cycle.py:1218-1228
Timestamp: 2025-10-17T10:46:13.825Z
Learning: In the cycle transfer functionality in apps/api/plane/api/views/cycle.py, transfers should only be allowed from completed cycles (end_date < timezone.now()), not from active or draft cycles. The guard should block when end_date is None or end_date >= timezone.now().
Applied to files:
apps/api/plane/api/serializers/cycle.py
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/api/serializers/cycle.pyapps/api/plane/api/serializers/module.py
🧬 Code graph analysis (2)
apps/api/plane/api/serializers/cycle.py (2)
apps/api/plane/db/models/project.py (1)
Project(65-159)apps/api/plane/tests/contract/api/test_cycles.py (1)
project(12-26)
apps/api/plane/api/serializers/module.py (1)
apps/api/plane/db/models/project.py (1)
Project(65-159)
⏰ 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 (javascript)
🔇 Additional comments (1)
apps/api/plane/api/serializers/cycle.py (1)
80-80: Clarify the purpose of theis_start_date=Trueparameter.This change adds an
is_start_date=Trueparameter to theconvert_to_utccall, which is not mentioned in the PR description. The PR objective states adding feature checks for module and cycle creation, not modifying timezone conversion behavior.Please clarify:
- Is this change intentional and related to the feature flag validation?
- What is the purpose of the
is_start_dateparameter and how does it affect UTC conversion?- Should this be part of a separate PR focused on timezone handling?
Comment on lines
+73
to
+76
| project = Project.objects.filter(id=project_id).first() | ||
| if not project.cycle_view: | ||
| raise serializers.ValidationError("Cycles are not enabled for this project") | ||
|
|
Contributor
There was a problem hiding this comment.
Critical: Add null check and move feature validation outside date validation block.
Two critical issues:
- Missing null check:
projectcan beNoneif the project doesn't exist, causingAttributeErrorwhen accessingproject.cycle_view. - Incorrect placement: The feature check is nested inside the date validation block (lines 65-86), meaning cycles created without both
start_dateandend_datewill bypass this validation entirely.
Apply this diff to fix both issues:
def validate(self, data):
+ # Validate project and feature availability first
+ project_id = self.initial_data.get("project_id") or (
+ self.instance.project_id if self.instance and hasattr(self.instance, "project_id") else None
+ )
+
+ if not project_id:
+ raise serializers.ValidationError("Project ID is required")
+
+ project = Project.objects.filter(id=project_id).first()
+ if not project:
+ raise serializers.ValidationError("Project not found")
+
+ if not project.cycle_view:
+ raise serializers.ValidationError("Cycles are not enabled for this project")
+
if (
data.get("start_date", None) is not None
and data.get("end_date", None) is not None
and data.get("start_date", None) > data.get("end_date", None)
):
raise serializers.ValidationError("Start date cannot exceed end date")
if data.get("start_date", None) is not None and data.get("end_date", None) is not None:
- project_id = self.initial_data.get("project_id") or (
- self.instance.project_id if self.instance and hasattr(self.instance, "project_id") else None
- )
-
- if not project_id:
- raise serializers.ValidationError("Project ID is required")
-
- project = Project.objects.filter(id=project_id).first()
- if not project.cycle_view:
- raise serializers.ValidationError("Cycles are not enabled for this project")
-
data["start_date"] = convert_to_utc(
date=str(data.get("start_date").date()),
project_id=project_id,
is_start_date=True,
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/plane/api/serializers/cycle.py around lines 73 to 76, the code
assumes Project.objects.filter(id=project_id).first() never returns None and
performs the cycles-feature check inside the date-validation block; change this
so you first fetch the project, immediately check for None and raise
serializers.ValidationError("Project does not exist") (or appropriate message),
then check project.cycle_view and raise serializers.ValidationError("Cycles are
not enabled for this project") before any date-specific validation, ensuring the
feature validation runs regardless of whether start_date/end_date are present.
Comment on lines
+57
to
+59
| project = Project.objects.get(id=self.context.get("project_id")) | ||
| if not project.module_view: | ||
| raise serializers.ValidationError("Modules are not enabled for this project") |
Contributor
There was a problem hiding this comment.
Handle missing project gracefully to prevent unhandled exceptions.
The code uses Project.objects.get() which will raise Project.DoesNotExist if the project is not found or if project_id is None. Additionally, if project_id is not present in the context, this will fail unexpectedly.
Apply this diff to add defensive checks:
- def validate(self, data):
- project = Project.objects.get(id=self.context.get("project_id"))
- if not project.module_view:
- raise serializers.ValidationError("Modules are not enabled for this project")
+ def validate(self, data):
+ project_id = self.context.get("project_id")
+ if not project_id:
+ raise serializers.ValidationError("Project ID is required")
+
+ project = Project.objects.filter(id=project_id).first()
+ if not project:
+ raise serializers.ValidationError("Project not found")
+
+ if not project.module_view:
+ raise serializers.ValidationError("Modules are not enabled for this project")🤖 Prompt for AI Agents
In apps/api/plane/api/serializers/module.py around lines 57 to 59, the code
calls Project.objects.get(id=self.context.get("project_id")) which will raise an
unhandled Project.DoesNotExist (or fail when project_id is None); update it to
first validate that project_id exists in self.context and then retrieve the
project safely (either use Project.objects.filter(id=project_id).first() and
check for None, or wrap get() in a try/except Project.DoesNotExist), and raise
serializers.ValidationError with a clear message (e.g., "Project not found" or
"project_id missing") when the project_id is missing or the project cannot be
found, instead of letting an exception bubble up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.