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

fix #994 : les tags avec des caractères spéciaux pas pris en compte #999

Merged
merged 7 commits into from Jun 27, 2014

Conversation

Projects
None yet
8 participants
@artragis
Contributor

artragis commented Jun 22, 2014

Q R
Correction de bugs ? [oui]
Nouvelle Fonctionnalité ? [non]
Tickets concernés #994

Cette PR corrige et teste le bug qui faisait que les tag tels que C++ ou C# étaient confondus avec le tag C car les caractères spéciaux n'étaient pas pris en compte.

Pour la QA, il faut tester les fonctionnalités suivantes :

[ ] Seul les tags en début de titre sont pris en compte
[ ] Les tags se comportent comme le parenthésage mathématique [[a] n'est pas parsé [a]] est parsé comme tag = a titre = ] (obligation de pouvoir utiliser ce caractère dans votre titre)
[ ] La liste de tag est insensible à la casse et aux espaces
[ ] En visitant la liste des posts pour un tag tous les posts comportant le tag sont listés
[ ] En visitant la liste des posts pour un tag, les posts sans ce tag ne sont pas listés
[ ] Les caractères spéciaux style # ++ ? sont pris en compte même si la slugification les enlève
[ ] les tags imbriqués dans les autres tags ne sont pas considérés comme des tag [[t1][t2]] ne sont pas deux tags mais un tag #[t1][t2]

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 22, 2014

Coverage Status

Coverage increased (+0.16%) when pulling e60aaba on artragis:issue_994 into 9e7045c on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.16%) when pulling e60aaba on artragis:issue_994 into 9e7045c on zestedesavoir:beta-0.2.

@Alex-D

This comment has been minimized.

Show comment
Hide comment
@Alex-D

Alex-D Jun 22, 2014

Contributor

Le code me semble OK, je laisse la QA faire son travail :)

Contributor

Alex-D commented Jun 22, 2014

Le code me semble OK, je laisse la QA faire son travail :)

@pierre-24

This comment has been minimized.

Show comment
Hide comment
@pierre-24

pierre-24 Jun 22, 2014

Member

Tu as oublié la partie qui reviens à la ligne 661 (ou pas loin) pour transformer slugify() en smart_text().

Pour le reste, j'ai testé, ça fonctionne, même quand on entre dans des tags farfelus qui pourrais donner des erreurs (type [?faille=True]). Donc pour moi, ça passe aussi si tu tient compte de ma remarque précédente (qui se marque quand on change le titre à posteriori).

Member

pierre-24 commented Jun 22, 2014

Tu as oublié la partie qui reviens à la ligne 661 (ou pas loin) pour transformer slugify() en smart_text().

Pour le reste, j'ai testé, ça fonctionne, même quand on entre dans des tags farfelus qui pourrais donner des erreurs (type [?faille=True]). Donc pour moi, ça passe aussi si tu tient compte de ma remarque précédente (qui se marque quand on change le titre à posteriori).

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 23, 2014

Contributor

Done @pierre-24.

Contributor

artragis commented Jun 23, 2014

Done @pierre-24.

@firm1

This comment has been minimized.

Show comment
Hide comment
@firm1

firm1 Jun 23, 2014

Contributor

Bon, je ne l'ai pas testé, mais en lisant le code vite fait, il me semble que le tag Bug sera différent du temps bug. Ce qui ne devrait pas être le cas.

Contributor

firm1 commented Jun 23, 2014

Bon, je ne l'ai pas testé, mais en lisant le code vite fait, il me semble que le tag Bug sera différent du temps bug. Ce qui ne devrait pas être le cas.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 23, 2014

Coverage Status

Coverage increased (+0.16%) when pulling 2903971 on artragis:issue_994 into 9e7045c on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.16%) when pulling 2903971 on artragis:issue_994 into 9e7045c on zestedesavoir:beta-0.2.

@pierre-24

This comment has been minimized.

Show comment
Hide comment
@pierre-24

pierre-24 Jun 23, 2014

Member

@firm1 : je confirme, en effet. (un coup de str.lower() avant ?)

Member

pierre-24 commented Jun 23, 2014

