Refactoring du module des forums

[GerardPaligot]

Q	R
Correction de bugs ?	Non
Nouvelle Fonctionnalité ?	Oui
Tickets (issues) concernés	ZEP-30

Que dois faire la QA :

• Lister les catégories avec ses forums
• Lister les forums d'une catégorie
• Lister les topics d'un forum
• Lister les topics d'un membre
• Lister les topics d'un tag
• Créer un topic
• Editer un topic (inclus marquer résolu, suivre, notifié par e-mail et les actions de modération)
• Lister les messages d'un topic
• Lister les messages d'un membre
• Créer une réponse
• Editer une réponse
• Mentionner utile une réponse
• Marquer un message comme non lu
• Aimer ou non un message

Voilà, n'hésitez pas à chercher les cas tordus et have fun!

[coveralls]

Coverage increased (+1.86%) to 83.77% when pulling 2d3f53a on GerardPaligot:refacto_forum into 85e62f9 on zestedesavoir:dev.

[landscape-bot]

Repository health increased by 0.54% when pulling 2091f1b on GerardPaligot:refacto_forum into 2472280 on zestedesavoir:dev.

• 48 new problems were found (including 0 errors and 5 code smells).
• 28 problems were fixed (including 0 errors and 16 code smells).

[pierre-24]

Une chance que ça améliore les perfs globale ? (voir #2623). C'est pas une obligation, mais puisque t'es dans le coin ...

[GerardPaligot]

A ce jour, j'ai pas refactorisé en gardant en tête d'améliorer les performances. Juste de refactoriser les anciennes vues vers CBV.

[pierre-24]

Je m'en doutais, mais c'est pas grave. Garde le ticket en tête, ceci étant dit ^^

[Eskimon]

Alors voici le resultat de mes tests.

Globalement tout roule SAUF:

• La pagination qui ne marche pas dans "liste des messages postes" dans un profil (uniquement les boutons suivant/precedent sont cassee, les chiffres semblent ok eux)
• Dans un sujet, quand on change de page on a la reprise du premier message mais ce dernier n'est pas grisé et indique comme "reprise du dernier message"

J'espere ne rien avoir laisse passer...

[Eskimon]

Ah si aussi quand on met un filtre dans la liste des sujets (avec/sans reponses par exemple) ca marche au debut puis merde quand on change de page via le paginator

[landscape-bot]

Repository health increased by 0.54% when pulling 00708b6 on GerardPaligot:refacto_forum into c426cb7 on zestedesavoir:dev.

• 48 new problems were found (including 0 errors and 5 code smells).
• 28 problems were fixed (including 0 errors and 16 code smells).

[Eskimon]

Comme tu as trop la classe !! Tout marche tres bien ! Par contre landscape montre des trucs nouveau assez simple a corriger...

[GerardPaligot]

Voilà le caribou. :)

[landscape-bot]

Repository health increased by 0.73% when pulling 0cf5d40 on GerardPaligot:refacto_forum into c426cb7 on zestedesavoir:dev.

• 24 new problems were found (including 0 errors and 4 code smells).
• 30 problems were fixed (including 0 errors and 17 code smells).

[firm1]

Cette méthode mérite un commentaire. Car en lisant perform_solved on pourrait penser que la méthode se contente de mettre un topic en résolu. Mais ce n'est pas le cas, elle mets en résolu si celui-ci n'est pas encore résolu, et elle mets en "non-résolu" si le topic est résolu.

[GerardPaligot]

Je préfère mettre un nom de méthode plus explicite qui sera plus naturel de mettre à jour à l'avenir. Est-ce que perform_solve_or_unsolve te conviendrait ?

[firm1]

Est-ce que perform_solve_or_unsolve te conviendrait ?

tout à fait.

[firm1]

C'est pas voulu mais il faudrait définir Mixin. Savoir si c'est vraiment ou pas des mixins.

Je ne sais meme pas qu'elle définition on donnerait a un mixin, mais le fait que tu ait introduit cette nomenclature fait que je me demande pourquoi ce n'était pas fait sur les autres classes qui jouent le même role ?

"Les MP sont trop différents du forum pour être fusionnés, parce qu'il y a plein de règles de gestions différentes, donc tant pis pour le code en double"

il y'a plusieurs choses dans les MPs. Le modèle de donnée, les vues, et les templates.

