From e00fe03d9d83b7d57845942e3e82ba1c323a2a11 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 30 Sep 2016 10:05:44 -0400 Subject: [PATCH 01/12] Revert "Merge pull request #13539 from edx/sstudent/TNL-5601" This reverts commit e3db08f1946e34c128c4fa9d56c180ca9bbca4c1, reversing changes made to 51c2444de30afaa9061fcff7a6dce1ddb9ed99df. --- lms/djangoapps/grades/scores.py | 38 ++++----- lms/djangoapps/grades/tests/test_scores.py | 10 +-- lms/djangoapps/instructor/enrollment.py | 47 +---------- .../instructor/tests/test_enrollment.py | 79 ------------------- 4 files changed, 26 insertions(+), 148 deletions(-) diff --git a/lms/djangoapps/grades/scores.py b/lms/djangoapps/grades/scores.py index 86832ef156..27abc562cf 100644 --- a/lms/djangoapps/grades/scores.py +++ b/lms/djangoapps/grades/scores.py @@ -124,23 +124,6 @@ def get_score(submissions_scores, csm_scores, persisted_block, block): ) -def weighted_score(raw_earned, raw_possible, weight): - """ - Returns a tuple that represents the weighted (earned, possible) score. - If weight is None or raw_possible is 0, returns the original values. - - When weight is used, it defines the weighted_possible. This allows - course authors to specify the exact maximum value for a problem when - they provide a weight. - """ - assert raw_possible is not None - cannot_compute_with_weight = weight is None or raw_possible == 0 - if cannot_compute_with_weight: - return raw_earned, raw_possible - else: - return float(raw_earned) * weight / raw_possible, float(weight) - - def _get_score_from_submissions(submissions_scores, block): """ Returns the score values from the submissions API if found. @@ -174,7 +157,7 @@ def _get_score_from_csm(csm_scores, block, weight): if has_valid_score: raw_earned = score.correct if score.correct is not None else 0.0 raw_possible = score.total - return (raw_earned, raw_possible) + weighted_score(raw_earned, raw_possible, weight) + return (raw_earned, raw_possible) + _weighted_score(raw_earned, raw_possible, weight) def _get_score_from_persisted_or_latest_block(persisted_block, block, weight): @@ -191,7 +174,7 @@ def _get_score_from_persisted_or_latest_block(persisted_block, block, weight): else: raw_possible = block.transformer_data[GradesTransformer].max_score - return (raw_earned, raw_possible) + weighted_score(raw_earned, raw_possible, weight) + return (raw_earned, raw_possible) + _weighted_score(raw_earned, raw_possible, weight) def _get_weight_from_block(persisted_block, block): @@ -233,6 +216,23 @@ def _get_explicit_graded(block): return True if field_value is None else field_value +def _weighted_score(raw_earned, raw_possible, weight): + """ + Returns a tuple that represents the weighted (earned, possible) score. + If weight is None or raw_possible is 0, returns the original values. + + When weight is used, it defines the weighted_possible. This allows + course authors to specify the exact maximum value for a problem when + they provide a weight. + """ + assert raw_possible is not None + cannot_compute_with_weight = weight is None or raw_possible == 0 + if cannot_compute_with_weight: + return raw_earned, raw_possible + else: + return float(raw_earned) * weight / raw_possible, float(weight) + + @memoized def _block_types_possibly_scored(): """ diff --git a/lms/djangoapps/grades/tests/test_scores.py b/lms/djangoapps/grades/tests/test_scores.py index f7d6e34055..a4002fbef2 100644 --- a/lms/djangoapps/grades/tests/test_scores.py +++ b/lms/djangoapps/grades/tests/test_scores.py @@ -162,9 +162,9 @@ class TestGetScore(TestCase): @ddt.ddt -class TestWeightedScore(TestCase): +class TestInternalWeightedScore(TestCase): """ - Tests the helper method: weighted_score + Tests the internal helper method: _weighted_score """ @ddt.data( (0, 0, 1), @@ -177,7 +177,7 @@ class TestWeightedScore(TestCase): @ddt.unpack def test_cannot_compute(self, raw_earned, raw_possible, weight): self.assertEquals( - scores.weighted_score(raw_earned, raw_possible, weight), + scores._weighted_score(raw_earned, raw_possible, weight), (raw_earned, raw_possible), ) @@ -192,13 +192,13 @@ class TestWeightedScore(TestCase): @ddt.unpack def test_computed(self, raw_earned, raw_possible, weight, expected_score): self.assertEquals( - scores.weighted_score(raw_earned, raw_possible, weight), + scores._weighted_score(raw_earned, raw_possible, weight), expected_score, ) def test_assert_on_invalid_r_possible(self): with self.assertRaises(AssertionError): - scores.weighted_score(raw_earned=1, raw_possible=None, weight=1) + scores._weighted_score(raw_earned=1, raw_possible=None, weight=1) @ddt.ddt diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index becae8f2ba..3540593483 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -4,7 +4,6 @@ Enrollment operations for use by instructor APIs. Does not include any access control, be sure to check access before calling. """ -import crum import json import logging from django.contrib.auth.models import User @@ -14,14 +13,11 @@ from django.core.mail import send_mail from django.utils.translation import override as override_language from course_modes.models import CourseMode -from courseware.model_data import FieldDataCache -from courseware.module_render import get_module_for_descriptor +from student.models import CourseEnrollment, CourseEnrollmentAllowed from courseware.models import StudentModule from edxmako.shortcuts import render_to_string -from grades.scores import weighted_score -from grades.signals.signals import SCORE_CHANGED from lang_pref import LANGUAGE_KEY -from student.models import CourseEnrollment, CourseEnrollmentAllowed + from submissions import api as sub_api # installed from the edx-submissions repository from student.models import anonymous_id_for_user from openedx.core.djangoapps.user_api.models import UserPreference @@ -249,7 +245,6 @@ def reset_student_attempts(course_id, student, module_state_key, requesting_user ) submission_cleared = True except ItemNotFoundError: - block = None log.warning("Could not find %s in modulestore when attempting to reset attempts.", module_state_key) # Reset the student's score in the submissions API, if xblock.clear_student_state has not done so already. @@ -272,7 +267,6 @@ def reset_student_attempts(course_id, student, module_state_key, requesting_user if delete_module: module_to_reset.delete() - _fire_score_changed_for_block(course_id, student, block, module_state_key) else: _reset_module_attempts(module_to_reset) @@ -293,43 +287,6 @@ def _reset_module_attempts(studentmodule): studentmodule.save() -def _fire_score_changed_for_block(course_id, student, block, module_state_key): - """ - Fires a SCORE_CHANGED event for the given module. The earned points are - always zero. We must retrieve the possible points from the XModule, as - noted below. - """ - if block and block.has_score: - cache = FieldDataCache.cache_for_descriptor_descendents( - course_id=course_id, - user=student, - descriptor=block, - depth=0 - ) - # For implementation reasons, we need to pull the max_score from the XModule, - # even though the data is not user-specific. Here we bind the data to the - # current user. - request = crum.get_current_request() - module = get_module_for_descriptor( - user=student, - request=request, - descriptor=block, - field_data_cache=cache, - course_key=course_id - ) - points_earned, points_possible = weighted_score(0, module.max_score(), getattr(module, 'weight', None)) - else: - points_earned, points_possible = 0, 0 - SCORE_CHANGED.send( - sender=None, - points_possible=points_possible, - points_earned=points_earned, - user=student, - course_id=course_id, - usage_id=module_state_key - ) - - def get_email_params(course, auto_enroll, secure=True, course_key=None, display_name=None): """ Generate parameters used when parsing email templates. diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 0b65f1c8a4..01559dfd8f 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -8,7 +8,6 @@ import mock from mock import patch from abc import ABCMeta from courseware.models import StudentModule -from courseware.tests.helpers import get_request_for_user from django.conf import settings from django.utils.translation import get_language from django.utils.translation import override as override_language @@ -17,11 +16,7 @@ from ccx_keys.locator import CCXLocator from student.tests.factories import UserFactory from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory -from grades.new.subsection_grade import SubsectionGradeFactory -from grades.tests.utils import answer_problem from lms.djangoapps.ccx.tests.factories import CcxFactory -from lms.djangoapps.course_blocks.api import get_course_blocks from openedx.core.djangolib.testing.utils import CacheIsolationTestCase from student.models import CourseEnrollment, CourseEnrollmentAllowed from student.roles import CourseCcxCoachRole @@ -499,80 +494,6 @@ class TestInstructorEnrollmentStudentModule(SharedModuleStoreTestCase): self.assertEqual(unrelated_state['brains'], 'zombie') -class TestStudentModuleGrading(SharedModuleStoreTestCase): - """ - Tests the effects of student module manipulations - on student grades. - """ - @classmethod - def setUpClass(cls): - super(TestStudentModuleGrading, cls).setUpClass() - cls.course = CourseFactory.create() - cls.chapter = ItemFactory.create( - parent=cls.course, - category="chapter", - display_name="Test Chapter" - ) - cls.sequence = ItemFactory.create( - parent=cls.chapter, - category='sequential', - display_name="Test Sequential 1", - graded=True - ) - cls.vertical = ItemFactory.create( - parent=cls.sequence, - category='vertical', - display_name='Test Vertical 1' - ) - problem_xml = MultipleChoiceResponseXMLFactory().build_xml( - question_text='The correct answer is Choice 3', - choices=[False, False, True, False], - choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] - ) - cls.problem = ItemFactory.create( - parent=cls.vertical, - category="problem", - display_name="Test Problem", - data=problem_xml - ) - cls.request = get_request_for_user(UserFactory()) - cls.user = cls.request.user - - def _get_subsection_grade_and_verify(self, all_earned, all_possible, graded_earned, graded_possible): - """ - Retrieves the subsection grade and verifies that - its scores match those expected. - """ - subsection_grade_factory = SubsectionGradeFactory( - self.user, - self.course, - get_course_blocks(self.user, self.course.location) - ) - grade = subsection_grade_factory.update(self.sequence) - self.assertEqual(grade.all_total.earned, all_earned) - self.assertEqual(grade.graded_total.earned, graded_earned) - self.assertEqual(grade.all_total.possible, all_possible) - self.assertEqual(grade.graded_total.possible, graded_possible) - - @patch('crum.get_current_request') - def test_delete_student_state(self, _crum_mock): - problem_location = self.problem.location - self._get_subsection_grade_and_verify(0, 1, 0, 1) - answer_problem(course=self.course, request=self.request, problem=self.problem, score=1, max_value=1) - self._get_subsection_grade_and_verify(1, 1, 1, 1) - - # Delete student state using the instructor dash - reset_student_attempts( - self.course.id, - self.user, - problem_location, - requesting_user=self.user, - delete_module=True, - ) - # Verify that the student's grades are reset - self._get_subsection_grade_and_verify(0, 1, 0, 1) - - class EnrollmentObjects(object): """ Container for enrollment objects. From ca287c73c7d89c9c8702ada83c14e4d7c558ec5e Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 30 Sep 2016 10:06:01 -0400 Subject: [PATCH 02/12] Revert "Merge pull request #13461 from edx/cdyer/test-block-types" This reverts commit c050ccc31839e181a60ac98964c1e38c6c0ffb0c, reversing changes made to 9ba977223940955390937e46eac206860ac543b4. --- common/test/data/blocks/capa.xml | 6 - common/test/data/blocks/coderesponse.xml | 25 --- common/test/data/blocks/library_content.xml | 4 - common/test/data/blocks/lti.xml | 1 - common/test/data/blocks/openassessment.xml | 87 ---------- lms/djangoapps/grades/tests/test_grades.py | 2 +- lms/djangoapps/grades/tests/test_new.py | 155 +----------------- .../__init__.py => xblock_utils.py} | 0 openedx/core/lib/xblock_utils/test_utils.py | 26 --- 9 files changed, 3 insertions(+), 303 deletions(-) delete mode 100644 common/test/data/blocks/capa.xml delete mode 100644 common/test/data/blocks/coderesponse.xml delete mode 100644 common/test/data/blocks/library_content.xml delete mode 100644 common/test/data/blocks/lti.xml delete mode 100644 common/test/data/blocks/openassessment.xml rename openedx/core/lib/{xblock_utils/__init__.py => xblock_utils.py} (100%) delete mode 100644 openedx/core/lib/xblock_utils/test_utils.py diff --git a/common/test/data/blocks/capa.xml b/common/test/data/blocks/capa.xml deleted file mode 100644 index a3460b1ef8..0000000000 --- a/common/test/data/blocks/capa.xml +++ /dev/null @@ -1,6 +0,0 @@ - - - - - - diff --git a/common/test/data/blocks/coderesponse.xml b/common/test/data/blocks/coderesponse.xml deleted file mode 100644 index fde73aa327..0000000000 --- a/common/test/data/blocks/coderesponse.xml +++ /dev/null @@ -1,25 +0,0 @@ - - -

- ESTIMATED TIME TO COMPLETE: 4 minutes -

