-
Notifications
You must be signed in to change notification settings - Fork 12
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
Monthly donation email hook #215
Conversation
from revolv.lib.mailer import send_revolv_email | ||
|
||
|
||
class Command(BaseCommand): |
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 we call this something semantic? How about MonthlyDonationEmailCommand
?
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.
Django expects something called Command =(
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.
Oh bleh, okay
We need tests! |
Tests currently fail. Main reason why is that we can't override the created_at variable for the payments. Will most likely implement this the first way mentioned in that link, unless you'd prefer some other way of fixing this issue. |
…ould be overriden
Updated factory boy so we can override the created_at attribute! Passes tests now, let me know if you would like more test coverage. |
date_of_last_month = datetime.date.today().replace(day=1) - datetime.timedelta(days=1) | ||
|
||
# creates two payments and sets their created_at date to a date from last month | ||
Payment.factories.base.create( |
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.
You actually don't need to specify all of these values, you can just say Payment.factories.donation.create(created_at=...)
. Everything else doesn't really matter, so no need to include it
@erictshen I think we should add two more tests: (1) one where we mock out the return value of Also, it might make sense to send the admins an email when this job is done running, like "An automatic batch of monthly donation engagement emails was sent" or something like that. What do you think - should be pretty easy to do, since we don't need a fancy email template and I believe we have a utility function to get all administrator emails already. |
context, [user.email] | ||
) | ||
else: | ||
print "Attempted to run monthlydonationemail command on a day that was not the 1st of the month" |
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 we say that the command was not actually run? Or something about using --override to force it? Also we should at least end this sentence with a period :)
Added additional test coverage. Also added a command line arguments to email administrators when the monthlydonationemail command is run, I can make it default behavior if you'd prefer. |
Two more things!
New tests look good! |
|
Ship it!
|
Monthly donation email hook
Fixes #125
Added a command for running monthly donation email jobs. Will add a job to heroku when this gets merged in.
@andylouisqin will also be working on email text once this gets merged in.
Example email from the terminal.