From 6917a29e84e2b8ac5bb2840457b661ee0214cb0f Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Wed, 4 Sep 2019 14:41:40 -0400 Subject: [PATCH] PROD-287 | Add an optional treat_undefined_as_zero flag to the helper functions is_score_higher_or_equal() and compare_scores(), which defaults to False, that we call with value True from SubsectionGradeFactory.update(), which occassionally deals with invalid subsection scores containing zero as the denominator. --- .../grades/subsection_grade_factory.py | 9 ++-- .../tests/test_subsection_grade_factory.py | 16 +++++++ openedx/core/lib/grade_utils.py | 30 ++++++++++-- openedx/core/lib/tests/test_grade_utils.py | 47 +++++++++++++++++++ 4 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 openedx/core/lib/tests/test_grade_utils.py diff --git a/lms/djangoapps/grades/subsection_grade_factory.py b/lms/djangoapps/grades/subsection_grade_factory.py index c62de6b034..ac47cc6149 100644 --- a/lms/djangoapps/grades/subsection_grade_factory.py +++ b/lms/djangoapps/grades/subsection_grade_factory.py @@ -88,10 +88,11 @@ class SubsectionGradeFactory(object): else: orig_subsection_grade = ReadSubsectionGrade(subsection, grade_model, self) if not is_score_higher_or_equal( - orig_subsection_grade.graded_total.earned, - orig_subsection_grade.graded_total.possible, - calculated_grade.graded_total.earned, - calculated_grade.graded_total.possible, + orig_subsection_grade.graded_total.earned, + orig_subsection_grade.graded_total.possible, + calculated_grade.graded_total.earned, + calculated_grade.graded_total.possible, + treat_undefined_as_zero=True, ): return orig_subsection_grade diff --git a/lms/djangoapps/grades/tests/test_subsection_grade_factory.py b/lms/djangoapps/grades/tests/test_subsection_grade_factory.py index 9e9fd3b5d8..93aa989984 100644 --- a/lms/djangoapps/grades/tests/test_subsection_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_subsection_grade_factory.py @@ -74,6 +74,22 @@ class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase): # ensure a grade has been persisted self.assertEqual(1, len(PersistentSubsectionGrade.objects.all())) + def test_update_if_higher_zero_denominator(self): + """ + Test that we get an updated score of 0, and not a ZeroDivisionError, + when dealing with an invalid score like 0/0. + """ + # This will create a PersistentSubsectionGrade with a score of 0/0. + with mock_get_score(0, 0): + grade = self.subsection_grade_factory.update(self.sequence) + self.assert_grade(grade, 0, 0) + + # Ensure that previously storing a possible score of 0 + # does not raise a ZeroDivisionError when updating the grade. + with mock_get_score(2, 2): + grade = self.subsection_grade_factory.update(self.sequence, only_if_higher=True) + self.assert_grade(grade, 2, 2) + def test_update_if_higher(self): def verify_update_if_higher(mock_score, expected_grade): """ diff --git a/openedx/core/lib/grade_utils.py b/openedx/core/lib/grade_utils.py index 5cf4dbf340..601b994d13 100644 --- a/openedx/core/lib/grade_utils.py +++ b/openedx/core/lib/grade_utils.py @@ -3,22 +3,42 @@ Helpers functions for grades and scores. """ -def compare_scores(earned1, possible1, earned2, possible2): +def compare_scores(earned1, possible1, earned2, possible2, treat_undefined_as_zero=False): """ Returns a tuple of: 1. Whether the 2nd set of scores is higher than the first. 2. Grade percentage of 1st set of scores. 3. Grade percentage of 2nd set of scores. + If ``treat_undefined_as_zero`` is True, this function will treat + cases where ``possible1`` or ``possible2`` is 0 as if + the (earned / possible) score is 0. If this flag is false, + a ZeroDivisionError is raised. """ - percentage1 = float(earned1) / float(possible1) - percentage2 = float(earned2) / float(possible2) + try: + percentage1 = float(earned1) / float(possible1) + except ZeroDivisionError: + if not treat_undefined_as_zero: + raise + percentage1 = 0.0 + + try: + percentage2 = float(earned2) / float(possible2) + except ZeroDivisionError: + if not treat_undefined_as_zero: + raise + percentage2 = 0.0 + is_higher = percentage2 >= percentage1 return is_higher, percentage1, percentage2 -def is_score_higher_or_equal(earned1, possible1, earned2, possible2): +def is_score_higher_or_equal(earned1, possible1, earned2, possible2, treat_undefined_as_zero=False): """ Returns whether the 2nd set of scores is higher than the first. + If ``treat_undefined_as_zero`` is True, this function will treat + cases where ``possible1`` or ``possible2`` is 0 as if + the (earned / possible) score is 0. If this flag is false, + a ZeroDivisionError is raised. """ - is_higher_or_equal, _, _ = compare_scores(earned1, possible1, earned2, possible2) + is_higher_or_equal, _, _ = compare_scores(earned1, possible1, earned2, possible2, treat_undefined_as_zero) return is_higher_or_equal diff --git a/openedx/core/lib/tests/test_grade_utils.py b/openedx/core/lib/tests/test_grade_utils.py new file mode 100644 index 0000000000..9c4f08216d --- /dev/null +++ b/openedx/core/lib/tests/test_grade_utils.py @@ -0,0 +1,47 @@ +""" +Tests for graph traversal generator functions. +""" + +from __future__ import absolute_import +from unittest import TestCase + +import ddt + +from ..grade_utils import compare_scores + + +@ddt.ddt +class TestGradeUtils(TestCase): + """ Tests for the grade_utils module. """ + @ddt.data( + (1, 2, 3, 4, False, True, 0.5, 0.75), + (3, 4, 1, 2, False, False, 0.75, 0.5), + (1, 2, 1, 2, False, True, 0.5, 0.5), + (1, 1, 0, 1, False, False, 1, 0), + ) + @ddt.unpack + def test_compare_scores_happy_path( + self, earned_1, possible_1, earned_2, possible_2, treat_undefined_as_zero, + expected_is_higher, expected_percentage_1, expected_percentage_2 + ): + is_higher, percentage_1, percentage_2 = compare_scores( + earned_1, possible_1, earned_2, possible_2, treat_undefined_as_zero + ) + assert expected_is_higher == is_higher + assert expected_percentage_1 == percentage_1 + assert expected_percentage_2 == percentage_2 + + def test_compare_scores_raise_zero_division(self): + with self.assertRaises(ZeroDivisionError): + compare_scores(1, 0, 1, 2) + + with self.assertRaises(ZeroDivisionError): + compare_scores(1, 2, 0, 0) + + def test_compare_scores_treat_undefined_as_zero(self): + is_higher, percentage_1, percentage_2 = compare_scores( + 0, 0, 0, 0, treat_undefined_as_zero=True + ) + assert is_higher is True + assert 0 == percentage_1 + assert 0 == percentage_2