From b86e912905679c8188b00a4efcde52b34c3ee926 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Tue, 29 Oct 2013 16:56:49 -0400 Subject: [PATCH] Make event handlers fire properly Respond to review comments LMS-1242 --- lms/djangoapps/instructor/tests/test_api.py | 33 ++++++++++--------- lms/djangoapps/instructor/views/api.py | 14 +++++--- .../instructor/views/instructor_dashboard.py | 2 +- .../src/instructor_dashboard/analytics.coffee | 10 +++--- .../instructor_dashboard/course_info.coffee | 24 +++++++------- .../instructor_dashboard/data_download.coffee | 23 +++++++------ .../instructor_dashboard.coffee | 2 +- .../instructor_dashboard/membership.coffee | 10 +++--- .../instructor_dashboard/send_email.coffee | 19 +++++------ .../instructor_dashboard/student_admin.coffee | 19 +++++------ .../src/instructor_dashboard/util.coffee | 2 +- .../instructor_dashboard_2/course_info.html | 6 +--- 12 files changed, 78 insertions(+), 86 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index ae8d02d63f..06d0f16ab8 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -770,8 +770,8 @@ class MockCompletionInfo(object): """Mock for get_task_completion_info""" self.times_called += 1 if self.times_called % 2 == 0: - return (True, 'Task Completed') - return (False, 'Task Errored In Some Way') + return True, 'Task Completed' + return False, 'Task Errored In Some Way' @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -791,34 +791,36 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase): 'created', 'status', 'task_message', - 'duration_sec', - 'task_output' + 'duration_sec' ] def __init__(self, completion): for feature in self.FEATURES: setattr(self, feature, 'expected') - # Make 'created' into a datetime - setattr(self, 'created', datetime.datetime(2013, 10, 25, 11, 42, 35)) + # created needs to be a datetime + self.created = datetime.datetime(2013, 10, 25, 11, 42, 35) # set 'status' and 'task_message' attrs success, task_message = completion() if success: - setattr(self, 'status', "Complete") + self.status = "Complete" else: - setattr(self, 'status', "Incomplete") - setattr(self, 'task_message', task_message) + self.status = "Incomplete" + self.task_message = task_message # Set 'task_output' attr, which will be parsed to the 'duration_sec' attr. - setattr(self, 'task_output', '{"duration_ms": 1035000}') - setattr(self, 'duration_sec', 1035000 / 1000.0) + self.task_output = '{"duration_ms": 1035000}' + self.duration_sec = 1035000 / 1000.0 + def make_invalid_output(self): + """Munge task_output to be invalid json""" + self.task_output = 'HI MY NAME IS INVALID JSON' + # This should be given the value of 'unknown' if the task output + # can't be properly parsed + self.duration_sec = 'unknown' def to_dict(self): """ Convert fake task to dictionary representation. """ attr_dict = {key: getattr(self, key) for key in self.FEATURES} attr_dict['created'] = attr_dict['created'].isoformat() - # Don't actually want task_output in the attribute dictionary, as this - # is not explicitly extracted in extract_task_features - del attr_dict['task_output'] return attr_dict def setUp(self): @@ -840,7 +842,8 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase): state=json.dumps({'attempts': 10}), ) mock_factory = MockCompletionInfo() - self.tasks = [self.FakeTask(mock_factory.mock_get_task_completion_info) for _ in xrange(6)] + self.tasks = [self.FakeTask(mock_factory.mock_get_task_completion_info) for _ in xrange(7)] + self.tasks[-1].make_invalid_output() def tearDown(self): """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d3ab72f098..8ab1aa8a76 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -690,21 +690,25 @@ def list_instructor_tasks(request, course_id): """ # Pull out information from the task features = ['task_type', 'task_input', 'task_id', 'requester', 'task_state'] - task_feature_dict = dict((feature, str(getattr(task, feature))) for feature in features) + task_feature_dict = {feature: str(getattr(task, feature)) for feature in features} # Some information (created, duration, status, task message) require additional formatting task_feature_dict['created'] = task.created.isoformat() # Get duration info, if known duration_sec = 'unknown' if hasattr(task, 'task_output') and task.task_output is not None: - task_output = json.loads(task.task_output) - if 'duration_ms' in task_output: - duration_sec = int(task_output['duration_ms'] / 1000.0) + try: + task_output = json.loads(task.task_output) + except ValueError: + log.error("Could not parse task output as valid json; task output: %s", task.task_output) + else: + if 'duration_ms' in task_output: + duration_sec = int(task_output['duration_ms'] / 1000.0) task_feature_dict['duration_sec'] = duration_sec # Get progress status message & success information success, task_message = get_task_completion_info(task) - status = "Complete" if success else "Incomplete" + status = _("Complete") if success else _("Incomplete") task_feature_dict['status'] = status task_feature_dict['task_message'] = task_message diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 27ee39e042..8b5c44ebf0 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -95,7 +95,7 @@ def _section_course_info(course_id): """ Provide data for the corresponding dashboard section """ course = get_course_by_id(course_id, depth=None) - (course_org, course_num, course_name) = course_id.split('/') + course_org, course_num, course_name = course_id.split('/') section_data = { 'section_key': 'course_info', diff --git a/lms/static/coffee/src/instructor_dashboard/analytics.coffee b/lms/static/coffee/src/instructor_dashboard/analytics.coffee index 018b7e9c57..9955f8ee11 100644 --- a/lms/static/coffee/src/instructor_dashboard/analytics.coffee +++ b/lms/static/coffee/src/instructor_dashboard/analytics.coffee @@ -230,9 +230,7 @@ class Analytics # 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, - Analytics: Analytics +_.defaults window, InstructorDashboard: {} +_.defaults window.InstructorDashboard, sections: {} +_.defaults window.InstructorDashboard.sections, + Analytics: Analytics diff --git a/lms/static/coffee/src/instructor_dashboard/course_info.coffee b/lms/static/coffee/src/instructor_dashboard/course_info.coffee index 21e719566e..c481a33bb5 100644 --- a/lms/static/coffee/src/instructor_dashboard/course_info.coffee +++ b/lms/static/coffee/src/instructor_dashboard/course_info.coffee @@ -7,8 +7,6 @@ such that the value can be defined later than this assignment (file load order). ### # Load utilities -plantTimeout = -> window.InstructorDashboard.util.plantTimeout.apply this, arguments -std_ajax_err = -> window.InstructorDashboard.util.std_ajax_err.apply this, arguments PendingInstructorTasks = -> window.InstructorDashboard.util.PendingInstructorTasks # A typical section object. @@ -16,6 +14,12 @@ PendingInstructorTasks = -> window.InstructorDashboard.util.PendingInstructorTas # which holds the section body container. class CourseInfo constructor: (@$section) -> + # attach self to html so that instructor_dashboard.coffee can find + # this object to call event handlers like 'onClickTitle' + @$section.data 'wrapper', @ + + # gather elements + @instructor_tasks = new (PendingInstructorTasks()) @$section @$course_errors_wrapper = @$section.find '.course-errors-wrapper' # if there are errors @@ -37,19 +41,15 @@ class CourseInfo else @$course_errors_wrapper.addClass 'open' - @instructor_tasks = new (PendingInstructorTasks()) @$section - # handler for when the section title is clicked. - onClickTitle: -> @instructor_tasks.task_poller?.start() + onClickTitle: -> @instructor_tasks.task_poller.start() # handler for when the section is closed - onExit: -> @instructor_tasks.task_poller?.stop() + onExit: -> @instructor_tasks.task_poller.stop() # 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, - CourseInfo: CourseInfo +_.defaults window, InstructorDashboard: {} +_.defaults window.InstructorDashboard, sections: {} +_.defaults window.InstructorDashboard.sections, + CourseInfo: CourseInfo diff --git a/lms/static/coffee/src/instructor_dashboard/data_download.coffee b/lms/static/coffee/src/instructor_dashboard/data_download.coffee index 040a4c1f13..e6108b1055 100644 --- a/lms/static/coffee/src/instructor_dashboard/data_download.coffee +++ b/lms/static/coffee/src/instructor_dashboard/data_download.coffee @@ -6,13 +6,16 @@ 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 +# Load utilities std_ajax_err = -> window.InstructorDashboard.util.std_ajax_err.apply this, arguments PendingInstructorTasks = -> window.InstructorDashboard.util.PendingInstructorTasks # Data Download Section class DataDownload constructor: (@$section) -> + # attach self to html so that instructor_dashboard.coffee can find + # this object to call event handlers like 'onClickTitle' + @$section.data 'wrapper', @ # gather elements @$display = @$section.find '.data-display' @$display_text = @$display.find '.data-display-text' @@ -21,9 +24,9 @@ class DataDownload @$list_studs_btn = @$section.find("input[name='list-profiles']'") @$list_anon_btn = @$section.find("input[name='list-anon-ids']'") @$grade_config_btn = @$section.find("input[name='dump-gradeconf']'") + @instructor_tasks = new (PendingInstructorTasks()) @$section # attach click handlers - # The list-anon case is always CSV @$list_anon_btn.click (e) => url = @$list_anon_btn.data 'endpoint' @@ -80,13 +83,11 @@ class DataDownload @clear_display() @$display_text.html data['grading_config_summary'] - @instructor_tasks = new (PendingInstructorTasks()) @$section - # handler for when the section title is clicked. - onClickTitle: -> @instructor_tasks.task_poller?.start() + onClickTitle: -> @instructor_tasks.task_poller.start() # handler for when the section is closed - onExit: -> @instructor_tasks.task_poller?.stop() + onExit: -> @instructor_tasks.task_poller.stop() clear_display: -> @$display_text.empty() @@ -96,9 +97,7 @@ class DataDownload # 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, - DataDownload: DataDownload +_.defaults window, InstructorDashboard: {} +_.defaults window.InstructorDashboard, sections: {} +_.defaults window.InstructorDashboard.sections, + DataDownload: DataDownload diff --git a/lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee b/lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee index c645fcf67e..313e5b4967 100644 --- a/lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee +++ b/lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee @@ -118,7 +118,7 @@ setup_instructor_dashboard = (idash_content) => location.hash = "#{HASH_LINK_PREFIX}#{section_name}" sections_have_loaded.after -> - $section.data('wrapper')?.onClickTitle?() + $section.data('wrapper').onClickTitle() # call onExit handler if exiting a section to a different section. unless $section.is $active_section diff --git a/lms/static/coffee/src/instructor_dashboard/membership.coffee b/lms/static/coffee/src/instructor_dashboard/membership.coffee index 54b04be5db..03c6b705b6 100644 --- a/lms/static/coffee/src/instructor_dashboard/membership.coffee +++ b/lms/static/coffee/src/instructor_dashboard/membership.coffee @@ -487,9 +487,7 @@ class Membership # 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, - Membership: Membership +_.defaults window, InstructorDashboard: {} +_.defaults window.InstructorDashboard, sections: {} +_.defaults window.InstructorDashboard.sections, + Membership: Membership diff --git a/lms/static/coffee/src/instructor_dashboard/send_email.coffee b/lms/static/coffee/src/instructor_dashboard/send_email.coffee index 8095804987..7bdb37c0e9 100644 --- a/lms/static/coffee/src/instructor_dashboard/send_email.coffee +++ b/lms/static/coffee/src/instructor_dashboard/send_email.coffee @@ -81,9 +81,8 @@ class SendEmail 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' + # 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 @@ -92,17 +91,15 @@ class Email @instructor_tasks = new (PendingInstructorTasks()) @$section # handler for when the section title is clicked. - onClickTitle: -> @instructor_tasks.task_poller?.start() + onClickTitle: -> @instructor_tasks.task_poller.start() # handler for when the section is closed - onExit: -> @instructor_tasks.task_poller?.stop() + onExit: -> @instructor_tasks.task_poller.stop() # 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 +_.defaults window, InstructorDashboard: {} +_.defaults window.InstructorDashboard, sections: {} +_.defaults window.InstructorDashboard.sections, + Email: Email diff --git a/lms/static/coffee/src/instructor_dashboard/student_admin.coffee b/lms/static/coffee/src/instructor_dashboard/student_admin.coffee index 46c041048c..d930dd4b13 100644 --- a/lms/static/coffee/src/instructor_dashboard/student_admin.coffee +++ b/lms/static/coffee/src/instructor_dashboard/student_admin.coffee @@ -7,10 +7,7 @@ such that the value can be defined later than this assignment (file load order). ### # Load utilities -plantTimeout = -> window.InstructorDashboard.util.plantTimeout.apply this, arguments -plantInterval = -> window.InstructorDashboard.util.plantInterval.apply this, arguments std_ajax_err = -> window.InstructorDashboard.util.std_ajax_err.apply this, arguments -load_IntervalManager = -> window.InstructorDashboard.util.IntervalManager create_task_list_table = -> window.InstructorDashboard.util.create_task_list_table.apply this, arguments PendingInstructorTasks = -> window.InstructorDashboard.util.PendingInstructorTasks @@ -27,6 +24,8 @@ find_and_assert = ($root, selector) -> class StudentAdmin constructor: (@$section) -> + # attach self to html so that instructor_dashboard.coffee can find + # this object to call event handlers like 'onClickTitle' @$section.data 'wrapper', @ # gather buttons @@ -255,17 +254,15 @@ class StudentAdmin @$request_response_error_all.empty() # handler for when the section title is clicked. - onClickTitle: -> @instructor_tasks.task_poller?.start() + onClickTitle: -> @instructor_tasks.task_poller.start() # handler for when the section is closed - onExit: -> @instructor_tasks.task_poller?.stop() + onExit: -> @instructor_tasks.task_poller.stop() # 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, - StudentAdmin: StudentAdmin +_.defaults window, InstructorDashboard: {} +_.defaults window.InstructorDashboard, sections: {} +_.defaults window.InstructorDashboard.sections, + StudentAdmin: StudentAdmin diff --git a/lms/static/coffee/src/instructor_dashboard/util.coffee b/lms/static/coffee/src/instructor_dashboard/util.coffee index ccce17eb5c..09d2ae26f3 100644 --- a/lms/static/coffee/src/instructor_dashboard/util.coffee +++ b/lms/static/coffee/src/instructor_dashboard/util.coffee @@ -101,8 +101,8 @@ class IntervalManager @intervalID = null # Start or restart firing every `ms` milliseconds. - # Soes not fire immediately. start: -> + @fn() if @intervalID is null @intervalID = setInterval @fn, @ms diff --git a/lms/templates/instructor/instructor_dashboard_2/course_info.html b/lms/templates/instructor/instructor_dashboard_2/course_info.html index c97ab0af32..7362014b09 100644 --- a/lms/templates/instructor/instructor_dashboard_2/course_info.html +++ b/lms/templates/instructor/instructor_dashboard_2/course_info.html @@ -37,11 +37,7 @@
  • - %if section_data['has_started']: - ${_("Yes")} - %else: - ${_("No")} - %endif + ${_("Yes") if section_data['grade_cutoffs'] else _("No")}