[change] Reworked config_modified implementation openwisp#394

- the signal is now always emitted on templates changes m2m events,
  also if config.status is modified, with the differences that
  only post_add and post_remove m2m events are used, while
  post_clear is ignored, which fixes the duplicate signal emission
  caused by the implementation of sortedm2m;
  i made sure to document these details in the README
- added action and previous_status arguments, which allow to
  understand where the config_modified signal is being emitted from,
  this allows more advanced usage of the signal by custom implementations

Closes openwisp#394

README.rst

@@ -969,17 +969,40 @@ Signals
 **Arguments**:
 
 - ``instance``: instance of ``Config`` which got its ``config`` modified
+- ``previous_status``: indicates the status of the config object before the
+  signal was emitted
+- ``action``: action which emitted the signal, can be any of the list below:
+  - ``config_changed``: the configuration of the config object was changed
+  - ``related_template_changed``: the configuration of a related template was changed
+  - ``m2m_templates_changed``: the assigned templates were changed
+  (either templates were added, removed or their order was changed)
 
 This signal is emitted every time the configuration of a device is modified.
 
 It does not matter if ``Config.status`` is already modified, this signal will
 be emitted anyway because it signals that the device configuration has changed.
 
-It is not triggered when the device is created for the first time.
-
 This signal is used to trigger the update of the configuration on devices,
 when the push feature is enabled (requires Device credentials).
 
+The signal is also emitted when one of the templates used by the device
+is modified or if the templates assigned to the device are changed.
+
+Special cases in which ``config_modified`` is not emitted
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+This signal is not emitted when the device is created for the first time.
+
+It is also not emitted when templates assigned to a config object are
+cleared (``post_clear`` m2m signal), this is necessary because
+`sortedm2m <https://github.com/jazzband/django-sortedm2m>`_, the package
+we use to implement ordered templates, uses the clear action to
+reorder templates (m2m relationships are first cleared and then added back),
+therefore we ignore ``post_clear`` to avoid emitting signals twice
+(one for the clear action and one for the add action).
+Please keep this in mind if you plan on using the clear method
+of the m2m manager.
+
 ``config_status_changed``
 ~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -991,6 +1014,11 @@ when the push feature is enabled (requires Device credentials).
 
 This signal is emitted only when the configuration status of a device has changed.
 
+The signal is emitted also when the m2m template relationships of a config
+object are changed, but only on ``post_add`` or ``post_remove`` actions,
+``post_clear`` is ignored for the same reason explained
+in the previous section.
+
 ``checksum_requested``
 ~~~~~~~~~~~~~~~~~~~~~~
 

openwisp_controller/config/base/config.py

@@ -1,7 +1,7 @@
 import collections
 
 from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError
-from django.db import models
+from django.db import models, transaction
 from django.utils.translation import ugettext_lazy as _
 from jsonfield import JSONField
 from model_utils import Choices
@@ -69,14 +69,20 @@ class AbstractConfig(BaseConfig):
         load_kwargs={'object_pairs_hook': collections.OrderedDict},
         dump_kwargs={'indent': 4},
     )
-    # for internal usage
-    _just_created = False
 
     class Meta:
         abstract = True
         verbose_name = _('configuration')
         verbose_name_plural = _('configurations')
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        # for internal usage
+        self._just_created = False
+        self._initial_status = self.status
+        self._send_config_modified_after_save = False
+        self._send_config_status_changed = False
+
     def __str__(self):
         if self._has_device():
             return self.name
@@ -152,11 +158,26 @@ def templates_changed(cls, action, instance, **kwargs):
         """
         this method is called from a django signal (m2m_changed)
         see config.apps.ConfigConfig.connect_signals
+
+        NOTE: post_clear is ignored because it is used by the
+        sortedm2m package to reorder templates
+        (m2m relationships are first cleared and then added back),
+        there fore we need to ignore it to avoid emitting signals twice
         """
-        if action not in ['post_add', 'post_remove', 'post_clear']:
+        if action not in ['post_add', 'post_remove']:
             return
-        if instance.status != 'modified':
-            instance.set_status_modified()
+        # use atomic to ensure any code bound to
+        # be executed via transaction.on_commit
+        # is executed after the whole block
+        with transaction.atomic():
+            # do not send config modified signal if
+            # config instance has just been created
+            if not instance._just_created:
+                # sends only config modified signal
+                instance._send_config_modified_signal(action='m2m_templates_changed')
+            if instance.status != 'modified':
+                # sends both status modified and config modified signals
+                instance.set_status_modified(send_config_modified_signal=False)
 
     @classmethod
     def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
@@ -311,22 +332,30 @@ def save(self, *args, **kwargs):
             default_templates = self.get_default_templates()
             if default_templates:
                 self.templates.add(*default_templates)
