Merge pull request #98 from amcclosky/add-on-commit-option

Add optional `@hook` `on_commit` argument to for executing hooks `on_commit`

Author: rsinger86

django_lifecycle/decorators.py

@@ -1,5 +1,5 @@
 from functools import wraps
-from typing import List
+from typing import List, Optional
 
 from django_lifecycle import NotSet
 
@@ -10,7 +10,7 @@ class DjangoLifeCycleException(Exception):
     pass
 
 
-def _validate_hook_params(hook, when, when_any, has_changed):
+def _validate_hook_params(hook, when, when_any, has_changed, on_commit):
     if hook not in VALID_HOOKS:
         raise DjangoLifeCycleException(
             "%s is not a valid hook; must be one of %s" % (hook, VALID_HOOKS)
@@ -46,6 +46,13 @@ def _validate_hook_params(hook, when, when_any, has_changed):
         raise DjangoLifeCycleException(
             "Can pass either 'when' or 'when_any' but not both"
         )
+    
+    if on_commit is not None:
+        if not hook.startswith("after_"):
+            raise DjangoLifeCycleException("'on_commit' hook param is only valid with AFTER_* hooks")
+
+        if not isinstance(on_commit, bool):
+            raise DjangoLifeCycleException("'on_commit' hook param must be a boolean")
 
 
 def hook(
@@ -58,8 +65,9 @@ def hook(
     is_not=NotSet,
     was_not=NotSet,
     changes_to=NotSet,
+    on_commit: Optional[bool] = None
 ):
-    _validate_hook_params(hook, when, when_any, has_changed)
+    _validate_hook_params(hook, when, when_any, has_changed, on_commit)
 
     def decorator(hooked_method):
         if not hasattr(hooked_method, "_hooked"):
@@ -83,6 +91,7 @@ def func(*args, **kwargs):
                 "was": was,
                 "was_not": was_not,
                 "changes_to": changes_to,
+                "on_commit": on_commit
             }
         )
 

django_lifecycle/mixins.py

@@ -1,4 +1,4 @@
-from functools import reduce, lru_cache
+from functools import partial, reduce, lru_cache
 from inspect import isfunction
 from typing import Any, List
 
@@ -200,6 +200,8 @@ def _run_hooked_methods(self, hook: str, **kwargs) -> List[str]:
             for callback_specs in method._hooked:
                 if callback_specs["hook"] != hook:
                     continue
+                
+                on_commit = callback_specs.get("on_commit", False)
 
                 when_field = callback_specs.get("when")
                 when_any_field = callback_specs.get("when_any")
@@ -225,10 +227,27 @@ def _run_hooked_methods(self, hook: str, **kwargs) -> List[str]:
                         ]
                     ):
                         continue
+                
+                # Save method name before potentially wrapping with `on_commit`
+                method_name = method.__name__
+    
+                # Apply `on_commit` after saving the method as `fired` to preserve
+                # the non-anonymous name
+                if on_commit:
+                    # Append `_on_commit` to the existing method name to allow for firing
+                    # the same hook within the atomic transaction and on_commit
+                    method_name = method_name + "_on_commit"
+
+                    # Use partial to create a function closure that binds `self`
+                    # to ensure its available to execute later.
+                    _on_commit_func = partial(method, self)
+                    _on_commit_func.__name__ = method_name
+                    transaction.on_commit(_on_commit_func)
+                else:
+                    method(self)
 
                 # Only call the method once per hook
-                fired.append(method.__name__)
-                method(self)
+                fired.append(method_name)
                 break
 
         return fired

docs/examples.md

