Merge pull request #134 from benjaoming/get_target_object

Change the return value of `notify()` (as already mentioned in documentation)

django_nyt/conf.py

@@ -35,10 +35,6 @@ class AppSettings:
     NYT_DB_TABLE_PREFIX: str = "nyt"
     """The table prefix for tables in the database. Do not change this unless you know what you are doing."""
 
-    NYT_ENABLED: bool = True
-    """Global setting to force-fully disable all propagation and creation of
-    notifications."""
-
     NYT_ENABLE_ADMIN: bool = False
     """Enable django-admin registration for django-nyt's ModelAdmin classes."""
 

django_nyt/decorators.py

@@ -8,17 +8,21 @@
 
 
 def disable_notify(f):
-    """Disable notifications. Example:
+    """Disable notifications.
 
-    @disable_notify
-    def your_function():
-        notify("no one will be notified", ...)
+    Does not work for async stuff, only disables notify in the same process.
+
+    Example::
+
+        @disable_notify
+        def your_function():
+            notify("no one will be notified", ...)
     """
 
     @wraps(f)
-    def wrapper(request, *args, **kwargs):
+    def wrapper(*args, **kwargs):
         django_nyt._disable_notifications = True
-        response = f(request, *args, **kwargs)
+        response = f(*args, **kwargs)
         django_nyt._disable_notifications = False
         return response
 

django_nyt/models.py

@@ -335,7 +335,15 @@ def save(self, *args, **kwargs):
         super(Notification, self).save(*args, **kwargs)
 
     @classmethod
-    def create_notifications(cls, key, **kwargs):
+    def create_notifications(
+        cls,
+        key,
+        object_id=None,
+        content_type=None,
+        filter_exclude=None,
+        recipient_users=None,
+        **kwargs
+    ):
         """
         Creates notifications directly in database -- do not call directly,
         use django_nyt.notify(...)
@@ -346,14 +354,15 @@ def create_notifications(cls, key, **kwargs):
         if not key or not isinstance(key, str):
             raise KeyError("No notification key (string) specified.")
 
-        object_id = kwargs.pop("object_id", None)
-        filter_exclude = kwargs.pop("filter_exclude", None) or {}
-        recipient_users = kwargs.pop("recipient_users", None)
+        if filter_exclude is None:
+            filter_exclude = {}
 
         objects_created = []
         subscriptions = Subscription.objects.filter(notification_type__key=key).exclude(
             **filter_exclude
         )
+
+        # TODO: This query should include notification_type__content_type? Why was this not added?
         if object_id:
             subscriptions = subscriptions.filter(
                 Q(object_id=object_id) | Q(object_id=None)

django_nyt/utils.py

@@ -1,11 +1,12 @@
 from typing import Any
+from typing import List
 from typing import Union
 
 from django.contrib.contenttypes.models import ContentType
 from django.db.models import Model
 from django.utils.translation import gettext as _
 
-from . import _disable_notifications
+import django_nyt
 from . import models
 from .conf import app_settings
 
@@ -17,7 +18,7 @@ def notify(
     url: str = None,
     filter_exclude: dict = None,
     recipient_users: list = None,
-) -> int:
+) -> List[models.Notification]:
     """
     Notify subscribing users of a new event. Key can be any kind of string,
     just make sure to reuse it where applicable.
@@ -51,14 +52,15 @@ def notify(
                             instead of notifying all subscribers of the event.
                             Notice that users still have to be actually subscribed
                             to the event key!
-    :param target_object: Any django model instance that this notification
-                          relates to. Use django content types.
+    :param target_object: Any Django model instance that this notification
+                          relates to. Uses Django content types.
+                          Subscriptions with a matching content_type and object_id will be notified.
     :param filter_exclude: Keyword arguments passed to filter out Subscriptions.
                            Will be handed to ``Subscription.objects.exclude(**filter_exclude)``.
     """
 
-    if _disable_notifications:
-        return 0
+    if django_nyt._disable_notifications:
+        return []
 
     if target_object:
         if not isinstance(target_object, Model):
