Skip to content

Conversation

@jorenham
Copy link

@jorenham jorenham commented Jan 3, 2017

For sending out large amounts of emails, it is generally a good idea to send them out in batches. This way your email servers aren't flooded and can recover in between batches.

@dokterbob
Copy link
Collaborator

Hey @jorenham,

Thanks for an awesome contrib. Code looks good, though I think it is important to stress that also sub-second delays can be used. Sending 10k emails with 1 second delay is not really feasable. ;)

It would be great, though to also have coverage here. Basically, you mock out time.sleep. In any case, fix the syntax errors! ;)

Mathijs

@coveralls
Copy link

coveralls commented Jan 4, 2017

Coverage Status

Coverage decreased (-0.2%) to 89.451% when pulling dabc3cb on jorenham:master into 549ed2b on dokterbob:master.

@dokterbob
Copy link
Collaborator

dokterbob commented Jan 4, 2017

Very well done, thanks!

LGTM except for one issue: settings in tests need to be overridden in a different way. See for example here: https://github.com/dokterbob/django-newsletter/blob/9d4b93ac875f91fcb9c4938fb3ffccd5b85dea8a/tests/test_web.py#L646

Or look at the Django docs on this topic here https://docs.djangoproject.com/en/1.8/topics/testing/tools/#overriding-settings.

Lastly, you've used Python 3's built-in mock, which is unavailable on 2.7. You need to add the mock library as a test requirement. https://pypi.python.org/pypi/mock

@coveralls
Copy link

coveralls commented Jan 4, 2017

Coverage Status

Coverage decreased (-0.2%) to 89.451% when pulling 52cc44d on jorenham:master into 549ed2b on dokterbob:master.

@jorenham
Copy link
Author

jorenham commented Jan 4, 2017

I see unittest.mock breaks in python 2.7. Should I use Mock instead?

@dokterbob
Copy link
Collaborator

dokterbob commented Jan 4, 2017 via email

@coveralls
Copy link

coveralls commented Jan 4, 2017

Coverage Status

Coverage increased (+0.05%) to 89.705% when pulling 4d8f5c9 on jorenham:master into 549ed2b on dokterbob:master.

@dokterbob dokterbob merged commit d71642c into jazzband:master Jan 5, 2017
@dokterbob
Copy link
Collaborator

Thanks!

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.

3 participants