Watching for ForeignKey value changes seems to not trigger the hook

[michaeljohnbarr]

Using the below example in version 0.6.0, I am expecting to print a message any time someone changes a user's first name that is saved in SomeModel. From what I can tell from the documentation, I am setting everything up correctly. Am I misunderstanding how this works?

Assuming a model set up like this:

from django.conf import settings
from django.db import models
from django_lifecycle import LifecycleModel, hook

class SomeModel(LifecycleModel):
    user = models.ForeignKey(
        on_delete=models.CASCADE,
        to=settings.AUTH_USER_MODEL
    )
    # More fields here...

    @hook('after_update', when='user.first_name', has_changed=True)
    def user_first_name_changed(self):
        print(
            f"User's first_name has changed from "
            f"{self.initial_value('user.first_name')} to {user.first_name}!"
        )

When we then perform the following code, nothing prints:

from django.contrib.auth import get_user_model

# Create a test user (Jane Doe)
get_user_model().objects.create_user(
    username='test', 
    password=None, 
    first_name='Jane', 
    last_name='Doe'
)

# Create an instance
SomeModel.objects.create(user=user)

# Retrieve a new instance of the user
user = get_user_model().objects.get(username='test')

# Change the name (John Doe)
user.first_name = 'John'
user.save()

# Nothing prints from the hook

In the tests, I see that it is calling user_account._clear_watched_fk_model_cache() explicitly after changing the Organization.name. However, from looking at the code, I do not see this call anywhere except for the overridden UserAccount.save() method. Thus, saving the Organization has no way to notify the UserAccount that a change has been made, and therefore, the hook cannot possibly be fired. The only reason that I can see that the test is passing is because of the explicit call to user_account._clear_watched_fk_model_cache().

    def test_has_changed_is_true_if_fk_related_model_field_has_changed(self):
        org = Organization.objects.create(name="Dunder Mifflin")
        UserAccount.objects.create(**self.stub_data, organization=org)
        user_account = UserAccount.objects.get()

        org.name = "Dwight's Paper Empire"
        org.save()
        user_account._clear_watched_fk_model_cache()
        self.assertTrue(user_account.has_changed("organization.name"))

[rsinger86]

I appreciate the detailed issue. I'll look into this.

[rsinger86]

I think I know what's happening. You want a model's hook to fire, upon making updates to its FK-related model. This isn't supported. The hook will only fire when you make the change to the model itself: the SomeModel instance in your example.

from django.contrib.auth import get_user_model

# Create a test user (Jane Doe)
get_user_model().objects.create_user(
    username='test', 
    password=None, 
    first_name='Jane', 
    last_name='Doe'
)

# Create an instance
some_instance = SomeModel.objects.create(user=user)

# Retrieve a new instance of the user
user = get_user_model().objects.get(username='test')

# Change the name (John Doe)
user.first_name = 'John'
user.save()

# The hook will fire and it will print the message
some_instance.save()

Maybe the docs should be updated to clarify? I'll think a little more about supporting this behavior down the line. Though, to be honest, I'm not quite sure if the "FK changes" feature should be included in the first place because of lack of support for handling it efficiently, ORM-wise.

[michaeljohnbarr]

Yes, that is exactly what I was expecting. The documentation on ForeignKey Field Value Changes shows an example of a hook of this nature, but doesn't show the example required to trigger it, so I do believe that a documentation update would be useful. 😃

We (at my company) are writing an event-driven system where saves on one model can impact a callback chain on another. I have to do something like "when a user changes their name, go to this other model and update this other thing." As a "workaround," we added some hooks on the user model to achieve this. Now that I understand what the limitations are, I'll make sure that our code handles ForeignKey changes this way.

I do have some ideas for this particular feature request/update in the future if you are interested. After creating this issue and looking at the code, I was fairly certain that what I was trying to do was not supported, so I tried figuring out a way to add support for it. Would you be interested in me researching this?

[michaeljohnbarr]

... Though, to be honest, I'm not quite sure if the "FK changes" feature should be included in the first place because of lack of support for handling it efficiently, ORM-wise.

I believe that if it became supported, efficiency would be in the hands of the developer. A nice little warning should suffice, just as it is now in the existing documentation. However, I can see the issue that in order to call from a specific model to a child model, the best way would be to provide some sort of functionality for calling select_related/prefetch_related on to child model prior to calling the hook. E.G. something along the lines of:

class SomeModel(models.Model):
    A_TO_M = 0
    N_TO_Z = 1

    GROUPING_CHOICES = (
        (A_TO_M, _('A to M')),
        (N_TO_Z, _('N to Z'))
    )

    user = models.ForeignKey(
        on_delete=models.CASCADE,
        to=settings.AUTH_USER_MODEL
    )
    last_name_group = models.PositiveSmallIntegerField(
        choices=GROUPING_CHOICES
    )
    # ...

    @hook(
        hook='after_save', 
        when='user.last_name', 
        has_changed=True, 
        is_not='',
        select_related=['user'],
        prefetch_related=['samples', models.Prefetch('others')]
    )
    def check_last_name_group(self):
        if self.user.last_name.lower() in string.ascii_lowercase[:13]:
            self.last_name_group = A_TO_M
        elif self.user.last_name.lower() in string.ascii_lowercase[14:]:
            self.last_name_group = O_TO_Z
        else:
            raise ValueError('Oops. We need to figure out what goes here.')

The select_related and prefetch_related could be called from the settings.AUTH_USER_MODEL after save dynamically.

[rsinger86]

Apologies for the radio silence - I appreciate the thought here. I just don't think I want to expand the scope of this project to support a feature of this complexity, given that it seems to be a pretty rare use case. Furthermore, handling chains of changes like this with application code in-memory seems a little shaky... I think it would be better solved with database triggers/functions.