Skip to content
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

Open
wants to merge 34 commits into
base: dev
Choose a base branch
from

Conversation

Rowin
Copy link
Contributor

@Rowin Rowin commented Mar 2, 2021

Les fonctions get_last_post et get_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'attribute last_message mais effectue une requête pour retourner le dernier post visible
  • forum.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é

  • Se connecter comme staff
  • Masquer le dernier message d'un topic
  • Sur la vue du forum, le dernier message du topic doit être le message précédent celui qui vient d'être masqué
  • Démasquer le message masqué précédemment
  • Sur la vue du forum, le dernier message du topic doit à nouveau être le message démasqué

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 !

Rowin added 2 commits March 2, 2021 01:12
get_last_post et get_last_message tiennent compte des postes masqués
@coveralls
Copy link

coveralls commented Mar 2, 2021

Coverage Status

Coverage increased (+0.01%) to 86.712% when pulling 046f4bf on Rowin:feature-lastposthidden into 113eee1 on zestedesavoir:dev.

@AmauryCarrade
Copy link
Member

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 .

  1. last_message était en cache, donc il faut voir que ça ne pose pas de soucis de performances que de le transformer ainsi.
  2. Il faudra vérifier que l'anti-flood (qui accède au dernier message) fonctionne toujours correctement lorsque le dernier message de l'auteur est masqué (pas sûr qu'on ait un test pour ça).
  3. Enfin, mais là encore je n'ai pas encore eu le temps de QA, il serait bien de trier les messages par dernier message visible sur la liste des sujets, comme ça les sujets avec un dernier message masqué ne remontent pas non plus dans la liste elle-même.

Si jamais il y a des soucis avec la redéfinition de last_message(), le plus simple est probablement de faire une propriété last_visible_message() et de l'utiliser là où c'est pertinent (typiquement dans le gabari de la liste des sujets et des forums).


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.

Copy link
Member

@AmauryCarrade AmauryCarrade left a 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 ;
    image
  • l'ordre des messages du forum : il serait mieux, je pense, de garder l'ordre chronologique en ignorant les messages masqués.
    image

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 !

@AmauryCarrade AmauryCarrade changed the title Feature lastposthidden Le forum évite de remonter les sujets dont le dernier message est masqué Mar 4, 2021
entwanne and others added 19 commits March 7, 2021 17:57
* 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
@AmauryCarrade AmauryCarrade self-requested a review March 8, 2021 10:31
@Situphen Situphen self-requested a review April 15, 2021 18:17
Copy link
Member

@Situphen Situphen left a 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 ! :)

zds/forum/models.py Outdated Show resolved Hide resolved
zds/utils/templatetags/topics_sort.py Outdated Show resolved Hide resolved
zds/forum/models.py Outdated Show resolved Hide resolved
@Situphen Situphen self-requested a review April 27, 2021 21:06
AmauryCarrade
AmauryCarrade previously approved these changes May 18, 2021
Copy link
Member

@AmauryCarrade AmauryCarrade left a 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.

Copy link
Member

@Situphen Situphen left a 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 :

Le mieux est donc probablement que tu modifies ceci :

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).

zds/forum/models.py Outdated Show resolved Hide resolved
Rowin added 2 commits May 25, 2021 00:28
Lorsque le dernier message visible était modifié, il était remonté. Ce n'est pas le comportement attendu. Correction de ce comportement.
@Rowin
Copy link
Contributor Author

Rowin commented May 24, 2021

@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 !

@Rowin Rowin requested a review from Situphen June 11, 2021 12:37
@Arnaud-D
Copy link
Contributor

@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.

@Situphen
Copy link
Member

@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 :)

Copy link
Member

@philippemilink philippemilink left a 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 %}
Copy link
Member

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 ?

Comment on lines +242 to +243
last_visible_post = self.get_last_visible_post()
return last_visible_post.pubdate
Copy link
Member

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.

@Situphen Situphen removed their request for review April 16, 2023 16:46
@Situphen Situphen dismissed their stale review April 16, 2023 16:46

Je ne met aucun veto à ce qu'elle soit fusionné si quelqu'un juge que c'est bon :)

@philippemilink philippemilink added the S-Zombie Ticket ou PR oubliée label May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Back Concerne le back-end Django S-Évolution Ajoute de nouvelles fonctionnalités S-Zombie Ticket ou PR oubliée
Projects
Status: En attente de reprise
Development

Successfully merging this pull request may close these issues.

Éviter de remonter les sujets du forum dont le dernier message est un spam
9 participants