Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FieldTracker broken in 4.1.1 #489

Closed
resurrexi opened this issue May 13, 2021 · 2 comments
Closed

FieldTracker broken in 4.1.1 #489

resurrexi opened this issue May 13, 2021 · 2 comments

Comments

@resurrexi
Copy link

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

  • Django Model Utils version: 4.1.1
  • Django version: 2.2.22
  • Python version: 3.8.3
  • Other libraries used, if any: factory-boy==2.12.0 for testing
@tumb1er
Copy link
Contributor

tumb1er commented May 13, 2021

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
Copy link
Author

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.

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

No branches or pull requests

2 participants