Skip to content
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

Rajout de champ mot de passe aux pages de Désinscription et de changements d'email #6334

Merged
merged 5 commits into from
Jul 30, 2022

Conversation

Migwel
Copy link
Contributor

@Migwel Migwel commented Jun 5, 2022

Fix #5372.

Change la façon dont on gère les confirmations par mot de passe en ajoutant un FieldPasswordMixin. Et utilise ce mixin pour ajouter une confirmation aux pages de désincriptions et celle de changement d'email.

Contrôle qualité

Scénario 1:

  • Se connecter
  • Se rendre sur la page de modification de nom d'utilisateur et d'email
  • Essayer de modifier son nom d'utilisateur ou email sans remplir le champ de mot de passe
    Résultat attendu: la soumission ne fonctionne pas, le mot de passe est un champ obligatoire

Scénario 2:

  • Se connecter
  • Se rendre sur la page de modification de nom d'utilisateur et d'email
  • Essayer de modifier son nom d'utilisateur ou email en fournissant un mot de passe incorrect
    Résultat attendu: la mise à jour échoue avec un message mentionnant que le mot de passe est incorrect

Scénario 3:

  • Se connecter
  • Se rendre sur la page de modification de nom d'utilisateur et d'email
  • Essayer de modifier son nom d'utilisateur ou email en fournissant le bon mot de passe
    Résultat attendu: La mise à jour fonctionne

Répéter ces trois scénarios pour les pages de déinscription et celle de changement de mot de passe.

@coveralls
Copy link

coveralls commented Jun 5, 2022

Coverage Status

Coverage decreased (-0.01%) to 88.188% when pulling cc15f83 on Migwel:issue5372 into 777e109 on zestedesavoir:dev.

Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joli travail ! Je viens juste de parcourir rapidement le code et je me demandais si tu ne pouvais pas mettre le password = forms.CharField(...) que tu as rajouté dans chaque formulaire, directement dans FieldPasswordMixin ?

@Migwel
Copy link
Contributor Author

Migwel commented Jun 6, 2022

Joli travail ! Je viens juste de parcourir rapidement le code et je me demandais si tu ne pouvais pas mettre le password = forms.CharField(...) que tu as rajouté dans chaque formulaire, directement dans FieldPasswordMixin ?

Je ne sais plus pourquoi, quand j'avais essayé cette approche ça n'avait pas fonctionné. Mais je viens de ré-éssayer ça et ça fonctionne correctement. Mais j'ai dû le faire hériter de forms.Form et donc ça m'a l'air d'être plus une vraie classe parent plutôt qu'un mixin donc je l'ai renommé

Copy link
Contributor

@Arnaud-D Arnaud-D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA NOK ✖️

C'est relativement mineur, mais un peu gênant d'un point de vue UX. Je trouve que les labels pour le changement de mot de passe sont confus. On a "Mot de passe (confirmation)" d'un côté et "Confirmer le nouveau mot de passe" de l'autre. Je pense qu'il faudrait voir apparaître "ancien mot de passe" pour que ça soit bien clair.

Quelques autres commentaires concernant plutôt le code.

Sinon, ça fonctionne.

widget=forms.PasswordInput,
)
def __init__(self, user, *args, **kwargs):
super(UnregisterForm, self).__init__(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ici, c'est probablement possible de juste écrire super() sans plus de précisions.

widget=forms.PasswordInput,
)

def check_correct_password(self, cleaned_data):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça ne serait pas mieux d'écrire une méthode clean() dans ce mixin, qui serait appelée par les différents utilisateurs dans leur propre méthode clean, via super().clean() (utiliser l'héritage, donc), plutôt que d'appeler cette méthode là explicitement ?

T'en penses quoi ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah oui, c'est carrément mieux, j'envoie çá

@Migwel Migwel requested a review from Arnaud-D July 27, 2022 21:03
@Arnaud-D Arnaud-D enabled auto-merge (squash) July 30, 2022 19:49
@Arnaud-D Arnaud-D merged commit 9a5075f into zestedesavoir:dev Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Profil et sécurité: rajouter un champ « Mot de passe »
4 participants