From e094434de685e0fc6e59a81aef6b54110cdd1cbd Mon Sep 17 00:00:00 2001 From: Vincent Porte Date: Thu, 19 Dec 2024 16:22:52 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20=F0=9F=A6=87=20d=C3=A9sactiver=20la=20su?= =?UTF-8?q?ppression=20en=20masse=20des=20`Post`=20depuis=20l'admin=20djan?= =?UTF-8?q?go=20(#861)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description 🎸 Il est apparu un cas de `Topic` où `posts_count` était supérieur à zéro après la suppression du `Post` associé via l'admin. 🎸 Ce `Topic` a fait échouer en erreur 500, l'affichage des listes de `Topic`, lors de la tentative d'affichage des données de détail du `first_post` 🪭 Issue #860, https://inclusion.sentry.io/issues/14670791/?project=4508410606452816 🪭 Cas reproduit en supprimant des `Post` depuis la liste dans l'admin 🪭 Cause identifiée : la méthode `delete_selected` n'appelle pas la méthode `delete` de `machina` sur ces objets. Les données dénormalisées des `Topic` associés ne sont pas mises à jour. 🧦 Patch : desactivation de la méthode `delete_selected` dans l'admin ## Type de changement 🪲 Correction de bug (changement non cassant qui corrige un problème). 🚧 technique ### Points d'attention 🦺 factorisation des paramètres commun des queryset `unanswered` et `optimized_for_topics_list` 🦺 mise à jour des tests impactés et ajout du trait `with_disapproved_post`, réécriture simplifiée en pytest 🦺 des cas similaires peuvent se produire sur les `Topic` vs les données dénormalisées des `Forum`. Celles-ci ne sont pas exploitées dans l'UI, pas d'incidence pour le moment. --- lacommunaute/forum/tests/tests_model.py | 34 ++++++++++++------- lacommunaute/forum_conversation/admin.py | 16 +++++++-- lacommunaute/forum_conversation/factories.py | 5 +++ lacommunaute/forum_conversation/models.py | 21 ++++++------ .../forum_conversation/tests/tests_models.py | 30 ++++++++-------- 5 files changed, 65 insertions(+), 41 deletions(-) diff --git a/lacommunaute/forum/tests/tests_model.py b/lacommunaute/forum/tests/tests_model.py index dcd0a13ab..9d3a74584 100644 --- a/lacommunaute/forum/tests/tests_model.py +++ b/lacommunaute/forum/tests/tests_model.py @@ -1,3 +1,4 @@ +import pytest from django.conf import settings from django.test import TestCase @@ -7,22 +8,29 @@ from lacommunaute.users.factories import UserFactory -class ForumModelTest(TestCase): - def test_get_unanswered_topics(self): - topic1 = TopicFactory(forum=ForumFactory(), posts_count=1) - topic2 = TopicFactory(forum=ForumFactory(parent=topic1.forum), posts_count=1) - TopicFactory(forum=ForumFactory(), posts_count=1) +@pytest.fixture(name="forum") +def fixture_forum(db): + return ForumFactory() + + +@pytest.fixture(name="unanswered_topics_on_forum") +def fixture_unanswered_topics_on_forum(db, forum): + # unanswered topic from outer forum (undesired) + TopicFactory(forum=ForumFactory(), with_post=True) + + return [TopicFactory(forum=forum, with_post=True), TopicFactory(forum=ForumFactory(parent=forum), with_post=True)] - unanswered_topics = topic1.forum.get_unanswered_topics() - self.assertEqual(unanswered_topics.count(), 2) - self.assertIn(topic1, unanswered_topics) - self.assertIn(topic2, unanswered_topics) - def test_count_unanswered_topics(self): - topic = TopicFactory(forum=ForumFactory(), posts_count=1) - TopicFactory(forum=ForumFactory(parent=topic.forum), posts_count=1) - self.assertEqual(topic.forum.count_unanswered_topics, 2) +class TestForumModel: + def test_get_unanswered_topics(self, db, forum, unanswered_topics_on_forum): + for topic in forum.get_unanswered_topics(): + assert topic in unanswered_topics_on_forum + def test_count_unanswered_topics(self, db, forum, unanswered_topics_on_forum): + assert forum.count_unanswered_topics == len(unanswered_topics_on_forum) + + +class ForumModelTest(TestCase): def test_get_absolute_url(self): forum = ForumFactory() self.assertEqual( diff --git a/lacommunaute/forum_conversation/admin.py b/lacommunaute/forum_conversation/admin.py index 8e7aa1a0e..2d12efdd8 100644 --- a/lacommunaute/forum_conversation/admin.py +++ b/lacommunaute/forum_conversation/admin.py @@ -1,9 +1,19 @@ from django.contrib import admin -from machina.apps.forum_conversation.admin import TopicAdmin as BaseTopicAdmin +from machina.apps.forum_conversation.admin import PostAdmin as BasePostAdmin, TopicAdmin as BaseTopicAdmin from lacommunaute.forum_conversation.models import CertifiedPost, Post, Topic +class PostAdmin(BasePostAdmin): + def get_actions(self, request): + # delete_selected action does not call delete method of the model, so Related topic is not updated. + # When the last post of a topic is deleted, topic.posts_count remains to 1, saying ambiguous information. + # So we remove the delete_selected action to force the user to delete posts one by one. + return [] + + list_filter = BasePostAdmin.list_filter + ("approved",) + + class PostInline(admin.StackedInline): model = Post list_display = ("__str__", "poster", "updated", "approved") @@ -22,7 +32,7 @@ class TopicAdmin(BaseTopicAdmin): inlines = [ PostInline, ] - list_filter = BaseTopicAdmin.list_filter + ("type",) + list_filter = BaseTopicAdmin.list_filter + ("type", "approved") class CertifiedPostAdmin(admin.ModelAdmin): @@ -34,6 +44,8 @@ class CertifiedPostAdmin(admin.ModelAdmin): ) +admin.site.unregister(Post) +admin.site.register(Post, PostAdmin) admin.site.unregister(Topic) admin.site.register(Topic, TopicAdmin) admin.site.register(CertifiedPost, CertifiedPostAdmin) diff --git a/lacommunaute/forum_conversation/factories.py b/lacommunaute/forum_conversation/factories.py index cdd4c0ebe..2caaff629 100644 --- a/lacommunaute/forum_conversation/factories.py +++ b/lacommunaute/forum_conversation/factories.py @@ -58,6 +58,11 @@ class Params: poster=factory.SelfAttribute("topic.poster"), ) ) + with_disapproved_post = factory.Trait( + post=factory.RelatedFactory( + PostFactory, factory_related_name="topic", poster=factory.SelfAttribute("topic.poster"), approved=False + ) + ) @factory.post_generation def with_poll_vote(self, create, extracted, **kwargs): diff --git a/lacommunaute/forum_conversation/models.py b/lacommunaute/forum_conversation/models.py index 648a8fd06..6f1eda481 100644 --- a/lacommunaute/forum_conversation/models.py +++ b/lacommunaute/forum_conversation/models.py @@ -16,12 +16,9 @@ class TopicQuerySet(models.QuerySet): - def unanswered(self): + def _for_topics_list(self): return ( self.exclude(approved=False) - .exclude(status=Topic.TOPIC_LOCKED) - .exclude(type=Topic.TOPIC_ANNOUNCE) - .filter(posts_count=1) .select_related( "forum", "poster", @@ -32,16 +29,19 @@ def unanswered(self): .order_by("-last_post_on") ) + def unanswered(self): + return ( + self._for_topics_list() + .exclude(status=Topic.TOPIC_LOCKED) + .exclude(type=Topic.TOPIC_ANNOUNCE) + .filter(posts_count=1) + ) + def optimized_for_topics_list(self, user_id): return ( - self.exclude(approved=False) + self._for_topics_list() .filter(type__in=[Topic.TOPIC_POST, Topic.TOPIC_STICKY, Topic.TOPIC_ANNOUNCE]) .select_related( - "forum", - "poster", - "poster__forum_profile", - "first_post", - "first_post__poster", "certified_post", "certified_post__post", "certified_post__post__poster", @@ -54,7 +54,6 @@ def optimized_for_topics_list(self, user_id): "certified_post__post__attachments", "tags", ) - .order_by("-last_post_on") ) diff --git a/lacommunaute/forum_conversation/tests/tests_models.py b/lacommunaute/forum_conversation/tests/tests_models.py index f74169e7a..fddf87ac4 100644 --- a/lacommunaute/forum_conversation/tests/tests_models.py +++ b/lacommunaute/forum_conversation/tests/tests_models.py @@ -1,12 +1,9 @@ -from datetime import datetime, timezone - import pytest from django.conf import settings from django.core.exceptions import ValidationError from django.db import IntegrityError from django.test import TestCase from django.urls import reverse -from factory import Iterator from lacommunaute.forum.factories import ForumFactory from lacommunaute.forum_conversation.factories import ( @@ -21,6 +18,11 @@ from lacommunaute.users.factories import UserFactory +@pytest.fixture(name="forum") +def fixture_forum(db): + return ForumFactory() + + class PostModelTest(TestCase): def test_username_is_emailfield(self): topic = TopicFactory() @@ -37,10 +39,8 @@ def test_is_certified(self): self.assertTrue(topic.last_post.is_certified) -class TopicManagerTest(TestCase): - def test_unanswered(self): - forum = ForumFactory() - +class TestTopicManager: + def test_unanswered(self, db, forum): TopicFactory(forum=forum, posts_count=0) topic = TopicFactory(forum=forum, posts_count=1) TopicFactory(forum=forum, posts_count=2) @@ -49,17 +49,17 @@ def test_unanswered(self): TopicFactory(forum=forum, posts_count=1, type=Topic.TOPIC_ANNOUNCE) TopicFactory(forum=forum, posts_count=1, approved=False) - self.assertEqual(Topic.objects.unanswered().get(), topic) + assert Topic.objects.unanswered().get() == topic - def test_unanswered_order(self): - forum = ForumFactory() - last_post_dates = [datetime(2025, 5, i, tzinfo=timezone.utc) for i in range(20, 24)] + def test_unanswered_order(self, db, forum): + topics = TopicFactory.create_batch(size=3, forum=forum, with_post=True) + expected_date_list = [topic.last_post_on for topic in reversed(topics)] + extracted_date_list = list(Topic.objects.unanswered().values_list("last_post_on", flat=True)) - TopicFactory.create_batch( - size=len(last_post_dates), forum=forum, posts_count=1, last_post_on=Iterator(last_post_dates) - ) - assert list(Topic.objects.unanswered().values_list("last_post_on", flat=True)) == last_post_dates[::-1] + assert extracted_date_list == expected_date_list + +class TopicManagerTest(TestCase): def test_optimized_for_topics_list_disapproved(self): TopicFactory(approved=False) self.assertEqual(Topic.objects.optimized_for_topics_list(1).count(), 0)