From 191c1250df1d2e5e5e00a13615f4e1bf6e86f5de Mon Sep 17 00:00:00 2001 From: zzy Date: Sat, 11 Apr 2015 13:28:52 +0800 Subject: [PATCH] Add option "NOTIFICATIONS_SOFT_DELETE", fix issue #52 --- .gitignore | 1 + README.rst | 42 +++++++++++++++- notifications/models.py | 71 ++++++++++++++++++++++++++-- notifications/tests/tests.py | 92 +++++++++++++++++++++++++++++++----- notifications/views.py | 25 ++++++---- 5 files changed, 205 insertions(+), 26 deletions(-) diff --git a/.gitignore b/.gitignore index a3c2b9a..e4cf895 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ dist .DS_Store MANIFEST .coverage +htmlcov diff --git a/README.rst b/README.rst index fa0951e..51f82a0 100644 --- a/README.rst +++ b/README.rst @@ -117,7 +117,20 @@ You can attach arbitrary data to your notifications by doing the following: * Add to your settings.py: ``NOTIFICATIONS_USE_JSONFIELD=True`` -Then, any extra arguments you pass to ``notify.send(...)`` will be attached to the ``.data`` attribute of the notification object. These will be serialised using the JSONField's serialiser, so you may need to take that into account: using only objects that will be serialised is a good idea. +Then, any extra arguments you pass to ``notify.send(...)`` will be attached to the ``.data`` attribute of the notification object. +These will be serialised using the JSONField's serialiser, so you may need to take that into account: using only objects that will be serialised is a good idea. + +Soft delete +----------- + +By default, ``delete/(?P\d+)/`` deletes specified notification record from DB. +You can change this behaviour to "mark ``Notification.deleted`` field as ``True``" by: + + * Add to your settings.py: ``NOTIFICATIONS_SOFT_DELETE=True`` + +With this option, QuerySet methods ``unread`` and ``read`` contain one more filter: ``deleted=False``. +Meanwhile, QuerySet methods ``deleted``, ``active``, ``mark_all_as_deleted``, ``mark_all_as_active`` are turned on. +See more details in QuerySet methods section. API ==== @@ -140,11 +153,13 @@ There are some other QuerySet methods, too. ~~~~~~~~~~~~~~~ Return all of the unread notifications, filtering the current queryset. +When ``NOTIFICATIONS_SOFT_DELETE=True``, this filter contains ``deleted=False``. ``qs.read()`` ~~~~~~~~~~~~~~~ Return all of the read notifications, filtering the current queryset. +When ``NOTIFICATIONS_SOFT_DELETE=True``, this filter contains ``deleted=False``. ``qs.mark_all_as_read()`` | ``qs.mark_all_as_read(recipient)`` @@ -158,6 +173,30 @@ Mark all of the unread notifications in the queryset (optionally also filtered b Mark all of the read notifications in the queryset (optionally also filtered by ``recipient``) as unread. +``qs.deleted()`` +~~~~~~~~~~~~~~~~ + +Return all notifications that have ``deleted=True``, filtering the current queryset. +Must be used with ``NOTIFICATIONS_SOFT_DELETE=True``. + +``qs.active()`` +~~~~~~~~~~~~~~~ + +Return all notifications that have ``deleted=False``, filtering the current queryset. +Must be used with ``NOTIFICATIONS_SOFT_DELETE=True``. + +``qs.mark_all_as_deleted()`` | ``qs.mark_all_as_deleted(recipient)`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Mark all notifications in the queryset (optionally also filtered by ``recipient``) as ``deleted=True``. +Must be used with ``NOTIFICATIONS_SOFT_DELETE=True``. + +``qs.mark_all_as_active()`` | ``qs.mark_all_as_active(recipient)`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Mark all notifications in the queryset (optionally also filtered by ``recipient``) as ``deleted=False``. +Must be used with ``NOTIFICATIONS_SOFT_DELETE=True``. + Model methods ------------- @@ -203,4 +242,3 @@ Storing the count in a variable for further processing is advised, such as:: :alt: Code coverage on coveralls :scale: 100% :target: https://coveralls.io/r/django-notifications/django-notifications?branch=master - diff --git a/notifications/models.py b/notifications/models.py index 7506809..94ca0b4 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -3,6 +3,7 @@ from django.conf import settings from django.contrib.contenttypes.models import ContentType from django.contrib.contenttypes import generic from django.db import models +from django.core.exceptions import ImproperlyConfigured from six import text_type from .utils import id2slug @@ -11,6 +12,7 @@ from .signals import notify from model_utils import managers, Choices from jsonfield.fields import JSONField + def now(): # Needs to be be a function as USE_TZ can change based on if we are testing or not. _now = datetime.datetime.now @@ -22,15 +24,43 @@ def now(): pass return _now() + +#SOFT_DELETE = getattr(settings, 'NOTIFICATIONS_SOFT_DELETE', False) +def is_soft_delete(): + #TODO: SOFT_DELETE = getattr(settings, ...) doesn't work with "override_settings" decorator in unittest + # But is_soft_delete is neither a very elegant way. Should try to find better approach + return getattr(settings, 'NOTIFICATIONS_SOFT_DELETE', False) + + +def assert_soft_delete(): + if not is_soft_delete(): + msg = """To use 'deleted' field, please set 'NOTIFICATIONS_SOFT_DELETE'=True in settings. + Otherwise NotificationQuerySet.unread and NotificationQuerySet.read do NOT filter by 'deleted' field. + """ + raise ImproperlyConfigured(msg) + + class NotificationQuerySet(models.query.QuerySet): def unread(self): - "Return only unread items in the current queryset" - return self.filter(unread=True) + """Return only unread items in the current queryset""" + if is_soft_delete(): + return self.filter(unread=True, deleted=False) + else: + """ when SOFT_DELETE=False, developers are supposed NOT to touch 'deleted' field. + In this case, to improve query performance, don't filter by 'deleted' field + """ + return self.filter(unread=True) def read(self): - "Return only read items in the current queryset" - return self.filter(unread=False) + """Return only read items in the current queryset""" + if is_soft_delete(): + return self.filter(unread=False, deleted=False) + else: + """ when SOFT_DELETE=False, developers are supposed NOT to touch 'deleted' field. + In this case, to improve query performance, don't filter by 'deleted' field + """ + return self.filter(unread=False) def mark_all_as_read(self, recipient=None): """Mark as read any unread messages in the current queryset. @@ -57,6 +87,39 @@ class NotificationQuerySet(models.query.QuerySet): qs.update(unread=True) + def deleted(self): + """Return only deleted items in the current queryset""" + assert_soft_delete() + return self.filter(deleted=True) + + def active(self): + """Return only active(un-deleted) items in the current queryset""" + assert_soft_delete() + return self.filter(deleted=False) + + def mark_all_as_deleted(self, recipient=None): + """Mark current queryset as deleted. + Optionally, filter by recipient first. + """ + assert_soft_delete() + qs = self.active() + if recipient: + qs = qs.filter(recipient=recipient) + + qs.update(deleted=True) + + def mark_all_as_active(self, recipient=None): + """Mark current queryset as active(un-deleted). + Optionally, filter by recipient first. + """ + assert_soft_delete() + qs = self.deleted() + if recipient: + qs = qs.filter(recipient=recipient) + + qs.update(deleted=False) + + class Notification(models.Model): """ Action model describing the actor acting out a verb (on an optional diff --git a/notifications/tests/tests.py b/notifications/tests/tests.py index 4ac8275..53e44c5 100644 --- a/notifications/tests/tests.py +++ b/notifications/tests/tests.py @@ -13,6 +13,7 @@ except ImportError: from django.conf import settings from django.contrib.auth.models import User +from django.core.exceptions import ImproperlyConfigured from django.core.urlresolvers import reverse from django.utils.timezone import utc, localtime from django.utils import timezone @@ -22,6 +23,7 @@ from notifications import notify from notifications.models import Notification from notifications.utils import id2slug + class NotificationTest(TestCase): @override_settings(USE_TZ=True) @@ -46,24 +48,26 @@ class NotificationTest(TestCase): delta = timezone.now() - notification.timestamp self.assertTrue(delta.seconds < 60) -class NotificationManagersTest(TestCase): - def setUp(self): +class NotificationManagersTest(TestCase): + + def setUp(self): + self.message_count = 10 self.from_user = User.objects.create(username="from2", password="pwd", email="example@example.com") self.to_user = User.objects.create(username="to2", password="pwd", email="example@example.com") - for i in range(10): + for i in range(self.message_count): notify.send(self.from_user, recipient=self.to_user, verb='commented', action_object=self.from_user) def test_unread_manager(self): - self.assertEqual(Notification.objects.unread().count(),10) + self.assertEqual(Notification.objects.unread().count(), self.message_count) n = Notification.objects.filter(recipient=self.to_user).first() n.mark_as_read() - self.assertEqual(Notification.objects.unread().count(),9) + self.assertEqual(Notification.objects.unread().count(), self.message_count-1) for n in Notification.objects.unread(): self.assertTrue(n.unread) def test_read_manager(self): - self.assertEqual(Notification.objects.unread().count(),10) + self.assertEqual(Notification.objects.unread().count(), self.message_count) n = Notification.objects.filter(recipient=self.to_user).first() n.mark_as_read() self.assertEqual(Notification.objects.read().count(),1) @@ -71,25 +75,54 @@ class NotificationManagersTest(TestCase): self.assertFalse(n.unread) def test_mark_all_as_read_manager(self): - self.assertEqual(Notification.objects.unread().count(),10) + self.assertEqual(Notification.objects.unread().count(), self.message_count) Notification.objects.filter(recipient=self.to_user).mark_all_as_read() self.assertEqual(Notification.objects.unread().count(),0) def test_mark_all_as_unread_manager(self): - self.assertEqual(Notification.objects.unread().count(),10) + self.assertEqual(Notification.objects.unread().count(), self.message_count) Notification.objects.filter(recipient=self.to_user).mark_all_as_read() self.assertEqual(Notification.objects.unread().count(),0) Notification.objects.filter(recipient=self.to_user).mark_all_as_unread() - self.assertEqual(Notification.objects.unread().count(),10) + self.assertEqual(Notification.objects.unread().count(), self.message_count) + + def test_mark_all_deleted_manager_without_soft_delete(self): + self.assertRaises(ImproperlyConfigured, Notification.objects.active) + self.assertRaises(ImproperlyConfigured, Notification.objects.active) + self.assertRaises(ImproperlyConfigured, Notification.objects.mark_all_as_deleted) + self.assertRaises(ImproperlyConfigured, Notification.objects.mark_all_as_active) + + @override_settings(NOTIFICATIONS_SOFT_DELETE=True) + def test_mark_all_deleted_manager(self): + n = Notification.objects.filter(recipient=self.to_user).first() + n.mark_as_read() + self.assertEqual(Notification.objects.read().count(), 1) + self.assertEqual(Notification.objects.unread().count(), self.message_count-1) + self.assertEqual(Notification.objects.active().count(), self.message_count) + self.assertEqual(Notification.objects.deleted().count(), 0) + + Notification.objects.mark_all_as_deleted() + self.assertEqual(Notification.objects.read().count(), 0) + self.assertEqual(Notification.objects.unread().count(), 0) + self.assertEqual(Notification.objects.active().count(), 0) + self.assertEqual(Notification.objects.deleted().count(), self.message_count) + + Notification.objects.mark_all_as_active() + self.assertEqual(Notification.objects.read().count(), 1) + self.assertEqual(Notification.objects.unread().count(), self.message_count-1) + self.assertEqual(Notification.objects.active().count(), self.message_count) + self.assertEqual(Notification.objects.deleted().count(), 0) class NotificationTestPages(TestCase): + def setUp(self): + self.message_count = 10 self.from_user = User.objects.create_user(username="from", password="pwd", email="example@example.com") self.to_user = User.objects.create_user(username="to", password="pwd", email="example@example.com") self.to_user.is_staff = True self.to_user.save() - for i in range(10): + for i in range(self.message_count): notify.send(self.from_user, recipient=self.to_user, verb='commented', action_object=self.from_user) def logout(self): @@ -113,7 +146,7 @@ class NotificationTestPages(TestCase): response = self.client.get(reverse('notifications:unread')) self.assertEqual(response.status_code,200) self.assertEqual(len(response.context['notifications']),len(self.to_user.notifications.unread())) - self.assertEqual(len(response.context['notifications']),10) + self.assertEqual(len(response.context['notifications']), self.message_count) for i,n in enumerate(self.to_user.notifications.all()): if i%3 == 0: @@ -123,7 +156,7 @@ class NotificationTestPages(TestCase): response = self.client.get(reverse('notifications:unread')) self.assertEqual(response.status_code,200) self.assertEqual(len(response.context['notifications']),len(self.to_user.notifications.unread())) - self.assertTrue(len(response.context['notifications'])<10) + self.assertTrue(len(response.context['notifications']) < self.message_count) response = self.client.get(reverse('notifications:mark_all_as_read')) self.assertRedirects(response,reverse('notifications:all')) @@ -143,3 +176,38 @@ class NotificationTestPages(TestCase): slug = id2slug(self.to_user.notifications.first().id) response = self.client.get(reverse('notifications:mark_as_unread',args=[slug])+"?next="+reverse('notifications:unread')) self.assertRedirects(response,reverse('notifications:unread')) + + def test_delete_messages_pages(self): + self.login('to', 'pwd') + + slug = id2slug(self.to_user.notifications.first().id) + response = self.client.get(reverse('notifications:delete', args=[slug])) + self.assertRedirects(response, reverse('notifications:all')) + + response = self.client.get(reverse('notifications:all')) + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.context['notifications']), len(self.to_user.notifications.all())) + self.assertEqual(len(response.context['notifications']), self.message_count-1) + + response = self.client.get(reverse('notifications:unread')) + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.context['notifications']), len(self.to_user.notifications.unread())) + self.assertEqual(len(response.context['notifications']), self.message_count-1) + + @override_settings(NOTIFICATIONS_SOFT_DELETE=True) + def test_soft_delete_messages_manager(self): + self.login('to', 'pwd') + + slug = id2slug(self.to_user.notifications.first().id) + response = self.client.get(reverse('notifications:delete', args=[slug])) + self.assertRedirects(response, reverse('notifications:all')) + + response = self.client.get(reverse('notifications:all')) + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.context['notifications']), len(self.to_user.notifications.active())) + self.assertEqual(len(response.context['notifications']), self.message_count-1) + + response = self.client.get(reverse('notifications:unread')) + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.context['notifications']), len(self.to_user.notifications.unread())) + self.assertEqual(len(response.context['notifications']), self.message_count-1) diff --git a/notifications/views.py b/notifications/views.py index 2807b67..088fa53 100644 --- a/notifications/views.py +++ b/notifications/views.py @@ -3,6 +3,7 @@ from django.contrib.auth.decorators import login_required from django.core.paginator import Paginator, EmptyPage, PageNotAnInteger from django.shortcuts import get_object_or_404, render, redirect from django.template.context import RequestContext +from django.conf import settings from .utils import slug2id from .models import Notification @@ -11,8 +12,12 @@ def all(request): """ Index page for authenticated user """ + if getattr(settings, 'NOTIFICATIONS_SOFT_DELETE', False): + qs = request.user.notifications.active() + else: + qs = request.user.notifications.all() return render(request, 'notifications/list.html', { - 'notifications': request.user.notifications.all() + 'notifications': qs }) actions = request.user.notifications.all() @@ -77,17 +82,21 @@ def mark_as_unread(request, slug=None): return redirect('notifications:all') + @login_required def delete(request, slug=None): - id = slug2id(slug) + _id = slug2id(slug) - notification = get_object_or_404(Notification, recipient=request.user, id=id) - notification.deleted = True - notification.save() + notification = get_object_or_404(Notification, recipient=request.user, id=_id) + if getattr(settings, 'NOTIFICATIONS_SOFT_DELETE', False): + notification.deleted = True + notification.save() + else: + notification.delete() - next = request.GET.get('next') + _next = request.GET.get('next') - if next: - return redirect(next) + if _next: + return redirect(_next) return redirect('notifications:all')