Merge pull request #94 from rsinger86/update-fields-hook-tweaks

Only check if fields actually updated for certain condition checks

Author: rsinger86

django_lifecycle/mixins.py

@@ -205,27 +205,35 @@ def _run_hooked_methods(self, hook: str, **kwargs) -> List[str]:
 
                 when_field = callback_specs.get("when")
                 when_any_field = callback_specs.get("when_any")
-
-                # None is explicit instead of an empty list; since Django aborts the save with an empty list
                 update_fields = kwargs.get("update_fields", None)
-                update_fields_exist = update_fields is not None
+                is_partial_fields_update = update_fields is not None
 
                 if when_field:
-                    if update_fields_exist and when_field not in update_fields:
-                        continue
-                    if not self._check_callback_conditions(when_field, callback_specs):
+                    if not self._check_callback_conditions(
+                        when_field,
+                        callback_specs,
+                        is_synced=(
+                            is_partial_fields_update is False
+                            or when_field in update_fields
+                        ),
+                    ):
                         continue
                 elif when_any_field:
-                    filtered_when_any_fields = when_any_field
-                    if update_fields_exist:
-                        filtered_when_any_fields = list(set(update_fields) & set(when_any_field))  # Intersected.
-
-                    if not any(
-                        [
-                            self._check_callback_conditions(field_name, callback_specs)
-                            for field_name in filtered_when_any_fields
-                        ]
-                    ):
+                    any_condition_matched = False
+
+                    for field_name in when_any_field:
+                        if self._check_callback_conditions(
+                            field_name,
+                            callback_specs,
+                            is_synced=(
+                                is_partial_fields_update is False
+                                or field_name in update_fields
+                            ),
+                        ):
+                            any_condition_matched = True
+                            break
+
+                    if not any_condition_matched:
                         continue
 
                 # Save method name before potentially wrapping with `on_commit`
@@ -252,8 +260,13 @@ def _run_hooked_methods(self, hook: str, **kwargs) -> List[str]:
 
         return fired
 
-    def _check_callback_conditions(self, field_name: str, specs: dict) -> bool:
-        if not self._check_has_changed(field_name, specs):
+    def _check_callback_conditions(
+        self, field_name: str, specs: dict, is_synced: bool
+    ) -> bool:
+        if not self._check_has_changed(field_name, specs, is_synced):
+            return False
+
+        if not self._check_changes_to_condition(field_name, specs, is_synced):
             return False
 
         if not self._check_is_now_condition(field_name, specs):
@@ -268,12 +281,12 @@ def _check_callback_conditions(self, field_name: str, specs: dict) -> bool:
         if not self._check_is_not_condition(field_name, specs):
             return False
 
-        if not self._check_changes_to_condition(field_name, specs):
-            return False
-
         return True
 
-    def _check_has_changed(self, field_name: str, specs: dict) -> bool:
+    def _check_has_changed(self, field_name: str, specs: dict, is_synced: bool) -> bool:
+        if not is_synced:
+            return False
+
         has_changed = specs["has_changed"]
         return has_changed is None or has_changed == self.has_changed(field_name)
 
@@ -291,7 +304,12 @@ def _check_was_not_condition(self, field_name: str, specs: dict) -> bool:
         was_not = specs["was_not"]
         return was_not is NotSet or self.initial_value(field_name) != was_not
 
-    def _check_changes_to_condition(self, field_name: str, specs: dict) -> bool:
+    def _check_changes_to_condition(
+        self, field_name: str, specs: dict, is_synced: bool
+    ) -> bool:
+        if not is_synced:
+            return False
+
         changes_to = specs["changes_to"]
         return any(
             [

tests/testapp/tests/test_mixin.py

@@ -136,7 +136,7 @@ def test_run_hooked_methods_for_when_any(self):
         instance = UserAccount(first_name="Bob")
 
         instance._potentially_hooked_methods = MagicMock(
-            return_value = [
+            return_value=[
                 MagicMock(
                     __name__="method_that_does_fires",
                     _hooked=[
@@ -208,9 +208,9 @@ def test_has_changed_specs(self):
         UserAccount.objects.create(**data)
         user_account = UserAccount.objects.get()
 
-        self.assertFalse(user_account._check_has_changed("first_name", specs))
+        self.assertFalse(user_account._check_has_changed("first_name", specs, True))
         user_account.first_name = "Ned"
-        self.assertTrue(user_account._check_has_changed("first_name", specs))
+        self.assertTrue(user_account._check_has_changed("first_name", specs, True))
 
     def test_check_is_now_condition_wildcard_should_pass(self):
         specs = {"when": "first_name", "is_now": "*"}
@@ -312,23 +312,31 @@ def test_changes_to_condition_should_pass(self):
         data = self.stub_data
         UserAccount.objects.create(**data)
         user_account = UserAccount.objects.get()
-        with self.assertRaises(CannotRename, msg="Oh, not Flanders. Anybody but Flanders."):
+        with self.assertRaises(
+            CannotRename, msg="Oh, not Flanders. Anybody but Flanders."
+        ):
             user_account.last_name = "Flanders"
             user_account.save()
 
     def test_changes_to_condition_included_in_update_fields_should_fire_hook(self):
         user_account = UserAccount.objects.create(**self.stub_data)
         user_account.first_name = "Flanders"
         user_account.last_name = "Flanders"
-        with self.assertRaises(CannotRename, msg="Oh, not Flanders. Anybody but Flanders."):
+        with self.assertRaises(
+            CannotRename, msg="Oh, not Flanders. Anybody but Flanders."
+        ):
             user_account.last_name = "Flanders"
             user_account.save(update_fields=["last_name"])
 
-    def test_changes_to_condition_not_included_in_update_fields_should_not_fire_hook(self):
+    def test_changes_to_condition_not_included_in_update_fields_should_not_fire_hook(
+        self,
+    ):
         user_account = UserAccount.objects.create(**self.stub_data)
         user_account.first_name = "Flanders"
         user_account.last_name = "Flanders"
-        user_account.save(update_fields=["first_name"])  # `CannotRename` exception is not raised
+        user_account.save(
+            update_fields=["first_name"]
+        )  # `CannotRename` exception is not raised
 
         user_account.refresh_from_db()
         self.assertEqual(user_account.first_name, "Flanders")
@@ -345,8 +353,8 @@ def test_changes_to_condition_should_not_pass(self):
 
     def test_should_not_call_cached_property(self):
         """
-            full_name is cached_property. Accessing _potentially_hooked_methods
-            should not call it incidentally.
+        full_name is cached_property. Accessing _potentially_hooked_methods
+        should not call it incidentally.
         """
         data = self.stub_data
         data["first_name"] = "Bart"