-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Flowerhack/feature/bulkemailnewdash #1180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8453ae2
2bf866c
750f2fe
603d265
7c1c6f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,10 @@ | |
| import analytics.distributions | ||
| import analytics.csvs | ||
|
|
||
| from bulk_email.models import CourseEmail | ||
| from html_to_text import html_to_text | ||
| from bulk_email import tasks | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
|
|
@@ -665,6 +669,44 @@ def extract_user_info(user): | |
| return JsonResponse(response_payload) | ||
|
|
||
|
|
||
| @ensure_csrf_cookie | ||
| @cache_control(no_cache=True, no_store=True, must_revalidate=True) | ||
| @require_level('staff') | ||
| @require_query_params(send_to="sending to whom", subject="subject line", message="message text") | ||
| def send_email(request, course_id): | ||
| """ | ||
| Send an email to self, staff, or everyone involved in a course. | ||
| Query Paramaters: | ||
| - 'send_to' specifies what group the email should be sent to | ||
| - 'subject' specifies email's subject | ||
| - 'message' specifies email's content | ||
| """ | ||
| course = get_course_by_id(course_id) | ||
| has_instructor_access = has_access(request.user, course, 'instructor') | ||
| send_to = request.GET.get("send_to") | ||
| subject = request.GET.get("subject") | ||
| message = request.GET.get("message") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's some nginx documentation on the size limits of http request headers: |
||
| text_message = html_to_text(message) | ||
| if not has_instructor_access: | ||
| return HttpResponseForbidden("Operation requires instructor access.") | ||
| email = CourseEmail( | ||
| course_id = course_id, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't put spaces around this equal sign |
||
| sender=request.user, | ||
| to_option=send_to, | ||
| subject=subject, | ||
| html_message=message, | ||
| text_message=text_message | ||
| ) | ||
| email.save() | ||
| tasks.delegate_email_batches.delay( | ||
| email.id, | ||
| request.user.id | ||
| ) | ||
| response_payload = { | ||
| 'course_id': course_id, | ||
| } | ||
| return JsonResponse(response_payload) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a short function, but there's a lot to test here. I'd start by thinking about possible paths:
In addition, there are some unhappy paths to test:
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. |
||
|
|
||
| @ensure_csrf_cookie | ||
| @cache_control(no_cache=True, no_store=True, must_revalidate=True) | ||
| @require_level('staff') | ||
|
|
@@ -728,7 +770,6 @@ def update_forum_role_membership(request, course_id): | |
| } | ||
| return JsonResponse(response_payload) | ||
|
|
||
|
|
||
| @ensure_csrf_cookie | ||
| @cache_control(no_cache=True, no_store=True, must_revalidate=True) | ||
| @require_level('staff') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,21 +9,27 @@ | |
| from django.core.urlresolvers import reverse | ||
| from django.utils.html import escape | ||
| from django.http import Http404 | ||
| from django.conf import settings | ||
|
|
||
| from xmodule_modifiers import wrap_xmodule | ||
| from xmodule.html_module import HtmlDescriptor | ||
| from xmodule.modulestore import MONGO_MODULESTORE_TYPE | ||
| from xmodule.modulestore.django import modulestore | ||
| from xblock.field_data import DictFieldData | ||
| from xblock.fields import ScopeIds | ||
| from courseware.access import has_access | ||
| from courseware.courses import get_course_by_id | ||
| from django_comment_client.utils import has_forum_access | ||
| from django_comment_common.models import FORUM_ROLE_ADMINISTRATOR | ||
| from xmodule.modulestore.django import modulestore | ||
| from student.models import CourseEnrollment | ||
|
|
||
|
|
||
| @ensure_csrf_cookie | ||
| @cache_control(no_cache=True, no_store=True, must_revalidate=True) | ||
| def instructor_dashboard_2(request, course_id): | ||
| """ Display the instructor dashboard for a course. """ | ||
|
|
||
| course = get_course_by_id(course_id, depth=None) | ||
| is_studio_course = modulestore().get_modulestore_type(course_id) == MONGO_MODULESTORE_TYPE | ||
|
|
||
| access = { | ||
| 'admin': request.user.is_staff, | ||
|
|
@@ -42,9 +48,12 @@ def instructor_dashboard_2(request, course_id): | |
| _section_membership(course_id, access), | ||
| _section_student_admin(course_id, access), | ||
| _section_data_download(course_id), | ||
| _section_analytics(course_id), | ||
| _section_analytics(course_id) | ||
| ] | ||
|
|
||
| if settings.MITX_FEATURES['ENABLE_INSTRUCTOR_EMAIL'] and is_studio_course: | ||
| sections.append(_section_send_email(course_id,access,course)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pep 8 here. |
||
|
|
||
| context = { | ||
| 'course': course, | ||
| 'old_dashboard_url': reverse('instructor_dashboard', kwargs={'course_id': course_id}), | ||
|
|
@@ -136,6 +145,18 @@ def _section_data_download(course_id): | |
| } | ||
| return section_data | ||
|
|
||
| def _section_send_email(course_id, access, course): | ||
| """ Provide data for the corresponding bulk email section """ | ||
| html_module = HtmlDescriptor(course.system, DictFieldData({'data': ''}), ScopeIds(None, None, None, None)) | ||
| section_data = { | ||
| 'section_key': 'send_email', | ||
| 'section_display_name': _('Email'), | ||
| 'access': access, | ||
| 'send_email': reverse('send_email',kwargs={'course_id': course_id}), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. put a space after the comma |
||
| 'editor': wrap_xmodule(html_module.get_html, html_module, 'xmodule_edit.html')() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should look at what @cpennington did with the editor in https://github.com/edx/edx-platform/pull/945/files (legacy.py) and copy that logic instead.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well... Except that that logic only works if you've got the changes from my branch. Sadly, whether you need to change this or not depends on whether you merge first or whether #945 does. |
||
| } | ||
| return section_data | ||
|
|
||
|
|
||
| def _section_analytics(course_id): | ||
| """ Provide data for the corresponding dashboard section """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| ### | ||
| Email Section | ||
|
|
||
| imports from other modules. | ||
| wrap in (-> ... apply) to defer evaluation | ||
| such that the value can be defined later than this assignment (file load order). | ||
| ### | ||
|
|
||
| plantTimeout = -> window.InstructorDashboard.util.plantTimeout.apply this, arguments | ||
| std_ajax_err = -> window.InstructorDashboard.util.std_ajax_err.apply this, arguments | ||
|
|
||
| class SendEmail | ||
| constructor: (@$container) -> | ||
| # gather elements | ||
| @$emailEditor = XModule.loadModule($('.xmodule_edit')); | ||
| @$send_to = @$container.find("select[name='send_to']'") | ||
| @$subject = @$container.find("input[name='subject']'") | ||
| @$btn_send = @$container.find("input[name='send']'") | ||
| @$task_response = @$container.find(".request-response") | ||
| @$request_response_error = @$container.find(".request-response-error") | ||
|
|
||
| # attach click handlers | ||
|
|
||
| @$btn_send.click => | ||
|
|
||
| send_data = | ||
| action: 'send' | ||
| send_to: @$send_to.val() | ||
| subject: @$subject.val() | ||
| message: @$emailEditor.save()['data'] | ||
|
|
||
| $.ajax | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could make it a POST here. |
||
| dataType: 'json' | ||
| url: @$btn_send.data 'endpoint' | ||
| data: send_data | ||
| success: (data) => @display_response gettext('Your email was successfully queued for sending.') | ||
| error: std_ajax_err => @fail_with_error gettext('Error sending email.') | ||
|
|
||
| fail_with_error: (msg) -> | ||
| console.warn msg | ||
| @$task_response.empty() | ||
| @$request_response_error.empty() | ||
| @$request_response_error.text gettext(msg) | ||
|
|
||
| display_response: (data_from_server) -> | ||
| @$task_response.empty() | ||
| @$request_response_error.empty() | ||
| @$task_response.text(gettext('Your email was successfully queued for sending.')) | ||
|
|
||
|
|
||
| # Email Section | ||
| class Email | ||
| # enable subsections. | ||
| constructor: (@$section) -> | ||
| # attach self to html | ||
| # so that instructor_dashboard.coffee can find this object | ||
| # to call event handlers like 'onClickTitle' | ||
| @$section.data 'wrapper', @ | ||
|
|
||
| # isolate # initialize SendEmail subsection | ||
| plantTimeout 0, => new SendEmail @$section.find '.send-email' | ||
|
|
||
| # handler for when the section title is clicked. | ||
| onClickTitle: -> | ||
|
|
||
|
|
||
| # export for use | ||
| # create parent namespaces if they do not already exist. | ||
| # abort if underscore can not be found. | ||
| if _? | ||
| _.defaults window, InstructorDashboard: {} | ||
| _.defaults window.InstructorDashboard, sections: {} | ||
| _.defaults window.InstructorDashboard.sections, | ||
| Email: Email | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameters