From 31953c5e0c6b79a336e0b8d01e912bef501a43a2 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Thu, 25 Aug 2016 15:51:36 -0400 Subject: [PATCH] Update correct persistent score * First take at forcing a subsection's grade to update when a signal is sent that a problem's score has changed * Refactor signal handler connection. * Expand bokchoy tests to cover progress page * Add some grading unit tests TNL-5394 TNL-5364 --- cms/envs/common.py | 2 +- common/test/acceptance/pages/lms/progress.py | 46 +++++- .../tests/lms/test_lms_courseware.py | 90 ++++++++++- .../transformers/tests/helpers.py | 5 +- .../course_blocks/transformers/utils.py | 37 +++++ lms/djangoapps/courseware/module_render.py | 2 +- lms/djangoapps/gating/signals.py | 2 +- lms/djangoapps/grades/apps.py | 22 +++ lms/djangoapps/grades/new/subsection_grade.py | 2 +- lms/djangoapps/grades/signals.py | 144 ------------------ lms/djangoapps/grades/signals/__init__.py | 0 lms/djangoapps/grades/signals/handlers.py | 127 +++++++++++++++ lms/djangoapps/grades/signals/signals.py | 21 +++ lms/djangoapps/grades/tests/test_grades.py | 13 +- lms/djangoapps/grades/tests/test_new.py | 24 --- lms/djangoapps/grades/tests/test_signals.py | 27 ++-- .../grades/tests/test_transformer.py | 92 +++++++++++ lms/djangoapps/grades/transformer.py | 9 +- lms/djangoapps/lti_provider/tasks.py | 2 +- lms/envs/common.py | 2 +- 20 files changed, 478 insertions(+), 191 deletions(-) create mode 100644 lms/djangoapps/grades/apps.py delete mode 100644 lms/djangoapps/grades/signals.py create mode 100644 lms/djangoapps/grades/signals/__init__.py create mode 100644 lms/djangoapps/grades/signals/handlers.py create mode 100644 lms/djangoapps/grades/signals/signals.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 532c175ebe..ce5c1ca62c 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -911,7 +911,7 @@ INSTALLED_APPS = ( # other apps that are. Django 1.8 wants to have imported models supported # by installed apps. 'lms.djangoapps.verify_student', - 'lms.djangoapps.grades', + 'lms.djangoapps.grades.apps.GradesConfig', # Microsite configuration application 'microsite_configuration', diff --git a/common/test/acceptance/pages/lms/progress.py b/common/test/acceptance/pages/lms/progress.py index 3d9ca3da38..f0a6d5aab6 100644 --- a/common/test/acceptance/pages/lms/progress.py +++ b/common/test/acceptance/pages/lms/progress.py @@ -22,13 +22,35 @@ class ProgressPage(CoursePage): def grading_formats(self): return [label.replace(' Scores:', '') for label in self.q(css="div.scores h3").text] + def section_score(self, chapter, section): + """ + Return a list of (points, max_points) tuples representing the + aggregate score for the section. + + Example: + page.section_score('Week 1', 'Lesson 1') --> (2, 5) + + Returns `None` if no such chapter and section can be found. + """ + # Find the index of the section in the chapter + chapter_index = self._chapter_index(chapter) + if chapter_index is None: + return None + + section_index = self._section_index(chapter_index, section) + if section_index is None: + return None + + # Retrieve the scores for the section + return self._aggregate_section_score(chapter_index, section_index) + def scores(self, chapter, section): """ Return a list of (points, max_points) tuples representing the scores for the section. Example: - scores('Week 1', 'Lesson 1') --> [(2, 4), (0, 1)] + page.scores('Week 1', 'Lesson 1') --> [(2, 4), (0, 1)] Returns `None` if no such chapter and section can be found. """ @@ -86,6 +108,28 @@ class ProgressPage(CoursePage): self.warning("Could not find section '{0}'".format(title)) return None + def _aggregate_section_score(self, chapter_index, section_index): + """ + Return a tuple of the form `(points, max_points)` representing + the aggregate score for the specified chapter and section. + """ + score_css = "div.chapters>section:nth-of-type({0}) div.sections>div:nth-of-type({1}) h3>span".format( + chapter_index, section_index + + ) + text_scores = self.q(css=score_css).text + assert len(text_scores) == 1 + text_score = text_scores[0] + text_score = text_score.split()[0] # strip off percentage, if present + + assert (text_score[0], text_score[-1]) == ('(', ')') + text_score = text_score.strip('()') + + assert '/' in text_score + score = tuple(int(x) for x in text_score.split('/')) + assert len(score) == 2 + return score + def _section_scores(self, chapter_index, section_index): """ Return a list of `(points, max_points)` tuples representing diff --git a/common/test/acceptance/tests/lms/test_lms_courseware.py b/common/test/acceptance/tests/lms/test_lms_courseware.py index 512f042786..84e2d3cb94 100644 --- a/common/test/acceptance/tests/lms/test_lms_courseware.py +++ b/common/test/acceptance/tests/lms/test_lms_courseware.py @@ -3,10 +3,12 @@ End-to-end tests for the LMS. """ +from contextlib import contextmanager import json -from nose.plugins.attrib import attr from datetime import datetime, timedelta + import ddt +from nose.plugins.attrib import attr from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from ..helpers import UniqueCourseTest, EventsTestMixin @@ -908,3 +910,89 @@ class SubsectionHiddenAfterDueDateTest(UniqueCourseTest): self.progress_page.visit() self.assertEqual(self.progress_page.scores('Test Section 1', 'Test Subsection 1'), [(0, 1)]) + + +class ProgressPageTest(UniqueCourseTest): + """ + Test that the progress page reports scores from completed assessments. + """ + USERNAME = "STUDENT_TESTER" + EMAIL = "student101@example.com" + + def setUp(self): + super(ProgressPageTest, self).setUp() + + self.courseware_page = CoursewarePage(self.browser, self.course_id) + self.problem_page = ProblemPage(self.browser) # pylint: disable=attribute-defined-outside-init + self.progress_page = ProgressPage(self.browser, self.course_id) + self.logout_page = LogoutPage(self.browser) + + self.course_outline = CourseOutlinePage( + self.browser, + self.course_info['org'], + self.course_info['number'], + self.course_info['run'] + ) + + # Install a course with sections/problems, tabs, updates, and handouts + course_fix = CourseFixture( + self.course_info['org'], + self.course_info['number'], + self.course_info['run'], + self.course_info['display_name'] + ) + + course_fix.add_children( + XBlockFixtureDesc('chapter', 'Test Section 1').add_children( + XBlockFixtureDesc('sequential', 'Test Subsection 1').add_children( + create_multiple_choice_problem('Test Problem 1') + ) + ) + ).install() + + # Auto-auth register for the course. + _auto_auth(self.browser, self.USERNAME, self.EMAIL, False, self.course_id) + + def test_progress_page_shows_scored_problems(self): + with self._logged_in_session(): + self.assertEqual(self._get_scores(), [(0, 1)]) + self.assertEqual(self._get_section_score(), (0, 1)) + self.courseware_page.visit() + self._answer_problem_correctly() + self.assertEqual(self._get_scores(), [(1, 1)]) + self.assertEqual(self._get_section_score(), (1, 1)) + + def _answer_problem_correctly(self): + """ + Submit a correct answer to the problem. + """ + self.courseware_page.go_to_sequential_position(1) + self.problem_page.click_choice('choice_choice_2') + self.problem_page.click_check() + + def _get_section_score(self): + """ + Return a list of scores from the progress page. + """ + self.progress_page.visit() + return self.progress_page.section_score('Test Section 1', 'Test Subsection 1') + + def _get_scores(self): + """ + Return a list of scores from the progress page. + """ + self.progress_page.visit() + return self.progress_page.scores('Test Section 1', 'Test Subsection 1') + + @contextmanager + def _logged_in_session(self): + """ + Ensure that the user is logged in and out appropriately at the beginning + and end of the current test. + """ + self.logout_page.visit() + try: + _auto_auth(self.browser, self.USERNAME, self.EMAIL, False, self.course_id) + yield + finally: + self.logout_page.visit() diff --git a/lms/djangoapps/course_blocks/transformers/tests/helpers.py b/lms/djangoapps/course_blocks/transformers/tests/helpers.py index fd304aad5b..9be79333a4 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/helpers.py +++ b/lms/djangoapps/course_blocks/transformers/tests/helpers.py @@ -115,8 +115,9 @@ class CourseStructureTestCase(TransformerRegistryTestMixin, ModuleStoreTestCase) # It would be re-added to the course if the course was # explicitly listed in parents. course = modulestore().get_item(block_map['course'].location) - course.children.remove(block_key) - block_map['course'] = update_block(course) + if block_key in course.children: + course.children.remove(block_key) + block_map['course'] = update_block(course) # Add this to block to each listed parent. for parent_ref in parents: diff --git a/lms/djangoapps/course_blocks/transformers/utils.py b/lms/djangoapps/course_blocks/transformers/utils.py index 5ca498053b..c9c7ca55bb 100644 --- a/lms/djangoapps/course_blocks/transformers/utils.py +++ b/lms/djangoapps/course_blocks/transformers/utils.py @@ -18,6 +18,43 @@ def get_field_on_block(block, field_name, default_value=None): return default_value +def collect_unioned_set_field(block_structure, transformer, merged_field_name, filter_by): + """ + Recursively union a set field on the block structure. + + If a block matches filter_by, it will be added to the result set. + This (potentially empty) set is unioned with the sets contained in + merged_field_name for all parents of the block. + + This set union operation takes place during a topological traversal + of the block_structure, so all sets are inherited by descendants. + + Parameters: + block_structure: BlockStructure to traverse + transformer: transformer that will be used for get_ and + set_transformer_block_field + merged_field_name: name of the field to store + filter_by: a unary lambda that returns true if a given + block_key should be included in the result set + """ + for block_key in block_structure.topological_traversal(): + result_set = {block_key} if filter_by(block_key) else set() + for parent in block_structure.get_parents(block_key): + result_set |= block_structure.get_transformer_block_field( + parent, + transformer, + merged_field_name, + set(), + ) + + block_structure.set_transformer_block_field( + block_key, + transformer, + merged_field_name, + result_set, + ) + + def collect_merged_boolean_field( block_structure, transformer, diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 6c22a86587..5d3e663b3c 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -45,7 +45,7 @@ from courseware.masquerade import ( setup_masquerade, ) from courseware.model_data import DjangoKeyValueStore, FieldDataCache, set_score -from lms.djangoapps.grades.signals import SCORE_CHANGED +from lms.djangoapps.grades.signals.signals import SCORE_CHANGED from edxmako.shortcuts import render_to_string from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig diff --git a/lms/djangoapps/gating/signals.py b/lms/djangoapps/gating/signals.py index 561d8df0ac..bfa5b974a7 100644 --- a/lms/djangoapps/gating/signals.py +++ b/lms/djangoapps/gating/signals.py @@ -4,7 +4,7 @@ Signal handlers for the gating djangoapp from django.dispatch import receiver from opaque_keys.edx.keys import CourseKey, UsageKey from xmodule.modulestore.django import modulestore -from lms.djangoapps.grades.signals import SCORE_CHANGED +from lms.djangoapps.grades.signals.signals import SCORE_CHANGED from gating import api as gating_api diff --git a/lms/djangoapps/grades/apps.py b/lms/djangoapps/grades/apps.py new file mode 100644 index 0000000000..3f509a9259 --- /dev/null +++ b/lms/djangoapps/grades/apps.py @@ -0,0 +1,22 @@ +""" +Grades Application Configuration + +Signal handlers are connected here. +""" + +from django.apps import AppConfig + + +class GradesConfig(AppConfig): + """ + Application Configuration for Grades. + """ + name = u'lms.djangoapps.grades' + + def ready(self): + """ + Connect handlers to recalculate grades. + """ + # Can't import models at module level in AppConfigs, and models get + # included from the signal handlers + from .signals import handlers # pylint: disable=unused-variable diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index a8d1bef12f..c9d13b68cb 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -132,7 +132,7 @@ class SubsectionGrade(object): # There's a chance that the value of weight is not the same value used when the problem was scored, # since we can get the value from either block_structure or CSM/submissions. - weight = getattr(block, 'weight', 1.0) + weight = getattr(block, 'weight', None) if persisted_values: possible = persisted_values.get('possible', possible) weight = persisted_values.get('weight', weight) diff --git a/lms/djangoapps/grades/signals.py b/lms/djangoapps/grades/signals.py deleted file mode 100644 index dce9a7bdf5..0000000000 --- a/lms/djangoapps/grades/signals.py +++ /dev/null @@ -1,144 +0,0 @@ -""" -Grades related signals. -""" -from django.conf import settings -from django.dispatch import receiver, Signal -from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag -from logging import getLogger -from opaque_keys.edx.locator import CourseLocator -from opaque_keys.edx.keys import UsageKey -from student.models import user_by_anonymous_id -from submissions.models import score_set, score_reset - -log = getLogger(__name__) - - -# Signal that indicates that a user's score for a problem has been updated. -# This signal is generated when a scoring event occurs either within the core -# platform or in the Submissions module. Note that this signal will be triggered -# regardless of the new and previous values of the score (i.e. it may be the -# case that this signal is generated when a user re-attempts a problem but -# 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 - ] -) - - -@receiver(score_set) -def submissions_score_set_handler(sender, **kwargs): # pylint: disable=unused-argument - """ - Consume the score_set signal defined in the Submissions API, and convert it - to a SCORE_CHANGED signal defined in this module. Converts the unicode keys - for user, course and item into the standard representation for the - SCORE_CHANGED signal. - - This method expects that the kwargs dictionary will contain the following - entries (See the definition of score_set): - - 'points_possible': integer, - - 'points_earned': integer, - - 'anonymous_user_id': unicode, - - 'course_id': unicode, - - 'item_id': unicode - """ - points_possible = kwargs.get('points_possible', None) - points_earned = kwargs.get('points_earned', None) - course_id = kwargs.get('course_id', None) - usage_id = kwargs.get('item_id', None) - user = None - if 'anonymous_user_id' in kwargs: - user = user_by_anonymous_id(kwargs.get('anonymous_user_id')) - - # If any of the kwargs were missing, at least one of the following values - # will be None. - if all((user, points_possible, points_earned, course_id, usage_id)): - SCORE_CHANGED.send( - sender=None, - points_possible=points_possible, - points_earned=points_earned, - user=user, - course_id=course_id, - usage_id=usage_id - ) - else: - log.exception( - u"Failed to process score_set signal from Submissions API. " - "points_possible: %s, points_earned: %s, user: %s, course_id: %s, " - "usage_id: %s", points_possible, points_earned, user, course_id, usage_id - ) - - -@receiver(score_reset) -def submissions_score_reset_handler(sender, **kwargs): # pylint: disable=unused-argument - """ - Consume the score_reset signal defined in the Submissions API, and convert - it to a SCORE_CHANGED signal indicating that the score has been set to 0/0. - Converts the unicode keys for user, course and item into the standard - representation for the SCORE_CHANGED signal. - - This method expects that the kwargs dictionary will contain the following - entries (See the definition of score_reset): - - 'anonymous_user_id': unicode, - - 'course_id': unicode, - - 'item_id': unicode - """ - course_id = kwargs.get('course_id', None) - usage_id = kwargs.get('item_id', None) - user = None - if 'anonymous_user_id' in kwargs: - user = user_by_anonymous_id(kwargs.get('anonymous_user_id')) - - # If any of the kwargs were missing, at least one of the following values - # will be None. - if all((user, course_id, usage_id)): - SCORE_CHANGED.send( - sender=None, - points_possible=0, - points_earned=0, - user=user, - course_id=course_id, - usage_id=usage_id - ) - else: - log.exception( - u"Failed to process score_reset signal from Submissions API. " - "user: %s, course_id: %s, usage_id: %s", user, course_id, usage_id - ) - - -@receiver(SCORE_CHANGED) -def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=unused-argument - """ - Consume the SCORE_CHANGED signal and trigger an update. - This method expects that the kwargs dictionary will contain the following - entries (See the definition of SCORE_CHANGED): - - points_possible: Maximum score available for the exercise - - points_earned: Score obtained by the user - - user: User object - - course_id: Unicode string representing the course - - usage_id: Unicode string indicating the courseware instance - """ - try: - course_id = kwargs.get('course_id', None) - usage_id = kwargs.get('usage_id', None) - student = kwargs.get('user', None) - - course_key = CourseLocator.from_string(course_id) - if not PersistentGradesEnabledFlag.feature_enabled(course_key): - return - - usage_key = UsageKey.from_string(usage_id).replace(course_key=course_key) - - from lms.djangoapps.grades.new.subsection_grade import SubsectionGradeFactory - SubsectionGradeFactory(student).update(usage_key, course_key) - except Exception as ex: # pylint: disable=broad-except - log.exception( - u"Failed to process SCORE_CHANGED signal. " - "user: %s, course_id: %s, " - "usage_id: %s. Exception: %s", unicode(student), course_id, usage_id, ex.message - ) diff --git a/lms/djangoapps/grades/signals/__init__.py b/lms/djangoapps/grades/signals/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py new file mode 100644 index 0000000000..ae9755e358 --- /dev/null +++ b/lms/djangoapps/grades/signals/handlers.py @@ -0,0 +1,127 @@ +""" +Grades related signals. +""" +from logging import getLogger + +from django.dispatch import receiver + +from opaque_keys.edx.locator import CourseLocator +from opaque_keys.edx.keys import UsageKey +from openedx.core.djangoapps.content.block_structure.api import get_course_in_cache +from student.models import user_by_anonymous_id +from submissions.models import score_set, score_reset + +from .signals import SCORE_CHANGED +from ..config.models import PersistentGradesEnabledFlag +from ..transformer import GradesTransformer +from ..new.subsection_grade import SubsectionGradeFactory + +log = getLogger(__name__) + + +@receiver(score_set) +def submissions_score_set_handler(sender, **kwargs): # pylint: disable=unused-argument + """ + Consume the score_set signal defined in the Submissions API, and convert it + to a SCORE_CHANGED signal defined in this module. Converts the unicode keys + for user, course and item into the standard representation for the + SCORE_CHANGED signal. + + This method expects that the kwargs dictionary will contain the following + entries (See the definition of score_set): + - 'points_possible': integer, + - 'points_earned': integer, + - 'anonymous_user_id': unicode, + - 'course_id': unicode, + - 'item_id': unicode + """ + points_possible = kwargs['points_possible'] + points_earned = kwargs['points_earned'] + course_id = kwargs['course_id'] + usage_id = kwargs['item_id'] + user = user_by_anonymous_id(kwargs['anonymous_user_id']) + + # If any of the kwargs were missing, at least one of the following values + # will be None. + SCORE_CHANGED.send( + sender=None, + points_possible=points_possible, + points_earned=points_earned, + user=user, + course_id=course_id, + usage_id=usage_id + ) + + +@receiver(score_reset) +def submissions_score_reset_handler(sender, **kwargs): # pylint: disable=unused-argument + """ + Consume the score_reset signal defined in the Submissions API, and convert + it to a SCORE_CHANGED signal indicating that the score has been set to 0/0. + Converts the unicode keys for user, course and item into the standard + representation for the SCORE_CHANGED signal. + + This method expects that the kwargs dictionary will contain the following + entries (See the definition of score_reset): + - 'anonymous_user_id': unicode, + - 'course_id': unicode, + - 'item_id': unicode + """ + course_id = kwargs['course_id'] + usage_id = kwargs['item_id'] + user = user_by_anonymous_id(kwargs['anonymous_user_id']) + + # If any of the kwargs were missing, at least one of the following values + # will be None. + SCORE_CHANGED.send( + sender=None, + points_possible=0, + points_earned=0, + user=user, + course_id=course_id, + usage_id=usage_id + ) + + +@receiver(SCORE_CHANGED) +def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=unused-argument + """ + Consume the SCORE_CHANGED signal and trigger an update. + This method expects that the kwargs dictionary will contain the following + entries (See the definition of SCORE_CHANGED): + - points_possible: Maximum score available for the exercise + - points_earned: Score obtained by the user + - user: User object + - course_id: Unicode string representing the course + - usage_id: Unicode string indicating the courseware instance + """ + try: + course_id = kwargs['course_id'] + usage_id = kwargs['usage_id'] + student = kwargs['user'] + except KeyError: + log.exception( + u"Failed to process SCORE_CHANGED signal, some arguments were missing." + "user: %s, course_id: %s, usage_id: %s.", + kwargs.get('user', None), + kwargs.get('course_id', None), + kwargs.get('usage_id', None), + ) + return + + course_key = CourseLocator.from_string(course_id) + if not PersistentGradesEnabledFlag.feature_enabled(course_key): + return + + usage_key = UsageKey.from_string(usage_id).replace(course_key=course_key) + block_structure = get_course_in_cache(course_key) + + subsections_to_update = block_structure.get_transformer_block_field( + usage_key, + GradesTransformer, + 'subsections', + set() + ) + + for subsection in subsections_to_update: + SubsectionGradeFactory(student).update(subsection, course_key) diff --git a/lms/djangoapps/grades/signals/signals.py b/lms/djangoapps/grades/signals/signals.py new file mode 100644 index 0000000000..250b27e9ab --- /dev/null +++ b/lms/djangoapps/grades/signals/signals.py @@ -0,0 +1,21 @@ +""" +Grades related signals. +""" +from django.dispatch import Signal + + +# Signal that indicates that a user's score for a problem has been updated. +# This signal is generated when a scoring event occurs either within the core +# platform or in the Submissions module. Note that this signal will be triggered +# regardless of the new and previous values of the score (i.e. it may be the +# case that this signal is generated when a user re-attempts a problem but +# 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 + ] +) diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index faa00e407d..1e70acfc9a 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -382,10 +382,12 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase): self.client.login(username=self.request.user.username, password="test") CourseEnrollment.enroll(self.request.user, self.course.id) + self.course_structure = get_course_blocks(self.request.user, self.course.location) + # warm up the score cache to allow accurate query counts, even if tests are run in random order get_module_score(self.request.user, self.course, self.seq1) - def test_get_module_score(self): + def test_subsection_scores(self): """ Test test_get_module_score """ @@ -393,21 +395,30 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase): # then stored in the request). with self.assertNumQueries(1): score = get_module_score(self.request.user, self.course, self.seq1) + new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course) self.assertEqual(score, 0) + self.assertEqual(new_score.all_total.earned, 0) answer_problem(self.course, self.request, self.problem1) answer_problem(self.course, self.request, self.problem2) with self.assertNumQueries(1): score = get_module_score(self.request.user, self.course, self.seq1) + new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course) self.assertEqual(score, 1.0) + self.assertEqual(new_score.all_total.earned, 2.0) + # These differ because get_module_score normalizes the subsection score + # to 1, which can cause incorrect aggregation behavior that will be + # fixed by TNL-5062. answer_problem(self.course, self.request, self.problem1) answer_problem(self.course, self.request, self.problem2, 0) with self.assertNumQueries(1): score = get_module_score(self.request.user, self.course, self.seq1) + new_score = SubsectionGradeFactory(self.request.user).create(self.seq1, self.course_structure, self.course) self.assertEqual(score, .5) + self.assertEqual(new_score.all_total.earned, 1.0) def test_get_module_score_with_empty_score(self): """ diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index e64ec033c8..0ddde1fe92 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -71,13 +71,6 @@ class TestCourseGradeFactory(GradeTestBase): Test that CourseGrades are calculated properly """ - @classmethod - def setUpClass(cls): - super(TestCourseGradeFactory, cls).setUpClass() - - def setUp(self): - super(TestCourseGradeFactory, self).setUp() - @ddt.data( (True, True), (True, False), @@ -110,16 +103,6 @@ class SubsectionGradeFactoryTest(GradeTestBase): enable saving subsection grades blocks/enables that feature as expected. """ - @classmethod - def setUpClass(cls): - super(SubsectionGradeFactoryTest, cls).setUpClass() - - def setUp(self): - """ - Set up test course - """ - super(SubsectionGradeFactoryTest, self).setUp() - def test_create(self): """ Tests to ensure that a persistent subsection grade is created, saved, then fetched on re-request. @@ -190,13 +173,6 @@ class SubsectionGradeTest(GradeTestBase): Tests SubsectionGrade functionality. """ - @classmethod - def setUpClass(cls): - super(SubsectionGradeTest, cls).setUpClass() - - def setUp(self): - super(SubsectionGradeTest, self).setUp() - def test_compute(self): """ Assuming the underlying score reporting methods work, test that the score is calculated properly. diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 68c85dfecb..2ecf0404f0 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -13,12 +13,12 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls -from ..signals import ( +from ..signals.handlers import ( submissions_score_set_handler, submissions_score_reset_handler, recalculate_subsection_grade_handler, - SCORE_CHANGED ) +from ..signals.signals import SCORE_CHANGED SUBMISSION_SET_KWARGS = { @@ -50,10 +50,13 @@ class SubmissionSignalRelayTest(TestCase): Configure mocks for all the dependencies of the render method """ super(SubmissionSignalRelayTest, self).setUp() - self.signal_mock = self.setup_patch('lms.djangoapps.grades.signals.SCORE_CHANGED.send', None) + self.signal_mock = self.setup_patch('lms.djangoapps.grades.signals.signals.SCORE_CHANGED.send', None) self.user_mock = MagicMock() self.user_mock.id = 42 - self.get_user_mock = self.setup_patch('lms.djangoapps.grades.signals.user_by_anonymous_id', self.user_mock) + self.get_user_mock = self.setup_patch( + 'lms.djangoapps.grades.signals.handlers.user_by_anonymous_id', + self.user_mock + ) def setup_patch(self, function_name, return_value): """ @@ -100,7 +103,8 @@ class SubmissionSignalRelayTest(TestCase): kwargs = SUBMISSION_SET_KWARGS.copy() del kwargs[missing] - submissions_score_set_handler(None, **kwargs) + with self.assertRaises(KeyError): + submissions_score_set_handler(None, **kwargs) self.signal_mock.assert_not_called() def test_score_set_bad_user(self): @@ -109,7 +113,7 @@ class SubmissionSignalRelayTest(TestCase): that has an invalid user ID, the courseware model does not generate a signal. """ - self.get_user_mock = self.setup_patch('lms.djangoapps.grades.signals.user_by_anonymous_id', None) + self.get_user_mock = self.setup_patch('lms.djangoapps.grades.signals.handlers.user_by_anonymous_id', None) submissions_score_set_handler(None, **SUBMISSION_SET_KWARGS) self.signal_mock.assert_not_called() @@ -149,7 +153,8 @@ class SubmissionSignalRelayTest(TestCase): kwargs = SUBMISSION_RESET_KWARGS.copy() del kwargs[missing] - submissions_score_reset_handler(None, **kwargs) + with self.assertRaises(KeyError): + submissions_score_reset_handler(None, **kwargs) self.signal_mock.assert_not_called() def test_score_reset_bad_user(self): @@ -158,7 +163,7 @@ class SubmissionSignalRelayTest(TestCase): that has an invalid user ID, the courseware model does not generate a signal. """ - self.get_user_mock = self.setup_patch('lms.djangoapps.grades.signals.user_by_anonymous_id', None) + self.get_user_mock = self.setup_patch('lms.djangoapps.grades.signals.handlers.user_by_anonymous_id', None) submissions_score_reset_handler(None, **SUBMISSION_RESET_KWARGS) self.signal_mock.assert_not_called() @@ -243,15 +248,15 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): @ddt.data( ('points_possible', 2, 19), ('points_earned', 2, 19), - ('user', 0, 3), + ('user', 0, 0), ('course_id', 0, 0), - ('usage_id', 0, 2), + ('usage_id', 0, 0), ) @ddt.unpack def test_missing_kwargs(self, kwarg, expected_mongo_calls, expected_sql_calls): self.set_up_course() del self.score_changed_kwargs[kwarg] - with patch('lms.djangoapps.grades.signals.log') as log_mock: + with patch('lms.djangoapps.grades.signals.handlers.log') as log_mock: with check_mongo_calls(expected_mongo_calls) and self.assertNumQueries(expected_sql_calls): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) self.assertEqual(log_mock.exception.called, kwarg not in ['points_possible', 'points_earned']) diff --git a/lms/djangoapps/grades/tests/test_transformer.py b/lms/djangoapps/grades/tests/test_transformer.py index 1fc8f7d2a7..d84f04336e 100644 --- a/lms/djangoapps/grades/tests/test_transformer.py +++ b/lms/djangoapps/grades/tests/test_transformer.py @@ -102,6 +102,98 @@ class GradesTransformerTestCase(CourseStructureTestCase): } ]) + def build_complicated_hypothetical_course(self): + """ + Create a test course with a very odd structure as a stress-test for various methods. + + Current design is to test containing_subsection logic in collect_unioned_set_field. + I can't reasonably draw this in ascii art (due to intentional complexities), so here's an overview: + We have 1 course, containing 1 chapter, containing 2 subsections. + + From here, it starts to get hairy. Call our subsections A and B. + Subsection A contains 3 verticals (call them 1, 2, and 3), and another subsection (C) + Subsection B contains vertical 3 and subsection C + Subsection C contains 1 problem (b) + Vertical 1 contains 1 vertical (11) + Vertical 2 contains no children + Vertical 3 contains no children + Vertical 11 contains 1 problem (aa) and vertical 2 + Problem b contains no children + """ + return self.build_course([ + { + u'org': u'GradesTestOrg', + u'course': u'GB101', + u'run': u'cannonball', + u'metadata': {u'format': u'homework'}, + u'#type': u'course', + u'#ref': u'course', + u'#children': [ + { + u'#type': u'chapter', + u'#ref': u'chapter', + u'#children': [ + { + u'#type': u'sequential', + u'#ref': 'sub_A', + u'#children': [ + { + u'#type': u'vertical', + u'#ref': 'vert_1', + u'#children': [ + { + u'#type': u'vertical', + u'#ref': u'vert_A11', + u'#children': [{u'#type': u'problem', u'#ref': u'prob_A1aa'}] + }, + ] + }, + {u'#type': u'vertical', u'#ref': 'vert_2', '#parents': [u'vert_A11']}, + ] + }, + { + u'#type': u'sequential', + u'#ref': u'sub_B', + u'#children': [ + {u'#type': u'vertical', u'#ref': 'vert_3', '#parents': ['sub_A']}, + { + u'#type': u'sequential', + u'#ref': 'sub_C', + '#parents': ['sub_A'], + u'#children': [{u'#type': u'problem', u'#ref': u'prob_BCb'}] + }, + ] + }, + ] + } + ] + } + ]) + + def test_collect_containing_subsection(self): + expected_subsections = { + 'course': set(), + 'chapter': set(), + 'sub_A': {'sub_A'}, + 'sub_B': {'sub_B'}, + 'sub_C': {'sub_A', 'sub_B', 'sub_C'}, + 'vert_1': {'sub_A'}, + 'vert_2': {'sub_A'}, + 'vert_3': {'sub_A', 'sub_B'}, + 'vert_A11': {'sub_A'}, + 'prob_A1aa': {'sub_A'}, + 'prob_BCb': {'sub_A', 'sub_B', 'sub_C'}, + } + blocks = self.build_complicated_hypothetical_course() + block_structure = get_course_blocks(self.student, blocks[u'course'].location, self.transformers) + for block_ref, expected_subsections in expected_subsections.iteritems(): + actual_subsections = block_structure.get_transformer_block_field( + blocks[block_ref].location, + self.TRANSFORMER_CLASS_TO_TEST, + 'subsections', + ) + self.assertEqual(actual_subsections, {blocks[sub].location for sub in expected_subsections}) + def test_ungraded_block_collection(self): blocks = self.build_course_with_problems() block_structure = get_course_blocks(self.student, blocks[u'course'].location, self.transformers) diff --git a/lms/djangoapps/grades/transformer.py b/lms/djangoapps/grades/transformer.py index 82c7f92c90..952bdc6dc7 100644 --- a/lms/djangoapps/grades/transformer.py +++ b/lms/djangoapps/grades/transformer.py @@ -5,6 +5,7 @@ from django.test.client import RequestFactory from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor +from lms.djangoapps.course_blocks.transformers.utils import collect_unioned_set_field from openedx.core.lib.block_structure.transformer import BlockStructureTransformer from openedx.core.djangoapps.util.user_utils import SystemUser @@ -30,7 +31,7 @@ class GradesTransformer(BlockStructureTransformer): max_score: (numeric) """ - VERSION = 2 + VERSION = 3 FIELDS_TO_COLLECT = [u'due', u'format', u'graded', u'has_score', u'weight', u'course_version', u'subtree_edited_on'] @classmethod @@ -49,6 +50,12 @@ class GradesTransformer(BlockStructureTransformer): """ block_structure.request_xblock_fields(*cls.FIELDS_TO_COLLECT) cls._collect_max_scores(block_structure) + collect_unioned_set_field( + block_structure=block_structure, + transformer=cls, + merged_field_name='subsections', + filter_by=lambda block_key: block_key.block_type == 'sequential', + ) def transform(self, block_structure, usage_context): """ diff --git a/lms/djangoapps/lti_provider/tasks.py b/lms/djangoapps/lti_provider/tasks.py index fa0958f04a..7980a68f22 100644 --- a/lms/djangoapps/lti_provider/tasks.py +++ b/lms/djangoapps/lti_provider/tasks.py @@ -8,7 +8,7 @@ from django.dispatch import receiver import logging from lms.djangoapps.grades import progress -from lms.djangoapps.grades.signals import SCORE_CHANGED +from lms.djangoapps.grades.signals.signals import SCORE_CHANGED from lms import CELERY_APP from lti_provider.models import GradedAssignment import lti_provider.outcomes as outcomes diff --git a/lms/envs/common.py b/lms/envs/common.py index fc4b8054ec..a4701b7456 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1912,7 +1912,7 @@ INSTALLED_APPS = ( 'openedx.core.djangoapps.course_groups', 'bulk_email', 'branding', - 'lms.djangoapps.grades', + 'lms.djangoapps.grades.apps.GradesConfig', # Student support tools 'support',