Skip to content

Commit

Permalink
fix: 🦇 désactiver la suppression en masse des Post depuis l'admin d…
Browse files Browse the repository at this point in the history
…jango (#861)

## 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.
  • Loading branch information
vincentporte authored Dec 19, 2024
1 parent 66ca0f5 commit e094434
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 41 deletions.
34 changes: 21 additions & 13 deletions lacommunaute/forum/tests/tests_model.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import pytest
from django.conf import settings
from django.test import TestCase

Expand All @@ -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(
Expand Down
16 changes: 14 additions & 2 deletions lacommunaute/forum_conversation/admin.py
Original file line number Diff line number Diff line change
@@ -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")
Expand All @@ -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):
Expand All @@ -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)
5 changes: 5 additions & 0 deletions lacommunaute/forum_conversation/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
21 changes: 10 additions & 11 deletions lacommunaute/forum_conversation/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -54,7 +54,6 @@ def optimized_for_topics_list(self, user_id):
"certified_post__post__attachments",
"tags",
)
.order_by("-last_post_on")
)


Expand Down
30 changes: 15 additions & 15 deletions lacommunaute/forum_conversation/tests/tests_models.py
Original file line number Diff line number Diff line change
@@ -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 (
Expand All @@ -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()
Expand All @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit e094434

Please sign in to comment.