feat(massEmail): create/register celery tasks mass emails TASK-750 #5574
Conversation
Guitlle
force-pushed
the
750-create-register-celery-tasks-mass-emails
branch
from
c5ad703 to
b0075f7
Compare
noliveleger
requested review from
rajpatel24
and removed request for
jnm,
noliveleger and
rajpatel24
Signed-off-by: Guillermo <lgar89@gmail.com>
…test, limit amount of emails sent
Guitlle
force-pushed
the
750-create-register-celery-tasks-mass-emails
branch
from
d8d0eb2 to
5f47ecd
Compare
Guitlle
requested review from
rgraber and
rajpatel24
and removed request for
magicznyleszek
| f'Processing {limit} records for MassEmailConfig({email_config})' | ||
| ) | ||
| for record in records: | ||
| self.send_email(email_config, record) |
There was a problem hiding this comment.
We should decrement the limit by 1 before sending each email. That way we won't risk the job dying in between sending the emails and updating the limit. Also it's safer to say we sent an email when we didn't than to say we didn't send an email when we did, which is why we should decrease and then send rather than the other way around.
There was a problem hiding this comment.
Yes, that's better, we should not have it all in one dict but have separate values for each config, that way it is faster
| now_mock_B.return_value = now_mock.return_value | ||
| send_emails() | ||
| assert len(mail.outbox) == 20 | ||
| now_mock.return_value = datetime(2025, 1, 2, 0, 0, 0, 0, pytz.UTC) |
There was a problem hiding this comment.
nit: since the return value hasn't changed, do we need to reset it?
There was a problem hiding this comment.
You're right, it's unnecessary
| f'Processing {limit} records for MassEmailConfig({email_config})' | ||
| ) | ||
| for record in records: | ||
| self.send_email(email_config, record) |
There was a problem hiding this comment.
I agree with Becca. It would be better to decrease the limit as each email is sent instead of setting it to zero immediately. This way, if the task fails before finishing, the remaining emails for that config can still be processed in subsequent runs.
There was a problem hiding this comment.
I do think we want to decrement the counter before sending the email though. It's a little counter-intuitive but it reduces the risk of sending too many emails because the task was canceled or died between sending the email and decrementing the counter
| rendered = render_template(self.template, data) | ||
| assert 'Username: Test Username' in rendered | ||
| assert 'Full name: Test Full Name' in rendered | ||
| assert 'Plan name: Test Plan Name' in rendered |
There was a problem hiding this comment.
Would it be possible to add a test to ensure that the limits decrement as each email is sent, rather than being reset immediately? :-)
There was a problem hiding this comment.
Yes, I can make that unit test
kobo/apps/mass_emails/tasks.py
Outdated
|
|
||
| def __init__(self): | ||
| self.today = localdate() | ||
| self.cache_key_prefix = f'mass_emails_{self.today.isoformat()}_email_limits' |
There was a problem hiding this comment.
What do you think about changing this to emails_remaining instead of email_limits? By no means mandatory but I think it would be a little clearer.
| f'Processing {limit} records for MassEmailConfig({email_config})' | ||
| ) | ||
| for record in records: | ||
| self.send_email(email_config, record) |
There was a problem hiding this comment.
I do think we want to decrement the counter before sending the email though. It's a little counter-intuitive but it reduces the risk of sending too many emails because the task was canceled or died between sending the email and decrementing the counter
|
For posterity: to test this, I had to update the |
kobo/apps/mass_emails/tasks.py
Outdated
| self.limits[email_config.id] = stored_limit | ||
| else: | ||
| logging.info('Setting up MassEmailConfig limits for the current day') | ||
| MAX_EMAILS = settings.MAX_MASS_EMAILS_PER_DAY |
There was a problem hiding this comment.
The declaration of MAX_EMAILS = settings.MAX_MASS_EMAILS_PER_DAY is redundant as it is already defined above
There was a problem hiding this comment.
The declaration of
MAX_EMAILS = settings.MAX_MASS_EMAILS_PER_DAYis redundant as it is already defined above
Do we need this? I think it is redundant and should be removed
| ) | ||
| if total_limit + config_limit > MAX_EMAILS: | ||
| config_limit = MAX_EMAILS - total_limits | ||
| self.cache_limit_value(email_config, config_limit) |
There was a problem hiding this comment.
Please correct the undefined variable total_limits to total_limit
There was a problem hiding this comment.
I think it's supposed to be total_limits, since the idea is just to give this config however many slots we have left.
There was a problem hiding this comment.
I fixed it and changed the name to avoid confusion with existing names
Signed-off-by: Guillermo <lgar89@gmail.com>
🗒️ Checklist
<type>(<scope>)<!>: <title> TASK-1234frontendorbackendunless it's global📣 Summary
Celery task and admin action to send emails for mass email configurations asynchronously by user request on the admin screen.
📖 Description
The mass email configs admin action "Send emails" triggers a celery task for each email config that creates or uses an existing mass email job that is associated to a set of mass email records. The records are linked to a user, they start with enqueued status and when they are processed by the celery task, their status is set to sent or failed. This is work in progress, future PRs will enable more functionality.
👀 Preview steps
EMAIL_BACKEND = 'django.core.mail.backends.filebased.EmailBackend'to debugkobo-docker/log/kpi/celery_kpi_worker.logand ensure that they mention the emails sent.