diff --git a/common/lib/xmodule/xmodule/capa_base.py b/common/lib/xmodule/xmodule/capa_base.py index c23071aff3..c2d7c402d5 100644 --- a/common/lib/xmodule/xmodule/capa_base.py +++ b/common/lib/xmodule/xmodule/capa_base.py @@ -1048,7 +1048,7 @@ class CapaMixin(CapaFields): return answers - def publish_grade(self): + def publish_grade(self, only_if_higher=None): """ Publishes the student's current grade to the system as an event """ @@ -1059,6 +1059,7 @@ class CapaMixin(CapaFields): { 'value': score['score'], 'max_value': score['total'], + 'only_if_higher': only_if_higher, } ) @@ -1370,13 +1371,16 @@ class CapaMixin(CapaFields): return input_metadata - def rescore_problem(self): + def rescore_problem(self, only_if_higher): """ Checks whether the existing answers to a problem are correct. This is called when the correct answer to a problem has been changed, and the grade should be re-evaluated. + If only_if_higher is True, the answer and grade are updated + only if the resulting score is higher than before. + Returns a dict with one key: {'success' : 'correct' | 'incorrect' | AJAX alert msg string } @@ -1428,7 +1432,7 @@ class CapaMixin(CapaFields): # need to increment here, or mark done. Just save. self.set_state_from_lcp() - self.publish_grade() + self.publish_grade(only_if_higher) new_score = self.lcp.get_score() event_info['new_score'] = new_score['score'] diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 5444e5298f..e4ca3f3c7a 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -487,15 +487,14 @@ class CapaModuleTest(unittest.TestCase): # Simulate that all answers are marked correct, no matter # what the input is, by patching CorrectMap.is_correct() # Also simulate rendering the HTML - # TODO: pep8 thinks the following line has invalid syntax - with patch('capa.correctmap.CorrectMap.is_correct') as mock_is_correct, \ - patch('xmodule.capa_module.CapaModule.get_problem_html') as mock_html: - mock_is_correct.return_value = True - mock_html.return_value = "Test HTML" + with patch('capa.correctmap.CorrectMap.is_correct') as mock_is_correct: + with patch('xmodule.capa_module.CapaModule.get_problem_html') as mock_html: + mock_is_correct.return_value = True + mock_html.return_value = "Test HTML" - # Check the problem - get_request_dict = {CapaFactory.input_key(): '3.14'} - result = module.submit_problem(get_request_dict) + # Check the problem + get_request_dict = {CapaFactory.input_key(): '3.14'} + result = module.submit_problem(get_request_dict) # Expect that the problem is marked correct self.assertEqual(result['success'], 'correct') @@ -861,7 +860,7 @@ class CapaModuleTest(unittest.TestCase): # what the input is, by patching LoncapaResponse.evaluate_answers() with patch('capa.responsetypes.LoncapaResponse.evaluate_answers') as mock_evaluate_answers: mock_evaluate_answers.return_value = CorrectMap(CapaFactory.answer_key(), 'correct') - result = module.rescore_problem() + result = module.rescore_problem(only_if_higher=False) # Expect that the problem is marked correct self.assertEqual(result['success'], 'correct') @@ -881,7 +880,7 @@ class CapaModuleTest(unittest.TestCase): # what the input is, by patching LoncapaResponse.evaluate_answers() with patch('capa.responsetypes.LoncapaResponse.evaluate_answers') as mock_evaluate_answers: mock_evaluate_answers.return_value = CorrectMap(CapaFactory.answer_key(), 'incorrect') - result = module.rescore_problem() + result = module.rescore_problem(only_if_higher=False) # Expect that the problem is marked incorrect self.assertEqual(result['success'], 'incorrect') @@ -895,7 +894,7 @@ class CapaModuleTest(unittest.TestCase): # Try to rescore the problem, and get exception with self.assertRaises(xmodule.exceptions.NotFoundError): - module.rescore_problem() + module.rescore_problem(only_if_higher=False) def test_rescore_problem_not_supported(self): module = CapaFactory.create(done=True) @@ -904,7 +903,7 @@ class CapaModuleTest(unittest.TestCase): with patch('capa.capa_problem.LoncapaProblem.supports_rescoring') as mock_supports_rescoring: mock_supports_rescoring.return_value = False with self.assertRaises(NotImplementedError): - module.rescore_problem() + module.rescore_problem(only_if_higher=False) def _rescore_problem_error_helper(self, exception_class): """Helper to allow testing all errors that rescoring might return.""" @@ -914,7 +913,7 @@ class CapaModuleTest(unittest.TestCase): # Simulate answering a problem that raises the exception with patch('capa.capa_problem.LoncapaProblem.rescore_existing_answers') as mock_rescore: mock_rescore.side_effect = exception_class(u'test error \u03a9') - result = module.rescore_problem() + result = module.rescore_problem(only_if_higher=False) # Expect an AJAX alert message in 'success' expected_msg = u'Error: test error \u03a9' @@ -1656,7 +1655,7 @@ class CapaModuleTest(unittest.TestCase): module.submit_problem(get_request_dict) # On rescore, state/student_answers should use unmasked names with patch.object(module.runtime, 'track_function') as mock_track_function: - module.rescore_problem() + module.rescore_problem(only_if_higher=False) mock_call = mock_track_function.mock_calls[0] event_info = mock_call[1][1] self.assertEquals(mock_call[1][0], 'problem_rescore') diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index 61966156b4..419723b06a 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -48,12 +48,14 @@ class InstructorDashboardPage(CoursePage): data_download_section.wait_for_page() return data_download_section - def select_student_admin(self): + def select_student_admin(self, admin_class): """ - Selects the student admin tab and returns the MembershipSection + Selects the student admin tab and returns the requested + admin section. + admin_class should be a subclass of StudentAdminPage. """ self.q(css='[data-section="student_admin"]').first.click() - student_admin_section = StudentAdminPage(self.browser) + student_admin_section = admin_class(self.browser) student_admin_section.wait_for_page() return student_admin_section @@ -1025,8 +1027,18 @@ class StudentAdminPage(PageObject): Student admin section of the Instructor dashboard. """ url = None - EE_CONTAINER = ".entrance-exam-grade-container" - CS_CONTAINER = ".course-specific-container" + CONTAINER = None + + PROBLEM_INPUT_NAME = None + STUDENT_EMAIL_INPUT_NAME = None + + RESET_ATTEMPTS_BUTTON_NAME = None + RESCORE_BUTTON_NAME = None + RESCORE_IF_HIGHER_BUTTON_NAME = None + DELETE_STATE_BUTTON_NAME = None + + BACKGROUND_TASKS_BUTTON_NAME = None + TASK_HISTORY_TABLE_NAME = None def is_browser_on_page(self): """ @@ -1034,167 +1046,185 @@ class StudentAdminPage(PageObject): """ return self.q(css='[data-section=student_admin].active-section').present + def _input_with_name(self, input_name): + """ + Returns the input box with the given name + for this object's container. + """ + return self.q(css='{} input[name={}]'.format(self.CONTAINER, input_name)) + @property - def student_email_input(self): + def problem_location_input(self): + """ + Returns input box for problem location + """ + return self._input_with_name(self.PROBLEM_INPUT_NAME) + + def set_problem_location(self, problem_location): + """ + Returns input box for problem location + """ + input_box = self.problem_location_input.first.results[0] + input_box.send_keys(unicode(problem_location)) + + @property + def student_email_or_username_input(self): """ Returns email address/username input box. """ - return self.q(css='{} input[name=entrance-exam-student-select-grade]'.format(self.EE_CONTAINER)) + return self._input_with_name(self.STUDENT_EMAIL_INPUT_NAME) - @property - def rescore_problem_input(self): + def set_student_email_or_username(self, email_or_username): """ - Returns input box for rescore/reset all on a problem + Sets given email or username as value of + student email/username input box. """ - return self.q(css='{} input[name=problem-select-all]'.format(self.CS_CONTAINER)) + input_box = self.student_email_or_username_input.first.results[0] + input_box.send_keys(email_or_username) @property def reset_attempts_button(self): """ Returns reset student attempts button. """ - return self.q(css='{} input[name=reset-entrance-exam-attempts]'.format(self.EE_CONTAINER)) + return self._input_with_name(self.RESET_ATTEMPTS_BUTTON_NAME) @property - def rescore_submission_button(self): + def rescore_button(self): """ - Returns rescore student submission button. + Returns rescore button. """ - return self.q(css='{} input[name=rescore-entrance-exam]'.format(self.EE_CONTAINER)) + return self._input_with_name(self.RESCORE_BUTTON_NAME) @property - def rescore_all_submissions_button(self): + def rescore_if_higher_button(self): """ - Returns rescore student submission button. + Returns rescore if higher button. """ - return self.q(css='{} input[name=rescore-problem-all]'.format(self.CS_CONTAINER)) + return self._input_with_name(self.RESCORE_IF_HIGHER_BUTTON_NAME) @property - def show_background_tasks_button(self): + def delete_state_button(self): """ - Return Show Background Tasks button. + Returns delete state button. """ - return self.q(css='{} input[name=task-history-all]'.format(self.CS_CONTAINER)) + return self._input_with_name(self.DELETE_STATE_BUTTON_NAME) + + @property + def task_history_button(self): + """ + Return Background Tasks History button. + """ + return self._input_with_name(self.BACKGROUND_TASKS_BUTTON_NAME) + + def wait_for_task_history_table(self): + """ + Waits until the task history table is visible. + """ + def check_func(): + """ + Promise Check Function + """ + query = self.q(css="{} .{}".format(self.CONTAINER, self.TASK_HISTORY_TABLE_NAME)) + return query.visible, query + + return Promise(check_func, "Waiting for student admin task history table to be visible.").fulfill() + + def wait_for_task_completion(self, expected_task_string): + """ + Waits until the task history table is visible. + """ + def check_func(): + """ + Promise Check Function + """ + self.task_history_button.click() + table = self.wait_for_task_history_table() + return len(table) > 0 and expected_task_string in table.results[0].text + + return EmptyPromise(check_func, "Waiting for student admin task to complete.").fulfill() + + +class StudentSpecificAdmin(StudentAdminPage): + """ + Student specific section of the Student Admin page. + """ + CONTAINER = ".student-grade-container" + + PROBLEM_INPUT_NAME = "problem-select-single" + STUDENT_EMAIL_INPUT_NAME = "student-select-grade" + + RESET_ATTEMPTS_BUTTON_NAME = "reset-attempts-single" + RESCORE_BUTTON_NAME = "rescore-problem-single" + RESCORE_IF_HIGHER_BUTTON_NAME = "rescore-problem-if-higher-single" + DELETE_STATE_BUTTON_NAME = "delete-state-single" + + BACKGROUND_TASKS_BUTTON_NAME = "task-history-single" + TASK_HISTORY_TABLE_NAME = "task-history-single-table" + + +class CourseSpecificAdmin(StudentAdminPage): + """ + Course specific section of the Student Admin page. + """ + CONTAINER = ".course-specific-container" + + PROBLEM_INPUT_NAME = "problem-select-all" + STUDENT_EMAIL_INPUT_NAME = None + + RESET_ATTEMPTS_BUTTON_NAME = "reset-attempts-all" + RESCORE_BUTTON_NAME = "rescore-problem-all" + RESCORE_IF_HIGHER_BUTTON_NAME = "rescore-problem-all-if-higher" + DELETE_STATE_BUTTON_NAME = None + + BACKGROUND_TASKS_BUTTON_NAME = "task-history-all" + TASK_HISTORY_TABLE_NAME = "task-history-all-table" + + +class EntranceExamAdmin(StudentAdminPage): + """ + Entrance exam section of the Student Admin page. + """ + CONTAINER = ".entrance-exam-grade-container" + + STUDENT_EMAIL_INPUT_NAME = "entrance-exam-student-select-grade" + PROBLEM_INPUT_NAME = None + + RESET_ATTEMPTS_BUTTON_NAME = "reset-entrance-exam-attempts" + RESCORE_BUTTON_NAME = "rescore-entrance-exam" + RESCORE_IF_HIGHER_BUTTON_NAME = "rescore-entrance-exam-if-higher" + DELETE_STATE_BUTTON_NAME = "delete-entrance-exam-state" + + BACKGROUND_TASKS_BUTTON_NAME = "entrance-exam-task-history" + TASK_HISTORY_TABLE_NAME = "entrance-exam-task-history-table" @property def skip_entrance_exam_button(self): """ Return Let Student Skip Entrance Exam button. """ - return self.q(css='{} input[name=skip-entrance-exam]'.format(self.EE_CONTAINER)) - - @property - def delete_student_state_button(self): - """ - Returns delete student state button. - """ - return self.q(css='{} input[name=delete-entrance-exam-state]'.format(self.EE_CONTAINER)) - - @property - def background_task_history_button(self): - """ - Returns show background task history for student button. - """ - return self.q(css='{} input[name=entrance-exam-task-history]'.format(self.EE_CONTAINER)) + return self.q(css='{} input[name=skip-entrance-exam]'.format(self.CONTAINER)) @property def top_notification(self): """ Returns show background task history for student button. """ - return self.q(css='{} .request-response-error'.format(self.EE_CONTAINER)).first + return self.q(css='{} .request-response-error'.format(self.CONTAINER)).first - def is_student_email_input_visible(self): + def are_all_buttons_visible(self): """ - Returns True if student email address/username input box is present. + Returns whether all buttons related to entrance exams + are visible. """ - return self.student_email_input.is_present() - - def is_reset_attempts_button_visible(self): - """ - Returns True if reset student attempts button is present. - """ - return self.reset_attempts_button.is_present() - - def is_rescore_submission_button_visible(self): - """ - Returns True if rescore student submission button is present. - """ - return self.rescore_submission_button.is_present() - - def is_delete_student_state_button_visible(self): - """ - Returns True if delete student state for entrance exam button is present. - """ - return self.delete_student_state_button.is_present() - - def is_background_task_history_button_visible(self): - """ - Returns True if show background task history for student button is present. - """ - return self.background_task_history_button.is_present() - - def is_background_task_history_table_visible(self): - """ - Returns True if background task history table is present. - """ - return self.q(css='{} .entrance-exam-task-history-table'.format(self.EE_CONTAINER)).is_present() - - def click_reset_attempts_button(self): - """ - clicks reset student attempts button. - """ - return self.reset_attempts_button.click() - - def click_rescore_submissions_button(self): - """ - clicks rescore submissions button. - """ - return self.rescore_submission_button.click() - - def click_rescore_all_button(self): - """ - clicks rescore all for problem button. - """ - return self.rescore_all_submissions_button.click() - - def click_show_background_tasks_button(self): - """ - clicks show background tasks button. - """ - return self.show_background_tasks_button.click() - - def click_skip_entrance_exam_button(self): - """ - clicks let student skip entrance exam button. - """ - return self.skip_entrance_exam_button.click() - - def click_delete_student_state_button(self): - """ - clicks delete student state button. - """ - return self.delete_student_state_button.click() - - def click_task_history_button(self): - """ - clicks background task history button. - """ - return self.background_task_history_button.click() - - def set_student_email(self, email_address): - """ - Sets given email address as value of student email address/username input box. - """ - input_box = self.student_email_input.first.results[0] - input_box.send_keys(email_address) - - def set_problem_to_rescore(self, problem_locator): - """ - Sets the problem for which to rescore/reset all scores. - """ - input_box = self.rescore_problem_input.first.results[0] - input_box.send_keys(problem_locator) + return ( + self.student_email_or_username_input.is_present() and + self.reset_attempts_button.is_present() and + self.rescore_button.is_present() and + self.rescore_if_higher_button.is_present() and + self.delete_state_button.is_present() and + self.task_history_button.is_present() + ) class CertificatesPage(PageObject): diff --git a/common/test/acceptance/pages/lms/staff_view.py b/common/test/acceptance/pages/lms/staff_view.py index 8c227faecd..507b3893d9 100644 --- a/common/test/acceptance/pages/lms/staff_view.py +++ b/common/test/acceptance/pages/lms/staff_view.py @@ -107,6 +107,15 @@ class StaffDebugPage(PageObject): self.q(css='input[id^=sd_fu_]').first.fill(user) self.q(css='.staff-modal .staff-debug-rescore').click() + def rescore_if_higher(self, user=None): + """ + This clicks on the reset attempts link with an optionally + specified user. + """ + if user: + self.q(css='input[id^=sd_fu_]').first.fill(user) + self.q(css='.staff-modal .staff-debug-rescore-if-higher').click() + @property def idash_msg(self): """ diff --git a/common/test/acceptance/pages/studio/container.py b/common/test/acceptance/pages/studio/container.py index 403e823135..fc71890d55 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -169,6 +169,13 @@ class ContainerPage(PageObject, HelpMixin): """ return self.q(css='.action-publish').first + def publish(self): + """ + Publishes the container. + """ + self.publish_action.click() + self.wait_for_ajax() + def discard_changes(self): """ Discards draft changes (which will then re-render the page). diff --git a/common/test/acceptance/tests/helpers.py b/common/test/acceptance/tests/helpers.py index a9443bd10d..37e01e62ac 100644 --- a/common/test/acceptance/tests/helpers.py +++ b/common/test/acceptance/tests/helpers.py @@ -353,17 +353,29 @@ def is_404_page(browser): return 'Page not found (404)' in browser.find_element_by_tag_name('h1').text +def create_multiple_choice_xml(correct_choice=2, num_choices=4): + """ + Return the Multiple Choice Problem XML, given the name of the problem. + """ + # all choices are incorrect except for correct_choice + choices = [False for _ in range(num_choices)] + choices[correct_choice] = True + + choice_names = ['choice_{}'.format(index) for index in range(num_choices)] + question_text = 'The correct answer is Choice {}'.format(correct_choice) + + return MultipleChoiceResponseXMLFactory().build_xml( + question_text=question_text, + choices=choices, + choice_names=choice_names, + ) + + def create_multiple_choice_problem(problem_name): """ Return the Multiple Choice Problem Descriptor, given the name of the problem. """ - factory = MultipleChoiceResponseXMLFactory() - xml_data = factory.build_xml( - question_text='The correct answer is Choice 2', - choices=[False, False, True, False], - choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] - ) - + xml_data = create_multiple_choice_xml() return XBlockFixtureDesc( 'problem', problem_name, diff --git a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py b/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py index adc11cbf91..c911d96b6b 100644 --- a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py +++ b/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py @@ -14,7 +14,7 @@ from common.test.acceptance.pages.lms.auto_auth import AutoAuthPage from common.test.acceptance.pages.studio.overview import CourseOutlinePage from common.test.acceptance.pages.lms.create_mode import ModeCreationPage from common.test.acceptance.pages.lms.courseware import CoursewarePage -from common.test.acceptance.pages.lms.instructor_dashboard import InstructorDashboardPage +from common.test.acceptance.pages.lms.instructor_dashboard import InstructorDashboardPage, EntranceExamAdmin from common.test.acceptance.fixtures.course import CourseFixture, XBlockFixtureDesc from common.test.acceptance.pages.lms.dashboard import DashboardPage from common.test.acceptance.pages.lms.problem import ProblemPage @@ -403,10 +403,17 @@ class ProctoredExamsTest(BaseInstructorDashboardTest): @attr(shard=7) +@ddt.ddt class EntranceExamGradeTest(BaseInstructorDashboardTest): """ Tests for Entrance exam specific student grading tasks. """ + admin_buttons = ( + 'reset_attempts_button', + 'rescore_button', + 'rescore_if_higher_button', + 'delete_state_button', + ) def setUp(self): super(EntranceExamGradeTest, self).setUp() @@ -426,7 +433,7 @@ class EntranceExamGradeTest(BaseInstructorDashboardTest): # go to the student admin page on the instructor dashboard self.log_in_as_instructor() - self.student_admin_section = self.visit_instructor_dashboard().select_student_admin() + self.entrance_exam_admin = self.visit_instructor_dashboard().select_student_admin(EntranceExamAdmin) def test_input_text_and_buttons_are_visible(self): """ @@ -437,86 +444,57 @@ class EntranceExamGradeTest(BaseInstructorDashboardTest): Then I see Student Email input box, Reset Student Attempt, Rescore Student Submission, Delete Student State for entrance exam and Show Background Task History for Student buttons """ - self.assertTrue(self.student_admin_section.is_student_email_input_visible()) - self.assertTrue(self.student_admin_section.is_reset_attempts_button_visible()) - self.assertTrue(self.student_admin_section.is_rescore_submission_button_visible()) - self.assertTrue(self.student_admin_section.is_delete_student_state_button_visible()) - self.assertTrue(self.student_admin_section.is_background_task_history_button_visible()) + self.assertTrue(self.entrance_exam_admin.are_all_buttons_visible()) - def test_clicking_reset_student_attempts_button_without_email_shows_error(self): + @ddt.data(*admin_buttons) + def test_admin_button_without_email_shows_error(self, button_to_test): """ - Scenario: Clicking on the Reset Student Attempts button without entering student email + Scenario: Clicking on the requested button without entering student email address or username results in error. Given that I am on the Student Admin tab on the Instructor Dashboard - When I click the Reset Student Attempts Button under Entrance Exam Grade + When I click the requested button under Entrance Exam Grade Adjustment without enter an email address Then I should be shown an Error Notification And The Notification message should read 'Please enter a student email address or username.' """ - self.student_admin_section.click_reset_attempts_button() + getattr(self.entrance_exam_admin, button_to_test).click() self.assertEqual( 'Please enter a student email address or username.', - self.student_admin_section.top_notification.text[0] + self.entrance_exam_admin.top_notification.text[0] ) - def test_clicking_reset_student_attempts_button_with_success(self): + @ddt.data(*admin_buttons) + def test_admin_button_with_success(self, button_to_test): """ - Scenario: Clicking on the Reset Student Attempts button with valid student email + Scenario: Clicking on the requested button with valid student email address or username should result in success prompt. Given that I am on the Student Admin tab on the Instructor Dashboard - When I click the Reset Student Attempts Button under Entrance Exam Grade + When I click the requested button under Entrance Exam Grade Adjustment after entering a valid student email address or username Then I should be shown an alert with success message """ - self.student_admin_section.set_student_email(self.student_identifier) - self.student_admin_section.click_reset_attempts_button() - alert = get_modal_alert(self.student_admin_section.browser) + self.entrance_exam_admin.set_student_email_or_username(self.student_identifier) + getattr(self.entrance_exam_admin, button_to_test).click() + alert = get_modal_alert(self.entrance_exam_admin.browser) alert.dismiss() - def test_clicking_reset_student_attempts_button_with_error(self): + @ddt.data(*admin_buttons) + def test_admin_button_with_error(self, button_to_test): """ - Scenario: Clicking on the Reset Student Attempts button with email address or username + Scenario: Clicking on the requested button with email address or username of a non existing student should result in error message. Given that I am on the Student Admin tab on the Instructor Dashboard - When I click the Reset Student Attempts Button under Entrance Exam Grade + When I click the requested Button under Entrance Exam Grade Adjustment after non existing student email address or username Then I should be shown an error message """ - self.student_admin_section.set_student_email('non_existing@example.com') - self.student_admin_section.click_reset_attempts_button() - self.student_admin_section.wait_for_ajax() - self.assertGreater(len(self.student_admin_section.top_notification.text[0]), 0) + self.entrance_exam_admin.set_student_email_or_username('non_existing@example.com') + getattr(self.entrance_exam_admin, button_to_test).click() + self.entrance_exam_admin.wait_for_ajax() + self.assertGreater(len(self.entrance_exam_admin.top_notification.text[0]), 0) - def test_clicking_rescore_submission_button_with_success(self): - """ - Scenario: Clicking on the Rescore Student Submission button with valid student email - address or username should result in success prompt. - Given that I am on the Student Admin tab on the Instructor Dashboard - When I click the Rescore Student Submission Button under Entrance Exam Grade - Adjustment after entering a valid student email address or username - Then I should be shown an alert with success message - """ - self.student_admin_section.set_student_email(self.student_identifier) - self.student_admin_section.click_rescore_submissions_button() - alert = get_modal_alert(self.student_admin_section.browser) - alert.dismiss() - - def test_clicking_rescore_submission_button_with_error(self): - """ - Scenario: Clicking on the Rescore Student Submission button with email address or username - of a non existing student should result in error message. - Given that I am on the Student Admin tab on the Instructor Dashboard - When I click the Rescore Student Submission Button under Entrance Exam Grade - Adjustment after non existing student email address or username - Then I should be shown an error message - """ - self.student_admin_section.set_student_email('non_existing@example.com') - self.student_admin_section.click_rescore_submissions_button() - self.student_admin_section.wait_for_ajax() - self.assertGreater(len(self.student_admin_section.top_notification.text[0]), 0) - - def test_clicking_skip_entrance_exam_button_with_success(self): + def test_skip_entrance_exam_button_with_success(self): """ Scenario: Clicking on the Let Student Skip Entrance Exam button with valid student email address or username should result in success prompt. @@ -526,17 +504,18 @@ class EntranceExamGradeTest(BaseInstructorDashboardTest): email address or username Then I should be shown an alert with success message """ - self.student_admin_section.set_student_email(self.student_identifier) - self.student_admin_section.click_skip_entrance_exam_button() + self.entrance_exam_admin.set_student_email_or_username(self.student_identifier) + self.entrance_exam_admin.skip_entrance_exam_button.click() + #first we have window.confirm - alert = get_modal_alert(self.student_admin_section.browser) + alert = get_modal_alert(self.entrance_exam_admin.browser) alert.accept() # then we have alert confirming action - alert = get_modal_alert(self.student_admin_section.browser) + alert = get_modal_alert(self.entrance_exam_admin.browser) alert.dismiss() - def test_clicking_skip_entrance_exam_button_with_error(self): + def test_skip_entrance_exam_button_with_error(self): """ Scenario: Clicking on the Let Student Skip Entrance Exam button with email address or username of a non existing student should result in error message. @@ -546,47 +525,17 @@ class EntranceExamGradeTest(BaseInstructorDashboardTest): student email address or username Then I should be shown an error message """ - self.student_admin_section.set_student_email('non_existing@example.com') - self.student_admin_section.click_skip_entrance_exam_button() + self.entrance_exam_admin.set_student_email_or_username('non_existing@example.com') + self.entrance_exam_admin.skip_entrance_exam_button.click() + #first we have window.confirm - alert = get_modal_alert(self.student_admin_section.browser) + alert = get_modal_alert(self.entrance_exam_admin.browser) alert.accept() - self.student_admin_section.wait_for_ajax() - self.assertGreater(len(self.student_admin_section.top_notification.text[0]), 0) + self.entrance_exam_admin.wait_for_ajax() + self.assertGreater(len(self.entrance_exam_admin.top_notification.text[0]), 0) - def test_clicking_delete_student_attempts_button_with_success(self): - """ - Scenario: Clicking on the Delete Student State for entrance exam button - with valid student email address or username should result in success prompt. - Given that I am on the Student Admin tab on the Instructor Dashboard - When I click the Delete Student State for entrance exam Button - under Entrance Exam Grade Adjustment after entering a valid student - email address or username - Then I should be shown an alert with success message - """ - self.student_admin_section.set_student_email(self.student_identifier) - self.student_admin_section.click_delete_student_state_button() - alert = get_modal_alert(self.student_admin_section.browser) - alert.dismiss() - - def test_clicking_delete_student_attempts_button_with_error(self): - """ - Scenario: Clicking on the Delete Student State for entrance exam button - with email address or username of a non existing student should result - in error message. - Given that I am on the Student Admin tab on the Instructor Dashboard - When I click the Delete Student State for entrance exam Button - under Entrance Exam Grade Adjustment after non existing student - email address or username - Then I should be shown an error message - """ - self.student_admin_section.set_student_email('non_existing@example.com') - self.student_admin_section.click_delete_student_state_button() - self.student_admin_section.wait_for_ajax() - self.assertGreater(len(self.student_admin_section.top_notification.text[0]), 0) - - def test_clicking_task_history_button_with_success(self): + def test_task_history_button_with_success(self): """ Scenario: Clicking on the Show Background Task History for Student with valid student email address or username should result in table of tasks. @@ -594,11 +543,11 @@ class EntranceExamGradeTest(BaseInstructorDashboardTest): When I click the Show Background Task History for Student Button under Entrance Exam Grade Adjustment after entering a valid student email address or username - Then I should be shown an table listing all background tasks + Then I should be shown a table listing all background tasks """ - self.student_admin_section.set_student_email(self.student_identifier) - self.student_admin_section.click_task_history_button() - self.assertTrue(self.student_admin_section.is_background_task_history_table_visible()) + self.entrance_exam_admin.set_student_email_or_username(self.student_identifier) + self.entrance_exam_admin.task_history_button.click() + self.entrance_exam_admin.wait_for_task_history_table() @attr(shard=7) diff --git a/common/test/acceptance/tests/lms/test_lms_user_preview.py b/common/test/acceptance/tests/lms/test_lms_user_preview.py index 71ecf6faa1..7193df3f24 100644 --- a/common/test/acceptance/tests/lms/test_lms_user_preview.py +++ b/common/test/acceptance/tests/lms/test_lms_user_preview.py @@ -116,8 +116,9 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = self._goto_staff_page().open_staff_debug_info() staff_debug_page.reset_attempts() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully reset the attempts ' - 'for user {}'.format(self.USERNAME), msg) + self.assertEqual( + u'Successfully reset the attempts for user {}'.format(self.USERNAME), msg, + ) def test_delete_state_empty(self): """ @@ -126,8 +127,9 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = self._goto_staff_page().open_staff_debug_info() staff_debug_page.delete_state() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully deleted student state ' - 'for user {}'.format(self.USERNAME), msg) + self.assertEqual( + u'Successfully deleted student state for user {}'.format(self.USERNAME), msg, + ) def test_reset_attempts_state(self): """ @@ -139,10 +141,11 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.reset_attempts() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully reset the attempts ' - 'for user {}'.format(self.USERNAME), msg) + self.assertEqual( + u'Successfully reset the attempts for user {}'.format(self.USERNAME), msg, + ) - def test_rescore_state(self): + def test_rescore_problem(self): """ Rescore the student """ @@ -152,7 +155,19 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.rescore() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully rescored problem for user STAFF_TESTER', msg) + self.assertEqual(u'Successfully rescored problem for user {}'.format(self.USERNAME), msg) + + def test_rescore_problem_if_higher(self): + """ + Rescore the student + """ + staff_page = self._goto_staff_page() + staff_page.answer_problem() + + staff_debug_page = staff_page.open_staff_debug_info() + staff_debug_page.rescore_if_higher() + msg = staff_debug_page.idash_msg[0] + self.assertEqual(u'Successfully rescored problem to improve score for user {}'.format(self.USERNAME), msg) def test_student_state_delete(self): """ @@ -164,8 +179,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.delete_state() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully deleted student state ' - 'for user {}'.format(self.USERNAME), msg) + self.assertEqual(u'Successfully deleted student state for user {}'.format(self.USERNAME), msg) def test_student_by_email(self): """ @@ -177,8 +191,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.reset_attempts(self.EMAIL) msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully reset the attempts ' - 'for user {}'.format(self.EMAIL), msg) + self.assertEqual(u'Successfully reset the attempts for user {}'.format(self.EMAIL), msg) def test_bad_student(self): """ @@ -189,8 +202,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.delete_state('INVALIDUSER') msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Failed to delete student state. ' - 'User does not exist.', msg) + self.assertEqual(u'Failed to delete student state for user. User does not exist.', msg) def test_reset_attempts_for_problem_loaded_via_ajax(self): """ @@ -203,8 +215,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.reset_attempts() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully reset the attempts ' - 'for user {}'.format(self.USERNAME), msg) + self.assertEqual(u'Successfully reset the attempts for user {}'.format(self.USERNAME), msg) def test_rescore_state_for_problem_loaded_via_ajax(self): """ @@ -217,7 +228,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.rescore() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully rescored problem for user STAFF_TESTER', msg) + self.assertEqual(u'Successfully rescored problem for user {}'.format(self.USERNAME), msg) def test_student_state_delete_for_problem_loaded_via_ajax(self): """ @@ -230,8 +241,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): staff_debug_page = staff_page.open_staff_debug_info() staff_debug_page.delete_state() msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully deleted student state ' - 'for user {}'.format(self.USERNAME), msg) + self.assertEqual(u'Successfully deleted student state for user {}'.format(self.USERNAME), msg) class CourseWithContentGroupsTest(StaffViewTest): diff --git a/common/test/acceptance/tests/lms/test_progress_page.py b/common/test/acceptance/tests/lms/test_progress_page.py index 111181eaea..dfc0087c15 100644 --- a/common/test/acceptance/tests/lms/test_progress_page.py +++ b/common/test/acceptance/tests/lms/test_progress_page.py @@ -3,19 +3,19 @@ End-to-end tests for the LMS that utilize the progress page. """ - from contextlib import contextmanager import ddt -from ..helpers import UniqueCourseTest, auto_auth, create_multiple_choice_problem +from ..helpers import ( + UniqueCourseTest, auto_auth, create_multiple_choice_problem, create_multiple_choice_xml, get_modal_alert +) from ...fixtures.course import CourseFixture, XBlockFixtureDesc from ...pages.common.logout import LogoutPage from ...pages.lms.courseware import CoursewarePage -from ...pages.lms.instructor_dashboard import InstructorDashboardPage +from ...pages.lms.instructor_dashboard import InstructorDashboardPage, StudentSpecificAdmin from ...pages.lms.problem import ProblemPage from ...pages.lms.progress import ProgressPage -from ...pages.lms.staff_view import StaffPage, StaffDebugPage from ...pages.studio.component_editor import ComponentEditorView from ...pages.studio.utils import type_in_codemirror from ...pages.studio.overview import CourseOutlinePage @@ -56,13 +56,13 @@ class ProgressPageBaseTest(UniqueCourseTest): self.course_info['display_name'] ) + self.problem1 = create_multiple_choice_problem(self.PROBLEM_NAME) + self.problem2 = create_multiple_choice_problem(self.PROBLEM_NAME_2) + self.course_fix.add_children( XBlockFixtureDesc('chapter', self.SECTION_NAME).add_children( XBlockFixtureDesc('sequential', self.SUBSECTION_NAME).add_children( - XBlockFixtureDesc('vertical', self.UNIT_NAME).add_children( - create_multiple_choice_problem(self.PROBLEM_NAME), - create_multiple_choice_problem(self.PROBLEM_NAME_2) - ) + XBlockFixtureDesc('vertical', self.UNIT_NAME).add_children(self.problem1, self.problem2) ) ) ).install() @@ -74,8 +74,14 @@ class ProgressPageBaseTest(UniqueCourseTest): """ Submit a correct answer to the problem. """ + self._answer_problem(choice=2) + + def _answer_problem(self, choice): + """ + Submit the given choice for the problem. + """ self.courseware_page.go_to_sequential_position(1) - self.problem_page.click_choice('choice_choice_2') + self.problem_page.click_choice('choice_choice_{}'.format(choice)) self.problem_page.click_submit() def _get_section_score(self): @@ -92,19 +98,6 @@ class ProgressPageBaseTest(UniqueCourseTest): self.progress_page.visit() return self.progress_page.scores(self.SECTION_NAME, self.SUBSECTION_NAME) - def _check_progress_page_with_scored_problem(self): - """ - Checks the progress page before and after answering - the course's first problem correctly. - """ - with self._logged_in_session(): - self.assertEqual(self._get_problem_scores(), [(0, 1), (0, 1)]) - self.assertEqual(self._get_section_score(), (0, 2)) - self.courseware_page.visit() - self._answer_problem_correctly() - self.assertEqual(self._get_problem_scores(), [(1, 1), (0, 1)]) - self.assertEqual(self._get_section_score(), (1, 2)) - @contextmanager def _logged_in_session(self, staff=False): """ @@ -122,14 +115,6 @@ class ProgressPageBaseTest(UniqueCourseTest): self.logout_page.visit() -class ProgressPageTest(ProgressPageBaseTest): - """ - Test that the progress page reports scores from completed assessments. - """ - def test_progress_page_shows_scored_problems(self): - self._check_progress_page_with_scored_problem() - - @ddt.ddt class PersistentGradesTest(ProgressPageBaseTest): """ @@ -145,104 +130,132 @@ class PersistentGradesTest(ProgressPageBaseTest): Adds a unit to the subsection, which should not affect a persisted subsection grade. """ - with self._logged_in_session(staff=True): - self.course_outline.visit() - subsection = self.course_outline.section(self.SECTION_NAME).subsection(self.SUBSECTION_NAME) - subsection.expand_subsection() - subsection.add_unit() + self.course_outline.visit() + subsection = self.course_outline.section(self.SECTION_NAME).subsection(self.SUBSECTION_NAME) + subsection.expand_subsection() + subsection.add_unit() + subsection.publish() def _set_staff_lock_on_subsection(self, locked): """ Sets staff lock for a subsection, which should hide the subsection score from students on the progress page. """ - with self._logged_in_session(staff=True): - self.course_outline.visit() - subsection = self.course_outline.section_at(0).subsection_at(0) - subsection.set_staff_lock(locked) - self.assertEqual(subsection.has_staff_lock_warning, locked) + self.course_outline.visit() + subsection = self.course_outline.section_at(0).subsection_at(0) + subsection.set_staff_lock(locked) + self.assertEqual(subsection.has_staff_lock_warning, locked) + + def _get_problem_in_studio(self): + """ + Returns the editable problem component in studio, + along with its container unit, so any changes can + be published. + """ + self.course_outline.visit() + self.course_outline.section_at(0).subsection_at(0).expand_subsection() + unit = self.course_outline.section_at(0).subsection_at(0).unit(self.UNIT_NAME).go_to() + component = unit.xblocks[1] + return unit, component def _change_weight_for_problem(self): """ Changes the weight of the problem, which should not affect persisted grades. """ - with self._logged_in_session(staff=True): - self.course_outline.visit() - self.course_outline.section_at(0).subsection_at(0).expand_subsection() - unit = self.course_outline.section_at(0).subsection_at(0).unit(self.UNIT_NAME).go_to() - component = unit.xblocks[1] - component.edit() - component_editor = ComponentEditorView(self.browser, component.locator) - component_editor.set_field_value_and_save('Problem Weight', 5) + unit, component = self._get_problem_in_studio() + component.edit() + component_editor = ComponentEditorView(self.browser, component.locator) + component_editor.set_field_value_and_save('Problem Weight', 5) + unit.publish() - def _edit_problem_content(self): + def _change_correct_answer_for_problem(self, new_correct_choice=1): """ - Replaces the content of a problem with other html. - Should not affect persisted grades. + Changes the correct answer of the problem. """ - with self._logged_in_session(staff=True): - self.course_outline.visit() - self.course_outline.section_at(0).subsection_at(0).expand_subsection() - unit = self.course_outline.section_at(0).subsection_at(0).unit(self.UNIT_NAME).go_to() - component = unit.xblocks[1] - modal = component.edit() + unit, component = self._get_problem_in_studio() + modal = component.edit() - # Set content in the CodeMirror editor. - modified_content = "

