Course email for instructors - #555
Conversation
|
Nice! We used SES for 6.002x, and the issues we ran into were:
With regards to error handling, it's also be nice to know if there is some gross issue (e.g. a quarter of the e-mails are not delivered) Analytics/tracking: It would be good if we could log when/if e-mails were sent to students in some way (e.g. log user, time, subject of e-mail, and hash of body/e-mail). This is a one-liner change. |
There was a problem hiding this comment.
Small style issue: we prefer to have docstrings surrounded by three double quotes, not single quotes.
|
|
A more specific comment: the terminology for the "to" option is inconsistent. In the model, it is called "to" (too short), but the choices are "TO_OPTIONS". In the delegating task, it is called "recipients" (which sounds like a list of emails). It would be good to standardize on something, and I suggest "to_option". I would also suggest a name change of the "hash" field to something indicating its purpose -- like index_key. Optout contains email and course_id. Is there a reason for using email instead of user_id? In particular, a user can change an email address. Do we want to store created/modified times on this model, or are we relying on logging to track changes? (I think Piotr pointed out the lack of tracking logs -- maybe that is enough.) |
|
The fixes look great! However I suspect the rebase will be challenging - between instructor dash, i18n, and various other changes that have landed recently. I recommend you do rebase sooner than later, to avoid the headache. |
There was a problem hiding this comment.
Internationalize all your user-visible strings. https://edx-wiki.atlassian.net/wiki/pages/viewpage.action?pageId=12517501
|
@talbs is there any place shared between LMS and CMS where things like problem-editor-icons.png can go? |
There was a problem hiding this comment.
Why not use @require_POST ?
There was a problem hiding this comment.
you're right, will update. Several other functions in this file should use decorators as well.
|
@cahrens, sorry for the delay. RE: your question around storing common bits, we do have a /common/static/images/ directory. It looks like it has some things (green problem checkmark icon) that are meant to be shared across apps that render our courseware, but there is a lot of junk/old files there as well. If I'm putting 2 and 2 together correctly, you're looking for a place to store templates. There's /common/templates. Hope that helps. If you're looking for help/recommendations on something specific let me know. |
There was a problem hiding this comment.
This .count() might do another query. The previous line materialized the entire recipient list, just use len(recipient_list).
There was a problem hiding this comment.
If optouts is a set because of duplicates, should num_optout be calculated on the size of the set? Actually it looks like it's only used to return a value at the end. If we want it to be an accurate number, it should be calculated as the change in the to_list before and after being filtered in the next line. If people opt out and then unenroll, there's no point in counting their optouts.
There was a problem hiding this comment.
it's not because of duplicates (see querying using pk). it's just there to speed up the filtering of to_list.
optouts is already a subset of the emails in to_list, and won't have duplicates since we're using pk to query also User.email and (Optout.user, Optout.course) have to be unique. So num_optout is accurate since each of its members will be stricken from to_list.
but we can change it to use len(set) just to be consistent.
|
thx for all the comments @brianhw , I'll take a pass at addressing them tomorrow morning |
Adds a new Email link to the instructor dashboard for frontend interface to send email to course members. Adds a feature flag ENABLE_INSTRUCTOR_EMAIL to toggle this. Creates a new djangoapp bulk_email that handles this action by getting the recipient list and batching the emails to different celery tasks to do the actual sending. Requires lynx package to convert HTML email to plaintext. Handles SMTP errors by retrying or falling through to the next email. Adds the option to opt out of course specific emails in the user dashboard with an Email Settings link for each course. Uses severable configurable settings with defaults. DEFAULT_BULK_FROM_EMAIL specifies the from address for email. EMAILS_PER_TASK specifies the number of emails each celery task takes on. EMAIL_HOST, EMAIL_PORT, EMAIL_HOST_USER, EMAIL_HOST_PASSWORD, and EMAIL_USE_TLS for the SMTP email backend settings. Co-authored-by: Akshay Jagadeesh <akjags@gmail.com>
Delay for possible race condition with fetching course email object. Use settings.SITE_NAME for host name to generate email footer url.
Switch to using decorators; refactor and cleanup tests.
Some minor pep8 cleanup
Includes Data + Schema migrations for optout email -> user.id change. Note that migrations should be reversible.
Adds the edX Marketing-approved template as html default.
…ents-params empty default for comments_service_key and comments_url because it was b...
…t-social-event Fix getting social login event openedx#341
9 participants
Updated from PR #437 with squashed commits for cleaner history and continued work.
Adds HTML editor for the email ported from CMS, which has raised some concerns that are still being discussed.
Unicode workaround for known issue Stanford-Online#3