@@ -71,7 +73,7 @@ def notify(
     else:
         object_id = None
 
-    objects = models.Notification.create_notifications(
+    notifications = models.Notification.create_notifications(
         key,
         object_id=object_id,
         message=message,
@@ -84,9 +86,9 @@ def notify(
     if app_settings.NYT_ENABLE_CHANNELS:
         from django_nyt import subscribers
 
-        subscribers.notify_subscribers(objects, key)
+        subscribers.notify_subscribers(notifications, key)
 
-    return len(objects)
+    return notifications
 
 
 def subscribe(

docs/howto/index.rst

@@ -29,6 +29,18 @@ users can also be subscribed to specific object (i.e. "*your* comment was update
 
    notify(_("OMG! Something happened"), EVENT_KEY, target_object=my_model_instance)
 
+In order to subscribe a user to receive notifications only for a specific object,
+you can create the subscription like this:
+
+.. code-block:: python
+
+    subscribe(
+        user_setting,
+        EVENT_KEY,
+        content_type=ContentType.objects.get_for_model(MyModel),
+        object_id=my_model_instance.id,
+    )
+
 
 Excluding certain recipients
 ----------------------------

docs/release_notes.rst

@@ -13,16 +13,19 @@ Release Notes
   so it's now encouraged to use ``/`` separators in notification keys,
   for instance ``comments/new`` is matched by ``comments/**``. #125 (Benjamin Balder Bach)
 * Django 5 support #128 (Benjamin Balder Bach)
-* New arguments ``--domain`` and ``--http-only`` for management command ``notifymail`` #130 (Benjamin Balder Bach)
+* New arguments ``--domain`` and ``--http-only`` for management command ``notifymail``. #130 (Benjamin Balder Bach)
+* Documentation reorganized with Diataxis structure and more how-to guides have been added. (Benjamin Balder Bach)
 
 **Changed**
 
 * Tests migrated to pytest #123 (Benjamin Balder Bach)
 * Default email notification paths are changed to ``notifications/emails/default.txt`` and ``notifications/emails/default_subject.txt`` #125 (Benjamin Balder Bach)
 * Notification URLs added to emails have a hard-coded `https://` (before, this was `http://`) #125 (Benjamin Balder Bach)
-* New test-friendly settings pattern changes internal names of settings, but has no effects on Django settings #127 (Benjamin Balder Bach).
+* New test-friendly settings pattern changes internal names of settings, but has no effects on Django settings #127 (Benjamin Balder Bach)
 * Corrected name of method to ``Settings.get_default_settings`` #129 (Benjamin Balder Bach)
-* Improvements to docstrings of main methods ``notify()`` and ``subscribe()``. #129 (Benjamin Balder Bach)
+* Improvements to docstrings of main methods ``notify()`` and ``subscribe()`` #129 (Benjamin Balder Bach)
+* Return value of ``notify()`` was changed - it no longer returns an `int` (number of created notifications), instead it returns a list of created notifications.
+  This is very useful, see :doc:`howto/object_relations` #134 (Benjamin Balder Bach)
 
 **Fixed**
 
@@ -33,6 +36,7 @@ Release Notes
 **Removed**
 
 * Python 3.7 support removed #129 (Benjamin Balder Bach)
+* Unused (!) setting ``NYT_ENABLED`` was removed #134 (Benjamin Balder Bach)
 
 1.3
 ---

tests/core/test_basic.py

@@ -1,8 +1,11 @@
 from django.contrib.auth import get_user_model
+from django.contrib.contenttypes.models import ContentType
 from django.test import TestCase
 
 from django_nyt import models
 from django_nyt import utils
+from django_nyt.decorators import disable_notify
+from tests.testapp.models import TestModel
 
 User = get_user_model()
 
@@ -38,6 +41,19 @@ def test_notify(self):
         # Check that there is exactly 1 notification
         self.assertEqual(models.Notification.objects.all().count(), 1)
 
+    def test_disable_notify(self):
+        # Subscribe User 1 to test key
+        utils.subscribe(self.user1_settings, self.TEST_KEY)
+
+        @disable_notify
+        def inner():
+            utils.notify("Test is a test", self.TEST_KEY)
+
+        inner()
+
+        # Check that there is exactly 1 notification
+        self.assertEqual(models.Notification.objects.all().count(), 0)
+
     def test_notify_two_users(self):
 
         # Subscribe User 2 to test key
@@ -52,3 +68,31 @@ def test_notify_two_users(self):
         utils.notify("Another test", self.TEST_KEY)
 
         self.assertEqual(models.Notification.objects.filter(occurrences=2).count(), 2)
+
+    def test_failure_target_object_not_model(self):
+
+        # Subscribe User 1 to test key
+        utils.subscribe(self.user1_settings, self.TEST_KEY)
+        with self.assertRaises(TypeError):
+            utils.notify("Another test", self.TEST_KEY, target_object=object())
+
+    def test_with_target_object(self):
+
+        related_object = TestModel.objects.create(name="test_with_target_object")
+        content_type = ContentType.objects.get_for_model(TestModel)
+        # Subscribe User 1 to test key
+        utils.subscribe(
+            self.user1_settings,
+            self.TEST_KEY,
+            content_type=content_type,
+            object_id=related_object.id,
+        )
+        utils.notify("Test related object", self.TEST_KEY, target_object=related_object)
+
+        self.assertEqual(
+            models.Notification.objects.filter(
+                subscription__object_id=related_object.id,
+                subscription__notification_type__content_type=content_type,
+            ).count(),
+            1,
+        )