Project

General

Profile

Development #43074

email_validator ne doit pas être utilisé de manière inconditionelle

Added by Benjamin Dauvergne about 2 months ago. Updated about 1 month ago.

Status:
Solution déployée
Priority:
Normal
Category:
-
Target version:
-
Start date:
19 May 2020
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Il faut l'ajouter à la définition du champ email du modèle User, pas l'appeler inconditionnellement dans BaseUserForm, sinon on ne peut plus créer d'utilisateur sans email.

0002-misc-define-email-validation-at-the-model-level-4307.patch View (3.14 KB) Benjamin Dauvergne, 19 May 2020 12:29 PM

0003-misc-simplify-ValidatedEmailField-43074.patch View (783 Bytes) Benjamin Dauvergne, 19 May 2020 12:29 PM

0001-tests-add-test-on-creation-of-user-without-email-430.patch View (1.52 KB) Benjamin Dauvergne, 19 May 2020 12:29 PM

0005-misc-simplify-ValidatedEmailField-43074.patch View (783 Bytes) Benjamin Dauvergne, 19 May 2020 06:17 PM

0003-misc-let-User-model-validate-identifiers-and-uniquen.patch View (4.63 KB) Benjamin Dauvergne, 19 May 2020 06:17 PM

0001-tests-add-tests-on-user-creation-trough-manager-4307.patch View (8.1 KB) Benjamin Dauvergne, 19 May 2020 06:17 PM

0002-misc-validate-emails-in-Model.clean-43074.patch View (3.12 KB) Benjamin Dauvergne, 19 May 2020 06:17 PM

0004-manager-set-created-user-s-OU-in-clean-43074.patch View (1.49 KB) Benjamin Dauvergne, 19 May 2020 06:17 PM

Associated revisions

Revision ab69544f (diff)
Added by Benjamin Dauvergne about 2 months ago