modified content

" - type_in_codemirror(self, 0, modified_content) - modal.q(css='.action-save').click() + modified_content = create_multiple_choice_xml(correct_choice=new_correct_choice) - def _delete_student_state_for_problem(self): + type_in_codemirror(self, 0, modified_content) + modal.q(css='.action-save').click() + unit.publish() + + def _student_admin_action_for_problem(self, action_button, has_cancellable_alert=False): """ As staff, clicks the "delete student state" button, deleting the student user's state for the problem. """ - with self._logged_in_session(staff=True): + self.instructor_dashboard_page.visit() + student_admin_section = self.instructor_dashboard_page.select_student_admin(StudentSpecificAdmin) + student_admin_section.set_student_email_or_username(self.USERNAME) + student_admin_section.set_problem_location(self.problem1.locator) + getattr(student_admin_section, action_button).click() + if has_cancellable_alert: + alert = get_modal_alert(student_admin_section.browser) + alert.accept() + alert = get_modal_alert(student_admin_section.browser) + alert.dismiss() + return student_admin_section + + def test_progress_page_shows_scored_problems(self): + """ + Checks the progress page before and after answering + the course's first problem correctly. + """ + with self._logged_in_session(): + self.assertEqual(self._get_problem_scores(), [(0, 1), (0, 1)]) + self.assertEqual(self._get_section_score(), (0, 2)) self.courseware_page.visit() - staff_page = StaffPage(self.browser, self.course_id) - self.assertEqual(staff_page.staff_view_mode, "Staff") - staff_page.q(css='a.instructor-info-action').nth(1).click() - staff_debug_page = StaffDebugPage(self.browser) - staff_debug_page.wait_for_page() - staff_debug_page.delete_state(self.USERNAME) - msg = staff_debug_page.idash_msg[0] - self.assertEqual(u'Successfully deleted student state for user {0}'.format(self.USERNAME), msg) + self._answer_problem_correctly() + self.assertEqual(self._get_problem_scores(), [(1, 1), (0, 1)]) + self.assertEqual(self._get_section_score(), (1, 2)) @ddt.data( - _edit_problem_content, + _change_correct_answer_for_problem, _change_subsection_structure, _change_weight_for_problem ) def test_content_changes_do_not_change_score(self, edit): with self._logged_in_session(): - self._check_progress_page_with_scored_problem() + self.courseware_page.visit() + self._answer_problem_correctly() - edit(self) + with self._logged_in_session(staff=True): + edit(self) with self._logged_in_session(): self.assertEqual(self._get_problem_scores(), [(1, 1), (0, 1)]) self.assertEqual(self._get_section_score(), (1, 2)) - def test_visibility_change_does_affect_score(self): + def test_visibility_change_affects_score(self): with self._logged_in_session(): - self._check_progress_page_with_scored_problem() + self.courseware_page.visit() + self._answer_problem_correctly() - self._set_staff_lock_on_subsection(True) + with self._logged_in_session(staff=True): + self._set_staff_lock_on_subsection(True) with self._logged_in_session(): self.assertEqual(self._get_problem_scores(), None) self.assertEqual(self._get_section_score(), None) - self._set_staff_lock_on_subsection(False) + with self._logged_in_session(staff=True): + self._set_staff_lock_on_subsection(False) with self._logged_in_session(): self.assertEqual(self._get_problem_scores(), [(1, 1), (0, 1)]) self.assertEqual(self._get_section_score(), (1, 2)) - def test_progress_page_updates_when_student_state_deleted(self): - self._check_progress_page_with_scored_problem() - self._delete_student_state_for_problem() + def test_delete_student_state_affects_score(self): + with self._logged_in_session(): + self.courseware_page.visit() + self._answer_problem_correctly() + + with self._logged_in_session(staff=True): + self._student_admin_action_for_problem('delete_state_button', has_cancellable_alert=True) + with self._logged_in_session(): self.assertEqual(self._get_problem_scores(), [(0, 1), (0, 1)]) self.assertEqual(self._get_section_score(), (0, 2)) diff --git a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py index 5f88c8004e..c6a8fd6b18 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py @@ -172,8 +172,9 @@ class MilestonesTransformerTestCase(CourseStructureTestCase, MilestonesTestCaseM # block passed in, so we pass in a child of the gating block lms_gating_api.evaluate_prerequisite( self.course, - UsageKey.from_string(unicode(self.blocks[gating_block_child].location)), - self.user.id) + self.blocks[gating_block_child], + self.user.id, + ) with self.assertNumQueries(2): self.get_blocks_and_check_against_expected(self.user, self.ALL_BLOCKS_EXCEPT_SPECIAL) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index cb0fdcfaa8..da6e88153c 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -16,7 +16,6 @@ from edxmako.shortcuts import render_to_string from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from static_replace import replace_static_urls -from xmodule.modulestore import ModuleStoreEnum from xmodule.x_module import STUDENT_VIEW from courseware.access import has_access diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 7911cd7bed..0c288286ec 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -1007,3 +1007,20 @@ def set_score(user_id, usage_key, score, max_score): student_module.grade = score student_module.max_grade = max_score student_module.save() + + +def get_score(user_id, usage_key): + """ + Get the score and max_score for the specified user and xblock usage. + Returns None if not found. + """ + try: + student_module = StudentModule.objects.get( + student_id=user_id, + module_state_key=usage_key, + course_id=usage_key.course_key, + ) + except StudentModule.DoesNotExist: + return None + else: + return student_module.grade, student_module.max_grade diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 147ef1ae84..77bbff227d 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -8,7 +8,6 @@ import logging from collections import OrderedDict from functools import partial -import dogstats_wrapper as dog_stats_api import newrelic.agent from capa.xqueue_interface import XQueueInterface from django.conf import settings @@ -18,7 +17,6 @@ from django.core.context_processors import csrf from django.core.exceptions import PermissionDenied from django.core.urlresolvers import reverse from django.http import Http404, HttpResponse -from django.test.client import RequestFactory from django.views.decorators.csrf import csrf_exempt from edx_proctoring.services import ProctoringService from eventtracking import tracker @@ -34,7 +32,6 @@ from xblock.reference.plugins import FSService import static_replace from courseware.access import has_access, get_user_role from courseware.entrance_exams import ( - get_entrance_exam_score, user_must_complete_entrance_exam, user_has_passed_entrance_exam ) @@ -44,14 +41,14 @@ from courseware.masquerade import ( is_masquerading_as_specific_student, setup_masquerade, ) -from courseware.model_data import DjangoKeyValueStore, FieldDataCache, set_score -from lms.djangoapps.grades.signals.signals import SCORE_CHANGED +from courseware.model_data import DjangoKeyValueStore, FieldDataCache from edxmako.shortcuts import render_to_string +from lms.djangoapps.grades.signals.signals import SCORE_PUBLISHED from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig -from openedx.core.djangoapps.bookmarks.services import BookmarksService from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem from lms.djangoapps.verify_student.services import VerificationService, ReverificationService +from openedx.core.djangoapps.bookmarks.services import BookmarksService from openedx.core.djangoapps.credit.services import CreditService from openedx.core.djangoapps.util.user_utils import SystemUser from openedx.core.lib.xblock_utils import ( @@ -466,89 +463,17 @@ def get_module_system_for_user(user, student_data, # TODO # pylint: disable=to course=course ) - def _fulfill_content_milestones(user, course_key, content_key): - """ - Internal helper to handle milestone fulfillments for the specified content module - """ - # Fulfillment Use Case: Entrance Exam - # If this module is part of an entrance exam, we'll need to see if the student - # has reached the point at which they can collect the associated milestone - if milestones_helpers.is_entrance_exams_enabled(): - course = modulestore().get_course(course_key) - content = modulestore().get_item(content_key) - entrance_exam_enabled = getattr(course, 'entrance_exam_enabled', False) - in_entrance_exam = getattr(content, 'in_entrance_exam', False) - if entrance_exam_enabled and in_entrance_exam: - # We don't have access to the true request object in this context, but we can use a mock - request = RequestFactory().request() - request.user = user - exam_pct = get_entrance_exam_score(request, course) - if exam_pct >= course.entrance_exam_minimum_score_pct: - exam_key = UsageKey.from_string(course.entrance_exam_id) - relationship_types = milestones_helpers.get_milestone_relationship_types() - content_milestones = milestones_helpers.get_course_content_milestones( - course_key, - exam_key, - relationship=relationship_types['FULFILLS'] - ) - # Add each milestone to the user's set... - user = {'id': request.user.id} - for milestone in content_milestones: - milestones_helpers.add_user_milestone(user, milestone) - - def handle_grade_event(block, event_type, event): # pylint: disable=unused-argument - """ - Manages the workflow for recording and updating of student module grade state - """ - user_id = user.id - - grade = event.get('value') - max_grade = event.get('max_value') - - set_score( - user_id, - descriptor.location, - grade, - max_grade, - ) - - # Bin score into range and increment stats - score_bucket = get_score_bucket(grade, max_grade) - - tags = [ - u"org:{}".format(course_id.org), - u"course:{}".format(course_id), - u"score_bucket:{0}".format(score_bucket) - ] - - if grade_bucket_type is not None: - tags.append('type:%s' % grade_bucket_type) - - dog_stats_api.increment("lms.courseware.question_answered", tags=tags) - - # Cycle through the milestone fulfillment scenarios to see if any are now applicable - # thanks to the updated grading information that was just submitted - _fulfill_content_milestones( - user, - course_id, - descriptor.location, - ) - - # Send a signal out to any listeners who are waiting for score change - # events. - SCORE_CHANGED.send( - sender=None, - points_possible=event['max_value'], - points_earned=event['value'], - user_id=user.id, - course_id=unicode(course_id), - usage_id=unicode(descriptor.location) - ) - def publish(block, event_type, event): """A function that allows XModules to publish events.""" if event_type == 'grade' and not is_masquerading_as_specific_student(user, course_id): - handle_grade_event(block, event_type, event) + SCORE_PUBLISHED.send( + sender=None, + block=block, + user=user, + raw_earned=event['value'], + raw_possible=event['max_value'], + only_if_higher=event.get('only_if_higher'), + ) else: aside_context = {} for aside in block.runtime.get_asides(block): @@ -1138,20 +1063,6 @@ def xblock_view(request, course_id, usage_id, view_name): }) -def get_score_bucket(grade, max_grade): - """ - Function to split arbitrary score ranges into 3 buckets. - Used with statsd tracking. - """ - score_bucket = "incorrect" - if grade > 0 and grade < max_grade: - score_bucket = "partial" - elif grade == max_grade: - score_bucket = "correct" - - return score_bucket - - def _check_files_limits(files): """ Check if the files in a request are under the limits defined by diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index 193de70570..ebe4828d13 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -1,3 +1,7 @@ +""" +Helpers for courseware tests. +""" +import crum import json from django.contrib.auth.models import User @@ -19,6 +23,9 @@ def get_request_for_user(user): request.is_secure = lambda: True request.get_host = lambda: "edx.org" request.method = 'GET' + request.GET = {} + request.POST = {} + crum.set_current_request(request) return request diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 6ec12729e8..7b89397c40 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -253,14 +253,6 @@ class ModuleRenderTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase): self.dispatch ) - def test_get_score_bucket(self): - self.assertEquals(render.get_score_bucket(0, 10), 'incorrect') - self.assertEquals(render.get_score_bucket(1, 10), 'partial') - self.assertEquals(render.get_score_bucket(10, 10), 'correct') - # get_score_bucket calls error cases 'incorrect' - self.assertEquals(render.get_score_bucket(11, 10), 'incorrect') - self.assertEquals(render.get_score_bucket(-1, 10), 'incorrect') - def test_anonymous_handle_xblock_callback(self): dispatch_url = reverse( 'xblock_handler', @@ -1839,7 +1831,7 @@ class TestXmoduleRuntimeEvent(TestSubmittingProblems): self.assertIsNone(student_module.grade) self.assertIsNone(student_module.max_grade) - @patch('courseware.module_render.SCORE_CHANGED.send') + @patch('lms.djangoapps.grades.signals.handlers.SCORE_CHANGED.send') def test_score_change_signal(self, send_mock): """Test that a Django signal is generated when a score changes""" self.set_module_grade_using_publish(self.grade_dict) @@ -1849,7 +1841,8 @@ class TestXmoduleRuntimeEvent(TestSubmittingProblems): 'points_earned': self.grade_dict['value'], 'user_id': self.student_user.id, 'course_id': unicode(self.course.id), - 'usage_id': unicode(self.problem.location) + 'usage_id': unicode(self.problem.location), + 'only_if_higher': None, } send_mock.assert_called_with(**expected_signal_kwargs) diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 0d53e247b4..046f356041 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -153,7 +153,7 @@ class TestSubmittingProblems(ModuleStoreTestCase, LoginEnrollmentTestCase, Probl self.student_user = User.objects.get(email=self.student) self.factory = RequestFactory() # Disable the score change signal to prevent other components from being pulled into tests. - self.score_changed_signal_patch = patch('courseware.module_render.SCORE_CHANGED.send') + self.score_changed_signal_patch = patch('lms.djangoapps.grades.signals.handlers.SCORE_CHANGED.send') self.score_changed_signal_patch.start() def tearDown(self): diff --git a/lms/djangoapps/gating/api.py b/lms/djangoapps/gating/api.py index 416d70d583..0f2a8ea55b 100644 --- a/lms/djangoapps/gating/api.py +++ b/lms/djangoapps/gating/api.py @@ -1,16 +1,19 @@ """ API for the gating djangoapp """ -import logging -import json - from collections import defaultdict from django.contrib.auth.models import User -from xmodule.modulestore.django import modulestore +from django.test.client import RequestFactory +import json +import logging + from openedx.core.lib.gating import api as gating_api +from opaque_keys.edx.keys import UsageKey +from lms.djangoapps.courseware.entrance_exams import get_entrance_exam_score from lms.djangoapps.grades.module_grades import get_module_score from util import milestones_helpers + log = logging.getLogger(__name__) @@ -34,7 +37,7 @@ def _get_xblock_parent(xblock, category=None): @gating_api.gating_enabled(default=False) -def evaluate_prerequisite(course, prereq_content_key, user_id): +def evaluate_prerequisite(course, block, user_id): """ Finds the parent subsection of the content in the course and evaluates any milestone relationships attached to that subsection. If the calculated @@ -42,15 +45,14 @@ def evaluate_prerequisite(course, prereq_content_key, user_id): dependent subsections, the related milestone will be fulfilled for the user. Arguments: - user_id (int): ID of User for which evaluation should occur course (CourseModule): The course prereq_content_key (UsageKey): The prerequisite content usage key + user_id (int): ID of User for which evaluation should occur Returns: None """ - xblock = modulestore().get_item(prereq_content_key) - sequential = _get_xblock_parent(xblock, 'sequential') + sequential = _get_xblock_parent(block, 'sequential') if sequential: prereq_milestone = gating_api.get_gating_milestone( course.id, @@ -83,3 +85,32 @@ def evaluate_prerequisite(course, prereq_content_key, user_id): milestones_helpers.add_user_milestone({'id': user_id}, prereq_milestone) else: milestones_helpers.remove_user_milestone({'id': user_id}, prereq_milestone) + + +def evaluate_entrance_exam(course, block, user_id): + """ + Update milestone fulfillments for the specified content module + """ + # Fulfillment Use Case: Entrance Exam + # If this module is part of an entrance exam, we'll need to see if the student + # has reached the point at which they can collect the associated milestone + if milestones_helpers.is_entrance_exams_enabled(): + entrance_exam_enabled = getattr(course, 'entrance_exam_enabled', False) + in_entrance_exam = getattr(block, 'in_entrance_exam', False) + if entrance_exam_enabled and in_entrance_exam: + # We don't have access to the true request object in this context, but we can use a mock + request = RequestFactory().request() + request.user = User.objects.get(id=user_id) + exam_pct = get_entrance_exam_score(request, course) + if exam_pct >= course.entrance_exam_minimum_score_pct: + exam_key = UsageKey.from_string(course.entrance_exam_id) + relationship_types = milestones_helpers.get_milestone_relationship_types() + content_milestones = milestones_helpers.get_course_content_milestones( + course.id, + exam_key, + relationship=relationship_types['FULFILLS'] + ) + # Add each milestone to the user's set... + user = {'id': request.user.id} + for milestone in content_milestones: + milestones_helpers.add_user_milestone(user, milestone) diff --git a/lms/djangoapps/gating/signals.py b/lms/djangoapps/gating/signals.py index 16eed7371b..6473351cd7 100644 --- a/lms/djangoapps/gating/signals.py +++ b/lms/djangoapps/gating/signals.py @@ -2,10 +2,11 @@ Signal handlers for the gating djangoapp """ from django.dispatch import receiver + +from gating import api as gating_api +from lms.djangoapps.grades.signals.signals import SCORE_CHANGED from opaque_keys.edx.keys import CourseKey, UsageKey from xmodule.modulestore.django import modulestore -from lms.djangoapps.grades.signals.signals import SCORE_CHANGED -from gating import api as gating_api @receiver(SCORE_CHANGED) @@ -22,9 +23,6 @@ def handle_score_changed(**kwargs): None """ course = modulestore().get_course(CourseKey.from_string(kwargs.get('course_id'))) - if course.enable_subsection_gating: - gating_api.evaluate_prerequisite( - course, - UsageKey.from_string(kwargs.get('usage_id')), - kwargs.get('user_id'), - ) + block = modulestore().get_item(UsageKey.from_string(kwargs.get('usage_id'))) + gating_api.evaluate_prerequisite(course, block, kwargs.get('user_id')) + gating_api.evaluate_entrance_exam(course, block, kwargs.get('user_id')) diff --git a/lms/djangoapps/gating/tests/test_api.py b/lms/djangoapps/gating/tests/test_api.py index 65d677a740..3b1df7ce87 100644 --- a/lms/djangoapps/gating/tests/test_api.py +++ b/lms/djangoapps/gating/tests/test_api.py @@ -134,7 +134,7 @@ class TestEvaluatePrerequisite(GatingTestCase, MilestonesTestCaseMixin): self._setup_gating_milestone(50) mock_module_score.return_value = module_score - evaluate_prerequisite(self.course, self.prob1.location, self.user.id) + evaluate_prerequisite(self.course, self.prob1, self.user.id) self.assertEqual(milestones_api.user_has_milestone(self.user_dict, self.prereq_milestone), result) @patch('gating.api.log.warning') @@ -147,7 +147,7 @@ class TestEvaluatePrerequisite(GatingTestCase, MilestonesTestCaseMixin): self._setup_gating_milestone(None) mock_module_score.return_value = module_score - evaluate_prerequisite(self.course, self.prob1.location, self.user.id) + evaluate_prerequisite(self.course, self.prob1, self.user.id) self.assertEqual(milestones_api.user_has_milestone(self.user_dict, self.prereq_milestone), result) self.assertTrue(mock_log.called) @@ -155,14 +155,14 @@ class TestEvaluatePrerequisite(GatingTestCase, MilestonesTestCaseMixin): def test_orphaned_xblock(self, mock_module_score): """ Test test_orphaned_xblock """ - evaluate_prerequisite(self.course, self.prob2.location, self.user.id) + evaluate_prerequisite(self.course, self.prob2, self.user.id) self.assertFalse(mock_module_score.called) @patch('gating.api.get_module_score') def test_no_prerequisites(self, mock_module_score): """ Test test_no_prerequisites """ - evaluate_prerequisite(self.course, self.prob1.location, self.user.id) + evaluate_prerequisite(self.course, self.prob1, self.user.id) self.assertFalse(mock_module_score.called) @patch('gating.api.get_module_score') @@ -172,5 +172,5 @@ class TestEvaluatePrerequisite(GatingTestCase, MilestonesTestCaseMixin): # Setup gating milestones data gating_api.add_prerequisite(self.course.id, self.seq1.location) - evaluate_prerequisite(self.course, self.prob1.location, self.user.id) + evaluate_prerequisite(self.course, self.prob1, self.user.id) self.assertFalse(mock_module_score.called) diff --git a/lms/djangoapps/gating/tests/test_signals.py b/lms/djangoapps/gating/tests/test_signals.py index 6e16498d87..7971927995 100644 --- a/lms/djangoapps/gating/tests/test_signals.py +++ b/lms/djangoapps/gating/tests/test_signals.py @@ -20,7 +20,7 @@ class TestHandleScoreChanged(ModuleStoreTestCase): super(TestHandleScoreChanged, self).setUp() self.course = CourseFactory.create(org='TestX', number='TS01', run='2016_Q1') self.user = UserFactory.create() - self.test_usage_key = UsageKey.from_string('i4x://the/content/key/12345678') + self.test_usage_key = self.course.location @patch('gating.signals.gating_api.evaluate_prerequisite') def test_gating_enabled(self, mock_evaluate): @@ -35,7 +35,7 @@ class TestHandleScoreChanged(ModuleStoreTestCase): course_id=unicode(self.course.id), usage_id=unicode(self.test_usage_key) ) - mock_evaluate.assert_called_with(self.course, self.test_usage_key, self.user.id) # pylint: disable=no-member + mock_evaluate.assert_called_with(self.course, self.course, self.user.id) # pylint: disable=no-member @patch('gating.signals.gating_api.evaluate_prerequisite') def test_gating_disabled(self, mock_evaluate): diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 74a3f90f99..33623009e0 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -11,6 +11,7 @@ from courseware.model_data import ScoresClient from lms.djangoapps.grades.scores import get_score, possibly_scored from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag +from openedx.core.lib.grade_utils import is_score_higher from student.models import anonymous_id_for_user, User from submissions import api as submissions_api from traceback import format_exc @@ -74,6 +75,7 @@ class SubsectionGrade(object): self.all_total, self.graded_total = graders.aggregate_scores(self.scores, self.display_name, self.location) self._log_event(log.debug, u"init_from_structure", student) + return self def init_from_model(self, student, model, course_structure, submissions_scores, csm_scores): """ @@ -97,6 +99,7 @@ class SubsectionGrade(object): module_id=self.location, ) self._log_event(log.debug, u"init_from_model", student) + return self @classmethod def bulk_create_models(cls, student, subsection_grades, course_key): @@ -222,10 +225,9 @@ class SubsectionGradeFactory(object): ) block_structure = self._get_block_structure(block_structure) - subsection_grade = self._get_saved_grade(subsection, block_structure) + subsection_grade = self._get_bulk_cached_grade(subsection, block_structure) if not subsection_grade: - subsection_grade = SubsectionGrade(subsection, self.course) - subsection_grade.init_from_structure( + subsection_grade = SubsectionGrade(subsection, self.course).init_from_structure( self.student, block_structure, self._submissions_scores, self._csm_scores, ) if PersistentGradesEnabledFlag.feature_enabled(self.course.id): @@ -247,7 +249,7 @@ class SubsectionGradeFactory(object): SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id) self._unsaved_subsection_grades = [] - def update(self, subsection, block_structure=None): + def update(self, subsection, block_structure=None, only_if_higher=None): """ Updates the SubsectionGrade object for the student and subsection. """ @@ -259,14 +261,30 @@ class SubsectionGradeFactory(object): self._log_event(log.warning, u"update, subsection: {}".format(subsection.location)) block_structure = self._get_block_structure(block_structure) - subsection_grade = SubsectionGrade(subsection, self.course) - subsection_grade.init_from_structure( - self.student, block_structure, self._submissions_scores, self._csm_scores + calculated_grade = SubsectionGrade(subsection, self.course).init_from_structure( + self.student, block_structure, self._submissions_scores, self._csm_scores, ) - grade_model = subsection_grade.update_or_create_model(self.student) + if only_if_higher: + try: + grade_model = PersistentSubsectionGrade.read_grade(self.student.id, subsection.location) + except PersistentSubsectionGrade.DoesNotExist: + pass + else: + orig_subsection_grade = SubsectionGrade(subsection, self.course).init_from_model( + self.student, grade_model, block_structure, self._submissions_scores, self._csm_scores, + ) + if not is_score_higher( + orig_subsection_grade.graded_total.earned, + orig_subsection_grade.graded_total.possible, + calculated_grade.graded_total.earned, + calculated_grade.graded_total.possible, + ): + return orig_subsection_grade + + grade_model = calculated_grade.update_or_create_model(self.student) self._update_saved_subsection_grade(subsection.location, grade_model) - return subsection_grade + return calculated_grade @lazy def _csm_scores(self): @@ -286,33 +304,34 @@ class SubsectionGradeFactory(object): anonymous_user_id = anonymous_id_for_user(self.student, self.course.id) return submissions_api.get_scores(unicode(self.course.id), anonymous_user_id) - def _get_saved_grade(self, subsection, block_structure): # pylint: disable=unused-argument + def _get_bulk_cached_grade(self, subsection, block_structure): # pylint: disable=unused-argument """ - Returns the saved grade for the student and subsection. + Returns the student's SubsectionGrade for the subsection, + while caching the results of a bulk retrieval for the + course, for future access of other subsections. + Returns None if not found. """ if not PersistentGradesEnabledFlag.feature_enabled(self.course.id): return - saved_subsection_grade = self._get_saved_subsection_grade(subsection.location) - if saved_subsection_grade: - subsection_grade = SubsectionGrade(subsection, self.course) - subsection_grade.init_from_model( - self.student, saved_subsection_grade, block_structure, self._submissions_scores, self._csm_scores, + saved_subsection_grades = self._get_bulk_cached_subsection_grades() + subsection_grade = saved_subsection_grades.get(subsection.location) + if subsection_grade: + return SubsectionGrade(subsection, self.course).init_from_model( + self.student, subsection_grade, block_structure, self._submissions_scores, self._csm_scores, ) - return subsection_grade - def _get_saved_subsection_grade(self, subsection_usage_key): + def _get_bulk_cached_subsection_grades(self): """ - Returns the saved value of the subsection grade for - the given subsection usage key, caching the value. - Returns None if not found. + Returns and caches (for future access) the results of + a bulk retrieval of all subsection grades in the course. """ if self._cached_subsection_grades is None: self._cached_subsection_grades = { record.full_usage_key: record for record in PersistentSubsectionGrade.bulk_read_grades(self.student.id, self.course.id) } - return self._cached_subsection_grades.get(subsection_usage_key) + return self._cached_subsection_grades def _update_saved_subsection_grade(self, subsection_usage_key, subsection_model): """ diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 0b60a47fe0..00c090b2da 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -5,12 +5,15 @@ Grades related signals. from django.dispatch import receiver from logging import getLogger +from courseware.model_data import get_score, set_score +from openedx.core.lib.grade_utils import is_score_higher from student.models import user_by_anonymous_id from submissions.models import score_set, score_reset -from .signals import SCORE_CHANGED +from .signals import SCORE_CHANGED, SCORE_PUBLISHED from ..tasks import recalculate_subsection_grade + log = getLogger(__name__) @@ -40,8 +43,8 @@ def submissions_score_set_handler(sender, **kwargs): # pylint: disable=unused-a SCORE_CHANGED.send( sender=None, - points_possible=points_possible, points_earned=points_earned, + points_possible=points_possible, user_id=user.id, course_id=course_id, usage_id=usage_id @@ -70,17 +73,61 @@ def submissions_score_reset_handler(sender, **kwargs): # pylint: disable=unused SCORE_CHANGED.send( sender=None, - points_possible=0, points_earned=0, + points_possible=0, user_id=user.id, course_id=course_id, usage_id=usage_id ) +@receiver(SCORE_PUBLISHED) +def score_published_handler(sender, block, user, raw_earned, raw_possible, only_if_higher, **kwargs): # pylint: disable=unused-argument + """ + Handles whenever a block's score is published. + Returns whether the score was actually updated. + """ + update_score = True + if only_if_higher: + previous_score = get_score(user.id, block.location) + + if previous_score: + prev_raw_earned, prev_raw_possible = previous_score # pylint: disable=unpacking-non-sequence + + if not is_score_higher(prev_raw_earned, prev_raw_possible, raw_earned, raw_possible): + update_score = False + log.warning( + u"Grades: Rescore is not higher than previous: " + u"user: {}, block: {}, previous: {}/{}, new: {}/{} ".format( + user, block.location, prev_raw_earned, prev_raw_possible, raw_earned, raw_possible, + ) + ) + + if update_score: + set_score(user.id, block.location, raw_earned, raw_possible) + + SCORE_CHANGED.send( + sender=None, + points_earned=raw_earned, + points_possible=raw_possible, + user_id=user.id, + course_id=unicode(block.location.course_key), + usage_id=unicode(block.location), + only_if_higher=only_if_higher, + ) + return update_score + + @receiver(SCORE_CHANGED) -def enqueue_update(sender, **kwargs): # pylint: disable=unused-argument +def enqueue_grade_update(sender, **kwargs): # pylint: disable=unused-argument """ Handles the SCORE_CHANGED signal by enqueueing an update operation to occur asynchronously. """ - recalculate_subsection_grade.apply_async(args=(kwargs['user_id'], kwargs['course_id'], kwargs['usage_id'])) + recalculate_subsection_grade.apply_async( + args=( + kwargs['user_id'], + kwargs['course_id'], + kwargs['usage_id'], + kwargs.get('only_if_higher'), + ) + ) diff --git a/lms/djangoapps/grades/signals/signals.py b/lms/djangoapps/grades/signals/signals.py index 250b27e9ab..78a594921a 100644 --- a/lms/djangoapps/grades/signals/signals.py +++ b/lms/djangoapps/grades/signals/signals.py @@ -12,10 +12,24 @@ from django.dispatch import Signal # receives the same score). SCORE_CHANGED = Signal( providing_args=[ - 'points_possible', # Maximum score available for the exercise - 'points_earned', # Score obtained by the user 'user_id', # Integer User ID 'course_id', # Unicode string representing the course - 'usage_id' # Unicode string indicating the courseware instance + 'usage_id', # Unicode string indicating the courseware instance + 'points_earned', # Score obtained by the user + 'points_possible', # Maximum score available for the exercise + 'only_if_higher', # Boolean indicating whether updates should be + # made only if the new score is higher than previous. + ] +) + + +SCORE_PUBLISHED = Signal( + providing_args=[ + 'block', # Course block object + 'user', # User object + 'raw_earned', # Score obtained by the user + 'raw_possible', # Maximum score available for the exercise + 'only_if_higher', # Boolean indicating whether updates should be + # made only if the new score is higher than previous. ] ) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index 95f774b643..942cff4175 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/lms/djangoapps/grades/tasks.py @@ -8,10 +8,10 @@ from django.contrib.auth.models import User from django.db.utils import IntegrityError from lms.djangoapps.course_blocks.api import get_course_blocks -from lms.djangoapps.courseware.courses import get_course_by_id from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import CourseLocator from openedx.core.djangoapps.content.block_structure.api import get_course_in_cache +from xmodule.modulestore.django import modulestore from .config.models import PersistentGradesEnabledFlag from .transformer import GradesTransformer @@ -19,13 +19,16 @@ from .new.subsection_grade import SubsectionGradeFactory @task(default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY) -def recalculate_subsection_grade(user_id, course_id, usage_id): +def recalculate_subsection_grade(user_id, course_id, usage_id, only_if_higher): """ Updates a saved subsection grade. This method expects the following parameters: - user_id: serialized id of applicable User object - course_id: Unicode string representing the course - usage_id: Unicode string indicating the courseware instance + - only_if_higher: boolean indicating whether grades should + be updated only if the new grade is higher than the previous + value. """ course_key = CourseLocator.from_string(course_id) if not PersistentGradesEnabledFlag.feature_enabled(course_key): @@ -35,7 +38,7 @@ def recalculate_subsection_grade(user_id, course_id, usage_id): scored_block_usage_key = UsageKey.from_string(usage_id).replace(course_key=course_key) collected_block_structure = get_course_in_cache(course_key) - course = get_course_by_id(course_key, depth=0) + course = modulestore().get_course(course_key, depth=0) subsection_grade_factory = SubsectionGradeFactory(student, course, collected_block_structure) subsections_to_update = collected_block_structure.get_transformer_block_field( scored_block_usage_key, @@ -52,7 +55,9 @@ def recalculate_subsection_grade(user_id, course_id, usage_id): collected_block_structure=collected_block_structure, ) subsection_grade_factory.update( - transformed_subsection_structure[subsection_usage_key], transformed_subsection_structure + transformed_subsection_structure[subsection_usage_key], + transformed_subsection_structure, + only_if_higher, ) except IntegrityError as exc: raise recalculate_subsection_grade.retry(args=[user_id, course_id, usage_id], exc=exc) diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 372d940e31..e5b4399ae3 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -106,7 +106,7 @@ class TestCourseGradeFactory(GradeTestBase): @ddt.ddt -class SubsectionGradeFactoryTest(GradeTestBase): +class TestSubsectionGradeFactory(GradeTestBase, ProblemSubmissionTestMixin): """ Tests for SubsectionGradeFactory functionality. @@ -115,17 +115,36 @@ class SubsectionGradeFactoryTest(GradeTestBase): enable saving subsection grades blocks/enables that feature as expected. """ + def assert_grade(self, grade, expected_earned, expected_possible): + """ + Asserts that the given grade object has the expected score. + """ + self.assertEqual( + (grade.all_total.earned, grade.all_total.possible), + (expected_earned, expected_possible), + ) + def test_create(self): """ - Tests to ensure that a persistent subsection grade is created, saved, then fetched on re-request. + Assuming the underlying score reporting methods work, + test that the score is calculated properly. + """ + with mock_get_score(1, 2): + grade = self.subsection_grade_factory.create(self.sequence) + self.assert_grade(grade, 1, 2) + + def test_create_internals(self): + """ + Tests to ensure that a persistent subsection grade is + created, saved, then fetched on re-request. """ with patch( 'lms.djangoapps.grades.new.subsection_grade.PersistentSubsectionGrade.create_grade', wraps=PersistentSubsectionGrade.create_grade ) as mock_create_grade: with patch( - 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', - wraps=self.subsection_grade_factory._get_saved_grade + 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_bulk_cached_grade', + wraps=self.subsection_grade_factory._get_bulk_cached_grade ) as mock_get_saved_grade: with self.assertNumQueries(14): grade_a = self.subsection_grade_factory.create(self.sequence) @@ -143,6 +162,30 @@ class SubsectionGradeFactoryTest(GradeTestBase): self.assertEqual(grade_a.url_name, grade_b.url_name) self.assertEqual(grade_a.all_total, grade_b.all_total) + def test_update(self): + """ + Assuming the underlying score reporting methods work, + test that the score is calculated properly. + """ + with mock_get_score(1, 2): + grade = self.subsection_grade_factory.update(self.sequence) + self.assert_grade(grade, 1, 2) + + def test_update_if_higher(self): + def verify_update_if_higher(mock_score, expected_grade): + """ + Updates the subsection grade and verifies the + resulting grade is as expected. + """ + with mock_get_score(*mock_score): + grade = self.subsection_grade_factory.update(self.sequence, only_if_higher=True) + self.assert_grade(grade, *expected_grade) + + verify_update_if_higher((1, 2), (1, 2)) # previous value was non-existent + verify_update_if_higher((2, 4), (1, 2)) # previous value was equivalent + verify_update_if_higher((1, 4), (1, 2)) # previous value was greater + verify_update_if_higher((3, 4), (3, 4)) # previous value was less + @ddt.data( ( 'lms.djangoapps.grades.new.subsection_grade.SubsectionGrade.create_model', @@ -162,7 +205,8 @@ class SubsectionGradeFactoryTest(GradeTestBase): with patch(underlying_method) as underlying: underlying.side_effect = DatabaseError("I'm afraid I can't do that") method_to_test(self) - # By making it this far, we implicitly assert "the factory method swallowed the exception correctly" + # By making it this far, we implicitly assert + # "the factory method swallowed the exception correctly" self.assertTrue( log_mock.warning.call_args_list[0].startswith("Persistent Grades: Persistence Error, falling back.") ) @@ -196,18 +240,10 @@ class SubsectionGradeTest(GradeTestBase): Tests SubsectionGrade functionality. """ - def test_compute(self): - """ - Assuming the underlying score reporting methods work, test that the score is calculated properly. - """ - with mock_get_score(1, 2): - grade = self.subsection_grade_factory.create(self.sequence) - self.assertEqual(grade.all_total.earned, 1) - self.assertEqual(grade.all_total.possible, 2) - def test_save_and_load(self): """ - Test that grades are persisted to the database properly, and that loading saved grades returns the same data. + Test that grades are persisted to the database properly, + and that loading saved grades returns the same data. """ # Create a grade that *isn't* saved to the database input_grade = SubsectionGrade(self.sequence, self.course) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 48b439d838..512a7f9e20 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -2,6 +2,7 @@ Tests for the functionality and infrastructure of grades tasks. """ +from collections import OrderedDict import ddt from django.conf import settings from django.db.utils import IntegrityError @@ -47,11 +48,12 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Open Sequential") self.problem = ItemFactory.create(parent=self.sequential, category='problem', display_name='problem') - self.score_changed_kwargs = { - 'user_id': self.user.id, - 'course_id': unicode(self.course.id), - 'usage_id': unicode(self.problem.location), - } + self.score_changed_kwargs = OrderedDict([ + ('user_id', self.user.id), + ('course_id', unicode(self.course.id)), + ('usage_id', unicode(self.problem.location)), + ('only_if_higher', None), + ]) # this call caches the anonymous id on the user object, saving 4 queries in all happy path tests _ = anonymous_id_for_user(self.user, self.course.id) @@ -67,13 +69,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): return_value=None ) as mock_task_apply: SCORE_CHANGED.send(sender=None, **self.score_changed_kwargs) - mock_task_apply.assert_called_once_with( - args=( - self.score_changed_kwargs['user_id'], - self.score_changed_kwargs['course_id'], - self.score_changed_kwargs['usage_id'], - ) - ) + mock_task_apply.assert_called_once_with(args=tuple(self.score_changed_kwargs.values())) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_subsection_grade_updated(self, default_store): @@ -81,13 +77,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): self.set_up_course() self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) with check_mongo_calls(2) and self.assertNumQueries(13): - recalculate_subsection_grade.apply( - args=( - self.score_changed_kwargs['user_id'], - self.score_changed_kwargs['course_id'], - self.score_changed_kwargs['usage_id'], - ) - ) + recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values())) def test_single_call_to_create_block_structure(self): self.set_up_course() @@ -96,13 +86,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): 'openedx.core.lib.block_structure.factory.BlockStructureFactory.create_from_cache', return_value=None, ) as mock_block_structure_create: - recalculate_subsection_grade.apply( - args=( - self.score_changed_kwargs['user_id'], - self.score_changed_kwargs['course_id'], - self.score_changed_kwargs['usage_id'], - ) - ) + recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values())) self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) @@ -113,13 +97,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2') ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3') with check_mongo_calls(2) and self.assertNumQueries(13): - recalculate_subsection_grade.apply( - args=( - self.score_changed_kwargs['user_id'], - self.score_changed_kwargs['course_id'], - self.score_changed_kwargs['usage_id'], - ) - ) + recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values())) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_subsection_grades_not_enabled_on_course(self, default_store): @@ -127,13 +105,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): self.set_up_course(enable_subsection_grades=False) self.assertFalse(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) with check_mongo_calls(2) and self.assertNumQueries(0): - recalculate_subsection_grade.apply( - args=( - self.score_changed_kwargs['user_id'], - self.score_changed_kwargs['course_id'], - self.score_changed_kwargs['usage_id'], - ) - ) + recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values())) @skip("Pending completion of TNL-5089") @ddt.data( @@ -148,13 +120,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): with self.store.default_store(default_store): self.set_up_course() with check_mongo_calls(0) and self.assertNumQueries(3 if feature_flag else 2): - recalculate_subsection_grade.apply( - args=( - self.score_changed_kwargs['user_id'], - self.score_changed_kwargs['course_id'], - self.score_changed_kwargs['usage_id'], - ) - ) + recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values())) @patch('lms.djangoapps.grades.tasks.recalculate_subsection_grade.retry') @patch('lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory.update') @@ -164,11 +130,5 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): """ self.set_up_course() mock_update.side_effect = IntegrityError("WHAMMY") - recalculate_subsection_grade.apply( - args=( - self.score_changed_kwargs['user_id'], - self.score_changed_kwargs['course_id'], - self.score_changed_kwargs['usage_id'], - ) - ) + recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values())) self.assertTrue(mock_retry.called) diff --git a/lms/djangoapps/grades/transformer.py b/lms/djangoapps/grades/transformer.py index 40442502ee..cacaf0b78d 100644 --- a/lms/djangoapps/grades/transformer.py +++ b/lms/djangoapps/grades/transformer.py @@ -9,7 +9,7 @@ from logging import getLogger import json from courseware.model_data import FieldDataCache -from courseware.module_render import get_module_for_descriptor +import courseware.module_render from lms.djangoapps.course_blocks.transformers.utils import collect_unioned_set_field, get_field_on_block from openedx.core.lib.block_structure.transformer import BlockStructureTransformer from openedx.core.djangoapps.util.user_utils import SystemUser @@ -186,5 +186,5 @@ class GradesTransformer(BlockStructureTransformer): for block_locator in block_structure.post_order_traversal(): block = block_structure.get_xblock(block_locator) if getattr(block, 'has_score', False): - module = get_module_for_descriptor(user, request, block, cache, course_key) + module = courseware.module_render.get_module_for_descriptor(user, request, block, cache, course_key) yield module diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index c8feee3ed5..660612b419 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -3190,7 +3190,7 @@ class TestInstructorAPIRegradeTask(SharedModuleStoreTestCase, LoginEnrollmentTes }) self.assertEqual(response.status_code, 400) - @patch('courseware.module_render.SCORE_CHANGED.send') + @patch('lms.djangoapps.grades.signals.handlers.SCORE_CHANGED.send') def test_reset_student_attempts_delete(self, _mock_signal): """ Test delete single student state. """ url = reverse('reset_student_attempts', kwargs={'course_id': self.course.id.to_deprecated_string()}) @@ -3469,6 +3469,15 @@ class TestEntranceExamInstructorAPIRegradeTask(SharedModuleStoreTestCase, LoginE }) self.assertEqual(response.status_code, 200) + def test_rescore_entrance_exam_if_higher_all_student(self): + """ Test rescoring for all students only if higher. """ + url = reverse('rescore_entrance_exam', kwargs={'course_id': unicode(self.course.id)}) + response = self.client.post(url, { + 'all_students': True, + 'only_if_higher': True, + }) + self.assertEqual(response.status_code, 200) + def test_rescore_entrance_exam_all_student_and_single(self): """ Test re-scoring with both all students and single student parameters. """ url = reverse('rescore_entrance_exam', kwargs={'course_id': unicode(self.course.id)}) diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 65ec2af976..779dd54ae7 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -378,7 +378,7 @@ class TestInstructorEnrollmentStudentModule(SharedModuleStoreTestCase): reset_student_attempts(self.course_key, self.user, msk, requesting_user=self.user) self.assertEqual(json.loads(module().state)['attempts'], 0) - @mock.patch('courseware.module_render.SCORE_CHANGED.send') + @mock.patch('lms.djangoapps.grades.signals.handlers.SCORE_CHANGED.send') def test_delete_student_attempts(self, _mock_signal): msk = self.course_key.make_usage_key('dummy', 'module') original_state = json.dumps({'attempts': 32, 'otherstuff': 'alsorobots'}) @@ -404,7 +404,7 @@ class TestInstructorEnrollmentStudentModule(SharedModuleStoreTestCase): # Disable the score change signal to prevent other components from being # pulled into tests. - @mock.patch('courseware.module_render.SCORE_CHANGED.send') + @mock.patch('lms.djangoapps.grades.signals.handlers.SCORE_CHANGED.send') def test_delete_submission_scores(self, _mock_signal): user = UserFactory() problem_location = self.course_key.make_usage_key('dummy', 'module') diff --git a/lms/djangoapps/instructor/tests/test_services.py b/lms/djangoapps/instructor/tests/test_services.py index 4e2ded21fc..4cc8ab504c 100644 --- a/lms/djangoapps/instructor/tests/test_services.py +++ b/lms/djangoapps/instructor/tests/test_services.py @@ -49,7 +49,7 @@ class InstructorServiceTests(SharedModuleStoreTestCase): state=json.dumps({'attempts': 2}), ) - @mock.patch('courseware.module_render.SCORE_CHANGED.send') + @mock.patch('lms.djangoapps.grades.signals.handlers.SCORE_CHANGED.send') def test_reset_student_attempts_delete(self, _mock_signal): """ Test delete student state. diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 42e1a8055c..8b27e24b49 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -602,8 +602,8 @@ def students_update_enrollment(request, course_id): action = request.POST.get('action') identifiers_raw = request.POST.get('identifiers') identifiers = _split_input_list(identifiers_raw) - auto_enroll = request.POST.get('auto_enroll') in ['true', 'True', True] - email_students = request.POST.get('email_students') in ['true', 'True', True] + auto_enroll = _get_boolean_param(request, 'auto_enroll') + email_students = _get_boolean_param(request, 'email_students') is_white_label = CourseMode.is_white_label(course_id) reason = request.POST.get('reason') if is_white_label: @@ -743,8 +743,8 @@ def bulk_beta_modify_access(request, course_id): action = request.POST.get('action') identifiers_raw = request.POST.get('identifiers') identifiers = _split_input_list(identifiers_raw) - email_students = request.POST.get('email_students') in ['true', 'True', True] - auto_enroll = request.POST.get('auto_enroll') in ['true', 'True', True] + email_students = _get_boolean_param(request, 'email_students') + auto_enroll = _get_boolean_param(request, 'auto_enroll') results = [] rolename = 'beta' course = get_course_by_id(course_id) @@ -1939,8 +1939,8 @@ def reset_student_attempts(request, course_id): student = None if student_identifier is not None: student = get_student_from_identifier(student_identifier) - all_students = request.POST.get('all_students', False) in ['true', 'True', True] - delete_module = request.POST.get('delete_module', False) in ['true', 'True', True] + all_students = _get_boolean_param(request, 'all_students') + delete_module = _get_boolean_param(request, 'delete_module') # parameter combinations if all_students and student: @@ -2027,8 +2027,8 @@ def reset_student_attempts_for_entrance_exam(request, course_id): # pylint: dis student = None if student_identifier is not None: student = get_student_from_identifier(student_identifier) - all_students = request.POST.get('all_students', False) in ['true', 'True', True] - delete_module = request.POST.get('delete_module', False) in ['true', 'True', True] + all_students = _get_boolean_param(request, 'all_students') + delete_module = _get_boolean_param(request, 'delete_module') # parameter combinations if all_students and student: @@ -2092,7 +2092,8 @@ def rescore_problem(request, course_id): if student_identifier is not None: student = get_student_from_identifier(student_identifier) - all_students = request.POST.get('all_students') in ['true', 'True', True] + all_students = _get_boolean_param(request, 'all_students') + only_if_higher = _get_boolean_param(request, 'only_if_higher') if not (problem_to_reset and (all_students or student)): return HttpResponseBadRequest("Missing query parameters.") @@ -2107,19 +2108,26 @@ def rescore_problem(request, course_id): except InvalidKeyError: return HttpResponseBadRequest("Unable to parse problem id") - response_payload = {} - response_payload['problem_to_reset'] = problem_to_reset + response_payload = {'problem_to_reset': problem_to_reset} if student: response_payload['student'] = student_identifier - lms.djangoapps.instructor_task.api.submit_rescore_problem_for_student(request, module_state_key, student) - response_payload['task'] = 'created' + lms.djangoapps.instructor_task.api.submit_rescore_problem_for_student( + request, + module_state_key, + student, + only_if_higher, + ) elif all_students: - lms.djangoapps.instructor_task.api.submit_rescore_problem_for_all_students(request, module_state_key) - response_payload['task'] = 'created' + lms.djangoapps.instructor_task.api.submit_rescore_problem_for_all_students( + request, + module_state_key, + only_if_higher, + ) else: return HttpResponseBadRequest() + response_payload['task'] = 'created' return JsonResponse(response_payload) @@ -2146,11 +2154,12 @@ def rescore_entrance_exam(request, course_id): ) student_identifier = request.POST.get('unique_student_identifier', None) + only_if_higher = request.POST.get('only_if_higher', None) student = None if student_identifier is not None: student = get_student_from_identifier(student_identifier) - all_students = request.POST.get('all_students') in ['true', 'True', True] + all_students = _get_boolean_param(request, 'all_students') if not course.entrance_exam_id: return HttpResponseBadRequest( @@ -2172,7 +2181,10 @@ def rescore_entrance_exam(request, course_id): response_payload['student'] = student_identifier else: response_payload['student'] = _("All Students") - lms.djangoapps.instructor_task.api.submit_rescore_entrance_exam_for_student(request, entrance_exam_key, student) + + lms.djangoapps.instructor_task.api.submit_rescore_entrance_exam_for_student( + request, entrance_exam_key, student, only_if_higher, + ) response_payload['task'] = 'created' return JsonResponse(response_payload) @@ -3318,3 +3330,12 @@ def validate_request_data_and_get_certificate(certificate_invalidation, course_k "username/email and the selected course are correct and try again." ).format(student=student.username, course=course_key.course)) return certificate + + +def _get_boolean_param(request, param_name): + """ + Returns the value of the boolean parameter with the given + name in the POST request. Handles translation from string + values to boolean values. + """ + return request.POST.get(param_name, False) in ['true', 'True', True] diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 3a2cd1c73c..a412daf3b9 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -95,7 +95,7 @@ def get_entrance_exam_instructor_task_history(course_id, usage_key=None, student # Disabling invalid-name because this fn name is longer than 30 chars. -def submit_rescore_problem_for_student(request, usage_key, student): # pylint: disable=invalid-name +def submit_rescore_problem_for_student(request, usage_key, student, only_if_higher=False): # pylint: disable=invalid-name """ Request a problem to be rescored as a background task. @@ -110,13 +110,14 @@ def submit_rescore_problem_for_student(request, usage_key, student): # pylint: # check arguments: let exceptions return up to the caller. check_arguments_for_rescoring(usage_key) - task_type = 'rescore_problem' + task_type = 'rescore_problem_if_higher' if only_if_higher else 'rescore_problem' task_class = rescore_problem task_input, task_key = encode_problem_and_student_input(usage_key, student) + task_input.update({'only_if_higher': only_if_higher}) return submit_task(request, task_type, task_class, usage_key.course_key, task_input, task_key) -def submit_rescore_problem_for_all_students(request, usage_key): # pylint: disable=invalid-name +def submit_rescore_problem_for_all_students(request, usage_key, only_if_higher=False): # pylint: disable=invalid-name """ Request a problem to be rescored as a background task. @@ -136,10 +137,11 @@ def submit_rescore_problem_for_all_students(request, usage_key): # pylint: disa task_type = 'rescore_problem' task_class = rescore_problem task_input, task_key = encode_problem_and_student_input(usage_key) + task_input.update({'only_if_higher': only_if_higher}) return submit_task(request, task_type, task_class, usage_key.course_key, task_input, task_key) -def submit_rescore_entrance_exam_for_student(request, usage_key, student=None): # pylint: disable=invalid-name +def submit_rescore_entrance_exam_for_student(request, usage_key, student=None, only_if_higher=False): # pylint: disable=invalid-name """ Request entrance exam problems to be re-scored as a background task. @@ -161,6 +163,7 @@ def submit_rescore_entrance_exam_for_student(request, usage_key, student=None): task_type = 'rescore_problem' task_class = rescore_problem task_input, task_key = encode_entrance_exam_and_student_input(usage_key, student) + task_input.update({'only_if_higher': only_if_higher}) return submit_task(request, task_type, task_class, usage_key.course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index fa0de7a7f8..bd05443d01 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -310,9 +310,9 @@ def perform_module_state_update(update_fcn, filter_fcn, _entry_id, course_id, ta argument, which is the query being filtered, and returns the filtered version of the query. The `update_fcn` is called on each StudentModule that passes the resulting filtering. - It is passed three arguments: the module_descriptor for the module pointed to by the - module_state_key, the particular StudentModule to update, and the xmodule_instance_args being - passed through. If the value returned by the update function evaluates to a boolean True, + It is passed four arguments: the module_descriptor for the module pointed to by the + module_state_key, the particular StudentModule to update, the xmodule_instance_args, and the task_input + being passed through. If the value returned by the update function evaluates to a boolean True, the update is successful; False indicates the update on the particular student module failed. A raised exception indicates a fatal condition -- that no other student modules should be considered. @@ -382,7 +382,7 @@ def perform_module_state_update(update_fcn, filter_fcn, _entry_id, course_id, ta # There is no try here: if there's an error, we let it throw, and the task will # be marked as FAILED, with a stack trace. with dog_stats_api.timer('instructor_tasks.module.time.step', tags=[u'action:{name}'.format(name=action_name)]): - update_status = update_fcn(module_descriptor, module_to_update) + update_status = update_fcn(module_descriptor, module_to_update, task_input) if update_status == UPDATE_STATUS_SUCCEEDED: # If the update_fcn returns true, then it performed some kind of work. # Logging of failures is left to the update_fcn itself. @@ -470,7 +470,7 @@ def _get_module_instance_for_task(course_id, student, module_descriptor, xmodule @outer_atomic -def rescore_problem_module_state(xmodule_instance_args, module_descriptor, student_module): +def rescore_problem_module_state(xmodule_instance_args, module_descriptor, student_module, task_input): ''' Takes an XModule descriptor and a corresponding StudentModule object, and performs rescoring on the student's problem submission. @@ -517,7 +517,7 @@ def rescore_problem_module_state(xmodule_instance_args, module_descriptor, stude msg = "Specified problem does not support rescoring." raise UpdateProblemModuleStateError(msg) - result = instance.rescore_problem() + result = instance.rescore_problem(only_if_higher=task_input['only_if_higher']) instance.save() if 'success' not in result: # don't consider these fatal, but false means that the individual call didn't complete: @@ -559,7 +559,7 @@ def rescore_problem_module_state(xmodule_instance_args, module_descriptor, stude @outer_atomic -def reset_attempts_module_state(xmodule_instance_args, _module_descriptor, student_module): +def reset_attempts_module_state(xmodule_instance_args, _module_descriptor, student_module, _task_input): """ Resets problem attempts to zero for specified `student_module`. @@ -586,7 +586,7 @@ def reset_attempts_module_state(xmodule_instance_args, _module_descriptor, stude @outer_atomic -def delete_problem_module_state(xmodule_instance_args, _module_descriptor, student_module): +def delete_problem_module_state(xmodule_instance_args, _module_descriptor, student_module, _task_input): """ Delete the StudentModule entry. diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index a64ac4b907..e22c3ba4cd 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -140,35 +140,27 @@ class TestRescoringTask(TestIntegrationTask): expected_subsection_grade, ) - def submit_rescore_all_student_answers(self, instructor, problem_url_name): + def submit_rescore_all_student_answers(self, instructor, problem_url_name, only_if_higher=False): """Submits the particular problem for rescoring""" - return submit_rescore_problem_for_all_students(self.create_task_request(instructor), - InstructorTaskModuleTestCase.problem_location(problem_url_name)) + return submit_rescore_problem_for_all_students( + self.create_task_request(instructor), + InstructorTaskModuleTestCase.problem_location(problem_url_name), + only_if_higher, + ) - def submit_rescore_one_student_answer(self, instructor, problem_url_name, student): + def submit_rescore_one_student_answer(self, instructor, problem_url_name, student, only_if_higher=False): """Submits the particular problem for rescoring for a particular student""" - return submit_rescore_problem_for_student(self.create_task_request(instructor), - InstructorTaskModuleTestCase.problem_location(problem_url_name), - student) + return submit_rescore_problem_for_student( + self.create_task_request(instructor), + InstructorTaskModuleTestCase.problem_location(problem_url_name), + student, + only_if_higher, + ) - RescoreTestData = namedtuple('RescoreTestData', 'edit, new_expected_scores, new_expected_max') - - @ddt.data( - RescoreTestData(edit=dict(correct_answer=OPTION_2), new_expected_scores=(0, 1, 1, 2), new_expected_max=2), - RescoreTestData(edit=dict(num_inputs=2), new_expected_scores=(2, 1, 1, 0), new_expected_max=4), - RescoreTestData(edit=dict(num_inputs=4), new_expected_scores=(2, 1, 1, 0), new_expected_max=8), - RescoreTestData(edit=dict(num_responses=4), new_expected_scores=(2, 1, 1, 0), new_expected_max=4), - RescoreTestData(edit=dict(num_inputs=2, num_responses=4), new_expected_scores=(2, 1, 1, 0), new_expected_max=8), - ) - @ddt.unpack - def test_rescoring_option_problem(self, problem_edit, new_expected_scores, new_expected_max): + def verify_rescore_results(self, problem_edit, new_expected_scores, new_expected_max, rescore_if_higher): """ - Run rescore scenario on option problem. - Verify rescoring updates grade after content change. - Original problem definition has: - num_inputs = 1 - num_responses = 2 - correct_answer = OPTION_1 + Common helper to verify the results of rescoring for a single + student and all students are as expected. """ # get descriptor: problem_url_name = 'H1P1' @@ -196,16 +188,50 @@ class TestRescoringTask(TestIntegrationTask): self.check_state(self.user1, descriptor, expected_original_scores[0], expected_original_max) # rescore the problem for only one student -- only that student's grade should change: - self.submit_rescore_one_student_answer('instructor', problem_url_name, self.user1) + self.submit_rescore_one_student_answer('instructor', problem_url_name, self.user1, rescore_if_higher) self.check_state(self.user1, descriptor, new_expected_scores[0], new_expected_max) for i, user in enumerate(self.users[1:], start=1): # everyone other than user1 self.check_state(user, descriptor, expected_original_scores[i], expected_original_max) # rescore the problem for all students - self.submit_rescore_all_student_answers('instructor', problem_url_name) + self.submit_rescore_all_student_answers('instructor', problem_url_name, rescore_if_higher) for i, user in enumerate(self.users): self.check_state(user, descriptor, new_expected_scores[i], new_expected_max) + RescoreTestData = namedtuple('RescoreTestData', 'edit, new_expected_scores, new_expected_max') + + @ddt.data( + RescoreTestData(edit=dict(correct_answer=OPTION_2), new_expected_scores=(0, 1, 1, 2), new_expected_max=2), + RescoreTestData(edit=dict(num_inputs=2), new_expected_scores=(2, 1, 1, 0), new_expected_max=4), + RescoreTestData(edit=dict(num_inputs=4), new_expected_scores=(2, 1, 1, 0), new_expected_max=8), + RescoreTestData(edit=dict(num_responses=4), new_expected_scores=(2, 1, 1, 0), new_expected_max=4), + RescoreTestData(edit=dict(num_inputs=2, num_responses=4), new_expected_scores=(2, 1, 1, 0), new_expected_max=8), + ) + @ddt.unpack + def test_rescoring_option_problem(self, problem_edit, new_expected_scores, new_expected_max): + """ + Run rescore scenario on option problem. + Verify rescoring updates grade after content change. + Original problem definition has: + num_inputs = 1 + num_responses = 2 + correct_answer = OPTION_1 + """ + self.verify_rescore_results( + problem_edit, new_expected_scores, new_expected_max, rescore_if_higher=False, + ) + + @ddt.data( + RescoreTestData(edit=dict(), new_expected_scores=(2, 1, 1, 0), new_expected_max=2), + RescoreTestData(edit=dict(correct_answer=OPTION_2), new_expected_scores=(2, 1, 1, 2), new_expected_max=2), + RescoreTestData(edit=dict(num_inputs=2), new_expected_scores=(2, 1, 1, 0), new_expected_max=2), + ) + @ddt.unpack + def test_rescoring_if_higher(self, problem_edit, new_expected_scores, new_expected_max): + self.verify_rescore_results( + problem_edit, new_expected_scores, new_expected_max, rescore_if_higher=True, + ) + def test_rescoring_failure(self): """Simulate a failure in rescoring a problem""" problem_url_name = 'H1P1' diff --git a/lms/djangoapps/instructor_task/tests/test_tasks.py b/lms/djangoapps/instructor_task/tests/test_tasks.py index fd24cf8921..c6e2a35024 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks.py @@ -52,10 +52,10 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self.instructor = self.create_instructor('instructor') self.location = self.problem_location(PROBLEM_URL_NAME) - def _create_input_entry(self, student_ident=None, use_problem_url=True, course_id=None): + def _create_input_entry(self, student_ident=None, use_problem_url=True, course_id=None, only_if_higher=False): """Creates a InstructorTask entry for testing.""" task_id = str(uuid4()) - task_input = {} + task_input = {'only_if_higher': only_if_higher} if use_problem_url: task_input['problem_url'] = self.location if student_ident is not None: diff --git a/lms/static/js/instructor_dashboard/student_admin.js b/lms/static/js/instructor_dashboard/student_admin.js index 9c65b24380..a2bc1ee0b2 100644 --- a/lms/static/js/instructor_dashboard/student_admin.js +++ b/lms/static/js/instructor_dashboard/student_admin.js @@ -39,18 +39,25 @@ this.$btn_reset_attempts_single = findAndAssert(this.$section, "input[name='reset-attempts-single']"); this.$btn_delete_state_single = this.$section.find("input[name='delete-state-single']"); this.$btn_rescore_problem_single = this.$section.find("input[name='rescore-problem-single']"); + this.$btn_rescore_problem_if_higher_single = this.$section.find( + "input[name='rescore-problem-if-higher-single']" + ); this.$btn_task_history_single = this.$section.find("input[name='task-history-single']"); this.$table_task_history_single = this.$section.find('.task-history-single-table'); this.$field_exam_grade = this.$section.find("input[name='entrance-exam-student-select-grade']"); this.$btn_reset_entrance_exam_attempts = this.$section.find("input[name='reset-entrance-exam-attempts']"); this.$btn_delete_entrance_exam_state = this.$section.find("input[name='delete-entrance-exam-state']"); this.$btn_rescore_entrance_exam = this.$section.find("input[name='rescore-entrance-exam']"); + this.$btn_rescore_entrance_exam_if_higher = this.$section.find( + "input[name='rescore-entrance-exam-if-higher']" + ); this.$btn_skip_entrance_exam = this.$section.find("input[name='skip-entrance-exam']"); this.$btn_entrance_exam_task_history = this.$section.find("input[name='entrance-exam-task-history']"); this.$table_entrance_exam_task_history = this.$section.find('.entrance-exam-task-history-table'); this.$field_problem_select_all = this.$section.find("input[name='problem-select-all']"); this.$btn_reset_attempts_all = this.$section.find("input[name='reset-attempts-all']"); this.$btn_rescore_problem_all = this.$section.find("input[name='rescore-problem-all']"); + this.$btn_rescore_problem_if_higher_all = this.$section.find("input[name='rescore-problem-all-if-higher']"); this.$btn_task_history_all = this.$section.find("input[name='task-history-all']"); this.$table_task_history_all = this.$section.find('.task-history-all-table'); this.instructor_tasks = new (PendingInstructorTasks())(this.$section); @@ -176,46 +183,10 @@ } }); this.$btn_rescore_problem_single.click(function() { - var errorMessage, fullErrorMessage, fullSuccessMessage, - problemToReset, sendData, successMessage, uniqStudentIdentifier; - uniqStudentIdentifier = studentadmin.$field_student_select_grade.val(); - problemToReset = studentadmin.$field_problem_select_single.val(); - if (!uniqStudentIdentifier) { - return studentadmin.$request_err_grade.text( - gettext('Please enter a student email address or username.') - ); - } - if (!problemToReset) { - return studentadmin.$request_err_grade.text( - gettext('Please enter a problem location.') - ); - } - sendData = { - unique_student_identifier: uniqStudentIdentifier, - problem_to_reset: problemToReset - }; - successMessage = gettext("Started rescore problem task for problem '<%- problem_id %>' and student '<%- student_id %>'. Click the 'Show Background Task History for Student' button to see the status of the task."); // eslint-disable-line max-len - fullSuccessMessage = _.template(successMessage)({ - student_id: uniqStudentIdentifier, - problem_id: problemToReset - }); - errorMessage = gettext("Error starting a task to rescore problem '<%- problem_id %>' for student '<%- student_id %>'. Make sure that the the problem and student identifiers are complete and correct."); // eslint-disable-line max-len - fullErrorMessage = _.template(errorMessage)({ - student_id: uniqStudentIdentifier, - problem_id: problemToReset - }); - return $.ajax({ - type: 'POST', - dataType: 'json', - url: studentadmin.$btn_rescore_problem_single.data('endpoint'), - data: sendData, - success: studentadmin.clear_errors_then(function() { - return alert(fullSuccessMessage); // eslint-disable-line no-alert - }), - error: statusAjaxError(function() { - return studentadmin.$request_err_grade.text(fullErrorMessage); - }) - }); + return studentadmin.rescore_problem_single(false); + }); + this.$btn_rescore_problem_if_higher_single.click(function() { + return studentadmin.rescore_problem_single(true); }); this.$btn_task_history_single.click(function() { var errorMessage, fullErrorMessage, problemToReset, sendData, uniqStudentIdentifier; @@ -289,38 +260,10 @@ }); }); this.$btn_rescore_entrance_exam.click(function() { - var sendData, uniqStudentIdentifier; - uniqStudentIdentifier = studentadmin.$field_exam_grade.val(); - if (!uniqStudentIdentifier) { - return studentadmin.$request_err_ee.text(gettext( - 'Please enter a student email address or username.') - ); - } - sendData = { - unique_student_identifier: uniqStudentIdentifier - }; - return $.ajax({ - type: 'POST', - dataType: 'json', - url: studentadmin.$btn_rescore_entrance_exam.data('endpoint'), - data: sendData, - success: studentadmin.clear_errors_then(function() { - var fullSuccessMessage, successMessage; - successMessage = gettext("Started entrance exam rescore task for student '{student_id}'. Click the 'Show Background Task History for Student' button to see the status of the task."); // eslint-disable-line max-len - fullSuccessMessage = interpolate_text(successMessage, { - student_id: uniqStudentIdentifier - }); - return alert(fullSuccessMessage); // eslint-disable-line no-alert - }), - error: statusAjaxError(function() { - var errorMessage, fullErrorMessage; - errorMessage = gettext("Error starting a task to rescore entrance exam for student '{student_id}'. Make sure that entrance exam has problems in it and student identifier is correct."); // eslint-disable-line max-len - fullErrorMessage = interpolate_text(errorMessage, { - student_id: uniqStudentIdentifier - }); - return studentadmin.$request_err_ee.text(fullErrorMessage); - }) - }); + return studentadmin.rescore_entrance_exam_all(false); + }); + this.$btn_rescore_entrance_exam_if_higher.click(function() { + return studentadmin.rescore_entrance_exam_all(true); }); this.$btn_skip_entrance_exam.click(function() { var confirmMessage, fullConfirmMessage, sendData, uniqStudentIdentifier; @@ -435,7 +378,7 @@ all_students: true, problem_to_reset: problemToReset }; - successMessage = gettext("Successfully started task to reset attempts for problem '<%- problem_id %>'. Click the 'Show Background Task History for Problem' button to see the status of the task."); // eslint-disable-line max-len + successMessage = gettext("Successfully started task to reset attempts for problem '<%- problem_id %>'. Click the 'Show Task Status' button to see the status of the task."); // eslint-disable-line max-len fullSuccessMessage = _.template(successMessage)({ problem_id: problemToReset }); @@ -460,46 +403,10 @@ } }); this.$btn_rescore_problem_all.click(function() { - var confirmMessage, errorMessage, fullConfirmMessage, - fullErrorMessage, fullSuccessMessage, problemToReset, sendData, successMessage; - problemToReset = studentadmin.$field_problem_select_all.val(); - if (!problemToReset) { - return studentadmin.$request_response_error_all.text( - gettext('Please enter a problem location.') - ); - } - confirmMessage = gettext("Rescore problem '<%- problem_id %>' for all students?"); - fullConfirmMessage = _.template(confirmMessage)({ - problem_id: problemToReset - }); - if (window.confirm(fullConfirmMessage)) { // eslint-disable-line no-alert - sendData = { - all_students: true, - problem_to_reset: problemToReset - }; - successMessage = gettext("Successfully started task to rescore problem '<%- problem_id %>' for all students. Click the 'Show Background Task History for Problem' button to see the status of the task."); // eslint-disable-line max-len - fullSuccessMessage = _.template(successMessage)({ - problem_id: problemToReset - }); - errorMessage = gettext("Error starting a task to rescore problem '<%- problem_id %>'. Make sure that the problem identifier is complete and correct."); // eslint-disable-line max-len - fullErrorMessage = _.template(errorMessage)({ - problem_id: problemToReset - }); - return $.ajax({ - type: 'POST', - dataType: 'json', - url: studentadmin.$btn_rescore_problem_all.data('endpoint'), - data: sendData, - success: studentadmin.clear_errors_then(function() { - return alert(fullSuccessMessage); // eslint-disable-line no-alert - }), - error: statusAjaxError(function() { - return studentadmin.$request_response_error_all.text(fullErrorMessage); - }) - }); - } else { - return studentadmin.clear_errors(); - } + return studentadmin.rescore_problem_all(false); + }); + this.$btn_rescore_problem_if_higher_all.click(function() { + return studentadmin.rescore_problem_all(true); }); this.$btn_task_history_all.click(function() { var sendData; @@ -528,6 +435,133 @@ }); } + StudentAdmin.prototype.rescore_problem_single = function(onlyIfHigher) { + var errorMessage, fullErrorMessage, fullSuccessMessage, + problemToReset, sendData, successMessage, uniqStudentIdentifier, + that = this; + uniqStudentIdentifier = this.$field_student_select_grade.val(); + problemToReset = this.$field_problem_select_single.val(); + if (!uniqStudentIdentifier) { + return this.$request_err_grade.text( + gettext('Please enter a student email address or username.') + ); + } + if (!problemToReset) { + return this.$request_err_grade.text( + gettext('Please enter a problem location.') + ); + } + sendData = { + unique_student_identifier: uniqStudentIdentifier, + problem_to_reset: problemToReset, + only_if_higher: onlyIfHigher + }; + successMessage = gettext("Started rescore problem task for problem '<%- problem_id %>' and student '<%- student_id %>'. Click the 'Show Task Status' button to see the status of the task."); // eslint-disable-line max-len + fullSuccessMessage = _.template(successMessage)({ + student_id: uniqStudentIdentifier, + problem_id: problemToReset + }); + errorMessage = gettext("Error starting a task to rescore problem '<%- problem_id %>' for student '<%- student_id %>'. Make sure that the the problem and student identifiers are complete and correct."); // eslint-disable-line max-len + fullErrorMessage = _.template(errorMessage)({ + student_id: uniqStudentIdentifier, + problem_id: problemToReset + }); + return $.ajax({ + type: 'POST', + dataType: 'json', + url: this.$btn_rescore_problem_single.data('endpoint'), + data: sendData, + success: this.clear_errors_then(function() { + return alert(fullSuccessMessage); // eslint-disable-line no-alert + }), + error: statusAjaxError(function() { + return that.$request_err_grade.text(fullErrorMessage); + }) + }); + }; + + StudentAdmin.prototype.rescore_entrance_exam_all = function(onlyIfHigher) { + var sendData, uniqStudentIdentifier, + that = this; + uniqStudentIdentifier = this.$field_exam_grade.val(); + if (!uniqStudentIdentifier) { + return this.$request_err_ee.text(gettext( + 'Please enter a student email address or username.') + ); + } + sendData = { + unique_student_identifier: uniqStudentIdentifier, + only_if_higher: onlyIfHigher + }; + return $.ajax({ + type: 'POST', + dataType: 'json', + url: this.$btn_rescore_entrance_exam.data('endpoint'), + data: sendData, + success: this.clear_errors_then(function() { + var fullSuccessMessage, successMessage; + successMessage = gettext("Started entrance exam rescore task for student '{student_id}'. Click the 'Show Task Status' button to see the status of the task."); // eslint-disable-line max-len + fullSuccessMessage = interpolate_text(successMessage, { + student_id: uniqStudentIdentifier + }); + return alert(fullSuccessMessage); // eslint-disable-line no-alert + }), + error: statusAjaxError(function() { + var errorMessage, fullErrorMessage; + errorMessage = gettext("Error starting a task to rescore entrance exam for student '{student_id}'. Make sure that entrance exam has problems in it and student identifier is correct."); // eslint-disable-line max-len + fullErrorMessage = interpolate_text(errorMessage, { + student_id: uniqStudentIdentifier + }); + return that.$request_err_ee.text(fullErrorMessage); + }) + }); + }; + + StudentAdmin.prototype.rescore_problem_all = function(onlyIfHigher) { + var confirmMessage, errorMessage, fullConfirmMessage, + fullErrorMessage, fullSuccessMessage, problemToReset, sendData, successMessage, + that = this; + problemToReset = this.$field_problem_select_all.val(); + if (!problemToReset) { + return this.$request_response_error_all.text( + gettext('Please enter a problem location.') + ); + } + confirmMessage = gettext("Rescore problem '<%- problem_id %>' for all students?"); + fullConfirmMessage = _.template(confirmMessage)({ + problem_id: problemToReset + }); + if (window.confirm(fullConfirmMessage)) { // eslint-disable-line no-alert + sendData = { + all_students: true, + problem_to_reset: problemToReset, + only_if_higher: onlyIfHigher + }; + successMessage = gettext("Successfully started task to rescore problem '<%- problem_id %>' for all students. Click the 'Show Task Status' button to see the status of the task."); // eslint-disable-line max-len + fullSuccessMessage = _.template(successMessage)({ + problem_id: problemToReset + }); + errorMessage = gettext("Error starting a task to rescore problem '<%- problem_id %>'. Make sure that the problem identifier is complete and correct."); // eslint-disable-line max-len + fullErrorMessage = _.template(errorMessage)({ + problem_id: problemToReset + }); + return $.ajax({ + type: 'POST', + dataType: 'json', + url: this.$btn_rescore_problem_all.data('endpoint'), + data: sendData, + success: this.clear_errors_then(function() { + return alert(fullSuccessMessage); // eslint-disable-line no-alert + }), + error: statusAjaxError(function() { + return that.$request_response_error_all.text(fullErrorMessage); + }) + }); + } else { + return this.clear_errors(); + } + }; + StudentAdmin.prototype.clear_errors_then = function(cb) { this.$request_err.empty(); this.$request_err_grade.empty(); diff --git a/lms/static/js/spec/instructor_dashboard/student_admin_spec.js b/lms/static/js/spec/instructor_dashboard/student_admin_spec.js index 1d75f30901..588713ee8c 100644 --- a/lms/static/js/spec/instructor_dashboard/student_admin_spec.js +++ b/lms/static/js/spec/instructor_dashboard/student_admin_spec.js @@ -84,7 +84,7 @@ define(['jquery', 'js/instructor_dashboard/student_admin', 'edx-ui-toolkit/js/ut it('initiates rescoring of the entrance exam when the button is clicked', function() { var successMessage = gettext("Started entrance exam rescore task for student '{student_id}'." + - " Click the 'Show Background Task History for Student' button to see the status of the task."); // eslint-disable-line max-len + " Click the 'Show Task Status' button to see the status of the task."); // eslint-disable-line max-len var fullSuccessMessage = interpolate_text(successMessage, { student_id: uniqStudentIdentifier }); @@ -94,7 +94,8 @@ define(['jquery', 'js/instructor_dashboard/student_admin', 'edx-ui-toolkit/js/ut var requests = AjaxHelpers.requests(this); // Verify that the client contacts the server to start instructor task var params = $.param({ - unique_student_identifier: uniqStudentIdentifier + unique_student_identifier: uniqStudentIdentifier, + only_if_higher: false }); studentadmin.$btn_rescore_entrance_exam.click(); @@ -121,7 +122,8 @@ define(['jquery', 'js/instructor_dashboard/student_admin', 'edx-ui-toolkit/js/ut var requests = AjaxHelpers.requests(this); // Verify that the client contacts the server to start instructor task var params = $.param({ - unique_student_identifier: uniqStudentIdentifier + unique_student_identifier: uniqStudentIdentifier, + only_if_higher: false }); var errorMessage = gettext( "Error starting a task to rescore entrance exam for student '{student_id}'." + diff --git a/lms/static/js/spec/staff_debug_actions_spec.js b/lms/static/js/spec/staff_debug_actions_spec.js index 05cc047f4f..cc95a867fa 100644 --- a/lms/static/js/spec/staff_debug_actions_spec.js +++ b/lms/static/js/spec/staff_debug_actions_spec.js @@ -11,51 +11,52 @@ define([ describe('StaffDebugActions', function() { var location = 'i4x://edX/Open_DemoX/edx_demo_course/problem/test_loc'; var locationName = 'test_loc'; - var fixture_id = 'sd_fu_' + locationName; - var fixture = $('', {id: fixture_id, placeholder: 'userman'}); + var fixtureID = 'sd_fu_' + locationName; + var $fixture = $('', {id: fixtureID, placeholder: 'userman'}); var escapableLocationName = 'test\.\*\+\?\^\:\$\{\}\(\)\|\]\[loc'; - var escapableFixture_id = 'sd_fu_' + escapableLocationName; - var escapableFixture = $('', {id: escapableFixture_id, placeholder: 'userman'}); + var escapableFixtureID = 'sd_fu_' + escapableLocationName; + var $escapableFixture = $('', {id: escapableFixtureID, placeholder: 'userman'}); var esclocationName = 'P2:problem_1'; var escapableId = 'result_' + esclocationName; var escapableResultArea = $('
', {id: escapableId}); - describe('get_url ', function() { + describe('getURL ', function() { it('defines url to courseware ajax entry point', function() { - spyOn(StaffDebug, 'get_current_url') + spyOn(StaffDebug, 'getCurrentUrl') .and.returnValue('/courses/edX/Open_DemoX/edx_demo_course/courseware/stuff'); - expect(StaffDebug.get_url('rescore_problem')) + expect(StaffDebug.getURL('rescore_problem')) .toBe('/courses/edX/Open_DemoX/edx_demo_course/instructor/api/rescore_problem'); }); }); - describe('sanitize_string', function() { + describe('sanitizeString', function() { it('escapes escapable characters in a string', function() { - expect(StaffDebug.sanitized_string('.*+?^:${}()|][')).toBe('\\.\\*\\+\\?\\^\\:\\$\\{\\}\\(\\)\\|\\]\\['); + expect(StaffDebug.sanitizeString('.*+?^:${}()|][')) + .toBe('\\.\\*\\+\\?\\^\\:\\$\\{\\}\\(\\)\\|\\]\\['); }); }); - describe('get_user', function() { + describe('getUser', function() { it('gets the placeholder username if input field is empty', function() { - $('body').append(fixture); - expect(StaffDebug.get_user(locationName)).toBe('userman'); - $('#' + fixture_id).remove(); + $('body').append($fixture); + expect(StaffDebug.getUser(locationName)).toBe('userman'); + $('#' + fixtureID).remove(); }); it('gets a filled in name if there is one', function() { - $('body').append(fixture); - $('#' + fixture_id).val('notuserman'); - expect(StaffDebug.get_user(locationName)).toBe('notuserman'); + $('body').append($fixture); + $('#' + fixtureID).val('notuserman'); + expect(StaffDebug.getUser(locationName)).toBe('notuserman'); - $('#' + fixture_id).val(''); - $('#' + fixture_id).remove(); + $('#' + fixtureID).val(''); + $('#' + fixtureID).remove(); }); it('gets the placeholder name if the id has escapable characters', function() { - $('body').append(escapableFixture); - expect(StaffDebug.get_user('test.*+?^:${}()|][loc')).toBe('userman'); + $('body').append($escapableFixture); + expect(StaffDebug.getUser('test.*+?^:${}()|][loc')).toBe('userman'); $("input[id^='sd_fu_']").remove(); }); }); - describe('do_idash_action success', function() { + describe('doInstructorDashAction success', function() { it('adds a success message to the results element after using an action', function() { $('body').append(escapableResultArea); var requests = AjaxHelpers.requests(this); @@ -63,82 +64,105 @@ define([ locationName: esclocationName, success_msg: 'Successfully reset the attempts for user userman' }; - StaffDebug.do_idash_action(action); + StaffDebug.doInstructorDashAction(action); AjaxHelpers.respondWithJson(requests, action); expect($('#idash_msg').text()).toBe('Successfully reset the attempts for user userman'); $('#result_' + locationName).remove(); }); }); - describe('do_idash_action error', function() { + describe('doInstructorDashAction error', function() { it('adds a failure message to the results element after using an action', function() { $('body').append(escapableResultArea); var requests = AjaxHelpers.requests(this); var action = { locationName: esclocationName, - error_msg: 'Failed to reset attempts.' + error_msg: 'Failed to reset attempts for user.' }; - StaffDebug.do_idash_action(action); + StaffDebug.doInstructorDashAction(action); AjaxHelpers.respondWithError(requests); - expect($('#idash_msg').text()).toBe('Failed to reset attempts. '); + expect($('#idash_msg').text()).toBe('Failed to reset attempts for user. '); $('#result_' + locationName).remove(); }); }); describe('reset', function() { it('makes an ajax call with the expected parameters', function() { - $('body').append(fixture); + $('body').append($fixture); spyOn($, 'ajax'); StaffDebug.reset(locationName, location); expect($.ajax.calls.mostRecent().args[0].type).toEqual('POST'); expect($.ajax.calls.mostRecent().args[0].data).toEqual({ - 'problem_to_reset': location, - 'unique_student_identifier': 'userman', - 'delete_module': false + problem_to_reset: location, + unique_student_identifier: 'userman', + delete_module: false, + only_if_higher: undefined }); expect($.ajax.calls.mostRecent().args[0].url).toEqual( '/instructor/api/reset_student_attempts' ); - $('#' + fixture_id).remove(); + $('#' + fixtureID).remove(); }); }); - describe('sdelete', function() { + describe('deleteStudentState', function() { it('makes an ajax call with the expected parameters', function() { - $('body').append(fixture); + $('body').append($fixture); spyOn($, 'ajax'); - StaffDebug.sdelete(locationName, location); + StaffDebug.deleteStudentState(locationName, location); expect($.ajax.calls.mostRecent().args[0].type).toEqual('POST'); expect($.ajax.calls.mostRecent().args[0].data).toEqual({ - 'problem_to_reset': location, - 'unique_student_identifier': 'userman', - 'delete_module': true + problem_to_reset: location, + unique_student_identifier: 'userman', + delete_module: true, + only_if_higher: undefined }); expect($.ajax.calls.mostRecent().args[0].url).toEqual( '/instructor/api/reset_student_attempts' ); - $('#' + fixture_id).remove(); + $('#' + fixtureID).remove(); }); }); describe('rescore', function() { it('makes an ajax call with the expected parameters', function() { - $('body').append(fixture); + $('body').append($fixture); spyOn($, 'ajax'); StaffDebug.rescore(locationName, location); expect($.ajax.calls.mostRecent().args[0].type).toEqual('POST'); expect($.ajax.calls.mostRecent().args[0].data).toEqual({ - 'problem_to_reset': location, - 'unique_student_identifier': 'userman', - 'delete_module': false + problem_to_reset: location, + unique_student_identifier: 'userman', + delete_module: undefined, + only_if_higher: false }); expect($.ajax.calls.mostRecent().args[0].url).toEqual( '/instructor/api/rescore_problem' ); - $('#' + fixture_id).remove(); + $('#' + fixtureID).remove(); + }); + }); + describe('rescoreIfHigher', function() { + it('makes an ajax call with the expected parameters', function() { + $('body').append($fixture); + + spyOn($, 'ajax'); + StaffDebug.rescoreIfHigher(locationName, location); + + expect($.ajax.calls.mostRecent().args[0].type).toEqual('POST'); + expect($.ajax.calls.mostRecent().args[0].data).toEqual({ + problem_to_reset: location, + unique_student_identifier: 'userman', + delete_module: undefined, + only_if_higher: true + }); + expect($.ajax.calls.mostRecent().args[0].url).toEqual( + '/instructor/api/rescore_problem' + ); + $('#' + fixtureID).remove(); }); }); }); diff --git a/lms/static/js/staff_debug_actions.js b/lms/static/js/staff_debug_actions.js index b7ae3705ce..91b68b9fe2 100644 --- a/lms/static/js/staff_debug_actions.js +++ b/lms/static/js/staff_debug_actions.js @@ -1,37 +1,34 @@ // Build StaffDebug object var StaffDebug = (function() { - get_current_url = function() { - return window.location.pathname; + /* global getCurrentUrl:true */ + var getURL = function(action) { + var pathname = this.getCurrentUrl(); + return pathname.substr(0, pathname.indexOf('/courseware')) + '/instructor/api/' + action; }; - get_url = function(action) { - var pathname = this.get_current_url(); - var url = pathname.substr(0, pathname.indexOf('/courseware')) + '/instructor/api/' + action; - return url; - }; - - sanitized_string = function(string) { + var sanitizeString = function(string) { return string.replace(/[.*+?^:${}()|[\]\\]/g, '\\$&'); }; - get_user = function(locname) { - locname = sanitized_string(locname); - var uname = $('#sd_fu_' + locname).val(); + var getUser = function(locationName) { + var sanitizedLocationName = sanitizeString(locationName); + var uname = $('#sd_fu_' + sanitizedLocationName).val(); if (uname === '') { - uname = $('#sd_fu_' + locname).attr('placeholder'); + uname = $('#sd_fu_' + sanitizedLocationName).attr('placeholder'); } return uname; }; - do_idash_action = function(action) { + var doInstructorDashAction = function(action) { var pdata = { - 'problem_to_reset': action.location, - 'unique_student_identifier': get_user(action.locationName), - 'delete_module': action.delete_module + problem_to_reset: action.location, + unique_student_identifier: getUser(action.locationName), + delete_module: action.delete_module, + only_if_higher: action.only_if_higher }; $.ajax({ type: 'POST', - url: get_url(action.method), + url: getURL(action.method), data: pdata, success: function(data) { var text = _.template(action.success_msg, {interpolate: /\{(.+?)\}/g})( @@ -40,72 +37,90 @@ var StaffDebug = (function() { var html = _.template('

{text}

', {interpolate: /\{(.+?)\}/g})( {text: text} ); - $('#result_' + sanitized_string(action.locationName)).html(html); + $('#result_' + sanitizeString(action.locationName)).html(html); }, error: function(request, status, error) { - var response_json; + var responseJSON; try { - response_json = $.parseJSON(request.responseText); + responseJSON = $.parseJSON(request.responseText); } catch (e) { - response_json = {error: gettext('Unknown Error Occurred.')}; + responseJSON = {error: gettext('Unknown Error Occurred.')}; } var text = _.template('{error_msg} {error}', {interpolate: /\{(.+?)\}/g})( { error_msg: action.error_msg, - error: response_json.error + error: responseJSON.error } ); var html = _.template('

{text}

', {interpolate: /\{(.+?)\}/g})( {text: text} ); - $('#result_' + sanitized_string(action.locationName)).html(html); + $('#result_' + sanitizeString(action.locationName)).html(html); }, dataType: 'json' }); }; - reset = function(locname, location) { - this.do_idash_action({ + var reset = function(locname, location) { + this.doInstructorDashAction({ locationName: locname, location: location, method: 'reset_student_attempts', success_msg: gettext('Successfully reset the attempts for user {user}'), - error_msg: gettext('Failed to reset attempts.'), + error_msg: gettext('Failed to reset attempts for user.'), delete_module: false }); }; - sdelete = function(locname, location) { - this.do_idash_action({ + var deleteStudentState = function(locname, location) { + this.doInstructorDashAction({ locationName: locname, location: location, method: 'reset_student_attempts', success_msg: gettext('Successfully deleted student state for user {user}'), - error_msg: gettext('Failed to delete student state.'), + error_msg: gettext('Failed to delete student state for user.'), delete_module: true }); }; - rescore = function(locname, location) { - this.do_idash_action({ + var rescore = function(locname, location) { + this.doInstructorDashAction({ locationName: locname, location: location, method: 'rescore_problem', success_msg: gettext('Successfully rescored problem for user {user}'), - error_msg: gettext('Failed to rescore problem.'), - delete_module: false + error_msg: gettext('Failed to rescore problem for user.'), + only_if_higher: false }); }; + var rescoreIfHigher = function(locname, location) { + this.doInstructorDashAction({ + locationName: locname, + location: location, + method: 'rescore_problem', + success_msg: gettext('Successfully rescored problem to improve score for user {user}'), + error_msg: gettext('Failed to rescore problem to improve score for user.'), + only_if_higher: true + }); + }; + + getCurrentUrl = function() { + return window.location.pathname; + }; + return { reset: reset, - sdelete: sdelete, + deleteStudentState: deleteStudentState, rescore: rescore, - do_idash_action: do_idash_action, - get_current_url: get_current_url, - get_url: get_url, - get_user: get_user, - sanitized_string: sanitized_string + rescoreIfHigher: rescoreIfHigher, + + // export for testing + doInstructorDashAction: doInstructorDashAction, + getCurrentUrl: getCurrentUrl, + getURL: getURL, + getUser: getUser, + sanitizeString: sanitizeString }; })(); // Register click handlers @@ -116,11 +131,15 @@ $(document).ready(function() { return false; }); $courseContent.on('click', '.staff-debug-sdelete', function() { - StaffDebug.sdelete($(this).parent().data('location-name'), $(this).parent().data('location')); + StaffDebug.deleteStudentState($(this).parent().data('location-name'), $(this).parent().data('location')); return false; }); $courseContent.on('click', '.staff-debug-rescore', function() { StaffDebug.rescore($(this).parent().data('location-name'), $(this).parent().data('location')); return false; }); + $courseContent.on('click', '.staff-debug-rescore-if-higher', function() { + StaffDebug.rescoreIfHigher($(this).parent().data('location-name'), $(this).parent().data('location')); + return false; + }); }); diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index defb7f8e73..f4f65b01c2 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -1283,7 +1283,8 @@ // view - student admin // -------------------- -.instructor-dashboard-wrapper-2 section.idash-section#student_admin > { +.instructor-dashboard-wrapper-2 section.idash-section#student_admin { + .action-type-container{ margin-bottom: $baseline * 2; } @@ -1295,12 +1296,23 @@ .task-history-all-table { margin-top: 1em; } + .task-history-single-table { margin-top: 1em; } + .running-tasks-table { margin-top: 1em; } + + input[type="text"] { + width: 651px; + } + + .location-example { + font-style: italic; + font-size: 0.9em; + } } // view - data download diff --git a/lms/templates/instructor/instructor_dashboard_2/student_admin.html b/lms/templates/instructor/instructor_dashboard_2/student_admin.html index 75842dd0e8..d4f1e1123a 100644 --- a/lms/templates/instructor/instructor_dashboard_2/student_admin.html +++ b/lms/templates/instructor/instructor_dashboard_2/student_admin.html @@ -1,173 +1,199 @@ <%page args="section_data" expression_filter="h"/> <%! from django.utils.translation import ugettext as _ %> -
- %if section_data['is_small_course']: - ## Show the gradebook for small courses -

${_("Student Gradebook")}

-

- ${_("Click here to view the gradebook for enrolled students. This feature is only visible to courses with a small number of total enrolled students.")} -

-
-

- ${_("View Gradebook")} -

-
- %endif +%if section_data['access']['instructor']: +
+ %if section_data['is_small_course']: +

+

${_("View gradebook for enrolled learners")}

+
+ +

+ ${_("View Gradebook")} +

+
+ %endif
-

${_("Student-specific grade inspection")}

-
-
- -
- - - +

${_("View a specific learner's grades and progress")}

+
+ +
+ +

+
-

${_("Student-specific grade adjustment")}

-
-

- -

-
+

${_("Adjust a learner's grade for a specific problem")}

+
+ +
+ + +

- + +
+ +


- ## Translators: A location (string of text) follows this sentence. -

${_("You must provide the complete location of the problem. In the Staff Debug viewer, the location looks like this:")}
- block-v1:edX+DemoX+2015+type@problem+block@618c5933b8b544e4a4cc103d3e508378

+
${_("Attempts")}
+ +
+ -

- ${_("Next, select an action to perform for the given user and problem:")} -

+

-

- + %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): +