Autant ça ne sert a rien de fusionner le modèle des MPs avec quoique ce soit (surtout pour des raisons de sécurité et de perfs), autant certaines vues ont un intéret ( @GerardPaligot en a fusionné quelques unes d'ailleurs dans cette PR ). Et il me semble que le code des template est déjà fusionné au maximum.

Mon commentaire donc se limite à la fusion de certains morceaux qui reviennent dans les vues comme l'édition d'un message, l'envoi d'un message, la citation d'un message, l'aperçu d'un message. En gros, un message du forum, n'est conceptuellement qu'une extension d'un message de mp.

[pierre-24]

Mon commentaire donc se limite à la fusion de certains morceaux qui reviennent dans les vues comme l'édition d'un message, l'envoi d'un message, la citation d'un message, l'aperçu d'un message. En gros, un message du forum, n'est conceptuellement qu'une extension d'un message de mp.

Je plussoie fortement ce commentaire, parce que je m'étais fait exactement la même réflexion pour les commentaires d'articles/tutos (l'idée de proposer une ZEP m'est même passée par la tête, mais je me suis arrêté sur le point des MPs, justement).

Par contre, gardez en tête que cette PR s'appelle "refactoring", et là on tape gentiment dans l'évolution. Je soutiens l'idée à 200%, mais ça demande une PR à part :)

[GerardPaligot]

@firm1 Modifications faites.

[landscape-bot]

Repository health increased by 0.74% when pulling 5f534ad on GerardPaligot:refacto_forum into 93559e3 on zestedesavoir:dev.

• 24 new problems were found (including 0 errors and 4 code smells).
• 30 problems were fixed (including 0 errors and 17 code smells).

[GerardPaligot]

Si ça va à tout le monde, @Eskimon tu peux merge.

@firm1, j'aimerais ton intervention avant pour valider la QA de la revue de code.

[Eskimon]

Chui chaud pour merge, j'attends juste @firm1 :)

[firm1]

Je regarde ça cette nuit promis.

Le dim. 17 mai 2015 09:37, Eskimon notifications@github.com a écrit :

Chui chaud pour merge, j'attends juste @firm1 https://github.com/firm1
:)

—
Reply to this email directly or view it on GitHub
#2697 (comment)
.

[firm1]

La PR est vraiment énorme. Je me suis concentré un peu plus sur les tests unitaires pour cette fois (désolé pas le temps de regarder plus). Voici les petites remarques :