@firm1 : je confirme, en effet. (un coup de str.lower() avant ?)

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 23, 2014

Contributor

C'est en train d' être teste par travis

Contributor

artragis commented Jun 23, 2014

C'est en train d' être teste par travis

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 23, 2014

Coverage Status

Coverage increased (+0.11%) when pulling b19ec69 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.11%) when pulling b19ec69 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

@pierre-24

This comment has been minimized.

Show comment
Hide comment
@pierre-24

pierre-24 Jun 23, 2014

Member

Du coup, chez moi, ça fonctionne :)

Member

pierre-24 commented Jun 23, 2014

Du coup, chez moi, ça fonctionne :)

@firm1

This comment has been minimized.

Show comment
Hide comment
@firm1

firm1 Jun 23, 2014

Contributor

Je n'ai toujours pas testé mais il me semble que le tag "bug" sera
différent du tag "bug " et ça ne devrait pas.
Le 23 juin 2014 08:16, "Pierre Beaujean" notifications@github.com a écrit
:

Du coup, chez moi, ça fonctionne :)


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

Contributor

firm1 commented Jun 23, 2014

Je n'ai toujours pas testé mais il me semble que le tag "bug" sera
différent du tag "bug " et ça ne devrait pas.
Le 23 juin 2014 08:16, "Pierre Beaujean" notifications@github.com a écrit
:

Du coup, chez moi, ça fonctionne :)


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

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 23, 2014

Contributor

Ok, chez moi, ce soir, je revois deux choses :

  • la réutilisation de code (factorisation)
  • le fait de faire un trim sur les tag
  • test unitaire sur le edit first topic

D'ailleurs en terme de comportement, le edit first topic doit faire quoi à propos des tags?
Par ce que je ne sais pas si quand je supprime un tag, celui-ci est enlevé.

Contributor

artragis commented Jun 23, 2014

Ok, chez moi, ce soir, je revois deux choses :

  • la réutilisation de code (factorisation)
  • le fait de faire un trim sur les tag
  • test unitaire sur le edit first topic

D'ailleurs en terme de comportement, le edit first topic doit faire quoi à propos des tags?
Par ce que je ne sais pas si quand je supprime un tag, celui-ci est enlevé.

@pierre-24

This comment has been minimized.

Show comment
Hide comment
@pierre-24

pierre-24 Jun 23, 2014

Member

D'ailleurs en terme de comportement, le edit first topic doit faire quoi à propos des tags?

Pour moi, juste mettre à jour la liste de tag de manière totale (si un tag disparait du titre, il doit disparaitre de la liste, mais pas de la BDD, voir ci dessous).

Par ce que je ne sais pas si quand je supprime un tag, celui-ci est enlevé.

Pas que je ne puisse le lire. Mais est-ce vraiment grave ? Oki, ça "prend de la place" dans la base de donnée (tout est relatif), mais au moins, si le tag est réutilisé plus tard, il existe déjà et le comportement reste cohérent quoiqu'il arrive (puisque l'url est en /<pk>/<slug>/).

Member

pierre-24 commented Jun 23, 2014

D'ailleurs en terme de comportement, le edit first topic doit faire quoi à propos des tags?

Pour moi, juste mettre à jour la liste de tag de manière totale (si un tag disparait du titre, il doit disparaitre de la liste, mais pas de la BDD, voir ci dessous).

Par ce que je ne sais pas si quand je supprime un tag, celui-ci est enlevé.

Pas que je ne puisse le lire. Mais est-ce vraiment grave ? Oki, ça "prend de la place" dans la base de donnée (tout est relatif), mais au moins, si le tag est réutilisé plus tard, il existe déjà et le comportement reste cohérent quoiqu'il arrive (puisque l'url est en /<pk>/<slug>/).

@SpaceFox

This comment has been minimized.

Show comment
Hide comment
@SpaceFox

SpaceFox Jun 23, 2014

Member

Ce genre de discussion n'a-t-il pas plus sa place sur le forum, vu qu'on en
est à discuter de points fonctionnels ?

Le 23 juin 2014 09:40, Pierre Beaujean notifications@github.com a écrit :