@@ -28,6 +28,14 @@ Or you want to email a user when their account is deleted. You could add the dec
         )
 ```
 
+Or if you want to enqueue a background job that depends on state being committed to your database
+
+```python
+    @hook(AFTER_CREATE, on_commit=True)
+    def do_after_create_jobs(self):
+        enqueue_job(send_item_shipped_notication, self.item_id)
+```
+
 Read on to see how to only fire the hooked method if certain conditions about the model's current and previous state are met.
 
 ## Transitions between specific values

docs/hooks_and_conditions.md

@@ -17,6 +17,7 @@ You can hook into one or more lifecycle moments by adding the `@hook` decorator
         was: Any = '*', 
         was_not: Any = None,
         changes_to: Any = None,
+        on_commit: Optional[bool] = None
     ):
 ```
 ## Lifecycle Moments
@@ -52,3 +53,4 @@ If you do not use any conditional parameters, the hook will fire every time the
 | was | Any | Only fire the hooked method if the value of the `when` field was equal to this value when first initialized; defaults to `*`.  |
 | was_not | Any | Only fire the hooked method if the value of the `when` field was NOT equal to this value when first initialized. |
 | changes_to | Any | Only fire the hooked method if the value of the `when` field was NOT equal to this value when first initialized but is currently equal to this value. |
+| on_commit | bool | When `True` only fire the hooked method after the current database transaction has been commited or not at all. (Only applies to `AFTER_*` hooks)  |

requirements.txt

@@ -1,6 +1,7 @@
 asgiref==3.4.1
 Click==7.0
 Django==3.2.8
+django-capture-on-commit-callbacks==1.10.0
 djangorestframework==3.11.2
 ghp-import==2.0.2
 importlib-metadata==4.8.1

tests/testapp/models.py

@@ -51,7 +51,7 @@ def lowercase_email(self):
     def timestamp_joined_at(self):
         self.joined_at = timezone.now()
 
