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 bug in normalize_and_hash_email_address function #752

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arnau126
Copy link

First commit

If email_address has no "@" in it, the function raises IndexError list index out of range in:

is_gmail = re.match(r"^(gmail|googlemail)\.com$", email_parts[1])

because email_parts is a list with only one item.

This PR fix this bug by moving the above line inside the if-block if len(email_parts) > 1 .

Second commit

I've added strip to email_address to honor the Enhanced Conversions doc which says:

[...] In order to standardize the hash results, prior to hashing one of these values you must:

  • Remove leading/trailing whitespaces.
  • [...]

@arnau126 arnau126 requested a review from a team as a code owner February 25, 2023 16:28
@google-cla
Copy link

google-cla bot commented Feb 25, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@arnau126
Copy link
Author

About the second commit, I've just realized that the function calls normalize_and_hash which already does the strip. However I think that we should keep this extra strip in the very beginning of the function so a string like "[email protected] " (with a trailing whitespace) works.
If we don't perfom this previous strip, the regex won't match, and the period won't be removed.

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.

1 participant