D'ailleurs en terme de comportement, le edit first topic doit faire quoi à
propos des tags?

Pour moi, juste mettre à jour la liste de tag de manière totale (si un
tag disparait du titre, il doit disparaitre de la liste, mais pas de la
BDD, voir ci dessous).

Par ce que je ne sais pas si quand je supprime un tag, celui-ci est enlevé.

Pas que je ne puisse le lire. Mais est-ce vraiment grave ? Oki, ça "prend
de la place" dans la base de donnée (tout est relatif), mais au moins, si
le tag est réutilisé plus tard, il existe déjà et le comportement reste
cohérent quoiqu'il arrive (puisque l'url est en ///).


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

Member

SpaceFox commented Jun 23, 2014

Ce genre de discussion n'a-t-il pas plus sa place sur le forum, vu qu'on en
est à discuter de points fonctionnels ?

Le 23 juin 2014 09:40, Pierre Beaujean notifications@github.com a écrit :

D'ailleurs en terme de comportement, le edit first topic doit faire quoi à
propos des tags?

Pour moi, juste mettre à jour la liste de tag de manière totale (si un
tag disparait du titre, il doit disparaitre de la liste, mais pas de la
BDD, voir ci dessous).

Par ce que je ne sais pas si quand je supprime un tag, celui-ci est enlevé.

Pas que je ne puisse le lire. Mais est-ce vraiment grave ? Oki, ça "prend
de la place" dans la base de donnée (tout est relatif), mais au moins, si
le tag est réutilisé plus tard, il existe déjà et le comportement reste
cohérent quoiqu'il arrive (puisque l'url est en ///).


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

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 23, 2014

Contributor

@pierre-24 Nous avons la même interprétation, mais de ce que j'ai pu voir, le code actuel ne délie pas le tag du profil quand on édite.

@SpaceFox J'ai deux raisons de ne pas avoir posté sur le forum :

  • je suis au travail et ils bloquent l'accès
  • j'ai pensé à ce "problème" au moment de poster mon message. J'étais prêt à appuyer sur "envoyer" quand ça m'est venu à l'esprit.
Contributor

artragis commented Jun 23, 2014

@pierre-24 Nous avons la même interprétation, mais de ce que j'ai pu voir, le code actuel ne délie pas le tag du profil quand on édite.

@SpaceFox J'ai deux raisons de ne pas avoir posté sur le forum :

  • je suis au travail et ils bloquent l'accès
  • j'ai pensé à ce "problème" au moment de poster mon message. J'étais prêt à appuyer sur "envoyer" quand ça m'est venu à l'esprit.
@pierre-24

This comment has been minimized.

Show comment
Hide comment
@pierre-24

pierre-24 Jun 23, 2014

Member

