-
Notifications
You must be signed in to change notification settings - Fork 11
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
Jinja email templates #113
Conversation
Oh, wait, I haven't added the new templates yet. Better do that :) |
Yep, fixed those. |
The code looks good to me on the service, but have you tested it? It looks like it's causing 500 errors, judging by the test results. |
How would it cause 500s, unless Jinja's doing something bad?
|
I'm going to review this locally. |
Looks like it's throwing up due to non-ascii characters in the email:
I fear that if we had this stuff wrapped in a try/catch we may not have seen this error 😉 |
This is probably good to know 😉
|
Hmm, looks like I have to just unicodify the template sources and then wrap On Thu, Jul 17, 2014 at 10:42 PM, L. Mars shakespeares.integral@gmail.com
|
@brad Tests aren't passing but it works locally. |
@brad Travis says it's OK, it works OK locally. Can we pull and deploy to dev to test? |
Added Unicode on email to be sent to. |
Thanks for the PR @brad |
No problem, I think maybe we should just do a |
Is this merge-able yet @brad? |
Does this look OK @jwhite66? |
@ArchimedesPi My last comment hasn't been addressed yet. This is probably fine as is though. |
I'm a little concerned that there is enough being changed in this, that it should get careful review and a careful test. I'd like to get our current effort on addresses done, and then maybe get this code live on dev for some testing and more review. (And it'd be nice to get this in so we can stop bugging Mailchimp every few minutes; our task queues are full of bad emails being subscribed over and over again :-( ). |
I want to know if my rebased branch tests OK, but Travis is having problems.
|
decode the name so the tests will pass
Thanks @brad, after rebasing against upstream/master I broke my history vs the PR's :( |
No problem. I love to see Travis happy |
Hey @brad, is this pushed to pledgedev? |
Don't think so, @jwhite66 has a way to specify a specific branch on pledgedev. |
Hey @aaronlifshin, can I test this out on pledgedev? |
Pushed to pledgedev. Please test it quickly, as it is now in the main branch. A On Fri, Aug 1, 2014 at 9:03 AM, Liam M notifications@github.com wrote:
|
Testing... |
@aaronlifshin It "works", but it looks like this. Should it look like this? |
I think it stripped the newlines. Any ideas @brad? |
No. It should not look like that. Please fix it or roll it back. A On Sun, Aug 3, 2014 at 6:44 PM, Liam M notifications@github.com wrote:
|
OK, looks like the newlines are wonky. Recreating the newlines in the
|
@ArchimedesPi this one was an extremely frustrating pull request. Now it's blocking priority work, and I have to fix and re-test the issues your changes have created. |
Sorry. Github has a "revert PR" button, you can use that to revert it. On Mon, Aug 4, 2014 at 11:10 AM, aaronlifshin notifications@github.com
|
I did. On Mon, Aug 4, 2014 at 11:51 AM, Liam M notifications@github.com wrote:
|
No biggie, just reverted it. On Mon, Aug 4, 2014 at 11:52 AM, Liam M notifications@github.com wrote:
|
Addresses #109. @brad does this look OK?