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

Fixed Verifiy email using regex and modified Login action #236

Merged
merged 8 commits into from Oct 2, 2018

Conversation

flyznex
Copy link
Contributor

@flyznex flyznex commented Oct 1, 2018

Prerequisites

  • Use regex to verify valid email or username
  • Change first parameter of PasswordSignInAsync() method in Login action to TUser type

Description

  • If we use username in method PasswordSignInAsync(), This will call FindUserByName() to find user in database. Of course, method FindByEmailOrUsernameAsync() in Userservice already to return user.

@rayrfan
Copy link
Owner

rayrfan commented Oct 1, 2018

Hi @flyznex you are so fast, before I could even get the Contributing Guide ready :)

For the email check, you probably got the code from the doc here, I originally thought it was too long (#139) so I didn't go for it :) But I agree this should be used instead, I also liked your use of extension method.

On the bottom of that same doc they provided some ready to use testing code with both inputs and outputs (img below). Would you mind add some Unit Tests with those? Just add a class RegexUtilitiesTest.cs in Fan.UnitTests/Helpers folder and create one [Theory] method for all of it.

email test cases

And indeed the PasswordSignInAsync with userName does make an extra call to look up user, since I already got the user no need for that. Good catch! Thank you!

@flyznex
Copy link
Contributor Author

flyznex commented Oct 1, 2018

I have created RegexUtilitiesTest.cs with 2 method to verify Extension method working.

@rayrfan rayrfan changed the base branch from master to pr-236-verify-email-mod-login-action October 1, 2018 16:44
@rayrfan
Copy link
Owner

rayrfan commented Oct 1, 2018

On a closer look the RegexUtilities cannot be static because it depends on a private variable _invalid, I updated the test class and you can see why easier. When you run this theory it executes all the test cases in parallel, if _invalid were static it would be shared and thus it'd get changed by any test thus makes some of the tests fail randomly. Your original test methods were using a loop, so you won't see this effect.

Other than that the PR looks good, I'll leave it open for a few hours if you have any feedback and merge it tonight.

Thank you!

@flyznex
Copy link
Contributor Author

flyznex commented Oct 2, 2018

Thanks for your suggest. I have changed RegexUtilities by remove depends _invalid.

{
return false;
if(ex is RegexMatchTimeoutException || ex is ArgumentNullException)
Copy link
Owner

Choose a reason for hiding this comment

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

If you are checking for null on line 25, why would you check for ArgumentNullException on this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I forgot to remove it. My mistake!

Copy link
Owner

@rayrfan rayrfan left a comment

Choose a reason for hiding this comment

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

I left you a question with a code comment, thanks.

@rayrfan
Copy link
Owner

rayrfan commented Oct 2, 2018

Now you got it to work with static, I'm thinking since I already have a Util.cs and this functionality is just for checking for a valid email. Would you mind if I move this method along with the private static DomainMapper into Util.cs?

From that point on calling it would just look like Util.IsValidEmail(""), instead of RegexUtilities.IsValidEmail(""), it feels tidier and with fewer classes floating around.

@flyznex
Copy link
Contributor Author

flyznex commented Oct 2, 2018

Yes you can move it to Util.cs. If your application have other string process using regex, I think we can keep it separate. You also can change IsValidEmail(this string strIn) to make it become extension method. Regards!

@rayrfan rayrfan changed the base branch from pr-236-verify-email-mod-login-action to dev October 2, 2018 02:02
@rayrfan rayrfan merged commit a0604e2 into rayrfan:dev Oct 2, 2018
@rayrfan
Copy link
Owner

rayrfan commented Oct 2, 2018

Yes we can make it an extension method anytime. And for right now it's probably a bit cleaner keeping fewer classes till till we have more code. I just merged it to dev branch and will push it live later.

Thank you so much for submitting this PR, you are awesome 👍

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.

None yet

2 participants