Btw (oui, je suis chiant), la regex est pas bonne : des tages comme [[c++]] ou [c[#|++]] ne passent pas (oki, je vais chercher la petite bête, et j'ai pas de cas réel ou ça serait vraiment utile, mais je préfère parer à toute éventualité). En gros, le tag est escamoté à partir du premier ] qu'il rencontre, et le reste ce trouve dans le titre. C'est un problème classique quand on fait des regexs, ceci dit.

Member

pierre-24 commented Jun 23, 2014

Btw (oui, je suis chiant), la regex est pas bonne : des tages comme [[c++]] ou [c[#|++]] ne passent pas (oki, je vais chercher la petite bête, et j'ai pas de cas réel ou ça serait vraiment utile, mais je préfère parer à toute éventualité). En gros, le tag est escamoté à partir du premier ] qu'il rencontre, et le reste ce trouve dans le titre. C'est un problème classique quand on fait des regexs, ceci dit.

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 23, 2014

Contributor

Il faut donc enlever les caractères style "[" ou "|"

Contributor

artragis commented Jun 23, 2014

Il faut donc enlever les caractères style "[" ou "|"

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 23, 2014

Coverage Status

Coverage increased (+0.26%) when pulling 1ed71a5 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.26%) when pulling 1ed71a5 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 23, 2014

Coverage Status

Coverage increased (+0.28%) when pulling 1ed71a5 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.28%) when pulling 1ed71a5 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 25, 2014

Coverage Status

Coverage increased (+0.3%) when pulling 9969c56 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.3%) when pulling 9969c56 on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

@Eskimon

This comment has been minimized.

Show comment
Hide comment
@Eskimon

Eskimon Jun 27, 2014

Member

Remarque

  • C, C++ et C# ont le meme slug (mais ca n'a pas l'air de poser de problèmes grace a l'id qui suit avec)
  • L'affichage des tags est forcément en lower, dommage mais pas dramatique

BUG

  • Si je met deux tags dans le titre, seul le premier est pris en compte si les tags sont séparés d'un espace. Ca marche pourtant sur la beta en prod' qu'il soit collé ou non
  • Si j'ai deux crochets ouvrants, et un seul fermant ([[C++]) j'ai pas le tag [C++ . Normal ?

Tests

Si d'autres veulent tester, voici un petit paquet de titre exotique à essayé (en gras les tags à obtenir, en italique le titre à obtenir)

Titre Résultats
[C++] test c++ test
[C++][C#] test c++ c# test
[C++] [C#] test c++ c# test
[C/] test c/ test
[C] test *c* test
[C'] test c' test
[C"] test c" test
[C++] test [C#] c test [C#]
[C C++] test
[[C][C++]] test [c][c++] test
[[C]] test [c] test
[[C] test [c test
[C]] test c ] test
Member

Eskimon commented Jun 27, 2014

Remarque

  • C, C++ et C# ont le meme slug (mais ca n'a pas l'air de poser de problèmes grace a l'id qui suit avec)
  • L'affichage des tags est forcément en lower, dommage mais pas dramatique

BUG

  • Si je met deux tags dans le titre, seul le premier est pris en compte si les tags sont séparés d'un espace. Ca marche pourtant sur la beta en prod' qu'il soit collé ou non
  • Si j'ai deux crochets ouvrants, et un seul fermant ([[C++]) j'ai pas le tag [C++ . Normal ?

Tests

Si d'autres veulent tester, voici un petit paquet de titre exotique à essayé (en gras les tags à obtenir, en italique le titre à obtenir)

Titre Résultats
[C++] test c++ test
[C++][C#] test c++ c# test
[C++] [C#] test c++ c# test
[C/] test c/ test
[C] test *c* test
[C'] test c' test
[C"] test c" test
[C++] test [C#] c test [C#]
[C C++] test
[[C][C++]] test [c][c++] test
[[C]] test [c] test
[[C] test [c test
[C]] test c ] test
@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 27, 2014

Contributor

•L'affichage des tags est forcément en lower, dommage mais pas dramatique

je me suis basé sur le comportement de stackoverflow pour ça.

•Si j'ai deux crochets ouvrants, et un seul fermant ([[C++]) j'ai pas le tag [C++ . Normal ?

C'est le comportement que j'ai désiré coder, pour preuve les TU.

•Si je met deux tags dans le titre, seul le premier est pris en compte si les tags sont séparés d'un espace. Ca marche pourtant sur la beta en prod' qu'il soit collé ou non

je n'y avais pas pensé, je m'en occupe rapidement.

Contributor

artragis commented Jun 27, 2014

•L'affichage des tags est forcément en lower, dommage mais pas dramatique

je me suis basé sur le comportement de stackoverflow pour ça.

•Si j'ai deux crochets ouvrants, et un seul fermant ([[C++]) j'ai pas le tag [C++ . Normal ?

C'est le comportement que j'ai désiré coder, pour preuve les TU.

•Si je met deux tags dans le titre, seul le premier est pris en compte si les tags sont séparés d'un espace. Ca marche pourtant sur la beta en prod' qu'il soit collé ou non

je n'y avais pas pensé, je m'en occupe rapidement.

@Eskimon

This comment has been minimized.

Show comment
Hide comment
@Eskimon

Eskimon Jun 27, 2014

Member

Ok ca roule, je me suis concentré sur les tests à la main et j'approuve les comportements comme tu les justifies...

Donc en gros pour merger il ne reste que ce problème d'espace entre deux tags je dirais ! Super :)
Tu rajoutes un TU pour ce cas ?

Member

Eskimon commented Jun 27, 2014

Ok ca roule, je me suis concentré sur les tests à la main et j'approuve les comportements comme tu les justifies...

Donc en gros pour merger il ne reste que ce problème d'espace entre deux tags je dirais ! Super :)
Tu rajoutes un TU pour ce cas ?

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 27, 2014

Contributor

Certain de tes tests sont faux, qui plus est :

[[C][C++]]

doit donner un seul tag, c'est dans ce qui est dit sur le forum

[C]] test

devrait plutôt donner tag = c et titre =] test

Contributor

artragis commented Jun 27, 2014

Certain de tes tests sont faux, qui plus est :

[[C][C++]]

doit donner un seul tag, c'est dans ce qui est dit sur le forum

[C]] test

devrait plutôt donner tag = c et titre =] test

@Eskimon

This comment has been minimized.

Show comment
Hide comment
@Eskimon

Eskimon Jun 27, 2014

Member

Oui mea culpa, je me suis embrouillé dans les tests sur GH (mais pourtant pas sur mes tests locaux XD ) Je corrige le tableau (mais faut dire pour ma défense que faire un tableau en MD c'est chi***)

Member

Eskimon commented Jun 27, 2014

Oui mea culpa, je me suis embrouillé dans les tests sur GH (mais pourtant pas sur mes tests locaux XD ) Je corrige le tableau (mais faut dire pour ma défense que faire un tableau en MD c'est chi***)

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 27, 2014

Contributor

Par contre, ayant de gros pb avec mon driver wifi, je ne peux pas pusher quand je suis chez mes parents. Je vais peut être m'acheter un câble mais si vous désirez un code avant dimanche soir, faudra le faire vous même.

Contributor

artragis commented Jun 27, 2014

Par contre, ayant de gros pb avec mon driver wifi, je ne peux pas pusher quand je suis chez mes parents. Je vais peut être m'acheter un câble mais si vous désirez un code avant dimanche soir, faudra le faire vous même.

@Eskimon

This comment has been minimized.

Show comment
Hide comment
@Eskimon

Eskimon Jun 27, 2014

Member

(Au pire) Tu peux m'envoyer un message avec la modif' à faire (fichier & ligne) ?

Member

Eskimon commented Jun 27, 2014

(Au pire) Tu peux m'envoyer un message avec la modif' à faire (fichier & ligne) ?

@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 27, 2014

Contributor

Il me semble qu'on peut modifier directement sur git, non?

Contributor

artragis commented Jun 27, 2014

Il me semble qu'on peut modifier directement sur git, non?

@Eskimon

This comment has been minimized.

Show comment
Hide comment
@Eskimon

Eskimon Jun 27, 2014

Member

ouai il me semble aussi...

Member

Eskimon commented Jun 27, 2014

ouai il me semble aussi...

artragis added some commits Jun 27, 2014

Update views.py
fix le bug des tags qui sont séparés par des espaces
Update tests.py
test unitaire pour les tags séparés par des espaces.
@artragis

This comment has been minimized.

Show comment
Hide comment
@artragis

artragis Jun 27, 2014

Contributor

C'est bon, j'ai réussi.

Contributor

artragis commented Jun 27, 2014

C'est bon, j'ai réussi.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 27, 2014

Coverage Status

Coverage increased (+0.28%) when pulling fecb4ac on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.28%) when pulling fecb4ac on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 27, 2014

Coverage Status

Coverage increased (+0.29%) when pulling fecb4ac on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

Coverage Status

Coverage increased (+0.29%) when pulling fecb4ac on artragis:issue_994 into 6de6512 on zestedesavoir:beta-0.2.

ArnaudCalmettes added a commit that referenced this pull request Jun 27, 2014

Merge pull request #999 from artragis/issue_994
fix #994 : les tags avec des caractères spéciaux pas pris en compte

@ArnaudCalmettes ArnaudCalmettes merged commit 01da992 into zestedesavoir:beta-0.2 Jun 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@artragis artragis deleted the artragis:issue_994 branch Oct 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment