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

Projects
None yet
2 participants
@flyznex
Contributor

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.

FanrayMedia and others added some commits Sep 30, 2018

@FanrayMedia

This comment has been minimized.

Show comment
Hide comment
@FanrayMedia

FanrayMedia Oct 1, 2018

Owner

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!

Owner

FanrayMedia 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

This comment has been minimized.

Show comment
Hide comment
@flyznex

flyznex Oct 1, 2018

Contributor

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

Contributor

flyznex commented Oct 1, 2018

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

@FanrayMedia FanrayMedia changed the base branch from master to pr-236-verify-email-mod-login-action Oct 1, 2018

@FanrayMedia

This comment has been minimized.

Show comment
Hide comment
@FanrayMedia

FanrayMedia Oct 1, 2018

Owner

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!

Owner

FanrayMedia 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

This comment has been minimized.

Show comment
Hide comment
@flyznex

flyznex Oct 2, 2018

Contributor

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

Contributor

flyznex commented Oct 2, 2018

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

@FanrayMedia

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

@FanrayMedia

This comment has been minimized.

Show comment
Hide comment
@FanrayMedia

FanrayMedia Oct 2, 2018

Owner

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.

Owner

FanrayMedia 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

This comment has been minimized.

Show comment
Hide comment
@flyznex

flyznex Oct 2, 2018

Contributor

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!

Contributor

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!

@FanrayMedia FanrayMedia changed the base branch from pr-236-verify-email-mod-login-action to dev Oct 2, 2018

@FanrayMedia FanrayMedia merged commit a0604e2 into FanrayMedia:dev Oct 2, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@FanrayMedia

This comment has been minimized.

Show comment
Hide comment
@FanrayMedia

FanrayMedia Oct 2, 2018

Owner

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 👍

Owner

FanrayMedia 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