tests: add tests on user creation trough manager (#43074)

Revision 018e276c (diff)
Added by Benjamin Dauvergne about 2 months ago

misc: validate emails in Model.clean (#43074)

Revision d37e8f0d (diff)
Added by Benjamin Dauvergne about 2 months ago

misc: let User model validate identifiers and uniqueness (#43074)

Revision bee7491f (diff)
Added by Benjamin Dauvergne about 2 months ago

manager: set created user's OU in clean() (#43074)

Any modification to the model should be done trough clean() method, as
it enables later checks like uniqueness validation. No instance
initialization should ever be done in save() methods unless it's
garanteed that it cannot mess with validation.

Revision f3f83743 (diff)
Added by Benjamin Dauvergne about 2 months ago

misc: simplify ValidatedEmailField (#43074)

Revision 8dee6912 (diff)
Added by Benjamin Dauvergne about 2 months ago

tests: work around bytes/str usage in webtest (#43074)

webtest only support byte string as GET parameters with python2.

Revision cd490ec3 (diff)
Added by Benjamin Dauvergne about 1 month ago

misc: validate emails in Model.clean (#43074)

History

#3 Updated by Benjamin Dauvergne about 2 months ago

  • Status changed from Solution proposée to En cours

#4 Updated by Benjamin Dauvergne about 2 months ago

C'est moche que ça casse dans un seul environnement, ça sent mauvais.

#6 Updated by Valentin Deniaud about 2 months ago

Dans 0002,

+        if (not self.username
+            and not self.email
+            and not (self.first_name
+                     or self.last_name)):

Chacun sa logique mais moi ça m'a demandé trop de temps de cerveau, j'aurais écris ça juste à coup de and not ou juste à coup de not or, pas un mix des deux (ou encore avec any()).

+            username_qs = qs
+            if not app_settings.A2_USERNAME_IS_UNIQUE:
+                username_qs = qs.filter(ou=self.ou)

La première ligne ne sert à rien (et faire pareil pour email_qs plus bas tant qu'à faire, où l'affectation sert bien à quelque chose mais on peut s'en passer en faisant comme ça).

Sinon de manière générale, j'aimerais que les tests ne soient pas cassés entre deux commits (coucou git rebase --exec). Dans le cas présent c'est sympa pour la relecture d'avoir plein de commits, mais 0001 et 0005 pourraient disparaître et être mis dans les commits qui cassent les-dits tests, ça aide aussi à la compréhension de ce qui est recherché dans ces commits je trouve.

#7 Updated by Benjamin Dauvergne about 2 months ago

Valentin Deniaud a écrit :

Dans 0002,
[...]
Chacun sa logique mais moi ça m'a demandé trop de temps de cerveau, j'aurais écris ça juste à coup de and not ou juste à coup de not or, pas un mix des deux (ou encore avec any()).

C'est parce que c'est pas la bonne condition.

[...]
La première ligne ne sert à rien (et faire pareil pour email_qs plus bas tant qu'à faire, où l'affectation sert bien à quelque chose mais on peut s'en passer en faisant comme ça).

Si je l'enlève ça ne marche plus dans le cas app_settings.A2_USERNAME_IS_UNIQUE is True par contre j'ai fait pareil coté email, email_qs = qs.filter.... pour la symétrie.

Sinon de manière générale, j'aimerais que les tests ne soient pas cassés entre deux commits (coucou git rebase --exec). Dans le cas présent c'est sympa pour la relecture d'avoir plein de commits, mais 0001 et 0005 pourraient disparaître et être mis dans les commits qui cassent les-dits tests, ça aide aussi à la compréhension de ce qui est recherché dans ces commits je trouve.

C'est très bien que les tests cassent entre deux commits sinon on ne voit pas ce qui est réparé, c'est le principe d'un test, un test qui ne foire jamais ça ne teste rien. Sans la progression des commits je ne vois pas comment tu aurais pu comprendre 0005 (ça c'est assez con mais on y peut rien si les libs python3 ne sont pas adaptés à unicode, ça va vite devenir inutile) ou 0004, faut voir ce que ça casse pour comprendre ce que ça corrige.

C'est le seul truc que j'ai appris pendant mes 3 ans de thèse (c'est dire...), avant de corriger un truc on fait un test qui marche pas.

Branche à jour.

#8 Updated by Valentin Deniaud about 2 months ago

  • Status changed from Solution proposée to Solution validée

Benjamin Dauvergne a écrit :

avant de corriger un truc on fait un test qui marche pas.

Oui oui je parlais surtout de n'avoir que des commits verts dans master, pratique adoptée par certains projets.
Parallèlement, perso je trouve plus clair que le test cassé et le fix associé soient dans le même commit (dans ta suite de patch c'est dur de savoir qui répare quoi). Par exemple j'avoue que je n'ai pas compris ce qui cassait test_roles_widget dans les 5 patchs qui précèdent, alors que j'aurais sûrement compris si le fix avait été dans le commit qui casse.

OK quand ça sera vert.

#9 Updated by Benjamin Dauvergne about 2 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 8dee69122427c2ba687d8e5e867ba7cf2e32fc5c
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue May 19 23:03:15 2020 +0200

    tests: work around bytes/str usage in webtest (#43074)

    webtest only support byte string as GET parameters with python2.

commit f3f837439b7087c5c9dd00509070bec7476d6990
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue May 19 12:27:53 2020 +0200

    misc: simplify ValidatedEmailField (#43074)

commit bee7491f482d2fdec15bfa0edaa8656555a3c4e0
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue May 19 17:29:39 2020 +0200

    manager: set created user's OU in clean() (#43074)

    Any modification to the model should be done trough clean() method, as
    it enables later checks like uniqueness validation. No instance
    initialization should ever be done in save() methods unless it's
    garanteed that it cannot mess with validation.

commit d37e8f0d5dcd6cff50cf58fd195188f4dd41998e
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue May 19 17:28:40 2020 +0200

    misc: let User model validate identifiers and uniqueness (#43074)

commit 018e276c07f4e3d71e66e40575c2db8c9eda6383
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue May 19 12:24:38 2020 +0200

    misc: validate emails in Model.clean (#43074)

commit ab69544f6c117431ba6d742bf1ce9c889874c58c
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue May 19 12:24:50 2020 +0200

    tests: add tests on user creation trough manager (#43074)

#10 Updated by Frédéric Péters about 1 month ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF