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: populate email templates at delivery time, add plaintext defaults #1155

Merged
merged 19 commits into from
Apr 9, 2021

Conversation

mattbonnell
Copy link
Contributor

@mattbonnell mattbonnell commented Mar 17, 2021

Related issue

#1065

  • Persist serialized template models in Messages
  • Add plaintext templates for "text/plain" portion of email body
  • Store populated plaintext templates in Message in DB (for auditing purposes), populate HTML templates at send-time

Proposed changes

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

@mattbonnell mattbonnell marked this pull request as ready for review March 18, 2021 17:44
@mattbonnell mattbonnell changed the title feat: populate email templates at delivery time feat: populate email templates at delivery time, add plaintext defaults Mar 19, 2021
@mattbonnell mattbonnell force-pushed the mattbonnell/feat/courier-plaintext branch from d2bfc93 to 91655e4 Compare March 25, 2021 11:39
@aeneasr
Copy link
Member

aeneasr commented Mar 29, 2021

LMK when good for another 👀

@mattbonnell mattbonnell force-pushed the mattbonnell/feat/courier-plaintext branch from 94c6ba3 to 43f05a7 Compare March 30, 2021 13:21
@aeneasr
Copy link
Member

aeneasr commented Mar 31, 2021

I have resolved the e2e tests so this should work again - rerunning tests now.

@aeneasr
Copy link
Member

aeneasr commented Mar 31, 2021

e2e test pass on master again, so the failures here are related to the changes in the PR

@mattbonnell
Copy link
Contributor Author

e2e test pass on master again, so the failures here are related to the changes in the PR

yep, need to fix some things related to the e2e tests.

@mattbonnell mattbonnell force-pushed the mattbonnell/feat/courier-plaintext branch from 43f05a7 to 9bf5900 Compare March 31, 2021 14:06
@mbonnell-wish
Copy link
Contributor

Hey @aeneasr, I'm wondering - do you think we need the plaintext portion of the email at all, or can we just drop that?

@aeneasr
Copy link
Member

aeneasr commented Apr 1, 2021

Yes, many email clients reject HTML versions as spam or do not display them correctly!

@mattbonnell
Copy link
Contributor Author

Yes, many email clients reject HTML versions as spam or do not display them correctly!

okay cool, will keep plugging away on this one.

@mattbonnell mattbonnell force-pushed the mattbonnell/feat/courier-plaintext branch from de3581b to af824d8 Compare April 5, 2021 13:26
@mattbonnell
Copy link
Contributor Author

seems something wonky is going on with the sdk

@mattbonnell
Copy link
Contributor Author

@aeneasr ready for another review :)

@aeneasr aeneasr self-assigned this Apr 6, 2021
@aeneasr aeneasr self-requested a review April 6, 2021 15:45
@aeneasr aeneasr merged commit 7749c7a into ory:master Apr 9, 2021
@aeneasr
Copy link
Member

aeneasr commented Apr 9, 2021

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

@mattbonnell mattbonnell deleted the mattbonnell/feat/courier-plaintext branch April 9, 2021 10:02
@mattbonnell
Copy link
Contributor Author

Thanks for the reviews, as always! Do you think we'll see this in v0.6?

@aeneasr
Copy link
Member

aeneasr commented Apr 9, 2021

Yes of course :)

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.

4 participants