U4-8782 - E-mail validation when editing user

Created by Denis Bednov 26 Jul 2016, 12:26:56 Updated by Claus Jensen 17 Jan 2017, 07:21:59

Tags: Up For Grabs PR

When editing user e-mail has no validation. Umbraco version 7.4.3 assembly: 1.0.5948.18141

Comments

Sebastiaan Janssen 19 Sep 2016, 11:20:17

Tip: don't make the validation too strict, we have had problems with people who have new tld's in their email address already (for example: someone@my.pizza - pizza is more than 4 characters, so it didn't validate).


Michael Latouche 19 Sep 2016, 11:33:00

OK, I'm gonna grab this one :-)


Dennis Flæng Jørgensen 15 Oct 2016, 20:16:56

I've made a PR with some validation incl. this email validation. https://github.com/umbraco/Umbraco-CMS/pull/1528


Michael Latouche 15 Oct 2016, 23:34:37

I guess HQ will have more fun now to merge both ;-)

Regarding the email validation itself, you used an explicit Regex, which is actually quite different from the one used on user creation:

args.IsValid = Regex.IsMatch(email.Text.Trim(), @"^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+.[A-Za-z]{2,}$");

while the Regex used for user creation email is this one:

@"(?!.)(""([\r\]|\[\r\])""|" + @"([-a-z0-9!#$%&'+/=?^_`{|}~]|(?<!.).))(?<!.)" + @"@[a-z0-9][\w.-][a-z0-9].[a-z][a-z.]*[a-z]$";

May I suggest to use the same validation rule as when creating the user, for consistency reason, like I did:

args.IsValid = MembershipProviderBase.IsEmailValid(email.Text);

This way we keep a centralized and common point of email validation, and if for some reason the regex needs to change, we need to do it in only a single place.

Cheers,

Michaël.


Dennis Flæng Jørgensen 16 Oct 2016, 16:47:19

Thanks for the suggestion. I didnt even think of that. I've updated my PR to include your suggested change.


Michael Latouche 17 Oct 2016, 07:46:49

Great!


Michael Latouche 17 Oct 2016, 07:56:38

By the way, being my first core bugfix, I did not realize I had to mention the pull request in here as well, so for the record, here is also the link to the PR I had made about two weeks ago regarding this: https://github.com/umbraco/Umbraco-CMS/pull/1502.

It also includes some small other small stuff like localizing the label "invalid email".

Cheers,

Michaël.


Dennis Flæng Jørgensen 19 Oct 2016, 15:05:29

@sebastiaan Should we set this issue to another state to indicate that there are potential fixes (with PR) for this issue or mabye change the "Fix submitted"? Both Michael and I are new to this whole contributing game :)


Michael Latouche 19 Oct 2016, 15:12:45

@DennisFJ Good question :-)


Sebastiaan Janssen 19 Oct 2016, 15:15:25

Done! Not sure if you're able to update that field. We'll look at this again in the next sprint (starting next Monday).


Michael Latouche 19 Oct 2016, 15:21:08

@sebastiaan OK thx!


Dennis Flæng Jørgensen 19 Oct 2016, 15:48:26

@sebastiaan Thank you. It looks like I can change the "Fix submitted"-field but not the "Sprint"-field. But thank you for your help :)


Claus Jensen 01 Nov 2016, 13:38:38

Thanks for the PRs! .. I've merged in the fixes from both, tested and refactored a little bit to make it all fit!


Michael Latouche 01 Nov 2016, 23:30:03

@claus Great, thx!


Priority: Up for grabs

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions:

Due in version: 7.5.5

Sprint: Sprint 45

Story Points: 1

Cycle: