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

fix: handle @ in display name when using MailHelper.StringToEmailAddress #903

Merged
merged 7 commits into from
Apr 9, 2020

Conversation

Fieora
Copy link
Contributor

@Fieora Fieora commented Jul 3, 2019

fix email regex to allow email addresses in the display name.

Input: [email protected] <[email protected]>
Expected:
-NameGroup: [email protected]
-EmailGroup: [email protected]

Initial:
-NameGroup: {empty string}
-EmailGroup: [email protected] <[email protected]
Result: A match is found, therefore there is no error caught and email string is bad

After:
-NameGroup: [email protected]
-EmailGroup: [email protected]
Result: Success!

Other test cases:
-<[email protected]> SUCCESS
-[email protected] SUCCESS
-username <[email protected]> SUCCESS
-username <usernameexample.com> NO MATCH (SUCCESS)

Testing with the updated regular expression can be done here

Fixes

Fixes #902

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the development branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Flips the or (|) clause order in the regular expression.

If you have questions, please send an email to Twilio Sendgrid, or file a Github Issue in this repository.

fix email regex to allow email addresses in the display name.

Input: [email protected] <[email protected]>
Expected:
-NameGroup: [email protected]
-EmailGroup: [email protected]

Initial:
-NameGroup: {empty string}
-EmailGroup: 	[email protected] <[email protected]
Result: A match is found, therefore there is no error caught and email string is bad

After:
-NameGroup: [email protected]
-EmailGroup: [email protected]
Result: Success!

Other test cases:
-<[email protected]> SUCCESS
[email protected] SUCCESS
-username <[email protected]> SUCCESS
-username <usernameexample.com> NO MATCH (SUCCESS)
@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Jul 3, 2019
@Fieora
Copy link
Contributor Author

Fieora commented Jul 3, 2019

Hey, this is my first pull request on a major project!

I was able to run the tests, however I didn't add any new tests or comments as this is a one line change.
I am also not very experienced with regex or tests, so I could have easily missed something.

add test casses for MailHelper.StringToEmail
Add test cases for MailHelper.StringToEmailAddress()
@Fieora
Copy link
Contributor Author

Fieora commented Jul 4, 2019

I have added test cases for MailHelper.StringToEmailAddress()

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty type: community enhancement feature request not on Twilio's roadmap labels Aug 21, 2019
@aevitas
Copy link
Contributor

aevitas commented Aug 24, 2019

Hey @Fieora, great job on the PR! I think your changes look good, except for one minor details, and that's the casing of parameters. In C#, it's custom to use camelCase for method parameters. It'd be great if you could fix them, but otherwise good work! :)

@childish-sambino childish-sambino changed the title Fix @ in display name when using MailHelper.StringToEmailAddress fix: handle @ in display name when using MailHelper.StringToEmailAddress Apr 9, 2020
Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

LGTM

@childish-sambino childish-sambino merged commit 57ba28b into sendgrid:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MailHelper.StringToEmailAddress() returns bad data when the display name contains an @ symbol
4 participants