- 
-      >>> print testList
-      [1, 16, 64, 81]
-
- - - - - -# Your Code Here - - -def square(a): - return a * a -applyToEach(testList, square) - - {"grader": "finger_exercises/L6/applyToEach3/grade_ate3.py"} - - - diff --git a/common/test/data/blocks/library_content.xml b/common/test/data/blocks/library_content.xml deleted file mode 100644 index 6c8fc8105b..0000000000 --- a/common/test/data/blocks/library_content.xml +++ /dev/null @@ -1,4 +0,0 @@ - - - - diff --git a/common/test/data/blocks/lti.xml b/common/test/data/blocks/lti.xml deleted file mode 100644 index ae8bed35c1..0000000000 --- a/common/test/data/blocks/lti.xml +++ /dev/null @@ -1 +0,0 @@ - diff --git a/common/test/data/blocks/openassessment.xml b/common/test/data/blocks/openassessment.xml deleted file mode 100644 index 12b0919ba6..0000000000 --- a/common/test/data/blocks/openassessment.xml +++ /dev/null @@ -1,87 +0,0 @@ - - - - - - - - - Censorship in the Libraries - - 'All of us can think of a book that we hope none of our children or any - other children have taken off the shelf. But if I have the right to remove - that book from the shelf -- that work I abhor -- then you also have exactly - the same right and so does everyone else. And then we have no books left on - the shelf for any of us.' --Katherine Paterson, Author - - Write a persuasive essay to a newspaper reflecting your views on censorship - in libraries. Do you believe that certain materials, such as books, music, - movies, magazines, etc., should be removed from the shelves if they are - found offensive? Support your position with convincing arguments from your - own experience, observations, and/or reading. - - Read for conciseness, clarity of thought, and form. - - - Ideas - Determine if there is a unifying theme or main idea. - - - - - - Content - Assess the content of the submission - - - - - - - diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index 038fff9147..0b493cf812 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -15,10 +15,10 @@ from lms.djangoapps.course_blocks.api import get_course_blocks from student.tests.factories import UserFactory from student.models import CourseEnrollment from xmodule.block_metadata_utils import display_name_with_default_escaped -from xmodule.graders import ProblemScore from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.graders import ProblemScore from .utils import answer_problem from .. import course_grades diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 4cecb8ee73..25402508c6 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -2,30 +2,24 @@ Test saved subsection grade functionality. """ # pylint: disable=protected-access - -import datetime - import ddt from django.conf import settings from django.db.utils import DatabaseError from mock import patch -import pytz from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from courseware.tests.helpers import get_request_for_user -from courseware.tests.test_submitting_problems import ProblemSubmissionTestMixin from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags -from openedx.core.lib.xblock_utils.test_utils import add_xml_block_from_file from student.models import CourseEnrollment from student.tests.factories import UserFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from ..models import PersistentSubsectionGrade from ..new.course_grade import CourseGradeFactory from ..new.subsection_grade import SubsectionGrade, SubsectionGradeFactory -from .utils import mock_get_score +from lms.djangoapps.grades.tests.utils import mock_get_score class GradeTestBase(SharedModuleStoreTestCase): @@ -236,148 +230,3 @@ class SubsectionGradeTest(GradeTestBase): self.assertEqual(input_grade.url_name, loaded_grade.url_name) self.assertEqual(input_grade.all_total, loaded_grade.all_total) - - -@ddt.ddt -class TestMultipleProblemTypesSubsectionScores(ModuleStoreTestCase, ProblemSubmissionTestMixin): - """ - Test grading of different problem types. - """ - - default_problem_metadata = { - u'graded': True, - u'weight': 2.5, - u'max_score': 7.0, - u'due': datetime.datetime(2099, 3, 15, 12, 30, 0, tzinfo=pytz.utc), - } - - COURSE_NAME = u'Problem Type Test Course' - COURSE_NUM = u'probtype' - - def setUp(self): - super(TestMultipleProblemTypesSubsectionScores, self).setUp() - password = u'test' - self.student = UserFactory.create(is_staff=False, username=u'test_student', password=password) - self.client.login(username=self.student.username, password=password) - self.request = get_request_for_user(self.student) - self.course = CourseFactory.create( - display_name=self.COURSE_NAME, - number=self.COURSE_NUM - ) - self.chapter = ItemFactory.create( - parent=self.course, - category=u'chapter', - display_name=u'Test Chapter' - ) - self.seq1 = ItemFactory.create( - parent=self.chapter, - category=u'sequential', - display_name=u'Test Sequential 1', - graded=True - ) - self.vert1 = ItemFactory.create( - parent=self.seq1, - category=u'vertical', - display_name=u'Test Vertical 1' - ) - - def _get_fresh_subsection_score(self, course_structure, subsection): - """ - Return a Score object for the specified subsection. - - Ensures that a stale cached value is not returned. - """ - subsection_factory = SubsectionGradeFactory( - self.student, - course_structure=course_structure, - course=self.course, - ) - return subsection_factory.update(subsection) - - def _get_altered_metadata(self, alterations): - """ - Returns a copy of the default_problem_metadata dict updated with the - specified alterations. - """ - metadata = self.default_problem_metadata.copy() - metadata.update(alterations) - return metadata - - def _get_score_with_alterations(self, alterations): - """ - Given a dict of alterations to the default_problem_metadata, return - the score when one correct problem (out of two) is submitted. - """ - metadata = self._get_altered_metadata(alterations) - - add_xml_block_from_file(u'problem', u'capa.xml', parent=self.vert1, metadata=metadata) - course_structure = get_course_blocks(self.student, self.course.location) - - self.submit_question_answer(u'problem', {u'2_1': u'Correct'}) - return self._get_fresh_subsection_score(course_structure, self.seq1) - - def test_score_submission_for_capa_problems(self): - add_xml_block_from_file(u'problem', u'capa.xml', parent=self.vert1, metadata=self.default_problem_metadata) - course_structure = get_course_blocks(self.student, self.course.location) - - score = self._get_fresh_subsection_score(course_structure, self.seq1) - self.assertEqual(score.all_total.earned, 0.0) - self.assertEqual(score.all_total.possible, 2.5) - - self.submit_question_answer(u'problem', {u'2_1': u'Correct'}) - score = self._get_fresh_subsection_score(course_structure, self.seq1) - self.assertEqual(score.all_total.earned, 1.25) - self.assertEqual(score.all_total.possible, 2.5) - - @ddt.data( - (u'openassessment', u'openassessment.xml'), - (u'coderesponse', u'coderesponse.xml'), - (u'lti', u'lti.xml'), - (u'library_content', u'library_content.xml'), - ) - @ddt.unpack - def test_loading_different_problem_types(self, block_type, filename): - """ - Test that transformation works for various block types - """ - metadata = self.default_problem_metadata.copy() - if block_type == u'library_content': - # Library content does not have a weight - del metadata[u'weight'] - add_xml_block_from_file(block_type, filename, parent=self.vert1, metadata=metadata) - - @ddt.data( - ({}, 1.25, 2.5), - ({u'weight': 27}, 13.5, 27), - ({u'weight': 1.0}, 0.5, 1.0), - ({u'weight': 0.0}, 0.0, 0.0), - ({u'weight': None}, 1.0, 2.0), - ) - @ddt.unpack - def test_weight_metadata_alterations(self, alterations, expected_earned, expected_possible): - score = self._get_score_with_alterations(alterations) - self.assertEqual(score.all_total.earned, expected_earned) - self.assertEqual(score.all_total.possible, expected_possible) - - @ddt.data( - ({u'graded': True}, 1.25, 2.5), - ({u'graded': False}, 0.0, 0.0), - ) - @ddt.unpack - def test_graded_metadata_alterations(self, alterations, expected_earned, expected_possible): - score = self._get_score_with_alterations(alterations) - self.assertEqual(score.graded_total.earned, expected_earned) - self.assertEqual(score.graded_total.possible, expected_possible) - - @ddt.data( - {u'max_score': 99.3}, - {u'max_score': 1.0}, - {u'max_score': 0.0}, - {u'max_score': None}, - ) - def test_max_score_does_not_change_results(self, alterations): - expected_earned = 1.25 - expected_possible = 2.5 - score = self._get_score_with_alterations(alterations) - self.assertEqual(score.all_total.earned, expected_earned) - self.assertEqual(score.all_total.possible, expected_possible) diff --git a/openedx/core/lib/xblock_utils/__init__.py b/openedx/core/lib/xblock_utils.py similarity index 100% rename from openedx/core/lib/xblock_utils/__init__.py rename to openedx/core/lib/xblock_utils.py diff --git a/openedx/core/lib/xblock_utils/test_utils.py b/openedx/core/lib/xblock_utils/test_utils.py deleted file mode 100644 index ac1b145dc0..0000000000 --- a/openedx/core/lib/xblock_utils/test_utils.py +++ /dev/null @@ -1,26 +0,0 @@ -""" -Utilities for testing xblocks -""" - -from django.conf import settings - -from xmodule.modulestore.tests.factories import ItemFactory - -TEST_DATA_DIR = settings.COMMON_ROOT / u'test/data' - - -def add_xml_block_from_file(block_type, filename, parent, metadata): - """ - Create a block of the specified type with content included from the - specified XML file. - - XML filenames are relative to common/test/data/blocks. - """ - with open(TEST_DATA_DIR / u'blocks' / filename) as datafile: - return ItemFactory.create( - parent=parent, - category=block_type, - data=datafile.read().decode('utf-8'), - metadata=metadata, - display_name=u'problem' - ) From ac2f71df9936348b51a12096108c76d97f25f311 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 30 Sep 2016 10:06:10 -0400 Subject: [PATCH 03/12] Revert "Merge pull request #13547 from edx/beryl/correct_weight" This reverts commit 663fbbbaa070dba33a3df3f3395079f7df74e4a3, reversing changes made to 3f7441c80fb5ddc7fc8cd7673a7adc3f8631a5f4. --- common/lib/xmodule/xmodule/graders.py | 79 +---- .../lib/xmodule/xmodule/tests/test_graders.py | 163 ++++----- lms/djangoapps/gating/tests/test_api.py | 4 +- lms/djangoapps/grades/models.py | 5 +- lms/djangoapps/grades/new/course_grade.py | 16 +- lms/djangoapps/grades/new/subsection_grade.py | 119 +++++-- lms/djangoapps/grades/scores.py | 308 +++++------------- lms/djangoapps/grades/tests/test_grades.py | 119 +------ lms/djangoapps/grades/tests/test_models.py | 29 +- lms/djangoapps/grades/tests/test_new.py | 12 +- lms/djangoapps/grades/tests/test_scores.py | 288 ---------------- lms/djangoapps/grades/tests/utils.py | 3 +- .../instructor_task/tasks_helper.py | 2 +- 13 files changed, 277 insertions(+), 870 deletions(-) delete mode 100644 lms/djangoapps/grades/tests/test_scores.py diff --git a/common/lib/xmodule/xmodule/graders.py b/common/lib/xmodule/xmodule/graders.py index 09caefe878..c84699df5d 100644 --- a/common/lib/xmodule/xmodule/graders.py +++ b/common/lib/xmodule/xmodule/graders.py @@ -10,67 +10,13 @@ import logging import random import sys +from collections import namedtuple log = logging.getLogger("edx.courseware") - -class ScoreBase(object): - """ - Abstract base class for encapsulating fields of values scores. - Field common to all scores include: - display_name (string) - the display name of the module - module_id (UsageKey) - the location of the module - graded (boolean) - whether or not this module is graded - """ - __metaclass__ = abc.ABCMeta - - def __init__(self, graded, display_name, module_id): - self.graded = graded - self.display_name = display_name - self.module_id = module_id - - def __eq__(self, other): - if type(other) is type(self): - return self.__dict__ == other.__dict__ - return False - - def __ne__(self, other): - return not self.__eq__(other) - - def __repr__(self): - return u"{class_name}({fields})".format(class_name=self.__class__.__name__, fields=self.__dict__) - - -class ProblemScore(ScoreBase): - """ - Encapsulates the fields of a Problem's score. - In addition to the fields in ScoreBase, also includes: - raw_earned (float) - raw points earned on this problem - raw_possible (float) - raw points possible to earn on this problem - weighted_earned = earned (float) - weighted value of the points earned - weighted_possible = possible (float) - weighted possible points on this problem - weight (float) - weight of this problem - """ - def __init__(self, raw_earned, raw_possible, weighted_earned, weighted_possible, weight, *args, **kwargs): - super(ProblemScore, self).__init__(*args, **kwargs) - self.raw_earned = raw_earned - self.raw_possible = raw_possible - self.earned = weighted_earned - self.possible = weighted_possible - self.weight = weight - - -class AggregatedScore(ScoreBase): - """ - Encapsulates the fields of a Subsection's score. - In addition to the fields in ScoreBase, also includes: - tw_earned = earned - total aggregated sum of all weighted earned values - tw_possible = possible - total aggregated sum of all weighted possible values - """ - def __init__(self, tw_earned, tw_possible, *args, **kwargs): - super(AggregatedScore, self).__init__(*args, **kwargs) - self.earned = tw_earned - self.possible = tw_possible +# This is a tuple for holding scores, either from problems or sections. +# Section either indicates the name of the problem or the name of the section +Score = namedtuple("Score", "earned possible graded section module_id") def float_sum(iterable): @@ -80,14 +26,13 @@ def float_sum(iterable): return float(sum(iterable)) -def aggregate_scores(scores, display_name="summary", location=None): +def aggregate_scores(scores, section_name="summary", location=None): """ - scores: A list of ScoreBase objects - display_name: The display name for the score object + scores: A list of Score objects location: The location under which all objects in scores are located returns: A tuple (all_total, graded_total). - all_total: A ScoreBase representing the total score summed over all input scores - graded_total: A ScoreBase representing the score summed over all graded input scores + all_total: A Score representing the total score summed over all input scores + graded_total: A Score representing the score summed over all graded input scores """ total_correct_graded = float_sum(score.earned for score in scores if score.graded) total_possible_graded = float_sum(score.possible for score in scores if score.graded) @@ -96,10 +41,10 @@ def aggregate_scores(scores, display_name="summary", location=None): total_possible = float_sum(score.possible for score in scores) #regardless of whether it is graded - all_total = AggregatedScore(total_correct, total_possible, False, display_name, location) + all_total = Score(total_correct, total_possible, False, section_name, location) #selecting only graded things - graded_total = AggregatedScore(total_correct_graded, total_possible_graded, True, display_name, location) + graded_total = Score(total_correct_graded, total_possible_graded, True, section_name, location) return all_total, graded_total @@ -275,7 +220,7 @@ class SingleSectionGrader(CourseGrader): found_score = None if self.type in grade_sheet: for score in grade_sheet[self.type]: - if score.display_name == self.name: + if score.section == self.name: found_score = score break @@ -397,7 +342,7 @@ class AssignmentFormatGrader(CourseGrader): else: earned = scores[i].earned possible = scores[i].possible - section_name = scores[i].display_name + section_name = scores[i].section percentage = earned / possible summary_format = u"{section_type} {index} - {name} - {percent:.0%} ({earned:.3n}/{possible:.3n})" diff --git a/common/lib/xmodule/xmodule/tests/test_graders.py b/common/lib/xmodule/xmodule/tests/test_graders.py index 341146dc23..d3e73fcc54 100644 --- a/common/lib/xmodule/xmodule/tests/test_graders.py +++ b/common/lib/xmodule/xmodule/tests/test_graders.py @@ -2,67 +2,42 @@ import unittest from xmodule import graders -from xmodule.graders import ProblemScore, AggregatedScore, aggregate_scores +from xmodule.graders import Score, aggregate_scores class GradesheetTest(unittest.TestCase): - """ - Tests the aggregate_scores method - """ + '''Tests the aggregate_scores method''' def test_weighted_grading(self): scores = [] - agg_fields = dict(display_name="aggregated_score", module_id=None) - prob_fields = dict(display_name="problem_score", module_id=None, raw_earned=0, raw_possible=0, weight=0) + Score.__sub__ = lambda me, other: (me.earned - other.earned) + (me.possible - other.possible) - all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) - self.assertEqual( - all_total, - AggregatedScore(tw_earned=0, tw_possible=0, graded=False, **agg_fields), - ) - self.assertEqual( - graded_total, - AggregatedScore(tw_earned=0, tw_possible=0, graded=True, **agg_fields), + all_total, graded_total = aggregate_scores(scores) + self.assertEqual(all_total, Score(earned=0, possible=0, graded=False, section="summary", module_id=None)) + self.assertEqual(graded_total, Score(earned=0, possible=0, graded=True, section="summary", module_id=None)) + + scores.append(Score(earned=0, possible=5, graded=False, section="summary", module_id=None)) + all_total, graded_total = aggregate_scores(scores) + self.assertEqual(all_total, Score(earned=0, possible=5, graded=False, section="summary", module_id=None)) + self.assertEqual(graded_total, Score(earned=0, possible=0, graded=True, section="summary", module_id=None)) + + scores.append(Score(earned=3, possible=5, graded=True, section="summary", module_id=None)) + all_total, graded_total = aggregate_scores(scores) + self.assertAlmostEqual(all_total, Score(earned=3, possible=10, graded=False, section="summary", module_id=None)) + self.assertAlmostEqual( + graded_total, Score(earned=3, possible=5, graded=True, section="summary", module_id=None) ) - scores.append(ProblemScore(weighted_earned=0, weighted_possible=5, graded=False, **prob_fields)) - all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) - self.assertEqual( - all_total, - AggregatedScore(tw_earned=0, tw_possible=5, graded=False, **agg_fields), - ) - self.assertEqual( - graded_total, - AggregatedScore(tw_earned=0, tw_possible=0, graded=True, **agg_fields), - ) - - scores.append(ProblemScore(weighted_earned=3, weighted_possible=5, graded=True, **prob_fields)) - all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) + scores.append(Score(earned=2, possible=5, graded=True, section="summary", module_id=None)) + all_total, graded_total = aggregate_scores(scores) + self.assertAlmostEqual(all_total, Score(earned=5, possible=15, graded=False, section="summary", module_id=None)) self.assertAlmostEqual( - all_total, - AggregatedScore(tw_earned=3, tw_possible=10, graded=False, **agg_fields), - ) - self.assertAlmostEqual( - graded_total, - AggregatedScore(tw_earned=3, tw_possible=5, graded=True, **agg_fields), - ) - - scores.append(ProblemScore(weighted_earned=2, weighted_possible=5, graded=True, **prob_fields)) - all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) - self.assertAlmostEqual( - all_total, - AggregatedScore(tw_earned=5, tw_possible=15, graded=False, **agg_fields), - ) - self.assertAlmostEqual( - graded_total, - AggregatedScore(tw_earned=5, tw_possible=10, graded=True, **agg_fields), + graded_total, Score(earned=5, possible=10, graded=True, section="summary", module_id=None) ) class GraderTest(unittest.TestCase): - """ - Tests grader implementations - """ + '''Tests grader implementations''' empty_gradesheet = { } @@ -74,25 +49,19 @@ class GraderTest(unittest.TestCase): } test_gradesheet = { - 'Homework': [ - AggregatedScore(tw_earned=2, tw_possible=20.0, graded=True, display_name='hw1', module_id=None), - AggregatedScore(tw_earned=16, tw_possible=16.0, graded=True, display_name='hw2', module_id=None) - ], - + 'Homework': [Score(earned=2, possible=20.0, graded=True, section='hw1', module_id=None), + Score(earned=16, possible=16.0, graded=True, section='hw2', module_id=None)], # The dropped scores should be from the assignments that don't exist yet - 'Lab': [ - AggregatedScore(tw_earned=1, tw_possible=2.0, graded=True, display_name='lab1', module_id=None), # Dropped - AggregatedScore(tw_earned=1, tw_possible=1.0, graded=True, display_name='lab2', module_id=None), - AggregatedScore(tw_earned=1, tw_possible=1.0, graded=True, display_name='lab3', module_id=None), - AggregatedScore(tw_earned=5, tw_possible=25.0, graded=True, display_name='lab4', module_id=None), # Dropped - AggregatedScore(tw_earned=3, tw_possible=4.0, graded=True, display_name='lab5', module_id=None), # Dropped - AggregatedScore(tw_earned=6, tw_possible=7.0, graded=True, display_name='lab6', module_id=None), - AggregatedScore(tw_earned=5, tw_possible=6.0, graded=True, display_name='lab7', module_id=None), - ], - 'Midterm': [ - AggregatedScore(tw_earned=50.5, tw_possible=100, graded=True, display_name="Midterm Exam", module_id=None), - ], + 'Lab': [Score(earned=1, possible=2.0, graded=True, section='lab1', module_id=None), # Dropped + Score(earned=1, possible=1.0, graded=True, section='lab2', module_id=None), + Score(earned=1, possible=1.0, graded=True, section='lab3', module_id=None), + Score(earned=5, possible=25.0, graded=True, section='lab4', module_id=None), # Dropped + Score(earned=3, possible=4.0, graded=True, section='lab5', module_id=None), # Dropped + Score(earned=6, possible=7.0, graded=True, section='lab6', module_id=None), + Score(earned=5, possible=6.0, graded=True, section='lab7', module_id=None)], + + 'Midterm': [Score(earned=50.5, possible=100, graded=True, section="Midterm Exam", module_id=None), ], } def test_single_section_grader(self): @@ -100,11 +69,9 @@ class GraderTest(unittest.TestCase): lab4_grader = graders.SingleSectionGrader("Lab", "lab4") bad_lab_grader = graders.SingleSectionGrader("Lab", "lab42") - for graded in [ - midterm_grader.grade(self.empty_gradesheet), - midterm_grader.grade(self.incomplete_gradesheet), - bad_lab_grader.grade(self.test_gradesheet), - ]: + for graded in [midterm_grader.grade(self.empty_gradesheet), + midterm_grader.grade(self.incomplete_gradesheet), + bad_lab_grader.grade(self.test_gradesheet)]: self.assertEqual(len(graded['section_breakdown']), 1) self.assertEqual(graded['percent'], 0.0) @@ -124,12 +91,10 @@ class GraderTest(unittest.TestCase): lab_grader = graders.AssignmentFormatGrader("Lab", 7, 3) # Test the grading of an empty gradesheet - for graded in [ - homework_grader.grade(self.empty_gradesheet), - no_drop_grader.grade(self.empty_gradesheet), - homework_grader.grade(self.incomplete_gradesheet), - no_drop_grader.grade(self.incomplete_gradesheet), - ]: + for graded in [homework_grader.grade(self.empty_gradesheet), + no_drop_grader.grade(self.empty_gradesheet), + homework_grader.grade(self.incomplete_gradesheet), + no_drop_grader.grade(self.incomplete_gradesheet)]: self.assertAlmostEqual(graded['percent'], 0.0) # Make sure the breakdown includes 12 sections, plus one summary self.assertEqual(len(graded['section_breakdown']), 12 + 1) @@ -153,10 +118,8 @@ class GraderTest(unittest.TestCase): def test_assignment_format_grader_on_single_section_entry(self): midterm_grader = graders.AssignmentFormatGrader("Midterm", 1, 0) # Test the grading on a section with one item: - for graded in [ - midterm_grader.grade(self.empty_gradesheet), - midterm_grader.grade(self.incomplete_gradesheet), - ]: + for graded in [midterm_grader.grade(self.empty_gradesheet), + midterm_grader.grade(self.incomplete_gradesheet)]: self.assertAlmostEqual(graded['percent'], 0.0) # Make sure the breakdown includes just the one summary self.assertEqual(len(graded['section_breakdown']), 0 + 1) @@ -174,31 +137,23 @@ class GraderTest(unittest.TestCase): # will act like SingleSectionGraders on single sections. midterm_grader = graders.AssignmentFormatGrader("Midterm", 1, 0) - weighted_grader = graders.WeightedSubsectionsGrader([ - (homework_grader, homework_grader.category, 0.25), - (lab_grader, lab_grader.category, 0.25), - (midterm_grader, midterm_grader.category, 0.5), - ]) + weighted_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.25), + (lab_grader, lab_grader.category, 0.25), + (midterm_grader, midterm_grader.category, 0.5)]) - over_one_weights_grader = graders.WeightedSubsectionsGrader([ - (homework_grader, homework_grader.category, 0.5), - (lab_grader, lab_grader.category, 0.5), - (midterm_grader, midterm_grader.category, 0.5), - ]) + over_one_weights_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.5), + (lab_grader, lab_grader.category, 0.5), + (midterm_grader, midterm_grader.category, 0.5)]) # The midterm should have all weight on this one - zero_weights_grader = graders.WeightedSubsectionsGrader([ - (homework_grader, homework_grader.category, 0.0), - (lab_grader, lab_grader.category, 0.0), - (midterm_grader, midterm_grader.category, 0.5), - ]) + zero_weights_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.0), + (lab_grader, lab_grader.category, 0.0), + (midterm_grader, midterm_grader.category, 0.5)]) # This should always have a final percent of zero - all_zero_weights_grader = graders.WeightedSubsectionsGrader([ - (homework_grader, homework_grader.category, 0.0), - (lab_grader, lab_grader.category, 0.0), - (midterm_grader, midterm_grader.category, 0.0), - ]) + all_zero_weights_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.0), + (lab_grader, lab_grader.category, 0.0), + (midterm_grader, midterm_grader.category, 0.0)]) empty_grader = graders.WeightedSubsectionsGrader([]) @@ -222,12 +177,10 @@ class GraderTest(unittest.TestCase): self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1) self.assertEqual(len(graded['grade_breakdown']), 3) - for graded in [ - weighted_grader.grade(self.empty_gradesheet), - weighted_grader.grade(self.incomplete_gradesheet), - zero_weights_grader.grade(self.empty_gradesheet), - all_zero_weights_grader.grade(self.empty_gradesheet), - ]: + for graded in [weighted_grader.grade(self.empty_gradesheet), + weighted_grader.grade(self.incomplete_gradesheet), + zero_weights_grader.grade(self.empty_gradesheet), + all_zero_weights_grader.grade(self.empty_gradesheet)]: self.assertAlmostEqual(graded['percent'], 0.0) self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1) self.assertEqual(len(graded['grade_breakdown']), 3) diff --git a/lms/djangoapps/gating/tests/test_api.py b/lms/djangoapps/gating/tests/test_api.py index 5e63cf9ef3..a217091582 100644 --- a/lms/djangoapps/gating/tests/test_api.py +++ b/lms/djangoapps/gating/tests/test_api.py @@ -166,8 +166,8 @@ class TestGatedContent(GatingTestCase, MilestonesTestCaseMixin): """ course_grade = CourseGradeFactory(user).create(self.course) for prob in [self.gating_prob1, self.gated_prob2, self.prob3]: - self.assertIn(prob.location, course_grade.locations_to_scores) - self.assertNotIn(self.orphan.location, course_grade.locations_to_scores) + self.assertIn(prob.location, course_grade.locations_to_weighted_scores) + self.assertNotIn(self.orphan.location, course_grade.locations_to_weighted_scores) self.assertEquals(course_grade.percent, expected_percent) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 19241ad08d..eabf9037ed 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -27,7 +27,7 @@ BLOCK_RECORD_LIST_VERSION = 1 # Used to serialize information about a block at the time it was used in # grade calculation. -BlockRecord = namedtuple('BlockRecord', ['locator', 'weight', 'raw_possible', 'graded']) +BlockRecord = namedtuple('BlockRecord', ['locator', 'weight', 'max_score']) class BlockRecordList(tuple): @@ -98,8 +98,7 @@ class BlockRecordList(tuple): BlockRecord( locator=UsageKey.from_string(block["locator"]).replace(course_key=course_key), weight=block["weight"], - raw_possible=block["raw_possible"], - graded=block["graded"], + max_score=block["max_score"], ) for block in block_dicts ) diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index 30712e7202..5fd1a7692c 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/lms/djangoapps/grades/new/course_grade.py @@ -43,15 +43,15 @@ class CourseGrade(object): return subsections_by_format @lazy - def locations_to_scores(self): + def locations_to_weighted_scores(self): """ Returns a dict of problem scores keyed by their locations. """ - locations_to_scores = {} + locations_to_weighted_scores = {} for chapter in self.chapter_grades: for subsection_grade in chapter['sections']: - locations_to_scores.update(subsection_grade.locations_to_scores) - return locations_to_scores + locations_to_weighted_scores.update(subsection_grade.locations_to_weighted_scores) + return locations_to_weighted_scores @lazy def grade_value(self): @@ -113,7 +113,7 @@ class CourseGrade(object): grade_summary['percent'] = self.percent grade_summary['grade'] = self.letter_grade grade_summary['totaled_scores'] = self.subsection_grade_totals_by_format - grade_summary['raw_scores'] = list(self.locations_to_scores.itervalues()) + grade_summary['raw_scores'] = list(self.locations_to_weighted_scores.itervalues()) return grade_summary @@ -141,7 +141,7 @@ class CourseGrade(object): subsections_total = sum(len(x) for x in self.subsection_grade_totals_by_format.itervalues()) subsections_read = len(subsection_grade_factory._unsaved_subsection_grades) # pylint: disable=protected-access subsections_created = subsections_total - subsections_read - blocks_total = len(self.locations_to_scores) + blocks_total = len(self.locations_to_weighted_scores) if not read_only: subsection_grade_factory.bulk_create_unsaved() @@ -166,8 +166,8 @@ class CourseGrade(object): composite module (a vertical or section ) the scores will be the sums of all scored problems that are children of the chosen location. """ - if location in self.locations_to_scores: - score = self.locations_to_scores[location] + if location in self.locations_to_weighted_scores: + score, _ = self.locations_to_weighted_scores[location] return score.earned, score.possible children = self.course_structure.get_children(location) earned = 0.0 diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 8f722523ec..b4ae910618 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -11,11 +11,12 @@ 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 lms.djangoapps.grades.transformer import GradesTransformer from student.models import anonymous_id_for_user, User from submissions import api as submissions_api from traceback import format_exc from xmodule import block_metadata_utils, graders -from xmodule.graders import AggregatedScore +from xmodule.graders import Score log = getLogger(__name__) @@ -53,47 +54,62 @@ class SubsectionGrade(object): self.graded_total = None # aggregated grade for all graded problems self.all_total = None # aggregated grade for all problems, regardless of whether they are graded - self.locations_to_scores = OrderedDict() # dict of problem locations to ProblemScore + self.locations_to_weighted_scores = OrderedDict() # dict of problem locations to (Score, weight) tuples + + self._scores = None @property def scores(self): """ List of all problem scores in the subsection. """ - return self.locations_to_scores.values() + if self._scores is None: + self._scores = [score for score, _ in self.locations_to_weighted_scores.itervalues()] + return self._scores - def init_from_structure(self, student, course_structure, submissions_scores, csm_scores): + def init_from_structure(self, student, course_structure, scores_client, submissions_scores): """ Compute the grade of this subsection for the given student and course. """ + assert self._scores is None for descendant_key in course_structure.post_order_traversal( filter_func=possibly_scored, start_node=self.location, ): - self._compute_block_score(descendant_key, course_structure, submissions_scores, csm_scores) - + self._compute_block_score( + student, descendant_key, course_structure, scores_client, submissions_scores, persisted_values={}, + ) 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) - def init_from_model(self, student, model, course_structure, submissions_scores, csm_scores): + def init_from_model(self, student, model, course_structure, scores_client, submissions_scores): """ Load the subsection grade from the persisted model. """ + assert self._scores is None for block in model.visible_blocks.blocks: - self._compute_block_score(block.locator, course_structure, submissions_scores, csm_scores, block) + persisted_values = {'weight': block.weight, 'possible': block.max_score} + self._compute_block_score( + student, + block.locator, + course_structure, + scores_client, + submissions_scores, + persisted_values + ) - self.graded_total = AggregatedScore( - tw_earned=model.earned_graded, - tw_possible=model.possible_graded, + self.graded_total = Score( + earned=model.earned_graded, + possible=model.possible_graded, graded=True, - display_name=self.display_name, + section=self.display_name, module_id=self.location, ) - self.all_total = AggregatedScore( - tw_earned=model.earned_all, - tw_possible=model.possible_all, + self.all_total = Score( + earned=model.earned_all, + possible=model.possible_all, graded=False, - display_name=self.display_name, + section=self.display_name, module_id=self.location, ) self._log_event(log.debug, u"init_from_model", student) @@ -124,11 +140,12 @@ class SubsectionGrade(object): def _compute_block_score( self, + student, block_key, course_structure, + scores_client, submissions_scores, - csm_scores, - persisted_block=None, + persisted_values, ): """ Compute score for the given block. If persisted_values @@ -137,14 +154,54 @@ class SubsectionGrade(object): block = course_structure[block_key] if getattr(block, 'has_score', False): - problem_score = get_score( - submissions_scores, - csm_scores, - persisted_block, + + possible = persisted_values.get('possible', None) + weight = persisted_values.get('weight', getattr(block, 'weight', None)) + + (earned, possible) = get_score( + student, block, + scores_client, + submissions_scores, + weight, + possible, ) - if problem_score: - self.locations_to_scores[block_key] = problem_score + + if earned is not None or possible is not None: + # There's a chance that the value of graded is not the same + # value when the problem was scored. Since we get the value + # from the block_structure. + # + # Cannot grade a problem with a denominator of 0. + # TODO: None > 0 is not python 3 compatible. + block_graded = self._get_explicit_graded(block, course_structure) if possible > 0 else False + + self.locations_to_weighted_scores[block.location] = ( + Score( + earned, + possible, + block_graded, + block_metadata_utils.display_name_with_default_escaped(block), + block.location, + ), + weight, + ) + + def _get_explicit_graded(self, block, course_structure): + """ + Returns the explicit graded field value for the given block + """ + field_value = course_structure.get_transformer_block_field( + block.location, + GradesTransformer, + GradesTransformer.EXPLICIT_GRADED_FIELD_NAME + ) + + # Set to True if grading is not explicitly disabled for + # this block. This allows us to include the block's score + # in the aggregated self.graded_total, regardless of the + # inherited graded value from the subsection. (TNL-5560) + return True if field_value is None else field_value def _persisted_model_params(self, student): """ @@ -169,9 +226,9 @@ class SubsectionGrade(object): Returns the list of visible blocks. """ return [ - BlockRecord(location, score.weight, score.raw_possible, score.graded) - for location, score in - self.locations_to_scores.iteritems() + BlockRecord(location, weight, score.possible) + for location, (score, weight) in + self.locations_to_weighted_scores.iteritems() ] def _log_event(self, log_func, log_statement, student): @@ -226,7 +283,7 @@ class SubsectionGradeFactory(object): if not subsection_grade: subsection_grade = SubsectionGrade(subsection, self.course) subsection_grade.init_from_structure( - self.student, block_structure, self._submissions_scores, self._csm_scores, + self.student, block_structure, self._scores_client, self._submissions_scores ) if PersistentGradesEnabledFlag.feature_enabled(self.course.id): if read_only: @@ -256,7 +313,7 @@ class SubsectionGradeFactory(object): 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 + self.student, block_structure, self._scores_client, self._submissions_scores ) if PersistentGradesEnabledFlag.feature_enabled(self.course.id): @@ -266,7 +323,7 @@ class SubsectionGradeFactory(object): return subsection_grade @lazy - def _csm_scores(self): + def _scores_client(self): """ Lazily queries and returns all the scores stored in the user state (in CSM) for the course, while caching the result. @@ -294,7 +351,7 @@ class SubsectionGradeFactory(object): 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, + self.student, saved_subsection_grade, block_structure, self._scores_client, self._submissions_scores ) return subsection_grade diff --git a/lms/djangoapps/grades/scores.py b/lms/djangoapps/grades/scores.py index 27abc562cf..d711ce2854 100644 --- a/lms/djangoapps/grades/scores.py +++ b/lms/djangoapps/grades/scores.py @@ -5,236 +5,14 @@ from logging import getLogger from openedx.core.lib.cache_utils import memoized from xblock.core import XBlock -from xmodule.block_metadata_utils import display_name_with_default_escaped -from xmodule.graders import ProblemScore from .transformer import GradesTransformer log = getLogger(__name__) -def possibly_scored(usage_key): - """ - Returns whether the given block could impact grading (i.e. - has_score or has_children). - """ - return usage_key.block_type in _block_types_possibly_scored() - - -def get_score(submissions_scores, csm_scores, persisted_block, block): - """ - Returns the score for a problem, as a ProblemScore object. It is - assumed that the provided storages have already been filtered for - a single user in question and have user-specific values. - - The score is retrieved from the provided storages in the following - order of precedence. If no value for the block is found in a - given storage, the next storage is checked. - - submissions_scores (dict of {unicode(usage_key): (earned, possible)}): - - A python dictionary of serialized UsageKeys to (earned, possible) - tuples. These values, retrieved using the Submissions API by the - caller (already filtered for the user and course), take precedence - above all other score storages. - - When the score is found in this storage, it implies the user's score - for the block was persisted via the submissions API. Typically, this API - is used by ORA. - - The returned score includes valid values for: - weighted_earned - weighted_possible - graded - retrieved from the persisted block, if found, else from - the latest block content. - - Note: raw_earned and raw_possible are not required when submitting scores - via the submissions API, so those values (along with the unused weight) - are invalid and irrelevant. - - csm_scores (ScoresClient): - - The ScoresClient object (already filtered for the user and course), - from which a courseware.models.StudentModule object can be retrieved for - the block. - - When the score is found from this storage, it implies the user's score - for the block was persisted in the Courseware Student Module. Typically, - this storage is used for all CAPA problems, including scores calculated - by external graders. - - The returned score includes valid values for: - raw_earned, raw_possible - retrieved from CSM - weighted_earned, weighted_possible - calculated from the raw scores and weight - weight, graded - retrieved from the persisted block, if found, - else from the latest block content - - persisted_block (.models.BlockRecord): - The block values as found in the grades persistence layer. These values - are used only if not found from an earlier storage, and take precedence - over values stored within the latest content-version of the block. - - When the score is found from this storage, it implies the user has not - yet attempted this problem, but the user's grade _was_ persisted. - - The returned score includes valid values for: - raw_earned - will equal 0.0 since the user's score was not found from - earlier storages - raw_possible - retrieved from the persisted block - weighted_earned, weighted_possible - calculated from the raw scores and weight - weight, graded - retrieved from the persisted block - - block (block_structure.BlockData): - Values from the latest content-version of the block are used only if - they were not available from a prior storage. - - When the score is found from this storage, it implies the user has not - yet attempted this problem and the user's grade was _not_ yet persisted. - - The returned score includes valid values for: - raw_earned - will equal 0.0 since the user's score was not found from - earlier storages - raw_possible - retrieved from the latest block content - weighted_earned, weighted_possible - calculated from the raw scores and weight - weight, graded - retrieved from the latest block content - """ - weight = _get_weight_from_block(persisted_block, block) - - # Priority order for retrieving the scores: - # submissions API -> CSM -> grades persisted block -> latest block content - raw_earned, raw_possible, weighted_earned, weighted_possible = ( - _get_score_from_submissions(submissions_scores, block) or - _get_score_from_csm(csm_scores, block, weight) or - _get_score_from_persisted_or_latest_block(persisted_block, block, weight) - ) - - assert weighted_possible is not None - has_valid_denominator = weighted_possible > 0.0 - graded = _get_graded_from_block(persisted_block, block) if has_valid_denominator else False - - return ProblemScore( - raw_earned, - raw_possible, - weighted_earned, - weighted_possible, - weight, - graded, - display_name=display_name_with_default_escaped(block), - module_id=block.location, - ) - - -def _get_score_from_submissions(submissions_scores, block): - """ - Returns the score values from the submissions API if found. - """ - if submissions_scores: - submission_value = submissions_scores.get(unicode(block.location)) - if submission_value: - weighted_earned, weighted_possible = submission_value - assert weighted_earned >= 0.0 and weighted_possible > 0.0 # per contract from submissions API - return (None, None) + (weighted_earned, weighted_possible) - - -def _get_score_from_csm(csm_scores, block, weight): - """ - Returns the score values from the courseware student module, via - ScoresClient, if found. - """ - # If an entry exists and has raw_possible (total) associated with it, we trust - # that value. This is important for cases where a student might have seen an - # older version of the problem -- they're still graded on what was possible - # when they tried the problem, not what it's worth now. - # - # Note: Storing raw_possible in CSM predates the implementation of the grades - # own persistence layer. Hence, we have duplicate storage locations for - # raw_possible, with potentially conflicting values, when a problem is - # attempted. Even though the CSM persistence for this value is now - # superfluous, for backward compatibility, we continue to use its value for - # raw_possible, giving it precedence over the one in the grades data model. - score = csm_scores.get(block.location) - has_valid_score = score and score.total is not None - if has_valid_score: - raw_earned = score.correct if score.correct is not None else 0.0 - raw_possible = score.total - return (raw_earned, raw_possible) + _weighted_score(raw_earned, raw_possible, weight) - - -def _get_score_from_persisted_or_latest_block(persisted_block, block, weight): - """ - Returns the score values, now assuming the earned score is 0.0 - since a - score was not found in an earlier storage. - Uses the raw_possible value from the persisted_block if found, else from - the latest block content. - """ - raw_earned = 0.0 - - if persisted_block: - raw_possible = persisted_block.raw_possible - else: - raw_possible = block.transformer_data[GradesTransformer].max_score - - return (raw_earned, raw_possible) + _weighted_score(raw_earned, raw_possible, weight) - - -def _get_weight_from_block(persisted_block, block): - """ - Returns the weighted value from the persisted_block if found, else from - the latest block content. - """ - if persisted_block: - return persisted_block.weight - else: - return getattr(block, 'weight', None) - - -def _get_graded_from_block(persisted_block, block): - """ - Returns the graded value from the persisted_block if found, else from - the latest block content. - """ - if persisted_block: - return persisted_block.graded - else: - return _get_explicit_graded(block) - - -def _get_explicit_graded(block): - """ - Returns the explicit graded field value for the given block. - """ - field_value = getattr( - block.transformer_data[GradesTransformer], - GradesTransformer.EXPLICIT_GRADED_FIELD_NAME, - None, - ) - - # Set to True if grading is not explicitly disabled for - # this block. This allows us to include the block's score - # in the aggregated self.graded_total, regardless of the - # inherited graded value from the subsection. (TNL-5560) - return True if field_value is None else field_value - - -def _weighted_score(raw_earned, raw_possible, weight): - """ - Returns a tuple that represents the weighted (earned, possible) score. - If weight is None or raw_possible is 0, returns the original values. - - When weight is used, it defines the weighted_possible. This allows - course authors to specify the exact maximum value for a problem when - they provide a weight. - """ - assert raw_possible is not None - cannot_compute_with_weight = weight is None or raw_possible == 0 - if cannot_compute_with_weight: - return raw_earned, raw_possible - else: - return float(raw_earned) * weight / raw_possible, float(weight) - - @memoized -def _block_types_possibly_scored(): +def block_types_possibly_scored(): """ Returns the block types that could have a score. @@ -244,7 +22,89 @@ def _block_types_possibly_scored(): which have state but cannot ever impact someone's grade. """ return frozenset( - category for (category, xblock_class) in XBlock.load_classes() if ( + cat for (cat, xblock_class) in XBlock.load_classes() if ( getattr(xblock_class, 'has_score', False) or getattr(xblock_class, 'has_children', False) ) ) + + +def possibly_scored(usage_key): + """ + Returns whether the given block could impact grading (i.e. scored, or has children). + """ + return usage_key.block_type in block_types_possibly_scored() + + +def weighted_score(raw_earned, raw_possible, weight=None): + """ + Return a tuple that represents the weighted (earned, possible) score. + If weight is None or raw_possible is 0, returns the original values. + """ + if weight is None or raw_possible == 0: + return (raw_earned, raw_possible) + return float(raw_earned) * weight / raw_possible, float(weight) + + +def get_score(user, block, scores_client, submissions_scores_cache, weight, possible=None): + """ + Return the score for a user on a problem, as a tuple (earned, possible). + e.g. (5,7) if you got 5 out of 7 points. + + If this problem doesn't have a score, or we couldn't load it, returns (None, + None). + + user: a Student object + block: a BlockStructure's BlockData object + scores_client: an initialized ScoresClient + submissions_scores_cache: A dict of location names to (earned, possible) + point tuples. If an entry is found in this cache, it takes precedence. + weight: The weight of the problem to use in the calculation. A value of + None signifies that the weight should not be applied. + possible (optional): The possible maximum score of the problem to use in the + calculation. If None, uses the value found either in scores_client or + from the block. + """ + submissions_scores_cache = submissions_scores_cache or {} + + if not user.is_authenticated(): + return (None, None) + + location_url = unicode(block.location) + if location_url in submissions_scores_cache: + return submissions_scores_cache[location_url] + + if not getattr(block, 'has_score', False): + # These are not problems, and do not have a score + return (None, None) + + # Check the score that comes from the ScoresClient (out of CSM). + # If an entry exists and has a total associated with it, we trust that + # value. This is important for cases where a student might have seen an + # older version of the problem -- they're still graded on what was possible + # when they tried the problem, not what it's worth now. + score = scores_client.get(block.location) + if score and score.total is not None: + # We have a valid score, just use it. + earned = score.correct if score.correct is not None else 0.0 + if possible is None: + possible = score.total + elif possible != score.total: + log.error( + u"Persistent Grades: scores.get_score, possible value {} != score.total value {}".format( + possible, + score.total + ) + ) + else: + # This means we don't have a valid score entry and we don't have a + # cached_max_score on hand. We know they've earned 0.0 points on this. + earned = 0.0 + if possible is None: + possible = block.transformer_data[GradesTransformer].max_score + + # Problem may be an error module (if something in the problem builder failed) + # In which case possible might be None + if possible is None: + return (None, None) + + return weighted_score(earned, possible, weight) diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index 0b493cf812..ac3993736e 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -2,29 +2,24 @@ Test grade calculation. """ -import ddt from django.http import Http404 -import itertools from mock import patch from nose.plugins.attrib import attr from opaque_keys.edx.locations import SlashSeparatedCourseKey -from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory -from courseware.tests.helpers import get_request_for_user -from lms.djangoapps.course_blocks.api import get_course_blocks +from courseware.tests.helpers import ( + LoginEnrollmentTestCase, + get_request_for_user +) from student.tests.factories import UserFactory from student.models import CourseEnrollment -from xmodule.block_metadata_utils import display_name_with_default_escaped -from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from xmodule.graders import ProblemScore from .utils import answer_problem from .. import course_grades from ..course_grades import summary as grades_summary from ..new.course_grade import CourseGradeFactory -from ..new.subsection_grade import SubsectionGradeFactory def _grade_with_errors(student, course): @@ -41,17 +36,6 @@ def _grade_with_errors(student, course): return grades_summary(student, course) -def _create_problem_xml(): - """ - Creates and returns XML for a multiple choice response problem - """ - return MultipleChoiceResponseXMLFactory().build_xml( - question_text='The correct answer is Choice 3', - choices=[False, False, True, False], - choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] - ) - - @attr(shard=1) class TestGradeIteration(SharedModuleStoreTestCase): """ @@ -153,101 +137,6 @@ class TestGradeIteration(SharedModuleStoreTestCase): return students_to_gradesets, students_to_errors -@ddt.ddt -class TestWeightedProblems(SharedModuleStoreTestCase): - """ - Test scores and grades with various problem weight values. - """ - @classmethod - def setUpClass(cls): - super(TestWeightedProblems, cls).setUpClass() - cls.course = CourseFactory.create() - cls.chapter = ItemFactory.create(parent=cls.course, category="chapter", display_name="chapter") - cls.sequential = ItemFactory.create(parent=cls.chapter, category="sequential", display_name="sequential") - cls.vertical = ItemFactory.create(parent=cls.sequential, category="vertical", display_name="vertical1") - problem_xml = _create_problem_xml() - cls.problems = [] - for i in range(2): - cls.problems.append( - ItemFactory.create( - parent=cls.vertical, - category="problem", - display_name="problem_{}".format(i), - data=problem_xml, - ) - ) - - def setUp(self): - super(TestWeightedProblems, self).setUp() - self.user = UserFactory() - self.request = get_request_for_user(self.user) - - def _verify_grades(self, raw_earned, raw_possible, weight, expected_score): - """ - Verifies the computed grades are as expected. - """ - with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): - # pylint: disable=no-member - for problem in self.problems: - problem.weight = weight - self.store.update_item(problem, self.user.id) - self.store.publish(self.course.location, self.user.id) - - course_structure = get_course_blocks(self.request.user, self.course.location) - - # answer all problems - for problem in self.problems: - answer_problem(self.course, self.request, problem, score=raw_earned, max_value=raw_possible) - - # get grade - subsection_grade = SubsectionGradeFactory( - self.request.user, self.course, course_structure - ).update(self.sequential) - - # verify all problem grades - for problem in self.problems: - problem_score = subsection_grade.locations_to_scores[problem.location] - expected_score.display_name = display_name_with_default_escaped(problem) - expected_score.module_id = problem.location - self.assertEquals(problem_score, expected_score) - - # verify subsection grades - self.assertEquals(subsection_grade.all_total.earned, expected_score.earned * len(self.problems)) - self.assertEquals(subsection_grade.all_total.possible, expected_score.possible * len(self.problems)) - - @ddt.data( - *itertools.product( - (0.0, 0.5, 1.0, 2.0), # raw_earned - (-2.0, -1.0, 0.0, 0.5, 1.0, 2.0), # raw_possible - (-2.0, -1.0, -0.5, 0.0, 0.5, 1.0, 2.0, 50.0, None), # weight - ) - ) - @ddt.unpack - def test_problem_weight(self, raw_earned, raw_possible, weight): - - use_weight = weight is not None and raw_possible != 0 - if use_weight: - expected_w_earned = raw_earned / raw_possible * weight - expected_w_possible = weight - else: - expected_w_earned = raw_earned - expected_w_possible = raw_possible - - expected_graded = expected_w_possible > 0 - - expected_score = ProblemScore( - raw_earned=raw_earned, - raw_possible=raw_possible, - weighted_earned=expected_w_earned, - weighted_possible=expected_w_possible, - weight=weight, - graded=expected_graded, - display_name=None, # problem-specific, filled in by _verify_grades - module_id=None, # problem-specific, filled in by _verify_grades - ) - self._verify_grades(raw_earned, raw_possible, weight, expected_score) - - class TestScoreForModule(SharedModuleStoreTestCase): """ Test the method that calculates the score for a given block based on the diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index 16fe1ea008..e3dd0a24a7 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -71,8 +71,8 @@ class GradesModelTestCase(TestCase): block_type='problem', block_id='block_id_b' ) - self.record_a = BlockRecord(locator=self.locator_a, weight=1, raw_possible=10, graded=False) - self.record_b = BlockRecord(locator=self.locator_b, weight=1, raw_possible=10, graded=True) + self.record_a = BlockRecord(locator=self.locator_a, weight=1, max_score=10) + self.record_b = BlockRecord(locator=self.locator_b, weight=1, max_score=10) @ddt.ddt @@ -88,31 +88,29 @@ class BlockRecordTest(GradesModelTestCase): Tests creation of a BlockRecord. """ weight = 1 - raw_possible = 10 + max_score = 10 record = BlockRecord( self.locator_a, weight, - raw_possible, - graded=False, + max_score, ) self.assertEqual(record.locator, self.locator_a) @ddt.data( - (0, 0, "0123456789abcdef", True), - (1, 10, 'totally_a_real_block_key', False), - ("BlockRecord is", "a dumb data store", "with no validation", None), + (0, 0, "0123456789abcdef"), + (1, 10, 'totally_a_real_block_key'), + ("BlockRecord is", "a dumb data store", "with no validation"), ) @ddt.unpack - def test_serialization(self, weight, raw_possible, block_key, graded): + def test_serialization(self, weight, max_score, block_key): """ Tests serialization of a BlockRecord using the _asdict() method. """ - record = BlockRecord(block_key, weight, raw_possible, graded) + record = BlockRecord(block_key, weight, max_score) expected = OrderedDict([ ("locator", block_key), ("weight", weight), - ("raw_possible", raw_possible), - ("graded", graded), + ("max_score", max_score), ]) self.assertEqual(expected, record._asdict()) @@ -136,12 +134,7 @@ class VisibleBlocksTest(GradesModelTestCase): for block_dict in list_of_block_dicts: block_dict['locator'] = unicode(block_dict['locator']) # BlockUsageLocator is not json-serializable expected_data = { - 'blocks': [{ - 'locator': unicode(self.record_a.locator), - 'raw_possible': 10, - 'weight': 1, - 'graded': self.record_a.graded, - }], + 'blocks': [{'locator': unicode(self.record_a.locator), 'max_score': 10, 'weight': 1}], 'course_key': unicode(self.record_a.locator.course_key), 'version': BLOCK_RECORD_LIST_VERSION, } diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 25402508c6..eaa8fe7bda 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -1,7 +1,7 @@ """ Test saved subsection grade functionality. """ -# pylint: disable=protected-access + import ddt from django.conf import settings from django.db.utils import DatabaseError @@ -116,7 +116,7 @@ class SubsectionGradeFactoryTest(GradeTestBase): ) as mock_create_grade: with patch( 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', - wraps=self.subsection_grade_factory._get_saved_grade + wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access ) as mock_get_saved_grade: with self.assertNumQueries(14): grade_a = self.subsection_grade_factory.create(self.sequence) @@ -205,8 +205,8 @@ class SubsectionGradeTest(GradeTestBase): input_grade.init_from_structure( self.request.user, self.course_structure, - self.subsection_grade_factory._submissions_scores, - self.subsection_grade_factory._csm_scores, + self.subsection_grade_factory._scores_client, # pylint: disable=protected-access + self.subsection_grade_factory._submissions_scores, # pylint: disable=protected-access ) self.assertEqual(PersistentSubsectionGrade.objects.count(), 0) @@ -224,8 +224,8 @@ class SubsectionGradeTest(GradeTestBase): self.request.user, saved_model, self.course_structure, - self.subsection_grade_factory._submissions_scores, - self.subsection_grade_factory._csm_scores, + self.subsection_grade_factory._scores_client, # pylint: disable=protected-access + self.subsection_grade_factory._submissions_scores, # pylint: disable=protected-access ) self.assertEqual(input_grade.url_name, loaded_grade.url_name) diff --git a/lms/djangoapps/grades/tests/test_scores.py b/lms/djangoapps/grades/tests/test_scores.py deleted file mode 100644 index a4002fbef2..0000000000 --- a/lms/djangoapps/grades/tests/test_scores.py +++ /dev/null @@ -1,288 +0,0 @@ -""" -Tests for grades.scores module. -""" -# pylint: disable=protected-access -from collections import namedtuple -import ddt -from django.test import TestCase -import itertools - -from lms.djangoapps.grades.models import BlockRecord -import lms.djangoapps.grades.scores as scores -from lms.djangoapps.grades.transformer import GradesTransformer -from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator -from openedx.core.lib.block_structure.block_structure import BlockData -from xmodule.graders import ProblemScore - - -class TestScoredBlockTypes(TestCase): - """ - Tests for the possibly_scored function. - """ - possibly_scored_block_types = { - 'course', 'chapter', 'sequential', 'vertical', - 'library_content', 'split_test', 'conditional', 'library', 'randomize', - 'problem', 'drag-and-drop-v2', 'openassessment', 'lti', 'lti_consumer', - 'videosequence', 'problemset', 'acid_parent', 'done', 'wrapper', 'edx_sga', - } - - def test_block_types_possibly_scored(self): - self.assertSetEqual( - self.possibly_scored_block_types, - scores._block_types_possibly_scored() - ) - - def test_possibly_scored(self): - course_key = CourseLocator(u'org', u'course', u'run') - for block_type in self.possibly_scored_block_types: - usage_key = BlockUsageLocator(course_key, block_type, 'mock_block_id') - self.assertTrue(scores.possibly_scored(usage_key)) - - -@ddt.ddt -class TestGetScore(TestCase): - """ - Tests for get_score - """ - display_name = 'test_name' - location = 'test_location' - - SubmissionValue = namedtuple('SubmissionValue', 'exists, weighted_earned, weighted_possible') - CSMValue = namedtuple('CSMValue', 'exists, raw_earned, raw_possible') - PersistedBlockValue = namedtuple('PersistedBlockValue', 'exists, raw_possible, weight, graded') - ContentBlockValue = namedtuple('ContentBlockValue', 'raw_possible, weight, explicit_graded') - ExpectedResult = namedtuple( - 'ExpectedResult', 'raw_earned, raw_possible, weighted_earned, weighted_possible, weight, graded' - ) - - def _create_submissions_scores(self, submission_value): - """ - Creates a stub result from the submissions API for the given values. - """ - if submission_value.exists: - return {self.location: (submission_value.weighted_earned, submission_value.weighted_possible)} - else: - return {} - - def _create_csm_scores(self, csm_value): - """ - Creates a stub result from courseware student module for the given values. - """ - if csm_value.exists: - stub_csm_record = namedtuple('stub_csm_record', 'correct, total') - return {self.location: stub_csm_record(correct=csm_value.raw_earned, total=csm_value.raw_possible)} - else: - return {} - - def _create_persisted_block(self, persisted_block_value): - """ - Creates and returns a minimal BlockRecord object with the give values. - """ - if persisted_block_value.exists: - return BlockRecord( - self.location, - persisted_block_value.weight, - persisted_block_value.raw_possible, - persisted_block_value.graded, - ) - else: - return None - - def _create_block(self, content_block_value): - """ - Creates and returns a minimal BlockData object with the give values. - """ - block = BlockData(self.location) - block.display_name = self.display_name - block.weight = content_block_value.weight - - block_grades_transformer_data = block.transformer_data.get_or_create(GradesTransformer) - block_grades_transformer_data.max_score = content_block_value.raw_possible - setattr( - block_grades_transformer_data, - GradesTransformer.EXPLICIT_GRADED_FIELD_NAME, - content_block_value.explicit_graded, - ) - return block - - @ddt.data( - # submissions _trumps_ other values; weighted and graded from persisted-block _trumps_ latest content values - ( - SubmissionValue(exists=True, weighted_earned=50, weighted_possible=100), - CSMValue(exists=True, raw_earned=10, raw_possible=40), - PersistedBlockValue(exists=True, raw_possible=5, weight=40, graded=True), - ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), - ExpectedResult( - raw_earned=None, raw_possible=None, weighted_earned=50, weighted_possible=100, weight=40, graded=True - ), - ), - # same as above, except submissions doesn't exist; CSM values used - ( - SubmissionValue(exists=False, weighted_earned=50, weighted_possible=100), - CSMValue(exists=True, raw_earned=10, raw_possible=40), - PersistedBlockValue(exists=True, raw_possible=5, weight=40, graded=True), - ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), - ExpectedResult( - raw_earned=10, raw_possible=40, weighted_earned=10, weighted_possible=40, weight=40, graded=True - ), - ), - # neither submissions nor CSM exist; Persisted values used - ( - SubmissionValue(exists=False, weighted_earned=50, weighted_possible=100), - CSMValue(exists=False, raw_earned=10, raw_possible=40), - PersistedBlockValue(exists=True, raw_possible=5, weight=40, graded=True), - ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), - ExpectedResult( - raw_earned=0, raw_possible=5, weighted_earned=0, weighted_possible=40, weight=40, graded=True - ), - ), - # none of submissions, CSM, or persisted exist; Latest content values used - ( - SubmissionValue(exists=False, weighted_earned=50, weighted_possible=100), - CSMValue(exists=False, raw_earned=10, raw_possible=40), - PersistedBlockValue(exists=False, raw_possible=5, weight=40, graded=True), - ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), - ExpectedResult( - raw_earned=0, raw_possible=1, weighted_earned=0, weighted_possible=20, weight=20, graded=False - ), - ), - ) - @ddt.unpack - def test_get_score(self, submission_value, csm_value, persisted_block_value, block_value, expected_result): - score = scores.get_score( - self._create_submissions_scores(submission_value), - self._create_csm_scores(csm_value), - self._create_persisted_block(persisted_block_value), - self._create_block(block_value), - ) - expected_score = ProblemScore( - display_name=self.display_name, module_id=self.location, **expected_result._asdict() - ) - self.assertEquals(score, expected_score) - - -@ddt.ddt -class TestInternalWeightedScore(TestCase): - """ - Tests the internal helper method: _weighted_score - """ - @ddt.data( - (0, 0, 1), - (5, 0, 0), - (10, 0, None), - (0, 5, None), - (5, 10, None), - (10, 10, None), - ) - @ddt.unpack - def test_cannot_compute(self, raw_earned, raw_possible, weight): - self.assertEquals( - scores._weighted_score(raw_earned, raw_possible, weight), - (raw_earned, raw_possible), - ) - - @ddt.data( - (0, 5, 0, (0, 0)), - (5, 5, 0, (0, 0)), - (2, 5, 1, (.4, 1)), - (5, 5, 1, (1, 1)), - (5, 5, 3, (3, 3)), - (2, 4, 6, (3, 6)), - ) - @ddt.unpack - def test_computed(self, raw_earned, raw_possible, weight, expected_score): - self.assertEquals( - scores._weighted_score(raw_earned, raw_possible, weight), - expected_score, - ) - - def test_assert_on_invalid_r_possible(self): - with self.assertRaises(AssertionError): - scores._weighted_score(raw_earned=1, raw_possible=None, weight=1) - - -@ddt.ddt -class TestInternalGetGraded(TestCase): - """ - Tests the internal helper method: _get_explicit_graded - """ - def _create_block(self, explicit_graded_value): - """ - Creates and returns a minimal BlockData object with the give value - for explicit_graded. - """ - block = BlockData('any_key') - setattr( - block.transformer_data.get_or_create(GradesTransformer), - GradesTransformer.EXPLICIT_GRADED_FIELD_NAME, - explicit_graded_value, - ) - return block - - @ddt.data(None, True, False) - def test_with_no_persisted_block(self, explicitly_graded_value): - block = self._create_block(explicitly_graded_value) - self.assertEquals( - scores._get_graded_from_block(None, block), - explicitly_graded_value is not False, # defaults to True unless explicitly False - ) - - @ddt.data( - *itertools.product((True, False), (True, False, None)) - ) - @ddt.unpack - def test_with_persisted_block(self, persisted_block_value, block_value): - block = self._create_block(block_value) - block_record = BlockRecord(block.location, 0, 0, persisted_block_value) - self.assertEquals( - scores._get_graded_from_block(block_record, block), - block_record.graded, # persisted value takes precedence - ) - - -@ddt.ddt -class TestInternalGetScoreFromBlock(TestCase): - """ - Tests the internal helper method: _get_score_from_persisted_or_latest_block - """ - def _create_block(self, raw_possible): - """ - Creates and returns a minimal BlockData object with the give value - for raw_possible. - """ - block = BlockData('any_key') - block.transformer_data.get_or_create(GradesTransformer).max_score = raw_possible - return block - - def _verify_score_result(self, persisted_block, block, weight, expected_r_possible): - """ - Verifies the result of _get_score_from_persisted_or_latest_block is as expected. - """ - # pylint: disable=unbalanced-tuple-unpacking - raw_earned, raw_possible, weighted_earned, weighted_possible = scores._get_score_from_persisted_or_latest_block( - persisted_block, block, weight, - ) - self.assertEquals(raw_earned, 0.0) - self.assertEquals(raw_possible, expected_r_possible) - self.assertEquals(weighted_earned, 0.0) - if weight is None or expected_r_possible == 0: - self.assertEquals(weighted_possible, expected_r_possible) - else: - self.assertEquals(weighted_possible, weight) - - @ddt.data( - *itertools.product((0, 1, 5), (None, 0, 1, 5)) - ) - @ddt.unpack - def test_with_no_persisted_block(self, block_r_possible, weight): - block = self._create_block(block_r_possible) - self._verify_score_result(None, block, weight, block_r_possible) - - @ddt.data( - *itertools.product((0, 1, 5), (None, 0, 1, 5), (None, 0, 1, 5)) - ) - @ddt.unpack - def test_with_persisted_block(self, persisted_block_r_possible, block_r_possible, weight): - block = self._create_block(block_r_possible) - block_record = BlockRecord(block.location, 0, persisted_block_r_possible, False) - self._verify_score_result(block_record, block, weight, persisted_block_r_possible) diff --git a/lms/djangoapps/grades/tests/utils.py b/lms/djangoapps/grades/tests/utils.py index 951e800329..ce821c5724 100644 --- a/lms/djangoapps/grades/tests/utils.py +++ b/lms/djangoapps/grades/tests/utils.py @@ -5,7 +5,6 @@ from contextlib import contextmanager from mock import patch from courseware.module_render import get_module from courseware.model_data import FieldDataCache -from xmodule.graders import ProblemScore @contextmanager @@ -24,7 +23,7 @@ def mock_get_score(earned=0, possible=1): Mocks the get_score function to return a valid grade. """ with patch('lms.djangoapps.grades.new.subsection_grade.get_score') as mock_score: - mock_score.return_value = ProblemScore(earned, possible, earned, possible, 1, True, None, None) + mock_score.return_value = (earned, possible) yield mock_score diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index abede406e9..32daa1be89 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -948,7 +948,7 @@ def upload_problem_grade_report(_xmodule_instance_args, _entry_id, course_id, _t final_grade = gradeset['percent'] # Only consider graded problems - problem_scores = {unicode(score.module_id): score for score in gradeset['raw_scores'] if score.graded} + problem_scores = {unicode(score.module_id): score for score, _ in gradeset['raw_scores'] if score.graded} earned_possible_values = list() for problem_id in problems: try: From 242f3a79c576d62881403b004155b91a55379fab Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 30 Sep 2016 10:46:45 -0400 Subject: [PATCH 04/12] Quality fix in revert --- lms/djangoapps/grades/tests/test_grades.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index ac3993736e..c937a71733 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -7,10 +7,7 @@ from mock import patch from nose.plugins.attrib import attr from opaque_keys.edx.locations import SlashSeparatedCourseKey -from courseware.tests.helpers import ( - LoginEnrollmentTestCase, - get_request_for_user -) +from courseware.tests.helpers import get_request_for_user from student.tests.factories import UserFactory from student.models import CourseEnrollment from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory From d772cb0ada2acc1c7f4d46b655a22f2c30a8e49e Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 30 Sep 2016 16:52:05 -0400 Subject: [PATCH 05/12] Revert "Merge pull request #13614 from edx/efischer/revert_grades" This reverts commit 0ea8e1b9b262fadce61bc4acbad1eacd12ebd3d8, reversing changes made to e3db08f1946e34c128c4fa9d56c180ca9bbca4c1. --- common/lib/xmodule/xmodule/graders.py | 79 ++++- .../lib/xmodule/xmodule/tests/test_graders.py | 163 +++++---- common/test/data/blocks/capa.xml | 6 + common/test/data/blocks/coderesponse.xml | 25 ++ common/test/data/blocks/library_content.xml | 4 + common/test/data/blocks/lti.xml | 1 + common/test/data/blocks/openassessment.xml | 87 +++++ lms/djangoapps/gating/tests/test_api.py | 4 +- lms/djangoapps/grades/models.py | 5 +- lms/djangoapps/grades/new/course_grade.py | 16 +- lms/djangoapps/grades/new/subsection_grade.py | 119 ++----- lms/djangoapps/grades/scores.py | 308 +++++++++++++----- lms/djangoapps/grades/tests/test_grades.py | 114 +++++++ lms/djangoapps/grades/tests/test_models.py | 29 +- lms/djangoapps/grades/tests/test_new.py | 165 +++++++++- lms/djangoapps/grades/tests/test_scores.py | 288 ++++++++++++++++ lms/djangoapps/grades/tests/utils.py | 3 +- lms/djangoapps/instructor/enrollment.py | 47 ++- .../instructor/tests/test_enrollment.py | 79 +++++ .../instructor_task/tasks_helper.py | 2 +- .../__init__.py} | 0 openedx/core/lib/xblock_utils/test_utils.py | 26 ++ 22 files changed, 1294 insertions(+), 276 deletions(-) create mode 100644 common/test/data/blocks/capa.xml create mode 100644 common/test/data/blocks/coderesponse.xml create mode 100644 common/test/data/blocks/library_content.xml create mode 100644 common/test/data/blocks/lti.xml create mode 100644 common/test/data/blocks/openassessment.xml create mode 100644 lms/djangoapps/grades/tests/test_scores.py rename openedx/core/lib/{xblock_utils.py => xblock_utils/__init__.py} (100%) create mode 100644 openedx/core/lib/xblock_utils/test_utils.py diff --git a/common/lib/xmodule/xmodule/graders.py b/common/lib/xmodule/xmodule/graders.py index c84699df5d..09caefe878 100644 --- a/common/lib/xmodule/xmodule/graders.py +++ b/common/lib/xmodule/xmodule/graders.py @@ -10,13 +10,67 @@ import logging import random import sys -from collections import namedtuple log = logging.getLogger("edx.courseware") -# This is a tuple for holding scores, either from problems or sections. -# Section either indicates the name of the problem or the name of the section -Score = namedtuple("Score", "earned possible graded section module_id") + +class ScoreBase(object): + """ + Abstract base class for encapsulating fields of values scores. + Field common to all scores include: + display_name (string) - the display name of the module + module_id (UsageKey) - the location of the module + graded (boolean) - whether or not this module is graded + """ + __metaclass__ = abc.ABCMeta + + def __init__(self, graded, display_name, module_id): + self.graded = graded + self.display_name = display_name + self.module_id = module_id + + def __eq__(self, other): + if type(other) is type(self): + return self.__dict__ == other.__dict__ + return False + + def __ne__(self, other): + return not self.__eq__(other) + + def __repr__(self): + return u"{class_name}({fields})".format(class_name=self.__class__.__name__, fields=self.__dict__) + + +class ProblemScore(ScoreBase): + """ + Encapsulates the fields of a Problem's score. + In addition to the fields in ScoreBase, also includes: + raw_earned (float) - raw points earned on this problem + raw_possible (float) - raw points possible to earn on this problem + weighted_earned = earned (float) - weighted value of the points earned + weighted_possible = possible (float) - weighted possible points on this problem + weight (float) - weight of this problem + """ + def __init__(self, raw_earned, raw_possible, weighted_earned, weighted_possible, weight, *args, **kwargs): + super(ProblemScore, self).__init__(*args, **kwargs) + self.raw_earned = raw_earned + self.raw_possible = raw_possible + self.earned = weighted_earned + self.possible = weighted_possible + self.weight = weight + + +class AggregatedScore(ScoreBase): + """ + Encapsulates the fields of a Subsection's score. + In addition to the fields in ScoreBase, also includes: + tw_earned = earned - total aggregated sum of all weighted earned values + tw_possible = possible - total aggregated sum of all weighted possible values + """ + def __init__(self, tw_earned, tw_possible, *args, **kwargs): + super(AggregatedScore, self).__init__(*args, **kwargs) + self.earned = tw_earned + self.possible = tw_possible def float_sum(iterable): @@ -26,13 +80,14 @@ def float_sum(iterable): return float(sum(iterable)) -def aggregate_scores(scores, section_name="summary", location=None): +def aggregate_scores(scores, display_name="summary", location=None): """ - scores: A list of Score objects + scores: A list of ScoreBase objects + display_name: The display name for the score object location: The location under which all objects in scores are located returns: A tuple (all_total, graded_total). - all_total: A Score representing the total score summed over all input scores - graded_total: A Score representing the score summed over all graded input scores + all_total: A ScoreBase representing the total score summed over all input scores + graded_total: A ScoreBase representing the score summed over all graded input scores """ total_correct_graded = float_sum(score.earned for score in scores if score.graded) total_possible_graded = float_sum(score.possible for score in scores if score.graded) @@ -41,10 +96,10 @@ def aggregate_scores(scores, section_name="summary", location=None): total_possible = float_sum(score.possible for score in scores) #regardless of whether it is graded - all_total = Score(total_correct, total_possible, False, section_name, location) + all_total = AggregatedScore(total_correct, total_possible, False, display_name, location) #selecting only graded things - graded_total = Score(total_correct_graded, total_possible_graded, True, section_name, location) + graded_total = AggregatedScore(total_correct_graded, total_possible_graded, True, display_name, location) return all_total, graded_total @@ -220,7 +275,7 @@ class SingleSectionGrader(CourseGrader): found_score = None if self.type in grade_sheet: for score in grade_sheet[self.type]: - if score.section == self.name: + if score.display_name == self.name: found_score = score break @@ -342,7 +397,7 @@ class AssignmentFormatGrader(CourseGrader): else: earned = scores[i].earned possible = scores[i].possible - section_name = scores[i].section + section_name = scores[i].display_name percentage = earned / possible summary_format = u"{section_type} {index} - {name} - {percent:.0%} ({earned:.3n}/{possible:.3n})" diff --git a/common/lib/xmodule/xmodule/tests/test_graders.py b/common/lib/xmodule/xmodule/tests/test_graders.py index d3e73fcc54..341146dc23 100644 --- a/common/lib/xmodule/xmodule/tests/test_graders.py +++ b/common/lib/xmodule/xmodule/tests/test_graders.py @@ -2,42 +2,67 @@ import unittest from xmodule import graders -from xmodule.graders import Score, aggregate_scores +from xmodule.graders import ProblemScore, AggregatedScore, aggregate_scores class GradesheetTest(unittest.TestCase): - '''Tests the aggregate_scores method''' + """ + Tests the aggregate_scores method + """ def test_weighted_grading(self): scores = [] - Score.__sub__ = lambda me, other: (me.earned - other.earned) + (me.possible - other.possible) + agg_fields = dict(display_name="aggregated_score", module_id=None) + prob_fields = dict(display_name="problem_score", module_id=None, raw_earned=0, raw_possible=0, weight=0) - all_total, graded_total = aggregate_scores(scores) - self.assertEqual(all_total, Score(earned=0, possible=0, graded=False, section="summary", module_id=None)) - self.assertEqual(graded_total, Score(earned=0, possible=0, graded=True, section="summary", module_id=None)) - - scores.append(Score(earned=0, possible=5, graded=False, section="summary", module_id=None)) - all_total, graded_total = aggregate_scores(scores) - self.assertEqual(all_total, Score(earned=0, possible=5, graded=False, section="summary", module_id=None)) - self.assertEqual(graded_total, Score(earned=0, possible=0, graded=True, section="summary", module_id=None)) - - scores.append(Score(earned=3, possible=5, graded=True, section="summary", module_id=None)) - all_total, graded_total = aggregate_scores(scores) - self.assertAlmostEqual(all_total, Score(earned=3, possible=10, graded=False, section="summary", module_id=None)) - self.assertAlmostEqual( - graded_total, Score(earned=3, possible=5, graded=True, section="summary", module_id=None) + all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) + self.assertEqual( + all_total, + AggregatedScore(tw_earned=0, tw_possible=0, graded=False, **agg_fields), + ) + self.assertEqual( + graded_total, + AggregatedScore(tw_earned=0, tw_possible=0, graded=True, **agg_fields), ) - scores.append(Score(earned=2, possible=5, graded=True, section="summary", module_id=None)) - all_total, graded_total = aggregate_scores(scores) - self.assertAlmostEqual(all_total, Score(earned=5, possible=15, graded=False, section="summary", module_id=None)) + scores.append(ProblemScore(weighted_earned=0, weighted_possible=5, graded=False, **prob_fields)) + all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) + self.assertEqual( + all_total, + AggregatedScore(tw_earned=0, tw_possible=5, graded=False, **agg_fields), + ) + self.assertEqual( + graded_total, + AggregatedScore(tw_earned=0, tw_possible=0, graded=True, **agg_fields), + ) + + scores.append(ProblemScore(weighted_earned=3, weighted_possible=5, graded=True, **prob_fields)) + all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) self.assertAlmostEqual( - graded_total, Score(earned=5, possible=10, graded=True, section="summary", module_id=None) + all_total, + AggregatedScore(tw_earned=3, tw_possible=10, graded=False, **agg_fields), + ) + self.assertAlmostEqual( + graded_total, + AggregatedScore(tw_earned=3, tw_possible=5, graded=True, **agg_fields), + ) + + scores.append(ProblemScore(weighted_earned=2, weighted_possible=5, graded=True, **prob_fields)) + all_total, graded_total = aggregate_scores(scores, display_name=agg_fields['display_name']) + self.assertAlmostEqual( + all_total, + AggregatedScore(tw_earned=5, tw_possible=15, graded=False, **agg_fields), + ) + self.assertAlmostEqual( + graded_total, + AggregatedScore(tw_earned=5, tw_possible=10, graded=True, **agg_fields), ) class GraderTest(unittest.TestCase): - '''Tests grader implementations''' + """ + Tests grader implementations + """ empty_gradesheet = { } @@ -49,19 +74,25 @@ class GraderTest(unittest.TestCase): } test_gradesheet = { - 'Homework': [Score(earned=2, possible=20.0, graded=True, section='hw1', module_id=None), - Score(earned=16, possible=16.0, graded=True, section='hw2', module_id=None)], + 'Homework': [ + AggregatedScore(tw_earned=2, tw_possible=20.0, graded=True, display_name='hw1', module_id=None), + AggregatedScore(tw_earned=16, tw_possible=16.0, graded=True, display_name='hw2', module_id=None) + ], + # The dropped scores should be from the assignments that don't exist yet + 'Lab': [ + AggregatedScore(tw_earned=1, tw_possible=2.0, graded=True, display_name='lab1', module_id=None), # Dropped + AggregatedScore(tw_earned=1, tw_possible=1.0, graded=True, display_name='lab2', module_id=None), + AggregatedScore(tw_earned=1, tw_possible=1.0, graded=True, display_name='lab3', module_id=None), + AggregatedScore(tw_earned=5, tw_possible=25.0, graded=True, display_name='lab4', module_id=None), # Dropped + AggregatedScore(tw_earned=3, tw_possible=4.0, graded=True, display_name='lab5', module_id=None), # Dropped + AggregatedScore(tw_earned=6, tw_possible=7.0, graded=True, display_name='lab6', module_id=None), + AggregatedScore(tw_earned=5, tw_possible=6.0, graded=True, display_name='lab7', module_id=None), + ], - 'Lab': [Score(earned=1, possible=2.0, graded=True, section='lab1', module_id=None), # Dropped - Score(earned=1, possible=1.0, graded=True, section='lab2', module_id=None), - Score(earned=1, possible=1.0, graded=True, section='lab3', module_id=None), - Score(earned=5, possible=25.0, graded=True, section='lab4', module_id=None), # Dropped - Score(earned=3, possible=4.0, graded=True, section='lab5', module_id=None), # Dropped - Score(earned=6, possible=7.0, graded=True, section='lab6', module_id=None), - Score(earned=5, possible=6.0, graded=True, section='lab7', module_id=None)], - - 'Midterm': [Score(earned=50.5, possible=100, graded=True, section="Midterm Exam", module_id=None), ], + 'Midterm': [ + AggregatedScore(tw_earned=50.5, tw_possible=100, graded=True, display_name="Midterm Exam", module_id=None), + ], } def test_single_section_grader(self): @@ -69,9 +100,11 @@ class GraderTest(unittest.TestCase): lab4_grader = graders.SingleSectionGrader("Lab", "lab4") bad_lab_grader = graders.SingleSectionGrader("Lab", "lab42") - for graded in [midterm_grader.grade(self.empty_gradesheet), - midterm_grader.grade(self.incomplete_gradesheet), - bad_lab_grader.grade(self.test_gradesheet)]: + for graded in [ + midterm_grader.grade(self.empty_gradesheet), + midterm_grader.grade(self.incomplete_gradesheet), + bad_lab_grader.grade(self.test_gradesheet), + ]: self.assertEqual(len(graded['section_breakdown']), 1) self.assertEqual(graded['percent'], 0.0) @@ -91,10 +124,12 @@ class GraderTest(unittest.TestCase): lab_grader = graders.AssignmentFormatGrader("Lab", 7, 3) # Test the grading of an empty gradesheet - for graded in [homework_grader.grade(self.empty_gradesheet), - no_drop_grader.grade(self.empty_gradesheet), - homework_grader.grade(self.incomplete_gradesheet), - no_drop_grader.grade(self.incomplete_gradesheet)]: + for graded in [ + homework_grader.grade(self.empty_gradesheet), + no_drop_grader.grade(self.empty_gradesheet), + homework_grader.grade(self.incomplete_gradesheet), + no_drop_grader.grade(self.incomplete_gradesheet), + ]: self.assertAlmostEqual(graded['percent'], 0.0) # Make sure the breakdown includes 12 sections, plus one summary self.assertEqual(len(graded['section_breakdown']), 12 + 1) @@ -118,8 +153,10 @@ class GraderTest(unittest.TestCase): def test_assignment_format_grader_on_single_section_entry(self): midterm_grader = graders.AssignmentFormatGrader("Midterm", 1, 0) # Test the grading on a section with one item: - for graded in [midterm_grader.grade(self.empty_gradesheet), - midterm_grader.grade(self.incomplete_gradesheet)]: + for graded in [ + midterm_grader.grade(self.empty_gradesheet), + midterm_grader.grade(self.incomplete_gradesheet), + ]: self.assertAlmostEqual(graded['percent'], 0.0) # Make sure the breakdown includes just the one summary self.assertEqual(len(graded['section_breakdown']), 0 + 1) @@ -137,23 +174,31 @@ class GraderTest(unittest.TestCase): # will act like SingleSectionGraders on single sections. midterm_grader = graders.AssignmentFormatGrader("Midterm", 1, 0) - weighted_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.25), - (lab_grader, lab_grader.category, 0.25), - (midterm_grader, midterm_grader.category, 0.5)]) + weighted_grader = graders.WeightedSubsectionsGrader([ + (homework_grader, homework_grader.category, 0.25), + (lab_grader, lab_grader.category, 0.25), + (midterm_grader, midterm_grader.category, 0.5), + ]) - over_one_weights_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.5), - (lab_grader, lab_grader.category, 0.5), - (midterm_grader, midterm_grader.category, 0.5)]) + over_one_weights_grader = graders.WeightedSubsectionsGrader([ + (homework_grader, homework_grader.category, 0.5), + (lab_grader, lab_grader.category, 0.5), + (midterm_grader, midterm_grader.category, 0.5), + ]) # The midterm should have all weight on this one - zero_weights_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.0), - (lab_grader, lab_grader.category, 0.0), - (midterm_grader, midterm_grader.category, 0.5)]) + zero_weights_grader = graders.WeightedSubsectionsGrader([ + (homework_grader, homework_grader.category, 0.0), + (lab_grader, lab_grader.category, 0.0), + (midterm_grader, midterm_grader.category, 0.5), + ]) # This should always have a final percent of zero - all_zero_weights_grader = graders.WeightedSubsectionsGrader([(homework_grader, homework_grader.category, 0.0), - (lab_grader, lab_grader.category, 0.0), - (midterm_grader, midterm_grader.category, 0.0)]) + all_zero_weights_grader = graders.WeightedSubsectionsGrader([ + (homework_grader, homework_grader.category, 0.0), + (lab_grader, lab_grader.category, 0.0), + (midterm_grader, midterm_grader.category, 0.0), + ]) empty_grader = graders.WeightedSubsectionsGrader([]) @@ -177,10 +222,12 @@ class GraderTest(unittest.TestCase): self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1) self.assertEqual(len(graded['grade_breakdown']), 3) - for graded in [weighted_grader.grade(self.empty_gradesheet), - weighted_grader.grade(self.incomplete_gradesheet), - zero_weights_grader.grade(self.empty_gradesheet), - all_zero_weights_grader.grade(self.empty_gradesheet)]: + for graded in [ + weighted_grader.grade(self.empty_gradesheet), + weighted_grader.grade(self.incomplete_gradesheet), + zero_weights_grader.grade(self.empty_gradesheet), + all_zero_weights_grader.grade(self.empty_gradesheet), + ]: self.assertAlmostEqual(graded['percent'], 0.0) self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1) self.assertEqual(len(graded['grade_breakdown']), 3) diff --git a/common/test/data/blocks/capa.xml b/common/test/data/blocks/capa.xml new file mode 100644 index 0000000000..a3460b1ef8 --- /dev/null +++ b/common/test/data/blocks/capa.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/common/test/data/blocks/coderesponse.xml b/common/test/data/blocks/coderesponse.xml new file mode 100644 index 0000000000..fde73aa327 --- /dev/null +++ b/common/test/data/blocks/coderesponse.xml @@ -0,0 +1,25 @@ + + +

