-
Notifications
You must be signed in to change notification settings - Fork 72
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
Allow template for sendgrid (if exists) #2728
Conversation
Passing run #611 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2728 +/- ##
=======================================
Coverage 86.59% 86.60%
=======================================
Files 290 290
Lines 16224 16242 +18
Branches 2062 2065 +3
=======================================
+ Hits 14049 14066 +17
- Misses 1792 1793 +1
Partials 383 383
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@adamsachs after reading your message on messaging ( 😏 ) thought you might be ok to review this. Adding a bit early as I grapple with codecov 🤼 and wrap up any admin stuff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the way this looks! simple and clean implementation. the only thing i found in the code is that hardcoded limit that you've called out, due to the limitation in the SDK/client - i think your decision to hardcode is totally fine for now, but i had a couple of notes on it. nothing critical but let me know what you think. and let me know if you're running into any issues grappling with codecov, happy to lend a hand if i can.
also, is there a fidesdocs
ticket to update our docs explaining how to use this functionality? let's make sure we at least ticket that before merging this in - ideally we can also just quickly make those doc updates since we've got a lot of the necessary content about templating already there for mailgun it seems, and it feels like the type of thing that if we don't update now, it may just sit stale unnecessarily.
last thing - were you able to run a manual end-to-end test actually using a template in sendgrid? happy to help with that if needed - but seems like you've likely done that considering the pagination bug you've found? (i just want to be sure!)
Docs issue created -> fidesdocs#68 |
Closes #1866
Code Changes
Handle paging of template IDsUse max for now and circle back (as needed)Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
Description Of Changes
This is following the same path that was set out for Mailgun, only minor differences exist in how the template is discovered and applied. The pagination for the SDK is not functioning, so to hit the release and not over-engineer I just ended up setting the max. Will add a comment in the files changed as well to highlight and see if we want to add a follow on issue now or wait until it becomes a problem