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

feat: remove duplicate emails ignoring case in Personalization #924

Merged

Conversation

DougCal
Copy link
Contributor

@DougCal DougCal commented Jul 20, 2020

Fixes #788

This removes duplicate emails on setting the Personalization object, ignoring case.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Jul 20, 2020
@DougCal
Copy link
Contributor Author

DougCal commented Jul 20, 2020

The setters currently don't work for lists of strings or lists of tuples. Wasn't sure if they're needed for this feature, but let me know what you think of them and the expected behavior for if they're used. At the moment, this will raise an AttributionError.

@thinkingserious thinkingserious changed the base branch from master to main July 28, 2020 14:35
@childish-sambino childish-sambino changed the title remove duplicate emails ignoring case in Personalization feat: remove duplicate emails ignoring case in Personalization Nov 9, 2020
Copy link
Contributor

@eshanholtz eshanholtz left a comment

Choose a reason for hiding this comment

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

Generally this looks good, but doesn't work when using just the add_to/add_cc/add_bcc functions:

p = Personalization()
to_email = To('[email protected]', 'Example To Name 0')
p.add_to(to_email)
p.add_to(to_email)

Since there are several ways of setting/adding emails to these fields, and each set would result in a call to _get_unique_recipients, why not just use that function once in each of the getters? For example:

@property
def tos(self):
   return _get_unique_recipients(self._tos)

@thinkingserious thinkingserious added status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap and removed status: code review request requesting a community code review or review from Twilio labels Dec 3, 2020
@DougCal
Copy link
Contributor Author

DougCal commented Dec 14, 2020

Going to get into updating this soon. Been extremely busy this last month, but going to make time for this when I can. Haven't forgotten this!

@DougCal
Copy link
Contributor Author

DougCal commented Dec 21, 2020

@eshanholtz ready for re-review! Loved the suggestion- cleaned up the code a lot. I tested making requests locally to be sure it prevented the duplicate email error and it worked quite well.

Copy link
Contributor

@eshanholtz eshanholtz left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

@eshanholtz eshanholtz merged commit fdf5c49 into sendgrid:main Jan 29, 2021
@DougCal DougCal deleted the remove_duplicate_emails_in_Personalizations branch February 18, 2022 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate emails in the Personalization object are not allowed, make this obvious
3 participants