-
Notifications
You must be signed in to change notification settings - Fork 55
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
Echo emails to the console if we're in sandbox mode. #45
Conversation
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.
Can you make this feature a config option readable in settings.py? And also add tests for this code?
Thanks!
Happy to write a test; would you want to see something along the lines of 'given expected input, assert self.stream.write' is called with that input? |
works for me!
…On Thu, Nov 15, 2018 at 3:00 PM liavkoren ***@***.***> wrote:
Happy to write a test; would you want to see something along the lines of
'given expected input, assert self.stream.write' is called with that input?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB1xRWmm0dkROHYzGjIbnqfM3c6sNeynks5uvcfRgaJpZM4Yhjpz>
.
|
d4b5585
to
77fdd6e
Compare
lol, first I tried to patch sys.stdout to test this, then got really confused that my terminal went unresponsive when I ran the tests. I've added a boolean setting specifically for echo to stdout, although alternatively, the setting could be the stream object that you want to echo to itself. I don't mind changing it over to that if you'd prefer. |
77fdd6e
to
f422872
Compare
f422872
to
a85d74a
Compare
Travis is falling over now at the point where it's trying to get the API KEY from the environment. There's something in the logs about them removing encrypted env vars, is that the problem? |
Yeah to prevent users from potentially exposing environment variables, travis removes those from tests run on PRs. You may need to add your test to this line (which would ignore it in travis builds so not the preferred choice) https://github.com/sklarsa/django-sendgrid-v5/blob/master/.travis.yml#L20 or set the API key using django's settings overrides https://docs.djangoproject.com/en/dev/topics/testing/tools/#overriding-settings |
If I use settings override, how do I point to the API key? |
Hmm you can't get to it until I merge the PR. I'll accept the PR if you feel that the tests should pass, then we can view what happens on the master branch when the key is injected into the environment by travis |
O _ O Oh my. That's.. not nice. Well. The patch and the test both Do The Things on my system, so give it a try. |
Ah looks like tests are still failing for 2 reasons (1 bad import in python 2 and a flake8 error).. I can fix them now |
Hi! I find it extremely useful to be able to see emails in my console while developing. AFAICT, sendgrid doesn't seem to keep a record of what's sent to it while in sandbox mode.
To that end, I've bodged in a thread-safe print-to-console fallback while in sandbox mode, shamelessly based off of Django's ConsoleBackend implementation.
Wanna merge this?