Fix error when double notif appears due to race condition (#4377)

* Fix sentry error for notification

* make functions atomic

and transform a dangerous save into update.

* good log format

* line

zds/notification/models.py

@@ -7,9 +7,9 @@
 from django.contrib.auth.models import User
 from django.contrib.contenttypes.fields import GenericForeignKey
 from django.contrib.contenttypes.models import ContentType
-from django.core.exceptions import ObjectDoesNotExist
+from django.core.exceptions import ObjectDoesNotExist, MultipleObjectsReturned
 from django.core.mail import EmailMultiAlternatives
-from django.db import models, IntegrityError
+from django.db import models, IntegrityError, transaction
 from django.template.loader import render_to_string
 from django.utils.translation import ugettext_lazy as _
 
@@ -143,30 +143,36 @@ def send_notification(self, content=None, send_email=True, sender=None):
         assert hasattr(self, 'get_notification_url')
         assert hasattr(self, 'get_notification_title')
         assert hasattr(self, 'send_email')
-
-        if self.last_notification is None or self.last_notification.is_read:
-            # If there isn't a notification yet or the last one is read, we generate a new one.
-            try:
-                notification = Notification.objects.get(subscription=self)
-            except Notification.DoesNotExist:
-                notification = Notification(subscription=self, content_object=content, sender=sender)
-            notification.content_object = content
-            notification.sender = sender
-            notification.url = self.get_notification_url(content)
-            notification.title = self.get_notification_title(content)
-            notification.pubdate = content.pubdate
-            notification.is_read = False
-            notification.save()
-            self.set_last_notification(notification)
-            self.save()
-
-            if send_email and self.by_email:
-                self.send_email(notification)
-        elif self.last_notification is not None:
-            # Update last notification if the new content is older (case of unreading answer)
-            if not self.last_notification.is_read and self.last_notification.pubdate > content.pubdate:
-                self.last_notification.content_object = content
-                self.last_notification.save()
+        with transaction.atomic():
+            if self.last_notification is None or self.last_notification.is_read:
+                # If there isn't a notification yet or the last one is read, we generate a new one.
+                try:
+                    notification = Notification.objects.get(subscription=self)
+                except Notification.DoesNotExist:
+                    notification = Notification(subscription=self, content_object=content, sender=sender)
+                except MultipleObjectsReturned:
+                    notifications = list(Notification.objects.filter(subscription=self))
+                    LOG.error('found %s notifications for %s', len(notifications), self, exc_info=True)
+                    Notification.objects.filter(pk__in=[n.pk for n in notifications[1:]]).delete()
+                    LOG.info('removed doubly.')
+                    notification = notifications[0]
+                notification.content_object = content
+                notification.sender = sender
+                notification.url = self.get_notification_url(content)
+                notification.title = self.get_notification_title(content)
+                notification.pubdate = content.pubdate
+                notification.is_read = False
+                notification.save()
+                self.set_last_notification(notification)
+                self.save()
+
+                if send_email and self.by_email:
+                    self.send_email(notification)
+            elif self.last_notification is not None:
+                # Update last notification if the new content is older (case of unreading answer)
+                if not self.last_notification.is_read and self.last_notification.pubdate > content.pubdate:
+                    self.last_notification.content_object = content
+                    self.last_notification.save()
 
     def mark_notification_read(self):
         """
@@ -175,8 +181,7 @@ def mark_notification_read(self):
         no need for more precision
         """
         if self.last_notification is not None:
-            self.last_notification.is_read = True
-            self.last_notification.save()
+            Notification.objects.filter(pk=self.last_notification.pk).update(is_read=True)
 
 
 class MultipleNotificationsMixin(object):
@@ -192,48 +197,49 @@ def send_notification(self, content=None, send_email=True, sender=None):
         assert hasattr(self, 'get_notification_url')
         assert hasattr(self, 'get_notification_title')
         assert hasattr(self, 'send_email')
+        with transaction.atomic():
+            notification = Notification(subscription=self, content_object=content, sender=sender)
+            notification.content_object = content
+            notification.sender = sender
+            notification.url = self.get_notification_url(content)
+            notification.title = self.get_notification_title(content)
+            notification.is_read = False
+            notification.save()
+            self.set_last_notification(notification)
+            self.save()
 
-        notification = Notification(subscription=self, content_object=content, sender=sender)
-        notification.content_object = content
-        notification.sender = sender
-        notification.url = self.get_notification_url(content)
-        notification.title = self.get_notification_title(content)
-        notification.is_read = False
-        notification.save()
-        self.set_last_notification(notification)
-        self.save()
-
-        if send_email and self.by_email:
-            self.send_email(notification)
+            if send_email and self.by_email:
+                self.send_email(notification)
 
     def mark_notification_read(self, content):
         """
         Marks the notification of the subscription as read.
         :param content : the content whose notification has been read
         """
         if content is None:
-            raise Exception('Object content of notification must be defined')
-
-        content_notification_type = ContentType.objects.get_for_model(content)
-        notifications = list(Notification.objects.filter(subscription=self,
-                                                         content_type__pk=content_notification_type.pk,
-                                                         object_id=content.pk, is_read=False))
-        # handles cases where a same subscription lead to several notifications
-        if not notifications:
-            LOG.debug('nothing to mark as read')
-            return
-        elif len(notifications) > 1:
-            LOG.warning('%s notifications were find for %s/%s', len(notifications), content.type, content.title)
-            for notif in notifications[1:]:
-                notif.delete()
-
-        notification = notifications[0]
-        notification.subscription = self
-        notification.is_read = True
-        try:
-            notification.save()
-        except IntegrityError:
-            LOG.exception('Could not save %s', notification)
+            raise ValueError('Object content of notification must be defined')
+
+        with transaction.atomic():
+            content_notification_type = ContentType.objects.get_for_model(content)
+            notifications = list(Notification.objects.filter(subscription=self,
+                                                             content_type__pk=content_notification_type.pk,
+                                                             object_id=content.pk, is_read=False))
+            # handles cases where a same subscription lead to several notifications
+            if not notifications:
+                LOG.debug('nothing to mark as read')
+                return
+            elif len(notifications) > 1:
+                LOG.warning('%s notifications were find for %s/%s', len(notifications), content.type, content.title)
+                for notif in notifications[1:]:
+                    notif.delete()
+
+            notification = notifications[0]
+            notification.subscription = self
+            notification.is_read = True
+            try:
+                notification.save()
+            except IntegrityError:
+                LOG.exception('Could not save %s', notification)
 
 
 @python_2_unicode_compatible