Ensure dirty-bit is updated when calling save on a syncable model with update_fields#326
Ensure dirty-bit is updated when calling save on a syncable model with update_fields#326bjester wants to merge 3 commits intolearningequality:release-v0.8.xfrom
update_fields#326Conversation
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean, minimal fix for a nasty P0 edge case.
CI passing. No UI files changed — visual inspection skipped.
- suggestion:
release-v0.9.xhas the same unfixedsavemethod (confirmed by reading the branch). Since this is P0, a follow-up PR or backport to the default branch is worth tracking. - suggestion: (inline) consider a test for the
update_dirty_bit_to=None+update_fieldsopt-out path - praise: (inline) the
update_dirty_bit_to is not Noneguard correctly scopes the fix
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| self._morango_dirty_bit = False | ||
|
|
||
| # ensure the dirty bit field is in the fields to update if present, to keep it in sync | ||
| if update_dirty_bit_to is not None and kwargs.get("update_fields") is not None: |
There was a problem hiding this comment.
praise: The update_dirty_bit_to is not None guard correctly handles the opt-out case — callers who pass None to suppress all dirty-bit handling also suppress the update_fields injection, which is exactly right.
|
|
||
| user.username = "updated-name" | ||
| user.save(update_fields=["username"]) | ||
| self.assertTrue(MyUser.objects.first()._morango_dirty_bit) |
There was a problem hiding this comment.
suggestion: This test covers the common case well. Consider adding a second assertion for save(update_dirty_bit_to=None, update_fields=["username"]) to verify that the dirty bit is NOT updated in the DB when callers explicitly opt out — the fix handles it correctly, but a test would prevent regression if the guard condition is ever changed.
2 participants
Summary
update_fieldswhen presentTODO
Reviewer guidance
Issues addressed
closes #325
AI usage
The fix was pretty clear, but I let AI take the wheel. It wrote the regression test, then we verified the issue, then proceeded with the fix.