Produits Entr'ouvert » Hobo

Je ne suis pas fan des self.clean() appelé dans self.save(), c'est fait pour être utilisé par les ModelForm normalement, et rien d'autre (enfin n'importe quoi qui fait de la validation comme ModelForm).

J'enlèverai cela, je m'assurerai que le formulaire de création de service appelle bien clean (que c'est bien un ModelForm) et dans te test je testerai directement la vue de création de service ou au moins son formulaire.

Il faut importer ValidationError de son origine, pas depuis le models.

raise ValidationError(_('The slug {} is already used. It must be unique.'.format(self.slug)))

L'appel à gettext doit se faire sur la chaine non formattée.

Je n'ai pas testé le patch.

Je mets un up, je pense qu'on peut pousser celui la.

• je ne pige pas le « return self.cleaned_data », c'est pas plutôt « return cleaned_data » ?
• et « cleaned_data.slug » ça serait pas plutôt « cleaned_data['slug'] » ?
• le hobo/environment/models.py n'a rien à faire dans ce patch, je pense

Le test a été exécuté ? (à le lire il ne doit pas fonctionner, la validation a lieu au niveau du formulaire, le .save() passerait).

Je regarde ça

Juste en passant: les méthode clean() des modèles ne sont appelées que par les formulaires (via full_clean()[1]), jamais sur un simple .save(). Si il est nécessaire de vérifier un invariant il vaut mieux le mettre dans le .save(), si c'est une vrai validation (avec raise ValidationError) ou l'intialisation d'une valeur par défaut ça va dans clean() (ça peut aussi aller dans une surcharge de clean_fields() si jamais il fallait pouvoir ignorer le champs des fois).

¹ https://docs.djangoproject.com/en/1.10/ref/models/instances/#validating-objects

Tu peux rebaser ton patch sur master?

Benjamin Dauvergne a écrit :

Juste en passant: les méthode clean() des modèles ne sont appelées que par les formulaires (via full_clean()[1]), jamais sur un simple .save()...

Je pense que c'est pour ça que JB appele self.full_clean dans le save. L'exception sera levée lors de la tentative de création de l'objet dans la console, même si elle sera moche, du genre:

ValidationError: {'__all__': [u'This slug is already used. It must be unique.']}

"dans la console" tu veux dire quoi par là ? Ça affiche ça dans la console js que l'usager ne voit pas ? Ça fait une erreur 500 et ça écrit ça dans un log ? (l'un comme l'autre ne vont absolument pas, il faut bien sûr afficher un message à l'usager pour lui signaler le probème).

Je sous-entends le shell django.

Le formulaire web affiche le message que le slug doit être unique et dans le shell django l'objet n'est pas créé. Ce que est le but recherché.

Ok, m'était venue une autre idée, comme quoi la console intervenait parce que tu testais avec la commande "cook", et donc question, ça se comporte comment là ? À lire le cook.py ça crashe, il faudrait sans doute un try/except ValidationError autour des appels aux étapes.

Dans le cas où il faut traiter le cook je virerai l'appel à full_clean() dans save (personnellement si le slug doit être unique j'aurai mis unique=True sur le champ).

Et dans cook on met un appel explicite à full_clean().

(personnellement si le slug doit être unique j'aurai mis unique=True sur le champ).

Il y a plusieurs modèles.

Ah oui dsl.

Benjamin Dauvergne a écrit :

Dans le cas où il faut traiter le cook je virerai l'appel à full_clean() dans save.

Et dans cook on met un appel explicite à full_clean().

Mais cela ne nous évite pas une erreur potentielle de modification du slug d'un service dans le shell d'un tenant (en dehors du cook).

De mon côté, quelqu'un qui passe par un shell python ou postgresql, qu'il puisse se tirer dans le pied, ça ne me pose pas problème.

OK. Du coup qu'est ce que je dois changer exactement?
J'avais appelé le full_clean explicitement parce que mon clean n'était jamais appelé lors d'un save (je parle aussi de tests en interface quand j'essayais de rajouter des services depuis l'ui de hobo).
Il me semble que le validationError remonte bien l'erreur à l'usager avant de sauvegarder (sur l'ui encore).
Et le full_clean n'est pas bon parce qu'il est appelé dans le cook c'est bien ça?

Pour moi une remontée correcte de l'erreur est à assurer dans deux cas, les deux cas où un usager normal utilise hobo : via l'ui web et via la commande cook. (sur la manière dont écrire le patch pour assurer ça je ne m'exprime pas).

J'aime pas les clean dans les save() j'en ai déjà mis dans le passé et quand on veut corriger un truc on se retrouve coincé, on ne crée des services qu'à deux endroits. les formulaires dans les vues, et là ça marche tout seul et cook. Je propose d'appeler le clean() dans cook et d'afficher une belle erreur bien propre quand une ValidationError remonte à cet endroit. C'est tout.

            try:
                obj.clean()
                obj.save()
            except ValidationError as e:
                raise CommandError(str(e))

Me semble que le ValidationError peut uniquement sortir de obj.clean(), je ne mettrais donc pas le .save() dans le bloc.

Pas bien fan du retour à la ligne, et de la modification à un fichier sans rapport, mais ack.

Le test n'est pas bon, si .clean() raise les deux lignes d'après ne s'exécuteront pas (l'exception est remontée au contextmanager qui va juste sauté après le bloc encadré par le with[1])

¹ https://docs.python.org/2.7/reference/compound_stmts.html#with bonne lecture :)

Ack.