[WEB-4851] chore: updated activity keys for work item #7733
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughNormalize input keys and ID resolution in issue_activities_task.py: add helper to extract IDs from primary or fallback keys; update trackers for parent, state, labels, and assignees to read both internal and external payload keys; validate UUIDs; fetch related models; emit IssueActivity entries and manage IssueSubscriber creation/removal; extend ISSUE_ACTIVITY_MAPPER. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Updater as update_issue_activity
participant Normalizer as extract_ids / normalizer
participant Mapper as ISSUE_ACTIVITY_MAPPER
participant Tracker as tracker funcs (parent/state/labels/assignees)
participant DB as DB (Issue/State/Label/User/Subscriber)
participant Activity as IssueActivity
Caller->>Updater: call with current_instance, requested_data
Updater->>Normalizer: extract/normalize IDs (primary or fallback keys) and validate UUIDs
Updater->>Mapper: map payload keys to tracker functions (includes external keys)
Mapper->>Tracker: invoke specific tracker(s)
rect rgba(200,230,255,0.18)
Tracker->>DB: query old/new entities as needed (guarded by UUID checks)
Tracker->>Activity: append activity entries (field, old, new, context)
alt added assignees
Tracker->>DB: bulk create IssueSubscriber
else removed assignees
Tracker->>DB: remove IssueSubscriber
end
end
Updater-->>Caller: return aggregated IssueActivity entries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for alternative activity tracking keys (parent, state, assignees, labels) in the external endpoint to complement existing internal keys (parent_id, state_id, assignee_ids, label_ids).
- Updated tracking functions to handle both internal and external key formats
- Modified label and assignee tracking logic to use conditional key checking
- Added external endpoint key mappings to the activity tracker dispatcher
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
apps/api/plane/bgtasks/issue_activities_task.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/bgtasks/issue_activities_task.py (1)
apps/api/plane/db/models/issue.py (1)
Issue(104-260)
⏰ 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 (5)
apps/api/plane/bgtasks/issue_activities_task.py (5)
119-125: LGTM! Improved normalization for parent ID tracking.The change properly normalizes parent identifier access by supporting both
parent_idandparentkeys from external and internal payloads, making the tracking more robust and compatible with different data sources.
203-206: LGTM! Enhanced state tracking with dual key support.The normalization allows the function to handle both
state_id(internal) andstate(external) keys, improving compatibility with external endpoints while maintaining backward compatibility.
311-330: LGTM! Enhanced label tracking with flexible key support.The changes properly handle both
label_idsandlabelskeys, providing better compatibility with external payloads while maintaining existing functionality.
395-413: LGTM! Improved assignee tracking with dual key support.The normalization handles both
assignee_idsandassigneeskeys effectively, with proper null checks and set conversion for comparison logic.
671-676: LGTM! Added external key mappings for activity tracking.The addition of external endpoint keys (
parent,state,assignees,labels) to the mapper enables the same tracking functions to handle both internal and external payloads seamlessly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/api/plane/bgtasks/issue_activities_task.py (2)
211-217: State lookups can crash; wrap/guard DoesNotExist/invalid IDs.Current .get() calls raise on invalid/None IDs, aborting the task. Handle safely. (Echoing prior review.)
- if current_state_id != requested_state_id: - new_state = State.objects.get(pk=requested_state_id) - old_state = State.objects.get(pk=current_state_id) + if current_state_id != requested_state_id: + try: + new_state = State.objects.get(pk=requested_state_id) + old_state = State.objects.get(pk=current_state_id) + except (State.DoesNotExist, ValueError, TypeError): + returnOptional: also constrain by project for consistency with closed_to.
#!/bin/bash # Verify State has project relation to safely filter by project_id rg -nP 'class\s+State\b' -C3 rg -nP '\bproject_id\b' --type=py -C2 | rg -nP 'class\s+State\b|models\.ForeignKey|project'
127-143: Validate UUIDs before querying parent to prevent ValueError on malformed IDs.Django will raise on UUIDField conversion if given a bad UUID. Guard both lookups.
- if current_parent_id != requested_parent_id: - old_parent = ( - Issue.objects.filter(pk=current_parent_id).first() - if current_parent_id is not None - else None - ) - new_parent = ( - Issue.objects.filter(pk=requested_parent_id).first() - if requested_parent_id is not None - else None - ) + if current_parent_id != requested_parent_id: + old_parent = ( + Issue.objects.filter(pk=current_parent_id).first() + if current_parent_id and is_valid_uuid(str(current_parent_id)) + else None + ) + new_parent = ( + Issue.objects.filter(pk=requested_parent_id).first() + if requested_parent_id and is_valid_uuid(str(requested_parent_id)) + else None + )
🧹 Nitpick comments (1)
apps/api/plane/bgtasks/issue_activities_task.py (1)
647-651: Avoid double activities when both canonical and external keys are present.If payload contains, e.g., state_id and state, track_state runs twice. Normalize keys before dispatch.
Changes needed outside the selected lines:
- for key in requested_data: - func = ISSUE_ACTIVITY_MAPPER.get(key) + synonyms = {"parent": "parent_id", "state": "state_id", "assignees": "assignee_ids", "labels": "label_ids"} + seen = set() + for key in requested_data: + canonical = synonyms.get(key, key) + if canonical in seen: + continue + func = ISSUE_ACTIVITY_MAPPER.get(canonical) if func is not None: func( - requested_data=requested_data, + requested_data=requested_data, current_instance=current_instance, issue_id=issue_id, project_id=project_id, workspace_id=workspace_id, actor_id=actor_id, issue_activities=issue_activities, epoch=epoch, ) + seen.add(canonical)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/bgtasks/issue_activities_task.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/bgtasks/issue_activities_task.py (2)
apps/api/plane/api/views/issue.py (14)
get(219-239)get(296-417)get(564-580)get(957-968)get(994-1001)get(1126-1137)get(1232-1253)get(1400-1411)get(1555-1564)get(1716-1741)get(1773-1798)get(2006-2021)get(2123-2159)get(2270-2319)apps/api/plane/db/models/issue.py (1)
Issue(104-260)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/api/plane/bgtasks/issue_activities_task.py (3)
37-43: Fix None/scalar handling in extract_ids to prevent TypeError.If a key exists with value None, the set comprehension iterates over None and crashes. Also accept single scalars.
-def extract_ids(data: dict | None, primary_key: str, fallback_key: str) -> set[str]: - if not data: - return set() - if primary_key in data: - return {str(x) for x in data.get(primary_key, [])} - return {str(x) for x in data.get(fallback_key, [])} +def extract_ids(data: dict | None, primary_key: str, fallback_key: str) -> set[str]: + if not data: + return set() + def _normalize(v): + if v is None: + return [] + if isinstance(v, (list, tuple, set)): + return v + return [v] + source = primary_key if primary_key in data else fallback_key + return {str(x) for x in _normalize(data.get(source))}
326-330: Harden label fetches to avoid DoesNotExist aborts; optionally batch to cut N+1 queries.Use safe lookups and skip missing rows; current .get(...) can crash the task.
- label = Label.objects.get(pk=added_label) + label = Label.objects.filter(pk=added_label).only("id", "name").first() + if not label: + continue @@ - label = Label.objects.get(pk=dropped_label) + label = Label.objects.filter(pk=dropped_label).only("id", "name").first() + if not label: + continueOptional (nice): prefetch both sets once to reduce queries; I can provide a batched refactor if you want.
Also applies to: 340-356, 358-380
394-397: Assignees: fix typo and use safe user lookups to avoid crashes.
- dropped_assginees typo;
- User.objects.get(...) raises; prefer safe filter().first().
- added_assignees = requested_assignees - current_assignees - dropped_assginees = current_assignees - requested_assignees + added_assignees = requested_assignees - current_assignees + dropped_assignees = current_assignees - requested_assignees @@ - assignee = User.objects.get(pk=added_asignee) + assignee = User.objects.filter(pk=added_asignee).only("id", "display_name").first() + if not assignee: + continue @@ - for dropped_assignee in dropped_assginees: + for dropped_assignee in dropped_assignees: @@ - assignee = User.objects.get(pk=dropped_assignee) + assignee = User.objects.filter(pk=dropped_assignee).only("id", "display_name").first() + if not assignee: + continueVerification: confirm created_by_id/updated_by_id should be the actor (who assigns) rather than the assignee. If needed, switch to actor_id.
Also applies to: 399-459
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/bgtasks/issue_activities_task.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/bgtasks/issue_activities_task.py (1)
apps/api/plane/db/models/issue.py (1)
Issue(104-260)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/bgtasks/issue_activities_task.py (1)
654-659: LGTM: external keys mapped to trackers.The additions make the external payload keys first-class. No further concerns here.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/plane/bgtasks/issue_activities_task.py (3)
345-345: Replace Label.objects.get() with safe lookups to avoid DoesNotExist exceptionsUsing
.get()will raiseLabel.DoesNotExistif a UUID is valid but the label doesn't exist in the database, causing the entire task to fail.Apply these changes to handle missing labels gracefully:
- label = Label.objects.get(pk=added_label) + label = Label.objects.filter(pk=added_label).only("id", "name").first() + if not label: + continueAnd similarly for dropped labels at line 369:
- label = Label.objects.get(pk=dropped_label) + label = Label.objects.filter(pk=dropped_label).only("id", "name").first() + if not label: + continueAlso applies to: 369-369
412-412: Replace User.objects.get() with safe lookups to avoid DoesNotExist exceptionsUsing
.get()will raiseUser.DoesNotExistif a UUID is valid but the user doesn't exist, causing the task to fail.Apply these changes to handle missing users gracefully:
- assignee = User.objects.get(pk=added_asignee) + assignee = User.objects.filter(pk=added_asignee).only("id", "display_name").first() + if not assignee: + continueAnd similarly for dropped assignees at line 449:
- assignee = User.objects.get(pk=dropped_assignee) + assignee = User.objects.filter(pk=dropped_assignee).only("id", "display_name").first() + if not assignee: + continueAlso applies to: 449-449
509-509: Fix potential AttributeError when new_estimate is NoneWhen
new_estimateis None (estimate point removed), accessingnew_estimate.estimate.typewill raise an AttributeError.- field="estimate_" + new_estimate.estimate.type, + field="estimate_" + (new_estimate.estimate.type if new_estimate else "point"),
♻️ Duplicate comments (2)
apps/api/plane/bgtasks/issue_activities_task.py (2)
37-42: Fix potential TypeError when key exists with None valueThe
extract_idsfunction will raise a TypeError if a key exists but has a None value, as the set comprehension will try to iterate over None.Apply this fix to handle None values safely:
def extract_ids(data: dict | None, primary_key: str, fallback_key: str) -> set[str]: if not data: return set() if primary_key in data: - return {str(x) for x in data.get(primary_key, [])} - return {str(x) for x in data.get(fallback_key, [])} + return {str(x) for x in (data.get(primary_key) or [])} + return {str(x) for x in (data.get(fallback_key) or [])}
127-139: Good UUID validation, but consider treating invalid IDs as None instead of returning earlyThe validation is good, but returning early when parent IDs are invalid might skip legitimate activity logging. Consider treating invalid IDs as None and continuing with the activity tracking.
# Validate UUIDs before database queries if current_parent_id is not None and not is_valid_uuid(current_parent_id): - return + current_parent_id = None if requested_parent_id is not None and not is_valid_uuid(requested_parent_id): - return + requested_parent_id = None
🧹 Nitpick comments (2)
apps/api/plane/bgtasks/issue_activities_task.py (2)
140-150: Add query optimization and project scoping for parent lookupsThe parent Issue queries could be optimized with
only()to fetch just the required fields, improving performance.old_parent = ( - Issue.objects.filter(pk=current_parent_id).first() + Issue.objects.filter(pk=current_parent_id) + .only("id", "sequence_id", "project__identifier") + .first() if current_parent_id is not None else None ) new_parent = ( - Issue.objects.filter(pk=requested_parent_id).first() + Issue.objects.filter(pk=requested_parent_id) + .only("id", "sequence_id", "project__identifier") + .first() if requested_parent_id is not None else None )
404-404: Fix typo in variable nameThere's a typo in the variable name that should be corrected for consistency.
- dropped_assginees = current_assignees - requested_assignees + dropped_assignees = current_assignees - requested_assigneesAlso update the usage at line 444:
- for dropped_assignee in dropped_assginees: + for dropped_assignee in dropped_assignees:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/bgtasks/issue_activities_task.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/bgtasks/issue_activities_task.py (2)
apps/api/plane/db/models/issue.py (1)
Issue(104-260)apps/api/plane/api/views/base.py (1)
project_id(144-150)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/api/plane/bgtasks/issue_activities_task.py (4)
218-229: LGTM! Well-implemented state tracking with proper validationThe state tracking implementation correctly handles both internal (
state_id) and external (state) keys, validates UUIDs, normalizes invalid values to None, and scopes queries by project_id to prevent cross-project data leaks.
333-334: LGTM! Clean implementation using the extract_ids helperThe label tracking correctly uses the new
extract_idshelper function to handle both internal and external keys.
399-401: LGTM! Consistent use of extract_ids for assigneesThe assignee tracking implementation correctly uses the
extract_idshelper for both internal and external keys.
659-663: LGTM! Well-structured mapper extension for external keysThe ISSUE_ACTIVITY_MAPPER correctly maps external endpoint keys to their respective tracking functions, enabling support for both internal and external API formats.
Description
this pull request adds support for the keys
parent,state,assigneeorlabelin the external endpoint to track the activity whenever they are changed.Type of Change
Summary by CodeRabbit
New Features
Bug Fixes