agateblue / django-dynamic-preferences

Dynamic global and instance settings for your django project
https://django-dynamic-preferences.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
350 stars 87 forks source link

`preference_updated` signal not triggered when updating preferences via `PreferenceViewSet` #310

Open KrYpTeD974 opened 2 weeks ago

KrYpTeD974 commented 2 weeks ago

Hello,

It seems that I've found an issue with the preference_updated signal in the latest version of django-dynamic-preferences.

Problem: The preference_updated signal is not triggered when I update a preference using the PreferenceViewSet or PerInstancePreferenceViewSet.

Steps to reproduce:

  1. Register a signal receiver:

    from django.dispatch import receiver
    from dynamic_preferences.signals import preference_updated
    
    @receiver(preference_updated)
    def my_signal_receiver(sender, section, name, old_value, new_value, **kwargs):
        print("Preference {} in section {} changed from {} to {}".format(
            name, section, old_value, new_value))
  2. Use the PreferenceViewSet to update a specific instance preference.
  3. Observe that the preference_updated signal is not emitted after the update.

Expected behavior: I expected the preference_updated signal to be triggered, as it seems to be the case for other preference update methods.

Possible solution Add the following code to the PreferencesViewSet:

    def perform_update(self, serializer):
        old_value = serializer.instance.value
        super().perform_update(serializer)
        preference_updated.send(
            sender=self.__class__,
            section=serializer.instance.section,
            name=serializer.instance.name,
            old_value=old_value,
            new_value=serializer.instance.value,
        )

Environment:

agateblue commented 1 week ago

Hi @KrYpTeD974, this does indeed look like a bug. Your suggested fix would work for updating individual preferences but not with the bulk endpoint.

I think adding the signal logic in the serializer here : https://github.com/agateblue/django-dynamic-preferences/blob/develop/dynamic_preferences/api/serializers.py#L64 would work for both use cases. I'd be happy to review and merge a pull request implementing this!

KrYpTeD974 commented 1 week ago

Indeed, ok fine ! I'll will try to submit a PR.