+ ESTIMATED TIME TO COMPLETE: 4 minutes +

+ 
+      >>> print testList
+      [1, 16, 64, 81]
+
+ + + + + +# Your Code Here + + +def square(a): + return a * a +applyToEach(testList, square) + + {"grader": "finger_exercises/L6/applyToEach3/grade_ate3.py"} + + + diff --git a/common/test/data/blocks/library_content.xml b/common/test/data/blocks/library_content.xml new file mode 100644 index 0000000000..6c8fc8105b --- /dev/null +++ b/common/test/data/blocks/library_content.xml @@ -0,0 +1,4 @@ + + + + diff --git a/common/test/data/blocks/lti.xml b/common/test/data/blocks/lti.xml new file mode 100644 index 0000000000..ae8bed35c1 --- /dev/null +++ b/common/test/data/blocks/lti.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/blocks/openassessment.xml b/common/test/data/blocks/openassessment.xml new file mode 100644 index 0000000000..12b0919ba6 --- /dev/null +++ b/common/test/data/blocks/openassessment.xml @@ -0,0 +1,87 @@ + + + + + + + + + Censorship in the Libraries + + 'All of us can think of a book that we hope none of our children or any + other children have taken off the shelf. But if I have the right to remove + that book from the shelf -- that work I abhor -- then you also have exactly + the same right and so does everyone else. And then we have no books left on + the shelf for any of us.' --Katherine Paterson, Author + + Write a persuasive essay to a newspaper reflecting your views on censorship + in libraries. Do you believe that certain materials, such as books, music, + movies, magazines, etc., should be removed from the shelves if they are + found offensive? Support your position with convincing arguments from your + own experience, observations, and/or reading. + + Read for conciseness, clarity of thought, and form. + + + Ideas + Determine if there is a unifying theme or main idea. + + + + + + Content + Assess the content of the submission + + + + + + + diff --git a/lms/djangoapps/gating/tests/test_api.py b/lms/djangoapps/gating/tests/test_api.py index a217091582..5e63cf9ef3 100644 --- a/lms/djangoapps/gating/tests/test_api.py +++ b/lms/djangoapps/gating/tests/test_api.py @@ -166,8 +166,8 @@ class TestGatedContent(GatingTestCase, MilestonesTestCaseMixin): """ course_grade = CourseGradeFactory(user).create(self.course) for prob in [self.gating_prob1, self.gated_prob2, self.prob3]: - self.assertIn(prob.location, course_grade.locations_to_weighted_scores) - self.assertNotIn(self.orphan.location, course_grade.locations_to_weighted_scores) + self.assertIn(prob.location, course_grade.locations_to_scores) + self.assertNotIn(self.orphan.location, course_grade.locations_to_scores) self.assertEquals(course_grade.percent, expected_percent) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index eabf9037ed..19241ad08d 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -27,7 +27,7 @@ BLOCK_RECORD_LIST_VERSION = 1 # Used to serialize information about a block at the time it was used in # grade calculation. -BlockRecord = namedtuple('BlockRecord', ['locator', 'weight', 'max_score']) +BlockRecord = namedtuple('BlockRecord', ['locator', 'weight', 'raw_possible', 'graded']) class BlockRecordList(tuple): @@ -98,7 +98,8 @@ class BlockRecordList(tuple): BlockRecord( locator=UsageKey.from_string(block["locator"]).replace(course_key=course_key), weight=block["weight"], - max_score=block["max_score"], + raw_possible=block["raw_possible"], + graded=block["graded"], ) for block in block_dicts ) diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index 5fd1a7692c..30712e7202 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/lms/djangoapps/grades/new/course_grade.py @@ -43,15 +43,15 @@ class CourseGrade(object): return subsections_by_format @lazy - def locations_to_weighted_scores(self): + def locations_to_scores(self): """ Returns a dict of problem scores keyed by their locations. """ - locations_to_weighted_scores = {} + locations_to_scores = {} for chapter in self.chapter_grades: for subsection_grade in chapter['sections']: - locations_to_weighted_scores.update(subsection_grade.locations_to_weighted_scores) - return locations_to_weighted_scores + locations_to_scores.update(subsection_grade.locations_to_scores) + return locations_to_scores @lazy def grade_value(self): @@ -113,7 +113,7 @@ class CourseGrade(object): grade_summary['percent'] = self.percent grade_summary['grade'] = self.letter_grade grade_summary['totaled_scores'] = self.subsection_grade_totals_by_format - grade_summary['raw_scores'] = list(self.locations_to_weighted_scores.itervalues()) + grade_summary['raw_scores'] = list(self.locations_to_scores.itervalues()) return grade_summary @@ -141,7 +141,7 @@ class CourseGrade(object): subsections_total = sum(len(x) for x in self.subsection_grade_totals_by_format.itervalues()) subsections_read = len(subsection_grade_factory._unsaved_subsection_grades) # pylint: disable=protected-access subsections_created = subsections_total - subsections_read - blocks_total = len(self.locations_to_weighted_scores) + blocks_total = len(self.locations_to_scores) if not read_only: subsection_grade_factory.bulk_create_unsaved() @@ -166,8 +166,8 @@ class CourseGrade(object): composite module (a vertical or section ) the scores will be the sums of all scored problems that are children of the chosen location. """ - if location in self.locations_to_weighted_scores: - score, _ = self.locations_to_weighted_scores[location] + if location in self.locations_to_scores: + score = self.locations_to_scores[location] return score.earned, score.possible children = self.course_structure.get_children(location) earned = 0.0 diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index b4ae910618..8f722523ec 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -11,12 +11,11 @@ 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 lms.djangoapps.grades.transformer import GradesTransformer from student.models import anonymous_id_for_user, User from submissions import api as submissions_api from traceback import format_exc from xmodule import block_metadata_utils, graders -from xmodule.graders import Score +from xmodule.graders import AggregatedScore log = getLogger(__name__) @@ -54,62 +53,47 @@ class SubsectionGrade(object): self.graded_total = None # aggregated grade for all graded problems self.all_total = None # aggregated grade for all problems, regardless of whether they are graded - self.locations_to_weighted_scores = OrderedDict() # dict of problem locations to (Score, weight) tuples - - self._scores = None + self.locations_to_scores = OrderedDict() # dict of problem locations to ProblemScore @property def scores(self): """ List of all problem scores in the subsection. """ - if self._scores is None: - self._scores = [score for score, _ in self.locations_to_weighted_scores.itervalues()] - return self._scores + return self.locations_to_scores.values() - def init_from_structure(self, student, course_structure, scores_client, submissions_scores): + def init_from_structure(self, student, course_structure, submissions_scores, csm_scores): """ Compute the grade of this subsection for the given student and course. """ - assert self._scores is None for descendant_key in course_structure.post_order_traversal( filter_func=possibly_scored, start_node=self.location, ): - self._compute_block_score( - student, descendant_key, course_structure, scores_client, submissions_scores, persisted_values={}, - ) + self._compute_block_score(descendant_key, course_structure, submissions_scores, csm_scores) + 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) - def init_from_model(self, student, model, course_structure, scores_client, submissions_scores): + def init_from_model(self, student, model, course_structure, submissions_scores, csm_scores): """ Load the subsection grade from the persisted model. """ - assert self._scores is None for block in model.visible_blocks.blocks: - persisted_values = {'weight': block.weight, 'possible': block.max_score} - self._compute_block_score( - student, - block.locator, - course_structure, - scores_client, - submissions_scores, - persisted_values - ) + self._compute_block_score(block.locator, course_structure, submissions_scores, csm_scores, block) - self.graded_total = Score( - earned=model.earned_graded, - possible=model.possible_graded, + self.graded_total = AggregatedScore( + tw_earned=model.earned_graded, + tw_possible=model.possible_graded, graded=True, - section=self.display_name, + display_name=self.display_name, module_id=self.location, ) - self.all_total = Score( - earned=model.earned_all, - possible=model.possible_all, + self.all_total = AggregatedScore( + tw_earned=model.earned_all, + tw_possible=model.possible_all, graded=False, - section=self.display_name, + display_name=self.display_name, module_id=self.location, ) self._log_event(log.debug, u"init_from_model", student) @@ -140,12 +124,11 @@ class SubsectionGrade(object): def _compute_block_score( self, - student, block_key, course_structure, - scores_client, submissions_scores, - persisted_values, + csm_scores, + persisted_block=None, ): """ Compute score for the given block. If persisted_values @@ -154,54 +137,14 @@ class SubsectionGrade(object): block = course_structure[block_key] if getattr(block, 'has_score', False): - - possible = persisted_values.get('possible', None) - weight = persisted_values.get('weight', getattr(block, 'weight', None)) - - (earned, possible) = get_score( - student, - block, - scores_client, + problem_score = get_score( submissions_scores, - weight, - possible, + csm_scores, + persisted_block, + block, ) - - if earned is not None or possible is not None: - # There's a chance that the value of graded is not the same - # value when the problem was scored. Since we get the value - # from the block_structure. - # - # Cannot grade a problem with a denominator of 0. - # TODO: None > 0 is not python 3 compatible. - block_graded = self._get_explicit_graded(block, course_structure) if possible > 0 else False - - self.locations_to_weighted_scores[block.location] = ( - Score( - earned, - possible, - block_graded, - block_metadata_utils.display_name_with_default_escaped(block), - block.location, - ), - weight, - ) - - def _get_explicit_graded(self, block, course_structure): - """ - Returns the explicit graded field value for the given block - """ - field_value = course_structure.get_transformer_block_field( - block.location, - GradesTransformer, - GradesTransformer.EXPLICIT_GRADED_FIELD_NAME - ) - - # Set to True if grading is not explicitly disabled for - # this block. This allows us to include the block's score - # in the aggregated self.graded_total, regardless of the - # inherited graded value from the subsection. (TNL-5560) - return True if field_value is None else field_value + if problem_score: + self.locations_to_scores[block_key] = problem_score def _persisted_model_params(self, student): """ @@ -226,9 +169,9 @@ class SubsectionGrade(object): Returns the list of visible blocks. """ return [ - BlockRecord(location, weight, score.possible) - for location, (score, weight) in - self.locations_to_weighted_scores.iteritems() + BlockRecord(location, score.weight, score.raw_possible, score.graded) + for location, score in + self.locations_to_scores.iteritems() ] def _log_event(self, log_func, log_statement, student): @@ -283,7 +226,7 @@ class SubsectionGradeFactory(object): if not subsection_grade: subsection_grade = SubsectionGrade(subsection, self.course) subsection_grade.init_from_structure( - self.student, block_structure, self._scores_client, self._submissions_scores + self.student, block_structure, self._submissions_scores, self._csm_scores, ) if PersistentGradesEnabledFlag.feature_enabled(self.course.id): if read_only: @@ -313,7 +256,7 @@ class SubsectionGradeFactory(object): block_structure = self._get_block_structure(block_structure) subsection_grade = SubsectionGrade(subsection, self.course) subsection_grade.init_from_structure( - self.student, block_structure, self._scores_client, self._submissions_scores + self.student, block_structure, self._submissions_scores, self._csm_scores ) if PersistentGradesEnabledFlag.feature_enabled(self.course.id): @@ -323,7 +266,7 @@ class SubsectionGradeFactory(object): return subsection_grade @lazy - def _scores_client(self): + def _csm_scores(self): """ Lazily queries and returns all the scores stored in the user state (in CSM) for the course, while caching the result. @@ -351,7 +294,7 @@ class SubsectionGradeFactory(object): if saved_subsection_grade: subsection_grade = SubsectionGrade(subsection, self.course) subsection_grade.init_from_model( - self.student, saved_subsection_grade, block_structure, self._scores_client, self._submissions_scores + self.student, saved_subsection_grade, block_structure, self._submissions_scores, self._csm_scores, ) return subsection_grade diff --git a/lms/djangoapps/grades/scores.py b/lms/djangoapps/grades/scores.py index d711ce2854..86832ef156 100644 --- a/lms/djangoapps/grades/scores.py +++ b/lms/djangoapps/grades/scores.py @@ -5,14 +5,236 @@ from logging import getLogger from openedx.core.lib.cache_utils import memoized from xblock.core import XBlock +from xmodule.block_metadata_utils import display_name_with_default_escaped +from xmodule.graders import ProblemScore from .transformer import GradesTransformer log = getLogger(__name__) +def possibly_scored(usage_key): + """ + Returns whether the given block could impact grading (i.e. + has_score or has_children). + """ + return usage_key.block_type in _block_types_possibly_scored() + + +def get_score(submissions_scores, csm_scores, persisted_block, block): + """ + Returns the score for a problem, as a ProblemScore object. It is + assumed that the provided storages have already been filtered for + a single user in question and have user-specific values. + + The score is retrieved from the provided storages in the following + order of precedence. If no value for the block is found in a + given storage, the next storage is checked. + + submissions_scores (dict of {unicode(usage_key): (earned, possible)}): + + A python dictionary of serialized UsageKeys to (earned, possible) + tuples. These values, retrieved using the Submissions API by the + caller (already filtered for the user and course), take precedence + above all other score storages. + + When the score is found in this storage, it implies the user's score + for the block was persisted via the submissions API. Typically, this API + is used by ORA. + + The returned score includes valid values for: + weighted_earned + weighted_possible + graded - retrieved from the persisted block, if found, else from + the latest block content. + + Note: raw_earned and raw_possible are not required when submitting scores + via the submissions API, so those values (along with the unused weight) + are invalid and irrelevant. + + csm_scores (ScoresClient): + + The ScoresClient object (already filtered for the user and course), + from which a courseware.models.StudentModule object can be retrieved for + the block. + + When the score is found from this storage, it implies the user's score + for the block was persisted in the Courseware Student Module. Typically, + this storage is used for all CAPA problems, including scores calculated + by external graders. + + The returned score includes valid values for: + raw_earned, raw_possible - retrieved from CSM + weighted_earned, weighted_possible - calculated from the raw scores and weight + weight, graded - retrieved from the persisted block, if found, + else from the latest block content + + persisted_block (.models.BlockRecord): + The block values as found in the grades persistence layer. These values + are used only if not found from an earlier storage, and take precedence + over values stored within the latest content-version of the block. + + When the score is found from this storage, it implies the user has not + yet attempted this problem, but the user's grade _was_ persisted. + + The returned score includes valid values for: + raw_earned - will equal 0.0 since the user's score was not found from + earlier storages + raw_possible - retrieved from the persisted block + weighted_earned, weighted_possible - calculated from the raw scores and weight + weight, graded - retrieved from the persisted block + + block (block_structure.BlockData): + Values from the latest content-version of the block are used only if + they were not available from a prior storage. + + When the score is found from this storage, it implies the user has not + yet attempted this problem and the user's grade was _not_ yet persisted. + + The returned score includes valid values for: + raw_earned - will equal 0.0 since the user's score was not found from + earlier storages + raw_possible - retrieved from the latest block content + weighted_earned, weighted_possible - calculated from the raw scores and weight + weight, graded - retrieved from the latest block content + """ + weight = _get_weight_from_block(persisted_block, block) + + # Priority order for retrieving the scores: + # submissions API -> CSM -> grades persisted block -> latest block content + raw_earned, raw_possible, weighted_earned, weighted_possible = ( + _get_score_from_submissions(submissions_scores, block) or + _get_score_from_csm(csm_scores, block, weight) or + _get_score_from_persisted_or_latest_block(persisted_block, block, weight) + ) + + assert weighted_possible is not None + has_valid_denominator = weighted_possible > 0.0 + graded = _get_graded_from_block(persisted_block, block) if has_valid_denominator else False + + return ProblemScore( + raw_earned, + raw_possible, + weighted_earned, + weighted_possible, + weight, + graded, + display_name=display_name_with_default_escaped(block), + module_id=block.location, + ) + + +def weighted_score(raw_earned, raw_possible, weight): + """ + Returns a tuple that represents the weighted (earned, possible) score. + If weight is None or raw_possible is 0, returns the original values. + + When weight is used, it defines the weighted_possible. This allows + course authors to specify the exact maximum value for a problem when + they provide a weight. + """ + assert raw_possible is not None + cannot_compute_with_weight = weight is None or raw_possible == 0 + if cannot_compute_with_weight: + return raw_earned, raw_possible + else: + return float(raw_earned) * weight / raw_possible, float(weight) + + +def _get_score_from_submissions(submissions_scores, block): + """ + Returns the score values from the submissions API if found. + """ + if submissions_scores: + submission_value = submissions_scores.get(unicode(block.location)) + if submission_value: + weighted_earned, weighted_possible = submission_value + assert weighted_earned >= 0.0 and weighted_possible > 0.0 # per contract from submissions API + return (None, None) + (weighted_earned, weighted_possible) + + +def _get_score_from_csm(csm_scores, block, weight): + """ + Returns the score values from the courseware student module, via + ScoresClient, if found. + """ + # If an entry exists and has raw_possible (total) associated with it, we trust + # that value. This is important for cases where a student might have seen an + # older version of the problem -- they're still graded on what was possible + # when they tried the problem, not what it's worth now. + # + # Note: Storing raw_possible in CSM predates the implementation of the grades + # own persistence layer. Hence, we have duplicate storage locations for + # raw_possible, with potentially conflicting values, when a problem is + # attempted. Even though the CSM persistence for this value is now + # superfluous, for backward compatibility, we continue to use its value for + # raw_possible, giving it precedence over the one in the grades data model. + score = csm_scores.get(block.location) + has_valid_score = score and score.total is not None + if has_valid_score: + raw_earned = score.correct if score.correct is not None else 0.0 + raw_possible = score.total + return (raw_earned, raw_possible) + weighted_score(raw_earned, raw_possible, weight) + + +def _get_score_from_persisted_or_latest_block(persisted_block, block, weight): + """ + Returns the score values, now assuming the earned score is 0.0 - since a + score was not found in an earlier storage. + Uses the raw_possible value from the persisted_block if found, else from + the latest block content. + """ + raw_earned = 0.0 + + if persisted_block: + raw_possible = persisted_block.raw_possible + else: + raw_possible = block.transformer_data[GradesTransformer].max_score + + return (raw_earned, raw_possible) + weighted_score(raw_earned, raw_possible, weight) + + +def _get_weight_from_block(persisted_block, block): + """ + Returns the weighted value from the persisted_block if found, else from + the latest block content. + """ + if persisted_block: + return persisted_block.weight + else: + return getattr(block, 'weight', None) + + +def _get_graded_from_block(persisted_block, block): + """ + Returns the graded value from the persisted_block if found, else from + the latest block content. + """ + if persisted_block: + return persisted_block.graded + else: + return _get_explicit_graded(block) + + +def _get_explicit_graded(block): + """ + Returns the explicit graded field value for the given block. + """ + field_value = getattr( + block.transformer_data[GradesTransformer], + GradesTransformer.EXPLICIT_GRADED_FIELD_NAME, + None, + ) + + # Set to True if grading is not explicitly disabled for + # this block. This allows us to include the block's score + # in the aggregated self.graded_total, regardless of the + # inherited graded value from the subsection. (TNL-5560) + return True if field_value is None else field_value + + @memoized -def block_types_possibly_scored(): +def _block_types_possibly_scored(): """ Returns the block types that could have a score. @@ -22,89 +244,7 @@ def block_types_possibly_scored(): which have state but cannot ever impact someone's grade. """ return frozenset( - cat for (cat, xblock_class) in XBlock.load_classes() if ( + category for (category, xblock_class) in XBlock.load_classes() if ( getattr(xblock_class, 'has_score', False) or getattr(xblock_class, 'has_children', False) ) ) - - -def possibly_scored(usage_key): - """ - Returns whether the given block could impact grading (i.e. scored, or has children). - """ - return usage_key.block_type in block_types_possibly_scored() - - -def weighted_score(raw_earned, raw_possible, weight=None): - """ - Return a tuple that represents the weighted (earned, possible) score. - If weight is None or raw_possible is 0, returns the original values. - """ - if weight is None or raw_possible == 0: - return (raw_earned, raw_possible) - return float(raw_earned) * weight / raw_possible, float(weight) - - -def get_score(user, block, scores_client, submissions_scores_cache, weight, possible=None): - """ - Return the score for a user on a problem, as a tuple (earned, possible). - e.g. (5,7) if you got 5 out of 7 points. - - If this problem doesn't have a score, or we couldn't load it, returns (None, - None). - - user: a Student object - block: a BlockStructure's BlockData object - scores_client: an initialized ScoresClient - submissions_scores_cache: A dict of location names to (earned, possible) - point tuples. If an entry is found in this cache, it takes precedence. - weight: The weight of the problem to use in the calculation. A value of - None signifies that the weight should not be applied. - possible (optional): The possible maximum score of the problem to use in the - calculation. If None, uses the value found either in scores_client or - from the block. - """ - submissions_scores_cache = submissions_scores_cache or {} - - if not user.is_authenticated(): - return (None, None) - - location_url = unicode(block.location) - if location_url in submissions_scores_cache: - return submissions_scores_cache[location_url] - - if not getattr(block, 'has_score', False): - # These are not problems, and do not have a score - return (None, None) - - # Check the score that comes from the ScoresClient (out of CSM). - # If an entry exists and has a total associated with it, we trust that - # value. This is important for cases where a student might have seen an - # older version of the problem -- they're still graded on what was possible - # when they tried the problem, not what it's worth now. - score = scores_client.get(block.location) - if score and score.total is not None: - # We have a valid score, just use it. - earned = score.correct if score.correct is not None else 0.0 - if possible is None: - possible = score.total - elif possible != score.total: - log.error( - u"Persistent Grades: scores.get_score, possible value {} != score.total value {}".format( - possible, - score.total - ) - ) - else: - # This means we don't have a valid score entry and we don't have a - # cached_max_score on hand. We know they've earned 0.0 points on this. - earned = 0.0 - if possible is None: - possible = block.transformer_data[GradesTransformer].max_score - - # Problem may be an error module (if something in the problem builder failed) - # In which case possible might be None - if possible is None: - return (None, None) - - return weighted_score(earned, possible, weight) diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index c937a71733..038fff9147 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -2,14 +2,21 @@ Test grade calculation. """ +import ddt from django.http import Http404 +import itertools from mock import patch from nose.plugins.attrib import attr from opaque_keys.edx.locations import SlashSeparatedCourseKey +from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from courseware.tests.helpers import get_request_for_user +from lms.djangoapps.course_blocks.api import get_course_blocks from student.tests.factories import UserFactory from student.models import CourseEnrollment +from xmodule.block_metadata_utils import display_name_with_default_escaped +from xmodule.graders import ProblemScore +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -17,6 +24,7 @@ from .utils import answer_problem from .. import course_grades from ..course_grades import summary as grades_summary from ..new.course_grade import CourseGradeFactory +from ..new.subsection_grade import SubsectionGradeFactory def _grade_with_errors(student, course): @@ -33,6 +41,17 @@ def _grade_with_errors(student, course): return grades_summary(student, course) +def _create_problem_xml(): + """ + Creates and returns XML for a multiple choice response problem + """ + return MultipleChoiceResponseXMLFactory().build_xml( + question_text='The correct answer is Choice 3', + choices=[False, False, True, False], + choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] + ) + + @attr(shard=1) class TestGradeIteration(SharedModuleStoreTestCase): """ @@ -134,6 +153,101 @@ class TestGradeIteration(SharedModuleStoreTestCase): return students_to_gradesets, students_to_errors +@ddt.ddt +class TestWeightedProblems(SharedModuleStoreTestCase): + """ + Test scores and grades with various problem weight values. + """ + @classmethod + def setUpClass(cls): + super(TestWeightedProblems, cls).setUpClass() + cls.course = CourseFactory.create() + cls.chapter = ItemFactory.create(parent=cls.course, category="chapter", display_name="chapter") + cls.sequential = ItemFactory.create(parent=cls.chapter, category="sequential", display_name="sequential") + cls.vertical = ItemFactory.create(parent=cls.sequential, category="vertical", display_name="vertical1") + problem_xml = _create_problem_xml() + cls.problems = [] + for i in range(2): + cls.problems.append( + ItemFactory.create( + parent=cls.vertical, + category="problem", + display_name="problem_{}".format(i), + data=problem_xml, + ) + ) + + def setUp(self): + super(TestWeightedProblems, self).setUp() + self.user = UserFactory() + self.request = get_request_for_user(self.user) + + def _verify_grades(self, raw_earned, raw_possible, weight, expected_score): + """ + Verifies the computed grades are as expected. + """ + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + # pylint: disable=no-member + for problem in self.problems: + problem.weight = weight + self.store.update_item(problem, self.user.id) + self.store.publish(self.course.location, self.user.id) + + course_structure = get_course_blocks(self.request.user, self.course.location) + + # answer all problems + for problem in self.problems: + answer_problem(self.course, self.request, problem, score=raw_earned, max_value=raw_possible) + + # get grade + subsection_grade = SubsectionGradeFactory( + self.request.user, self.course, course_structure + ).update(self.sequential) + + # verify all problem grades + for problem in self.problems: + problem_score = subsection_grade.locations_to_scores[problem.location] + expected_score.display_name = display_name_with_default_escaped(problem) + expected_score.module_id = problem.location + self.assertEquals(problem_score, expected_score) + + # verify subsection grades + self.assertEquals(subsection_grade.all_total.earned, expected_score.earned * len(self.problems)) + self.assertEquals(subsection_grade.all_total.possible, expected_score.possible * len(self.problems)) + + @ddt.data( + *itertools.product( + (0.0, 0.5, 1.0, 2.0), # raw_earned + (-2.0, -1.0, 0.0, 0.5, 1.0, 2.0), # raw_possible + (-2.0, -1.0, -0.5, 0.0, 0.5, 1.0, 2.0, 50.0, None), # weight + ) + ) + @ddt.unpack + def test_problem_weight(self, raw_earned, raw_possible, weight): + + use_weight = weight is not None and raw_possible != 0 + if use_weight: + expected_w_earned = raw_earned / raw_possible * weight + expected_w_possible = weight + else: + expected_w_earned = raw_earned + expected_w_possible = raw_possible + + expected_graded = expected_w_possible > 0 + + expected_score = ProblemScore( + raw_earned=raw_earned, + raw_possible=raw_possible, + weighted_earned=expected_w_earned, + weighted_possible=expected_w_possible, + weight=weight, + graded=expected_graded, + display_name=None, # problem-specific, filled in by _verify_grades + module_id=None, # problem-specific, filled in by _verify_grades + ) + self._verify_grades(raw_earned, raw_possible, weight, expected_score) + + class TestScoreForModule(SharedModuleStoreTestCase): """ Test the method that calculates the score for a given block based on the diff --git a/lms/djangoapps/grades/tests/test_models.py b/lms/djangoapps/grades/tests/test_models.py index e3dd0a24a7..16fe1ea008 100644 --- a/lms/djangoapps/grades/tests/test_models.py +++ b/lms/djangoapps/grades/tests/test_models.py @@ -71,8 +71,8 @@ class GradesModelTestCase(TestCase): block_type='problem', block_id='block_id_b' ) - self.record_a = BlockRecord(locator=self.locator_a, weight=1, max_score=10) - self.record_b = BlockRecord(locator=self.locator_b, weight=1, max_score=10) + self.record_a = BlockRecord(locator=self.locator_a, weight=1, raw_possible=10, graded=False) + self.record_b = BlockRecord(locator=self.locator_b, weight=1, raw_possible=10, graded=True) @ddt.ddt @@ -88,29 +88,31 @@ class BlockRecordTest(GradesModelTestCase): Tests creation of a BlockRecord. """ weight = 1 - max_score = 10 + raw_possible = 10 record = BlockRecord( self.locator_a, weight, - max_score, + raw_possible, + graded=False, ) self.assertEqual(record.locator, self.locator_a) @ddt.data( - (0, 0, "0123456789abcdef"), - (1, 10, 'totally_a_real_block_key'), - ("BlockRecord is", "a dumb data store", "with no validation"), + (0, 0, "0123456789abcdef", True), + (1, 10, 'totally_a_real_block_key', False), + ("BlockRecord is", "a dumb data store", "with no validation", None), ) @ddt.unpack - def test_serialization(self, weight, max_score, block_key): + def test_serialization(self, weight, raw_possible, block_key, graded): """ Tests serialization of a BlockRecord using the _asdict() method. """ - record = BlockRecord(block_key, weight, max_score) + record = BlockRecord(block_key, weight, raw_possible, graded) expected = OrderedDict([ ("locator", block_key), ("weight", weight), - ("max_score", max_score), + ("raw_possible", raw_possible), + ("graded", graded), ]) self.assertEqual(expected, record._asdict()) @@ -134,7 +136,12 @@ class VisibleBlocksTest(GradesModelTestCase): for block_dict in list_of_block_dicts: block_dict['locator'] = unicode(block_dict['locator']) # BlockUsageLocator is not json-serializable expected_data = { - 'blocks': [{'locator': unicode(self.record_a.locator), 'max_score': 10, 'weight': 1}], + 'blocks': [{ + 'locator': unicode(self.record_a.locator), + 'raw_possible': 10, + 'weight': 1, + 'graded': self.record_a.graded, + }], 'course_key': unicode(self.record_a.locator.course_key), 'version': BLOCK_RECORD_LIST_VERSION, } diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index eaa8fe7bda..4cecb8ee73 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -1,25 +1,31 @@ """ Test saved subsection grade functionality. """ +# pylint: disable=protected-access + +import datetime import ddt from django.conf import settings from django.db.utils import DatabaseError from mock import patch +import pytz from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from courseware.tests.helpers import get_request_for_user +from courseware.tests.test_submitting_problems import ProblemSubmissionTestMixin from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags +from openedx.core.lib.xblock_utils.test_utils import add_xml_block_from_file from student.models import CourseEnrollment from student.tests.factories import UserFactory -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from ..models import PersistentSubsectionGrade from ..new.course_grade import CourseGradeFactory from ..new.subsection_grade import SubsectionGrade, SubsectionGradeFactory -from lms.djangoapps.grades.tests.utils import mock_get_score +from .utils import mock_get_score class GradeTestBase(SharedModuleStoreTestCase): @@ -116,7 +122,7 @@ class SubsectionGradeFactoryTest(GradeTestBase): ) as mock_create_grade: with patch( 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', - wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access + wraps=self.subsection_grade_factory._get_saved_grade ) as mock_get_saved_grade: with self.assertNumQueries(14): grade_a = self.subsection_grade_factory.create(self.sequence) @@ -205,8 +211,8 @@ class SubsectionGradeTest(GradeTestBase): input_grade.init_from_structure( self.request.user, self.course_structure, - self.subsection_grade_factory._scores_client, # pylint: disable=protected-access - self.subsection_grade_factory._submissions_scores, # pylint: disable=protected-access + self.subsection_grade_factory._submissions_scores, + self.subsection_grade_factory._csm_scores, ) self.assertEqual(PersistentSubsectionGrade.objects.count(), 0) @@ -224,9 +230,154 @@ class SubsectionGradeTest(GradeTestBase): self.request.user, saved_model, self.course_structure, - self.subsection_grade_factory._scores_client, # pylint: disable=protected-access - self.subsection_grade_factory._submissions_scores, # pylint: disable=protected-access + self.subsection_grade_factory._submissions_scores, + self.subsection_grade_factory._csm_scores, ) self.assertEqual(input_grade.url_name, loaded_grade.url_name) self.assertEqual(input_grade.all_total, loaded_grade.all_total) + + +@ddt.ddt +class TestMultipleProblemTypesSubsectionScores(ModuleStoreTestCase, ProblemSubmissionTestMixin): + """ + Test grading of different problem types. + """ + + default_problem_metadata = { + u'graded': True, + u'weight': 2.5, + u'max_score': 7.0, + u'due': datetime.datetime(2099, 3, 15, 12, 30, 0, tzinfo=pytz.utc), + } + + COURSE_NAME = u'Problem Type Test Course' + COURSE_NUM = u'probtype' + + def setUp(self): + super(TestMultipleProblemTypesSubsectionScores, self).setUp() + password = u'test' + self.student = UserFactory.create(is_staff=False, username=u'test_student', password=password) + self.client.login(username=self.student.username, password=password) + self.request = get_request_for_user(self.student) + self.course = CourseFactory.create( + display_name=self.COURSE_NAME, + number=self.COURSE_NUM + ) + self.chapter = ItemFactory.create( + parent=self.course, + category=u'chapter', + display_name=u'Test Chapter' + ) + self.seq1 = ItemFactory.create( + parent=self.chapter, + category=u'sequential', + display_name=u'Test Sequential 1', + graded=True + ) + self.vert1 = ItemFactory.create( + parent=self.seq1, + category=u'vertical', + display_name=u'Test Vertical 1' + ) + + def _get_fresh_subsection_score(self, course_structure, subsection): + """ + Return a Score object for the specified subsection. + + Ensures that a stale cached value is not returned. + """ + subsection_factory = SubsectionGradeFactory( + self.student, + course_structure=course_structure, + course=self.course, + ) + return subsection_factory.update(subsection) + + def _get_altered_metadata(self, alterations): + """ + Returns a copy of the default_problem_metadata dict updated with the + specified alterations. + """ + metadata = self.default_problem_metadata.copy() + metadata.update(alterations) + return metadata + + def _get_score_with_alterations(self, alterations): + """ + Given a dict of alterations to the default_problem_metadata, return + the score when one correct problem (out of two) is submitted. + """ + metadata = self._get_altered_metadata(alterations) + + add_xml_block_from_file(u'problem', u'capa.xml', parent=self.vert1, metadata=metadata) + course_structure = get_course_blocks(self.student, self.course.location) + + self.submit_question_answer(u'problem', {u'2_1': u'Correct'}) + return self._get_fresh_subsection_score(course_structure, self.seq1) + + def test_score_submission_for_capa_problems(self): + add_xml_block_from_file(u'problem', u'capa.xml', parent=self.vert1, metadata=self.default_problem_metadata) + course_structure = get_course_blocks(self.student, self.course.location) + + score = self._get_fresh_subsection_score(course_structure, self.seq1) + self.assertEqual(score.all_total.earned, 0.0) + self.assertEqual(score.all_total.possible, 2.5) + + self.submit_question_answer(u'problem', {u'2_1': u'Correct'}) + score = self._get_fresh_subsection_score(course_structure, self.seq1) + self.assertEqual(score.all_total.earned, 1.25) + self.assertEqual(score.all_total.possible, 2.5) + + @ddt.data( + (u'openassessment', u'openassessment.xml'), + (u'coderesponse', u'coderesponse.xml'), + (u'lti', u'lti.xml'), + (u'library_content', u'library_content.xml'), + ) + @ddt.unpack + def test_loading_different_problem_types(self, block_type, filename): + """ + Test that transformation works for various block types + """ + metadata = self.default_problem_metadata.copy() + if block_type == u'library_content': + # Library content does not have a weight + del metadata[u'weight'] + add_xml_block_from_file(block_type, filename, parent=self.vert1, metadata=metadata) + + @ddt.data( + ({}, 1.25, 2.5), + ({u'weight': 27}, 13.5, 27), + ({u'weight': 1.0}, 0.5, 1.0), + ({u'weight': 0.0}, 0.0, 0.0), + ({u'weight': None}, 1.0, 2.0), + ) + @ddt.unpack + def test_weight_metadata_alterations(self, alterations, expected_earned, expected_possible): + score = self._get_score_with_alterations(alterations) + self.assertEqual(score.all_total.earned, expected_earned) + self.assertEqual(score.all_total.possible, expected_possible) + + @ddt.data( + ({u'graded': True}, 1.25, 2.5), + ({u'graded': False}, 0.0, 0.0), + ) + @ddt.unpack + def test_graded_metadata_alterations(self, alterations, expected_earned, expected_possible): + score = self._get_score_with_alterations(alterations) + self.assertEqual(score.graded_total.earned, expected_earned) + self.assertEqual(score.graded_total.possible, expected_possible) + + @ddt.data( + {u'max_score': 99.3}, + {u'max_score': 1.0}, + {u'max_score': 0.0}, + {u'max_score': None}, + ) + def test_max_score_does_not_change_results(self, alterations): + expected_earned = 1.25 + expected_possible = 2.5 + score = self._get_score_with_alterations(alterations) + self.assertEqual(score.all_total.earned, expected_earned) + self.assertEqual(score.all_total.possible, expected_possible) diff --git a/lms/djangoapps/grades/tests/test_scores.py b/lms/djangoapps/grades/tests/test_scores.py new file mode 100644 index 0000000000..f7d6e34055 --- /dev/null +++ b/lms/djangoapps/grades/tests/test_scores.py @@ -0,0 +1,288 @@ +""" +Tests for grades.scores module. +""" +# pylint: disable=protected-access +from collections import namedtuple +import ddt +from django.test import TestCase +import itertools + +from lms.djangoapps.grades.models import BlockRecord +import lms.djangoapps.grades.scores as scores +from lms.djangoapps.grades.transformer import GradesTransformer +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from openedx.core.lib.block_structure.block_structure import BlockData +from xmodule.graders import ProblemScore + + +class TestScoredBlockTypes(TestCase): + """ + Tests for the possibly_scored function. + """ + possibly_scored_block_types = { + 'course', 'chapter', 'sequential', 'vertical', + 'library_content', 'split_test', 'conditional', 'library', 'randomize', + 'problem', 'drag-and-drop-v2', 'openassessment', 'lti', 'lti_consumer', + 'videosequence', 'problemset', 'acid_parent', 'done', 'wrapper', 'edx_sga', + } + + def test_block_types_possibly_scored(self): + self.assertSetEqual( + self.possibly_scored_block_types, + scores._block_types_possibly_scored() + ) + + def test_possibly_scored(self): + course_key = CourseLocator(u'org', u'course', u'run') + for block_type in self.possibly_scored_block_types: + usage_key = BlockUsageLocator(course_key, block_type, 'mock_block_id') + self.assertTrue(scores.possibly_scored(usage_key)) + + +@ddt.ddt +class TestGetScore(TestCase): + """ + Tests for get_score + """ + display_name = 'test_name' + location = 'test_location' + + SubmissionValue = namedtuple('SubmissionValue', 'exists, weighted_earned, weighted_possible') + CSMValue = namedtuple('CSMValue', 'exists, raw_earned, raw_possible') + PersistedBlockValue = namedtuple('PersistedBlockValue', 'exists, raw_possible, weight, graded') + ContentBlockValue = namedtuple('ContentBlockValue', 'raw_possible, weight, explicit_graded') + ExpectedResult = namedtuple( + 'ExpectedResult', 'raw_earned, raw_possible, weighted_earned, weighted_possible, weight, graded' + ) + + def _create_submissions_scores(self, submission_value): + """ + Creates a stub result from the submissions API for the given values. + """ + if submission_value.exists: + return {self.location: (submission_value.weighted_earned, submission_value.weighted_possible)} + else: + return {} + + def _create_csm_scores(self, csm_value): + """ + Creates a stub result from courseware student module for the given values. + """ + if csm_value.exists: + stub_csm_record = namedtuple('stub_csm_record', 'correct, total') + return {self.location: stub_csm_record(correct=csm_value.raw_earned, total=csm_value.raw_possible)} + else: + return {} + + def _create_persisted_block(self, persisted_block_value): + """ + Creates and returns a minimal BlockRecord object with the give values. + """ + if persisted_block_value.exists: + return BlockRecord( + self.location, + persisted_block_value.weight, + persisted_block_value.raw_possible, + persisted_block_value.graded, + ) + else: + return None + + def _create_block(self, content_block_value): + """ + Creates and returns a minimal BlockData object with the give values. + """ + block = BlockData(self.location) + block.display_name = self.display_name + block.weight = content_block_value.weight + + block_grades_transformer_data = block.transformer_data.get_or_create(GradesTransformer) + block_grades_transformer_data.max_score = content_block_value.raw_possible + setattr( + block_grades_transformer_data, + GradesTransformer.EXPLICIT_GRADED_FIELD_NAME, + content_block_value.explicit_graded, + ) + return block + + @ddt.data( + # submissions _trumps_ other values; weighted and graded from persisted-block _trumps_ latest content values + ( + SubmissionValue(exists=True, weighted_earned=50, weighted_possible=100), + CSMValue(exists=True, raw_earned=10, raw_possible=40), + PersistedBlockValue(exists=True, raw_possible=5, weight=40, graded=True), + ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), + ExpectedResult( + raw_earned=None, raw_possible=None, weighted_earned=50, weighted_possible=100, weight=40, graded=True + ), + ), + # same as above, except submissions doesn't exist; CSM values used + ( + SubmissionValue(exists=False, weighted_earned=50, weighted_possible=100), + CSMValue(exists=True, raw_earned=10, raw_possible=40), + PersistedBlockValue(exists=True, raw_possible=5, weight=40, graded=True), + ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), + ExpectedResult( + raw_earned=10, raw_possible=40, weighted_earned=10, weighted_possible=40, weight=40, graded=True + ), + ), + # neither submissions nor CSM exist; Persisted values used + ( + SubmissionValue(exists=False, weighted_earned=50, weighted_possible=100), + CSMValue(exists=False, raw_earned=10, raw_possible=40), + PersistedBlockValue(exists=True, raw_possible=5, weight=40, graded=True), + ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), + ExpectedResult( + raw_earned=0, raw_possible=5, weighted_earned=0, weighted_possible=40, weight=40, graded=True + ), + ), + # none of submissions, CSM, or persisted exist; Latest content values used + ( + SubmissionValue(exists=False, weighted_earned=50, weighted_possible=100), + CSMValue(exists=False, raw_earned=10, raw_possible=40), + PersistedBlockValue(exists=False, raw_possible=5, weight=40, graded=True), + ContentBlockValue(raw_possible=1, weight=20, explicit_graded=False), + ExpectedResult( + raw_earned=0, raw_possible=1, weighted_earned=0, weighted_possible=20, weight=20, graded=False + ), + ), + ) + @ddt.unpack + def test_get_score(self, submission_value, csm_value, persisted_block_value, block_value, expected_result): + score = scores.get_score( + self._create_submissions_scores(submission_value), + self._create_csm_scores(csm_value), + self._create_persisted_block(persisted_block_value), + self._create_block(block_value), + ) + expected_score = ProblemScore( + display_name=self.display_name, module_id=self.location, **expected_result._asdict() + ) + self.assertEquals(score, expected_score) + + +@ddt.ddt +class TestWeightedScore(TestCase): + """ + Tests the helper method: weighted_score + """ + @ddt.data( + (0, 0, 1), + (5, 0, 0), + (10, 0, None), + (0, 5, None), + (5, 10, None), + (10, 10, None), + ) + @ddt.unpack + def test_cannot_compute(self, raw_earned, raw_possible, weight): + self.assertEquals( + scores.weighted_score(raw_earned, raw_possible, weight), + (raw_earned, raw_possible), + ) + + @ddt.data( + (0, 5, 0, (0, 0)), + (5, 5, 0, (0, 0)), + (2, 5, 1, (.4, 1)), + (5, 5, 1, (1, 1)), + (5, 5, 3, (3, 3)), + (2, 4, 6, (3, 6)), + ) + @ddt.unpack + def test_computed(self, raw_earned, raw_possible, weight, expected_score): + self.assertEquals( + scores.weighted_score(raw_earned, raw_possible, weight), + expected_score, + ) + + def test_assert_on_invalid_r_possible(self): + with self.assertRaises(AssertionError): + scores.weighted_score(raw_earned=1, raw_possible=None, weight=1) + + +@ddt.ddt +class TestInternalGetGraded(TestCase): + """ + Tests the internal helper method: _get_explicit_graded + """ + def _create_block(self, explicit_graded_value): + """ + Creates and returns a minimal BlockData object with the give value + for explicit_graded. + """ + block = BlockData('any_key') + setattr( + block.transformer_data.get_or_create(GradesTransformer), + GradesTransformer.EXPLICIT_GRADED_FIELD_NAME, + explicit_graded_value, + ) + return block + + @ddt.data(None, True, False) + def test_with_no_persisted_block(self, explicitly_graded_value): + block = self._create_block(explicitly_graded_value) + self.assertEquals( + scores._get_graded_from_block(None, block), + explicitly_graded_value is not False, # defaults to True unless explicitly False + ) + + @ddt.data( + *itertools.product((True, False), (True, False, None)) + ) + @ddt.unpack + def test_with_persisted_block(self, persisted_block_value, block_value): + block = self._create_block(block_value) + block_record = BlockRecord(block.location, 0, 0, persisted_block_value) + self.assertEquals( + scores._get_graded_from_block(block_record, block), + block_record.graded, # persisted value takes precedence + ) + + +@ddt.ddt +class TestInternalGetScoreFromBlock(TestCase): + """ + Tests the internal helper method: _get_score_from_persisted_or_latest_block + """ + def _create_block(self, raw_possible): + """ + Creates and returns a minimal BlockData object with the give value + for raw_possible. + """ + block = BlockData('any_key') + block.transformer_data.get_or_create(GradesTransformer).max_score = raw_possible + return block + + def _verify_score_result(self, persisted_block, block, weight, expected_r_possible): + """ + Verifies the result of _get_score_from_persisted_or_latest_block is as expected. + """ + # pylint: disable=unbalanced-tuple-unpacking + raw_earned, raw_possible, weighted_earned, weighted_possible = scores._get_score_from_persisted_or_latest_block( + persisted_block, block, weight, + ) + self.assertEquals(raw_earned, 0.0) + self.assertEquals(raw_possible, expected_r_possible) + self.assertEquals(weighted_earned, 0.0) + if weight is None or expected_r_possible == 0: + self.assertEquals(weighted_possible, expected_r_possible) + else: + self.assertEquals(weighted_possible, weight) + + @ddt.data( + *itertools.product((0, 1, 5), (None, 0, 1, 5)) + ) + @ddt.unpack + def test_with_no_persisted_block(self, block_r_possible, weight): + block = self._create_block(block_r_possible) + self._verify_score_result(None, block, weight, block_r_possible) + + @ddt.data( + *itertools.product((0, 1, 5), (None, 0, 1, 5), (None, 0, 1, 5)) + ) + @ddt.unpack + def test_with_persisted_block(self, persisted_block_r_possible, block_r_possible, weight): + block = self._create_block(block_r_possible) + block_record = BlockRecord(block.location, 0, persisted_block_r_possible, False) + self._verify_score_result(block_record, block, weight, persisted_block_r_possible) diff --git a/lms/djangoapps/grades/tests/utils.py b/lms/djangoapps/grades/tests/utils.py index ce821c5724..951e800329 100644 --- a/lms/djangoapps/grades/tests/utils.py +++ b/lms/djangoapps/grades/tests/utils.py @@ -5,6 +5,7 @@ from contextlib import contextmanager from mock import patch from courseware.module_render import get_module from courseware.model_data import FieldDataCache +from xmodule.graders import ProblemScore @contextmanager @@ -23,7 +24,7 @@ def mock_get_score(earned=0, possible=1): Mocks the get_score function to return a valid grade. """ with patch('lms.djangoapps.grades.new.subsection_grade.get_score') as mock_score: - mock_score.return_value = (earned, possible) + mock_score.return_value = ProblemScore(earned, possible, earned, possible, 1, True, None, None) yield mock_score diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 3540593483..becae8f2ba 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -4,6 +4,7 @@ Enrollment operations for use by instructor APIs. Does not include any access control, be sure to check access before calling. """ +import crum import json import logging from django.contrib.auth.models import User @@ -13,11 +14,14 @@ from django.core.mail import send_mail from django.utils.translation import override as override_language from course_modes.models import CourseMode -from student.models import CourseEnrollment, CourseEnrollmentAllowed +from courseware.model_data import FieldDataCache +from courseware.module_render import get_module_for_descriptor from courseware.models import StudentModule from edxmako.shortcuts import render_to_string +from grades.scores import weighted_score +from grades.signals.signals import SCORE_CHANGED from lang_pref import LANGUAGE_KEY - +from student.models import CourseEnrollment, CourseEnrollmentAllowed from submissions import api as sub_api # installed from the edx-submissions repository from student.models import anonymous_id_for_user from openedx.core.djangoapps.user_api.models import UserPreference @@ -245,6 +249,7 @@ def reset_student_attempts(course_id, student, module_state_key, requesting_user ) submission_cleared = True except ItemNotFoundError: + block = None log.warning("Could not find %s in modulestore when attempting to reset attempts.", module_state_key) # Reset the student's score in the submissions API, if xblock.clear_student_state has not done so already. @@ -267,6 +272,7 @@ def reset_student_attempts(course_id, student, module_state_key, requesting_user if delete_module: module_to_reset.delete() + _fire_score_changed_for_block(course_id, student, block, module_state_key) else: _reset_module_attempts(module_to_reset) @@ -287,6 +293,43 @@ def _reset_module_attempts(studentmodule): studentmodule.save() +def _fire_score_changed_for_block(course_id, student, block, module_state_key): + """ + Fires a SCORE_CHANGED event for the given module. The earned points are + always zero. We must retrieve the possible points from the XModule, as + noted below. + """ + if block and block.has_score: + cache = FieldDataCache.cache_for_descriptor_descendents( + course_id=course_id, + user=student, + descriptor=block, + depth=0 + ) + # For implementation reasons, we need to pull the max_score from the XModule, + # even though the data is not user-specific. Here we bind the data to the + # current user. + request = crum.get_current_request() + module = get_module_for_descriptor( + user=student, + request=request, + descriptor=block, + field_data_cache=cache, + course_key=course_id + ) + points_earned, points_possible = weighted_score(0, module.max_score(), getattr(module, 'weight', None)) + else: + points_earned, points_possible = 0, 0 + SCORE_CHANGED.send( + sender=None, + points_possible=points_possible, + points_earned=points_earned, + user=student, + course_id=course_id, + usage_id=module_state_key + ) + + def get_email_params(course, auto_enroll, secure=True, course_key=None, display_name=None): """ Generate parameters used when parsing email templates. diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 01559dfd8f..0b65f1c8a4 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -8,6 +8,7 @@ import mock from mock import patch from abc import ABCMeta from courseware.models import StudentModule +from courseware.tests.helpers import get_request_for_user from django.conf import settings from django.utils.translation import get_language from django.utils.translation import override as override_language @@ -16,7 +17,11 @@ from ccx_keys.locator import CCXLocator from student.tests.factories import UserFactory from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory +from grades.new.subsection_grade import SubsectionGradeFactory +from grades.tests.utils import answer_problem from lms.djangoapps.ccx.tests.factories import CcxFactory +from lms.djangoapps.course_blocks.api import get_course_blocks from openedx.core.djangolib.testing.utils import CacheIsolationTestCase from student.models import CourseEnrollment, CourseEnrollmentAllowed from student.roles import CourseCcxCoachRole @@ -494,6 +499,80 @@ class TestInstructorEnrollmentStudentModule(SharedModuleStoreTestCase): self.assertEqual(unrelated_state['brains'], 'zombie') +class TestStudentModuleGrading(SharedModuleStoreTestCase): + """ + Tests the effects of student module manipulations + on student grades. + """ + @classmethod + def setUpClass(cls): + super(TestStudentModuleGrading, cls).setUpClass() + cls.course = CourseFactory.create() + cls.chapter = ItemFactory.create( + parent=cls.course, + category="chapter", + display_name="Test Chapter" + ) + cls.sequence = ItemFactory.create( + parent=cls.chapter, + category='sequential', + display_name="Test Sequential 1", + graded=True + ) + cls.vertical = ItemFactory.create( + parent=cls.sequence, + category='vertical', + display_name='Test Vertical 1' + ) + problem_xml = MultipleChoiceResponseXMLFactory().build_xml( + question_text='The correct answer is Choice 3', + choices=[False, False, True, False], + choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] + ) + cls.problem = ItemFactory.create( + parent=cls.vertical, + category="problem", + display_name="Test Problem", + data=problem_xml + ) + cls.request = get_request_for_user(UserFactory()) + cls.user = cls.request.user + + def _get_subsection_grade_and_verify(self, all_earned, all_possible, graded_earned, graded_possible): + """ + Retrieves the subsection grade and verifies that + its scores match those expected. + """ + subsection_grade_factory = SubsectionGradeFactory( + self.user, + self.course, + get_course_blocks(self.user, self.course.location) + ) + grade = subsection_grade_factory.update(self.sequence) + self.assertEqual(grade.all_total.earned, all_earned) + self.assertEqual(grade.graded_total.earned, graded_earned) + self.assertEqual(grade.all_total.possible, all_possible) + self.assertEqual(grade.graded_total.possible, graded_possible) + + @patch('crum.get_current_request') + def test_delete_student_state(self, _crum_mock): + problem_location = self.problem.location + self._get_subsection_grade_and_verify(0, 1, 0, 1) + answer_problem(course=self.course, request=self.request, problem=self.problem, score=1, max_value=1) + self._get_subsection_grade_and_verify(1, 1, 1, 1) + + # Delete student state using the instructor dash + reset_student_attempts( + self.course.id, + self.user, + problem_location, + requesting_user=self.user, + delete_module=True, + ) + # Verify that the student's grades are reset + self._get_subsection_grade_and_verify(0, 1, 0, 1) + + class EnrollmentObjects(object): """ Container for enrollment objects. diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index 32daa1be89..abede406e9 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -948,7 +948,7 @@ def upload_problem_grade_report(_xmodule_instance_args, _entry_id, course_id, _t final_grade = gradeset['percent'] # Only consider graded problems - problem_scores = {unicode(score.module_id): score for score, _ in gradeset['raw_scores'] if score.graded} + problem_scores = {unicode(score.module_id): score for score in gradeset['raw_scores'] if score.graded} earned_possible_values = list() for problem_id in problems: try: diff --git a/openedx/core/lib/xblock_utils.py b/openedx/core/lib/xblock_utils/__init__.py similarity index 100% rename from openedx/core/lib/xblock_utils.py rename to openedx/core/lib/xblock_utils/__init__.py diff --git a/openedx/core/lib/xblock_utils/test_utils.py b/openedx/core/lib/xblock_utils/test_utils.py new file mode 100644 index 0000000000..ac1b145dc0 --- /dev/null +++ b/openedx/core/lib/xblock_utils/test_utils.py @@ -0,0 +1,26 @@ +""" +Utilities for testing xblocks +""" + +from django.conf import settings + +from xmodule.modulestore.tests.factories import ItemFactory + +TEST_DATA_DIR = settings.COMMON_ROOT / u'test/data' + + +def add_xml_block_from_file(block_type, filename, parent, metadata): + """ + Create a block of the specified type with content included from the + specified XML file. + + XML filenames are relative to common/test/data/blocks. + """ + with open(TEST_DATA_DIR / u'blocks' / filename) as datafile: + return ItemFactory.create( + parent=parent, + category=block_type, + data=datafile.read().decode('utf-8'), + metadata=metadata, + display_name=u'problem' + ) From 969c75a7d17eabef2867dc0b069932869a793544 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Fri, 30 Sep 2016 15:56:19 -0400 Subject: [PATCH 06/12] Fix for grade assertions --- lms/djangoapps/grades/scores.py | 34 ++++++++++++++++------------ lms/djangoapps/grades/transformer.py | 10 ++++++-- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/grades/scores.py b/lms/djangoapps/grades/scores.py index 86832ef156..22e378bda6 100644 --- a/lms/djangoapps/grades/scores.py +++ b/lms/djangoapps/grades/scores.py @@ -108,20 +108,23 @@ def get_score(submissions_scores, csm_scores, persisted_block, block): _get_score_from_persisted_or_latest_block(persisted_block, block, weight) ) - assert weighted_possible is not None - has_valid_denominator = weighted_possible > 0.0 - graded = _get_graded_from_block(persisted_block, block) if has_valid_denominator else False + if weighted_possible is None or weighted_earned is None: + return None - return ProblemScore( - raw_earned, - raw_possible, - weighted_earned, - weighted_possible, - weight, - graded, - display_name=display_name_with_default_escaped(block), - module_id=block.location, - ) + else: + has_valid_denominator = weighted_possible > 0.0 + graded = _get_graded_from_block(persisted_block, block) if has_valid_denominator else False + + return ProblemScore( + raw_earned, + raw_possible, + weighted_earned, + weighted_possible, + weight, + graded, + display_name=display_name_with_default_escaped(block), + module_id=block.location, + ) def weighted_score(raw_earned, raw_possible, weight): @@ -191,7 +194,10 @@ def _get_score_from_persisted_or_latest_block(persisted_block, block, weight): else: raw_possible = block.transformer_data[GradesTransformer].max_score - return (raw_earned, raw_possible) + weighted_score(raw_earned, raw_possible, weight) + if raw_possible is None: + return (raw_earned, raw_possible) + (None, None) + else: + return (raw_earned, raw_possible) + weighted_score(raw_earned, raw_possible, weight) def _get_weight_from_block(persisted_block, block): diff --git a/lms/djangoapps/grades/transformer.py b/lms/djangoapps/grades/transformer.py index df2e0a4b37..481c2e700c 100644 --- a/lms/djangoapps/grades/transformer.py +++ b/lms/djangoapps/grades/transformer.py @@ -3,6 +3,7 @@ Grades Transformer """ from django.test.client import RequestFactory from functools import reduce as functools_reduce +from logging import getLogger from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor @@ -11,6 +12,9 @@ from openedx.core.lib.block_structure.transformer import BlockStructureTransform from openedx.core.djangoapps.util.user_utils import SystemUser +log = getLogger(__name__) + + class GradesTransformer(BlockStructureTransformer): """ The GradesTransformer collects grading information and stores it on @@ -119,8 +123,10 @@ class GradesTransformer(BlockStructureTransformer): Collect the `max_score` from the given module, storing it as a `transformer_block_field` associated with the `GradesTransformer`. """ - score = module.max_score() - block_structure.set_transformer_block_field(module.location, cls, 'max_score', score) + max_score = module.max_score() + block_structure.set_transformer_block_field(module.location, cls, 'max_score', max_score) + if max_score is None: + log.warning("GradesTransformer: max_score is None for {}".format(module.location)) @staticmethod def _iter_scorable_xmodules(block_structure): From 4793a427677179b4763f7240a19ea90eed8dfb1d Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Thu, 29 Sep 2016 16:15:04 -0400 Subject: [PATCH 07/12] Silence more grades-related logging --- lms/djangoapps/grades/new/subsection_grade.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 8f722523ec..1d196c4333 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -112,14 +112,14 @@ class SubsectionGrade(object): """ Saves the subsection grade in a persisted model. """ - self._log_event(log.info, u"create_model", student) + self._log_event(log.debug, u"create_model", student) return PersistentSubsectionGrade.create_grade(**self._persisted_model_params(student)) def update_or_create_model(self, student): """ Saves or updates the subsection grade in a persisted model. """ - self._log_event(log.info, u"update_or_create_model", student) + self._log_event(log.debug, u"update_or_create_model", student) return PersistentSubsectionGrade.update_or_create_grade(**self._persisted_model_params(student)) def _compute_block_score( From bcaa873be0af447c520a56cc85e29ea2770fdb6d Mon Sep 17 00:00:00 2001 From: Sanford Student Date: Fri, 30 Sep 2016 10:45:30 -0400 Subject: [PATCH 08/12] bug fix for TNL-5601 fixes signal handling for delete student state --- .../test/acceptance/pages/lms/staff_view.py | 4 ++-- .../tests/lms/test_progress_page.py | 24 +++++++++++++++++++ lms/djangoapps/instructor/enrollment.py | 8 +++---- lms/djangoapps/instructor/tests/test_api.py | 3 ++- .../instructor/tests/test_enrollment.py | 7 +++--- .../instructor/tests/test_services.py | 3 ++- 6 files changed, 38 insertions(+), 11 deletions(-) diff --git a/common/test/acceptance/pages/lms/staff_view.py b/common/test/acceptance/pages/lms/staff_view.py index db23f89fd1..c87fef1d7d 100644 --- a/common/test/acceptance/pages/lms/staff_view.py +++ b/common/test/acceptance/pages/lms/staff_view.py @@ -95,8 +95,8 @@ class StaffDebugPage(PageObject): This delete's a student's state for the problem """ if user: - self.q(css='input[id^=sd_fu_]').fill(user) - self.q(css='.staff-modal .staff-debug-sdelete').click() + self.q(css='input[id^=sd_fu_]').first.fill(user) + self.q(css='.staff-modal .staff-debug-sdelete').first.click() def rescore(self, user=None): """ diff --git a/common/test/acceptance/tests/lms/test_progress_page.py b/common/test/acceptance/tests/lms/test_progress_page.py index cad30b789c..3553cc9e44 100644 --- a/common/test/acceptance/tests/lms/test_progress_page.py +++ b/common/test/acceptance/tests/lms/test_progress_page.py @@ -15,6 +15,7 @@ from ...pages.lms.courseware import CoursewarePage from ...pages.lms.instructor_dashboard import InstructorDashboardPage 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 @@ -192,6 +193,22 @@ class PersistentGradesTest(ProgressPageBaseTest): type_in_codemirror(self, 0, modified_content) modal.q(css='.action-save').click() + def _delete_student_state_for_problem(self): + """ + 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.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) + @ddt.data( _edit_problem_content, _change_subsection_structure, @@ -223,6 +240,13 @@ class PersistentGradesTest(ProgressPageBaseTest): 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() + with self._logged_in_session(): + self.assertEqual(self._get_problem_scores(), [(0, 1), (0, 1)]) + self.assertEqual(self._get_section_score(), (0, 2)) + class SubsectionGradingPolicyTest(ProgressPageBaseTest): """ diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index becae8f2ba..fd3921431b 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -18,8 +18,8 @@ from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor from courseware.models import StudentModule from edxmako.shortcuts import render_to_string -from grades.scores import weighted_score -from grades.signals.signals import SCORE_CHANGED +from lms.djangoapps.grades.scores import weighted_score +from lms.djangoapps.grades.signals.signals import SCORE_CHANGED from lang_pref import LANGUAGE_KEY from student.models import CourseEnrollment, CourseEnrollmentAllowed from submissions import api as sub_api # installed from the edx-submissions repository @@ -325,8 +325,8 @@ def _fire_score_changed_for_block(course_id, student, block, module_state_key): points_possible=points_possible, points_earned=points_earned, user=student, - course_id=course_id, - usage_id=module_state_key + course_id=unicode(course_id), + usage_id=unicode(module_state_key) ) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 312a40adab..4a1c292eca 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -3190,7 +3190,8 @@ class TestInstructorAPIRegradeTask(SharedModuleStoreTestCase, LoginEnrollmentTes }) self.assertEqual(response.status_code, 400) - def test_reset_student_attempts_delete(self): + @patch('courseware.module_render.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()}) response = self.client.post(url, { diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 0b65f1c8a4..ca7b0f3aea 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -378,7 +378,8 @@ class TestInstructorEnrollmentStudentModule(SharedModuleStoreTestCase): reset_student_attempts(self.course_key, self.user, msk, requesting_user=self.user) self.assertEqual(json.loads(module().state)['attempts'], 0) - def test_delete_student_attempts(self): + @mock.patch('courseware.module_render.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'}) StudentModule.objects.create( @@ -404,7 +405,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') - def test_delete_submission_scores(self, _lti_mock): + def test_delete_submission_scores(self, _mock_signal): user = UserFactory() problem_location = self.course_key.make_usage_key('dummy', 'module') @@ -548,7 +549,7 @@ class TestStudentModuleGrading(SharedModuleStoreTestCase): self.course, get_course_blocks(self.user, self.course.location) ) - grade = subsection_grade_factory.update(self.sequence) + grade = subsection_grade_factory.create(self.sequence) self.assertEqual(grade.all_total.earned, all_earned) self.assertEqual(grade.graded_total.earned, graded_earned) self.assertEqual(grade.all_total.possible, all_possible) diff --git a/lms/djangoapps/instructor/tests/test_services.py b/lms/djangoapps/instructor/tests/test_services.py index 2cf59a663e..648f6350ee 100644 --- a/lms/djangoapps/instructor/tests/test_services.py +++ b/lms/djangoapps/instructor/tests/test_services.py @@ -49,7 +49,8 @@ class InstructorServiceTests(SharedModuleStoreTestCase): state=json.dumps({'attempts': 2}), ) - def test_reset_student_attempts_delete(self): + @mock.patch('courseware.module_render.SCORE_CHANGED.send') + def test_reset_student_attempts_delete(self, _mock_signal): """ Test delete student state. """ From ec6c924b7c0447b9119e5d4c36565c7858659328 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Mon, 3 Oct 2016 10:04:07 -0400 Subject: [PATCH 09/12] Fixed site keying for SAMLConfigurationsfor SAML management command --- .../djangoapps/third_party_auth/management/commands/saml.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index dd2c6c1049..8a1590ae66 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -4,7 +4,6 @@ Management commands for third_party_auth """ from django.core.management.base import BaseCommand, CommandError import logging -from third_party_auth.models import SAMLConfiguration from third_party_auth.tasks import fetch_saml_metadata @@ -16,9 +15,6 @@ class Command(BaseCommand): parser.add_argument('--pull', action='store_true', help="Pull updated metadata from external IDPs") def handle(self, *args, **options): - if not SAMLConfiguration.is_enabled(): - raise CommandError("SAML support is disabled via SAMLConfiguration.") - if options['pull']: log_handler = logging.StreamHandler(self.stdout) log_handler.setLevel(logging.DEBUG) From 62042188f544d6f4b57cbf079f42ad5e580ce6b3 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Tue, 4 Oct 2016 17:19:30 -0400 Subject: [PATCH 10/12] Revert "Fix Gating to use grades API, instead of its own calculations" This reverts commit 318121411a04e1fe4465b921cae06344ade1261f. --- .../transformers/tests/test_milestones.py | 43 ++-- lms/djangoapps/gating/api.py | 87 ++++--- lms/djangoapps/gating/signals.py | 17 +- lms/djangoapps/gating/tests/test_api.py | 240 ++++++------------ lms/djangoapps/gating/tests/test_signals.py | 24 +- lms/djangoapps/grades/module_grades.py | 96 +++++++ lms/djangoapps/grades/new/subsection_grade.py | 11 +- lms/djangoapps/grades/signals/handlers.py | 12 +- lms/djangoapps/grades/signals/signals.py | 14 +- lms/djangoapps/grades/tests/test_grades.py | 228 ++++++++++++++++- lms/djangoapps/grades/tests/test_signals.py | 2 +- lms/djangoapps/grades/tests/utils.py | 31 --- 12 files changed, 518 insertions(+), 287 deletions(-) create mode 100644 lms/djangoapps/grades/module_grades.py 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 a626d096dc..5f88c8004e 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py @@ -1,7 +1,7 @@ """ Tests for ProctoredExamTransformer. """ -from mock import patch +from mock import patch, Mock from nose.plugins.attrib import attr import ddt @@ -53,8 +53,7 @@ class MilestonesTransformerTestCase(CourseStructureTestCase, MilestonesTestCaseM 'course', 'A', 'B', 'C', 'ProctoredExam', 'D', 'E', 'PracticeExam', 'F', 'G', 'H', 'I', 'TimedExam', 'J', 'K' ) - # The special exams (proctored, practice, timed) are not visible to - # students via the Courses API. + # The special exams (proctored, practice, timed) should never be visible to students ALL_BLOCKS_EXCEPT_SPECIAL = ('course', 'A', 'B', 'C', 'H', 'I') def get_course_hierarchy(self): @@ -135,27 +134,27 @@ class MilestonesTransformerTestCase(CourseStructureTestCase, MilestonesTestCaseM ( 'H', 'A', - ('course', 'A', 'B', 'C'), + 'B', + ('course', 'A', 'B', 'C',) ), ( 'H', 'ProctoredExam', + 'D', ('course', 'A', 'B', 'C'), ), ) @ddt.unpack - def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_completion): + def test_gated(self, gated_block_ref, gating_block_ref, gating_block_child, expected_blocks_before_completion): """ - First, checks that a student cannot see the gated block when it is gated - by the gating block and no attempt has been made to complete the gating - block. Then, checks that the student can see the gated block after the - gating block has been completed. + First, checks that a student cannot see the gated block when it is gated by the gating block and no + attempt has been made to complete the gating block. + Then, checks that the student can see the gated block after the gating block has been completed. - expected_blocks_before_completion is the set of blocks we expect to be - visible to the student before the student has completed the gating block. + expected_blocks_before_completion is the set of blocks we expect to be visible to the student + before the student has completed the gating block. - The test data includes one special exam and one non-special block as the - gating blocks. + The test data includes one special exam and one non-special block as the gating blocks. """ self.course.enable_subsection_gating = True self.setup_gated_section(self.blocks[gated_block_ref], self.blocks[gating_block_ref]) @@ -166,16 +165,16 @@ class MilestonesTransformerTestCase(CourseStructureTestCase, MilestonesTestCaseM # clear the request cache to simulate a new request self.clear_caches() - # this call triggers reevaluation of prerequisites fulfilled by the - # gating block. - lms_gating_api.evaluate_prerequisite( - self.course, - self.user, - self.blocks[gating_block_ref].location, - 100.0, - ) + # mock the api that the lms gating api calls to get the score for each block to always return 1 (ie 100%) + with patch('gating.api.get_module_score', Mock(return_value=1)): - with self.assertNumQueries(3): + # this call triggers reevaluation of prerequisites fulfilled by the parent of the + # 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) + with self.assertNumQueries(2): self.get_blocks_and_check_against_expected(self.user, self.ALL_BLOCKS_EXCEPT_SPECIAL) def test_staff_access(self): diff --git a/lms/djangoapps/gating/api.py b/lms/djangoapps/gating/api.py index 64e35602cb..416d70d583 100644 --- a/lms/djangoapps/gating/api.py +++ b/lms/djangoapps/gating/api.py @@ -6,14 +6,35 @@ import json from collections import defaultdict from django.contrib.auth.models import User +from xmodule.modulestore.django import modulestore from openedx.core.lib.gating import api as gating_api +from lms.djangoapps.grades.module_grades import get_module_score from util import milestones_helpers log = logging.getLogger(__name__) +def _get_xblock_parent(xblock, category=None): + """ + Returns the parent of the given XBlock. If an optional category is supplied, + traverses the ancestors of the XBlock and returns the first with the + given category. + + Arguments: + xblock (XBlock): Get the parent of this XBlock + category (str): Find an ancestor with this category (e.g. sequential) + """ + parent = xblock.get_parent() + if parent and category: + if parent.category == category: + return parent + else: + return _get_xblock_parent(parent, category) + return parent + + @gating_api.gating_enabled(default=False) -def evaluate_prerequisite(course, user, subsection_usage_key, new_score): +def evaluate_prerequisite(course, prereq_content_key, user_id): """ Finds the parent subsection of the content in the course and evaluates any milestone relationships attached to that subsection. If the calculated @@ -21,40 +42,44 @@ def evaluate_prerequisite(course, user, subsection_usage_key, new_score): dependent subsections, the related milestone will be fulfilled for the user. Arguments: - user (User): User for which evaluation should occur + user_id (int): ID of User for which evaluation should occur course (CourseModule): The course - subsection_usage_key (UsageKey): Usage key of the updated subsection - new_score (float): New score of the given subsection, in percentage. + prereq_content_key (UsageKey): The prerequisite content usage key Returns: None """ - prereq_milestone = gating_api.get_gating_milestone( - course.id, - subsection_usage_key, - 'fulfills' - ) - if prereq_milestone: - gated_content_milestones = defaultdict(list) - for milestone in gating_api.find_gating_milestones(course.id, None, 'requires'): - gated_content_milestones[milestone['id']].append(milestone) + xblock = modulestore().get_item(prereq_content_key) + sequential = _get_xblock_parent(xblock, 'sequential') + if sequential: + prereq_milestone = gating_api.get_gating_milestone( + course.id, + sequential.location.for_branch(None), + 'fulfills' + ) + if prereq_milestone: + gated_content_milestones = defaultdict(list) + for milestone in gating_api.find_gating_milestones(course.id, None, 'requires'): + gated_content_milestones[milestone['id']].append(milestone) - gated_content = gated_content_milestones.get(prereq_milestone['id']) - if gated_content: - for milestone in gated_content: - # Default minimum score to 100 - min_score = 100.0 - requirements = milestone.get('requirements') - if requirements: - try: - min_score = float(requirements.get('min_score')) - except (ValueError, TypeError): - log.warning( - 'Failed to find minimum score for gating milestone %s, defaulting to 100', - json.dumps(milestone) - ) + gated_content = gated_content_milestones.get(prereq_milestone['id']) + if gated_content: + user = User.objects.get(id=user_id) + score = get_module_score(user, course, sequential) * 100 + for milestone in gated_content: + # Default minimum score to 100 + min_score = 100 + requirements = milestone.get('requirements') + if requirements: + try: + min_score = int(requirements.get('min_score')) + except (ValueError, TypeError): + log.warning( + 'Failed to find minimum score for gating milestone %s, defaulting to 100', + json.dumps(milestone) + ) - if new_score >= min_score: - milestones_helpers.add_user_milestone({'id': user.id}, prereq_milestone) - else: - milestones_helpers.remove_user_milestone({'id': user.id}, prereq_milestone) + if score >= min_score: + milestones_helpers.add_user_milestone({'id': user_id}, prereq_milestone) + else: + milestones_helpers.remove_user_milestone({'id': user_id}, prereq_milestone) diff --git a/lms/djangoapps/gating/signals.py b/lms/djangoapps/gating/signals.py index 06c79ad90c..bfa5b974a7 100644 --- a/lms/djangoapps/gating/signals.py +++ b/lms/djangoapps/gating/signals.py @@ -2,12 +2,14 @@ Signal handlers for the gating djangoapp """ from django.dispatch import receiver -from lms.djangoapps.grades.signals.signals import SUBSECTION_SCORE_UPDATED +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(SUBSECTION_SCORE_UPDATED) -def handle_subsection_score_updated(**kwargs): +@receiver(SCORE_CHANGED) +def handle_score_changed(**kwargs): """ Receives the SCORE_CHANGED signal sent by LMS when a student's score has changed for a given component and triggers the evaluation of any milestone relationships @@ -19,13 +21,10 @@ def handle_subsection_score_updated(**kwargs): Returns: None """ - course = kwargs['course'] + course = modulestore().get_course(CourseKey.from_string(kwargs.get('course_id'))) if course.enable_subsection_gating: - subsection_grade = kwargs['subsection_grade'] - new_score = subsection_grade.graded_total.earned / subsection_grade.graded_total.possible * 100.0 gating_api.evaluate_prerequisite( course, - kwargs['user'], - subsection_grade.location, - new_score, + UsageKey.from_string(kwargs.get('usage_id')), + kwargs.get('user').id, ) diff --git a/lms/djangoapps/gating/tests/test_api.py b/lms/djangoapps/gating/tests/test_api.py index 5e63cf9ef3..65d677a740 100644 --- a/lms/djangoapps/gating/tests/test_api.py +++ b/lms/djangoapps/gating/tests/test_api.py @@ -6,18 +6,15 @@ from nose.plugins.attrib import attr from ddt import ddt, data, unpack from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from courseware.tests.helpers import LoginEnrollmentTestCase -from lms.djangoapps.courseware.access import has_access -from lms.djangoapps.courseware.tests.helpers import get_request_for_user -from lms.djangoapps.grades.tests.utils import answer_problem -from lms.djangoapps.grades.new.course_grade import CourseGradeFactory from milestones import api as milestones_api from milestones.tests.utils import MilestonesTestCaseMixin from openedx.core.lib.gating import api as gating_api -from request_cache.middleware import RequestCache +from gating.api import _get_xblock_parent, evaluate_prerequisite -class GatingTestCase(ModuleStoreTestCase): +class GatingTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): """ Base TestCase class for setting up a basic course structure and testing the gating feature @@ -37,16 +34,6 @@ class GatingTestCase(ModuleStoreTestCase): display_name='edX 101' ) self.course.enable_subsection_gating = True - grading_policy = { - "GRADER": [{ - "type": "Homework", - "min_count": 3, - "drop_count": 0, - "short_label": "HW", - "weight": 1.0 - }] - } - self.course.grading_policy = grading_policy self.course.save() self.store.update_item(self.course, 0) @@ -54,197 +41,136 @@ class GatingTestCase(ModuleStoreTestCase): self.chapter1 = ItemFactory.create( parent_location=self.course.location, category='chapter', - display_name='chapter 1' + display_name='untitled chapter 1' ) # create sequentials self.seq1 = ItemFactory.create( parent_location=self.chapter1.location, category='sequential', - display_name='gating sequential 1', - graded=True, - format='Homework', + display_name='untitled sequential 1' ) self.seq2 = ItemFactory.create( parent_location=self.chapter1.location, category='sequential', - display_name='gated sequential 2', - graded=True, - format='Homework', + display_name='untitled sequential 2' ) - self.seq3 = ItemFactory.create( - parent_location=self.chapter1.location, - category='sequential', - display_name='sequential 3', - graded=True, - format='Homework', + + # create vertical + self.vert1 = ItemFactory.create( + parent_location=self.seq1.location, + category='vertical', + display_name='untitled vertical 1' ) # create problem - self.gating_prob1 = ItemFactory.create( - parent_location=self.seq1.location, + self.prob1 = ItemFactory.create( + parent_location=self.vert1.location, category='problem', - display_name='gating problem 1', - ) - self.gated_prob2 = ItemFactory.create( - parent_location=self.seq2.location, - category='problem', - display_name='gated problem 2', - ) - self.prob3 = ItemFactory.create( - parent_location=self.seq3.location, - category='problem', - display_name='problem 3', + display_name='untitled problem 1' ) # create orphan - self.orphan = ItemFactory.create( + self.prob2 = ItemFactory.create( parent_location=self.course.location, category='problem', - display_name='orphan' - ) - - self.prereq_milestone = None - - def setup_gating_milestone(self, min_score): - """ - Setup a gating milestone for testing. - Gating content: seq1 (must be fulfilled before access to seq2) - Gated content: seq2 (requires completion of seq1 before access) - """ - gating_api.add_prerequisite(self.course.id, self.seq1.location) - gating_api.set_required_content(self.course.id, self.seq2.location, self.seq1.location, min_score) - self.prereq_milestone = gating_api.get_gating_milestone(self.course.id, self.seq1.location, 'fulfills') - - def verify_access_to_gated_content(self, user, expected_access): - """ - Verifies access to gated content for the given user is as expected. - """ - # clear the request cache to flush any cached access results - RequestCache.clear_request_cache() - - # access to gating content (seq1) remains constant - self.assertTrue(bool(has_access(user, 'load', self.seq1, self.course.id))) - - # access to gated content (seq2) is as expected - self.assertEquals(bool(has_access(user, 'load', self.seq2, self.course.id)), expected_access) - - def verify_user_has_prereq_milestone(self, user, expected_has_milestone): - """ - Verifies whether or not the user has the prereq milestone - """ - self.assertEquals( - milestones_api.user_has_milestone({'id': user.id}, self.prereq_milestone), - expected_has_milestone, + display_name='untitled problem 2' ) -@attr(shard=3) -class TestGatedContent(GatingTestCase, MilestonesTestCaseMixin): +class TestGetXBlockParent(GatingTestCase): """ - Tests for gated content. + Tests for the get_xblock_parent function """ - def setUp(self): - super(TestGatedContent, self).setUp() - self.setup_gating_milestone(100) - self.non_staff_user, _ = self.create_non_staff_user() - def test_gated_for_nonstaff(self): - self.verify_user_has_prereq_milestone(self.non_staff_user, expected_has_milestone=False) - self.verify_access_to_gated_content(self.non_staff_user, expected_access=False) + def test_get_direct_parent(self): + """ Test test_get_direct_parent """ - def test_not_gated_for_staff(self): - self.verify_user_has_prereq_milestone(self.user, expected_has_milestone=False) - self.verify_access_to_gated_content(self.user, expected_access=True) + result = _get_xblock_parent(self.vert1) + self.assertEqual(result.location, self.seq1.location) - def _verify_course_grade(self, user, expected_percent): - """ - Verifies the given user's course grade is the expected percentage. - Also verifies the user's grade information contains values for - all problems in the course, whether or not they are currently - gated. - """ - course_grade = CourseGradeFactory(user).create(self.course) - for prob in [self.gating_prob1, self.gated_prob2, self.prob3]: - self.assertIn(prob.location, course_grade.locations_to_scores) - self.assertNotIn(self.orphan.location, course_grade.locations_to_scores) + def test_get_parent_with_category(self): + """ Test test_get_parent_of_category """ - self.assertEquals(course_grade.percent, expected_percent) + result = _get_xblock_parent(self.vert1, 'sequential') + self.assertEqual(result.location, self.seq1.location) + result = _get_xblock_parent(self.vert1, 'chapter') + self.assertEqual(result.location, self.chapter1.location) - def test_gated_content_always_in_grades(self): - request = get_request_for_user(self.non_staff_user) + def test_get_parent_none(self): + """ Test test_get_parent_none """ - # start with a grade from a non-gated subsection - answer_problem(self.course, request, self.prob3, 10, 10) - - # verify gated status and overall course grade percentage - self.verify_user_has_prereq_milestone(self.non_staff_user, expected_has_milestone=False) - self.verify_access_to_gated_content(self.non_staff_user, expected_access=False) - self._verify_course_grade(self.non_staff_user, .33) - - # fulfill the gated requirements - answer_problem(self.course, request, self.gating_prob1, 10, 10) - - # verify gated status and overall course grade percentage - self.verify_user_has_prereq_milestone(self.non_staff_user, expected_has_milestone=True) - self.verify_access_to_gated_content(self.non_staff_user, expected_access=True) - self._verify_course_grade(self.non_staff_user, .67) + result = _get_xblock_parent(self.vert1, 'unit') + self.assertIsNone(result) @attr(shard=3) @ddt -class TestHandleSubsectionGradeUpdates(GatingTestCase, MilestonesTestCaseMixin): +class TestEvaluatePrerequisite(GatingTestCase, MilestonesTestCaseMixin): """ - Tests for gated content when subsection grade is updated. + Tests for the evaluate_prerequisite function """ def setUp(self): - super(TestHandleSubsectionGradeUpdates, self).setUp() - self.user, _ = self.create_non_staff_user() # run tests for a non-staff user - self.request = get_request_for_user(self.user) + super(TestEvaluatePrerequisite, self).setUp() + self.user_dict = {'id': self.user.id} + self.prereq_milestone = None - def test_signal_handler_called(self): - with patch('lms.djangoapps.gating.signals.gating_api.evaluate_prerequisite') as mock_handler: - self.assertFalse(mock_handler.called) - answer_problem(self.course, self.request, self.gating_prob1, 1, 1) - self.assertTrue(mock_handler.called) + def _setup_gating_milestone(self, min_score): + """ + Setup a gating milestone for testing + """ - @data((1, 2, True), (1, 1, True), (0, 1, False)) + gating_api.add_prerequisite(self.course.id, self.seq1.location) + gating_api.set_required_content(self.course.id, self.seq2.location, self.seq1.location, min_score) + self.prereq_milestone = gating_api.get_gating_milestone(self.course.id, self.seq1.location, 'fulfills') + + @patch('gating.api.get_module_score') + @data((.5, True), (1, True), (0, False)) @unpack - def test_min_score_achieved(self, earned, max_possible, result): - self.setup_gating_milestone(50) + def test_min_score_achieved(self, module_score, result, mock_module_score): + """ Test test_min_score_achieved """ - self.verify_user_has_prereq_milestone(self.user, expected_has_milestone=False) - self.verify_access_to_gated_content(self.user, expected_access=False) + self._setup_gating_milestone(50) - answer_problem(self.course, self.request, self.gating_prob1, earned, max_possible) - - self.verify_user_has_prereq_milestone(self.user, expected_has_milestone=result) - self.verify_access_to_gated_content(self.user, expected_access=result) + mock_module_score.return_value = module_score + evaluate_prerequisite(self.course, self.prob1.location, self.user.id) + self.assertEqual(milestones_api.user_has_milestone(self.user_dict, self.prereq_milestone), result) @patch('gating.api.log.warning') - @data((1, 2, False), (1, 1, True)) + @patch('gating.api.get_module_score') + @data((.5, False), (1, True)) @unpack - def test_invalid_min_score(self, earned, max_possible, result, mock_log): - self.setup_gating_milestone(None) + def test_invalid_min_score(self, module_score, result, mock_module_score, mock_log): + """ Test test_invalid_min_score """ - answer_problem(self.course, self.request, self.gating_prob1, earned, max_possible) - self.verify_user_has_prereq_milestone(self.user, expected_has_milestone=result) + self._setup_gating_milestone(None) + + mock_module_score.return_value = module_score + evaluate_prerequisite(self.course, self.prob1.location, self.user.id) + self.assertEqual(milestones_api.user_has_milestone(self.user_dict, self.prereq_milestone), result) self.assertTrue(mock_log.called) - def test_orphaned_xblock(self): - with patch('lms.djangoapps.gating.signals.gating_api.evaluate_prerequisite') as mock_handler: - self.assertFalse(mock_handler.called) - answer_problem(self.course, self.request, self.orphan, 1, 1) - self.assertFalse(mock_handler.called) + @patch('gating.api.get_module_score') + def test_orphaned_xblock(self, mock_module_score): + """ Test test_orphaned_xblock """ - @patch('gating.api.milestones_helpers') - def test_no_prerequisites(self, mock_milestones): - answer_problem(self.course, self.request, self.gating_prob1, 1, 1) - self.assertFalse(mock_milestones.called) + evaluate_prerequisite(self.course, self.prob2.location, self.user.id) + self.assertFalse(mock_module_score.called) - @patch('gating.api.milestones_helpers') - def test_no_gated_content(self, mock_milestones): + @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) + self.assertFalse(mock_module_score.called) + + @patch('gating.api.get_module_score') + def test_no_gated_content(self, mock_module_score): + """ Test test_no_gated_content """ + + # Setup gating milestones data gating_api.add_prerequisite(self.course.id, self.seq1.location) - answer_problem(self.course, self.request, self.gating_prob1, 1, 1) - self.assertFalse(mock_milestones.called) + + evaluate_prerequisite(self.course, self.prob1.location, 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 f910529e77..16116da608 100644 --- a/lms/djangoapps/gating/tests/test_signals.py +++ b/lms/djangoapps/gating/tests/test_signals.py @@ -1,7 +1,7 @@ """ Unit tests for gating.signals module """ -from mock import patch, MagicMock +from mock import patch from opaque_keys.edx.keys import UsageKey from student.tests.factories import UserFactory @@ -9,7 +9,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.django import modulestore -from gating.signals import handle_subsection_score_updated +from gating.signals import handle_score_changed class TestHandleScoreChanged(ModuleStoreTestCase): @@ -24,22 +24,28 @@ class TestHandleScoreChanged(ModuleStoreTestCase): @patch('gating.signals.gating_api.evaluate_prerequisite') def test_gating_enabled(self, mock_evaluate): + """ Test evaluate_prerequisite is called when course.enable_subsection_gating is True """ self.course.enable_subsection_gating = True modulestore().update_item(self.course, 0) - handle_subsection_score_updated( + handle_score_changed( sender=None, - course=self.course, + points_possible=1, + points_earned=1, user=self.user, - subsection_grade=MagicMock(), + course_id=unicode(self.course.id), + usage_id=unicode(self.test_usage_key) ) - mock_evaluate.assert_called() + mock_evaluate.assert_called_with(self.course, self.test_usage_key, self.user.id) # pylint: disable=no-member @patch('gating.signals.gating_api.evaluate_prerequisite') def test_gating_disabled(self, mock_evaluate): - handle_subsection_score_updated( + """ Test evaluate_prerequisite is not called when course.enable_subsection_gating is False """ + handle_score_changed( sender=None, - course=self.course, + points_possible=1, + points_earned=1, user=self.user, - subsection_grade=MagicMock(), + course_id=unicode(self.course.id), + usage_id=unicode(self.test_usage_key) ) mock_evaluate.assert_not_called() diff --git a/lms/djangoapps/grades/module_grades.py b/lms/djangoapps/grades/module_grades.py new file mode 100644 index 0000000000..a0e95a57e9 --- /dev/null +++ b/lms/djangoapps/grades/module_grades.py @@ -0,0 +1,96 @@ +""" +Functionality for module-level grades. +""" +# TODO The score computation in this file is not accurate +# since it is summing percentages instead of computing a +# final percentage of the individual sums. +# Regardless, this file and its code should be removed soon +# as part of TNL-5062. + +from django.test.client import RequestFactory +from courseware.model_data import FieldDataCache, ScoresClient +from courseware.module_render import get_module_for_descriptor +from opaque_keys.edx.locator import BlockUsageLocator +from util.module_utils import yield_dynamic_descriptor_descendants + + +def _get_mock_request(student): + """ + Make a fake request because grading code expects to be able to look at + the request. We have to attach the correct user to the request before + grading that student. + """ + request = RequestFactory().get('/') + request.user = student + return request + + +def _calculate_score_for_modules(user_id, course, modules): + """ + Calculates the cumulative score (percent) of the given modules + """ + + # removing branch and version from exam modules locator + # otherwise student module would not return scores since module usage keys would not match + modules = [m for m in modules] + locations = [ + BlockUsageLocator( + course_key=course.id, + block_type=module.location.block_type, + block_id=module.location.block_id + ) + if isinstance(module.location, BlockUsageLocator) and module.location.version + else module.location + for module in modules + ] + + scores_client = ScoresClient(course.id, user_id) + scores_client.fetch_scores(locations) + + # Iterate over all of the exam modules to get score percentage of user for each of them + module_percentages = [] + ignore_categories = ['course', 'chapter', 'sequential', 'vertical', 'randomize', 'library_content'] + for index, module in enumerate(modules): + if module.category not in ignore_categories and (module.graded or module.has_score): + module_score = scores_client.get(locations[index]) + if module_score: + correct = module_score.correct or 0 + total = module_score.total or 1 + module_percentages.append(correct / total) + + return sum(module_percentages) / float(len(module_percentages)) if module_percentages else 0 + + +def get_module_score(user, course, module): + """ + Collects all children of the given module and calculates the cumulative + score for this set of modules for the given user. + + Arguments: + user (User): The user + course (CourseModule): The course + module (XBlock): The module + + Returns: + float: The cumulative score + """ + def inner_get_module(descriptor): + """ + Delegate to get_module_for_descriptor + """ + field_data_cache = FieldDataCache([descriptor], course.id, user) + return get_module_for_descriptor( + user, + _get_mock_request(user), + descriptor, + field_data_cache, + course.id, + course=course + ) + + modules = yield_dynamic_descriptor_descendants( + module, + user.id, + inner_get_module + ) + return _calculate_score_for_modules(user.id, course, modules) diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 1d196c4333..74a3f90f99 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -251,6 +251,11 @@ class SubsectionGradeFactory(object): """ Updates the SubsectionGrade object for the student and subsection. """ + # Save ourselves the extra queries if the course does not persist + # subsection grades. + if not PersistentGradesEnabledFlag.feature_enabled(self.course.id): + return + self._log_event(log.warning, u"update, subsection: {}".format(subsection.location)) block_structure = self._get_block_structure(block_structure) @@ -259,10 +264,8 @@ class SubsectionGradeFactory(object): self.student, block_structure, self._submissions_scores, self._csm_scores ) - if PersistentGradesEnabledFlag.feature_enabled(self.course.id): - grade_model = subsection_grade.update_or_create_model(self.student) - self._update_saved_subsection_grade(subsection.location, grade_model) - + grade_model = subsection_grade.update_or_create_model(self.student) + self._update_saved_subsection_grade(subsection.location, grade_model) return subsection_grade @lazy diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 56dd9017ad..b13fefd7c9 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -13,7 +13,7 @@ from openedx.core.djangoapps.content.block_structure.api import get_course_in_ca from student.models import user_by_anonymous_id from submissions.models import score_set, score_reset -from .signals import SCORE_CHANGED, SUBSECTION_SCORE_UPDATED +from .signals import SCORE_CHANGED from ..config.models import PersistentGradesEnabledFlag from ..transformer import GradesTransformer from ..new.subsection_grade import SubsectionGradeFactory @@ -95,6 +95,8 @@ def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=u """ student = kwargs['user'] course_key = CourseLocator.from_string(kwargs['course_id']) + if not PersistentGradesEnabledFlag.feature_enabled(course_key): + return scored_block_usage_key = UsageKey.from_string(kwargs['usage_id']).replace(course_key=course_key) collected_block_structure = get_course_in_cache(course_key) @@ -113,12 +115,6 @@ def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=u subsection_usage_key, collected_block_structure=collected_block_structure, ) - subsection_grade = subsection_grade_factory.update( + subsection_grade_factory.update( transformed_subsection_structure[subsection_usage_key], transformed_subsection_structure ) - SUBSECTION_SCORE_UPDATED.send( - sender=None, - course=course, - user=student, - subsection_grade=subsection_grade, - ) diff --git a/lms/djangoapps/grades/signals/signals.py b/lms/djangoapps/grades/signals/signals.py index b3a3b67f8e..250b27e9ab 100644 --- a/lms/djangoapps/grades/signals/signals.py +++ b/lms/djangoapps/grades/signals/signals.py @@ -14,20 +14,8 @@ SCORE_CHANGED = Signal( providing_args=[ 'points_possible', # Maximum score available for the exercise 'points_earned', # Score obtained by the user - 'user', # User object + 'user_id', # Integer User ID 'course_id', # Unicode string representing the course 'usage_id' # Unicode string indicating the courseware instance ] ) - - -# Signal that indicates that a user's score for a subsection has been updated. -# This is a downstream signal of SCORE_CHANGED sent for each affected containing -# subsection. -SUBSECTION_SCORE_UPDATED = Signal( - providing_args=[ - 'course', # Course object - 'user', # User object - 'subsection_grade', # SubsectionGrade object - ] -) diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index 038fff9147..91212a8706 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -10,7 +10,12 @@ from nose.plugins.attrib import attr from opaque_keys.edx.locations import SlashSeparatedCourseKey from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory -from courseware.tests.helpers import get_request_for_user +from courseware.module_render import get_module +from courseware.model_data import FieldDataCache, set_score +from courseware.tests.helpers import ( + LoginEnrollmentTestCase, + get_request_for_user +) from lms.djangoapps.course_blocks.api import get_course_blocks from student.tests.factories import UserFactory from student.models import CourseEnrollment @@ -20,9 +25,9 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from .utils import answer_problem from .. import course_grades from ..course_grades import summary as grades_summary +from ..module_grades import get_module_score from ..new.course_grade import CourseGradeFactory from ..new.subsection_grade import SubsectionGradeFactory @@ -337,3 +342,222 @@ class TestScoreForModule(SharedModuleStoreTestCase): earned, possible = self.course_grade.score_for_module(self.m.location) self.assertEqual(earned, 0) self.assertEqual(possible, 0) + + +class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase): + """ + Test get_module_score + """ + @classmethod + def setUpClass(cls): + super(TestGetModuleScore, cls).setUpClass() + cls.course = CourseFactory.create() + cls.chapter = ItemFactory.create( + parent=cls.course, + category="chapter", + display_name="Test Chapter" + ) + cls.seq1 = ItemFactory.create( + parent=cls.chapter, + category='sequential', + display_name="Test Sequential 1", + graded=True + ) + cls.seq2 = ItemFactory.create( + parent=cls.chapter, + category='sequential', + display_name="Test Sequential 2", + graded=True + ) + cls.seq3 = ItemFactory.create( + parent=cls.chapter, + category='sequential', + display_name="Test Sequential 3", + graded=True + ) + cls.vert1 = ItemFactory.create( + parent=cls.seq1, + category='vertical', + display_name='Test Vertical 1' + ) + cls.vert2 = ItemFactory.create( + parent=cls.seq2, + category='vertical', + display_name='Test Vertical 2' + ) + cls.vert3 = ItemFactory.create( + parent=cls.seq3, + category='vertical', + display_name='Test Vertical 3' + ) + cls.randomize = ItemFactory.create( + parent=cls.vert2, + category='randomize', + display_name='Test Randomize' + ) + cls.library_content = ItemFactory.create( + parent=cls.vert3, + category='library_content', + display_name='Test Library Content' + ) + problem_xml = MultipleChoiceResponseXMLFactory().build_xml( + question_text='The correct answer is Choice 3', + choices=[False, False, True, False], + choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] + ) + cls.problem1 = ItemFactory.create( + parent=cls.vert1, + category="problem", + display_name="Test Problem 1", + data=problem_xml + ) + cls.problem2 = ItemFactory.create( + parent=cls.vert1, + category="problem", + display_name="Test Problem 2", + data=problem_xml + ) + cls.problem3 = ItemFactory.create( + parent=cls.randomize, + category="problem", + display_name="Test Problem 3", + data=problem_xml + ) + cls.problem4 = ItemFactory.create( + parent=cls.randomize, + category="problem", + display_name="Test Problem 4", + data=problem_xml + ) + + cls.problem5 = ItemFactory.create( + parent=cls.library_content, + category="problem", + display_name="Test Problem 5", + data=problem_xml + ) + cls.problem6 = ItemFactory.create( + parent=cls.library_content, + category="problem", + display_name="Test Problem 6", + data=problem_xml + ) + + def setUp(self): + """ + Set up test course + """ + super(TestGetModuleScore, self).setUp() + + self.request = get_request_for_user(UserFactory()) + 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_subsection_scores(self): + """ + Test test_get_module_score + """ + # One query is for getting the list of disabled XBlocks (which is + # 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, self.course, self.course_structure).create(self.seq1) + 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, self.course, self.course_structure).create(self.seq1) + 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, self.course, self.course_structure).create(self.seq1) + self.assertEqual(score, .5) + self.assertEqual(new_score.all_total.earned, 1.0) + + def test_get_module_score_with_empty_score(self): + """ + Test test_get_module_score_with_empty_score + """ + set_score(self.request.user.id, self.problem1.location, None, None) # pylint: disable=no-member + set_score(self.request.user.id, self.problem2.location, None, None) # pylint: disable=no-member + + with self.assertNumQueries(1): + score = get_module_score(self.request.user, self.course, self.seq1) + self.assertEqual(score, 0) + + answer_problem(self.course, self.request, self.problem1) + + with self.assertNumQueries(1): + score = get_module_score(self.request.user, self.course, self.seq1) + self.assertEqual(score, 0.5) + + answer_problem(self.course, self.request, self.problem2) + + with self.assertNumQueries(1): + score = get_module_score(self.request.user, self.course, self.seq1) + self.assertEqual(score, 1.0) + + def test_get_module_score_with_randomize(self): + """ + Test test_get_module_score_with_randomize + """ + answer_problem(self.course, self.request, self.problem3) + answer_problem(self.course, self.request, self.problem4) + + score = get_module_score(self.request.user, self.course, self.seq2) + self.assertEqual(score, 1.0) + + def test_get_module_score_with_library_content(self): + """ + Test test_get_module_score_with_library_content + """ + answer_problem(self.course, self.request, self.problem5) + answer_problem(self.course, self.request, self.problem6) + + score = get_module_score(self.request.user, self.course, self.seq3) + self.assertEqual(score, 1.0) + + +def answer_problem(course, request, problem, score=1, max_value=1): + """ + Records a correct answer for the given problem. + + Arguments: + course (Course): Course object, the course the required problem is in + request (Request): request Object + problem (xblock): xblock object, the problem to be answered + """ + + user = request.user + grade_dict = {'value': score, 'max_value': max_value, 'user_id': user.id} + field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + course.id, + user, + course, + depth=2 + ) + # pylint: disable=protected-access + module = get_module( + user, + request, + problem.scope_ids.usage_id, + field_data_cache, + )._xmodule + module.system.publish(problem, 'grade', grade_dict) diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 51705a0460..2054f967f5 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -244,7 +244,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): with self.store.default_store(default_store): self.set_up_course(enable_subsection_grades=False) self.assertFalse(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) - with check_mongo_calls(2) and self.assertNumQueries(3): + with check_mongo_calls(2) and self.assertNumQueries(0): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @skip("Pending completion of TNL-5089") diff --git a/lms/djangoapps/grades/tests/utils.py b/lms/djangoapps/grades/tests/utils.py index 951e800329..94fc555ccd 100644 --- a/lms/djangoapps/grades/tests/utils.py +++ b/lms/djangoapps/grades/tests/utils.py @@ -3,8 +3,6 @@ Utilities for grades related tests """ from contextlib import contextmanager from mock import patch -from courseware.module_render import get_module -from courseware.model_data import FieldDataCache from xmodule.graders import ProblemScore @@ -26,32 +24,3 @@ def mock_get_score(earned=0, possible=1): with patch('lms.djangoapps.grades.new.subsection_grade.get_score') as mock_score: mock_score.return_value = ProblemScore(earned, possible, earned, possible, 1, True, None, None) yield mock_score - - -def answer_problem(course, request, problem, score=1, max_value=1): - """ - Records an answer for the given problem. - - Arguments: - course (Course): Course object, the course the required problem is in - request (Request): request Object - problem (xblock): xblock object, the problem to be answered - score (float): The new score for the problem - max_value (float): The new maximum score for the problem - """ - - user = request.user - grade_dict = {'value': score, 'max_value': max_value, 'user_id': user.id} - field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - course.id, - user, - course, - depth=2 - ) - module = get_module( - user, - request, - problem.location, - field_data_cache, - ) - module.runtime.publish(problem, 'grade', grade_dict) From 99ce254b15088b987cb6bd6d729fe59a9c4da4f9 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 4 Oct 2016 18:30:56 -0400 Subject: [PATCH 11/12] Fix import required by downstream commit. --- lms/djangoapps/grades/tests/test_grades.py | 32 ++-------------------- lms/djangoapps/grades/tests/utils.py | 30 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index 91212a8706..655bd6f7ca 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/lms/djangoapps/grades/tests/test_grades.py @@ -10,8 +10,7 @@ from nose.plugins.attrib import attr from opaque_keys.edx.locations import SlashSeparatedCourseKey from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory -from courseware.module_render import get_module -from courseware.model_data import FieldDataCache, set_score +from courseware.model_data import set_score from courseware.tests.helpers import ( LoginEnrollmentTestCase, get_request_for_user @@ -25,6 +24,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from .utils import answer_problem from .. import course_grades from ..course_grades import summary as grades_summary from ..module_grades import get_module_score @@ -533,31 +533,3 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase): score = get_module_score(self.request.user, self.course, self.seq3) self.assertEqual(score, 1.0) - - -def answer_problem(course, request, problem, score=1, max_value=1): - """ - Records a correct answer for the given problem. - - Arguments: - course (Course): Course object, the course the required problem is in - request (Request): request Object - problem (xblock): xblock object, the problem to be answered - """ - - user = request.user - grade_dict = {'value': score, 'max_value': max_value, 'user_id': user.id} - field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - course.id, - user, - course, - depth=2 - ) - # pylint: disable=protected-access - module = get_module( - user, - request, - problem.scope_ids.usage_id, - field_data_cache, - )._xmodule - module.system.publish(problem, 'grade', grade_dict) diff --git a/lms/djangoapps/grades/tests/utils.py b/lms/djangoapps/grades/tests/utils.py index 94fc555ccd..0d640100bd 100644 --- a/lms/djangoapps/grades/tests/utils.py +++ b/lms/djangoapps/grades/tests/utils.py @@ -3,6 +3,8 @@ Utilities for grades related tests """ from contextlib import contextmanager from mock import patch +from courseware.module_render import get_module +from courseware.model_data import FieldDataCache from xmodule.graders import ProblemScore @@ -24,3 +26,31 @@ def mock_get_score(earned=0, possible=1): with patch('lms.djangoapps.grades.new.subsection_grade.get_score') as mock_score: mock_score.return_value = ProblemScore(earned, possible, earned, possible, 1, True, None, None) yield mock_score + + +def answer_problem(course, request, problem, score=1, max_value=1): + """ + Records a correct answer for the given problem. + + Arguments: + course (Course): Course object, the course the required problem is in + request (Request): request Object + problem (xblock): xblock object, the problem to be answered + """ + + user = request.user + grade_dict = {'value': score, 'max_value': max_value, 'user_id': user.id} + field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + course.id, + user, + course, + depth=2 + ) + # pylint: disable=protected-access + module = get_module( + user, + request, + problem.scope_ids.usage_id, + field_data_cache, + )._xmodule + module.system.publish(problem, 'grade', grade_dict) From 7be2093edb2a860027429044dd82b9a12434bcd7 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Wed, 5 Oct 2016 16:48:42 -0400 Subject: [PATCH 12/12] Break early if max_score is not defined Some XModules are breaking their contracts and defining has_score but not max_score. TNL-5715 --- lms/djangoapps/instructor/enrollment.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index fd3921431b..69acebff36 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -317,7 +317,11 @@ def _fire_score_changed_for_block(course_id, student, block, module_state_key): field_data_cache=cache, course_key=course_id ) - points_earned, points_possible = weighted_score(0, module.max_score(), getattr(module, 'weight', None)) + max_score = module.max_score() + if max_score is None: + return + else: + points_earned, points_possible = weighted_score(0, max_score, getattr(module, 'weight', None)) else: points_earned, points_possible = 0, 0 SCORE_CHANGED.send(