• Certaines vérifications de codes sont passées de check d'une 302 à check d'une 405 (comme ici) cependant, quand on regarde bien, le but du test était surtout de vérifier que le like/dislike/useful passe. Et donc, il aurait été plus efficace je pense de changer le get en post pour tester ce cas. Et quand je lis plus, je vois que le fichier test_views.py resteste des cas déjà testés dans tests.py c'est voulu ? si oui, pourquoi ne pas virer les doublons de test ?
• On a énormément de duplication de lignes dans les tests (ce qui les rend presqu'illisible parfois). je ne compte plus le nombre de fois que j'ai vu la ligne profile = ProfileFactory() alors qu'il existe une méthode setUp(self): qui permet de factoriser cela. En gros tout ce qui se répète dans les tests pour poser le contexte, il vaut mieux le remonter dans cette méthode qui est éxecuter en before de toute les autres.
• certaines méthode telles que def add_topic_in_a_forum() ou def create_category ont plus leurs place dans le fichier factories.py que dans le fichier des tests. On peut en avoir besoin ailleurs que dans ce fichiers et c'est plus logique de le retrouver dans le fichier générateur de factories.
• Les tests ne vérifient pas tout ce qui a trait à la gestion des mails dans les forums. Pour un membre qui suit un topic par mail il y'a des uses cases que je ne vois pas : est-ce que le mec qui suit un topic par mail reçoit des mails a chaque nouveau post ?, celui qui ne follow pas le topic le reçoit lui ?, etc.
• Dans pas mal de cas (partout ou on a des 302) c'est bien de vérifier qu'on est redirigé, mais il faut en plus vérifier qu'on est redirigé sur la bonne url. Je ne l'ai pas vu dans les tests. D'autant plus que je vois pas mal de fois (ici par exemple) ou tu fais des post sans remplir le formulaire. On ne sait donc pas si la 302 renvoyée c'est à cause du formulaire invalide ou d'autre chose.
• Je ne vois pas de test qui vérifie le fait qu'on ne peut pas poster de message si on est LS, LS temporaire, Ban temporaire. Seul le cas du ban définitif est testé.
• ça manque globalement de test sur les données après l'action. Je prend l'exemple de cette fonction qui est censée déplacer un topic. Au delà du test de code retour, il faut vérifier que le topic a vraiment été déplacé selon les données en base. Et que tout ceux qui suivaient le topic ne le follow plus s'ils n'ont plus accès au forum.

Voilà, pour un premier jet quelques remarques qui s'appliquent aux tests en général.

Je crois que vu la taille du fichier de test_views, tu pourrais en profiter pour le splitter en plusieurs afin de le rendre lisible. Là, il faut le vouloir pour s'y retrouver.

[GerardPaligot]

• 302 -> 405 : Le but était justement le contraire pour ces fonctions. Montrer que le like/dislike/uselful ne passent pas. Du moins, c'est le cas pour l'exemple que tu me donnes et pour les quelques suivants que je viens de regarder. Par contre, nous sommes d'accord pour dire que le test d'origine n'était pas bon puisqu'il n'a jamais été possible de faire du GET sur ces URLs. Le code d'erreur était simplement différent parce qu'il était arrêté avant par une autre permission.
• Duplication : Je n'utilise setUp uniquement si j'ai quelque chose de commun à tous mes tests sinon je vais créer un nombre incroyable de requêtes en base pour chaque test sans en avoir besoin et je ne maitriserais plus parfaitement l'environnement de mes tests. Donc non, c'est pas une bonne idée.
• Méthodes utilitaires : Je m'étais posé la question et je suis content que tu penses comme moi. Je vais les mettre dans factory.py.
• Le reste : Globalement, je n'étais pas obligé de rajouter des tests (comme je l'ai fais au refactoring des MPs) mais j'ai décidé d'en rajouter pour tester les vues (et j'insiste sur ce mot) CBV et non pas directement le business (bien qu'il a fallu le faire dans certains tests pour être certain que j'étais bien stoppé). Dès le départ, je suis parti de principe que les tests fonctionnels couvraient une grande majorité des fonctionnalités. Je ne peux décemment pas à moi tout seul faire monter la couverture de tests sur le module des forums à 100%. Forcément, il y aura des choses non testées mais qui seront testées à l'avenir.

[firm1]

le test d'origine n'était pas bon puisqu'il n'a jamais été possible de faire du GET sur ces URLs.

Justement, avant (juste avant la sortie en 1.0) c'était des GET qui étaient fait là dessus. Le problème est que le test n'a pas été maintenu quand c'est passé a du POST

Je n'utilise setUp uniquement si j'ai quelque chose de commun à tous mes tests

Je trouve que ça nuit pas mal à la lisibilité des tests, et ça rallonge inutilement le code, et je n'imagine même pas ce que ça doit donner comme millefeuille lorsqu'on veut faire bouger une règle de gestion, il faut retrouver tous les contextes dans chaque mini-fonction. Après, je ne fais que donner mon avis, tu en fais ce que tu veux derrière.

Je ne peux décemment pas à moi tout seul faire monter la couverture de tests sur le module des forums à 100%

Je suis bien d'accord. Mais vu que tu refais les vues du modules, je trouve ça normal de tester tous les cas introduis par ton code. Ce n'est pas a toi que je vais vendre l'utilité des tests dans un projet. ça serait dommage qu'on découvre un bug qui aurait pu être trouvé avec un test unitaire.

[GerardPaligot]

Je trouve que ça nuit pas mal à la lisibilité des tests, et ça rallonge inutilement le code, et je n'imagine même pas ce que ça doit donner comme millefeuille lorsqu'on veut faire bouger une règle de gestion, il faut retrouver tous les contextes dans chaque mini-fonction. Après, je ne fais que donner mon avis, tu en fais ce que tu veux derrière.

Pour tenter de te convaincre, plusieurs points qui me séduisent dans ma manière de faire :

• Mes tests sont unitaires. Quand tu en consultes un, tu as une vision clair de ce que tu possèdes et des assertions qui touche les choses exacts du test. Tu n'es pas obligé de remonter à la méthode setUp de ta classe pour voir quelles sont les attributs que tu utilises (je rappelle que les noms sont du type variable1, variable2, etc. dans les autres tests, très parlant !) et de comprendre comment ils sont initialisés.
• Je ne test pas les règles métiers mais des fonctionnalités. C'est-à-dire que je vais tester qu'un member ne peut pas éditer le titre d'un sujet s'il n'est pas l'auteur. Si cette fonctionnalité change, il n'y aura que ce test qui ne passera plus. Les autres passeront toujours puisqu'ils ne testent pas cette fonctionnalité et que l'environnement que j'utilise ne correspond pas (c'est bien entendu la théorie, l'erreur est humaine).

Je suis bien d'accord. Mais vu que tu refais les vues du modules, je trouve ça normal de tester tous les cas introduis par ton code. Ce n'est pas a toi que je vais vendre l'utilité des tests dans un projet. ça serait dommage qu'on découvre un bug qui aurait pu être trouvé avec un test unitaire.

Nous sommes d'accord pour dire que les tests c'est important dans un projet mais ce refactoring touche à tout. Autrement dit, tu me demandes une couverture à 100%. Tout en sachant que les anciens tests sont toujours valides et test un bon nombre de choses.

Tout ceci étant dit firm1, je pense que nous avons une façon de tester complètement différente (puisque tu es à l'origine d'un grand nombre de test dans ZdS), nous ne pourrons pas nous mettre complètement d'accord je pense. Si vraiment tu n'es pas d'accord, je laisse @SpaceFox trancher sur la question.

[Eskimon]

Si je suis bien la conversation, on a une différence d'approche/philosophie. Ca arrive, c'est normal. Mais le coeur du problème aussi est : Le code/la facon de faire est-il ok pour tout le monde en mettant en marche sa tolérance personnelle sur l'approche à avoir

[GerardPaligot]

Je laisse @firm1 s'exprimer là dessus et @SpaceFox trancher s'il faut.

[firm1]

Le code/la facon de faire est-il ok pour tout le monde en mettant en marche sa tolérance personnelle sur l'approche à avoir

Mon commentaire mélange à la fois l'approche et la couverture de test. Je conçois que chacun peut avoir une approche différente. Cependant pour la question de la couverture de tests j'ai noté celles qui manquent, car mine de rien on a une refacto sur un gros module et le risque de bug est elevé. Donc je laisse @SpaceFox juger si ça lui parait assez.

[Eskimon]

Si je ne m'abuse la couverture augmente de 0.75%, donc @GerardPaligot fait même mieux qu'avant non ?

[firm1]

Si je ne m'abuse la couverture augmente de 0.75%, donc @GerardPaligot fait même mieux qu'avant non ?

ça signifie surtout que les tests passent sur plus de lignes de code qu'avant, et ça c'est bien. Là ou c'est moins bien, c'est que les tests se contentent de passer, sans vraiment vérifier que l'on a ce qu'on voulait. On a plein d'exemple comme celui-ci qui est censé créer un topic. Comment peut-on dire qu'on a testé cette fonctionnalité, si on n'a pas vérifié qu'après le POST, un nouveau topic est apparu dans la base ?

[GerardPaligot]

Je vais bien être patient mais pas me répéter. Allez lire mes précédents commentaires pour savoir pourquoi. PR si welcome sur cette branche si vous voulez compléter. Basta.

[SpaceFox]

Vous m'avez un peu perdu avec vos pavés.

Ce que j'en dis c'est que :

• Les tests sont perfectibles mais vu le boulot que c'est, on peut difficilement exiger mieux. Surtout qu'on est loin d'avoir une couverture ridicule.
• Le coup des codes 302 etc., y'a un vrai risque de ne pas détecter de vraies erreurs problématiques ou c'est juste un machin hypothétique du même niveau que "on vérifie que le cache de Django marche au cas où il ne marcherait plus" ?
• Concernant la philosophie des tests et en particulier les setUp, je préfère l'approche de @GerardPaligot à celle de @firm1 . Je rappelle au passage que la philosophie Python préfère la clarté locale à la factorisation de code (contrairement à Java où la tendance est plutôt de factoriser à tout prix).
• Si on a un moyen simple de découper les ~1500 lignes de zds/forum/tests/tests_views.py en un truc plus maniable, je suis preneur.

J'espère que j'ai répondu à toutes vos inquiétudes ?

[Eskimon]

Si on a un moyen simple de découper les ~1500 lignes de zds/forum/tests/tests_views.py en un truc plus maniable, je suis preneur.

J'attendais la refacto du module des fofo pour reprendre les tests et les rediviser comme on fait pour les autres (tests_forms, tests_models, tests_views). Quand ici ce sera merge j'aimerais faire ca bientôt après.

Le coup des codes 302 etc.

Du coup par rapport a ma remarque je garderais ceci en tête pour y faire attention...

J'en profite pour rappeler que j'avais fait une QA assez globale a la main et que cette dernière était concluante...

[GerardPaligot]

Le coup des codes 302 etc., y'a un vrai risque de ne pas détecter de vraies erreurs problématiques ou c'est juste un machin hypothétique du même niveau que "on vérifie que le cache de Django marche au cas où il ne marcherait plus" ?

Du côté de mes tests, je ne fais pas de vérification après (pas tout le temps). Du côté des tests existants, ils le font.

Si on a un moyen simple de découper les ~1500 lignes de zds/forum/tests/tests_views.py en un truc plus maniable, je suis preneur.

J'attendais la refacto du module des fofo pour reprendre les tests et les rediviser comme on fait pour les autres (tests_forms, tests_models, tests_views). Quand ici ce sera merge j'aimerais faire ca bientôt après.

Tous les nouveaux tests ne sont que des tests de vues. Si on veut les découper, ça sera par fonctionnalité et non plus par type de tests.

[Eskimon]

Bon... Au vu de ce qui est dit, j'aimerais déclarer "OK" pour merge. Ma QA passe, les tests sont les memes voire mieux qu'avant (puisqu'on garde une partie de l'ancien) et plus on attends plus c'est chiant car on bloque tout le module des fofos avec cette PR. Du coup @SpaceFox si tu es OK avec moi, je te laisse merger au plus tôt...

[SpaceFox]

OK.