fix(api): validate assignees/labels in IssueSerializer instead of silently dropping invalid ids - #9526
Conversation
…ently dropping invalid ids Work item create/update via the external API returned 200/201 even when assignees or labels didn't belong to the project, quietly filtering the invalid ids out with no error. Now raises the same kind of ValidationError already used for state/parent, matching the pattern of makeplane#9517.
📝 WalkthroughWalkthrough
ChangesIssue relationship validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 Warning |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/tests/contract/api/test_issue_assignee_label_validation.py`:
- Around line 19-23: Update the fixture teardown around the task_always_eager
setup to capture the original celery_app.conf.task_eager_propagates value before
changing it, then restore that value after yield alongside task_always_eager.
Preserve the fixture’s existing eager-task configuration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01fbfb6e-5b4a-4384-bdac-a914e6e89d4c
📒 Files selected for processing (3)
apps/api/plane/api/serializers/issue.pyapps/api/plane/tests/contract/api/test_issue_assignee_label_validation.pyapps/api/plane/tests/unit/serializers/test_issue_serializer_api.py
| original = celery_app.conf.task_always_eager | ||
| celery_app.conf.task_always_eager = True | ||
| celery_app.conf.task_eager_propagates = False | ||
| yield | ||
| celery_app.conf.task_always_eager = original |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore task_eager_propagates during fixture teardown.
The fixture changes task_eager_propagates on Line 21 but does not restore it. Later tests can observe False instead of their prior configuration.
Proposed fix
original = celery_app.conf.task_always_eager
+ original_propagates = celery_app.conf.task_eager_propagates
celery_app.conf.task_always_eager = True
celery_app.conf.task_eager_propagates = False
yield
celery_app.conf.task_always_eager = original
+ celery_app.conf.task_eager_propagates = original_propagates📝 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.
| original = celery_app.conf.task_always_eager | |
| celery_app.conf.task_always_eager = True | |
| celery_app.conf.task_eager_propagates = False | |
| yield | |
| celery_app.conf.task_always_eager = original | |
| original = celery_app.conf.task_always_eager | |
| original_propagates = celery_app.conf.task_eager_propagates | |
| celery_app.conf.task_always_eager = True | |
| celery_app.conf.task_eager_propagates = False | |
| yield | |
| celery_app.conf.task_always_eager = original | |
| celery_app.conf.task_eager_propagates = original_propagates |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/plane/tests/contract/api/test_issue_assignee_label_validation.py`
around lines 19 - 23, Update the fixture teardown around the task_always_eager
setup to capture the original celery_app.conf.task_eager_propagates value before
changing it, then restore that value after yield alongside task_always_eager.
Preserve the fixture’s existing eager-task configuration behavior.
Summary
Fixes #9517.
Creating or updating a work item through the external API (
POST/PATCH /api/v1/.../issues/) with anassigneesorlabelslist returned200/201even when some of those ids weren't valid for the project (not an active project member with role >= 15, or a label from a different project). The invalid ids were silently filtered out inIssueSerializer.validate()— no error, and the work item just came back unassigned/unlabeled.This matches the same silent-narrowing pattern already fixed for
state/parent, which raise aValidationErrorwhen the id isn't valid for the project. Assignees and labels were the odd ones out.Change
In
apps/api/plane/api/serializers/issue.py,IssueSerializer.validate():ValidationErrornaming the offending ids, instead of quietly dropping them.create()/update(), which already just iterate over them — no change needed there.A partially-valid list (some valid, some invalid ids) now rejects the whole request rather than silently keeping only the valid subset.
Test plan
apps/api/plane/tests/unit/serializers/test_issue_serializer_api.py) covering the serializer directly: rejects a non-member assignee, rejects a foreign-project label, still accepts a valid active project member.apps/api/plane/tests/contract/api/test_issue_assignee_label_validation.py) hitting the real/api/v1/.../issues/endpoint viaAPIClient, covering the exact repro steps from the issue:POSTwith a non-member assignee,POSTwith a foreign label,PATCHadding a non-member assignee, a mixed valid+invalid assignee list, and a regression guard that a genuinely valid assignee still works.201/200responses instead of400) and GREEN after.Summary by CodeRabbit
Bug Fixes
Tests