${_("Rescore")}
+ +
+ + + + + %endif - %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS') and section_data['access']['instructor']: - - %endif -

+

-

- %if section_data['access']['instructor']: - - %endif -

+
${_("Problem History")}
+ +
+ +

- %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS') and section_data['access']['instructor']: -

- ${_("Rescoring runs in the background, and status for active tasks will appear in the 'Pending Tasks' table. " - "To see status for all tasks submitted for this problem and student, click on this button:")} -

- -

-
- %endif + %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): +
${_("Task Status")}
+ +
+ +
+ %endif
% if course.entrance_exam_enabled:
-

${_("Entrance Exam Adjustment")}

-
- -
+

${_("Adjust a learner's entrance exam results")}

+
-

- ${_("Select an action for the student's entrance exam. This action will affect every problem in the student's exam.")} -

+ +
+ +


- +
${_("Attempts")}
+ +
+ +

- %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS') and section_data['access']['instructor']: - - %endif - +
${_("Allow Skip")}
+ +
+ +

-

- %if section_data['access']['instructor']: - - %endif -

+ %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): +
${_("Rescore")}
+ +
+ + + + +

+ %endif + +
${_("Entrance Exam History")}
+ +
+ +

- %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS') and section_data['access']['instructor']: -

- ${_("Rescoring runs in the background, and status for active tasks will appear in the 'Pending Tasks' table. " - "To see status for all tasks submitted for this problem and student, click on this button:")} -

