[GIT-175] fix: completed_at updation logic for work items#9044
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesIssue Model State Tracking Refactor
Sequence Diagram(s)sequenceDiagram
participant Issue
participant Project
participant State
Issue->>Project: get_default_non_triage_state()
Project->>Issue: State or None
alt default found
Issue->>State: assign to self.state
else fallback
Project->>State: get_any_non_triage_state()
State->>Issue: assign to self.state
end
Issue->>Issue: determine state.group == "completed"
alt completed
Issue->>Issue: set completed_at = timezone.now()
else not completed
Issue->>Issue: clear completed_at
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![github-code-quality[bot]](https://avatars.gh.lumenfield.work/u/9919?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
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/db/models/issue.py`:
- Around line 181-182: The helper calls _ensure_default_state() and
_sync_completed_at(kwargs) must be skipped for partial saves that don't include
state to avoid writing completed_at without state_id; change the save/update
code paths (the call sites invoking _ensure_default_state and _sync_completed_at
in the Issue model) to only invoke these helpers when update_fields is None
(full save) or when update_fields contains the state-related field names
('state' or 'state_id'); apply the same guard to the other occurrence of these
helpers in the code around the second block (previously noted at lines 240-255)
so both spots only mutate completed_at/state when state is being persisted.
🪄 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
Run ID: 90f82593-c53f-41c3-b1a2-1f15a60ea148
📒 Files selected for processing (1)
apps/api/plane/db/models/issue.py
There was a problem hiding this comment.
Pull request overview
This PR refactors the Issue model’s state-handling logic to make default state assignment and completed_at synchronization more maintainable and to avoid updating completed_at on unrelated saves by tracking state_id changes via ChangeTrackerMixin.
Changes:
Issuenow inherits fromChangeTrackerMixinand tracksstate_idchanges viaTRACKED_FIELDS.- Extracts default state assignment into
_ensure_default_state(). - Extracts
completed_atupdate logic into_sync_completed_at()and adjustsupdate_fieldsto includecompleted_atwhen needed.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/api/plane/db/models/issue.py (1)
181-182:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard state side effects behind
update_fieldscheck.These helpers currently run even for partial saves that exclude the state field. This can cause data corruption: if
issue.stateis modified in memory butsave(update_fields=["name"])is called,_sync_completed_atwill writecompleted_atwithout persisting the newstate_id, leaving the database inconsistent.The previous review comment on lines 181-182 correctly identified this issue and provided a fix. Please apply the suggested guard:
update_fields = kwargs.get("update_fields") state_is_being_saved = ( update_fields is None or "state" in update_fields or "state_id" in update_fields ) if self._state.adding or state_is_being_saved: self._ensure_default_state() kwargs = self._sync_completed_at(kwargs)🤖 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/db/models/issue.py` around lines 181 - 182, The helpers _ensure_default_state and _sync_completed_at are being executed even for partial saves, causing completed_at to be updated when state isn't being persisted; modify the save (or the method containing the shown lines) to first compute update_fields = kwargs.get("update_fields") and determine state_is_being_saved = (update_fields is None or "state" in update_fields or "state_id" in update_fields), then only call self._ensure_default_state() and kwargs = self._sync_completed_at(kwargs) when self._state.adding or state_is_being_saved is true so side effects are guarded behind the update_fields check.
🤖 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.
Duplicate comments:
In `@apps/api/plane/db/models/issue.py`:
- Around line 181-182: The helpers _ensure_default_state and _sync_completed_at
are being executed even for partial saves, causing completed_at to be updated
when state isn't being persisted; modify the save (or the method containing the
shown lines) to first compute update_fields = kwargs.get("update_fields") and
determine state_is_being_saved = (update_fields is None or "state" in
update_fields or "state_id" in update_fields), then only call
self._ensure_default_state() and kwargs = self._sync_completed_at(kwargs) when
self._state.adding or state_is_being_saved is true so side effects are guarded
behind the update_fields check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99abe9ca-04ed-4da0-a9e1-c3e27f0b0ada
📒 Files selected for processing (1)
apps/api/plane/db/models/issue.py
This pull request refactors how the
Issuemodel inissue.pyhandles state management and tracking of changes. The main improvements include extracting logic for assigning default states and updating thecompleted_atfield into dedicated helper methods, and enabling change tracking for thestate_idfield. These changes improve code clarity, maintainability, and ensure that state transitions are tracked more reliably.State management and tracking improvements:
Issuemodel now inherits fromChangeTrackerMixinand explicitly tracks changes to thestate_idfield, enabling more robust detection of state changes.completed_atfield has been refactored into two private methods:_ensure_default_stateand_sync_completed_at, improving code organization and readability. [1] [2]savemethod inIssuenow delegates state assignment andcompleted_atsynchronization to these new helper methods, ensuring consistent behavior and reducing code duplication.Code organization:
ChangeTrackerMixinhas been moved to the main import statement for consistency.Summary by CodeRabbit