[WEB-4544] chore: added field validations in serializer#7460
[WEB-4544] chore: added field validations in serializer#7460sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
WalkthroughThe changes introduce enhanced validation logic for issue and draft serializers, ensuring that related entities (such as assignees, labels, state, parent, and estimate point) belong to the correct project context. HTML content in descriptions is now parsed and sanitized. Related object creation is updated to use validated ID fields. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Serializer
participant DB
Client->>Serializer: Submit issue/draft data (with HTML, assignees, labels, etc.)
Serializer->>DB: Validate estimate_point, parent, state, assignees, labels (by ID and project)
DB-->>Serializer: Return validation results
Serializer->>Serializer: Parse and sanitize description_html
alt Validation fails
Serializer-->>Client: Raise ValidationError
else Validation succeeds
Serializer->>DB: Create or update issue/draft with validated IDs
DB-->>Serializer: Confirm creation/update
Serializer-->>Client: Return success
end
Estimated code review effort3 (~45 minutes) Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsapps/api/plane/app/serializers/draft.py (3)Learnt from: vamsikrishnamathala Learnt from: prateekshourya29 Learnt from: prateekshourya29 ⏰ 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)
🔇 Additional comments (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes validation issues in the Work Item model by adding comprehensive field validations and correcting database relationship references in the serializers. The changes ensure that all related entities (assignees, labels, states, parents, and estimate points) belong to the correct project scope and prevent invalid data from being saved.
- Adds HTML validation for description fields using lxml library
- Implements project-scoped validation for states, parents, labels, assignees, and estimate points
- Fixes foreign key references to use ID-based assignments instead of object assignments
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/api/plane/app/serializers/issue.py | Adds comprehensive validation logic and fixes label/assignee FK references |
| apps/api/plane/app/serializers/draft.py | Mirrors validation improvements from issue serializer for draft issues |
| apps/api/plane/api/serializers/issue.py | Adds estimate point validation and improves parent issue validation scope |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
apps/api/plane/api/serializers/issue.py (1)
105-112: Validation logic contradicts the comment about cross-project parent issues.The comment states "Check parent issue is from workspace as it can be cross workspace" (which seems to imply cross-project within the same workspace), but the validation now requires the parent issue to be from the same project. This is a breaking change if parent-child relationships were previously allowed across projects.
Consider either:
- Updating the comment to reflect the new stricter validation
- Removing the
project_idfilter if cross-project parent issues should be allowedapps/api/plane/app/serializers/draft.py (1)
245-260: Critical bug: Incorrect field name in label bulk creation.The update method incorrectly uses
label=labelinstead oflabel_id=label_id. Since the validated data contains label IDs (not objects), this will cause a runtime error.DraftIssueLabel.objects.bulk_create( [ DraftIssueLabel( - label=label, + label_id=label_id, draft_issue=instance, workspace_id=workspace_id, project_id=project_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) - for label in labels + for label_id in labels ], batch_size=10, )
🧹 Nitpick comments (3)
apps/api/plane/app/serializers/draft.py (1)
90-97: Consider simplifying the label validation logic.The validation correctly ensures labels belong to the project. However, the code can be simplified:
- if attrs.get("label_ids"): - label_ids = [label.id for label in attrs["label_ids"]] - attrs["label_ids"] = list( - Label.objects.filter( - project_id=self.context.get("project_id"), id__in=label_ids - ).values_list("id", flat=True) - ) + if attrs.get("label_ids"): + label_ids = [label.id for label in attrs["label_ids"]] + attrs["label_ids"] = Label.objects.filter( + project_id=self.context.get("project_id"), id__in=label_ids + ).values_list("id", flat=True)The
list()wrapper is unnecessary asvalues_list()returns a QuerySet that behaves like a list.apps/api/plane/app/serializers/issue.py (2)
143-151: Consider simplifying the label validation logic.Similar to the draft serializer, the
list()wrapper is unnecessary:- if attrs.get("label_ids"): - label_ids = [label.id for label in attrs["label_ids"]] - attrs["label_ids"] = list( - Label.objects.filter( - project_id=self.context.get("project_id"), - id__in=label_ids, - ).values_list("id", flat=True) - ) + if attrs.get("label_ids"): + label_ids = [label.id for label in attrs["label_ids"]] + attrs["label_ids"] = Label.objects.filter( + project_id=self.context.get("project_id"), + id__in=label_ids, + ).values_list("id", flat=True)
177-186: Good implementation of estimate_point validation.This validation correctly checks both
workspace_idandproject_id. However, theDraftIssueCreateSerializeronly checksproject_id. These should be made consistent.Consider creating a shared validation mixin or utility function for common validations (estimate_point, parent, state) to ensure consistency across all issue-related serializers. This would prevent the validation logic from diverging between different serializers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/plane/api/serializers/issue.py(2 hunks)apps/api/plane/app/serializers/draft.py(6 hunks)apps/api/plane/app/serializers/issue.py(6 hunks)
🧠 Learnings (1)
📓 Common learnings
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
🧬 Code Graph Analysis (1)
apps/api/plane/app/serializers/issue.py (2)
apps/api/plane/db/models/issue.py (1)
Issue(104-260)apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py (1)
issue(25-31)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
🧬 Code Graph Analysis (1)
apps/api/plane/app/serializers/issue.py (2)
apps/api/plane/db/models/issue.py (1)
Issue(104-260)apps/api/plane/tests/unit/bg_tasks/test_copy_s3_objects.py (1)
issue(25-31)
⏰ 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 (7)
apps/api/plane/api/serializers/issue.py (1)
118-129: LGTM! Proper validation for estimate_point field.The validation ensures that the estimate point belongs to the correct workspace and project context, maintaining data integrity. The implementation follows the established pattern for similar validations.
apps/api/plane/app/serializers/draft.py (4)
72-79: Good addition of HTML validation for security.The HTML parsing and sanitization helps prevent XSS attacks. The implementation is consistent with similar validation in other serializers.
81-88: Excellent validation for assignees.The filtering ensures only active project members with appropriate permissions (role >= 15) can be assigned to draft issues, maintaining data integrity and access control.
99-109: Proper validation for state field.The validation ensures the state belongs to the correct project context with a clear error message.
155-185: Correct usage of ID fields in bulk create operations.The changes properly use
assignee_idandlabel_idto match the validated data structure after filtering. This ensures the bulk create operations work correctly with the ID lists.apps/api/plane/app/serializers/issue.py (2)
125-132: ****
246-263: Correct implementation of label handling in create and update methods.Both methods properly use
label_idto match the validated data structure. The implementation is consistent and correct.Also applies to: 298-317
* chore: added field validations in serializer * chore: added enum for roles
* chore: added field validations in serializer * chore: added enum for roles
* chore: added field validations in serializer * chore: added enum for roles
Description
this pull request fixes the issue with the validation of the field in the Work Item model.
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Style