fix: make bulk_email send email task handles a retryable exception#29080
fix: make bulk_email send email task handles a retryable exception#29080hurtstotouchfire merged 3 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @ghassanmas! I've created OSPR-6157 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
4afe3ca to
9e1f811
Compare
|
This PR is ready to be reviwed. |
|
@ghassanmas Thank you for your contribution. Please let me know once this is ready for our review. |
9e1f811 to
52a6367
Compare
52a6367 to
91e7a0e
Compare
|
Your PR has finished running tests. There were no failures. |
|
@natabene It's ready now that all tests have passed |
91e7a0e to
8182dc5
Compare
8182dc5 to
d7156e0
Compare
According to bulk_email and SMTP doc, when an error code is between (400-499) it can be retried after sometime, this commit adds a new type of exception called SMTPSenderRefused, so it can be retried **if** it's code fails under above constraint.
a8f76dd to
0d56d98
Compare
|
@natabene any chance I might a look for this soon? |
|
@ghassanmas Let me see what I can do. |
|
Thanks @ghassanmas we will take a look at this. Sorry for the delay, it has been a pretty eventful couple quarters for us. |
justinhynes
left a comment
There was a problem hiding this comment.
Hey @ghassanmas, thanks for your contribution!
I would love to see a test for the new logic added before accepting this change.
The bulk email tasks tests can be found here: /lms/djangoapps/bulk_email/tests/test_tasks.py. The relevant test class/suite would be TestBulkEmailInstructorTask.
39c7cfc to
4177b56
Compare
justinhynes
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for adding the test (and for your patience!).
|
It's alright no worries. It was just when a conflict occurs at that point I would have already forgotten what I actually did! Anyway thanks for your review |
|
@ghassanmas We are looking into this and will get back to you soon |
hurtstotouchfire
left a comment
There was a problem hiding this comment.
Tentatively hope to merge this in the morning.
|
@ghassanmas 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
|
EdX Release Notice: This PR has been deployed to the production environment. |
Description
According to bulk_email and SMTP doc, when an error code is between
(400-499) it can be retried after sometime, this commit adds a new
type of exception called SMTPSenderRefused, so it can be retried
if it's code fails under above constraint.
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
The issue have been reported at the forums
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.