Refactorise la fonction "login_view()" et rend les messages d'erreurs plus précis

[rezemika]

Cette modif' a été discutée sur le forum mais le topic semble au point mort. Je propose donc cette PR, à débattre pour savoir si on la garde ou pas. :)

Ce commit change la manière dont la fonction gère les erreurs pour la rendre un peu plus lisible.
Il rend aussi les messages d'erreur plus précis.

Avant, quand les identifiants fournis étaient invalides, seul "Les identifiants fournis ne sont pas valides." était indiqué. Désormais, le message est soit "Nom d’utilisateur inconnu.", soit "Mot de passe incorrect.".

Contrôle qualité

• Essayer de se connecter avec :
  • un pseudo et un MDP valides ;
  • un pseudo valide et un mauvais MDP ;
  • un pseudo inconnu.

[Anto59290]

J'ai relu rapidement depuis mon téléphone et j'ai l'impression qu'il manque les tests qui vont bien pour les différents cas.

Note: ce n'est pas la faute de cette PR en particulier, mais ne faudrait-il pas gérer ces erreurs dans la validation du formulaire plutôt que dans la vue ?

[motet-a]

👍

[motet-a]

Ça serait mieux avec un return ici pour éviter d’indenter tout le else : https://softwareengineering.stackexchange.com/a/18459

[motet-a]

Ça serait mieux avec une apostrophe : _('Nom d’utilisateur inconnu.')

[gllmc]

Merci pour cette PR !

Je pense que ce serait bien d'utiliser des phrases un peu moins "brutales". Je pense à quelque chose comme :

• "Ce nom d'utilisateur est inconnu. Si vous ne possédez pas de compte, vous pouvez vous inscrire."
• "Le mot de passe saisi est incorrect. Cliquez sur le lien « Mot de passe oublié ? » si vous ne vous en souvenez plus."

[gllmc]

Pour éviter les erreurs 500, remplace ces deux lignes par quelque chose comme :

- username = request.POST['username']
- password = request.POST['password']
+ username = request.POST.get('username', '')
+ password = request.POST.get('password', '')

Toutefois, je pense que le plus propre est d'utiliser une vraie validation de formulaire pour éviter ces problèmes (avec form.is_valid), qui vérifiera que le contenu des champs est normal (pas que les identifiants sont corrects). Le tutoriel Django fait les choses assez bien, même s'il faut ensuite l'adapter à notre code.

[artragis]

+1 pour l'utilisation de form.is_valid() et form.cleaned_data['username']

[rezemika]

J'ai essayé de réécrire la fonction en utilisant is_valid(), mais pour une raison que j'ignore, cette condition n'est pas validée alors que le dictionnaire form.errors est vide. Voilà le code : https://gist.github.com/rezemika/769685937b726c857fd998a69bca4c5b

Est-ce que l'un d'entre-vous aurait une idée de ce qui peut causer ça ?

[Anto59290]

Est ce que tu es sur que ton formulaire contient des données, cf. ce cas particulier ?

[rezemika]

@Anto59290 : Bonne idée, ça semble correspondre. Pourtant, quand je fais print(request.POST), j'ai ceci qui s'affiche : <QueryDict: {'csrfmiddlewaretoken': ['SAJTAu2TUTVKH3iJ3y13xwsjBiO3AROUI1HdpP9H0iOxcTYpnUcwVHnUzJ65kgGg'], 'password': ['admin'], 'username': ['admin'], 'remember': ['on']}>.

Même quand je fais form = LoginForm({'username':'admin', 'password':'admin', 'remember':'on'}), form.is_valid() renvoie False et form.errors est toujours vide.

[Anto59290]

@rezemika : Si tu inspecte le contenu de form tu verra qu'il est bien vide pas de données :

In [10]: form.__dict__
Out[10]: 
{'_bound_fields_cache': {},
 '_errors': {},
 'auto_id': 'id_%s',
 'data': {},
 'empty_permitted': False,
 'error_class': django.forms.utils.ErrorList,
 'fields': OrderedDict([('username',
               <django.forms.fields.CharField at 0x7f16326f4160>),
              ('password', <django.forms.fields.CharField at 0x7f16325f75c0>),
              ('remember',
               <django.forms.fields.BooleanField at 0x7f16325f7668>)]),
 'files': {},
 'helper': <crispy_forms.helper.FormHelper at 0x7f16325f7898>,
 'initial': {},
 'is_bound': False,
 'label_suffix': '\xa0:'}

Pour l'initialiser je dirais qu'il faut faire form = LoginForm(data=request.POST) à la place de form = LoginForm(request.POST)

[rezemika]

@Anto59290 : tu as raison, il suffisait d'un LoginForm(data=request.POST), merci ! :)

(C'est quand même bizarre, ça n'apparait même pas sur la page de doc de Django...)

J'en ai aussi profité pour ajouter un lien vers la page d'inscription dans le message "[...] vous pouvez vous inscrire.". ;)

[Anto59290]

Je ne suis pas 100% sur, mais je pense que c'est à cause de la signature de cette méthode. Je me demande si ton request.POST n'était pas assigné au paramètre next. En nommant le paramètre on contourne le problème.

[rezemika]

Ah, bien vu ! Je pense que tu as raison. Du coup, on pourrait peut-être faire quelque chose comme ça, vu que le paramètre next n'est pas utilisé dans la méthode. Qu'en pense-tu ?

def __init__(self, *args, **kwargs):
    kwargs.pop('next', None)
    super(LoginForm, self).__init__(*args, **kwargs)
    # etc

[Anto59290]

Je ne voit pas de bonne raison de garder le paramètre next, je ne voit pas trop son utilité. Il me semble que ça a été introduit ici.

[Anto59290]

Alors quitte a refactorer du code dans le coin je propose de revoir aussi ce bout de code. Pour moi les problèmes sont les suivants:

• except générique
• duplication de code entre le try/except
• trop de ligne dans le try, on ne sait pas quelle ligne va lever une exception.

Je propose quelque chose du genre:

try:
    response = redirect(next_page)
except: # Ajouter l'exception en question
    response = redirect(reverse('homepage'))

set_old_smileys_cookie(response, profile)
return response

[rezemika]

Bien vu ! C'est fait. :)

[coveralls]

Coverage increased (+0.002%) to 89.636% when pulling 67ef304 on rezemika:improves-login-view into 2960a59 on zestedesavoir:dev.

[gllmc]

Ça me paraît bon, merci de ton travail !

[rezemika]

\o/ Merci !