Hooks do not fire reliably with "+" option

[briankung]

I noticed that hooks with the "+" option were not being triggered for all users as I'd expected from the documentation - at least, not reliably.

From the documentation:

If you want a Hook to be triggered for all users, add '+' to built-in Hooks or pass user_override=False for custom_hook events

However, in utils.py, user_override looks like it is set to True if there is a plus sign: https://github.com/zapier/django-rest-hooks/blob/master/rest_hooks/utils.py#L82

            if model == maybe_model and action == maybe_action[0]:
                event_name = maybe_event_name
                if len(maybe_action) == 2:
                    user_override = True

    if event_name:
        find_and_fire_hook(event_name, instance, user_override=user_override)

In find_and_fire_hook, meanwhile, user_override=True is not accounted for:

    if user_override is not False:
        if user_override:
            filters['user'] = user_override
        elif hasattr(instance, 'user'):
            filters['user'] = instance.user
        elif isinstance(instance, User):
            filters['user'] = instance
        else:
            raise Exception(
                '{} has no `user` property. REST Hooks needs this.'.format(repr(instance))
            )

       # ...comments...

    hooks = Hook.objects.filter(**filters)
    for hook in hooks:
        hook.deliver_hook(instance)

Setting filters['user'] = user_override results in filters['user'] = True. This means that Hook.objects.filter(**filters) will filter for user=True. I was very confused what that meant - I thought it reasonable to assume that it would might be equivalent to user__isnull=False, but the actual result is stranger. I tested it with a few of our own models.

It turns out that if you ask for SoAndSoModel.objects.filter(related_object=True), Django casts related_object=True to related_object_id=1 and related_object=False to related_object_id=0 in the query.

pprint(connection.queries)
# For SoAndSoModel.objects.filter(related_object=True)
[{'sql': 'SELECT "thing_SoAndSoModel"."id", '
         # ...
         'WHERE "thing_SoAndSoModel"."related_object_id" = 1 LIMIT 21',
  'time': '0.004'},

# For SoAndSoModel.objects.filter(related_object=False)
 {'sql': 'SELECT "thing_SoAndSoModel"."id", '
         # …
         'WHERE "thing_SoAndSoModel"."related_object_id" = 0 LIMIT 21',
  'time': '0.001’}]

Which means that rest_hooks is only returning hooks where user_id=1.

That's my theory, anyway. If I have a moment later, I'll write a test to confirm.

[avelis]

@briankung If you find the time to make a pull request I can happily take a look and merge it in.

[briankung]

Hey, sorry, I couldn't devote enough time to figure out how to write a simple test for it. The function is difficult to test. I could submit a PR without a test, but that makes me uncomfortable.

[briankung]

Would it be correct to set user_override to False if len(maybe_action) == 2?

[avelis]

@briankung I believe you assessment is correct. If we changed user_override=False it should operate as expected. I still would try to get a unit test on it though.

I merged this enhancement in via a pull-request from another developer.

In an effort to be thorough I am CC'ing @GVRV to make sure such a change wouldn't create unintended consequences.

I doubtful that this is a consequence of using different Django versions.

I will still be looking into a way to confirm expected behavior via a unit test.

[imsickofmaps]

I can confirm this is a bug having spent two hours trying to confirm the same thing (without looking at the open issues! Idiot!). #21 (comment) is the correct fix. The unit tests on this don't provide enough existing coverage to making adding a test just for this a short task! Are you still set on not patching without one?

[avelis]

@imsickofmaps I have pushed the change in b3387f6 Pull from master and let me know if that addresses this issue. I can then move to make a release.

[imsickofmaps]

👍 works for me... thanks!

[imsickofmaps]

@avelis can we have a release now?

[RadoRado]

Agreed - the changes work.

[pgergov]

I confirm the change fixes the problem.

[avelis]

Fixed and released in v1.2.2. Release has also been pushed to PyPi.

Links:
https://github.com/zapier/django-rest-hooks/releases/tag/1.2.2
https://pypi.python.org/pypi/django-rest-hooks/1.2.2

[imsickofmaps]

@avelis -->