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

amélioration de la validation des emails via Devise #4945

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

adipasquale
Copy link
Contributor

@adipasquale adipasquale commented Jan 6, 2025

Contexte

Cette erreur Sentry correspond à une adresse email destinataire qui commence par un . : .nom@sfr.fr.

Solution

Je propose d’adapter la regexp pour interdire les points en début d’email.

J’en profite pour passer la regexp existante sur plusieurs lignes et rajouter un test pour un cas existant déjà traité mais non testé (à ma connaissance).

@adipasquale adipasquale marked this pull request as ready for review January 6, 2025 08:47
Copy link
Member

@AntoineGirard AntoineGirard left a comment

Choose a reason for hiding this comment

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

J’ai l’impression que cela ne fait pas partie du standard RFC 5322

En revanche, j’ai essayé sur plusieurs providers et il ne semble effectivement pas possible de créer des comptes avec une adresse qui commence par un .

Donc, LGTM 🚀

[A-Za-z0-9-]+
(\.[A-Za-z0-9-]+)* # subdomains
\.[A-Za-z]{2,} # TLD cannot be shorter than 2 letters
\z/x # x = extended mode = whitespaces ignored + allow comments
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 trop bien cette syntaxe de regex, je savais pas que ça existait

expect(form.user.errors.count).to eq 1
expect(form.user.errors.first.attribute).to eq :email
expect(form.user.errors.first.type).to eq :invalid
end
Copy link
Contributor

Choose a reason for hiding this comment

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

est-ce que tu penses que ça vaut le coup d'ajouter cette règle à l'autocorrect sur les emails des usagers mis en place dans app/models/user.rb:90 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Très bonne suggestion ! mais je me demande si ce sont vraiment des fautes de frappe ou plutôt des erreurs de copier coller dans ce cas. Ça me paraît plus sûr d’afficher une erreur pour que les personnes corrigent elles-mêmes, en espérant qu’elles voient le . en début de champ 🤞

Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense qu'il est plus sûr de toujours informer la personne que l'adresse est invalide plutôt que d'auto-correct silencieusement.

Par exemple si le mail contient .@, ça peut l'amener à réaliser qu'elle n'a tapé que prenom.@gmail.com alors qu'elle voulait taper prenom.nom@gmail.com. Avec l'auto-correct on rend l'erreur invisible, et les mails partent pour prenom@gmail.com qui n'a rien demandé (et l'adresse est valide donc rien ne remonte chez Brevo).

Qu'en pensez-vous ?

PS : en complément, si vous avez un avis sur cette autre proposition, je vous laisse commenter direct sur l'issue : #3406. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

je suis aligné sur l’idée qu’il vaut mieux informer qu’auto-corriger 👍

@adipasquale adipasquale enabled auto-merge (squash) January 6, 2025 11:10
@adipasquale adipasquale merged commit 864d7a4 into production Jan 6, 2025
15 checks passed
@adipasquale adipasquale deleted the feature/improve-email-validation branch January 6, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants