abuse/signals.py

@@ -1,7 +1,7 @@
 import logging
 
 from actstream import action
-from django.db.models.signals import post_save
+from django.db.models.signals import post_save, pre_delete
 from django.dispatch import receiver
 
 from abuse.models import AbuseReport
@@ -45,3 +45,8 @@ def _create_action_from_report(
         target=instance.extension,
         action_object=instance,
     )
+
+
+@receiver(pre_delete, sender=AbuseReport)
+def _log_deletion(sender: object, instance: AbuseReport, **kwargs: object) -> None:
+    instance.record_deletion()

common/model_mixins.py

@@ -2,7 +2,9 @@ from typing import Set, Tuple, Mapping, Any
 import copy
 import logging
 
+from django.contrib.admin.models import DELETION
 from django.contrib.contenttypes.models import ContentType
+from django.core import serializers
 from django.db import models
 from django.shortcuts import reverse
 from django.utils import timezone
@@ -45,7 +47,36 @@ class CreatedModifiedMixin(models.Model):
         return f'{path}?{query}'
 
 
-class TrackChangesMixin(models.Model):
+class RecordDeletionMixin:
+    def serialise(self) -> dict:
+        data = serializers.serialize('python', [self])[0]
+        data['fields']['pk'] = data['pk']
+        return data['fields']
+
+    def record_deletion(self):
+        """Create a LogEntry describing a deletion of this object."""
+        msg_args = {'type': type(self), 'pk': self.pk}
+        logger.info('Deleting %(type)s pk=%(pk)s', msg_args)
+        if hasattr(self, 'cannot_be_deleted_reasons'):
+            cannot_be_deleted_reasons = self.cannot_be_deleted_reasons
+            if len(cannot_be_deleted_reasons) > 0:
+                # This shouldn't happen: prior validation steps should have taken care of this.
+                msg_args['reasons'] = cannot_be_deleted_reasons
+                logger.error("%(type)s pk=%(pk)s is being deleted but it %(reasons)s", msg_args)
+        state = self.serialise()
+        message = [
+            {
+                'deleted': {
+                    'name': str(self._meta.verbose_name),
+                    'object': repr(self),
+                    'old_state': state,
+                },
+            }
+        ]
+        attach_log_entry(self, message, action_flag=DELETION)
+
+
+class TrackChangesMixin(RecordDeletionMixin, models.Model):
     """Tracks changes of Django models.
 
     Tracks which fields have changed in the save() function, so that

common/templates/common/base.html

@@ -1,3 +1,4 @@
+{% load common %}
 {% load pipeline %}
 {% load static %}
 {% load i18n %}
@@ -133,6 +134,9 @@
             {% endblock nav-upload %}
 
             {% if user.is_authenticated %}
+              <a href="{% url 'notifications:notifications' %}">
+                <i class="i-bell {% if user|unread_notification_count %}text-primary{% endif %}"></i>
+              </a>
               <li class="nav-item dropdown">
                 <button id="navbarDropdown" aria-expanded="false" aria-haspopup="true" data-toggle-menu-id="nav-account-dropdown" role="button" class="nav-link dropdown-toggle js-dropdown-toggle">
                   <i class="i-user"></i>

common/templatetags/common.py

@@ -13,6 +13,7 @@ from common.markdown import (
     render_as_text as render_markdown_as_text,
 )
 from extensions.models import Tag
+from notifications.models import Notification
 
 import utils
 
@@ -160,3 +161,8 @@ def replace(value, old_char_new_char):
     """Replaces occurrences of old_char with new_char in the given value."""
     old_char, new_char = old_char_new_char.split(',')
     return value.replace(old_char, new_char)
+
+
+@register.filter(name='unread_notification_count')
+def unread_notification_count(user):
+    return Notification.objects.filter(recipient=user, read_at__isnull=True).count()

extensions/fields.py

@@ -22,7 +22,7 @@ class VersionStringField(SemanticVersionField):
         return str(value)
 
     def value_to_string(self, obj):
-        value = self._get_val_from_obj(obj)
+        value = self.value_from_object(obj)
         return self.get_prep_value(value)
 
     def from_json(self, json_str):

extensions/models.py

@@ -10,7 +10,7 @@ from django.db.models import F, Q, Count
 from django.urls import reverse
 
 from common.fields import FilterableManyToManyField
-from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin
+from common.model_mixins import CreatedModifiedMixin, RecordDeletionMixin, TrackChangesMixin
 from constants.base import (
     AUTHOR_ROLE_CHOICES,
     AUTHOR_ROLE_DEV,
@@ -651,7 +651,7 @@ class Maintainer(CreatedModifiedMixin, models.Model):
         ]
 
 
-class Preview(CreatedModifiedMixin, models.Model):
+class Preview(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
     extension = models.ForeignKey(Extension, on_delete=models.CASCADE)
     file = models.ForeignKey(
         'files.File', related_name='extension_preview', on_delete=models.CASCADE

extensions/signals.py

@@ -18,15 +18,14 @@ User = get_user_model()
 @receiver(pre_delete, sender=extensions.models.Extension)
 @receiver(pre_delete, sender=extensions.models.Preview)
 @receiver(pre_delete, sender=extensions.models.Version)
-def _log_extension_delete(sender: object, instance: object, **kwargs: object) -> None:
-    cannot_be_deleted_reasons = instance.cannot_be_deleted_reasons
-    if len(cannot_be_deleted_reasons) > 0:
-        # This shouldn't happen: prior validation steps should have taken care of this.
-        # raise ValidationError({'__all__': cannot_be_deleted_reasons})
-        args = {'sender': sender, 'pk': instance.pk, 'reasons': cannot_be_deleted_reasons}
-        logger.error("%(sender)s pk=%(pk)s is being deleted but it %(reasons)s", args)
-
-    logger.info('Deleting %s pk=%s "%s"', sender, instance.pk, str(instance))
+def _log_deletion(
+    sender: object,
+    instance: Union[
+        extensions.models.Extension, extensions.models.Version, extensions.models.Preview
+    ],
+    **kwargs: object,
+) -> None:
+    instance.record_deletion()
 
 
 @receiver(post_delete, sender=extensions.models.Preview)

extensions/tests/test_delete.py

@@ -1,16 +1,21 @@
-from django.test import TestCase
+import json
+
+from django.contrib.admin.models import LogEntry, DELETION
+from django.test import TestCase  # , TransactionTestCase
 
 from common.tests.factories.extensions import create_approved_version, create_version
 from common.tests.factories.files import FileFactory
-from common.tests.factories.users import UserFactory
+from common.tests.factories.users import UserFactory, create_moderator
 import extensions.models
 import files.models
+import reviewers.models
 
 
 class DeleteTest(TestCase):
     fixtures = ['dev', 'licenses']
 
     def test_unlisted_unrated_extension_can_be_deleted_by_author(self):
+        self.maxDiff = None
         version = create_version(
             file__status=files.models.File.STATUSES.AWAITING_REVIEW,
             ratings=[],
@@ -29,6 +34,31 @@ class DeleteTest(TestCase):
         self.assertEqual(extension.cannot_be_deleted_reasons, [])
         preview_file = extension.previews.first()
         self.assertIsNotNone(preview_file)
+        # Create some ApprovalActivity as well
+        moderator = create_moderator()
+        approval_activity = reviewers.models.ApprovalActivity.objects.create(
+            extension=extension,
+            user=moderator,
+            message='This is a message in approval activity',
+        )
+        # Create a file validation record
+        file_validation = files.models.FileValidation.objects.create(
+            file=version_file, results={'deadbeef': 'foobar'}
+        )
+        object_reprs = list(
+            map(
+                repr,
+                [
+                    preview_file,
+                    version_file,
+                    file_validation,
+                    extension,
+                    approval_activity,
+                    preview_file.extension_preview.first(),
+                    version,
+                ],
+            )
+        )
 
         url = extension.get_delete_url()
         user = extension.authors.first()
@@ -49,6 +79,47 @@ class DeleteTest(TestCase):
         self.assertIsNone(extensions.models.Version.objects.filter(pk=version.pk).first())
         self.assertIsNone(files.models.File.objects.filter(pk=version_file.pk).first())
         self.assertIsNone(files.models.File.objects.filter(pk=preview_file.pk).first())
+
+        # Check that each of the deleted records was logged
+        deletion_log_entries_q = LogEntry.objects.filter(action_flag=DELETION)
+        self.assertEqual(deletion_log_entries_q.count(), 7)
+        self.assertEqual(
+            [_.object_repr for _ in deletion_log_entries_q],
+            object_reprs,
+        )
+        log_entry = deletion_log_entries_q.filter(object_repr__contains='Extension').first()
+        change_message_data = json.loads(log_entry.change_message)
+        self.assertEqual(
+            change_message_data[0]['deleted']['object'], f'<Extension: Add-on "{extension.name}">'
+        )
+        self.assertEqual(
+            set(change_message_data[0]['deleted']['old_state'].keys()),
+            {
+                'average_score',
+                'date_approved',
+                'date_created',
+                'date_modified',
+                'date_status_changed',
+                'description',
+                'download_count',
+                'extension_id',
+                'is_listed',
+                'name',
+                'pk',
+                'slug',
+                'status',
+                'support',
+                'team',
+                'type',
+                'view_count',
+                'website',
+            },
+        )
+        self.assertEqual(
+            log_entry.get_change_message(),
+            f'Deleted extension “<Extension: Add-on "{extension.name}">”.',
+        )
+
         # TODO: check that files were deleted from storage (create a temp one prior to the check)
 
     def test_publicly_listed_extension_cannot_be_deleted(self):

files/signals.py

@@ -36,5 +36,6 @@ def _scan_new_file(
 
 
 @receiver(pre_delete, sender=files.models.File)
-def _log_file_delete(sender: object, instance: files.models.File, **kwargs: object) -> None:
-    logger.info('Deleting file pk=%s source=%s', instance.pk, instance.source.name)
+@receiver(pre_delete, sender=files.models.FileValidation)
+def _log_deletion(sender: object, instance: files.models.File, **kwargs: object) -> None:
+    instance.record_deletion()

notifications/management/commands/send_notification_emails.py

@@ -28,11 +28,6 @@ class Command(BaseCommand):
                 logger.info(f'{recipient} has unconfirmed email, skipping')
                 n.save()
                 continue
-            # FIXME test with only internal emails first
-            if not recipient.email.endswith('@blender.org'):
-                logger.info('skipping: not an internal email')
-                n.save()
-                continue
             n.email_sent = True
             # first mark as processed, then send: avoid spamming in case of a crash-loop
             n.save()

notifications/models.py

@@ -32,9 +32,9 @@ class Notification(models.Model):
 
     def format_email(self):
         action = self.action
-        subject = f'New Activity: {action.actor.full_name} {action.verb} {action.target}'
+        subject = f'New Activity: {action.actor} {action.verb} {action.target}'
         url = self.get_absolute_url()
-        mesage = f'{action.actor.full_name} {action.verb} {action.target}: {url}'
+        mesage = f'{action.actor} {action.verb} {action.target}: {url}'
         return (subject, mesage)
 
     def get_absolute_url(self):

notifications/templates/notifications/notification_list.html

@@ -1,22 +1,48 @@
 {% extends "common/base.html" %}
-{% load i18n %}
+{% load common filters i18n %}
 
 {% block page_title %}{% blocktranslate %}Notifications{% endblocktranslate %}{% endblock page_title %}
 
 {% block content %}
+<h1>
+{% trans 'Notifications' %}
+  {% if user|unread_notification_count %}
+  <form class="d-inline" action="{% url 'notifications:notifications-mark-read-all' %}" method="post">
+    {% csrf_token %}
+    <button class="btn btn-sm" type="submit">{% trans 'Mark all as read' %}</button>
+  </form>
+  {% endif %}
+</h1>
 {% if notification_list %}
   {% for notification in notification_list %}
-    <div class="row">
-      {{ notification.action }}
-      {% if notification.read_at %}
-      {% else %}
-      {% blocktranslate %}Mark as read{% endblocktranslate %}
+  <div class="row mb-2 {% if notification.read_at%}text-muted{% endif %}">
+      <div class="col">
+
+      {{ notification.action.timestamp | naturaltime_compact }}
+
+      <a href="{% url 'extensions:by-author' user_id=notification.action.actor.pk %}">
+      {{ notification.action.actor }}
+      </a>
+
+      {{ notification.action.verb }}
+
+      <a href="{{ notification.action.target.get_absolute_url }}">{{ notification.action.target }}</a>
+
+      <a href="{{ notification.get_absolute_url }}"><button class="btn btn-sm">{% trans 'View' %}</button></a>
+
+      {% if not notification.read_at %}
+      <form class="d-inline" action="{% url 'notifications:notifications-mark-read' pk=notification.pk %}" method="post">
+        {% csrf_token %}
+        <button class="btn btn-sm" type="submit">{% trans 'Mark as read' %}</button>
+      </form>
       {% endif %}
+
+      </div>
     </div>
   {% endfor %}
 {% else %}
 <p>
-  {% blocktranslate %}You have no notifications{% endblocktranslate %}
+  {% trans 'You have no notifications' %}
 </p>
 {% endif %}
 {% endblock content %}

notifications/views.py

@@ -1,7 +1,7 @@
 """Notifications pages."""
 from django.contrib.auth.mixins import LoginRequiredMixin
 from django.http import HttpResponseForbidden
-from django.http.response import JsonResponse
+from django.shortcuts import redirect
 from django.utils import timezone
 from django.views.generic import ListView
 from django.views.generic.detail import SingleObjectMixin
@@ -13,11 +13,10 @@ from notifications.models import Notification
 
 class NotificationsView(LoginRequiredMixin, ListView):
     model = Notification
-    ordering = None  # FIXME
     paginate_by = 10
 
     def get_queryset(self):
-        return Notification.objects.filter(recipient=self.request.user)
+        return Notification.objects.filter(recipient=self.request.user).order_by('-id')
 
 
 class MarkReadAllView(LoginRequiredMixin, FormView):
@@ -32,8 +31,7 @@ class MarkReadAllView(LoginRequiredMixin, FormView):
             notification.read_at = now
 
         Notification.objects.bulk_update(unread, ['read_at'])
-
-        return JsonResponse({})
+        return redirect('notifications:notifications')
 
 
 class MarkReadView(LoginRequiredMixin, SingleObjectMixin, View):
@@ -46,4 +44,4 @@ class MarkReadView(LoginRequiredMixin, SingleObjectMixin, View):
             return HttpResponseForbidden()
         notification.read_at = timezone.now()
         notification.save(update_fields=['read_at'])
-        return JsonResponse({})
+        return redirect('notifications:notifications')

ratings/signals.py

@@ -1,5 +1,5 @@
 from actstream import action
-from django.db.models.signals import pre_save, post_save
+from django.db.models.signals import pre_save, post_save, pre_delete
 from django.dispatch import receiver
 
 from constants.activity import Verb
@@ -40,3 +40,8 @@ def _create_action_from_rating(
         action_object=instance,
         target=instance.extension,
     )
+
+
+@receiver(pre_delete, sender=Rating)
+def _log_deletion(sender: object, instance: Rating, **kwargs: object) -> None:
+    instance.record_deletion()

reviewers/models.py

@@ -9,7 +9,7 @@ from django.utils.translation import gettext_lazy as _
 
 import common.help_texts
 from extensions.models import Extension
-from common.model_mixins import CreatedModifiedMixin
+from common.model_mixins import CreatedModifiedMixin, RecordDeletionMixin
 from utils import absolutify, send_mail
 
 from constants.base import EXTENSION_TYPE_CHOICES
@@ -74,7 +74,7 @@ class ReviewerSubscription(CreatedModifiedMixin, models.Model):
         )
 
 
-class ApprovalActivity(CreatedModifiedMixin, models.Model):
+class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
     class ActivityType(models.TextChoices):
         COMMENT = "COM", _("Comment")
         APPROVED = "APR", _("Approved")

reviewers/signals.py

@@ -1,6 +1,6 @@
 from actstream import action
 from actstream.actions import follow
-from django.db.models.signals import post_save
+from django.db.models.signals import post_save, pre_delete
 from django.dispatch import receiver
 
 from constants.activity import Flag, Verb
@@ -37,3 +37,8 @@ def _create_action_from_review_and_follow(
         action_object=instance,
         target=instance.extension,
     )
+
+
+@receiver(pre_delete, sender=ApprovalActivity)
+def _log_deletion(sender: object, instance: ApprovalActivity, **kwargs: object) -> None:
+    instance.record_deletion()

reviewers/templates/reviewers/extensions_review_detail.html

@@ -168,7 +168,6 @@
         <div class="btn-row ms-3 w-100 justify-content-end">
           {% if is_maintainer or request.user.is_moderator %}
           {% include "common/components/field.html" with field=form.type %}
-          {% endif %}
           <button type="submit" id="activity-submit" class="btn btn-primary">
             <span>{% trans "Comment" %}</span>
           </button>

reviewers/views.py

@@ -4,6 +4,7 @@ from django.contrib.auth.mixins import LoginRequiredMixin
 from django.views.generic.list import ListView
 from django.views.generic import DetailView, FormView
 from django.shortcuts import reverse
+import django.forms
 
 from files.models import File
 from extensions.models import Extension
@@ -39,7 +40,9 @@ class ExtensionsApprovalDetailView(DetailView):
         if self.request.user.is_authenticated:
             form = ctx['comment_form'] = CommentForm()
             # Remove 'Approved' status from dropdown it not moderator
-            if not (self.request.user.is_moderator or self.request.user.is_superuser):
+            filtered_activity_types = ApprovalActivity.ActivityType.choices
+            user = self.request.user
+            if not (user.is_moderator or user.is_superuser):
                 filtered_activity_types = [
                     t
                     for t in ApprovalActivity.ActivityType.choices
@@ -49,8 +52,17 @@ class ExtensionsApprovalDetailView(DetailView):
                         ApprovalActivity.ActivityType.AWAITING_CHANGES,
                     ]
                 ]
+                if not self.object.has_maintainer(user):
+                    # Other accounts can only comment
+                    filtered_activity_types = [
+                        t
+                        for t in ApprovalActivity.ActivityType.choices
+                        if t[0] == ApprovalActivity.ActivityType.COMMENT
+                    ]
                 form.fields['type'].choices = filtered_activity_types
                 form.fields['type'].widget.choices = filtered_activity_types
+                if len(filtered_activity_types) == 1:
+                    form.fields['type'].widget = django.forms.HiddenInput()
         return ctx