Flowerhack/feature/bulkemailnewdash

[flowerhack]

Bulk email added to new dashboard

@sarina @adampalay

One question I had: I looked into this and, on my local machine, it seemed like I needed to set ENABLE_INSTRUCTOR_EMAIL to True in lms/envs/common.py to make bulk emailing work. Does anyone have any confirmation or insight on this? I'm a little nervous changing a massive settings file for fear of blowing something else up

[adampalay]

Yes, so that's how bulk email will eventually be turned on. Keep it set at False for now, and when the feature is ready to be released, we'll switch it to True. The changes you're making in this PR are invisible for now.

[adampalay]

Don't you want to check if has_instructor_access is True or False? And is this the appropriate HttpResponse? (I'm not sure; would it be 403?)

[sarina]

You should definitely not ever introduce more pep8 or pylint errors if you can possibly help it.

http://jenkins.edx.org:8080/job/edx-feature-branch-tests/12398/Diff_Quality_Report/?

Your test coverage is really low: http://jenkins.edx.org:8080/job/edx-feature-branch-tests/12398/Diff_Coverage_Report/? @wedaly is the LMS team's test engineering liaison. Don't hesitate to reach out to him if you need help understanding how to write tests for your code.

[sarina]

Parameters

[sarina]

Issues:

• Visually there's been a bunch of regressions. Messages don't pop up to indicate sends have occurred. The spacing of the send to and subject lines are compressed. Text below the box is squished and not bolded. Button says "Send" instead of "Send Email". Compare existing:
  
  to your branch:
  
• I don't see the email go by in my console output so I don't think it's actually sending the email correctly.

[sarina]

Feel free to add in ENABLE_INSTRUCTOR_EMAIL back to dev.py. It was accidentally taken out in this commit: edx@ccc3589#diff-a26715f68224f0a34f87a95db99e9cf4

[sarina]

I'd like @jbau and @talbs to take a quick pass at this pull request.

[flowerhack]

re: not seeing the email go by in the console: that's odd, since I see the email go by in my console locally—I'll look into what may be causing the issue, working on another commit with tests + better visuals now

[sarina]

OK. I think my dev env could be wonky as well so if you can see it I'm less concerned. We will be testing your changes in the development server once they're merged in os if there are any issues we will be able to see them before this goes live.

[sarina]

Also:

• Add yourself to AUTHORS
• Add an entry to CHANGELOG (like commit messages, keep it simple! Always ideally < 70 characters. Eg, "Ported bulk email to beta dashboard")

[jbau]

The scope of this may not just be limited to this PR (since from a cursory glance it appears the rest of the new dashboard works this way), but I'm pretty sure that we don't want to be passing the message in a GET parameter. That seems like a lot of data to stuff into a URL. I think this should naturally be a POST, no?

[jbau]

Here's some nginx documentation on the size limits of http request headers:
http://wiki.nginx.org/NginxHttpCoreModule#large_client_header_buffers
It seems to me that it's not impossible to exceed 8K for HTML formatted email messages, so I think it has to be POST.

[jbau]

also, should probably have some unit tests for the python functionality?

[sarina]

also, should probably have some unit tests for the python functionality?

Yeah, and probably some acceptance and Javascript tests as well. In an earlier comment I referred Julia to Will if she's unfamiliar with how to write any of these tests.

[wedaly]

This is a short function, but there's a lot to test here. I'd start by thinking about possible paths:

1. User is logged in and has instructor access; course exists (happy path)
2. User does not have instructor access.
3. User is not logged in.
4. Course does not exist.

In addition, there are some unhappy paths to test:

1. Request is missing the "send to" parameter.
2. Request is missing the "subject" parameter.
3. Request is missing the "message" parameter.

I'd also expect at least one test with non-ASCII unicode in each of the user-specified fields (this could be your happy-path test). In Python, using unicode incorrectly can cause exceptions, which means users will see a 500 error. Ned gave a great talk about this: see http://nedbatchelder.com/text/unipain.html

Since this code involves JavaScript / Python integration, I'd expect one happy-path test at the UI level. This is something I can help with, since we don't have any existing UI-level tests for the instuctor dash.

[flowerhack]

Closing in favor of https://github.com/edx/edx-platform/pull/1215