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

Support attachments in SendGridAdapter #294

Merged
merged 3 commits into from
Nov 9, 2017

Conversation

randycoulman
Copy link
Contributor

WARNING This PR is built using the v3 SendGrid API that is currently pending in #293, so that PR should be merged first and then this PR rebased on top of master. All of the diffs that are unique to this PR start at commit dc8e927.

Adds attachment support to SendGridAdapter. The code and tests are very similar to MandrillAdapter.

If #292 gets merged before this PR, I'm happy to make the necessary changes here.

@gitviola
Copy link
Contributor

@randycoulman #292 is merged 🎉 🌮

@randycoulman
Copy link
Contributor Author

@schurig I saw that. My open source time is limited, but I'll try to get a rebase done when I can in the next day or two.

@randycoulman
Copy link
Contributor Author

I rebased #293 onto the latest master and then rebased this branch on top of that one.

@randycoulman
Copy link
Contributor Author

OK, changes made. This one should be ready to go once #293 has been merged.

@manukall
Copy link

hey, we've tried this branch and it works great when sending emails with attachment. it breaks for emails without attachment however.
sendgrid requires the body to not include the attachments key at all in such cases. adding a guard clause when attachments != [] to put_attachments fixed the issue for us.

@randycoulman
Copy link
Contributor Author

@manukall Oh, good catch! I thought I'd tested that in our application, but perhaps not. I'll try to address that as soon as I can (probably in a day or two).

@randycoulman
Copy link
Contributor Author

@manukall I just pushed up another commit that addresses the issue you found. I did it a bit differently: Email's attachments field is always an array and not nil, so we don't need the final clause that I had written; instead, I've added an earlier clause that handles the empty array case. Thanks for catching this!

@javierjulio
Copy link

@randycoulman thanks for doing this! I just started a small Elixir project last Thursday that we've been using in production since then. Its just meant to send 3 CSV data dump reports as an email attachment using SendGrid. This has worked great for us! No issues sending emails with attachments from a local file or using raw data. Its worked flawlessly. Please let me know if you need help/confirmation on changes here as I can help test it as it gets reviewed. Thanks again! ❤️

Copy link
Contributor

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

@randycoulman Thanks so much for this! Could you rebase to get the conflicts in mix.lock fixed? Once that's done I can merge this in :D

@paulcsmith paulcsmith closed this Nov 8, 2017
@paulcsmith paulcsmith reopened this Nov 8, 2017
@randycoulman
Copy link
Contributor Author

@paulcsmith I'll rebase once #293 has been merged.

@paulcsmith
Copy link
Contributor

#293 is in! Thanks so much for the quick response!

@randycoulman
Copy link
Contributor Author

@paulcsmith Rebased!

@paulcsmith paulcsmith merged commit 9783c19 into beam-community:master Nov 9, 2017
@paulcsmith
Copy link
Contributor

Thank you! :D

@randycoulman randycoulman deleted the sendgrid-attachments branch November 9, 2017 16:44
@henry-hz
Copy link

great feature. let me suggest to add attachment to emails direct from a function call (instead of saving on the disk before sending).

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.

6 participants