- -

-
- %endif -
+ %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): +
${_("Task Status")}
+ +
+

+
+ %endif +
%endif -%if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS') and section_data['access']['instructor']: -
-

${_('Course-specific grade adjustment')}

+%if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): +
+

${_("Adjust all enrolled learners' grades for a specific problem")}

-
+
+ +


+ +
${_("Attempts")}
+ +
+ +

+ +
${_("Rescore")}
+ +
+ + + + +

+ +
${_("Task Status")}
+ +
+ +
+
+
%endif %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): -
-
-

${_("Pending Tasks")}

+
+

${_("Pending Tasks")}

-

${_("The status for any active tasks appears in a table below.")}

-
- -
+ +
+
-
+
+%endif %endif diff --git a/lms/templates/staff_problem_info.html b/lms/templates/staff_problem_info.html index 589c39c68b..7b68803424 100644 --- a/lms/templates/staff_problem_info.html +++ b/lms/templates/staff_problem_info.html @@ -69,14 +69,16 @@ ${block_content}
[ % if can_reset_attempts: - + | % endif % if has_instructor_access: - + % if can_rescore_problem: | - + + | + % endif % endif ] diff --git a/openedx/core/djangoapps/credit/api/eligibility.py b/openedx/core/djangoapps/credit/api/eligibility.py index 8f37f8d29e..278b741bd6 100644 --- a/openedx/core/djangoapps/credit/api/eligibility.py +++ b/openedx/core/djangoapps/credit/api/eligibility.py @@ -297,7 +297,7 @@ def set_credit_requirement_status(user, course_key, req_namespace, req_name, sta try: send_credit_notifications(user.username, course_key) except Exception: # pylint: disable=broad-except - log.error("Error sending email") + log.exception("Error sending email") # pylint: disable=invalid-name diff --git a/openedx/core/lib/grade_utils.py b/openedx/core/lib/grade_utils.py new file mode 100644 index 0000000000..e7a68a2f06 --- /dev/null +++ b/openedx/core/lib/grade_utils.py @@ -0,0 +1,24 @@ +""" +Helpers functions for grades and scores. +""" + + +def compare_scores(earned1, possible1, earned2, possible2): + """ + Returns a tuple of: + 1. Whether the 2nd set of scores is higher than the first. + 2. Grade percentage of 1st set of scores. + 3. Grade percentage of 2nd set of scores. + """ + percentage1 = float(earned1) / float(possible1) + percentage2 = float(earned2) / float(possible2) + is_higher = percentage2 > percentage1 + return is_higher, percentage1, percentage2 + + +def is_score_higher(earned1, possible1, earned2, possible2): + """ + Returns whether the 2nd set of scores is higher than the first. + """ + is_higher, _, _ = compare_scores(earned1, possible1, earned2, possible2) + return is_higher diff --git a/openedx/tests/xblock_integration/xblock_testcase.py b/openedx/tests/xblock_integration/xblock_testcase.py index 2098de2e52..1b602e2999 100644 --- a/openedx/tests/xblock_integration/xblock_testcase.py +++ b/openedx/tests/xblock_integration/xblock_testcase.py @@ -204,8 +204,7 @@ class GradePublishTestMixin(object): 'max_score': max_score}) self.scores = [] - patcher = mock.patch("courseware.module_render.set_score", - capture_score) + patcher = mock.patch("lms.djangoapps.grades.signals.handlers.set_score", capture_score) patcher.start() self.addCleanup(patcher.stop)