Fix bug on no tag #3875

artragis · 2016-10-19T20:09:20Z

Q	R
Type de modification	correction de bug
Ticket(s) (issue(s)) concerné(s)	(j'ai pas encore créé, c'est une alerte sentry)

QA

créez un tuto
, j'ai utilisé la console du navigateur pour modifier la valeur du champ contenant les tags (ce qui outrepasse le maxlength) et mettre un tag plus long que prévu, ce qui a entraîné une erreur 500 en validant
vérifiez que ça bug pas et que le tuto est créé sans ses tags.

coveralls · 2016-10-19T20:23:11Z

Coverage remained the same at 87.615% when pulling 85e8e25 on artragis:patch-11 into 123e744 on zestedesavoir:release-v20.

pierre-24 · 2016-10-19T20:32:46Z

Je crois que ça ira :

GerardPaligot · 2016-10-19T20:44:07Z

Je voudrais être chiant, je dirais qu'il manque un TU. (:

pierre-24 · 2016-10-19T20:47:17Z

Je suis d'accord ;)

vhf · 2016-10-19T21:32:45Z

Merci @artragis . Outre le test manquant, j'ai quelques questions :

Est-ce que ce bug a été introduit par cette release ou est-il antérieur ?
Ne vaudrait-il pas mieux modifier cette méthode pour y faire cette vérification ?*
~~S'il n'y a que ce bug mineur dans la RC4, est-ce que vous seriez contre faire cette PR sur dev pour s'éviter une 5e RC ?~~

* Un truc dans ce goût-là :

+        if not raw_string:
+            return []
         return TagValidator.validate_string_list(raw_string.split(","))

artragis · 2016-10-20T05:33:03Z

Etant donné que je l'ai introduit, j'ai la casi certitude que oui c'est
du pur v20.

Le 19/10/2016 à 23:32, victor felder a écrit :

Merci @artragis https://github.com/artragis . Outre le test
manquant, j'ai quelques questions :

Est-ce que ce bug a été introduit par cette release ou est-il
antérieur ?

Ne vaudrait-il pas mieux modifier cette ligne
artragis@d8c3425#diff-ec82b3a079c8b6115716953cfd70ad93R70
pour y mettre …|(raw_string=''):| ?

S'il n'y a que ce bug mineur dans la RC4, est-ce que vous seriez
contre faire cette PR sur |dev| pour s'éviter une 5e RC ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3875 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABc_xa-lTVg_KylznqtLNYaS0rnFz5zdks5q1ox_gaJpZM4KbZo_.

coveralls · 2016-10-20T18:35:28Z

Coverage decreased (-34.9%) to 52.709% when pulling c719940 on artragis:patch-11 into 123e744 on zestedesavoir:release-v20.

coveralls · 2016-10-20T18:49:27Z

Coverage decreased (-0.007%) to 87.608% when pulling 3db2a62 on artragis:patch-11 into 123e744 on zestedesavoir:release-v20.

vhf · 2016-10-20T19:00:57Z

Je pense qu'il vaut la peine d'en profiter pour retravailler un peu ce validateur de tag et la pertinence des messages qu'il retourne.

Exemple de logique ici. Comme ça on dit à l'utilisateur directement quels tags sont trop longs, et on lui dit aussi quand un ou des tags, et le cas échéant le ou lesquels, contiennent des caractères pas autorisés. C'est mieux que de les virer sans avertir.

pierre-24 · 2016-10-20T19:06:23Z

Je suis d'accord. Après, faut voir si on veut ça pour le bugfix ou pour plus tard

vhf · 2016-10-20T19:10:48Z

Je pense que vu que le gros du travail est fait et qu'il manque juste l'intégration un peu plus propre du brouillon dont je donne le lien, y a pas de raison de pas faire passer le tout ici.

vhf

intégrer la validation utf8 au validateur de tags.

vhf · 2016-10-21T07:15:35Z

zds/tutorialv2/forms.py

