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: type validation on to_emails parameter on mail object #920

Merged

Conversation

DougCal
Copy link
Contributor

@DougCal DougCal commented Jul 4, 2020

Fixes #778

Validated values at runtime for a clearer error message for when someone sets to_emails to a list of lists

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

Thanks for the PR! Everything looks good, but would you please add tests to verify no error is thrown with the list(tuple), list(str), str, and tuple formats, for completeness? Looks like all the other tests use To and list(To) formats.

@DougCal
Copy link
Contributor Author

DougCal commented Jul 7, 2020

Tests are in @eshanholtz !

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 c51b429 into sendgrid:master Jul 7, 2020
@honzajavorek
Copy link
Contributor

This breaks my code, the type check now won't allow anything else than To:

Mail(to_emails=[To(job.email, job.company_name),
                Bcc('[email protected]', 'Honza')], ...)

Do I use to_emails in a wrong way or is it a regression?

@eshanholtz
Copy link
Contributor

@honzajavorek

The original intention was for the to_emails to refer specifically to the "to" portion of the email, but testing locally shows that it properly rendered Cc and Bcc emails the way you were using it. I'll work on a fix to allow these recipient types and will release a new patch today. Thanks for pointing this out!

@honzajavorek
Copy link
Contributor

@eshanholtz Thank you! What is the preferred way of doing this if this worked only by accident? Also, let me know if I can help with the fix.

@eshanholtz
Copy link
Contributor

eshanholtz commented Jul 10, 2020

@honzajavorek I'd say the way you were doing it is a perfectly valid use of to_emails. The patch release to fix the break of this use case is version 6.4.3

@honzajavorek
Copy link
Contributor

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6.0.2 fails to send to list of lists
4 participants