-        if not created and getattr(self, '_send_config_modified_after_save', False):
-            self._send_config_modified_signal()
-            self._send_config_modified_after_save = None
-        if getattr(self, '_send_config_status_changed', False):
+        if not created and self._send_config_modified_after_save:
+            self._send_config_modified_signal(action='config_changed')
+            self._send_config_modified_after_save = False
+        if self._send_config_status_changed:
             self._send_config_status_changed_signal()
-            self._send_config_status_changed = None
+            self._send_config_status_changed = False
+        self._initial_status = self.status
         return result
 
-    def _send_config_modified_signal(self):
+    def _send_config_modified_signal(self, action):
         """
         Emits ``config_modified`` signal.
         Called also by Template when templates of a device are modified
         """
+        assert action in [
+            'config_changed',
+            'related_template_changed',
+            'm2m_templates_changed',
+        ]
         config_modified.send(
             sender=self.__class__,
             instance=self,
+            previous_status=self._initial_status,
+            action=action,
             # kept for backward compatibility
             config=self,
             device=self.device,
@@ -343,10 +372,11 @@ def _set_status(self, status, save=True):
         self.status = status
         self._send_config_status_changed = True
         if save:
-            self.save()
+            self.save(update_fields=['status'])
 
-    def set_status_modified(self, save=True):
-        self._send_config_modified_after_save = True
+    def set_status_modified(self, save=True, send_config_modified_signal=True):
+        if send_config_modified_signal:
+            self._send_config_modified_after_save = True
         self._set_status('modified', save)
 
     def set_status_applied(self, save=True):

openwisp_controller/config/base/template.py

@@ -127,14 +127,22 @@ def save(self, *args, **kwargs):
             )
 
     def _update_related_config_status(self):
-        changing_status = list(self.config_relations.exclude(status='modified').values_list('pk', flat=True))
-        self.config_relations.update(status='modified')
-        for config in self.config_relations.select_related('device').iterator():
-            # config modified signal sent regardless
-            config._send_config_modified_signal()
-            # config status changed signal sent only if status changed
-            if config.pk in changing_status:
-                config._send_config_status_changed_signal()
+        # use atomic to ensure any code bound to
+        # be executed via transaction.on_commit
+        # is executed after the whole block
+        with transaction.atomic():
+            changing_status = list(
+                self.config_relations.exclude(status='modified').values_list(
+                    'pk', flat=True
+                )
+            )
+            for config in self.config_relations.select_related('device').iterator():
+                # config modified signal sent regardless
+                config._send_config_modified_signal(action='related_template_changed')
+                # config status changed signal sent only if status changed
+                if config.pk in changing_status:
+                    config._send_config_status_changed_signal()
+            self.config_relations.exclude(status='modified').update(status='modified')
 
     def clean(self, *args, **kwargs):
         """

openwisp_controller/config/signals.py

@@ -4,7 +4,9 @@
 config_download_requested = Signal(providing_args=['instance', 'request'])
 config_status_changed = Signal(providing_args=['instance'])
 # device and config args are maintained for backward compatibility
-config_modified = Signal(providing_args=['instance', 'device', 'config'])
+config_modified = Signal(
+    providing_args=['instance', 'device', 'config', 'previous_status', 'action']
+)
 device_registered = Signal(providing_args=['instance', 'is_new'])
 management_ip_changed = Signal(
     providing_args=['instance', 'management_ip', 'old_management_ip']

openwisp_controller/config/tests/test_config.py

@@ -576,15 +576,26 @@ def test_config_modified_sent(self):
                 instance=c,
                 device=c.device,
                 config=c,
+                previous_status='applied',
+                action='config_changed',
             )
             self.assertEqual(c.status, 'modified')
 
         with catch_signal(config_modified) as handler:
             c.config = {'general': {'description': 'changed again'}}
             c.full_clean()
+            # repeated on purpose
             c.full_clean()
             c.save()
-            handler.assert_called_once()
+            handler.assert_called_once_with(
+                sender=Config,
+                signal=config_modified,
+                instance=c,
+                device=c.device,
+                config=c,
+                previous_status='modified',
+                action='config_changed',
+            )
             self.assertEqual(c.status, 'modified')
 
     def test_config_get_system_context(self):
@@ -593,3 +604,12 @@ def test_config_get_system_context(self):
         )
         system_context = config.get_system_context()
         self.assertNotIn('test', system_context.keys())
+
+    def test_initial_status(self):
+        config = self._create_config(
+            organization=self._get_org(), context={'test': 'value'}
+        )
+        self.assertEqual(config._initial_status, config.status)
+        config.status = 'modified'
+        config.save()
+        self.assertEqual(config._initial_status, 'modified')