@@ -265,6 +265,7 @@ def clean(self):
            self._errors['image'] = self.error_class(
                [_(u'Votre logo est trop lourd, la limite autorisée est de {} Ko')
                 .format(settings.ZDS_APP['gallery']['image_max_size'] / 1024)])
+        # because tags are not mandatory


Commentaire inutile.

vhf · 2016-10-21T07:18:25Z

zds/utils/tests/tests_models.py

@@ -58,3 +59,9 @@ def insert_duplicated_tags(tags):
        self.assertIn('azerty', all_slugs)
        self.assertIn('qwerty', all_slugs)
        self.assertIn('another-tag', all_slugs)
+
+    def test_validator(self):


Ce test n'est pas bon. Une implémentation évidente de la méthode testée ici serait def validate_raw_string(): return True

artragis · 2016-10-21T12:14:48Z

utf8mb4 validé + refonte du validateur pour que ça soit beau et pythonique.

vhf · 2016-10-21T12:18:58Z

Merci @artragis ! @pierre-24 quand t'es motiv, QA ?

@GerardPaligot tu vois d'autres trucs à régler pour la v20 ou je peux RC ?

pierre-24 · 2016-10-21T12:29:57Z

Tout dépend à quel point tu considère #3842 comme bloquant :)

(je vais QA)

pierre-24 · 2016-10-21T12:47:43Z

QA: NOK!

Si je met pour tags "test,😄", j'obtient "object of type 'function' has no len()" dans
"zds/utils/forms.py in validate_length" (à la ligne 85). Ça, c'est parce que _ n'est pas défini dans la fonction.
Mettons que je mette raw_string à la place de ce fameux _, eh bien j'ai les deux erreurs, dont la première 2 fois:

En même temps, la ligne 93 fait tout les tests sur tout les tags. Donc forcément, on se tape toutes les erreurs à la suite. Si c'est ce que tu recherches, alors précise quel tag est fautif dans l'erreur :)
Et en fait, la logique de la ligne if len(_) <= Tag._meta.get_field("title").max_length: est à l'envers, ça devrait être >= (d'où les erreurs "le tag est trop long").
Sinon, j'ai toujours bien "Assurez-vous que cette valeur comporte au plus 64 caractères (actuellement 599).", donc ça, c'est bon !

vhf · 2016-10-21T13:14:59Z

Je comprends pas pourquoi travis est pas passé par ici.

pierre-24 · 2016-10-21T13:24:00Z

Parce qu'il semblerai qu'internet soit cassé (et qu'il suffise que je dise ça pour que Travis se réveille).

artragis · 2016-10-21T15:23:13Z

logically fixed

pierre-24 · 2016-10-21T15:44:30Z

zds/utils/forms.py


    def validate_raw_string(self, raw_string):
        if raw_string is None or not isinstance(raw_string, basestring):
            return self.validate_string_list([])
        return self.validate_string_list(raw_string.split(","))

    def validate_length(self, raw_string):
-        if len(_) <= Tag._meta.get_field("title").max_length:
-            self.errors.append(_(u"Le tag est trop long"))
+        if len(raw_string) <= Tag._meta.get_field("title").max_length:


T'as oublié d'inverser la condition ;) (du coup les tests sont tout cassés)

... Et si les erreurs doivent ce suivre, il faudrait les faire finir par . (avec un espace après le point)

coveralls · 2016-10-22T09:56:58Z

Coverage decreased (-34.9%) to 52.756% when pulling b798fa0 on artragis:patch-11 into 123e744 on zestedesavoir:release-v20.

coveralls · 2016-10-22T10:11:19Z

Coverage decreased (-34.9%) to 52.756% when pulling 4e4241a on artragis:patch-11 into 123e744 on zestedesavoir:release-v20.

coveralls · 2016-10-22T11:08:55Z

Coverage decreased (-34.9%) to 52.756% when pulling 9f6f5d1 on artragis:patch-11 into 123e744 on zestedesavoir:release-v20.

