-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Le forum évite de remonter les sujets dont le dernier message est masqué #6056
base: dev
Are you sure you want to change the base?
Conversation
get_last_post et get_last_message tiennent compte des postes masqués
Salut, et bienvenue sur le projet ! Merci pour cette contribution qui sera fort utile pour l'ergonomie du forum :) . Je n'ai pas encore pris le temps de faire une revue extensive, mais je note quelques points auxquels il faudra faire attention. Peut-être que certains sont déjà OK, et alors… tant mieux :p .
Si jamais il y a des soucis avec la redéfinition de P.-S. Si jamais ça peut t'intéresser, on discute aussi du projet de façon plus directe et informelle sur le serveur Discord non-officiel de ZdS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bon, du coup j'ai QA. ❌ NOK, mais c'est un bon début !
Ce qui fonctionne :
- tout ce qui est indiqué dans la QA : le bon message est bien affiché sans souci.
Ce qui ne fonctionne pas :
- l'anti-flood est effectivement cassé : ici, on ne devrait pas pouvoir poster ;
- l'ordre des messages du forum : il serait mieux, je pense, de garder l'ordre chronologique en ignorant les messages masqués.
Faire comme suggéré au précédent commentaire fonctionnerait, je pense, avec une méthode dédiée pour récupérer le dernier message non-masqué, utilisée dans les gabarits des listes de sujets et de forum (car dans la liste des forums, il faudrait aussi afficher le dernier sujet en prenant en compte la date du dernier message non-masqué).
On pourrait envisager de mettre ce message là (non-masqué) en cache également, tout comme le dernier message tout court, ou même à la place, éventuellement, s'il n'y a pas d'usage critique du dernier message tout court.
Ça impliquerait une migration qui enregistrerait le-dit dernier message — n'hésite pas si tu veux t'y coller, on peut aider sans souci si tu débutes avec Django (j'ignore ton niveau exact) :) .
Il faudrait également modifier les vues des listes de sujets afin de trier sur la date du dernier message visible, histoire d'éviter d'avoir des ordres chronologiques étranges et de bien répondre à la demande de #5980.
Enfin, vu que ça ne semble pas te faire peur, ajouter un test pour ce cas limite de l'anti-flood (s'assurer que l'envoi est bien bloqué même si le dernier message posté est masqué) serait pertinent, je pense.
Merci encore pour tout ça !
* Fix 6047 Nouvelle url plus simple pour le profil utilisateur Redirection de l'ancienne url * Ajout des tests * Mise à jour du test vérifiant l'url du profil * Enlever une ligne (commentée) de l'ancien code Co-authored-by: Situphen <[email protected]> * Changement du template pour afficher la bonne url * Changement de l'URL du profile inscrite en dur Co-authored-by: Situphen <[email protected]>
Ajout d'une propriété "update" pour le Topic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est du très beau travail @Rowin !
Ceci dit étant donné que l'on touche ici aux messages des forums il ne faut vraiment pas faire d'erreurs c'est pourquoi la phase de QA prend du temps ! :)
Utilisation de Post.update et Post.pubdate à la place de Post.update_index_date
…to feature-lastposthidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ QA OK
Désolé pour le délai, j'ai passé un petit temps sur d'autres choses. Mais j'ai relu le code et testé le comportement et cette fois tout me semble bon : l'ordre est OK, et l'antispam fonctionne correctement. Merci pour cette contribution !
Je ne fusionne pas tout de suite car @Situphen désirait revoir le code lui aussi, mais pour moi c'est ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je me rends compte que cette PR n'est vraiment pas triviale, il y a encore quelques changements à effectuer !
Actuellement tu tries l'ordre des sujets dans le gabarit avec le templatetag topics_sort
mais ça ne peut pas fonctionner correctement car la liste des sujets est paginée.
Prenons le cas de cinq sujets :
- Sujet A dont le dernier message est il y a 10 heures ;
- Sujet B dont le dernier message est il y a 11 heures ;
- Sujet C dont le dernier message est il y a 12 heures ;
- Sujet D dont le dernier message est il y a 13 heures ;
- Sujet E dont le dernier message est il y a 14 heures.
Si la pagination est configurée pour n'afficher que 3 sujets par page, les sujets affichés sur la première page sont dans l'ordre Sujet E, Sujet D et Sujet C.
Si le dernier message de Sujet B est édité, les sujets affichés sur la première page sont dans l'ordre Sujet B, Sujet E et Sujet D.
Si ensuite le dernier message de Sujet est masqué, on s'attend à ce que les sujets affichés sur la première page redeviennent dans l'ordre Sujet E, Sujet D et Sujet C. Or ce n'est pas le cas actuellement avec ta PR, les sujets affichés sur la première page sont Sujet E, Sujet D, Sujet B.
Étant donné que la page est paginée, la liste topics
dans templates/forum/category/forum.html
ne contient que dans l'ordre Sujet B, Sujet E et Sujet D, ce que fait que topics|topics_sort
contient dans l'ordre Sujet E, Sujet D, Sujet B.
Si tu veux reproduire cela chez toi, il faut modifier topics_per_page
dans zds/abstract_base/zds.py
pour modifier le nombre de sujets affichés par page.
Il faudrait donc que tu fasses ce tri avant la pagination. Les requêtes sont :
- pour la liste des sujets épinglés
Line 124 in 415038c
Topic.objects.get_all_topics_of_a_forum(self.object.pk, is_sticky=True), context["filter"] - pour le reste des sujets
Line 158 in 415038c
self.queryset = Topic.objects.get_all_topics_of_a_forum(self.object.pk)
Le mieux est donc probablement que tu modifies ceci :
zds-site/zds/forum/managers.py
Lines 93 to 100 in 415038c
def get_all_topics_of_a_forum(self, forum_pk, is_sticky=False): | |
return ( | |
self.filter(forum__pk=forum_pk, is_sticky=is_sticky) | |
.order_by("-last_message__pubdate") | |
.select_related("author__profile") | |
.prefetch_related("last_message", "tags") | |
.all() | |
) |
Il faudra voir si les changements posent des soucis de performance ou pas (notamment regarder si le nombre de requêtes SQL augmente ou pas).
Lorsque le dernier message visible était modifié, il était remonté. Ce n'est pas le comportement attendu. Correction de ce comportement.
@Situphen je pense avoir trouvé une solution propre pour gérer ce cas, directement dans le manager et sans rajouter de requêtes en rajoutant une annotation qui contient la date. Ça m'évite d'avoir à gérer une persistance ou a rajouter des requêtes supplémentaires ! |
@Situphen tu auras le temps de repasser ici ? C'était toi qui avait fait la QA à l'époque et j'ai l'impression que c'est presque bon. Sauf peut-être l'histoire de performance, je ne sais pas si c'est facile à QAiser. |
Je t'avoue qu'en ce moment le développement technique de ZDS ne m'intéresse pas trop, donc il est peu probable que je finisse la QA de cette PR. Libre à toi ou quelqu'un d'autre d'en faire la QA ! Je ne met aucun veto à ce qu'elle soit fusionné si quelqu'un juge que c'est bon :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deux petites questions sur le code, et il faudrait régler le conflit sur le fichier zds/utils/tests/test_misc.py
.
Une fois que ces modifications seront faites, je testerai, ça me semble bien.
PS : @Rowin puisque cette PR est ouverte depuis longtemps, dis-le si tu n'as pas le temps / l'envie / la motivation / ... de faire les modifications demandées, pour qu'on sache de notre côté comment considérer cette PR :)
@@ -86,7 +87,7 @@ | |||
|
|||
{% if topics %} | |||
<div class="topic-list navigable-list" itemscope itemtype="http://schema.org/ItemList"> | |||
{% for topic in topics %} | |||
{% for topic in topics|topics_sort %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Puisque le tri est fait dans get_all_topics_of_a_forum()
, on n'a plus besoin de trier ici, non ?
last_visible_post = self.get_last_visible_post() | ||
return last_visible_post.pubdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.get_last_visible_post()
peut retourner None
, dans ce cas, on ne peut pas accéder à l'attribut pubdate
et ça va générer une erreur.
Je ne met aucun veto à ce qu'elle soit fusionné si quelqu'un juge que c'est bon :)
Les fonctions
get_last_post
etget_last_message
tiennent désormais compte de la visibilité des messages pour ne renvoyer que le dernier message visible.En particulier :
forum.models.Topic.get_last_post
ne renvoie plus l'attributelast_message
mais effectue une requête pour retourner le dernier post visibleforum.models.Forum.get_last_message
tient compte de la visibilité dans la requête qui était déjà effectuée.Numéro du ticket concerné (optionnel) : #5980
Contrôle qualité
C'est ma première PR ici (et presque ma première de manière générale), n'hésitez pas à me dire si des choses ne vont pas !