[WEB-5285] feat: add ChangeTrackerMixin to track model field changes and original values#8145
Conversation
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
Caution Review failedThe pull request is closed. WalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a ChangeTrackerMixin for Django models to track field changes between model initialization and save operations, providing a clean API for detecting which fields have been modified.
Key Changes:
- Added
ChangeTrackerMixinclass with change detection capabilities - Provides
has_changed(),changed_fields, andold_valuesAPIs - Supports selective field tracking via
TRACKED_FIELDSattribute - Automatically excludes deferred fields to prevent unnecessary database queries
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fields that are being tracked (either via TRACKED_FIELDS or | ||
| all non-deferred fields). | ||
| """ | ||
| return self._original_values |
There was a problem hiding this comment.
Returning the internal _original_values dictionary directly exposes it to external modification. Consider returning a copy to prevent unintended mutations:
@property
def old_values(self) -> dict[str, Any]:
"""
Get a dictionary of the original field values from initialization.
Returns:
dict: A dictionary mapping field names to their original values
as they were when the instance was initialized. Only includes
fields that are being tracked (either via TRACKED_FIELDS or
all non-deferred fields).
"""
return self._original_values.copy()This prevents callers from accidentally modifying the tracked values, which could lead to incorrect change detection.
| return self._original_values | |
| return self._original_values.copy() |
| changed = [] | ||
| for field, old_val in self._original_values.items(): | ||
| new_val = getattr(self, field) | ||
| if old_val != new_val: | ||
| changed.append(field) | ||
| return changed |
There was a problem hiding this comment.
The equality comparison old_val != new_val may not work correctly for mutable field types like JSONField, ArrayField, or related objects. Django field values for these types might be different object instances with the same content, leading to false positives for changes.
Consider using Django's field comparison or deep equality checks for complex field types. For example:
@property
def changed_fields(self) -> list[str]:
"""
Get a list of all fields that have changed since initialization.
Returns:
list[str]: A list of field names that have different values than
when the instance was initialized. Returns an empty list
if no fields have changed.
"""
changed = []
for field_name, old_val in self._original_values.items():
new_val = getattr(self, field_name)
# Use field-specific comparison for complex types
field = self._meta.get_field(field_name)
if hasattr(field, 'to_python'):
# Compare using field's internal comparison
if old_val != new_val:
changed.append(field_name)
else:
if old_val != new_val:
changed.append(field_name)
return changedThis ensures accurate change detection for all Django field types, including JSON fields which are commonly used in this codebase (as seen in models like Integration and WorkspaceIntegration).
| """ | ||
| if field_name not in self._original_values: | ||
| return False | ||
| return self._original_values[field_name] != getattr(self, field_name) |
There was a problem hiding this comment.
The equality comparison in has_changed may not work correctly for mutable field types like JSONField, ArrayField, or related objects, which are commonly used in this codebase. This is the same issue as in the changed_fields property.
The comparison should be consistent with the changed_fields property to ensure has_changed(field) and field in changed_fields always return the same result.
| def _track_fields(self) -> None: | ||
| """ | ||
| Capture the initial values of fields to track. | ||
|
|
||
| This method stores the current values of fields that should be tracked. | ||
| If TRACKED_FIELDS is defined on the model, only those fields are tracked. | ||
| Otherwise, all non-deferred fields are tracked. Deferred fields are | ||
| automatically excluded to prevent unnecessary database queries. | ||
| """ | ||
| deferred_fields = self.get_deferred_fields() | ||
| tracked_fields = getattr(self, "TRACKED_FIELDS", None) | ||
| if tracked_fields: | ||
| for field in tracked_fields: | ||
| if field not in deferred_fields: | ||
| self._original_values[field] = getattr(self, field) | ||
| else: | ||
| for field in self._meta.fields: | ||
| if field.attname not in deferred_fields: | ||
| self._original_values[field.attname] = getattr(self, field.attname) |
There was a problem hiding this comment.
Storing field values directly with getattr(self, field) creates a shallow reference to mutable objects (like lists from ArrayField or dicts from JSONField). If the original object is mutated in place (e.g., issue.metadata['key'] = 'value'), the tracked value will also change, preventing accurate change detection.
Consider deep copying mutable field values during tracking:
import copy
def _track_fields(self) -> None:
"""
Capture the initial values of fields to track.
...
"""
deferred_fields = self.get_deferred_fields()
tracked_fields = getattr(self, "TRACKED_FIELDS", None)
if tracked_fields:
for field in tracked_fields:
if field not in deferred_fields:
value = getattr(self, field)
# Deep copy mutable types to prevent reference issues
self._original_values[field] = copy.deepcopy(value) if isinstance(value, (dict, list)) else value
else:
for field in self._meta.fields:
if field.attname not in deferred_fields:
value = getattr(self, field.attname)
self._original_values[field.attname] = copy.deepcopy(value) if isinstance(value, (dict, list)) else valueThis ensures that in-place mutations don't affect the original tracked values.
| abstract = True | ||
|
|
||
|
|
||
| class ChangeTrackerMixin: |
There was a problem hiding this comment.
The ChangeTrackerMixin should inherit from models.Model to ensure proper Method Resolution Order (MRO) when used with other Django model mixins. Django model mixins should typically inherit from models.Model with abstract = True in the Meta class.
Suggested fix:
class ChangeTrackerMixin(models.Model):
"""
A mixin to track changes in model fields between initialization and save.
...
"""
_original_values: dict[str, Any]
class Meta:
abstract = True
def __init__(self, *args: Any, **kwargs: Any) -> None:
...This ensures compatibility with Django's model system and prevents potential MRO issues when combined with other mixins like AuditModel.
…and original values makeplane#8145
…and original values makeplane#8145
…and original values makeplane#8145
Description
This PR introduces
ChangeTrackerMixin, a Django model mixin that enables tracking of field changes between model initialization and save operations. This provides a clean way to detect which fields have been modified on model instances.Features
TRACKED_FIELDSto track specific fields, or track all non-deferred fields by defaulthas_changed(field_name: str) -> bool: Check if a specific field changedchanged_fields: list[str]: Get all changed field namesold_values: dict[str, Any]: Get original field valuesUsage
Use Cases
Notes
AuditModel,SoftDeleteModel, etc.)Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.