Skip to content

Conversation

@FranciscodeMaussion
Copy link

@FranciscodeMaussion FranciscodeMaussion commented Sep 26, 2025

Problem

Monitor field does not gets populated on creation since the previous != current evaluates to False. The problem is when when is setted and not respected on creation.
Related issues:

Solution

Now 2 checks are done:

  • when is None we use previous != current
  • when is not None:
  • we use previous != current and check if current in when
  • (change here) check if is add and current in when

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests.
  • Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
  • Pay attention to backward compatibility, or if it breaks it, explain why.
  • Update documentation (if relevant).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in MonitorField where the field would not be populated on object creation when the when parameter is specified and the initial value matches a value in the when list. The issue occurred because during creation, the previous and current values were equal (both set to the initial value), causing the previous != current check to fail even when the value was in the when list.

Key changes:

  • Modified the pre_save logic to check if it's a creation operation (add=True) when when is specified, allowing the monitor field to update on creation if the value is in the when list
  • Added a new test model MonitorWhenNullable to test the nullable + when combination
  • Added tests to verify the fix works correctly for both matching and non-matching values on creation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
model_utils/fields.py Refactored pre_save logic to handle creation with when parameter by adding or add condition when checking if the monitor field should update
tests/models.py Added MonitorWhenNullable model to test the combination of when parameter with null=True
tests/test_fields/test_monitor_field.py Added MonitorWhenNullableFieldTests class with tests for creation scenarios with matching and non-matching values
CHANGES.rst Added changelog entry documenting the fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add support for `Python 3.13` (GH-#628)
- Add formal support for `Django 5.2` (GH-#641)
- Drop support for older versions than `Django 4.2`
- Fixes MonitorField not updating on creation with `when` and `null=True`
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog entry should reference the GitHub issue numbers. According to the PR description, this fixes issues GH-401 and GH-352. The entry should follow the format used in other entries, such as "Fixes MonitorField not updating on creation with when and null=True (GH-#401, GH-#352)".

Suggested change
- Fixes MonitorField not updating on creation with `when` and `null=True`
- Fixes MonitorField not updating on creation with `when` and `null=True` (GH-#401, GH-#352)

Copilot uses AI. Check for mistakes.
Comment on lines +94 to +109
class MonitorWhenNullableFieldTests(TestCase):
"""
Will record changes only when name is 'Jose'
"""
def test_created_Jose(self) -> None:
with time_machine.travel(datetime(2016, 1, 1, 12, 0, 0, tzinfo=timezone.utc)):
instance = MonitorWhenNullable(name='Jose')
instance.save()
self.assertEqual(instance.name_changed, datetime(2016, 1, 1, 12, 0, 0, tzinfo=timezone.utc))

def test_created_Charlie(self) -> None:
with time_machine.travel(datetime(2016, 1, 1, 12, 0, 0, tzinfo=timezone.utc)):
instance = MonitorWhenNullable(name='Charlie')
instance.save()
self.assertIsNone(instance.name_changed)

Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test class lacks coverage for update scenarios. Consider adding tests for:

  1. Updating from a non-matching value (e.g., 'Charlie') to a matching value ('Jose') to verify the monitor field updates correctly
  2. Updating from a matching value ('Jose') to a non-matching value to verify the monitor field remains unchanged
  3. Updating from one matching value to another matching value (if multiple values in 'when')
  4. Double save behavior similar to the MonitorWhenFieldTests class

These tests would ensure the fix doesn't break existing update functionality and only affects creation behavior.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant