-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: work item assignee update validation #6704
Conversation
WalkthroughThe update modifies the conditional check within the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant ICS as IssueCreateSerializer
participant DB as Database
C->>ICS: Call update(instance, validated_data)
alt valid_assignee_ids is not None
ICS->>DB: Delete existing IssueAssignee records
ICS->>DB: Bulk create new IssueAssignee records (even when list is empty)
else
ICS->>C: Skip assignee update
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apiserver/plane/app/serializers/issue.py (1)
221-239
: Consider adding a comment explaining the empty list behavior.While the code logic is correct, it would be helpful to add a comment explaining that an empty list is intentionally allowed to remove all assignees. This would make the intentional behavior change clearer to future developers.
if valid_assignee_ids is not None: + # An empty list is allowed and will remove all assignees IssueAssignee.objects.filter(issue=instance).delete() try: IssueAssignee.objects.bulk_create(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apiserver/plane/app/serializers/issue.py
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
apiserver/plane/app/serializers/issue.py (3)
219-220
: Fixed assignee update validation to correctly handle empty lists.This change correctly modifies the condition in the
update
method to handle the case wherevalid_assignee_ids
is an empty list. The previous logic only processed assignee updates when the list wasn't empty, which meant users couldn't remove all assignees from an issue. Now, when users submit an empty list of assignees (as opposed toNone
), the system will correctly delete all existing assignees.Note that this change introduces a slight behavior difference between
create
andupdate
methods:
- In
create
(line 150), an empty list still triggers the default assignee logic- In
update
, an empty list now removes all assignees without applying a defaultThis difference makes sense from a UX perspective as an empty list during update likely indicates an explicit user action to remove all assignees.
149-151
: Consider aligning create and update method behaviors for assignee handling.There's now a behavioral difference between how empty assignee lists are handled in
create
vsupdate
:
- In
create
: Empty list → Use default assignee- In
update
: Empty list → Remove all assigneesFor consistency, you might want to consider aligning these behaviors if it makes sense for your product requirements. If the different behavior is intentional, consider adding a comment explaining why.
This is a minor observation about the behavioral difference and not a bug. Please verify with product/UX teams if this difference in handling is intentional and expected.
150-185
:❓ Verification inconclusive
Verify UI handling of assignee removal.
To ensure this bug fix works end-to-end, verify that the frontend UI sends an empty list (not
null
orNone
) when a user wants to remove all assignees. If the UI sendsnull
instead of an empty list, this fix won't have the desired effect.
🏁 Script executed:
#!/bin/bash # Check if the frontend sends an empty list rather than null when removing assignees # Look for API calls related to issue updates with assignee_ids rg -l "assignee_ids.*\[\]" --type jsLength of output: 38
I'm not getting a conclusive result from the initial grep. Let's run a more comprehensive search to check for patterns where the UI might be sending an empty array for
assignee_ids
. This will help verify whether the frontend is actually sending an empty list when assignees are removed.Please execute this script and share the output so we can ensure the bug fix is verified end-to-end.
🏁 Script executed:
#!/bin/bash # Searching for frontend API calls where assignee_ids is explicitly set to an empty array. rg -l "assignee_ids\\s*:\\s*\\[\\s*\\]" --type js --type jsx --type tsLength of output: 95
Action: Verify Frontend Sends an Empty Array on Assignee Removal
Our initial search didn't yield conclusive results due to file type issues. Please re-run the following command to search across JavaScript, JSX, and TypeScript files using glob filters:
#!/bin/bash # Searching for explicit empty array assignment to assignee_ids in frontend API calls rg -g "*.js" -g "*.jsx" -g "*.ts" -l "assignee_ids\\s*:\\s*\\[\\s*\\]"If this search again returns no output, manually verify that the UI sends an empty array (and not null) when a user removes all assignees. Confirming this behavior is essential for ensuring the bug fix works fully end-to-end.
* fix: issue activity for project id validation (#6668) * fix: work item attachment count mutation (#6670) * updated the action to modify the release build assets (#6669) * feat: russian translation (#6666) * chore: ru translation updated (#6672) * fix: state drop down refactor * fix: intake work item creation refactor * fix: cleanup for deprecated functions * fix: date range picker on cycles and modules list (#6676) * fix: Handled workspace switcher closing on click * fix: replaced date range picker with date picker at some places * chore: add common translation keys (#6688) * chore: add missing translation keys * chore: add russian translation keys * fix: issue activity task (#6689) * changed github workflow action ubuntu version to `ubuntu-22.04` (#6683) * chore: update russian translation (#6682) * chore: update russian translation * chore: rename issues to work items in russian translation * [PE-275] chore: editor line spacing variables (#6678) * chore: variable editor line spacing * chore: variable list spacing --------- Co-authored-by: Aaryan Khandelwal <aaryankhandu123@gmail.com> * [WEB-3475] fix: cycle dates dropdown (#6690) * fix: Handled workspace switcher closing on click * fix: Cycle date picker * fix: Made onSelect optional in range range component * fix: module date picker (#6691) * fix: Handled workspace switcher closing on click * fix: reverted module date picker changes * chore: extended sidebar improvement (#6693) * feat: italian translations (#6692) * Create translations.json - ITALIAN translation (#6667) * chore: italian translation updated * feat: italian translation added * fix: module end date translation --------- Co-authored-by: Nicolas Bossi <nicolasbossi@gmail.com> Co-authored-by: gakshita <akshitagoyal1516@gmail.com> * fix: attachment item created by (#6695) * fix: module flicker issue on property updation (#6699) * [WEB-3477] fix: mutation issue on moving work items for a manually ended cycle (#6696) * fix: package version update * fix: esbuild version fix * fix: package license repliation * [WEB-3488] improvement: assignee validation for work item creation (#6701) * fix: work item assignee update validation (#6704) --------- Co-authored-by: Nikhil <118773738+pablohashescobar@users.noreply.github.com> Co-authored-by: Anmol Singh Bhatia <121005188+anmolsinghbhatia@users.noreply.github.com> Co-authored-by: Manish Gupta <59428681+mguptahub@users.noreply.github.com> Co-authored-by: Nikita Mitasov <32384814+ch4og@users.noreply.github.com> Co-authored-by: Akshita Goyal <36129505+gakshita@users.noreply.github.com> Co-authored-by: Aaryan Khandelwal <65252264+aaryan610@users.noreply.github.com> Co-authored-by: Akshat Jain <akshatjain9782@gmail.com> Co-authored-by: Lakhan Baheti <94619783+1akhanBaheti@users.noreply.github.com> Co-authored-by: Aaryan Khandelwal <aaryankhandu123@gmail.com> Co-authored-by: Nicolas Bossi <nicolasbossi@gmail.com> Co-authored-by: gakshita <akshitagoyal1516@gmail.com> Co-authored-by: Prateek Shourya <prateekshourya29@gmail.com>
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit