Skip to content

Allow creation of Mail from EmailMessage #631

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 30, 2018
Merged

Allow creation of Mail from EmailMessage #631

merged 3 commits into from
Oct 30, 2018

Conversation

@cmccandless
Copy link
Contributor

@cmccandless cmccandless commented Oct 4, 2018

Fixes #571

Checklist

  • 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 master 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:

  • Allow direct creation of Mail object from an instance of EmailMessage

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 4, 2018
@codecov
Copy link

codecov bot commented Oct 4, 2018 •
edited
Loading

Codecov Report

Merging #631 into v4 will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v4     #631      +/-   ##
==========================================
+ Coverage   84.38%   84.54%   +0.15%     
==========================================
  Files          37       36       -1     
  Lines         967      977      +10     
  Branches      108      108              
==========================================
+ Hits          816      826      +10     
  Misses         93       93              
  Partials       58       58
Impacted Files Coverage Δ
sendgrid/helpers/mail/mail.py 100% <100%> (ø) ⬆️
sendgrid/sendgrid.py 85.71% <0%> (ø) ⬆️
sendgrid/version.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33a3576...d1f3545. Read the comment docs.

@cmccandless cmccandless force-pushed the support-email.message.EmailMessage branch 3 times, most recently from 839deb4 to 7eddee4 Compare October 4, 2018 13:33
@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap hacktoberfest difficulty: medium fix is medium in difficulty labels Oct 9, 2018
@thinkingserious
Copy link
Contributor

thinkingserious commented Oct 9, 2018

Hello @cmccandless,

Thanks again for the PR!

It's HACKTOBERFEST! We want to show our appreciation by sending you some special Hacktoberfest swag. If you have not already, could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@thinkingserious
Copy link
Contributor

thinkingserious commented Oct 9, 2018

This is awesome @cmccandless!

Could you please merge this into the v4 branch? Thanks!

@cmccandless cmccandless changed the base branch from master to v4 October 9, 2018 13:10
@cmccandless cmccandless force-pushed the support-email.message.EmailMessage branch from 7eddee4 to 064978b Compare October 9, 2018 13:11
@cmccandless cmccandless force-pushed the support-email.message.EmailMessage branch from 064978b to 6909a88 Compare October 9, 2018 13:23
@cmccandless cmccandless force-pushed the support-email.message.EmailMessage branch from 6909a88 to 8e4e721 Compare October 9, 2018 13:33
misterdorm
misterdorm approved these changes Oct 24, 2018
View reviewed changes
Copy link
Contributor

@misterdorm misterdorm left a comment

Choose a reason for hiding this comment

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

Thanks!

@misterdorm misterdorm added status: work in progress Twilio or the community is in the process of implementing and removed status: code review request requesting a community code review or review from Twilio labels Oct 24, 2018
42B
42B reviewed Oct 27, 2018
View reviewed changes
test/test_mail.py Outdated
self.assertEqual(mail.from_email.email, 'test@example.com')
self.assertEqual(len(mail.personalizations), 1)
self.assertEqual(len(mail.personalizations[0].tos), 1)
self.assertDictEqual(mail.personalizations[0].tos[0], {'email': 'test@sendgrid.com'})
Copy link

@42B 42B Oct 27, 2018

Choose a reason for hiding this comment

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

Python docs show that assertEqual will already use assertDictEqual to compare dicts by default and that it isn't necessary to call it directly.

Copy link
Contributor Author

@cmccandless cmccandless Oct 27, 2018

Choose a reason for hiding this comment

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

Good to know! I'll make the correction.

@cmccandless
Use assertEqual instead of assertDictEqual
d1f3545 
assertEqual will call assertDictEqual
@thinkingserious thinkingserious merged commit bb16f15 into sendgrid:v4 Oct 30, 2018
@cmccandless cmccandless deleted the support-email.message.EmailMessage branch November 1, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@misterdorm misterdorm misterdorm approved these changes

+1 more reviewer

@42B 42B 42B left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

difficulty: medium fix is medium in difficulty status: work in progress Twilio or the community is in the process of implementing type: community enhancement feature request not on Twilio's roadmap

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cmccandless @thinkingserious @misterdorm @42B