From d852aa8b756af7250e5906da86546cef6814a64c Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 11 Apr 2017 17:51:25 -0400 Subject: [PATCH] Missing grades tests and Don't persist when creating grades unless policy changed. --- lms/djangoapps/grades/new/course_data.py | 18 +- lms/djangoapps/grades/new/course_grade.py | 12 + .../grades/new/course_grade_factory.py | 4 +- .../grades/tests/test_course_data.py | 36 +- lms/djangoapps/grades/tests/test_new.py | 364 ++++++++++-------- 5 files changed, 257 insertions(+), 177 deletions(-) diff --git a/lms/djangoapps/grades/new/course_data.py b/lms/djangoapps/grades/new/course_data.py index b2d59ac069..37b6b6db7b 100644 --- a/lms/djangoapps/grades/new/course_data.py +++ b/lms/djangoapps/grades/new/course_data.py @@ -30,14 +30,14 @@ class CourseData(object): if self._course: self._course_key = self._course.id else: - structure = self.effective_structure + structure = self._effective_structure self._course_key = structure.root_block_usage_key.course_key return self._course_key @property def location(self): if not self._location: - structure = self.effective_structure + structure = self._effective_structure if structure: self._location = structure.root_block_usage_key elif self._course: @@ -64,7 +64,7 @@ class CourseData(object): @property def grading_policy_hash(self): - structure = self.effective_structure + structure = self._effective_structure if structure: return structure.get_transformer_block_field( structure.root_block_usage_key, @@ -76,7 +76,7 @@ class CourseData(object): @property def version(self): - structure = self.effective_structure + structure = self._effective_structure course_block = structure[self.location] if structure else self.course return getattr(course_block, 'course_version', None) @@ -86,17 +86,17 @@ class CourseData(object): course_block = self.structure[self.location] return getattr(course_block, 'subtree_edited_on', None) - @property - def effective_structure(self): - return self._structure or self._collected_block_structure - def __unicode__(self): return u'Course: course_key: {}'.format(self.course_key) def full_string(self): - if self.effective_structure: + if self._effective_structure: return u'Course: course_key: {}, version: {}, edited_on: {}, grading_policy: {}'.format( self.course_key, self.version, self.edited_on, self.grading_policy_hash, ) else: return u'Course: course_key: {}, empty course structure'.format(self.course_key) + + @property + def _effective_structure(self): + return self._structure or self._collected_block_structure diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index d358875e45..0f65040c31 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/lms/djangoapps/grades/new/course_grade.py @@ -64,6 +64,18 @@ class CourseGradeBase(object): for chapter_key in course_structure.get_children(self.course_data.location) } + @lazy + def subsection_grades(self): + """ + Returns an ordered dictionary of subsection grades, + keyed by subsection location. + """ + subsection_grades = defaultdict(OrderedDict) + for chapter in self.chapter_grades.itervalues(): + for subsection_grade in chapter['sections']: + subsection_grades[subsection_grade.location] = subsection_grade + return subsection_grades + @lazy def locations_to_scores(self): """ diff --git a/lms/djangoapps/grades/new/course_grade_factory.py b/lms/djangoapps/grades/new/course_grade_factory.py index f71f86b3c1..410a9f81bc 100644 --- a/lms/djangoapps/grades/new/course_grade_factory.py +++ b/lms/djangoapps/grades/new/course_grade_factory.py @@ -153,9 +153,9 @@ class CourseGradeFactory(object): course_grade.update() should_persist = ( - not read_only and # TODO(TNL-6786) Remove the read_only boolean once all grades are back-filled. + (not read_only) and # TODO(TNL-6786) Remove the read_only boolean once all grades are back-filled. should_persist_grades(course_data.course_key) and - not waffle().is_enabled(WRITE_ONLY_IF_ENGAGED) or course_grade.attempted + (not waffle().is_enabled(WRITE_ONLY_IF_ENGAGED) or course_grade.attempted) ) if should_persist: course_grade._subsection_grade_factory.bulk_create_unsaved() diff --git a/lms/djangoapps/grades/tests/test_course_data.py b/lms/djangoapps/grades/tests/test_course_data.py index 8d98a9bbff..f8a78047cf 100644 --- a/lms/djangoapps/grades/tests/test_course_data.py +++ b/lms/djangoapps/grades/tests/test_course_data.py @@ -1,9 +1,11 @@ """ Tests for CourseData utility class. """ -from lms.djangoapps.course_blocks.api import get_course_blocks from mock import patch + +from lms.djangoapps.course_blocks.api import get_course_blocks from student.tests.factories import UserFactory +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from ..new.course_data import CourseData @@ -16,7 +18,10 @@ class CourseDataTest(ModuleStoreTestCase): def setUp(self): super(CourseDataTest, self).setUp() - self.course = CourseFactory.create() + with self.store.default_store(ModuleStoreEnum.Type.split): + self.course = CourseFactory.create() + # need to re-retrieve the course since the version on the original course isn't accurate. + self.course = self.store.get_course(self.course.id) self.user = UserFactory.create() self.one_true_structure = get_course_blocks(self.user, self.course.location) self.expected_results = { @@ -45,11 +50,28 @@ class CourseDataTest(ModuleStoreTestCase): actual = getattr(course_data, arg) self.assertEqual(expected, actual) - def test_no_data(self): - """ - Tests to ensure ??? happens when none of the data are provided. + def test_properties(self): + expected_edited_on = getattr( + self.one_true_structure[self.one_true_structure.root_block_usage_key], + 'subtree_edited_on', + ) - Maybe a dict pairing asked-for properties to resulting exceptions? Or an exception on init? - """ + for kwargs in [ + dict(course=self.course), + dict(collected_block_structure=self.one_true_structure), + dict(structure=self.one_true_structure), + dict(course_key=self.course.id), + ]: + course_data = CourseData(self.user, **kwargs) + self.assertEquals(course_data.course_key, self.course.id) + self.assertEquals(course_data.location, self.course.location) + self.assertEquals(course_data.structure.root_block_usage_key, self.one_true_structure.root_block_usage_key) + self.assertEquals(course_data.course.id, self.course.id) + self.assertEquals(course_data.version, self.course.course_version) + self.assertEquals(course_data.edited_on, expected_edited_on) + self.assertIn(u'Course: course_key', unicode(course_data)) + self.assertIn(u'Course: course_key', course_data.full_string()) + + def test_no_data(self): with self.assertRaises(ValueError): _ = CourseData(self.user) diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index e49761a335..656c010921 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -25,12 +25,13 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.utils import TEST_DATA_DIR from xmodule.modulestore.xml_importer import import_course_from_xml -from ..config.waffle import waffle, ASSUME_ZERO_GRADE_IF_ABSENT +from ..config.waffle import waffle, ASSUME_ZERO_GRADE_IF_ABSENT, WRITE_ONLY_IF_ENGAGED from ..models import PersistentSubsectionGrade from ..new.course_data import CourseData from ..new.course_grade_factory import CourseGradeFactory from ..new.course_grade import ZeroCourseGrade, CourseGrade -from ..new.subsection_grade_factory import SubsectionGrade, SubsectionGradeFactory +from ..new.subsection_grade_factory import SubsectionGradeFactory +from ..new.subsection_grade import ZeroSubsectionGrade, SubsectionGrade from .utils import mock_get_score, mock_get_submissions_score @@ -42,52 +43,63 @@ class GradeTestBase(SharedModuleStoreTestCase): def setUpClass(cls): super(GradeTestBase, 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, - format="Homework" - ) - 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 - ) + with cls.store.bulk_operations(cls.course.id): + 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, + format="Homework" + ) + 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.sequence2 = ItemFactory.create( + parent=cls.chapter, + category='sequential', + display_name="Test Sequential 2", + graded=True, + format="Homework" + ) + cls.problem2 = ItemFactory.create( + parent=cls.sequence2, + category="problem", + display_name="Test Problem", + data=problem_xml + ) def setUp(self): super(GradeTestBase, self).setUp() self.request = get_mock_request(UserFactory()) self.client.login(username=self.request.user.username, password="test") + self._update_grading_policy() self.course_structure = get_course_blocks(self.request.user, self.course.location) self.subsection_grade_factory = SubsectionGradeFactory(self.request.user, self.course, self.course_structure) CourseEnrollment.enroll(self.request.user, self.course.id) - -@ddt.ddt -class TestCourseGradeFactory(GradeTestBase): - """ - Test that CourseGrades are calculated properly - """ - def setUp(self): - super(TestCourseGradeFactory, self).setUp() - grading_policy = { + def _update_grading_policy(self, passing=0.5): + """ + Updates the course's grading policy. + """ + self.grading_policy = { "GRADER": [ { "type": "Homework", @@ -98,10 +110,27 @@ class TestCourseGradeFactory(GradeTestBase): }, ], "GRADE_CUTOFFS": { - "Pass": 0.5, + "Pass": passing, }, } - self.course.set_grading_policy(grading_policy) + self.course.set_grading_policy(self.grading_policy) + self.store.update_item(self.course, 0) + + +@ddt.ddt +class TestCourseGradeFactory(GradeTestBase): + """ + Test that CourseGrades are calculated properly + """ + def _assert_zero_grade(self, course_grade, expected_grade_class): + """ + Asserts whether the given course_grade is as expected with + zero values. + """ + self.assertIsInstance(course_grade, expected_grade_class) + self.assertIsNone(course_grade.letter_grade) + self.assertEqual(course_grade.percent, 0.0) + self.assertIsNotNone(course_grade.chapter_grades) def test_course_grade_no_access(self): """ @@ -138,60 +167,76 @@ class TestCourseGradeFactory(GradeTestBase): grade_factory.create(self.request.user, self.course) self.assertEqual(mock_read_grade.called, feature_flag and course_setting) - def test_course_grade_creation(self): + def test_create(self): grade_factory = CourseGradeFactory() - with mock_get_score(1, 2): + + def _assert_create(expected_pass): + """ + Creates the grade, ensuring it is as expected. + """ course_grade = grade_factory.create(self.request.user, self.course) - self.assertEqual(course_grade.letter_grade, u'Pass') - self.assertEqual(course_grade.percent, 0.5) + self.assertEqual(course_grade.letter_grade, u'Pass' if expected_pass else None) + self.assertEqual(course_grade.percent, 0.5) + + with self.assertNumQueries(12), mock_get_score(1, 2): + _assert_create(expected_pass=True) + + with self.assertNumQueries(14), mock_get_score(1, 2): + grade_factory.update(self.request.user, self.course) + + with self.assertNumQueries(1): + _assert_create(expected_pass=True) + + self._update_grading_policy(passing=0.9) + + with self.assertNumQueries(8): + _assert_create(expected_pass=False) @ddt.data(True, False) - def test_zero_course_grade(self, assume_zero_enabled): + def test_create_zero(self, assume_zero_enabled): with waffle().override(ASSUME_ZERO_GRADE_IF_ABSENT, active=assume_zero_enabled): grade_factory = CourseGradeFactory() - with mock_get_score(0, 2): - course_grade = grade_factory.create(self.request.user, self.course) + course_grade = grade_factory.create(self.request.user, self.course) + self._assert_zero_grade(course_grade, ZeroCourseGrade if assume_zero_enabled else CourseGrade) - self.assertIsInstance(course_grade, ZeroCourseGrade if assume_zero_enabled else CourseGrade) - self.assertIsNone(course_grade.letter_grade) - self.assertEqual(course_grade.percent, 0.0) - self.assertIsNotNone(course_grade.chapter_grades) + def test_create_zero_subs_grade_for_nonzero_course_grade(self): + with waffle().override(ASSUME_ZERO_GRADE_IF_ABSENT), waffle().override(WRITE_ONLY_IF_ENGAGED): + subsection = self.course_structure[self.sequence.location] + with mock_get_score(1, 2): + self.subsection_grade_factory.update(subsection) + course_grade = CourseGradeFactory().update(self.request.user, self.course) + subsection1_grade = course_grade.subsection_grades[self.sequence.location] + subsection2_grade = course_grade.subsection_grades[self.sequence2.location] + self.assertIsInstance(subsection1_grade, SubsectionGrade) + self.assertIsInstance(subsection2_grade, ZeroSubsectionGrade) - def test_get_persisted(self): + def test_read(self): grade_factory = CourseGradeFactory() - # first, create a grade in the database with mock_get_score(1, 2): grade_factory.update(self.request.user, self.course) - # retrieve the grade, ensuring it is as expected and take just one query - with self.assertNumQueries(1): - course_grade = grade_factory.read(self.request.user, self.course) - self.assertEqual(course_grade.letter_grade, u'Pass') - self.assertEqual(course_grade.percent, 0.5) + def _assert_read(): + """ + Reads the grade, ensuring it is as expected and requires just one query + """ + with self.assertNumQueries(1): + course_grade = grade_factory.read(self.request.user, self.course) + self.assertEqual(course_grade.letter_grade, u'Pass') + self.assertEqual(course_grade.percent, 0.5) - # update the grading policy - new_grading_policy = { - "GRADER": [ - { - "type": "Homework", - "min_count": 1, - "drop_count": 0, - "short_label": "HW", - "weight": 1.0, - }, - ], - "GRADE_CUTOFFS": { - "Pass": 0.9, - }, - } - self.course.set_grading_policy(new_grading_policy) + _assert_read() + self._update_grading_policy(passing=0.9) + _assert_read() - # ensure the grade can still be retrieved via read - # despite its outdated grading policy - with self.assertNumQueries(1): - course_grade = grade_factory.read(self.request.user, self.course) - self.assertEqual(course_grade.letter_grade, u'Pass') - self.assertEqual(course_grade.percent, 0.5) + @ddt.data(True, False) + def test_read_zero(self, assume_zero_enabled): + with waffle().override(ASSUME_ZERO_GRADE_IF_ABSENT, active=assume_zero_enabled): + grade_factory = CourseGradeFactory() + course_grade = grade_factory.read(self.request.user, course_key=self.course.id) + if assume_zero_enabled: + self._assert_zero_grade(course_grade, ZeroCourseGrade) + else: + self.assertIsNone(course_grade) @ddt.ddt @@ -305,7 +350,6 @@ class ZeroGradeTest(GradeTestBase): Tests ZeroCourseGrade (and, implicitly, ZeroSubsectionGrade) functionality. """ - def test_zero(self): """ Creates a ZeroCourseGrade and ensures it's empty. @@ -450,22 +494,23 @@ class TestVariedMetadata(ProblemSubmissionTestMixin, ModuleStoreTestCase): def setUp(self): super(TestVariedMetadata, self).setUp() self.course = CourseFactory.create() - self.chapter = ItemFactory.create( - parent=self.course, - category="chapter", - display_name="Test Chapter" - ) - self.sequence = ItemFactory.create( - parent=self.chapter, - category='sequential', - display_name="Test Sequential 1", - graded=True - ) - self.vertical = ItemFactory.create( - parent=self.sequence, - category='vertical', - display_name='Test Vertical 1' - ) + with self.store.bulk_operations(self.course.id): + self.chapter = ItemFactory.create( + parent=self.course, + category="chapter", + display_name="Test Chapter" + ) + self.sequence = ItemFactory.create( + parent=self.chapter, + category='sequential', + display_name="Test Sequential 1", + graded=True + ) + self.vertical = ItemFactory.create( + parent=self.sequence, + category='vertical', + display_name='Test Vertical 1' + ) self.problem_xml = u''' @@ -551,67 +596,68 @@ class TestCourseGradeLogging(ProblemSubmissionTestMixin, SharedModuleStoreTestCa def setUp(self): super(TestCourseGradeLogging, self).setUp() self.course = CourseFactory.create() - self.chapter = ItemFactory.create( - parent=self.course, - category="chapter", - display_name="Test Chapter" - ) - self.sequence = ItemFactory.create( - parent=self.chapter, - category='sequential', - display_name="Test Sequential 1", - graded=True - ) - self.sequence_2 = ItemFactory.create( - parent=self.chapter, - category='sequential', - display_name="Test Sequential 2", - graded=True - ) - self.sequence_3 = ItemFactory.create( - parent=self.chapter, - category='sequential', - display_name="Test Sequential 3", - graded=False - ) - self.vertical = ItemFactory.create( - parent=self.sequence, - category='vertical', - display_name='Test Vertical 1' - ) - self.vertical_2 = ItemFactory.create( - parent=self.sequence_2, - category='vertical', - display_name='Test Vertical 2' - ) - self.vertical_3 = ItemFactory.create( - parent=self.sequence_3, - category='vertical', - display_name='Test Vertical 3' - ) - problem_xml = MultipleChoiceResponseXMLFactory().build_xml( - question_text='The correct answer is Choice 2', - choices=[False, False, True, False], - choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] - ) - self.problem = ItemFactory.create( - parent=self.vertical, - category="problem", - display_name="test_problem_1", - data=problem_xml - ) - self.problem_2 = ItemFactory.create( - parent=self.vertical_2, - category="problem", - display_name="test_problem_2", - data=problem_xml - ) - self.problem_3 = ItemFactory.create( - parent=self.vertical_3, - category="problem", - display_name="test_problem_3", - data=problem_xml - ) + with self.store.bulk_operations(self.course.id): + self.chapter = ItemFactory.create( + parent=self.course, + category="chapter", + display_name="Test Chapter" + ) + self.sequence = ItemFactory.create( + parent=self.chapter, + category='sequential', + display_name="Test Sequential 1", + graded=True + ) + self.sequence_2 = ItemFactory.create( + parent=self.chapter, + category='sequential', + display_name="Test Sequential 2", + graded=True + ) + self.sequence_3 = ItemFactory.create( + parent=self.chapter, + category='sequential', + display_name="Test Sequential 3", + graded=False + ) + self.vertical = ItemFactory.create( + parent=self.sequence, + category='vertical', + display_name='Test Vertical 1' + ) + self.vertical_2 = ItemFactory.create( + parent=self.sequence_2, + category='vertical', + display_name='Test Vertical 2' + ) + self.vertical_3 = ItemFactory.create( + parent=self.sequence_3, + category='vertical', + display_name='Test Vertical 3' + ) + problem_xml = MultipleChoiceResponseXMLFactory().build_xml( + question_text='The correct answer is Choice 2', + choices=[False, False, True, False], + choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] + ) + self.problem = ItemFactory.create( + parent=self.vertical, + category="problem", + display_name="test_problem_1", + data=problem_xml + ) + self.problem_2 = ItemFactory.create( + parent=self.vertical_2, + category="problem", + display_name="test_problem_2", + data=problem_xml + ) + self.problem_3 = ItemFactory.create( + parent=self.vertical_3, + category="problem", + display_name="test_problem_3", + data=problem_xml + ) self.request = get_mock_request(UserFactory()) self.client.login(username=self.request.user.username, password="test") self.course_structure = get_course_blocks(self.request.user, self.course.location)