Add management command for sync suppressed emails #21784
Conversation
| """Sync Socket labs suppression list to database.""" | ||
|
|
||
| def handle(self, *args, **options): | ||
| sync_blocked_emails.apply() |
There was a problem hiding this comment.
I think using apply might be the right thing here. Then the task executes immediately, and logs are printed to the console where the management command is executed.
Happy to hear feedback on that but it felt right when executing locally.
KevinMind
requested review from
a team and
eviljeff
and removed request for
a team
| @@ -139,6 +146,9 @@ def sync_blocked_emails(batch_size=BATCH_SIZE, **kw): | |||
|
|
|||
| email_blocks = [SuppressedEmail(email=record[3]) for record in batch] | |||
| SuppressedEmail.objects.bulk_create(email_blocks, ignore_conflicts=True) | |||
| count += len(batch) | |||
There was a problem hiding this comment.
maybe we want some logging every batch to show progress?
There was a problem hiding this comment.
I don't think it's necessary, at least not yet. Only added the logging here because the download part can take some time to run and without any logs it can be confusing and made me wonder if something was wrong with the code. I added the count at the end to just verify that we have synced "something". The syncing is really fast due to the batching so it wouldn't add much value IMO.
There was a problem hiding this comment.
The issue might explain further, but if this is something that is expected to be run as a cron, with no args or options, it would probably be better implemented as a simple function in cron.py instead?
There was a problem hiding this comment.
This PR is a simple convenience for testing and if we ever want to manually resync the suppression list. We should run this task on a periodic schedule. The issue will likely not explain much unfortunately.
There was a problem hiding this comment.
You can do that with a simple function in cron.py too 🙂
There was a problem hiding this comment.
Can I trigger a function in cron in both a management command and periodically? Just curious? Also would there be retry logic if implemented that way?
There was a problem hiding this comment.
in crontab.tpl we execute both management commands directly, and simple functions via the cron management command.
There is no retry logic in our cron setup (other that it'd be executed again the time it was scheduled). The cron.py function vs. a separate management command .py file is just about the initial execution - in both cases you'd have to either write some custom retry logic, or wrap in a task and set a retry on the task.
relates to: mozilla/addons#9426
Description
This pull request adds a new management command for syncing suppressed emails from Socket Labs to the database. The command fetches the suppression list from Socket Labs, downloads it, and then bulk creates the suppressed emails in the database. The pull request also includes a task for sending suppressed email confirmations.
Context
This PR exposes the sync_blocked_emails task via a management command.
This can be useful for a few reasons:
In general the sync_blocked_emails task will be run periodically. This is not in production yet as we have only just recently been given the necessary credentials to communicate with socketlabs via API and having that job executing until now would have simply erred indefinitely.