feat: update course deadline email cadence and templates#4709
feat: update course deadline email cadence and templates#4709rjv31 wants to merge 1 commit intoopenedx:masterfrom
Conversation
3ca6f2b to
72dbfc6
Compare
0d747a2 to
4b4f473
Compare
4b4f473 to
b818f2b
Compare
ankit-sonata
left a comment
There was a problem hiding this comment.
Must-fix before merge: there is a logic bug that prevents “course_ended” emails from being sent in common cases, and there’s also a crash risk when there’s no course run.
1) BUG: “course_ended” path is effectively unreachable for courses with any old runs
In send_course_deadline_emails.py, the query selects courses where any course_runs__end matches the deltas:
days_until_end = TruncDate(F('course_runs__end')) - TruncDate(Now())
.filter(days_until_end__in=[timedelta(days=d) for d in EMAIL_DELTA_DAYS])Because EMAIL_DELTA_DAYS includes -1, any course that has any run that ended “yesterday” will be included.
But then inside the loop, you compute days_until_end using only the advertised run:
advertised_run = course.advertised_course_run
days_until_end = (advertised_run.end.date() - now.date()).daysSo if a course has:
- an advertised run ending in 60 days, and
- an older run that ended yesterday
…it will be included by the queryset due to the old run, but days_until_end will be 60 and you’ll send the 60-day email. That’s OK.
However, for courses with no advertised_run, you do:
last_course_run = course.course_runs.last()
days_since_end = (last_course_run.end.date() - now.date()).days
if days_since_end == -1: send course_endedcourse.course_runs.last() is not ordered, so “last” is not guaranteed to be the most recently-ended run. If it picks an older run (common if default ordering is by PK asc/desc, or undefined), days_since_end likely won’t be -1, and the course_ended email won’t send, even if there is a run that ended yesterday (which is exactly why the course was selected in the queryset).
Fix required:
- Explicitly choose the correct run for the ended-path, e.g.:
- order by
endand select the most recent ended run (order_by('-end').first()), or - compute “ended yesterday” run directly in the queryset and use that run, or
- remove the
-1from the broad course filter and handle ended runs separately with an explicit query.
- order by
2) Crash risk: course.course_runs.last() can be None
Even though the queryset filters course_runs__end__isnull=False, it’s still safer to guard because:
- the queryset is about any run, but prefetch + distinct + iteration can produce edge cases; and
- defensive code is important in management commands.
Right now this will crash if last_course_run is None:
last_course_run = course.course_runs.last()
days_since_end = (last_course_run.end.date() - now.date()).daysFix required:
- Add a guard:
- if no runs exist, log and continue.
3) Missing must-have tests for the fixed behavior
The command tests were updated for (60, 30, 15, 7, -1) variants, but there should be an explicit regression test for:
- A course with no advertised run and multiple historic runs, where one ended yesterday: ensure
course_endedtriggers. - A course where
.last()would pick the wrong run unless ordering is applied.
ankit-sonata
left a comment
There was a problem hiding this comment.
@rjv31 can you tell me the purpose of the following file - course_deadline.txt
|
Hi @ankit-sonata , The It’s used along with the HTML template ( Also, this is part of the existing/legacy pattern followed in Discovery from earlier teams, where both |
|
@UsamaSadiq , @openedx/committers-course-discovery could anyone please check and help review/merge this PR |
|
@rjv31 PR can approved by someone who has write access. |
|
@UsamaSadiq Could you please check and help review/merge this PR |
Summary
This PR updates the automated course deadline email notifications sent to Course Editors and Project Coordinators.
2U ticket - https://2u-internal.atlassian.net/browse/CATALOG-80
Changes
Updated reminder cadence from:
to:
Updated email templates (
course_deadline.htmlandcourse_deadline.txt) to match the new messaging format.Updated email subjects according to the new requirements.
Updated management command to support the new reminder intervals.
Updated test cases to reflect the new cadence.
Testing