feat(ecosystem): Implement cross-system issue synchronization#7
feat(ecosystem): Implement cross-system issue synchronization#7everettbu wants to merge 1 commit into
Conversation
WalkthroughThis change introduces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
tests/sentry/models/test_groupassignee.py (1)
211-212: Consider simplifying nested with statements.The static analysis tool correctly identified that nested
withstatements can be combined for better readability.Apply this diff to simplify the nested context managers:
- with self.feature({"organizations:integrations-issue-sync": True}): - with self.tasks(): + with self.feature({"organizations:integrations-issue-sync": True}), self.tasks():
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/sentry/integrations/mixins/issues.py(4 hunks)src/sentry/integrations/services/assignment_source.py(1 hunks)src/sentry/integrations/tasks/sync_assignee_outbound.py(3 hunks)src/sentry/integrations/utils/sync.py(4 hunks)src/sentry/models/groupassignee.py(5 hunks)tests/sentry/integrations/services/test_assignment_source.py(1 hunks)tests/sentry/models/test_groupassignee.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/sentry/models/test_groupassignee.py
211-212: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🔇 Additional comments (21)
tests/sentry/integrations/services/test_assignment_source.py (1)
7-39: Comprehensive test coverage for AssignmentSource.The test class provides good coverage of the core functionality:
- Edge case handling for empty and invalid data
- Proper serialization/deserialization validation
- Verification of all expected fields in the output dictionary
The tests properly validate that
from_dictreturnsNonefor invalid inputs and correctly instantiates objects for valid data.src/sentry/integrations/tasks/sync_assignee_outbound.py (2)
30-35: Good backward compatibility with optional parameter.The addition of the optional
assignment_source_dictparameter maintains backward compatibility while enabling the new assignment source tracking functionality.
53-61: Proper handling of assignment source deserialization.The code correctly:
- Deserializes the assignment source dictionary only when provided
- Passes the parsed assignment source to both
should_syncandsync_assignee_outbound- Handles the case where no assignment source is provided
The
AssignmentSource.from_dictmethod will returnNonefor invalid data, which is handled appropriately.src/sentry/integrations/services/assignment_source.py (2)
21-25: Well-designed factory method.The
from_integrationmethod provides a clean way to createAssignmentSourceinstances from integration objects, properly extracting the required fields.
31-35: Robust error handling in deserialization.The
from_dictmethod properly handles invalid input by catching bothValueErrorandTypeErrorexceptions and returningNone, which aligns with the test expectations.src/sentry/integrations/utils/sync.py (4)
96-99: Proper assignment source tracking in inbound sync.The deassignment call correctly includes the assignment source derived from the integration, enabling proper tracking of the operation's origin.
112-116: Consistent assignment source propagation.The assignment call properly includes the assignment source, maintaining consistency with the deassignment flow above.
123-128: Good API design for outbound sync.The function signature extension with the optional
assignment_sourceparameter maintains backward compatibility while enabling the new functionality.
137-145: Proper serialization for async task.The code correctly:
- Serializes the assignment source to a dictionary when provided
- Passes
Nonewhen no assignment source is available- Maintains all existing task parameters
src/sentry/models/groupassignee.py (4)
138-138: Good parameter addition for assignment tracking.The optional
assignment_sourceparameter maintains backward compatibility while enabling the new assignment source tracking functionality.
192-194: Proper propagation to sync utilities.The assignment source is correctly passed through to the outbound sync function, ensuring the assignment origin is tracked throughout the sync pipeline.
207-207: Consistent parameter pattern in deassign.The deassign method follows the same pattern as assign, maintaining API consistency.
238-240: Consistent sync integration in deassign.The deassignment sync call properly includes the assignment source, maintaining consistency with the assignment flow.
tests/sentry/models/test_groupassignee.py (4)
7-7: LGTM!The import of
AssignmentSourceis correctly added to support the new assignment source tracking functionality.
152-177: LGTM! Comprehensive test coverage for assignment with source tracking.The test correctly verifies that:
sync_assignee_outboundis called withassignment_source=None- The assignment is properly created
- Activity data is correctly recorded
The additional assertions strengthen the test coverage.
179-236: Excellent test for preventing sync loops.This test properly validates the critical behavior where outbound sync is prevented when the assignment source matches the target integration, which is essential for avoiding infinite sync loops between integrations.
The test correctly:
- Creates an integration and external issue
- Uses
AssignmentSource.from_integration(integration)as the source- Verifies
sync_assignee_outboundis NOT called- Confirms the assignment and activity are still properly created
274-276: LGTM! Consistent parameter usage.The test correctly includes
assignment_source=Nonein the deassign test assertion, maintaining consistency with the updated method signature.src/sentry/integrations/mixins/issues.py (4)
15-15: LGTM!The import of
AssignmentSourceis correctly added to support the new assignment source tracking functionality.
66-67: LGTM! Consistent method signature update.The base class method signature is correctly updated to accept the optional
sync_sourceparameter, maintaining consistency with the concrete implementation.
382-394: Excellent implementation of sync loop prevention.The logic correctly prevents sync cycles by:
- Checking if a
sync_sourceis provided- Comparing the source integration ID with the current integration ID
- Returning
Falseto prevent sync when they matchThis is a critical safety mechanism that allows other integrations to propagate changes while preventing an integration from syncing back to itself.
The implementation follows the fail-safe principle by only preventing sync when there's a definitive match, allowing normal sync behavior in all other cases.
411-418: LGTM! Method signature correctly updated.The abstract method signature is properly updated to include the optional
assignment_sourceparameter, which will be implemented by concrete integration classes to support assignment source tracking in status synchronization.
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
Fix mutable default argument issue.
Using timezone.now() as a default argument creates a mutable default that is evaluated at class definition time, not at instance creation time. This means all instances will have the same timestamp if created without explicitly providing a queued value.
- queued: datetime = timezone.now()
+ queued: datetime = None
+
+ def __post_init__(self):
+ if self.queued is None:
+ object.__setattr__(self, 'queued', timezone.now())Alternatively, use a factory function:
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)You'll need to import field from dataclasses for the second approach.
📝 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.
| queued: datetime = timezone.now() | |
| # at the top of src/sentry/integrations/services/assignment_source.py | |
| from dataclasses import field | |
| # inside the AssignmentSource dataclass | |
| queued: datetime = field(default_factory=timezone.now) |
🤖 Prompt for AI Agents
In src/sentry/integrations/services/assignment_source.py at line 18, the default
argument for the queued field is set to timezone.now(), which is evaluated once
at class definition, causing all instances to share the same timestamp. To fix
this, replace the default value with a default factory by importing field from
dataclasses and setting queued to field(default_factory=timezone.now) so that
timezone.now() is called each time a new instance is created.
| result = AssignmentSource.from_dict(data) | ||
| assert result is None | ||
|
|
||
| def test_from_dict_inalid_data(self): |
There was a problem hiding this comment.
Fix typo in method name.
The method name has a typo: test_from_dict_inalid_data should be test_from_dict_invalid_data.
- def test_from_dict_inalid_data(self):
+ def test_from_dict_invalid_data(self):📝 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.
| def test_from_dict_inalid_data(self): | |
| def test_from_dict_invalid_data(self): |
🤖 Prompt for AI Agents
In tests/sentry/integrations/services/test_assignment_source.py at line 13,
rename the method from test_from_dict_inalid_data to test_from_dict_invalid_data
to correct the typo in the method name.
2 participants
Test 7
Summary by CodeRabbit
New Features
Bug Fixes
Tests