vhf · 2016-10-22T11:54:59Z

zds/utils/forms.py

+        to get all internationalized error.
+        """
+        if raw_string is None or not isinstance(raw_string, basestring):
+            return self.validate_string_list([])


Pourquoi pas return True ?

question d'évolutivité : si demain pour une raison aussi saugrenue soit-elle on décide qu'une liste vide est invalide ça retournera false.
De plus c'est juste ici de la sémantique "si tu mets None ou autre chose qu'un string, on le considère comme un string vide".

vhf · 2016-10-22T11:55:02Z

zds/utils/tests/tests_models.py

+
+    def test_validator_with_utf8mb4(self):
+
+        raw_string = u"☢☢☢☢☢☢,bla"


Remplace ☢ par 🐙. Le premier est pas utf8mb4, le second l'est. On peut pas vraiment éviter ça malheureusement.

coveralls · 2016-10-22T12:34:11Z

Coverage decreased (-0.008%) to 87.607% when pulling f181add on artragis:patch-11 into 123e744 on zestedesavoir:release-v20.

pierre-24 · 2016-10-22T13:45:47Z

QA OK. On est parti :)

artragis · 2016-10-25T19:01:05Z

il faudra donc attendre demain que je m'y mette dans des bonnes conditions.

Le 20/10/2016 à 21:00, vhf a écrit :

Je pense qu'il vaut la peine d'en profiter pour retravailler un peu ce
validateur de tag et la pertinence des messages qu'il retourne.

Exemple de logique ici
vhf@34dbd97.
Comme ça on dit à l'utilisateur directement quels tags sont trop
longs, et on lui dit aussi quand un ou des tags, et le cas échéant le
ou lesquels, contiennent des caractères pas autorisés. C'est mieux que
de les virer sans avertir.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3875 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABc_xSyCuknXpwtbZFFVx86W0Q99CntKks5q17pqgaJpZM4KbZo_.

vhf · 2016-10-25T19:03:39Z

Je suis pas sûr de savoir de quoi tu parles @artragis, mais si c'est à propos de #3895 rien d'urgent, a priori ce sera pour la v21.

artragis · 2016-10-25T19:07:58Z

hum, on appelle ça le cache de thunderbird qui finit par se réveiller dès que la connexion est stable il semblerait. Car ce message je l'ai envoyé non pas il y a cinq minutes mais il y a plusieurs jours.

Les problèmes récents de DNS sont peut être en cause.

Problème apparu suite au merge de v20 dans dev à cause de cet état : * #3875 change les validateurs dans v20 * bb4f285 utilise les vieux validateurs dans dev

Fix bug on no tag

85e8e25

vhf added this to the Version 20 milestone Oct 20, 2016

add unittest and move fix location.

c719940

prout

3db2a62

vhf suggested changes Oct 21, 2016

View reviewed changes

meilleur validateur et utf8mb4

f11bbed

vhf approved these changes Oct 21, 2016

View reviewed changes

vhf added S-BUG Corrige un problème C-Back Concerne le back-end Django S-Régression Corrige un problème sur un composant qui fonctionnait auparavant and removed S-BUG Corrige un problème labels Oct 21, 2016

fix filtering.

7d9fd07

pierre-24 reviewed Oct 21, 2016

View reviewed changes

good comparison

b798fa0

refactor and doc

4e4241a

good str

9f6f5d1

vhf suggested changes Oct 22, 2016

View reviewed changes

bizarrerie mal encodée dans mon ide.

f181add

pierre-24 merged commit 9495485 into zestedesavoir:release-v20 Oct 22, 2016

This was referenced Oct 25, 2016

[beta][v20] Incohérence dans les tags autorisés entre contenus et forums #3894

Closed

Validation des tags à la création/édition de sujet #3895

Closed

vhf added a commit that referenced this pull request Oct 26, 2016

Corrige validation tag lors de création sujet

ea3c973

Problème apparu suite au merge de v20 dans dev à cause de cet état : * #3875 change les validateurs dans v20 * bb4f285 utilise les vieux validateurs dans dev

artragis deleted the patch-11 branch April 13, 2017 07:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug on no tag #3875

Fix bug on no tag #3875

artragis commented Oct 19, 2016 •

edited

coveralls commented Oct 19, 2016 •

edited

pierre-24 commented Oct 19, 2016

GerardPaligot commented Oct 19, 2016

pierre-24 commented Oct 19, 2016

vhf commented Oct 19, 2016 •

edited

artragis commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016 •

edited

vhf commented Oct 20, 2016

pierre-24 commented Oct 20, 2016

vhf commented Oct 20, 2016

vhf left a comment

vhf Oct 21, 2016

vhf Oct 21, 2016

artragis commented Oct 21, 2016

vhf commented Oct 21, 2016

pierre-24 commented Oct 21, 2016 •

edited

pierre-24 commented Oct 21, 2016

vhf commented Oct 21, 2016

pierre-24 commented Oct 21, 2016 •

edited

artragis commented Oct 21, 2016

pierre-24 Oct 21, 2016 •

edited

pierre-24 Oct 21, 2016

coveralls commented Oct 22, 2016

coveralls commented Oct 22, 2016

coveralls commented Oct 22, 2016

vhf Oct 22, 2016

artragis Oct 22, 2016

vhf Oct 22, 2016

coveralls commented Oct 22, 2016 •

edited

pierre-24 commented Oct 22, 2016

artragis commented Oct 25, 2016 •

edited by vhf

vhf commented Oct 25, 2016

artragis commented Oct 25, 2016


		def test_validator_with_utf8mb4(self):

		raw_string = u"☢☢☢☢☢☢,bla"

Fix bug on no tag #3875

Fix bug on no tag #3875

Conversation

artragis commented Oct 19, 2016 • edited

QA

coveralls commented Oct 19, 2016 • edited

pierre-24 commented Oct 19, 2016

GerardPaligot commented Oct 19, 2016

pierre-24 commented Oct 19, 2016

vhf commented Oct 19, 2016 • edited

artragis commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016 • edited

vhf commented Oct 20, 2016

pierre-24 commented Oct 20, 2016

vhf commented Oct 20, 2016

vhf left a comment

Choose a reason for hiding this comment

vhf Oct 21, 2016

Choose a reason for hiding this comment

vhf Oct 21, 2016

Choose a reason for hiding this comment

artragis commented Oct 21, 2016

vhf commented Oct 21, 2016

pierre-24 commented Oct 21, 2016 • edited

pierre-24 commented Oct 21, 2016

vhf commented Oct 21, 2016

pierre-24 commented Oct 21, 2016 • edited

artragis commented Oct 21, 2016

pierre-24 Oct 21, 2016 • edited

Choose a reason for hiding this comment

pierre-24 Oct 21, 2016

Choose a reason for hiding this comment

coveralls commented Oct 22, 2016

coveralls commented Oct 22, 2016

coveralls commented Oct 22, 2016

vhf Oct 22, 2016

Choose a reason for hiding this comment

artragis Oct 22, 2016

Choose a reason for hiding this comment

vhf Oct 22, 2016

Choose a reason for hiding this comment

coveralls commented Oct 22, 2016 • edited

pierre-24 commented Oct 22, 2016

artragis commented Oct 25, 2016 • edited by vhf

vhf commented Oct 25, 2016

artragis commented Oct 25, 2016

artragis commented Oct 19, 2016 •

edited

coveralls commented Oct 19, 2016 •

edited

vhf commented Oct 19, 2016 •

edited

coveralls commented Oct 20, 2016 •

edited

pierre-24 commented Oct 21, 2016 •

edited

pierre-24 commented Oct 21, 2016 •

edited

pierre-24 Oct 21, 2016 •

edited

coveralls commented Oct 22, 2016 •

edited

artragis commented Oct 25, 2016 •

edited by vhf