From 6ee84610b4fcd0c6b2702042ba305ca85f11dec4 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 9 Apr 2024 18:14:55 +0200 Subject: [PATCH 01/28] a stub notification signal for approval queue activity --- blender_extensions/settings.py | 8 ++++++-- extensions/apps.py | 3 +++ requirements.txt | 1 + reviewers/apps.py | 5 +++++ reviewers/views.py | 12 ++++++++++++ users/apps.py | 4 ++++ users/signals.py | 19 ++++++++++++++++++- 7 files changed, 49 insertions(+), 3 deletions(-) diff --git a/blender_extensions/settings.py b/blender_extensions/settings.py index 4c75234f..1bcb8d89 100644 --- a/blender_extensions/settings.py +++ b/blender_extensions/settings.py @@ -1,5 +1,4 @@ -""" -Django settings for blender_extensions project. +"""Django settings for blender_extensions project. Generated by 'django-admin startproject' using Django 4.0.6. @@ -74,6 +73,7 @@ INSTALLED_APPS = [ 'django.contrib.staticfiles', 'django.contrib.flatpages', 'django.contrib.humanize', + 'actstream', ] MIDDLEWARE = [ @@ -320,3 +320,7 @@ EMAIL_HOST = os.getenv('EMAIL_HOST') EMAIL_PORT = os.getenv('EMAIL_PORT', '587') EMAIL_HOST_USER = os.getenv('EMAIL_HOST_USER') EMAIL_HOST_PASSWORD = os.getenv('EMAIL_HOST_PASSWORD') + +ACTSTREAM_SETTINGS = { + 'MANAGER': 'actstream.managers.ActionManager', +} diff --git a/extensions/apps.py b/extensions/apps.py index 2942caa2..d51ca42a 100644 --- a/extensions/apps.py +++ b/extensions/apps.py @@ -7,3 +7,6 @@ class ExtensionsConfig(AppConfig): def ready(self): import extensions.signals # noqa: F401 + from actstream import registry + + registry.register(self.get_model('Extension')) diff --git a/requirements.txt b/requirements.txt index 34a934f0..93f88c9f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,6 +12,7 @@ click==8.1.3 colorhash==1.0.4 Django==4.2.11 dj-database-url==1.0.0 +django-activity-stream==2.0.0 django-admin-rangefilter==0.8.5 django-background-tasks-updated @ git+https://projects.blender.org/infrastructure/django-background-tasks.git@2e60c4ec2fd1e7155bc3f041e0ea4875495a476b django-compat==1.0.15 diff --git a/reviewers/apps.py b/reviewers/apps.py index 02fcdcbc..fe959188 100644 --- a/reviewers/apps.py +++ b/reviewers/apps.py @@ -4,3 +4,8 @@ from django.apps import AppConfig class ReviewersConfig(AppConfig): default_auto_field = 'django.db.models.BigAutoField' name = 'reviewers' + + def ready(self) -> None: + from actstream import registry + + registry.register(self.get_model('ApprovalActivity')) diff --git a/reviewers/views.py b/reviewers/views.py index fb2668e0..f0b258cc 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -1,5 +1,7 @@ import logging +from actstream import action +from actstream.actions import follow from django.contrib.auth.mixins import LoginRequiredMixin from django.views.generic.list import ListView from django.views.generic import DetailView, FormView @@ -76,4 +78,14 @@ class ExtensionsApprovalFormView(LoginRequiredMixin, FormView): form.instance.extension = Extension.objects.get(slug=self.kwargs['slug']) form.save() self.approve_if_allowed(form) + # automatically follow after ineraction + # if a user had unfollowed this extension before, + # we unfortunately are making them a follower again + follow(self.request.user, form.instance.extension, send_action=False) + action.send( + self.request.user, + verb=form.cleaned_data['type'], # FIXME map the type into a "registered" verb + action_object=form.instance, + target=form.instance.extension, + ) return super().form_valid(form) diff --git a/users/apps.py b/users/apps.py index c6b7ce20..e50ae5c0 100644 --- a/users/apps.py +++ b/users/apps.py @@ -7,3 +7,7 @@ class UsersConfig(AppConfig): def ready(self) -> None: import users.signals # noqa: F401 + from actstream import registry + from django.contrib.auth import get_user_model + + registry.register(get_user_model()) diff --git a/users/signals.py b/users/signals.py index 8a6b5e91..4a367c9f 100644 --- a/users/signals.py +++ b/users/signals.py @@ -1,8 +1,9 @@ from typing import Dict import logging +from actstream.models import Action, followers from django.contrib.auth import get_user_model -from django.db.models.signals import pre_save +from django.db.models.signals import post_save, pre_save from django.dispatch import receiver from blender_id_oauth_client import signals as bid_signals @@ -35,3 +36,19 @@ def update_user( bid.copy_avatar_from_blender_id(user=instance) bid.copy_badges_from_blender_id(user=instance) + + +@receiver(post_save, sender=Action) +def create_notification(sender: object, instance: Action, created: bool, **kwargs: object) -> None: + if not created: + return + + if not instance.target: + return + + audience = filter(lambda f: f != instance.actor, followers(instance.target)) + for recipient in audience: + print( + f'create notification for {recipient}: ', + f'{instance.actor} {instance.verb} {instance.target}', + ) -- 2.30.2 From 5e31fdcc84324241b1a08bdf7894bd3c84fd9e84 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 9 Apr 2024 19:18:09 +0200 Subject: [PATCH 02/28] subscribe moderators to new extensions submitted for review currently save as draft also submits for review, TODO investigate --- extensions/signals.py | 25 ++++++++++++++++--------- extensions/views/manage.py | 6 ++++++ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/extensions/signals.py b/extensions/signals.py index e6f2d682..400f3965 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -1,16 +1,14 @@ from typing import Union +from actstream.actions import follow +from django.contrib.auth.models import Group from django.db.models.signals import pre_save, post_save, post_delete from django.dispatch import receiver -import django.dispatch import extensions.models import extensions.tasks import files.models -version_changed = django.dispatch.Signal() -version_uploaded = django.dispatch.Signal() - @receiver(post_delete, sender=extensions.models.Preview) def _delete_file(sender: object, instance: extensions.models.Preview, **kwargs: object) -> None: @@ -39,8 +37,20 @@ def _update_search_index(sender, instance, **kw): pass # TODO: update search index -def send_notifications(sender=None, instance=None, signal=None, **kw): - pass # TODO: send email notification about new version upload +@receiver(post_save, sender=extensions.models.Extension) +def setup_initial_followers( + sender: object, + instance: extensions.models.Extension, + created: bool, + **kwargs: object, +) -> None: + if not created: + return + + moderators = Group.objects.get(name='moderators').user_set.all() + audience = list(moderators) + list(instance.authors.all()) + for recipient in audience: + follow(recipient, instance, send_action=False) def extension_should_be_listed(extension): @@ -84,6 +94,3 @@ def _set_is_listed( extension.is_listed = new_is_listed extension.save() - - -version_uploaded.connect(send_notifications, dispatch_uid='send_notifications') diff --git a/extensions/views/manage.py b/extensions/views/manage.py index eac467e9..0853a2bf 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -1,4 +1,5 @@ """Contains views allowing developers to manage their add-ons.""" +from actstream import action from django import forms from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin from django.contrib.messages.views import SuccessMessageMixin @@ -394,6 +395,11 @@ class DraftExtensionView( extension_form.save() add_preview_formset.save() form.save() + action.send( + self.request.user, + verb='submitted for review', # FIXME map the type into a "registered" verb + target=extension_form.instance, + ) return super().form_valid(form) except forms.ValidationError as e: if 'hash' in e.error_dict: -- 2.30.2 From ff3a13c1fee104fe0fae3742387c97073fc199ac Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 11 Apr 2024 12:34:01 +0200 Subject: [PATCH 03/28] move follow calls to a view --- extensions/signals.py | 18 ------------------ extensions/views/manage.py | 18 +++++++++++++----- 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/extensions/signals.py b/extensions/signals.py index 400f3965..d4a3bbaf 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -1,7 +1,5 @@ from typing import Union -from actstream.actions import follow -from django.contrib.auth.models import Group from django.db.models.signals import pre_save, post_save, post_delete from django.dispatch import receiver @@ -37,22 +35,6 @@ def _update_search_index(sender, instance, **kw): pass # TODO: update search index -@receiver(post_save, sender=extensions.models.Extension) -def setup_initial_followers( - sender: object, - instance: extensions.models.Extension, - created: bool, - **kwargs: object, -) -> None: - if not created: - return - - moderators = Group.objects.get(name='moderators').user_set.all() - audience = list(moderators) + list(instance.authors.all()) - for recipient in audience: - follow(recipient, instance, send_action=False) - - def extension_should_be_listed(extension): return ( extension.latest_version is not None diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 0853a2bf..3d106b88 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -1,5 +1,7 @@ """Contains views allowing developers to manage their add-ons.""" from actstream import action +from actstream.actions import follow +from django.contrib.auth.models import Group from django import forms from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin from django.contrib.messages.views import SuccessMessageMixin @@ -395,11 +397,17 @@ class DraftExtensionView( extension_form.save() add_preview_formset.save() form.save() - action.send( - self.request.user, - verb='submitted for review', # FIXME map the type into a "registered" verb - target=extension_form.instance, - ) + # setup initial followers + moderators = Group.objects.get(name='moderators').user_set.all() + audience = list(moderators) + list(extension_form.instance.authors.all()) + for recipient in audience: + follow(recipient, extension_form.instance, send_action=False) + if 'submit_draft' in self.request.POST: + action.send( + self.request.user, + verb='submitted for review', # FIXME map the type into a "registered" verb + target=extension_form.instance, + ) return super().form_valid(form) except forms.ValidationError as e: if 'hash' in e.error_dict: -- 2.30.2 From 2a6f6c144bb812c762a95429fdf4f915167703f0 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 11 Apr 2024 17:44:32 +0200 Subject: [PATCH 04/28] create actions for extension and review reports --- abuse/apps.py | 6 ++++++ abuse/models.py | 1 + abuse/signals.py | 44 ++++++++++++++++++++++++++++++++++++++ constants/activity.py | 10 +++++++++ extensions/views/manage.py | 3 ++- reviewers/views.py | 12 ++++++++++- users/signals.py | 1 + 7 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 abuse/signals.py create mode 100644 constants/activity.py diff --git a/abuse/apps.py b/abuse/apps.py index 90891569..08a2c9d3 100644 --- a/abuse/apps.py +++ b/abuse/apps.py @@ -4,3 +4,9 @@ from django.apps import AppConfig class AbuseConfig(AppConfig): default_auto_field = 'django.db.models.BigAutoField' name = 'abuse' + + def ready(self): + import extensions.signals # noqa: F401 + from actstream import registry + + registry.register(self.get_model('AbuseReport')) diff --git a/abuse/models.py b/abuse/models.py index 634ef2eb..31907b28 100644 --- a/abuse/models.py +++ b/abuse/models.py @@ -34,6 +34,7 @@ class AbuseReport(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, mode ) # NULL if the reporter is anonymous. + # FIXME? make non-null reporter = models.ForeignKey( User, null=True, diff --git a/abuse/signals.py b/abuse/signals.py new file mode 100644 index 00000000..119c4bc5 --- /dev/null +++ b/abuse/signals.py @@ -0,0 +1,44 @@ +import logging + +from actstream import action +from django.db.models.signals import post_save +from django.dispatch import receiver + +from abuse.models import AbuseReport +from constants.activity import Verb +from constants.base import ( + ABUSE_TYPE_EXTENSION, + ABUSE_TYPE_REVIEW, + ABUSE_TYPE_USER, +) + +logger = logging.getLogger(__name__) + + +@receiver(post_save, sender=AbuseReport) +def create_action_from_report( + sender: object, + instance: AbuseReport, + created: bool, + **kwargs: object, +) -> None: + if not created: + return + + if instance.type == ABUSE_TYPE_EXTENSION: + verb = Verb.REPORTED_EXTENSION + elif instance.type == ABUSE_TYPE_REVIEW: + verb = Verb.REPORTED_REVIEW + elif instance.type == ABUSE_TYPE_USER: + # TODO? + pass + else: + logger.warning("ignoring an unexpected AbuseReport type={instance.type}") + return + + action.send( + instance.reporter, + verb=verb, + target=instance.extension, + action_object=instance, + ) diff --git a/constants/activity.py b/constants/activity.py new file mode 100644 index 00000000..1eff5382 --- /dev/null +++ b/constants/activity.py @@ -0,0 +1,10 @@ +class Verb: + SUBMITTED_FOR_REVIEW = 'submitted for review' + + REPORTED_EXTENSION = 'reported extension' + REPORTED_REVIEW = 'reported review' + + APPROVED = 'approved' + COMMENTED = 'commented' + REQUESTED_CHANGES = 'requested changes' + REQUESTED_REVIEW = 'requested review' diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 3d106b88..decc7552 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -18,6 +18,7 @@ from .mixins import ( DraftVersionMixin, DraftMixin, ) +from constants.activity import Verb from extensions.forms import ( EditPreviewFormSet, AddPreviewFormSet, @@ -405,7 +406,7 @@ class DraftExtensionView( if 'submit_draft' in self.request.POST: action.send( self.request.user, - verb='submitted for review', # FIXME map the type into a "registered" verb + verb=Verb.SUBMITTED_FOR_REVIEW, target=extension_form.instance, ) return super().form_valid(form) diff --git a/reviewers/views.py b/reviewers/views.py index f0b258cc..b8e409cf 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -7,6 +7,7 @@ from django.views.generic.list import ListView from django.views.generic import DetailView, FormView from django.shortcuts import reverse +from constants.activity import Verb from files.models import File from extensions.models import Extension from reviewers.forms import CommentForm @@ -78,14 +79,23 @@ class ExtensionsApprovalFormView(LoginRequiredMixin, FormView): form.instance.extension = Extension.objects.get(slug=self.kwargs['slug']) form.save() self.approve_if_allowed(form) + # automatically follow after ineraction # if a user had unfollowed this extension before, # we unfortunately are making them a follower again follow(self.request.user, form.instance.extension, send_action=False) + + activity_type2verb = { + ApprovalActivity.ActivityType.APPROVED: Verb.APPROVED, + ApprovalActivity.ActivityType.AWAITING_CHANGES: Verb.REQUESTED_CHANGES, + ApprovalActivity.ActivityType.AWAITING_REVIEW: Verb.REQUESTED_REVIEW, + ApprovalActivity.ActivityType.COMMENT: Verb.COMMENTED, + } action.send( self.request.user, - verb=form.cleaned_data['type'], # FIXME map the type into a "registered" verb + verb=activity_type2verb.get(form.cleaned_data['type']), action_object=form.instance, target=form.instance.extension, ) + return super().form_valid(form) diff --git a/users/signals.py b/users/signals.py index 4a367c9f..576d7b4f 100644 --- a/users/signals.py +++ b/users/signals.py @@ -44,6 +44,7 @@ def create_notification(sender: object, instance: Action, created: bool, **kwarg return if not instance.target: + logger.warning('ignoring an unexpected Action without a target, verb={instance.verb}') return audience = filter(lambda f: f != instance.actor, followers(instance.target)) -- 2.30.2 From dbaeeddcfde4e4df0298fe9846f348addde70603 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 11 Apr 2024 18:35:18 +0200 Subject: [PATCH 05/28] register new ratings as actions --- constants/activity.py | 2 ++ ratings/apps.py | 3 +++ ratings/signals.py | 23 +++++++++++++++++++++++ users/signals.py | 10 +++++++++- 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/constants/activity.py b/constants/activity.py index 1eff5382..0697f2d6 100644 --- a/constants/activity.py +++ b/constants/activity.py @@ -8,3 +8,5 @@ class Verb: COMMENTED = 'commented' REQUESTED_CHANGES = 'requested changes' REQUESTED_REVIEW = 'requested review' + + RATED_EXTENSION = 'rated extension' diff --git a/ratings/apps.py b/ratings/apps.py index 15464229..13b958e8 100644 --- a/ratings/apps.py +++ b/ratings/apps.py @@ -7,3 +7,6 @@ class RatingsConfig(AppConfig): def ready(self): import ratings.signals # noqa: F401 + from actstream import registry + + registry.register(self.get_model('Rating')) diff --git a/ratings/signals.py b/ratings/signals.py index 23e10d29..4a5071cd 100644 --- a/ratings/signals.py +++ b/ratings/signals.py @@ -1,6 +1,8 @@ +from actstream import action from django.db.models.signals import pre_save, post_save from django.dispatch import receiver +from constants.activity import Verb from ratings.models import Rating @@ -17,3 +19,24 @@ def _update_rating_counters(sender, instance, *args, **kwargs): version = instance.version version.recalculate_average_score() + + +@receiver(post_save, sender=Rating) +def create_action_from_rating( + sender: object, + instance: Rating, + created: bool, + raw: bool, + **kwargs: object, +) -> None: + if raw: + return + if not created: + return + + action.send( + instance.user, + verb=Verb.RATED_EXTENSION, + action_object=instance, + target=instance.extension, + ) diff --git a/users/signals.py b/users/signals.py index 576d7b4f..5528b041 100644 --- a/users/signals.py +++ b/users/signals.py @@ -39,7 +39,15 @@ def update_user( @receiver(post_save, sender=Action) -def create_notification(sender: object, instance: Action, created: bool, **kwargs: object) -> None: +def create_notification( + sender: object, + instance: Action, + created: bool, + raw: bool, + **kwargs: object, +) -> None: + if raw: + return if not created: return -- 2.30.2 From 144712c3f7713fee7a705a3708041bffdc94c4d5 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 11 Apr 2024 19:26:31 +0200 Subject: [PATCH 06/28] implement mass follow for moderators --- extensions/views/manage.py | 8 +++++--- reviewers/views.py | 3 ++- users/models.py | 2 +- users/signals.py | 33 ++++++++++++++++++++++++++++++++- 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/extensions/views/manage.py b/extensions/views/manage.py index decc7552..36b79fd6 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -399,10 +399,12 @@ class DraftExtensionView( add_preview_formset.save() form.save() # setup initial followers + authors = extension_form.instance.authors.all() moderators = Group.objects.get(name='moderators').user_set.all() - audience = list(moderators) + list(extension_form.instance.authors.all()) - for recipient in audience: - follow(recipient, extension_form.instance, send_action=False) + for recipient in authors: + follow(recipient, extension_form.instance, send_action=False, flag='author') + for recipient in moderators: + follow(recipient, extension_form.instance, send_action=False, flag='moderator') if 'submit_draft' in self.request.POST: action.send( self.request.user, diff --git a/reviewers/views.py b/reviewers/views.py index b8e409cf..4ecb0e2c 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -80,9 +80,10 @@ class ExtensionsApprovalFormView(LoginRequiredMixin, FormView): form.save() self.approve_if_allowed(form) - # automatically follow after ineraction + # automatically follow after an interaction # if a user had unfollowed this extension before, # we unfortunately are making them a follower again + # TODO? set a specific flag? follow(self.request.user, form.instance.extension, send_action=False) activity_type2verb = { diff --git a/users/models.py b/users/models.py index e1c9773d..5feaa392 100644 --- a/users/models.py +++ b/users/models.py @@ -4,7 +4,7 @@ import logging import time from django.contrib.admin.utils import NestedObjects -from django.contrib.auth.models import AbstractUser, Group +from django.contrib.auth.models import AbstractUser from django.db import models, DEFAULT_DB_ALIAS, transaction from django.templatetags.static import static diff --git a/users/signals.py b/users/signals.py index 5528b041..f321684e 100644 --- a/users/signals.py +++ b/users/signals.py @@ -1,14 +1,17 @@ from typing import Dict import logging +from actstream.actions import follow, unfollow from actstream.models import Action, followers from django.contrib.auth import get_user_model -from django.db.models.signals import post_save, pre_save +from django.contrib.auth.models import Group +from django.db.models.signals import m2m_changed, post_save, pre_save from django.dispatch import receiver from blender_id_oauth_client import signals as bid_signals from users.blender_id import BIDSession +from extensions.models import Extension User = get_user_model() bid = BIDSession() @@ -61,3 +64,31 @@ def create_notification( f'create notification for {recipient}: ', f'{instance.actor} {instance.verb} {instance.target}', ) + + +@receiver(m2m_changed, sender=User.groups.through) +def update_moderator_follows(instance, action, model, reverse, pk_set, **kwargs): + """Users becoming moderators should follow all extensions, + and users that stop being moderators should no longer follow all extensions. + The flag='moderator' is used to avoid deleting follow relations that were created in contexts + other than moderator's duties. + """ + moderators = Group.objects.get(name='moderators') + extensions = Extension.objects.all() + users = [] + if model == Group and not reverse: + if moderators.pk not in pk_set: + return + users = [instance] + else: + if instance != moderators: + return + users = User.objects.filter(pk__in=pk_set) + + for user in users: + if action == 'post_remove': + for extension in extensions: + unfollow(user, extension, send_action=False, flag='moderator') + elif action == 'post_add': + for extension in extensions: + follow(user, extension, send_action=False, flag='moderator') -- 2.30.2 From 6cde44ccb03195840015a2e4d1475e9a5dce7e3a Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 12 Apr 2024 14:14:40 +0200 Subject: [PATCH 07/28] stub methods for sending emails about notification batches --- .../management/commands/send_notifications.py | 38 +++++++++++++++++++ users/migrations/0003_notification.py | 26 +++++++++++++ users/models.py | 11 ++++++ users/signals.py | 8 ++-- 4 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 users/management/commands/send_notifications.py create mode 100644 users/migrations/0003_notification.py diff --git a/users/management/commands/send_notifications.py b/users/management/commands/send_notifications.py new file mode 100644 index 00000000..9c7258e3 --- /dev/null +++ b/users/management/commands/send_notifications.py @@ -0,0 +1,38 @@ +"""Send user notifications as emails, at most once delivery.""" +from collections import defaultdict +import logging + +from django.core.management.base import BaseCommand +from django.utils import timezone + +from users.models import Notification + +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + + +class Command(BaseCommand): + def handle(self, *args, **options): # noqa: D102 + logger.info('start, checking for outstanding notifications') + notifications = Notification.objects.filter(processed_by_mailer_at=None) + batched_by_recipient = defaultdict(list) + for n in notifications: + batched_by_recipient[n.recipient].append(n) + + for recipient, batch in batched_by_recipient.items(): + to_send = [] + for n in batch: + n.processed_by_mailer_at = timezone.now() + if recipient.is_subscribed(n): + n.sent = True + to_send.append(n) + # first mark, then send: to avoid spamming in case of a crash-loop + Notification.objects.bulk_update(batch, ['processed_by_mailer_at', 'sent']) + if len(to_send) > 0: + logger.info(f'sending an email to {recipient} about {len(to_send)} notifications') + send_email(recipient, to_send) + logger.info('finish') + + +def send_email(recipient, to_send): + pass diff --git a/users/migrations/0003_notification.py b/users/migrations/0003_notification.py new file mode 100644 index 00000000..4cdfefa8 --- /dev/null +++ b/users/migrations/0003_notification.py @@ -0,0 +1,26 @@ +# Generated by Django 4.2.11 on 2024-04-12 12:06 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('actstream', '0003_add_follow_flag'), + ('users', '0002_moderators_group'), + ] + + operations = [ + migrations.CreateModel( + name='Notification', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('processed_by_mailer_at', models.DateTimeField(default=None, null=True)), + ('sent', models.BooleanField(default=False)), + ('action', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='actstream.action')), + ('recipient', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + ), + ] diff --git a/users/models.py b/users/models.py index 5feaa392..28da32e6 100644 --- a/users/models.py +++ b/users/models.py @@ -3,6 +3,7 @@ from typing import Optional import logging import time +from actstream.models import Action from django.contrib.admin.utils import NestedObjects from django.contrib.auth.models import AbstractUser from django.db import models, DEFAULT_DB_ALIAS, transaction @@ -139,3 +140,13 @@ class User(TrackChangesMixin, AbstractUser): def is_moderator(self): # Used to review and approve extensions return self.groups.filter(name='moderators').exists() + + def is_subscribed(self, notification: 'Notification') -> bool: + return True + + +class Notification(models.Model): + recipient = models.ForeignKey(User, null=False, on_delete=models.CASCADE) + action = models.ForeignKey(Action, null=False, on_delete=models.CASCADE) + processed_by_mailer_at = models.DateTimeField(default=None, null=True) + sent = models.BooleanField(default=False, null=False) diff --git a/users/signals.py b/users/signals.py index f321684e..a5e88555 100644 --- a/users/signals.py +++ b/users/signals.py @@ -10,8 +10,9 @@ from django.dispatch import receiver from blender_id_oauth_client import signals as bid_signals -from users.blender_id import BIDSession from extensions.models import Extension +from users.blender_id import BIDSession +from users.models import Notification User = get_user_model() bid = BIDSession() @@ -60,10 +61,7 @@ def create_notification( audience = filter(lambda f: f != instance.actor, followers(instance.target)) for recipient in audience: - print( - f'create notification for {recipient}: ', - f'{instance.actor} {instance.verb} {instance.target}', - ) + Notification.objects.get_or_create(recipient=recipient, action=instance) @receiver(m2m_changed, sender=User.groups.through) -- 2.30.2 From e4a77c6a9fa8869e684f5fbeaece4c975bc76b3d Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 12 Apr 2024 17:48:24 +0200 Subject: [PATCH 08/28] rudimentary plain text emails --- abuse/apps.py | 2 +- abuse/signals.py | 7 ++-- constants/activity.py | 4 +++ .../management/commands/send_notifications.py | 15 ++++++-- users/models.py | 35 +++++++++++++++++++ 5 files changed, 57 insertions(+), 6 deletions(-) diff --git a/abuse/apps.py b/abuse/apps.py index 08a2c9d3..906dcf88 100644 --- a/abuse/apps.py +++ b/abuse/apps.py @@ -6,7 +6,7 @@ class AbuseConfig(AppConfig): name = 'abuse' def ready(self): - import extensions.signals # noqa: F401 + import abuse.signals # noqa: F401 from actstream import registry registry.register(self.get_model('AbuseReport')) diff --git a/abuse/signals.py b/abuse/signals.py index 119c4bc5..e5ad1a2e 100644 --- a/abuse/signals.py +++ b/abuse/signals.py @@ -20,10 +20,13 @@ def create_action_from_report( sender: object, instance: AbuseReport, created: bool, + raw: bool, **kwargs: object, ) -> None: if not created: return + if raw: + return if instance.type == ABUSE_TYPE_EXTENSION: verb = Verb.REPORTED_EXTENSION @@ -31,9 +34,9 @@ def create_action_from_report( verb = Verb.REPORTED_REVIEW elif instance.type == ABUSE_TYPE_USER: # TODO? - pass + return else: - logger.warning("ignoring an unexpected AbuseReport type={instance.type}") + logger.warning(f'ignoring an unexpected AbuseReport type={instance.type}') return action.send( diff --git a/constants/activity.py b/constants/activity.py index 0697f2d6..594eb083 100644 --- a/constants/activity.py +++ b/constants/activity.py @@ -1,4 +1,8 @@ class Verb: + """These constants are used to dispatch Action records, + changing the values will result in a mismatch with historical values stored in db. + """ + SUBMITTED_FOR_REVIEW = 'submitted for review' REPORTED_EXTENSION = 'reported extension' diff --git a/users/management/commands/send_notifications.py b/users/management/commands/send_notifications.py index 9c7258e3..7da85883 100644 --- a/users/management/commands/send_notifications.py +++ b/users/management/commands/send_notifications.py @@ -2,6 +2,8 @@ from collections import defaultdict import logging +from django.conf import settings +from django.core.mail import send_mail from django.core.management.base import BaseCommand from django.utils import timezone @@ -30,9 +32,16 @@ class Command(BaseCommand): Notification.objects.bulk_update(batch, ['processed_by_mailer_at', 'sent']) if len(to_send) > 0: logger.info(f'sending an email to {recipient} about {len(to_send)} notifications') - send_email(recipient, to_send) + send_batch_notification_email(recipient, to_send) logger.info('finish') -def send_email(recipient, to_send): - pass +def send_batch_notification_email(recipient, notifications): + subject = 'New activity' + message = '\n\n'.join([n.format_email_txt() for n in notifications]) + send_mail( + subject, + message, + settings.DEFAULT_FROM_EMAIL, + [recipient.email], + ) diff --git a/users/models.py b/users/models.py index 28da32e6..b49ba1fa 100644 --- a/users/models.py +++ b/users/models.py @@ -6,9 +6,11 @@ import time from actstream.models import Action from django.contrib.admin.utils import NestedObjects from django.contrib.auth.models import AbstractUser +from django.contrib.sites.models import Site from django.db import models, DEFAULT_DB_ALIAS, transaction from django.templatetags.static import static +from constants.activity import Verb from common.model_mixins import TrackChangesMixin from files.utils import get_sha256_from_value @@ -142,11 +144,44 @@ class User(TrackChangesMixin, AbstractUser): return self.groups.filter(name='moderators').exists() def is_subscribed(self, notification: 'Notification') -> bool: + # TODO user profile settings to subscribe to groups of action verbs return True class Notification(models.Model): + """Notification records are created in Action's post_save signal. + When a user marks a notification as read, a record is deleted. + While a notification exists it can be picked up by a send_notifications management command + that runs periodically in background. + If a recipient opted out from receiving notifications of particular type (based on action.verb), + we shouldn't include it in the email, leaving sent=False. + """ + recipient = models.ForeignKey(User, null=False, on_delete=models.CASCADE) action = models.ForeignKey(Action, null=False, on_delete=models.CASCADE) processed_by_mailer_at = models.DateTimeField(default=None, null=True) sent = models.BooleanField(default=False, null=False) + + def format_email_txt(self): + url = self.get_absolute_url() + # TODO construct a proper phrase, depending on the verb, maybe use a template + return f'{self.action.actor.full_name} {self.action.verb} {self.action.target}: {url}' + + def get_absolute_url(self): + if self.action.verb == Verb.RATED_EXTENSION: + url = self.action.target.get_ratings_url() + elif self.action.verb in [ + Verb.APPROVED, + Verb.COMMENTED, + Verb.REQUESTED_CHANGES, + Verb.REQUESTED_REVIEW, + ]: + url = self.action.target.get_review_url() + elif self.action.action_object is not None: + url = self.action.action_object.get_absolute_url() + else: + url = self.action.target.get_absolute_url() + + # TODO? url cloacking to auto-delete visited notifications + domain = Site.objects.get_current().domain + return f'https://{domain}{url}' -- 2.30.2 From 6f30e145cd18ffbbd9d770304fec7af9ec975cc5 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 12 Apr 2024 19:50:07 +0200 Subject: [PATCH 09/28] signals refactoring --- abuse/apps.py | 2 +- abuse/signals.py | 2 +- blender_extensions/settings.py | 1 + extensions/apps.py | 2 +- extensions/signals.py | 18 ++++++++ extensions/views/manage.py | 9 ---- notifications/__init.py__ | 0 notifications/apps.py | 9 ++++ notifications/models.py | 46 +++++++++++++++++++ notifications/signals.py | 35 ++++++++++++++ ratings/apps.py | 2 +- ratings/signals.py | 2 +- reviewers/apps.py | 1 + reviewers/signals.py | 39 ++++++++++++++++ reviewers/views.py | 23 ---------- users/apps.py | 2 +- .../management/commands/send_notifications.py | 8 ++-- users/models.py | 46 ------------------- users/signals.py | 26 +---------- 19 files changed, 160 insertions(+), 113 deletions(-) create mode 100644 notifications/__init.py__ create mode 100644 notifications/apps.py create mode 100644 notifications/models.py create mode 100644 notifications/signals.py create mode 100644 reviewers/signals.py diff --git a/abuse/apps.py b/abuse/apps.py index 906dcf88..c775109d 100644 --- a/abuse/apps.py +++ b/abuse/apps.py @@ -6,7 +6,7 @@ class AbuseConfig(AppConfig): name = 'abuse' def ready(self): - import abuse.signals # noqa: F401 from actstream import registry + import abuse.signals # noqa: F401 registry.register(self.get_model('AbuseReport')) diff --git a/abuse/signals.py b/abuse/signals.py index e5ad1a2e..a305ad3d 100644 --- a/abuse/signals.py +++ b/abuse/signals.py @@ -16,7 +16,7 @@ logger = logging.getLogger(__name__) @receiver(post_save, sender=AbuseReport) -def create_action_from_report( +def _create_action_from_report( sender: object, instance: AbuseReport, created: bool, diff --git a/blender_extensions/settings.py b/blender_extensions/settings.py index 1bcb8d89..0c3dfae7 100644 --- a/blender_extensions/settings.py +++ b/blender_extensions/settings.py @@ -54,6 +54,7 @@ INSTALLED_APPS = [ 'common', 'files', 'loginas', + 'notifications', 'pipeline', 'ratings', 'rangefilter', diff --git a/extensions/apps.py b/extensions/apps.py index d51ca42a..4bec7d8c 100644 --- a/extensions/apps.py +++ b/extensions/apps.py @@ -6,7 +6,7 @@ class ExtensionsConfig(AppConfig): name = 'extensions' def ready(self): - import extensions.signals # noqa: F401 from actstream import registry + import extensions.signals # noqa: F401 registry.register(self.get_model('Extension')) diff --git a/extensions/signals.py b/extensions/signals.py index d4a3bbaf..d70eb0fb 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -1,5 +1,7 @@ from typing import Union +from actstream.actions import follow +from django.contrib.auth.models import Group from django.db.models.signals import pre_save, post_save, post_delete from django.dispatch import receiver @@ -76,3 +78,19 @@ def _set_is_listed( extension.is_listed = new_is_listed extension.save() + + +@receiver(post_save, sender=extensions.models.Extension) +def _setup_followers( + sender: object, + instance: extensions.models.Extension, + created: bool, + **kwargs: object, +) -> None: + if not created: + return + + for user in instance.authors.all(): + follow(user, instance, send_action=False, flag='author') + for user in Group.objects.get(name='moderators').user_set.all(): + follow(user, instance, send_action=False, flag='moderator') diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 36b79fd6..e549a1a5 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -1,7 +1,5 @@ """Contains views allowing developers to manage their add-ons.""" from actstream import action -from actstream.actions import follow -from django.contrib.auth.models import Group from django import forms from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin from django.contrib.messages.views import SuccessMessageMixin @@ -398,13 +396,6 @@ class DraftExtensionView( extension_form.save() add_preview_formset.save() form.save() - # setup initial followers - authors = extension_form.instance.authors.all() - moderators = Group.objects.get(name='moderators').user_set.all() - for recipient in authors: - follow(recipient, extension_form.instance, send_action=False, flag='author') - for recipient in moderators: - follow(recipient, extension_form.instance, send_action=False, flag='moderator') if 'submit_draft' in self.request.POST: action.send( self.request.user, diff --git a/notifications/__init.py__ b/notifications/__init.py__ new file mode 100644 index 00000000..e69de29b diff --git a/notifications/apps.py b/notifications/apps.py new file mode 100644 index 00000000..3d6987ef --- /dev/null +++ b/notifications/apps.py @@ -0,0 +1,9 @@ +from django.apps import AppConfig + + +class NotificationsConfig(AppConfig): + default_auto_field = 'django.db.models.BigAutoField' + name = 'notifications' + + def ready(self): + import notifications.signals # noqa: F401 diff --git a/notifications/models.py b/notifications/models.py new file mode 100644 index 00000000..a4ee3d6e --- /dev/null +++ b/notifications/models.py @@ -0,0 +1,46 @@ +from actstream.models import Action +from django.contrib.auth import get_user_model +from django.db import models + +from constants.activity import Verb +from common.templatetags.common import absolutify + +User = get_user_model() + + +class Notification(models.Model): + """Notification records are created in Action's post_save signal. + When a user marks a notification as read, a record is deleted. + While a notification exists it can be picked up by a send_notifications management command + that runs periodically in background. + If a recipient opted out from receiving notifications of particular type (based on action.verb), + we shouldn't include it in the email, leaving sent=False. + """ + + recipient = models.ForeignKey(User, null=False, on_delete=models.CASCADE) + action = models.ForeignKey(Action, null=False, on_delete=models.CASCADE) + processed_by_mailer_at = models.DateTimeField(default=None, null=True) + sent = models.BooleanField(default=False, null=False) + + def format_email_txt(self): + url = self.get_absolute_url() + # TODO construct a proper phrase, depending on the verb, maybe use a template + return f'{self.action.actor.full_name} {self.action.verb} {self.action.target}: {url}' + + def get_absolute_url(self): + if self.action.verb == Verb.RATED_EXTENSION: + url = self.action.target.get_ratings_url() + elif self.action.verb in [ + Verb.APPROVED, + Verb.COMMENTED, + Verb.REQUESTED_CHANGES, + Verb.REQUESTED_REVIEW, + ]: + url = self.action.target.get_review_url() + elif self.action.action_object is not None: + url = self.action.action_object.get_absolute_url() + else: + url = self.action.target.get_absolute_url() + + # TODO? url cloacking to auto-delete visited notifications + return absolutify(url) diff --git a/notifications/signals.py b/notifications/signals.py new file mode 100644 index 00000000..64ffafe8 --- /dev/null +++ b/notifications/signals.py @@ -0,0 +1,35 @@ +import logging + +from actstream.models import Action, followers +from django.db.models.signals import post_save +from django.dispatch import receiver + +from notifications.models import Notification + +logger = logging.getLogger(__name__) + + +@receiver(post_save, sender=Action) +def _create_notifications( + sender: object, + instance: Action, + created: bool, + raw: bool, + **kwargs: object, +) -> None: + if raw: + return + if not created: + return + + if not instance.target: + logger.warning('ignoring an unexpected Action without a target, verb={instance.verb}') + return + + notifications = [] + for recipient in followers(instance.target): + if recipient == instance.actor: + continue + notifications.append(Notification(recipient=recipient, action=instance)) + if len(notifications) > 0: + Notification.objects.bulk_create(notifications) diff --git a/ratings/apps.py b/ratings/apps.py index 13b958e8..f1849f97 100644 --- a/ratings/apps.py +++ b/ratings/apps.py @@ -6,7 +6,7 @@ class RatingsConfig(AppConfig): name = 'ratings' def ready(self): - import ratings.signals # noqa: F401 from actstream import registry + import ratings.signals # noqa: F401 registry.register(self.get_model('Rating')) diff --git a/ratings/signals.py b/ratings/signals.py index 4a5071cd..5ff6c271 100644 --- a/ratings/signals.py +++ b/ratings/signals.py @@ -22,7 +22,7 @@ def _update_rating_counters(sender, instance, *args, **kwargs): @receiver(post_save, sender=Rating) -def create_action_from_rating( +def _create_action_from_rating( sender: object, instance: Rating, created: bool, diff --git a/reviewers/apps.py b/reviewers/apps.py index fe959188..0ddcb5b3 100644 --- a/reviewers/apps.py +++ b/reviewers/apps.py @@ -7,5 +7,6 @@ class ReviewersConfig(AppConfig): def ready(self) -> None: from actstream import registry + import reviewers.signals # noqa: F401 registry.register(self.get_model('ApprovalActivity')) diff --git a/reviewers/signals.py b/reviewers/signals.py new file mode 100644 index 00000000..6bd91e07 --- /dev/null +++ b/reviewers/signals.py @@ -0,0 +1,39 @@ +from actstream import action +from actstream.actions import follow +from django.db.models.signals import post_save +from django.dispatch import receiver + +from constants.activity import Verb +from reviewers.models import ApprovalActivity + + +@receiver(post_save, sender=ApprovalActivity) +def _create_action_from_review_and_follow( + sender: object, + instance: ApprovalActivity, + created: bool, + raw: bool, + **kwargs: object, +) -> None: + if raw: + return + if not created: + return + + # automatically follow after an interaction + # if a user had unfollowed this extension before, + # we are making them a follower again + follow(instance.user, instance.extension, send_action=False) + + activity_type2verb = { + ApprovalActivity.ActivityType.APPROVED: Verb.APPROVED, + ApprovalActivity.ActivityType.AWAITING_CHANGES: Verb.REQUESTED_CHANGES, + ApprovalActivity.ActivityType.AWAITING_REVIEW: Verb.REQUESTED_REVIEW, + ApprovalActivity.ActivityType.COMMENT: Verb.COMMENTED, + } + action.send( + instance.user, + verb=activity_type2verb.get(instance.type), + action_object=instance, + target=instance.extension, + ) diff --git a/reviewers/views.py b/reviewers/views.py index 4ecb0e2c..fb2668e0 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -1,13 +1,10 @@ import logging -from actstream import action -from actstream.actions import follow 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 -from constants.activity import Verb from files.models import File from extensions.models import Extension from reviewers.forms import CommentForm @@ -79,24 +76,4 @@ class ExtensionsApprovalFormView(LoginRequiredMixin, FormView): form.instance.extension = Extension.objects.get(slug=self.kwargs['slug']) form.save() self.approve_if_allowed(form) - - # automatically follow after an interaction - # if a user had unfollowed this extension before, - # we unfortunately are making them a follower again - # TODO? set a specific flag? - follow(self.request.user, form.instance.extension, send_action=False) - - activity_type2verb = { - ApprovalActivity.ActivityType.APPROVED: Verb.APPROVED, - ApprovalActivity.ActivityType.AWAITING_CHANGES: Verb.REQUESTED_CHANGES, - ApprovalActivity.ActivityType.AWAITING_REVIEW: Verb.REQUESTED_REVIEW, - ApprovalActivity.ActivityType.COMMENT: Verb.COMMENTED, - } - action.send( - self.request.user, - verb=activity_type2verb.get(form.cleaned_data['type']), - action_object=form.instance, - target=form.instance.extension, - ) - return super().form_valid(form) diff --git a/users/apps.py b/users/apps.py index e50ae5c0..fa628092 100644 --- a/users/apps.py +++ b/users/apps.py @@ -6,8 +6,8 @@ class UsersConfig(AppConfig): verbose_name = 'Authentication and authorization' def ready(self) -> None: - import users.signals # noqa: F401 from actstream import registry from django.contrib.auth import get_user_model + import users.signals # noqa: F401 registry.register(get_user_model()) diff --git a/users/management/commands/send_notifications.py b/users/management/commands/send_notifications.py index 7da85883..78ce651c 100644 --- a/users/management/commands/send_notifications.py +++ b/users/management/commands/send_notifications.py @@ -7,7 +7,7 @@ from django.core.mail import send_mail from django.core.management.base import BaseCommand from django.utils import timezone -from users.models import Notification +from notifications.models import Notification logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) @@ -25,9 +25,9 @@ class Command(BaseCommand): to_send = [] for n in batch: n.processed_by_mailer_at = timezone.now() - if recipient.is_subscribed(n): - n.sent = True - to_send.append(n) + # TODO check some form of recipient.is_subscribed(n): + n.sent = True + to_send.append(n) # first mark, then send: to avoid spamming in case of a crash-loop Notification.objects.bulk_update(batch, ['processed_by_mailer_at', 'sent']) if len(to_send) > 0: diff --git a/users/models.py b/users/models.py index b49ba1fa..5feaa392 100644 --- a/users/models.py +++ b/users/models.py @@ -3,14 +3,11 @@ from typing import Optional import logging import time -from actstream.models import Action from django.contrib.admin.utils import NestedObjects from django.contrib.auth.models import AbstractUser -from django.contrib.sites.models import Site from django.db import models, DEFAULT_DB_ALIAS, transaction from django.templatetags.static import static -from constants.activity import Verb from common.model_mixins import TrackChangesMixin from files.utils import get_sha256_from_value @@ -142,46 +139,3 @@ class User(TrackChangesMixin, AbstractUser): def is_moderator(self): # Used to review and approve extensions return self.groups.filter(name='moderators').exists() - - def is_subscribed(self, notification: 'Notification') -> bool: - # TODO user profile settings to subscribe to groups of action verbs - return True - - -class Notification(models.Model): - """Notification records are created in Action's post_save signal. - When a user marks a notification as read, a record is deleted. - While a notification exists it can be picked up by a send_notifications management command - that runs periodically in background. - If a recipient opted out from receiving notifications of particular type (based on action.verb), - we shouldn't include it in the email, leaving sent=False. - """ - - recipient = models.ForeignKey(User, null=False, on_delete=models.CASCADE) - action = models.ForeignKey(Action, null=False, on_delete=models.CASCADE) - processed_by_mailer_at = models.DateTimeField(default=None, null=True) - sent = models.BooleanField(default=False, null=False) - - def format_email_txt(self): - url = self.get_absolute_url() - # TODO construct a proper phrase, depending on the verb, maybe use a template - return f'{self.action.actor.full_name} {self.action.verb} {self.action.target}: {url}' - - def get_absolute_url(self): - if self.action.verb == Verb.RATED_EXTENSION: - url = self.action.target.get_ratings_url() - elif self.action.verb in [ - Verb.APPROVED, - Verb.COMMENTED, - Verb.REQUESTED_CHANGES, - Verb.REQUESTED_REVIEW, - ]: - url = self.action.target.get_review_url() - elif self.action.action_object is not None: - url = self.action.action_object.get_absolute_url() - else: - url = self.action.target.get_absolute_url() - - # TODO? url cloacking to auto-delete visited notifications - domain = Site.objects.get_current().domain - return f'https://{domain}{url}' diff --git a/users/signals.py b/users/signals.py index a5e88555..b5d6ebbd 100644 --- a/users/signals.py +++ b/users/signals.py @@ -2,17 +2,15 @@ from typing import Dict import logging from actstream.actions import follow, unfollow -from actstream.models import Action, followers from django.contrib.auth import get_user_model from django.contrib.auth.models import Group -from django.db.models.signals import m2m_changed, post_save, pre_save +from django.db.models.signals import m2m_changed, pre_save from django.dispatch import receiver from blender_id_oauth_client import signals as bid_signals from extensions.models import Extension from users.blender_id import BIDSession -from users.models import Notification User = get_user_model() bid = BIDSession() @@ -42,28 +40,6 @@ def update_user( bid.copy_badges_from_blender_id(user=instance) -@receiver(post_save, sender=Action) -def create_notification( - sender: object, - instance: Action, - created: bool, - raw: bool, - **kwargs: object, -) -> None: - if raw: - return - if not created: - return - - if not instance.target: - logger.warning('ignoring an unexpected Action without a target, verb={instance.verb}') - return - - audience = filter(lambda f: f != instance.actor, followers(instance.target)) - for recipient in audience: - Notification.objects.get_or_create(recipient=recipient, action=instance) - - @receiver(m2m_changed, sender=User.groups.through) def update_moderator_follows(instance, action, model, reverse, pk_set, **kwargs): """Users becoming moderators should follow all extensions, -- 2.30.2 From 402bfd8cdccbdfac59b8074d634ffa0c35a1c74b Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 12 Apr 2024 20:10:43 +0200 Subject: [PATCH 10/28] add unique index for Notification(recipient, action) --- extensions/views/manage.py | 3 +++ notifications/models.py | 3 +++ users/management/commands/send_notifications.py | 2 +- users/migrations/0004_delete_notification.py | 16 ++++++++++++++++ 4 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 users/migrations/0004_delete_notification.py diff --git a/extensions/views/manage.py b/extensions/views/manage.py index e549a1a5..fea37ed6 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -397,6 +397,9 @@ class DraftExtensionView( add_preview_formset.save() form.save() if 'submit_draft' in self.request.POST: + # in the signal context we won't know the real user doing that, + # so keeping this in the view code for now + # alternatively (TODO?) this could be turned into an ApprovalActivity record action.send( self.request.user, verb=Verb.SUBMITTED_FOR_REVIEW, diff --git a/notifications/models.py b/notifications/models.py index a4ee3d6e..7aa7d7a4 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -22,6 +22,9 @@ class Notification(models.Model): processed_by_mailer_at = models.DateTimeField(default=None, null=True) sent = models.BooleanField(default=False, null=False) + class Meta: + unique_together = ['recipient', 'action'] + def format_email_txt(self): url = self.get_absolute_url() # TODO construct a proper phrase, depending on the verb, maybe use a template diff --git a/users/management/commands/send_notifications.py b/users/management/commands/send_notifications.py index 78ce651c..05693abc 100644 --- a/users/management/commands/send_notifications.py +++ b/users/management/commands/send_notifications.py @@ -28,7 +28,7 @@ class Command(BaseCommand): # TODO check some form of recipient.is_subscribed(n): n.sent = True to_send.append(n) - # first mark, then send: to avoid spamming in case of a crash-loop + # first mark as processed, then send: avoid spamming in case of a crash-loop Notification.objects.bulk_update(batch, ['processed_by_mailer_at', 'sent']) if len(to_send) > 0: logger.info(f'sending an email to {recipient} about {len(to_send)} notifications') diff --git a/users/migrations/0004_delete_notification.py b/users/migrations/0004_delete_notification.py new file mode 100644 index 00000000..d71a0235 --- /dev/null +++ b/users/migrations/0004_delete_notification.py @@ -0,0 +1,16 @@ +# Generated by Django 4.2.11 on 2024-04-12 18:09 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0003_notification'), + ] + + operations = [ + migrations.DeleteModel( + name='Notification', + ), + ] -- 2.30.2 From 0d38deb7d2a75778502f0f9cd9c567a646df180f Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 12 Apr 2024 20:12:02 +0200 Subject: [PATCH 11/28] remove migrations --- users/migrations/0003_notification.py | 26 -------------------- users/migrations/0004_delete_notification.py | 16 ------------ 2 files changed, 42 deletions(-) delete mode 100644 users/migrations/0003_notification.py delete mode 100644 users/migrations/0004_delete_notification.py diff --git a/users/migrations/0003_notification.py b/users/migrations/0003_notification.py deleted file mode 100644 index 4cdfefa8..00000000 --- a/users/migrations/0003_notification.py +++ /dev/null @@ -1,26 +0,0 @@ -# Generated by Django 4.2.11 on 2024-04-12 12:06 - -from django.conf import settings -from django.db import migrations, models -import django.db.models.deletion - - -class Migration(migrations.Migration): - - dependencies = [ - ('actstream', '0003_add_follow_flag'), - ('users', '0002_moderators_group'), - ] - - operations = [ - migrations.CreateModel( - name='Notification', - fields=[ - ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('processed_by_mailer_at', models.DateTimeField(default=None, null=True)), - ('sent', models.BooleanField(default=False)), - ('action', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='actstream.action')), - ('recipient', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), - ], - ), - ] diff --git a/users/migrations/0004_delete_notification.py b/users/migrations/0004_delete_notification.py deleted file mode 100644 index d71a0235..00000000 --- a/users/migrations/0004_delete_notification.py +++ /dev/null @@ -1,16 +0,0 @@ -# Generated by Django 4.2.11 on 2024-04-12 18:09 - -from django.db import migrations - - -class Migration(migrations.Migration): - - dependencies = [ - ('users', '0003_notification'), - ] - - operations = [ - migrations.DeleteModel( - name='Notification', - ), - ] -- 2.30.2 From 185fedf1ea9f49a6d3055f0c22f7bc1f1479ec1e Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 12 Apr 2024 20:16:00 +0200 Subject: [PATCH 12/28] add migration --- notifications/migrations/0001_initial.py | 31 ++++++++++++++++++++++++ notifications/migrations/__init__.py | 0 notifications/models.py | 2 +- 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 notifications/migrations/0001_initial.py create mode 100644 notifications/migrations/__init__.py diff --git a/notifications/migrations/0001_initial.py b/notifications/migrations/0001_initial.py new file mode 100644 index 00000000..9e4ee35c --- /dev/null +++ b/notifications/migrations/0001_initial.py @@ -0,0 +1,31 @@ +# Generated by Django 4.2.11 on 2024-04-12 18:14 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ('actstream', '0003_add_follow_flag'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='Notification', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('recipient', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ('action', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='actstream.action')), + ('sent', models.BooleanField(default=False)), + ('processed_by_mailer_at', models.DateTimeField(default=None, null=True)), + ], + options={ + 'unique_together': {('recipient', 'action')}, + }, + ), + ] diff --git a/notifications/migrations/__init__.py b/notifications/migrations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/notifications/models.py b/notifications/models.py index 7aa7d7a4..eb8d83ab 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -19,8 +19,8 @@ class Notification(models.Model): recipient = models.ForeignKey(User, null=False, on_delete=models.CASCADE) action = models.ForeignKey(Action, null=False, on_delete=models.CASCADE) - processed_by_mailer_at = models.DateTimeField(default=None, null=True) sent = models.BooleanField(default=False, null=False) + processed_by_mailer_at = models.DateTimeField(default=None, null=True) class Meta: unique_together = ['recipient', 'action'] -- 2.30.2 From ec61798c03ecf4933899aa5f7a440e9138135654 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 15 Apr 2024 14:06:46 +0200 Subject: [PATCH 13/28] explicit notification dispatching using VERB2FLAGS --- constants/activity.py | 15 +++++--- extensions/signals.py | 5 ++- .../migrations/0002_setup_followers.py | 38 +++++++++++++++++++ notifications/signals.py | 29 ++++++++++++-- reviewers/signals.py | 4 +- users/signals.py | 7 ++-- 6 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 notifications/migrations/0002_setup_followers.py diff --git a/constants/activity.py b/constants/activity.py index 594eb083..834e6b61 100644 --- a/constants/activity.py +++ b/constants/activity.py @@ -3,14 +3,17 @@ class Verb: changing the values will result in a mismatch with historical values stored in db. """ - SUBMITTED_FOR_REVIEW = 'submitted for review' - - REPORTED_EXTENSION = 'reported extension' - REPORTED_REVIEW = 'reported review' - APPROVED = 'approved' COMMENTED = 'commented' + RATED_EXTENSION = 'rated extension' + REPORTED_EXTENSION = 'reported extension' + REPORTED_REVIEW = 'reported review' REQUESTED_CHANGES = 'requested changes' REQUESTED_REVIEW = 'requested review' + SUBMITTED_FOR_REVIEW = 'submitted for review' - RATED_EXTENSION = 'rated extension' + +class Flag: + AUTHOR = 'author' + MODERATOR = 'moderator' + REVIEWER = 'reviewer' diff --git a/extensions/signals.py b/extensions/signals.py index d70eb0fb..53ef4178 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -5,6 +5,7 @@ from django.contrib.auth.models import Group from django.db.models.signals import pre_save, post_save, post_delete from django.dispatch import receiver +from constants.activity import Flag import extensions.models import extensions.tasks import files.models @@ -91,6 +92,6 @@ def _setup_followers( return for user in instance.authors.all(): - follow(user, instance, send_action=False, flag='author') + follow(user, instance, send_action=False, flag=Flag.AUTHOR) for user in Group.objects.get(name='moderators').user_set.all(): - follow(user, instance, send_action=False, flag='moderator') + follow(user, instance, send_action=False, flag=Flag.MODERATOR) diff --git a/notifications/migrations/0002_setup_followers.py b/notifications/migrations/0002_setup_followers.py new file mode 100644 index 00000000..97d63e2b --- /dev/null +++ b/notifications/migrations/0002_setup_followers.py @@ -0,0 +1,38 @@ +# Generated by Django 4.2.11 on 2024-04-15 10:18 + +from actstream.actions import follow +from django.contrib.auth.models import Group +from django.db import migrations + +from constants.activity import Flag + + +def setup_followers(apps, schema_editor): + # !!! not using apps.get_model('extensions', 'Extension') + # because it doesn't work with actstream.registry: + # + # File ".../site-packages/actstream/actions.py", line 34, in follow + # check(obj) + # File ".../site-packages/actstream/registry.py", line 105, in check + # raise ImproperlyConfigured( + # django.core.exceptions.ImproperlyConfigured: The model Extension is not registered. + # Please use actstream.registry to register it. + # + # if this ever causes issues in the future, delete this code or find a workaround + from extensions.models import Extension + for extension in Extension.objects.all(): + for user in extension.authors.all(): + follow(user, extension, send_action=False, flag=Flag.AUTHOR) + for user in Group.objects.get(name='moderators').user_set.all(): + follow(user, extension, send_action=False, flag=Flag.MODERATOR) + + +class Migration(migrations.Migration): + + dependencies = [ + ('notifications', '0001_initial'), + ] + + operations = [ + migrations.RunPython(setup_followers, reverse_code=migrations.RunPython.noop), + ] diff --git a/notifications/signals.py b/notifications/signals.py index 64ffafe8..72e5c71d 100644 --- a/notifications/signals.py +++ b/notifications/signals.py @@ -1,13 +1,26 @@ import logging -from actstream.models import Action, followers +from actstream.models import Action, Follow +from django.contrib.auth import get_user_model from django.db.models.signals import post_save from django.dispatch import receiver +from constants.activity import Flag, Verb from notifications.models import Notification logger = logging.getLogger(__name__) +VERB2FLAGS = { + Verb.APPROVED: [Flag.AUTHOR, Flag.MODERATOR, Flag.REVIEWER], + Verb.COMMENTED: [Flag.AUTHOR, Flag.MODERATOR, Flag.REVIEWER], + Verb.RATED_EXTENSION: [Flag.AUTHOR], + Verb.REPORTED_EXTENSION: [Flag.MODERATOR], + Verb.REPORTED_REVIEW: [Flag.MODERATOR], + Verb.REQUESTED_CHANGES: [Flag.AUTHOR, Flag.MODERATOR, Flag.REVIEWER], + Verb.REQUESTED_REVIEW: [Flag.MODERATOR, Flag.REVIEWER], + Verb.SUBMITTED_FOR_REVIEW: [Flag.MODERATOR], +} + @receiver(post_save, sender=Action) def _create_notifications( @@ -23,11 +36,21 @@ def _create_notifications( return if not instance.target: - logger.warning('ignoring an unexpected Action without a target, verb={instance.verb}') + logger.warning(f'ignoring an unexpected Action without a target, verb={instance.verb}') return notifications = [] - for recipient in followers(instance.target): + + flags = VERB2FLAGS.get(instance.verb, None) + if not flags: + logger.warning(f'no follower flags for verb={instance.verb}, nobody will be notified') + return + + followers = Follow.objects.for_object(instance.target).filter(flag__in=flags) + user_ids = followers.values_list('user', flat=True) + followers = get_user_model().objects.filter(id__in=user_ids) + + for recipient in followers: if recipient == instance.actor: continue notifications.append(Notification(recipient=recipient, action=instance)) diff --git a/reviewers/signals.py b/reviewers/signals.py index 6bd91e07..4e143c05 100644 --- a/reviewers/signals.py +++ b/reviewers/signals.py @@ -3,7 +3,7 @@ from actstream.actions import follow from django.db.models.signals import post_save from django.dispatch import receiver -from constants.activity import Verb +from constants.activity import Flag, Verb from reviewers.models import ApprovalActivity @@ -23,7 +23,7 @@ def _create_action_from_review_and_follow( # automatically follow after an interaction # if a user had unfollowed this extension before, # we are making them a follower again - follow(instance.user, instance.extension, send_action=False) + follow(instance.user, instance.extension, send_action=False, flag=Flag.REVIEWER) activity_type2verb = { ApprovalActivity.ActivityType.APPROVED: Verb.APPROVED, diff --git a/users/signals.py b/users/signals.py index b5d6ebbd..5887bc21 100644 --- a/users/signals.py +++ b/users/signals.py @@ -9,6 +9,7 @@ from django.dispatch import receiver from blender_id_oauth_client import signals as bid_signals +from constants.activity import Flag from extensions.models import Extension from users.blender_id import BIDSession @@ -44,7 +45,7 @@ def update_user( def update_moderator_follows(instance, action, model, reverse, pk_set, **kwargs): """Users becoming moderators should follow all extensions, and users that stop being moderators should no longer follow all extensions. - The flag='moderator' is used to avoid deleting follow relations that were created in contexts + The flag=Flag.MODERATOR is used to avoid deleting follow relations that were created in contexts other than moderator's duties. """ moderators = Group.objects.get(name='moderators') @@ -62,7 +63,7 @@ def update_moderator_follows(instance, action, model, reverse, pk_set, **kwargs) for user in users: if action == 'post_remove': for extension in extensions: - unfollow(user, extension, send_action=False, flag='moderator') + unfollow(user, extension, send_action=False, flag=Flag.MODERATOR) elif action == 'post_add': for extension in extensions: - follow(user, extension, send_action=False, flag='moderator') + follow(user, extension, send_action=False, flag=Flag.MODERATOR) -- 2.30.2 From 4de74cfa0dd593782720cfc806c96b7c632b9938 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 15 Apr 2024 16:53:57 +0200 Subject: [PATCH 14/28] implement unsubscribe --- users/admin.py | 10 ++++++++- users/forms.py | 5 +++++ .../management/commands/send_notifications.py | 6 +++++- ...er_is_subscribed_to_notification_emails.py | 18 ++++++++++++++++ users/models.py | 3 +++ users/templates/users/settings/profile.html | 21 +++++++++++++++++++ users/urls.py | 5 +++++ users/views/settings.py | 21 +++++++++++++++++++ 8 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 users/forms.py create mode 100644 users/migrations/0003_user_is_subscribed_to_notification_emails.py diff --git a/users/admin.py b/users/admin.py index 8e94fcdf..a7e8bb06 100644 --- a/users/admin.py +++ b/users/admin.py @@ -26,7 +26,15 @@ class UserAdmin(auth_admin.UserAdmin): (None, {'fields': ('username', 'password')}), ( _('Personal info'), - {'fields': ('full_name', 'image', 'email', 'badges')}, + { + 'fields': ( + 'full_name', + 'image', + 'email', + 'badges', + 'is_subscribed_to_notification_emails', + ) + }, ), ( _('Permissions'), diff --git a/users/forms.py b/users/forms.py new file mode 100644 index 00000000..7c1da727 --- /dev/null +++ b/users/forms.py @@ -0,0 +1,5 @@ +from django import forms + + +class SubscribeNotificationEmailsForm(forms.Form): + subscribe = forms.BooleanField(widget=forms.HiddenInput(), required=False) diff --git a/users/management/commands/send_notifications.py b/users/management/commands/send_notifications.py index 05693abc..27fa9988 100644 --- a/users/management/commands/send_notifications.py +++ b/users/management/commands/send_notifications.py @@ -25,7 +25,11 @@ class Command(BaseCommand): to_send = [] for n in batch: n.processed_by_mailer_at = timezone.now() - # TODO check some form of recipient.is_subscribed(n): + if not recipient.is_subscribed_to_notification_emails: + continue + # check that email is confirmed to avoid spamming unsuspecting email owners + if recipient.confirmed_email_at is None: + continue n.sent = True to_send.append(n) # first mark as processed, then send: avoid spamming in case of a crash-loop diff --git a/users/migrations/0003_user_is_subscribed_to_notification_emails.py b/users/migrations/0003_user_is_subscribed_to_notification_emails.py new file mode 100644 index 00000000..61cc1a31 --- /dev/null +++ b/users/migrations/0003_user_is_subscribed_to_notification_emails.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.11 on 2024-04-15 12:53 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0002_moderators_group'), + ] + + operations = [ + migrations.AddField( + model_name='user', + name='is_subscribed_to_notification_emails', + field=models.BooleanField(default=True), + ), + ] diff --git a/users/models.py b/users/models.py index 5feaa392..464e5dd3 100644 --- a/users/models.py +++ b/users/models.py @@ -36,6 +36,7 @@ class User(TrackChangesMixin, AbstractUser): 'confirmed_email_at', 'full_name', 'email', + 'is_subscribed_to_notification_emails', } class Meta: @@ -49,6 +50,8 @@ class User(TrackChangesMixin, AbstractUser): date_deletion_requested = models.DateTimeField(null=True, blank=True) confirmed_email_at = models.DateTimeField(null=True, blank=True) + is_subscribed_to_notification_emails = models.BooleanField(null=False, default=True) + def __str__(self) -> str: return f'{self.full_name or self.username}' diff --git a/users/templates/users/settings/profile.html b/users/templates/users/settings/profile.html index 2f08fa73..9d2fefa6 100644 --- a/users/templates/users/settings/profile.html +++ b/users/templates/users/settings/profile.html @@ -117,4 +117,25 @@ +

Notifications

+
+
+
+
+ {% csrf_token %} + {{ subscribe_notification_emails_form }} + {% if user.is_subscribed_to_notification_emails %} + You are subscribed to notification emails. + + {% if not user.confirmed_email_at %} +

Your need to confirm your email to receive notification emails.

+ {% endif %} + {% else %} + You are not subscribed to notification emails. + + {% endif %} +
+
+
+
{% endblock settings %} diff --git a/users/urls.py b/users/urls.py index c398e8f4..3f54f4eb 100644 --- a/users/urls.py +++ b/users/urls.py @@ -11,6 +11,11 @@ urlpatterns = [ include( [ path('profile/', settings.ProfileView.as_view(), name='my-profile'), + path( + 'profile/subscribe-notification-emails/', + settings.SubscribeNotificationEmailsView.as_view(), + name='subscribe-notification-emails', + ), path('delete/', settings.DeleteView.as_view(), name='my-profile-delete'), ] ), diff --git a/users/views/settings.py b/users/views/settings.py index e2bee9b0..baaa1f83 100644 --- a/users/views/settings.py +++ b/users/views/settings.py @@ -1,7 +1,11 @@ """User profile pages.""" from django.contrib.auth import get_user_model from django.contrib.auth.mixins import LoginRequiredMixin +from django.urls import reverse_lazy from django.views.generic import TemplateView +from django.views.generic.edit import FormView + +from users.forms import SubscribeNotificationEmailsForm User = get_user_model() @@ -11,8 +15,25 @@ class ProfileView(LoginRequiredMixin, TemplateView): template_name = 'users/settings/profile.html' + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context['subscribe_notification_emails_form'] = SubscribeNotificationEmailsForm( + {'subscribe': not self.request.user.is_subscribed_to_notification_emails}, + ) + return context + class DeleteView(LoginRequiredMixin, TemplateView): """Template view where account deletion can be requested.""" template_name = 'users/settings/delete.html' + + +class SubscribeNotificationEmailsView(LoginRequiredMixin, FormView): + form_class = SubscribeNotificationEmailsForm + success_url = reverse_lazy('users:my-profile') + + def form_valid(self, form): + self.request.user.is_subscribed_to_notification_emails = form.cleaned_data['subscribe'] + self.request.user.save(update_fields={'is_subscribed_to_notification_emails'}) + return super().form_valid(form) -- 2.30.2 From 2e04e377187567588ec5c5b7924557d3575d665f Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 15 Apr 2024 19:06:08 +0200 Subject: [PATCH 15/28] add tests --- common/tests/factories/users.py | 8 ++ extensions/signals.py | 56 ++++++--- notifications/tests/test_follow_logic.py | 140 +++++++++++++++++++++++ users/signals.py | 7 +- 4 files changed, 189 insertions(+), 22 deletions(-) create mode 100644 notifications/tests/test_follow_logic.py diff --git a/common/tests/factories/users.py b/common/tests/factories/users.py index ed085882..5925e1c4 100644 --- a/common/tests/factories/users.py +++ b/common/tests/factories/users.py @@ -1,6 +1,7 @@ import random from django.contrib.auth import get_user_model +from django.contrib.auth.models import Group from factory.django import DjangoModelFactory import factory @@ -42,3 +43,10 @@ class UserFactory(DjangoModelFactory): oauth_tokens = factory.RelatedFactoryList(OAuthUserTokenFactory, factory_related_name='user') oauth_info = factory.RelatedFactory(OAuthUserInfoFactory, factory_related_name='user') + + +def create_moderator(): + user = UserFactory() + moderators = Group.objects.get(name='moderators') + user.groups.add(moderators) + return user diff --git a/extensions/signals.py b/extensions/signals.py index 1e29b558..eba2d3f7 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -1,25 +1,28 @@ from typing import Union -from actstream.actions import follow +from actstream.actions import follow, unfollow +from django.contrib.auth import get_user_model from django.contrib.auth.models import Group -from django.db.models.signals import pre_save, post_save, post_delete +from django.db.models.signals import m2m_changed, pre_save, post_save, post_delete from django.dispatch import receiver from constants.activity import Flag -import extensions.models +from extensions.models import Extension, Preview, Version import files.models +User = get_user_model() -@receiver(post_delete, sender=extensions.models.Preview) -def _delete_file(sender: object, instance: extensions.models.Preview, **kwargs: object) -> None: + +@receiver(post_delete, sender=Preview) +def _delete_file(sender: object, instance: Preview, **kwargs: object) -> None: instance.file.delete() -@receiver(pre_save, sender=extensions.models.Extension) -@receiver(pre_save, sender=extensions.models.Version) +@receiver(pre_save, sender=Extension) +@receiver(pre_save, sender=Version) def _record_changes( sender: object, - instance: Union[extensions.models.Extension, extensions.models.Version], + instance: Union[Extension, Version], **kwargs: object, ) -> None: was_changed, old_state = instance.pre_save_record() @@ -32,7 +35,7 @@ def _record_changes( instance.record_status_change(was_changed, old_state, **kwargs) -@receiver(post_save, sender=extensions.models.Extension) +@receiver(post_save, sender=Extension) def _update_search_index(sender, instance, **kw): pass # TODO: update search index @@ -46,18 +49,18 @@ def extension_should_be_listed(extension): ) -@receiver(post_save, sender=extensions.models.Extension) -@receiver(post_save, sender=extensions.models.Version) +@receiver(post_save, sender=Extension) +@receiver(post_save, sender=Version) @receiver(post_save, sender=files.models.File) def _set_is_listed( sender: object, - instance: Union[extensions.models.Extension, extensions.models.Version, files.models.File], + instance: Union[Extension, Version, files.models.File], *args: object, **kwargs: object, ) -> None: - if isinstance(instance, extensions.models.Extension): + if isinstance(instance, Extension): extension = instance - elif isinstance(instance, extensions.models.Version): + elif isinstance(instance, Version): extension = instance.extension else: # Some file types (e.g., image or video) have no version associated to them. @@ -73,17 +76,17 @@ def _set_is_listed( if old_is_listed == new_is_listed: return - if extension.status == extensions.models.Extension.STATUSES.APPROVED and not new_is_listed: - extension.status = extensions.models.Extension.STATUSES.INCOMPLETE + if extension.status == Extension.STATUSES.APPROVED and not new_is_listed: + extension.status = Extension.STATUSES.INCOMPLETE extension.is_listed = new_is_listed extension.save() -@receiver(post_save, sender=extensions.models.Extension) +@receiver(post_save, sender=Extension) def _setup_followers( sender: object, - instance: extensions.models.Extension, + instance: Extension, created: bool, **kwargs: object, ) -> None: @@ -94,3 +97,20 @@ def _setup_followers( follow(user, instance, send_action=False, flag=Flag.AUTHOR) for user in Group.objects.get(name='moderators').user_set.all(): follow(user, instance, send_action=False, flag=Flag.MODERATOR) + + +@receiver(m2m_changed, sender=Extension.authors.through) +def _update_authors_follow(instance, action, model, reverse, pk_set, **kwargs): + if model == Extension and not reverse: + users = [instance] + extensions = Extension.objects.filter(pk__in=pk_set) + else: + extensions = [instance] + users = User.objects.filter(pk__in=pk_set) + + for user in users: + for extension in extensions: + if action == 'post_remove': + unfollow(user, extension, send_action=False, flag=Flag.AUTHOR) + elif action == 'post_add': + follow(user, extension, send_action=False, flag=Flag.AUTHOR) diff --git a/notifications/tests/test_follow_logic.py b/notifications/tests/test_follow_logic.py new file mode 100644 index 00000000..01ec3e84 --- /dev/null +++ b/notifications/tests/test_follow_logic.py @@ -0,0 +1,140 @@ +from pathlib import Path + +from django.test import TestCase +from django.urls import reverse + +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, create_moderator +from files.models import File +from notifications.models import Notification +from reviewers.models import ApprovalActivity + +TEST_FILES_DIR = Path(__file__).resolve().parent / '../../extensions/tests/files' + + +class TestTasks(TestCase): + fixtures = ['dev', 'licenses'] + + def test_ratings(self): + extension = create_approved_version(ratings=[]).extension + author = extension.authors.first() + notification_nr = Notification.objects.filter(recipient=author).count() + some_user = UserFactory() + self.client.force_login(some_user) + url = extension.get_rate_url() + response = self.client.post(url, {'score': 3, 'text': 'rating text'}) + self.assertEqual(response.status_code, 302) + self.assertEqual(extension.ratings.count(), 1) + new_notification_nr = Notification.objects.filter(recipient=author).count() + self.assertEqual(new_notification_nr, notification_nr + 1) + + def test_abuse(self): + extension = create_approved_version(ratings=[]).extension + moderator = create_moderator() + notification_nr = Notification.objects.filter(recipient=moderator).count() + some_user = UserFactory() + self.client.force_login(some_user) + url = extension.get_report_url() + self.client.post( + url, + { + 'message': 'test message', + 'reason': '127', + 'version': '', + }, + ) + new_notification_nr = Notification.objects.filter(recipient=moderator).count() + self.assertEqual(new_notification_nr, notification_nr + 1) + + def test_new_extension_submitted(self): + moderator = create_moderator() + notification_nr = Notification.objects.filter(recipient=moderator).count() + some_user = UserFactory() + file_data = { + 'metadata': { + 'tagline': 'Get insight on the complexity of an edit', + 'id': 'edit_breakdown', + 'name': 'Edit Breakdown', + 'version': '0.1.0', + 'blender_version_min': '4.2.0', + 'type': 'add-on', + 'schema_version': "1.0.0", + }, + 'file_hash': 'sha256:4f3664940fc41641c7136a909270a024bbcfb2f8523a06a0d22f85c459b0b1ae', + 'size_bytes': 53959, + 'tags': ['Sequencer'], + 'version_str': '0.1.0', + 'slug': 'edit-breakdown', + } + file = FileFactory( + type=File.TYPES.BPY, + user=some_user, + original_hash=file_data['file_hash'], + hash=file_data['file_hash'], + metadata=file_data['metadata'], + ) + create_version( + file=file, + extension__name=file_data['metadata']['name'], + extension__slug=file_data['metadata']['id'].replace("_", "-"), + extension__website=None, + tagline=file_data['metadata']['tagline'], + version=file_data['metadata']['version'], + blender_version_min=file_data['metadata']['blender_version_min'], + schema_version=file_data['metadata']['schema_version'], + ) + self.client.force_login(some_user) + data = { + # Most of these values should come from the form's initial values, set in the template + # Version fields + 'release_notes': 'initial release', + # Extension fields + 'description': 'Rather long and verbose description', + 'support': 'https://example.com/issues', + # Previews + 'form-TOTAL_FORMS': ['2'], + 'form-INITIAL_FORMS': ['0'], + 'form-MIN_NUM_FORMS': ['0'], + 'form-MAX_NUM_FORMS': ['1000'], + 'form-0-id': '', + 'form-0-caption': ['First Preview Caption Text'], + 'form-1-id': '', + 'form-1-caption': ['Second Preview Caption Text'], + # Submit for Approval. + 'submit_draft': '', + } + file_name1 = 'test_preview_image_0001.png' + file_name2 = 'test_preview_image_0002.png' + with open(TEST_FILES_DIR / file_name1, 'rb') as fp1, open( + TEST_FILES_DIR / file_name2, 'rb' + ) as fp2: + files = { + 'form-0-source': fp1, + 'form-1-source': fp2, + } + self.client.post(file.get_submit_url(), {**data, **files}) + new_notification_nr = Notification.objects.filter(recipient=moderator).count() + self.assertEqual(new_notification_nr, notification_nr + 1) + + def test_approval_queue_activity(self): + extension = create_approved_version(ratings=[]).extension + author = extension.authors.first() + moderator = create_moderator() + some_user = UserFactory() + notification_nrs = {} + for user in [author, moderator, some_user]: + notification_nrs[user.pk] = Notification.objects.filter(recipient=user).count() + self._leave_a_comment(some_user, extension, 'this is bad') + self._leave_a_comment(moderator, extension, 'thanks for the heads up') + new_notification_nrs = {} + for user in [author, moderator, some_user]: + new_notification_nrs[user.pk] = Notification.objects.filter(recipient=user).count() + self.assertEqual(new_notification_nrs[author.pk], notification_nrs[author.pk] + 2) + self.assertEqual(new_notification_nrs[moderator.pk], notification_nrs[moderator.pk] + 1) + self.assertEqual(new_notification_nrs[some_user.pk], notification_nrs[some_user.pk] + 1) + + def _leave_a_comment(self, user, extension, text): + self.client.force_login(user) + url = reverse('reviewers:approval-comment', args=[extension.slug]) + self.client.post(url, {'type': ApprovalActivity.ActivityType.COMMENT, 'message': text}) diff --git a/users/signals.py b/users/signals.py index 5887bc21..0d937ecb 100644 --- a/users/signals.py +++ b/users/signals.py @@ -61,9 +61,8 @@ def update_moderator_follows(instance, action, model, reverse, pk_set, **kwargs) users = User.objects.filter(pk__in=pk_set) for user in users: - if action == 'post_remove': - for extension in extensions: + for extension in extensions: + if action == 'post_remove': unfollow(user, extension, send_action=False, flag=Flag.MODERATOR) - elif action == 'post_add': - for extension in extensions: + elif action == 'post_add': follow(user, extension, send_action=False, flag=Flag.MODERATOR) -- 2.30.2 From 692bb06b3afc617765b5b475e6ce6b4483152293 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 16 Apr 2024 15:59:30 +0200 Subject: [PATCH 16/28] remove the migration: it breaks due to a mismatch between fake and real models --- .../migrations/0002_setup_followers.py | 38 ------------------- 1 file changed, 38 deletions(-) delete mode 100644 notifications/migrations/0002_setup_followers.py diff --git a/notifications/migrations/0002_setup_followers.py b/notifications/migrations/0002_setup_followers.py deleted file mode 100644 index 97d63e2b..00000000 --- a/notifications/migrations/0002_setup_followers.py +++ /dev/null @@ -1,38 +0,0 @@ -# Generated by Django 4.2.11 on 2024-04-15 10:18 - -from actstream.actions import follow -from django.contrib.auth.models import Group -from django.db import migrations - -from constants.activity import Flag - - -def setup_followers(apps, schema_editor): - # !!! not using apps.get_model('extensions', 'Extension') - # because it doesn't work with actstream.registry: - # - # File ".../site-packages/actstream/actions.py", line 34, in follow - # check(obj) - # File ".../site-packages/actstream/registry.py", line 105, in check - # raise ImproperlyConfigured( - # django.core.exceptions.ImproperlyConfigured: The model Extension is not registered. - # Please use actstream.registry to register it. - # - # if this ever causes issues in the future, delete this code or find a workaround - from extensions.models import Extension - for extension in Extension.objects.all(): - for user in extension.authors.all(): - follow(user, extension, send_action=False, flag=Flag.AUTHOR) - for user in Group.objects.get(name='moderators').user_set.all(): - follow(user, extension, send_action=False, flag=Flag.MODERATOR) - - -class Migration(migrations.Migration): - - dependencies = [ - ('notifications', '0001_initial'), - ] - - operations = [ - migrations.RunPython(setup_followers, reverse_code=migrations.RunPython.noop), - ] -- 2.30.2 From 0e0e8df0248e32c89a8f7f2a6242014f5eaed9ff Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 16 Apr 2024 17:26:56 +0200 Subject: [PATCH 17/28] send notifications individually, not in batches --- .../management/commands/send_notifications.py | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/users/management/commands/send_notifications.py b/users/management/commands/send_notifications.py index 27fa9988..b275029a 100644 --- a/users/management/commands/send_notifications.py +++ b/users/management/commands/send_notifications.py @@ -1,5 +1,4 @@ """Send user notifications as emails, at most once delivery.""" -from collections import defaultdict import logging from django.conf import settings @@ -15,37 +14,33 @@ logger.setLevel(logging.INFO) class Command(BaseCommand): def handle(self, *args, **options): # noqa: D102 - logger.info('start, checking for outstanding notifications') - notifications = Notification.objects.filter(processed_by_mailer_at=None) - batched_by_recipient = defaultdict(list) - for n in notifications: - batched_by_recipient[n.recipient].append(n) - - for recipient, batch in batched_by_recipient.items(): - to_send = [] - for n in batch: - n.processed_by_mailer_at = timezone.now() - if not recipient.is_subscribed_to_notification_emails: - continue - # check that email is confirmed to avoid spamming unsuspecting email owners - if recipient.confirmed_email_at is None: - continue - n.sent = True - to_send.append(n) + unprocessed_notifications = Notification.objects.filter(processed_by_mailer_at=None) + for n in unprocessed_notifications: + logger.info(f'processing Notification pk={n.pk}') + n.processed_by_mailer_at = timezone.now() + recipient = n.recipient + if not recipient.is_subscribed_to_notification_emails: + logger.info(f'{recipient} is not subscribed, skipping') + n.save() + continue + # check that email is confirmed to avoid spamming unsuspecting email owners + if recipient.confirmed_email_at is None: + logger.info(f'{recipient} has unconfirmed email, skipping') + n.save() + continue + n.sent = True # first mark as processed, then send: avoid spamming in case of a crash-loop - Notification.objects.bulk_update(batch, ['processed_by_mailer_at', 'sent']) - if len(to_send) > 0: - logger.info(f'sending an email to {recipient} about {len(to_send)} notifications') - send_batch_notification_email(recipient, to_send) - logger.info('finish') + n.save() + logger.info(f'sending an email to {recipient}: {n.action}') + send_notification_email(n) -def send_batch_notification_email(recipient, notifications): +def send_notification_email(notification): subject = 'New activity' - message = '\n\n'.join([n.format_email_txt() for n in notifications]) + message = notification.format_email_txt() send_mail( subject, message, settings.DEFAULT_FROM_EMAIL, - [recipient.email], + [notification.recipient.email], ) -- 2.30.2 From 93472e9799c9504074b9ecdb3a74f764ebee89e4 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 16 Apr 2024 17:32:27 +0200 Subject: [PATCH 18/28] m2m_changed signals: early exit for anything except post_* --- extensions/signals.py | 3 +++ users/signals.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/extensions/signals.py b/extensions/signals.py index eba2d3f7..50fc2486 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -101,6 +101,9 @@ def _setup_followers( @receiver(m2m_changed, sender=Extension.authors.through) def _update_authors_follow(instance, action, model, reverse, pk_set, **kwargs): + if action not in ['post_add', 'post_remove']: + return + if model == Extension and not reverse: users = [instance] extensions = Extension.objects.filter(pk__in=pk_set) diff --git a/users/signals.py b/users/signals.py index 0d937ecb..4fac99e9 100644 --- a/users/signals.py +++ b/users/signals.py @@ -48,6 +48,9 @@ def update_moderator_follows(instance, action, model, reverse, pk_set, **kwargs) The flag=Flag.MODERATOR is used to avoid deleting follow relations that were created in contexts other than moderator's duties. """ + if action not in ['post_add', 'post_remove']: + return + moderators = Group.objects.get(name='moderators') extensions = Extension.objects.all() users = [] -- 2.30.2 From eccfbd3baa48ecf816e5460ca56243d68b9a7d56 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 16 Apr 2024 17:59:13 +0200 Subject: [PATCH 19/28] update notification model --- notifications/migrations/0001_initial.py | 8 +++++--- notifications/models.py | 17 +++++++++++------ ...fications.py => send_notification_emails.py} | 2 +- 3 files changed, 17 insertions(+), 10 deletions(-) rename users/management/commands/{send_notifications.py => send_notification_emails.py} (98%) diff --git a/notifications/migrations/0001_initial.py b/notifications/migrations/0001_initial.py index 9e4ee35c..5e38f335 100644 --- a/notifications/migrations/0001_initial.py +++ b/notifications/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.11 on 2024-04-12 18:14 +# Generated by Django 4.2.11 on 2024-04-16 15:56 from django.conf import settings from django.db import migrations, models @@ -10,8 +10,8 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('actstream', '0003_add_follow_flag'), migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('actstream', '0003_add_follow_flag'), ] operations = [ @@ -21,10 +21,12 @@ class Migration(migrations.Migration): ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('recipient', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), ('action', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='actstream.action')), - ('sent', models.BooleanField(default=False)), + ('email_sent', models.BooleanField(default=False)), ('processed_by_mailer_at', models.DateTimeField(default=None, null=True)), + ('read_at', models.DateTimeField(default=None, null=True)), ], options={ + 'indexes': [models.Index(fields=['processed_by_mailer_at'], name='notificatio_process_fc95bc_idx'), models.Index(fields=['recipient', 'read_at'], name='notificatio_recipie_564b1f_idx')], 'unique_together': {('recipient', 'action')}, }, ), diff --git a/notifications/models.py b/notifications/models.py index eb8d83ab..95225145 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -10,19 +10,24 @@ User = get_user_model() class Notification(models.Model): """Notification records are created in Action's post_save signal. - When a user marks a notification as read, a record is deleted. - While a notification exists it can be picked up by a send_notifications management command - that runs periodically in background. - If a recipient opted out from receiving notifications of particular type (based on action.verb), - we shouldn't include it in the email, leaving sent=False. + When a user marks a notification as read, read_at is set. + send_notification_emails management command runs periodically in background and sends all + notifications that haven't been processed yet, read_at is not checked when sending emails. + email_sent flag is used only to record the fact that we attempted to send an email. + A user can unsubscribe from notification emails in their profile settings. """ recipient = models.ForeignKey(User, null=False, on_delete=models.CASCADE) action = models.ForeignKey(Action, null=False, on_delete=models.CASCADE) - sent = models.BooleanField(default=False, null=False) + email_sent = models.BooleanField(default=False, null=False) processed_by_mailer_at = models.DateTimeField(default=None, null=True) + read_at = models.DateTimeField(default=None, null=True) class Meta: + indexes = [ + models.Index(fields=['processed_by_mailer_at']), + models.Index(fields=['recipient', 'read_at']), + ] unique_together = ['recipient', 'action'] def format_email_txt(self): diff --git a/users/management/commands/send_notifications.py b/users/management/commands/send_notification_emails.py similarity index 98% rename from users/management/commands/send_notifications.py rename to users/management/commands/send_notification_emails.py index b275029a..e8cb7c56 100644 --- a/users/management/commands/send_notifications.py +++ b/users/management/commands/send_notification_emails.py @@ -28,7 +28,7 @@ class Command(BaseCommand): logger.info(f'{recipient} has unconfirmed email, skipping') n.save() continue - n.sent = True + n.email_sent = True # first mark as processed, then send: avoid spamming in case of a crash-loop n.save() logger.info(f'sending an email to {recipient}: {n.action}') -- 2.30.2 From 4310e69a394f8fe84e6da8f02cf5f344c0990594 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 16 Apr 2024 19:32:55 +0200 Subject: [PATCH 20/28] notifications views --- blender_extensions/urls.py | 1 + .../notifications/notification_list.html | 22 +++++++++ notifications/urls.py | 25 ++++++++++ notifications/views.py | 49 +++++++++++++++++++ 4 files changed, 97 insertions(+) create mode 100644 notifications/templates/notifications/notification_list.html create mode 100644 notifications/urls.py create mode 100644 notifications/views.py diff --git a/blender_extensions/urls.py b/blender_extensions/urls.py index 711170de..fb6f91d8 100644 --- a/blender_extensions/urls.py +++ b/blender_extensions/urls.py @@ -39,6 +39,7 @@ urlpatterns = [ path('', include('users.urls')), path('', include('teams.urls')), path('', include('reviewers.urls')), + path('', include('notifications.urls')), path('api/swagger/', RedirectView.as_view(url='/api/v1/swagger/')), path('api/v1/', SpectacularAPIView.as_view(), name='schema_v1'), path('api/v1/swagger/', SpectacularSwaggerView.as_view(url_name='schema_v1'), name='swagger'), diff --git a/notifications/templates/notifications/notification_list.html b/notifications/templates/notifications/notification_list.html new file mode 100644 index 00000000..9ac5dc59 --- /dev/null +++ b/notifications/templates/notifications/notification_list.html @@ -0,0 +1,22 @@ +{% extends "common/base.html" %} +{% load i18n %} + +{% block page_title %}{% blocktranslate %}Notifications{% endblocktranslate %}{% endblock page_title %} + +{% block content %} +{% if notification_list %} + {% for notification in notification_list %} +
+ {{ notification.action }} + {% if notification.read_at %} + {% else %} + {% blocktranslate %}Mark as read{% endblocktranslate %} + {% endif %} +
+ {% endfor %} +{% else %} +

+ {% blocktranslate %}You have no notifications{% endblocktranslate %} +

+{% endif %} +{% endblock content %} diff --git a/notifications/urls.py b/notifications/urls.py new file mode 100644 index 00000000..7a961ae3 --- /dev/null +++ b/notifications/urls.py @@ -0,0 +1,25 @@ +from django.urls import path, include + +import notifications.views as views + +app_name = 'notifications' +urlpatterns = [ + path( + 'notifications/', + include( + [ + path('', views.NotificationsView.as_view(), name='notifications'), + path( + 'mark-read-all/', + views.MarkReadAllView.as_view(), + name='notifications-mark-read-all', + ), + path( + '/mark-read/', + views.MarkReadView.as_view(), + name='notifications-mark-read', + ), + ], + ), + ), +] diff --git a/notifications/views.py b/notifications/views.py new file mode 100644 index 00000000..5fb08b9c --- /dev/null +++ b/notifications/views.py @@ -0,0 +1,49 @@ +"""Notifications pages.""" +from django.contrib.auth.mixins import LoginRequiredMixin +from django.http import HttpResponseForbidden +from django.http.response import JsonResponse +from django.utils import timezone +from django.views.generic import ListView +from django.views.generic.detail import SingleObjectMixin +from django.views.generic.edit import FormView +from django.views import View + +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) + + +class MarkReadAllView(LoginRequiredMixin, FormView): + model = Notification + raise_exception = True + + def post(self, request, *args, **kwargs): + """Mark all previously unread notifications as read.""" + unread = self.model.objects.filter(recipient=request.user, read_at__isnull=True) + now = timezone.now() + for notification in unread: + notification.read_at = now + + Notification.objects.bulk_update(unread, ['read_at']) + + return JsonResponse({}) + + +class MarkReadView(LoginRequiredMixin, SingleObjectMixin, View): + model = Notification + raise_exception = True + + def post(self, request, *args, **kwargs): + notification = self.get_object() + if notification.recipient != request.user: + return HttpResponseForbidden() + notification.read_at = timezone.now() + notification.save(update_fields=['read_at']) + return JsonResponse({}) -- 2.30.2 From 1df4a0626e980d5d4bff55e57f151e03369477fb Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 12:25:28 +0200 Subject: [PATCH 21/28] revert import change --- extensions/signals.py | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/extensions/signals.py b/extensions/signals.py index 50fc2486..34dfd935 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -7,22 +7,22 @@ from django.db.models.signals import m2m_changed, pre_save, post_save, post_dele from django.dispatch import receiver from constants.activity import Flag -from extensions.models import Extension, Preview, Version +import extensions.models import files.models User = get_user_model() -@receiver(post_delete, sender=Preview) -def _delete_file(sender: object, instance: Preview, **kwargs: object) -> None: +@receiver(post_delete, sender=extensions.models.Preview) +def _delete_file(sender: object, instance: extensions.models.Preview, **kwargs: object) -> None: instance.file.delete() -@receiver(pre_save, sender=Extension) -@receiver(pre_save, sender=Version) +@receiver(pre_save, sender=extensions.models.Extension) +@receiver(pre_save, sender=extensions.models.Version) def _record_changes( sender: object, - instance: Union[Extension, Version], + instance: Union[extensions.models.Extension, extensions.models.Version], **kwargs: object, ) -> None: was_changed, old_state = instance.pre_save_record() @@ -35,7 +35,7 @@ def _record_changes( instance.record_status_change(was_changed, old_state, **kwargs) -@receiver(post_save, sender=Extension) +@receiver(post_save, sender=extensions.models.Extension) def _update_search_index(sender, instance, **kw): pass # TODO: update search index @@ -49,18 +49,18 @@ def extension_should_be_listed(extension): ) -@receiver(post_save, sender=Extension) -@receiver(post_save, sender=Version) +@receiver(post_save, sender=extensions.models.Extension) +@receiver(post_save, sender=extensions.models.Version) @receiver(post_save, sender=files.models.File) def _set_is_listed( sender: object, - instance: Union[Extension, Version, files.models.File], + instance: Union[extensions.models.Extension, extensions.models.Version, files.models.File], *args: object, **kwargs: object, ) -> None: - if isinstance(instance, Extension): + if isinstance(instance, extensions.models.Extension): extension = instance - elif isinstance(instance, Version): + elif isinstance(instance, extensions.models.Version): extension = instance.extension else: # Some file types (e.g., image or video) have no version associated to them. @@ -76,17 +76,17 @@ def _set_is_listed( if old_is_listed == new_is_listed: return - if extension.status == Extension.STATUSES.APPROVED and not new_is_listed: - extension.status = Extension.STATUSES.INCOMPLETE + if extension.status == extensions.models.Extension.STATUSES.APPROVED and not new_is_listed: + extension.status = extensions.models.Extension.STATUSES.INCOMPLETE extension.is_listed = new_is_listed extension.save() -@receiver(post_save, sender=Extension) +@receiver(post_save, sender=extensions.models.Extension) def _setup_followers( sender: object, - instance: Extension, + instance: extensions.models.Extension, created: bool, **kwargs: object, ) -> None: @@ -99,20 +99,20 @@ def _setup_followers( follow(user, instance, send_action=False, flag=Flag.MODERATOR) -@receiver(m2m_changed, sender=Extension.authors.through) +@receiver(m2m_changed, sender=extensions.models.Extension.authors.through) def _update_authors_follow(instance, action, model, reverse, pk_set, **kwargs): if action not in ['post_add', 'post_remove']: return - if model == Extension and not reverse: + if model == extensions.models.Extension and not reverse: + targets = extensions.models.Extension.objects.filter(pk__in=pk_set) users = [instance] - extensions = Extension.objects.filter(pk__in=pk_set) else: - extensions = [instance] + targets = [instance] users = User.objects.filter(pk__in=pk_set) for user in users: - for extension in extensions: + for extension in targets: if action == 'post_remove': unfollow(user, extension, send_action=False, flag=Flag.AUTHOR) elif action == 'post_add': -- 2.30.2 From a7bcd419fefc1dbb91e97b2431e7c76804f9552c Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 12:34:04 +0200 Subject: [PATCH 22/28] don't handle submitted for review now postpone till we start creating ApprovalActivity --- constants/activity.py | 1 - extensions/views/manage.py | 12 +----------- notifications/signals.py | 1 - notifications/tests/test_follow_logic.py | 2 ++ 4 files changed, 3 insertions(+), 13 deletions(-) diff --git a/constants/activity.py b/constants/activity.py index 834e6b61..86d3b969 100644 --- a/constants/activity.py +++ b/constants/activity.py @@ -10,7 +10,6 @@ class Verb: REPORTED_REVIEW = 'reported review' REQUESTED_CHANGES = 'requested changes' REQUESTED_REVIEW = 'requested review' - SUBMITTED_FOR_REVIEW = 'submitted for review' class Flag: diff --git a/extensions/views/manage.py b/extensions/views/manage.py index fea37ed6..77498540 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -1,5 +1,4 @@ """Contains views allowing developers to manage their add-ons.""" -from actstream import action from django import forms from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin from django.contrib.messages.views import SuccessMessageMixin @@ -16,7 +15,6 @@ from .mixins import ( DraftVersionMixin, DraftMixin, ) -from constants.activity import Verb from extensions.forms import ( EditPreviewFormSet, AddPreviewFormSet, @@ -393,18 +391,10 @@ class DraftExtensionView( # Send the extension and version to the review if 'submit_draft' in self.request.POST: extension_form.instance.status = extension_form.instance.STATUSES.AWAITING_REVIEW + # FIXME create ApprovalActivity extension_form.save() add_preview_formset.save() form.save() - if 'submit_draft' in self.request.POST: - # in the signal context we won't know the real user doing that, - # so keeping this in the view code for now - # alternatively (TODO?) this could be turned into an ApprovalActivity record - action.send( - self.request.user, - verb=Verb.SUBMITTED_FOR_REVIEW, - target=extension_form.instance, - ) return super().form_valid(form) except forms.ValidationError as e: if 'hash' in e.error_dict: diff --git a/notifications/signals.py b/notifications/signals.py index 72e5c71d..66d1fb70 100644 --- a/notifications/signals.py +++ b/notifications/signals.py @@ -18,7 +18,6 @@ VERB2FLAGS = { Verb.REPORTED_REVIEW: [Flag.MODERATOR], Verb.REQUESTED_CHANGES: [Flag.AUTHOR, Flag.MODERATOR, Flag.REVIEWER], Verb.REQUESTED_REVIEW: [Flag.MODERATOR, Flag.REVIEWER], - Verb.SUBMITTED_FOR_REVIEW: [Flag.MODERATOR], } diff --git a/notifications/tests/test_follow_logic.py b/notifications/tests/test_follow_logic.py index 01ec3e84..8a3d5b50 100644 --- a/notifications/tests/test_follow_logic.py +++ b/notifications/tests/test_follow_logic.py @@ -1,4 +1,5 @@ from pathlib import Path +import unittest from django.test import TestCase from django.urls import reverse @@ -47,6 +48,7 @@ class TestTasks(TestCase): new_notification_nr = Notification.objects.filter(recipient=moderator).count() self.assertEqual(new_notification_nr, notification_nr + 1) + @unittest.skip('FIXME in DraftExtensionView') def test_new_extension_submitted(self): moderator = create_moderator() notification_nr = Notification.objects.filter(recipient=moderator).count() -- 2.30.2 From 7a22d2f4dc1b0e39356b81647f89a91dd15b79bb Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 12:50:26 +0200 Subject: [PATCH 23/28] send emails only to internal emails first --- users/management/commands/send_notification_emails.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/users/management/commands/send_notification_emails.py b/users/management/commands/send_notification_emails.py index e8cb7c56..88b92ddf 100644 --- a/users/management/commands/send_notification_emails.py +++ b/users/management/commands/send_notification_emails.py @@ -28,6 +28,11 @@ 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() -- 2.30.2 From 3e1b0b25fe589cf5b71f63ff17b07f99b92e89d1 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 13:22:20 +0200 Subject: [PATCH 24/28] email subjects --- notifications/models.py | 10 ++++++---- users/management/commands/send_notification_emails.py | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/notifications/models.py b/notifications/models.py index 95225145..633c4192 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -30,10 +30,12 @@ class Notification(models.Model): ] unique_together = ['recipient', 'action'] - def format_email_txt(self): + def format_email(self): + action = self.action + subject = f'New Activity: {action.actor.full_name} {action.verb} {action.target}' url = self.get_absolute_url() - # TODO construct a proper phrase, depending on the verb, maybe use a template - return f'{self.action.actor.full_name} {self.action.verb} {self.action.target}: {url}' + mesage = f'{action.actor.full_name} {action.verb} {action.target}: {url}' + return (subject, mesage) def get_absolute_url(self): if self.action.verb == Verb.RATED_EXTENSION: @@ -50,5 +52,5 @@ class Notification(models.Model): else: url = self.action.target.get_absolute_url() - # TODO? url cloacking to auto-delete visited notifications + # TODO? url cloacking to mark visited notifications as read automatically return absolutify(url) diff --git a/users/management/commands/send_notification_emails.py b/users/management/commands/send_notification_emails.py index 88b92ddf..8d704664 100644 --- a/users/management/commands/send_notification_emails.py +++ b/users/management/commands/send_notification_emails.py @@ -41,8 +41,9 @@ class Command(BaseCommand): def send_notification_email(notification): - subject = 'New activity' - message = notification.format_email_txt() + # TODO construct a proper phrase, depending on the verb, + # possibly share a template with NotificationsView + subject, message = notification.format_email() send_mail( subject, message, -- 2.30.2 From c3a8ec41c6943482403b729a1cf79815b5046fcf Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 14:06:22 +0200 Subject: [PATCH 25/28] move command to notifications app --- notifications/management/__init__.py | 0 notifications/management/commands/__init__.py | 0 .../management/commands/send_notification_emails.py | 0 3 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 notifications/management/__init__.py create mode 100644 notifications/management/commands/__init__.py rename {users => notifications}/management/commands/send_notification_emails.py (100%) diff --git a/notifications/management/__init__.py b/notifications/management/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/notifications/management/commands/__init__.py b/notifications/management/commands/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/users/management/commands/send_notification_emails.py b/notifications/management/commands/send_notification_emails.py similarity index 100% rename from users/management/commands/send_notification_emails.py rename to notifications/management/commands/send_notification_emails.py -- 2.30.2 From 49d73d1b9f093c3ce1e8853638cb9e8565b7ea43 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 14:49:14 +0200 Subject: [PATCH 26/28] ensure_followers command --- .../management/commands/ensure_followers.py | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 notifications/management/commands/ensure_followers.py diff --git a/notifications/management/commands/ensure_followers.py b/notifications/management/commands/ensure_followers.py new file mode 100644 index 00000000..7ea5a938 --- /dev/null +++ b/notifications/management/commands/ensure_followers.py @@ -0,0 +1,34 @@ +"""Create all necessary follow records.""" +import logging + +from actstream.actions import follow +from django.contrib.auth.models import Group +from django.core.management.base import BaseCommand + +from constants.activity import Flag +from extensions.models import Extension +from reviewers.models import ApprovalActivity + +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + + +class Command(BaseCommand): + def handle(self, *args, **options): # noqa: D102 + extensions = Extension.objects.all() + moderators = Group.objects.get(name='moderators').user_set.all() + for extension in extensions: + authors = extension.authors.all() + for recipient in authors: + _follow_with_log(recipient, extension, Flag.AUTHOR) + for recipient in moderators: + _follow_with_log(recipient, extension, Flag.MODERATOR) + + approval_activity_items = ApprovalActivity.objects.all().select_related('extension', 'user') + for item in approval_activity_items: + _follow_with_log(item.user, item.extension, Flag.MODERATOR) + + +def _follow_with_log(user, target, flag): + follow(user, target, send_action=False, flag=flag) + logger.info(f'{user} follows {target} with flag={flag}') -- 2.30.2 From f71361826d95a3cf03f0e3a0972f277386ecf65e Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 14:55:31 +0200 Subject: [PATCH 27/28] add a todo note --- notifications/management/commands/ensure_followers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/notifications/management/commands/ensure_followers.py b/notifications/management/commands/ensure_followers.py index 7ea5a938..5b3461c5 100644 --- a/notifications/management/commands/ensure_followers.py +++ b/notifications/management/commands/ensure_followers.py @@ -15,6 +15,7 @@ logger.setLevel(logging.INFO) class Command(BaseCommand): def handle(self, *args, **options): # noqa: D102 + # TODO? keep a record of explicit unfollow requests to avoid re-following extensions = Extension.objects.all() moderators = Group.objects.get(name='moderators').user_set.all() for extension in extensions: -- 2.30.2 From 0cdaac1a2586beae577069dbced15db6ab296aa5 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 18 Apr 2024 15:59:53 +0200 Subject: [PATCH 28/28] rename REVIEW->RATING --- abuse/signals.py | 6 +++--- constants/activity.py | 2 +- notifications/signals.py | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/abuse/signals.py b/abuse/signals.py index a305ad3d..129a5c77 100644 --- a/abuse/signals.py +++ b/abuse/signals.py @@ -8,7 +8,7 @@ from abuse.models import AbuseReport from constants.activity import Verb from constants.base import ( ABUSE_TYPE_EXTENSION, - ABUSE_TYPE_REVIEW, + ABUSE_TYPE_RATING, ABUSE_TYPE_USER, ) @@ -30,8 +30,8 @@ def _create_action_from_report( if instance.type == ABUSE_TYPE_EXTENSION: verb = Verb.REPORTED_EXTENSION - elif instance.type == ABUSE_TYPE_REVIEW: - verb = Verb.REPORTED_REVIEW + elif instance.type == ABUSE_TYPE_RATING: + verb = Verb.REPORTED_RATING elif instance.type == ABUSE_TYPE_USER: # TODO? return diff --git a/constants/activity.py b/constants/activity.py index 86d3b969..e5127d69 100644 --- a/constants/activity.py +++ b/constants/activity.py @@ -7,7 +7,7 @@ class Verb: COMMENTED = 'commented' RATED_EXTENSION = 'rated extension' REPORTED_EXTENSION = 'reported extension' - REPORTED_REVIEW = 'reported review' + REPORTED_RATING = 'reported rating' REQUESTED_CHANGES = 'requested changes' REQUESTED_REVIEW = 'requested review' diff --git a/notifications/signals.py b/notifications/signals.py index 66d1fb70..468e921c 100644 --- a/notifications/signals.py +++ b/notifications/signals.py @@ -15,7 +15,7 @@ VERB2FLAGS = { Verb.COMMENTED: [Flag.AUTHOR, Flag.MODERATOR, Flag.REVIEWER], Verb.RATED_EXTENSION: [Flag.AUTHOR], Verb.REPORTED_EXTENSION: [Flag.MODERATOR], - Verb.REPORTED_REVIEW: [Flag.MODERATOR], + Verb.REPORTED_RATING: [Flag.MODERATOR], Verb.REQUESTED_CHANGES: [Flag.AUTHOR, Flag.MODERATOR, Flag.REVIEWER], Verb.REQUESTED_REVIEW: [Flag.MODERATOR, Flag.REVIEWER], } -- 2.30.2