jazzband / django-model-utils

Django model mixins and utilities.
https://django-model-utils.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.65k stars 365 forks source link

FieldTracker broken in 4.1.1 #489

Closed resurrexi closed 3 years ago

resurrexi commented 3 years ago

Problem

It seems like FieldTracker isn't able to detect changes on a field anymore. I'm not sure if it's because there were changes made to its implementation that prevents it from observing a field's change state when the field's model instance is being saved.

What I would do is override the model's save method and add a "post-save hook" where I check if the field has changed, and if so, then I'll run some additional logic.

Let's take for example, I have an Employee model that's tied to an extended auth user model. If an employee is activated/deactivated, I would like the employee's associated user profile to also be activated/deactivated.

class Employee(models.Model):
    ...
    is_active = models.BooleanField(default=True)
    field_tracker = FieldTracker(fields=["is_active"])

    def save(self, *args, **kwargs):
        super().save(*args, **kwargs)

        # post-save
        if self.field_tracker.has_changed("is_active"):
            get_user_model().objects.filter(employee_profile=self).update(
                is_active=self.is_active
            )

This post-save logic would previously work on django-model-utils==4.0.0, as my unit test would pass:

class EmployeeModelTest(TestCase):
    def test_employee_active_flag_updates_login_account_active(self):
        # prep data
        employee = EmployeeFactory.create()
        user = ProfileFactory.create(employee_profile=employee)

        # first ensure user is active
        self.assertTrue(user.is_active)
        employee.is_active = False
        employee.save()

        # user should also now be deactivated
        user.refresh_from_db()
        self.assertFalse(user.is_active)  # fails here

For completeness (to rule out factoryboy), I also did a manual test, where I would log into django admin to deactivate the employee. The employee's associated user account still wouldn't deactivate.

Environment

tumb1er commented 3 years ago

Relates to #404

Before 4.1 this code will behave like this:

model_patched_save().start
  save()
    super().save() start
      save_base() start
        pre_save signal handling
        db insert/update
        post_save signal handling
      save_base() finish
    super().save() finish
    if has_changed:
      ...
  reset_tracker_state()
model_patched_save().finish

New implementation moves reset_tracker_state to earlier stage

save()
  super().save() start
    model_patched_save_base().start
      save_base() start
        pre_save signal handling
        db insert/update
        post_save signal handling
      save_base() finish
      reset_tracker_state()
    model_patched_save().finish
  super().save() finish
if has_changed:
  ...

You may check "changed" before super().save or use signals.

resurrexi commented 3 years ago

Ok. Thanks for clarifying the order of where the reset state is executed. I want to avoid using signals as much as possible so I'll just move the check before the super().save call.