From 737e8f35d504c2e974687eb23d2a3fbcd3c4e85e Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Fri, 16 Mar 2018 12:35:28 +0100 Subject: [PATCH 4/4] notifications: improve internal API (#22390) * (user, external_id) is now unique, no need to user .first() / .last() * new filter .find(user, id), .visible(user), new queryset actions .ack() and .forget() * new property notification.visible to know if a notification is shown * when notify() is called on an existing notification, it's unacked * notify's parameters url, body and origin now default to '', serializer has been updated --- combo/apps/notifications/api_views.py | 17 ++- .../migrations/0004_auto_20180316_1026.py | 21 ++++ combo/apps/notifications/models.py | 122 +++++++++++++-------- tests/test_notification.py | 90 ++++++++------- 4 files changed, 155 insertions(+), 95 deletions(-) create mode 100644 combo/apps/notifications/migrations/0004_auto_20180316_1026.py diff --git a/combo/apps/notifications/api_views.py b/combo/apps/notifications/api_views.py index db2aa7f..66e9ff1 100644 --- a/combo/apps/notifications/api_views.py +++ b/combo/apps/notifications/api_views.py @@ -24,9 +24,9 @@ from .models import Notification class NotificationSerializer(serializers.Serializer): summary = serializers.CharField(required=True, allow_blank=False, max_length=140) id = serializers.SlugField(required=False, allow_null=True) - body = serializers.CharField(required=False, allow_blank=False) - url = serializers.URLField(required=False, allow_blank=True) - origin = serializers.CharField(required=False, allow_blank=True) + body = serializers.CharField(allow_blank=False, default='') + url = serializers.URLField(allow_blank=True, default='') + origin = serializers.CharField(allow_blank=True, default='') start_timestamp = serializers.DateTimeField(required=False, allow_null=True) end_timestamp = serializers.DateTimeField(required=False, allow_null=True) duration = serializers.IntegerField(required=False, allow_null=True, min_value=0) @@ -43,7 +43,7 @@ class Add(GenericAPIView): return Response(response, status.HTTP_400_BAD_REQUEST) data = serializer.validated_data - notification_id, created = Notification.notify( + notification, created = Notification.notify( user=request.user, summary=data['summary'], id=data.get('id'), @@ -54,18 +54,17 @@ class Add(GenericAPIView): end_timestamp=data.get('end_timestamp'), duration=data.get('duration') ) - response = {'err': 0, 'data': {'id': notification_id, 'created': created}} + response = {'err': 0, 'data': {'id': notification.public_id, 'created': created}} return Response(response) add = Add.as_view() class Ack(GenericAPIView): - authentication_classes = (authentication.SessionAuthentication,) permission_classes = (permissions.IsAuthenticated,) - def get(self, request, notification_id): - Notification.ack(request.user, notification_id) + def get(self, request, notification_id, *args, **kwargs): + Notification.objects.find(request.user, notification_id).ack() return Response({'err': 0}) ack = Ack.as_view() @@ -75,7 +74,7 @@ class Forget(GenericAPIView): permission_classes = (permissions.IsAuthenticated,) def get(self, request, notification_id, *args, **kwargs): - Notification.forget(request.user, notification_id) + Notification.objects.find(request.user, notification_id).forget() return Response({'err': 0}) forget = Forget.as_view() diff --git a/combo/apps/notifications/migrations/0004_auto_20180316_1026.py b/combo/apps/notifications/migrations/0004_auto_20180316_1026.py new file mode 100644 index 0000000..6f4bf64 --- /dev/null +++ b/combo/apps/notifications/migrations/0004_auto_20180316_1026.py @@ -0,0 +1,21 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.11 on 2018-03-16 10:26 +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('notifications', '0003_notification_origin'), + ] + + operations = [ + migrations.AlterUniqueTogether( + name='notification', + unique_together=set([('user', 'external_id')]), + ), + ] diff --git a/combo/apps/notifications/models.py b/combo/apps/notifications/models.py index b695629..20bbefe 100644 --- a/combo/apps/notifications/models.py +++ b/combo/apps/notifications/models.py @@ -17,26 +17,46 @@ from django.conf import settings from django.db import models from django.utils.translation import ugettext_lazy as _ -from django.utils.timezone import now, localtime, timedelta +from django.utils.timezone import now, timedelta from django.db.models import Q +from django.db.models.query import QuerySet from combo.data.models import CellBase from combo.data.library import register_cell_class -class NotificationManager(models.Manager): - def filter_by_id(self, id): +class NotificationQuerySet(QuerySet): + def find(self, user, id): + qs = self.filter(user=user) try: int(id) except (ValueError, TypeError): search_id = Q(external_id=id) else: search_id = Q(pk=id) | Q(external_id=id) - return self.filter(search_id) + return qs.filter(search_id) + + def ack(self): + self.update(acked=True) + + def visible(self, user): + n = now() + qs = self.filter(user=user, + start_timestamp__lte=n, + end_timestamp__gt=n) + return qs.order_by('-start_timestamp') + + def new(self): + return self.filter(acked=False) + + def forget(self): + past_end_timestamp = now() - timedelta(seconds=5) + self.update( + end_timestamp=past_end_timestamp, acked=True) class Notification(models.Model): - objects = NotificationManager() + objects = NotificationQuerySet.as_manager() user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE) summary = models.CharField(_('Label'), max_length=140) @@ -50,16 +70,20 @@ class Notification(models.Model): class Meta: verbose_name = _('Notification') + unique_together = ( + ('user', 'external_id'), + ) def __unicode__(self): return self.summary + @property def public_id(self): return self.external_id or str(self.pk) @classmethod - def notify(cls, user, summary, id=None, body=None, url=None, origin=None, - start_timestamp=None, duration=None, end_timestamp=None): + def notify(cls, user, summary, id=None, body='', url='', origin='', + start_timestamp=None, duration=None, end_timestamp=None): ''' Create a new notification: Notification.notify(user, 'summary') -> id @@ -68,43 +92,55 @@ class Notification(models.Model): Renew an existing notification, or create a new one, with an external_id: Notification.notify(user, 'summary', id='id') ''' - created = False - # get ... - notification = Notification.objects.filter_by_id(id).filter(user=user).first() if id else None - if not notification: # ... or create - notification = Notification(user=user, summary=summary, - body=body or '', url=url or '', - external_id=id) - created = True - notification.summary = summary - notification.body = body or '' - notification.url = url or '' - notification.origin = origin or '' - notification.start_timestamp = start_timestamp or now() + start_timestamp = start_timestamp or now() + if duration: if not isinstance(duration, timedelta): duration = timedelta(seconds=duration) - notification.end_timestamp = notification.start_timestamp + duration + end_timestamp = start_timestamp + duration else: - notification.end_timestamp = end_timestamp or notification.start_timestamp + timedelta(3) + end_timestamp = end_timestamp or start_timestamp + timedelta(days=3) + + defaults = { + 'summary': summary, + 'body': body, + 'url': url, + 'origin': origin, + 'start_timestamp': start_timestamp, + 'end_timestamp': end_timestamp, + 'acked': False, + } - notification.save() - - if notification.external_id is None: - notification_id = '%s' % notification.pk + try: + pk = int(id) + # id is maybe an implicit id + notification = Notification.objects.get(pk=pk) + Notification.objects.filter(pk=pk).update(**defaults) + return notification, False + except (ValueError, TypeError, Notification.DoesNotExist): + pass + + if id: + notification, created = Notification.objects.update_or_create( + user=user, external_id=unicode(id), + defaults=defaults) else: - notification_id = notification.external_id - return notification_id, created + notification = Notification.objects.create(user=user, **defaults) + created = True + return notification, created - @classmethod - def ack(cls, user, id): - Notification.objects.filter_by_id(id).filter(user=user).update(acked=True) + @property + def visible(self): + return self.end_timestamp > now() - @classmethod - def forget(cls, user, id): - past = now() - timedelta(seconds=5) - Notification.objects.filter_by_id(id).filter(user=user).update(end_timestamp=past, - acked=True) + def forget(self): + self.end_timestamp = now() - timedelta(seconds=5) + self.acked = True + self.save(update_fields=['end_timestamp', 'acked']) + + def ack(self): + self.acked = True + self.save(update_fields=['acked']) @register_cell_class @@ -125,18 +161,16 @@ class NotificationsCell(CellBase): extra_context = super(NotificationsCell, self).get_cell_extra_context(context) user = getattr(context.get('request'), 'user', None) if user and user.is_authenticated(): - extra_context['notifications'] = Notification.objects.filter(user=user, - start_timestamp__lte=now(), end_timestamp__gt=now()).order_by('-start_timestamp') - extra_context['new_notifications'] = extra_context['notifications'].filter(acked=False) + qs = Notification.objects.visible(user) + extra_context['notifications'] = qs + extra_context['new_notifications'] = qs.new() return extra_context def get_badge(self, context): user = getattr(context.get('request'), 'user', None) if not user or not user.is_authenticated(): return - notifs = Notification.objects.filter(user=user, start_timestamp__lte=now(), - end_timestamp__gt=now()) - new = notifs.filter(acked=False).count() - if not new: + new_count = Notification.objects.visible(user).new().count() + if not new_count: return - return {'badge': str(new)} + return {'badge': str(new_count)} diff --git a/tests/test_notification.py b/tests/test_notification.py index 4a31fc2..02f44ad 100644 --- a/tests/test_notification.py +++ b/tests/test_notification.py @@ -16,6 +16,7 @@ pytestmark = pytest.mark.django_db client = Client() + @pytest.fixture def user(): try: @@ -26,6 +27,7 @@ def user(): admin.save() return admin + @pytest.fixture def user2(): try: @@ -34,49 +36,49 @@ def user2(): admin2 = User.objects.create_user('admin2', email=None, password='admin2') return admin2 + def login(username='admin', password='admin'): resp = client.post('/login/', {'username': username, 'password': password}) assert resp.status_code == 302 def test_notification_api(user, user2): - id_notifoo, created = Notification.notify(user, 'notifoo') + notification, created = Notification.notify(user, 'notifoo') assert created - assert Notification.objects.all().count() == 1 - noti = Notification.objects.filter_by_id(id_notifoo).filter(user=user).first() - assert noti.pk == int(id_notifoo) - assert noti.summary == 'notifoo' - assert noti.body == '' - assert noti.url == '' - assert noti.external_id is None - assert noti.end_timestamp - noti.start_timestamp == timedelta(3) - assert noti.acked is False - Notification.ack(user, id_notifoo) - noti = Notification.objects.filter_by_id(id_notifoo).filter(user=user).first() - assert noti.acked is True - - Notification.notify(user, 'notirefoo', id=id_notifoo) - assert Notification.objects.all().count() == 1 - noti = Notification.objects.filter_by_id(id_notifoo).filter(user=user).first() - assert noti.pk == int(id_notifoo) - assert noti.summary == 'notirefoo' - - Notification.notify(user, 'notirefoo', id=id_notifoo, duration=3600) - noti = Notification.objects.filter_by_id(id_notifoo).filter(user=user).first() + assert Notification.objects.count() == 1 + assert notification.summary == 'notifoo' + assert notification.body == '' + assert notification.url == '' + assert notification.origin == '' + assert notification.external_id is None + assert notification.end_timestamp - notification.start_timestamp == timedelta(3) + assert notification.acked is False + Notification.objects.visible(user).ack() + assert Notification.objects.get().acked is True + + Notification.notify(user, 'notirefoo', id=str(notification.pk)) + assert Notification.objects.count() == 1 + assert Notification.objects.get().summary == 'notirefoo' + # we updated the notification, it's un-acked + assert Notification.objects.get().acked is False + + Notification.notify(user, 'notirefoo', id=str(notification.pk), duration=3600) + noti = Notification.objects.get() assert noti.end_timestamp - noti.start_timestamp == timedelta(seconds=3600) notification, created = Notification.notify(user, 'notibar', id='notibar') - assert created - assert Notification.objects.all().count() == 2 + assert Notification.objects.count() == 2 notification, created = Notification.notify(user, 'notirebar', id='notibar') assert not created - assert Notification.objects.all().count() == 2 + assert Notification.objects.count() == 2 + + notification, created = Notification.notify(user2, 'notiother') + notification.forget() + assert Notification.objects.filter(user=user2).count() == 1 + notification = Notification.objects.filter(user=user2).get() + assert notification.end_timestamp < now() + assert notification.acked is True - id2, created = Notification.notify(user2, 'notiother') - Notification.forget(user2, id2) - noti = Notification.objects.filter_by_id(id2).filter(user=user2).first() - assert noti.end_timestamp < now() - assert noti.acked is True def test_notification_cell(user, user2): page = Page(title='notif', slug='test_notification_cell', template_name='standard') @@ -92,36 +94,36 @@ def test_notification_cell(user, user2): assert cell.is_visible(context['request'].user) is True assert cell.get_badge(context) is None - id_noti1, created = Notification.notify(user, 'notibar') - id_noti2, created = Notification.notify(user, 'notifoo') + notification1, created = Notification.notify(user, 'notibar') + notification2, created = Notification.notify(user, 'notifoo') content = cell.render(context) assert 'notibar' in content assert 'notifoo' in content assert cell.get_badge(context) == {'badge': '2'} - Notification.forget(user, id_noti2) + notification2.forget() content = cell.render(context) assert 'notibar' in content assert 'notifoo' not in content assert cell.get_badge(context) == {'badge': '1'} - Notification.notify(user, 'notirebar', id=id_noti1) + Notification.notify(user, 'notirebar', id=str(notification1.pk)) content = cell.render(context) assert 'notirebar' in content assert 'notibar' not in content - Notification.notify(user, 'notiurl', id=id_noti1, url='https://www.example.net/') + Notification.notify(user, 'notiurl', id=str(notification1.pk), url='https://www.example.net/') content = cell.render(context) assert 'notiurl' in content assert 'https://www.example.net/' in content - ackme, created = Notification.notify(user, 'ackme') - Notification.ack(user, id=ackme) + notification3, created = Notification.notify(user, 'ackme') + notification3.ack() content = cell.render(context) assert 'acked' in content assert cell.get_badge(context) == {'badge': '1'} - Notification.ack(user, id=id_noti1) + notification1.ack() content = cell.render(context) assert cell.get_badge(context) is None @@ -136,6 +138,7 @@ def test_notification_cell(user, user2): assert 'notiother' in content assert cell.get_badge(context) == {'badge': '1'} + def test_notification_ws(user): def notify(data, check_id, count): @@ -145,7 +148,7 @@ def test_notification_ws(user): result = json.loads(resp.content) assert result == {'data': {'id': check_id, 'created': True}, 'err': 0} assert Notification.objects.filter(user=user).count() == count - return Notification.objects.filter_by_id(check_id).last() + return Notification.objects.find(user, check_id).get() login() notify({'summary': 'foo'}, '1', 1) @@ -179,16 +182,17 @@ def test_notification_ws(user): resp = client.get(reverse('api-notification-ack', kwargs={'notification_id': '6'})) assert resp.status_code == 200 assert Notification.objects.filter(acked=True).count() == 1 - assert Notification.objects.filter(acked=True).first().public_id() == '6' + assert Notification.objects.filter(acked=True).get().public_id == '6' resp = client.get(reverse('api-notification-forget', kwargs={'notification_id': '5'})) assert resp.status_code == 200 assert Notification.objects.filter(acked=True).count() == 2 - notif = Notification.objects.filter_by_id('5').filter(user=user).first() - assert notif.public_id() == '5' + notif = Notification.objects.find(user, '5').get() + assert notif.public_id == '5' assert notif.acked is True assert notif.end_timestamp < now() + def test_notification_ws_badrequest(user): def check_error(data, message): @@ -212,6 +216,7 @@ def test_notification_ws_badrequest(user): check_error({'summary': 'ok', 'duration': 4.01}, 'valid integer is required') check_error({'summary': 'ok', 'duration': -4}, 'greater than') + def test_notification_ws_deny(): assert client.post(reverse('api-notification-add'), json.dumps({'summary': 'ok'}), @@ -221,6 +226,7 @@ def test_notification_ws_deny(): assert client.get(reverse('api-notification-forget', kwargs={'notification_id': '1'})).status_code == 403 + def test_notification_ws_check_urls(): assert reverse('api-notification-add') == '/api/notification/add/' assert reverse('api-notification-ack', -- 2.14.2