From 17c50934bbbf364310b11d154d9245957879832f Mon Sep 17 00:00:00 2001 From: imperosol Date: Wed, 3 Sep 2025 13:55:07 +0200 Subject: [PATCH 1/2] fix: news notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Résout trois problèmes : - la création des notifications faisait un N+1 queries - le décompte du nombre de nouvelles à modérer était mauvais - modérer une nouvelle ne modifiait pas les notifications des autres admins --- com/models.py | 59 ++++++++++++++++++++------------- com/tests/test_notifications.py | 32 ++++++++++++++++-- sith/settings.py | 4 ++- 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/com/models.py b/com/models.py index ebc458e7..849c6b12 100644 --- a/com/models.py +++ b/com/models.py @@ -27,7 +27,7 @@ from django.conf import settings from django.core.exceptions import ValidationError from django.core.mail import EmailMultiAlternatives from django.db import models, transaction -from django.db.models import F, Q +from django.db.models import Exists, F, OuterRef, Q from django.shortcuts import render from django.templatetags.static import static from django.urls import reverse @@ -55,9 +55,17 @@ class Sith(models.Model): class NewsQuerySet(models.QuerySet): - def moderated(self) -> Self: + def published(self) -> Self: return self.filter(is_published=True) + def waiting_moderation(self) -> Self: + """Filter all non-finished non-published news""" + # Because of the way News and NewsDates are created, + # there may be some cases where this method is called before + # the NewsDates linked to a Date are actually persisted in db. + # Thus, it's important to filter by "not past date" rather than by "future date" + return self.filter(~Q(dates__start_date__lt=timezone.now()), is_published=False) + def viewable_by(self, user: User) -> Self: """Filter news that the given user can view. @@ -127,20 +135,28 @@ class News(models.Model): def save(self, *args, **kwargs): super().save(*args, **kwargs) - if self.is_published: - return - for user in User.objects.filter( - groups__id__in=[settings.SITH_GROUP_COM_ADMIN_ID] - ): - Notification.objects.create( - user=user, url=reverse("com:news_admin_list"), type="NEWS_MODERATION" + if not self.is_published: + admins_without_notif = User.objects.filter( + ~Exists( + Notification.objects.filter( + user=OuterRef("pk"), type="NEWS_MODERATION" + ) + ), + groups__id=settings.SITH_GROUP_COM_ADMIN_ID, ) + notif_url = reverse("com:news_admin_list") + new_notifs = [ + Notification(user=user, url=notif_url, type="NEWS_MODERATION") + for user in admins_without_notif + ] + Notification.objects.bulk_create(new_notifs) + self.update_moderation_notifs() def get_absolute_url(self): return reverse("com:news_detail", kwargs={"news_id": self.id}) def get_full_url(self): - return "https://%s%s" % (settings.SITH_URL, self.get_absolute_url()) + return f"https://{settings.SITH_URL}{self.get_absolute_url()}" def is_owned_by(self, user): if user.is_anonymous: @@ -159,19 +175,16 @@ class News(models.Model): or (user.is_authenticated and self.author_id == user.id) ) - -def news_notification_callback(notif: Notification): - # the NewsDate linked to the News - # which creation triggered this callback may not exist yet, - # so it's important to filter by "not past date" rather than by "future date" - count = News.objects.filter( - ~Q(dates__start_date__gt=timezone.now()), is_published=False - ).count() - if count: - notif.viewed = False - notif.param = str(count) - else: - notif.viewed = True + @staticmethod + def update_moderation_notifs(): + count = News.objects.waiting_moderation().count() + notifs_qs = Notification.objects.filter( + type="NEWS_MODERATION", user__groups__id=settings.SITH_GROUP_COM_ADMIN_ID + ) + if count: + notifs_qs.update(viewed=False, param=str(count)) + else: + notifs_qs.update(viewed=True) class NewsDateQuerySet(models.QuerySet): diff --git a/com/tests/test_notifications.py b/com/tests/test_notifications.py index fa541efb..8ddbfcb3 100644 --- a/com/tests/test_notifications.py +++ b/com/tests/test_notifications.py @@ -1,13 +1,22 @@ +from datetime import timedelta + import pytest from django.conf import settings +from django.utils.timezone import now from model_bakery import baker -from com.models import News +from com.models import News, NewsDate +from core.baker_recipes import subscriber_user from core.models import Group, Notification, User @pytest.mark.django_db def test_notification_created(): + # this news is unpublished, but is set in the past + # it shouldn't be taken into account when counting the number + # of news that are to be moderated + past_news = baker.make(News, is_published=False) + baker.make(NewsDate, news=past_news, start_date=now() - timedelta(days=1)) com_admin_group = Group.objects.get(pk=settings.SITH_GROUP_COM_ADMIN_ID) com_admin_group.users.all().delete() Notification.objects.all().delete() @@ -15,9 +24,28 @@ def test_notification_created(): for i in range(2): # news notifications are permanent, so the notification created # during the first iteration should be reused during the second one. - baker.make(News) + baker.make(News, is_published=False) notifications = list(Notification.objects.all()) assert len(notifications) == 1 assert notifications[0].user == com_admin assert notifications[0].type == "NEWS_MODERATION" assert notifications[0].param == str(i + 1) + + +@pytest.mark.django_db +def test_notification_edited_when_moderating_news(): + com_admin_group = Group.objects.get(pk=settings.SITH_GROUP_COM_ADMIN_ID) + com_admins = subscriber_user.make(_quantity=3) + com_admin_group.users.set(com_admins) + Notification.objects.all().delete() + news = baker.make(News, is_published=False) + assert Notification.objects.count() == 3 + assert Notification.objects.filter(viewed=False).count() == 3 + + news.is_published = True + news.moderator = com_admins[0] + news.save() + # when the news is moderated, the notification should be marked as read + # for all admins + assert Notification.objects.count() == 3 + assert Notification.objects.filter(viewed=False).count() == 0 diff --git a/sith/settings.py b/sith/settings.py index b095a114..d2f152c5 100644 --- a/sith/settings.py +++ b/sith/settings.py @@ -686,8 +686,10 @@ SITH_NOTIFICATIONS = [ # The keys are the notification names as found in SITH_NOTIFICATIONS, and the # values are the callback function to update the notifs. # The callback must take the notif object as first and single argument. +# If a notification is permanent but requires no post-action, set the +# callback import string as None SITH_PERMANENT_NOTIFICATIONS = { - "NEWS_MODERATION": "com.models.news_notification_callback", + "NEWS_MODERATION": None, "SAS_MODERATION": "sas.models.sas_notification_callback", } From cb454935ad2d07ee10c573fb4ae6998704c0a443 Mon Sep 17 00:00:00 2001 From: imperosol Date: Wed, 3 Sep 2025 14:00:09 +0200 Subject: [PATCH 2/2] fix: N+1 queries on ICS generation --- com/ics_calendar.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com/ics_calendar.py b/com/ics_calendar.py index 6cb10d2d..b0a2da5b 100644 --- a/com/ics_calendar.py +++ b/com/ics_calendar.py @@ -68,7 +68,7 @@ class IcsCalendar: start=news_date.start_date, end=news_date.end_date, url=as_absolute_url( - reverse("com:news_detail", kwargs={"news_id": news_date.news.id}) + reverse("com:news_detail", kwargs={"news_id": news_date.news_id}) ), ) calendar.events.append(event)