From 1210445336605117c59c21cc45acb9c425dd82cf Mon Sep 17 00:00:00 2001 From: carofun Date: Wed, 16 Jun 2021 12:14:15 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(forum)=20fix=20sorting=20on=20stic?= =?UTF-8?q?ky=20and=20announcements=20topics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are different kind of topics: regular, sticky and announcements topics. Sticky topics are supposed to stay on top of the topic listing. Announcements are displayed in another block that is above the regular topics block and has its own header columns. Previous commit 3f92f4a enabled sorting topics on different header columns. This new feature introduces two different inconstancies. First, sticky topics were sorted as well whereas they are meant to stay on top of the listing. Second, columns of announcements block when the order was requested in URL showed toggling arrow as if they were sorted as well. We only want to sort regular topic discussions and not announcements. Announcements must stay with the default order which is ordered on the creation date. The commit introduces these two fixes. --- CHANGELOG.md | 3 + src/ashley/machina_extensions/forum/views.py | 3 +- .../forum_conversation/topic_list.html | 28 ++- .../forum/test_forum_view.py | 170 ++++++++++++++++++ 4 files changed, 197 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad10eb9a..7c5c17ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ Versioning](https://semver.org/spec/v2.0.0.html). - add django rest framework to promote/revoke moderators for current LTIContext - add new role moderator and permission to manage moderators +### Fixed + - fix sorting on sticky and announcements topics + ## [1.0.0-beta.5] - 2020-03-01 ### Changed diff --git a/src/ashley/machina_extensions/forum/views.py b/src/ashley/machina_extensions/forum/views.py index b5a633e1..2a46e8c1 100644 --- a/src/ashley/machina_extensions/forum/views.py +++ b/src/ashley/machina_extensions/forum/views.py @@ -72,7 +72,8 @@ def get_ordering(self): def get_queryset(self): """Returns the list of items for this view ordered by asked param.""" query = super().get_queryset() - return query.order_by(self.get_ordering_column()) + # Type of topic is kept as first order argument to keep sticky option + return query.order_by("-type", self.get_ordering_column()) def get_context_data(self, **kwargs): """Returns the context data to provide to the template.""" diff --git a/src/ashley/templates/forum_conversation/topic_list.html b/src/ashley/templates/forum_conversation/topic_list.html index 77b545e5..3fa2516f 100644 --- a/src/ashley/templates/forum_conversation/topic_list.html +++ b/src/ashley/templates/forum_conversation/topic_list.html @@ -9,23 +9,39 @@
-
+

+ {% if topics == announces %} + {% trans topic_list_title %} + {% else %} {% trans topic_list_title %} {% include "forum_conversation/partials/topic_list_header_sort.html" with col=header.0%} + {% endif %}

-
- {% trans "Replies" %} - {% include "forum_conversation/partials/topic_list_header_sort.html" with col=header.1%} +
+ {% if topics == announces %} + {% trans "Replies" %} + {% else %} + {% trans "Replies" %} + {% include "forum_conversation/partials/topic_list_header_sort.html" with col=header.1%} + {% endif %}
-
+
+ {% if topics == announces %} + {% trans "Views" %} + {% else %} {% trans "Views" %} {% include "forum_conversation/partials/topic_list_header_sort.html" with col=header.2%} + {% endif %}
-
+
+ {% if topics == announces %} + {% trans "Last post" %} + {% else %} {% trans "Last post" %} {% include "forum_conversation/partials/topic_list_header_sort.html" with col=header.3%} + {% endif %}
diff --git a/tests/ashley/machina_extensions/forum/test_forum_view.py b/tests/ashley/machina_extensions/forum/test_forum_view.py index fd803eca..19ddebd0 100644 --- a/tests/ashley/machina_extensions/forum/test_forum_view.py +++ b/tests/ashley/machina_extensions/forum/test_forum_view.py @@ -1,8 +1,13 @@ +import lxml.html # nosec from django.test import TestCase +from lxml import etree # nosec from machina.apps.forum_permission.shortcuts import assign_perm +from machina.core.db.models import get_model from ashley.factories import ForumFactory, PostFactory, TopicFactory, UserFactory +Topic = get_model("forum_conversation", "Topic") + class TestForumView(TestCase): """Displays a forum and its topics overridden from django machina.""" @@ -364,3 +369,168 @@ def test_testing_query_param(self): '', html=True, ) + + def test_testing_topic_is_sticky_stay_sticky_default_order(self): + """ + Request page with a sticky topic and check that the sticky topic is always the first + result on default order + """ + forum, url_list_topic = self._get_url_list_topic_with_three_topics() + # Creates a post for sticky topic + PostFactory( + topic=TopicFactory(forum=forum, type=Topic.TOPIC_STICKY), + subject="TOPIC STICKY ONE", + ) + + # Calls page with default order + response = self.client.get(url_list_topic) + + topicC = forum.topics.get(subject__startswith="TOPIC C the newest") + topicB = forum.topics.get(subject__startswith="TOPIC B the eldest") + topicA = forum.topics.get(subject__startswith="TOPIC A created second") + topicSticky = forum.topics.get(subject__startswith="TOPIC STICKY ONE") + + # Controls topics have been created in the order assumed in their titles + self.assertGreater(topicSticky.last_post_on, topicC.last_post_on) + self.assertGreater(topicC.last_post_on, topicA.last_post_on) + self.assertGreater(topicA.last_post_on, topicB.last_post_on) + + # Should be ordered by date by default with the sticky topic shown first + self.assertContentBefore(response, "TOPIC STICKY ONE", "TOPIC C the newest") + self.assertContentBefore( + response, "TOPIC C the newest", "TOPIC A created second" + ) + self.assertContentBefore( + response, "TOPIC A created second", "TOPIC B the eldest" + ) + + # Reverses the order, the sticky topic should remain first + response = self.client.get(f"{url_list_topic}?o=-0") + + self.assertContentBefore(response, "TOPIC STICKY ONE", "TOPIC B") + self.assertContentBefore(response, "TOPIC B", "TOPIC A ") + self.assertContentBefore(response, "TOPIC C", "TOPIC A") + + def test_testing_topic_is_sticky_stay_sticky_other_column_than_default(self): + """ + Request page with a sticky topic and check that the sticky topic is always + the first result even if it's sorted on column other than default one. The order + will be done on view counts column. + """ + forum, url_list_topic = self._get_url_list_topic_with_three_topics() + # Creates a post for the sticky topic + PostFactory( + topic=TopicFactory(forum=forum, type=Topic.TOPIC_STICKY, views_count=7), + subject="TOPIC STICKY ONE", + ) + + topic12 = forum.topics.get(subject__contains="12 views_count") + topic9 = forum.topics.get(subject__contains="9 views_count") + topic6 = forum.topics.get(subject__contains="6 views_count") + topicSticky = forum.topics.get(subject__startswith="TOPIC STICKY ONE") + # Confirms that the sticky topic has neither max or lowest view_count + self.assertGreater(topic12.views_count, topic9.views_count) + self.assertGreater(topic9.views_count, topicSticky.views_count) + self.assertGreater(topicSticky.views_count, topic6.views_count) + + # Orders on column view_post + response = self.client.get(f"{url_list_topic}?o=2") + + # Sticky should stay first, we compare it with the max and the min views_count + self.assertContentBefore(response, "TOPIC STICKY ONE", "12 views_count") + self.assertContentBefore(response, "TOPIC STICKY ONE", "6 views_count") + + # Reverses the order and confirms sticky topic stays on top + response = self.client.get(f"{url_list_topic}?o=-2") + + # Sticky should stay first, we compare it with the max and the min views_count + self.assertContentBefore(response, "TOPIC STICKY ONE", "6 views_count") + self.assertContentBefore(response, "TOPIC STICKY ONE", "12 views_count") + + def test_testing_topic_announce(self): + """Controls topics that are of type announcement don't have sorted options""" + # Creates posts for announcement topics + forum = ForumFactory() + PostFactory(topic=TopicFactory(forum=forum, type=Topic.TOPIC_ANNOUNCE)) + PostFactory(topic=TopicFactory(forum=forum, type=Topic.TOPIC_ANNOUNCE)) + + user = UserFactory() + assign_perm("can_read_forum", user, forum) + self.client.force_login(user) + + response = self.client.get(f"/forum/forum/{forum.slug}-{forum.pk}/") + + html = lxml.html.fromstring(response.content) + # Select the header block of the announcement block, the first block + announce_block = str( + etree.tostring(html.cssselect(".topiclist .card-header")[0]) + ) + + # Controls that announce_block is about announcements and not topics + self.assertIn("Announcements", announce_block) + self.assertNotIn("Topics", announce_block) + self.assertIn("Replies", announce_block) + self.assertIn("Views", announce_block) + self.assertIn("Last post", announce_block) + + # There's no sortable informations + self.assertNotIn("sortable sorted", announce_block) + # There's no column that has a sorting link on + self.assertNotIn("