-    @hook("after_create")
+    @hook("after_create", on_commit=True)
     def do_after_create_jobs(self):
         # queue background job to process thumbnail image...
         mail.send_mail(
@@ -75,7 +75,7 @@ def ensure_trial_not_active(self):
     def ensure_last_name_is_not_changed_to_flanders(self):
         raise CannotRename("Oh, not Flanders. Anybody but Flanders.")
 
-    @hook("after_update", when="organization.name", has_changed=True)
+    @hook("after_update", when="organization.name", has_changed=True, on_commit=True)
     def notify_org_name_change(self):
         mail.send_mail(
             "The name of your organization has changed!",

tests/testapp/tests/test_mixin.py

@@ -366,3 +366,85 @@ def test_comparison_state_should_reset_after_save(self):
         self.assertTrue(account.has_changed("first_name"))
         account.save()
         self.assertFalse(account.has_changed("first_name"))
+
+    def test_run_hooked_methods_for_on_commit(self):
+        instance = UserAccount(first_name="Bob")
+
+        instance._potentially_hooked_methods = MagicMock(
+            return_value = [
+                MagicMock(
+                    __name__="method_that_fires_on_commit",
+                    _hooked=[
+                        {
+                            "hook": "after_create",
+                            "when": None,
+                            "when_any": None,
+                            "has_changed": None,
+                            "is_now": "*",
+                            "is_not": NotSet,
+                            "was": "*",
+                            "was_not": NotSet,
+                            "changes_to": NotSet,
+                            "on_commit": True
+                        }
+                    ],
+                ),
+                MagicMock(
+                    __name__="method_that_fires_in_transaction",
+                    _hooked=[
+                        {
+                            "hook": "after_create",
+                            "when": None,
+                            "when_any": None,
+                            "has_changed": None,
+                            "is_now": "*",
+                            "is_not": NotSet,
+                            "was": "*",
+                            "was_not": NotSet,
+                            "changes_to": NotSet,
+                            "on_commit": False
+                        }
+                    ],
+                ),
+                MagicMock(
+                    __name__="method_that_fires_in_default",
+                    _hooked=[
+                        {
+                            "hook": "after_create",
+                            "when": None,
+                            "when_any": None,
+                            "has_changed": None,
+                            "is_now": "*",
+                            "is_not": NotSet,
+                            "was": "*",
+                            "was_not": NotSet,
+                            "changes_to": NotSet,
+                            "on_commit": None
+                        }
+                    ],
+                ),
+                MagicMock(
+                    __name__="after_save_method_that_fires_on_commit",
+                    _hooked=[
+                        {
+                            "hook": "after_save",
+                            "when": None,
+                            "when_any": None,
+                            "has_changed": None,
+                            "is_now": "*",
+                            "is_not": NotSet,
+                            "was": "*",
+                            "was_not": NotSet,
+                            "changes_to": NotSet,
+                            "on_commit": True
+                        }
+                    ],
+                ),
+            ]
+        )
+
+        fired_methods = instance._run_hooked_methods("after_create")
+        self.assertEqual(fired_methods, ["method_that_fires_on_commit_on_commit", "method_that_fires_in_transaction", "method_that_fires_in_default"])
+
+        fired_methods = instance._run_hooked_methods("after_save")
+        self.assertEqual(fired_methods, ["after_save_method_that_fires_on_commit_on_commit"])

tests/testapp/tests/test_user_account.py

@@ -3,6 +3,8 @@
 from django.core import mail
 from django.test import TestCase
 
+from django_capture_on_commit_callbacks import capture_on_commit_callbacks
+
 from tests.testapp.models import CannotDeleteActiveTrial, Organization, UserAccount
 
 
@@ -22,7 +24,10 @@ def test_update_joined_at_before_create(self):
         self.assertTrue(isinstance(account.joined_at, datetime.datetime))
 
     def test_send_welcome_email_after_create(self):
-        UserAccount.objects.create(**self.stub_data)
+        with capture_on_commit_callbacks(execute=True) as callbacks:
+            UserAccount.objects.create(**self.stub_data)
+        
+        self.assertEquals(len(callbacks), 1, msg=f"{callbacks}")
         self.assertEqual(len(mail.outbox), 1)
         self.assertEqual(mail.outbox[0].subject, "Welcome!")
 
@@ -73,12 +78,16 @@ def test_notify_org_name_change(self):
         org = Organization.objects.create(name="Hogwarts")
         UserAccount.objects.create(**self.stub_data, organization=org)
         mail.outbox = []
+
         account = UserAccount.objects.get()
 
-        org.name = "Coursera Wizardry"
-        org.save()
+        with capture_on_commit_callbacks(execute=True) as callbacks:
+            org.name = "Coursera Wizardry"
+            org.save()
 
-        account.save()
+            account.save()
+        
+        self.assertEquals(len(callbacks), 1)
         self.assertEqual(len(mail.outbox), 1)
         self.assertEqual(
             mail.outbox[0].subject, "The name of your organization has changed!"
@@ -95,18 +104,23 @@ def test_no_notify_sent_if_org_name_has_not_changed(self):
     def test_additional_notify_sent_for_specific_org_name_change(self):
         org = Organization.objects.create(name="Hogwarts")
         UserAccount.objects.create(**self.stub_data, organization=org)
+
         mail.outbox = []
-        account = UserAccount.objects.get()
 
-        org.name = "Hogwarts Online"
-        org.save()
+        with capture_on_commit_callbacks(execute=True) as callbacks:
+            account = UserAccount.objects.get()
 
-        account.save()
+            org.name = "Hogwarts Online"
+            org.save()
+
+            account.save()
+
+        self.assertEquals(len(callbacks), 1, msg="Only one hook should be an on_commit callback")
         self.assertEqual(len(mail.outbox), 2)
         self.assertEqual(
-            mail.outbox[0].subject, "The name of your organization has changed!"
+            mail.outbox[1].subject, "The name of your organization has changed!"
         )
-        self.assertEqual(mail.outbox[1].subject, "You were moved to our online school!")
+        self.assertEqual(mail.outbox[0].subject, "You were moved to our online school!")
 
     def test_email_user_about_name_change(self):
         account = UserAccount.objects.create(**self.stub_data)