Merge pull request #12381 from edx/clintonb/credit-fix

Multiple Credit Fixes

openedx/core/djangoapps/credit/admin.py

@@ -2,8 +2,10 @@
 Django admin page for credit eligibility
 """
 from ratelimitbackend import admin
+
 from openedx.core.djangoapps.credit.models import (
-    CreditConfig, CreditCourse, CreditProvider, CreditEligibility, CreditRequest
+    CreditConfig, CreditCourse, CreditProvider, CreditEligibility, CreditRequest, CreditRequirement,
+    CreditRequirementStatus
 )
 
 
@@ -47,8 +49,26 @@ class CreditRequestAdmin(admin.ModelAdmin):
         model = CreditRequest
 
 
+class CreditRequirementAdmin(admin.ModelAdmin):
+    """ Admin for CreditRequirement. """
+    list_display = ('course', 'namespace', 'name', 'display_name', 'active',)
+
+    class Meta(object):
+        model = CreditRequirement
+
+
+class CreditRequirementStatusAdmin(admin.ModelAdmin):
+    """ Admin for CreditRequirementStatus. """
+    list_display = ('username', 'requirement', 'status',)
+
+    class Meta(object):
+        model = CreditRequirementStatus
+
+
 admin.site.register(CreditCourse, CreditCourseAdmin)
 admin.site.register(CreditProvider, CreditProviderAdmin)
 admin.site.register(CreditEligibility, CreditEligibilityAdmin)
 admin.site.register(CreditRequest, CreditRequestAdmin)
 admin.site.register(CreditConfig)
+admin.site.register(CreditRequirement, CreditRequirementAdmin)
+admin.site.register(CreditRequirementStatus, CreditRequirementStatusAdmin)

openedx/core/djangoapps/credit/api/eligibility.py

@@ -7,13 +7,10 @@ import logging
 
 from opaque_keys.edx.keys import CourseKey
 
-from openedx.core.djangoapps.credit.exceptions import InvalidCreditRequirements, InvalidCreditCourse
 from openedx.core.djangoapps.credit.email_utils import send_credit_notifications
+from openedx.core.djangoapps.credit.exceptions import InvalidCreditRequirements, InvalidCreditCourse
 from openedx.core.djangoapps.credit.models import (
-    CreditCourse,
-    CreditRequirement,
-    CreditRequirementStatus,
-    CreditEligibility,
+    CreditCourse, CreditRequirement, CreditRequirementStatus, CreditEligibility, CreditRequest
 )
 
 # TODO: Cleanup this mess! ECOM-2908
@@ -228,13 +225,21 @@ def set_credit_requirement_status(username, course_key, req_namespace, req_name,
             )
 
     """
-    # Check if we're already eligible for credit.
-    # If so, short-circuit this process.
-    if CreditEligibility.is_user_eligible_for_credit(course_key, username):
+    # Do not allow students who have requested credit to change their eligibility
+    if CreditRequest.get_user_request_status(username, course_key):
+        log.info(
+            u'Refusing to set status of requirement with namespace "%s" and name "%s" because the '
+            u'user "%s" has already requested credit for the course "%s".',
+            req_namespace, req_name, username, course_key
+        )
+        return
+
+    # Do not allow a student who has earned eligibility to un-earn eligibility
+    eligible_before_update = CreditEligibility.is_user_eligible_for_credit(course_key, username)
+    if eligible_before_update and status == 'failed':
         log.info(
-            u'Skipping update of credit requirement with namespace "%s" '
-            u'and name "%s" because the user "%s" is already eligible for credit '
-            u'in the course "%s".',
+            u'Refusing to set status of requirement with namespace "%s" and name "%s" to "failed" because the '
+            u'user "%s" is already eligible for credit in the course "%s".',
             req_namespace, req_name, username, course_key
         )
         return
@@ -273,9 +278,9 @@ def set_credit_requirement_status(username, course_key, req_namespace, req_name,
         username, req_to_update, status=status, reason=reason
     )
 
-    # If we're marking this requirement as "satisfied", there's a chance
-    # that the user has met all eligibility requirements.
-    if status == "satisfied":
+    # If we're marking this requirement as "satisfied", there's a chance that the user has met all eligibility
+    # requirements, and should be notified. However, if the user was already eligible, do not send another notification.
+    if status == "satisfied" and not eligible_before_update:
         is_eligible, eligibility_record_created = CreditEligibility.update_eligibility(reqs, username, course_key)
         if eligibility_record_created and is_eligible:
             try:

openedx/core/djangoapps/credit/models.py

@@ -294,6 +294,9 @@ class CreditRequirement(TimeStampedModel):
         unique_together = ('namespace', 'name', 'course')
         ordering = ["order"]
 
+    def __unicode__(self):
+        return self.display_name
+
     @classmethod
     def add_or_update_course_requirement(cls, credit_course, requirement, order):
         """

openedx/core/djangoapps/credit/signals.py

@@ -7,11 +7,10 @@ import logging
 from django.dispatch import receiver
 from django.utils import timezone
 from opaque_keys.edx.keys import CourseKey
-
-from openedx.core.djangoapps.signals.signals import GRADES_UPDATED
-from openedx.core.djangoapps.credit.verification_access import update_verification_partitions
 from xmodule.modulestore.django import SignalHandler
 
+from openedx.core.djangoapps.credit.verification_access import update_verification_partitions
+from openedx.core.djangoapps.signals.signals import GRADES_UPDATED
 
 log = logging.getLogger(__name__)
 
@@ -55,8 +54,7 @@ def on_pre_publish(sender, course_key, **kwargs):  # pylint: disable=unused-argu
 
 @receiver(GRADES_UPDATED)
 def listen_for_grade_calculation(sender, username, grade_summary, course_key, deadline, **kwargs):  # pylint: disable=unused-argument
-    """Receive 'MIN_GRADE_REQUIREMENT_STATUS' signal and update minimum grade
-    requirement status.
+    """Receive 'MIN_GRADE_REQUIREMENT_STATUS' signal and update minimum grade requirement status.
 
     Args:
         sender: None
@@ -81,12 +79,39 @@ def listen_for_grade_calculation(sender, username, grade_summary, course_key, de
             criteria = requirements[0].get('criteria')
             if criteria:
                 min_grade = criteria.get('min_grade')
-                if grade_summary['percent'] >= min_grade:
-                    reason_dict = {'final_grade': grade_summary['percent']}
-                    api.set_credit_requirement_status(
-                        username, course_id, 'grade', 'grade', status="satisfied", reason=reason_dict
-                    )
-                elif deadline and deadline < timezone.now():
+                passing_grade = grade_summary['percent'] >= min_grade
+                now = timezone.now()
+                status = None
+                reason = None
+
+                if (deadline and now < deadline) or not deadline:
+                    # Student completed coursework on-time
+
+                    if passing_grade:
+                        # Student received a passing grade
+                        status = 'satisfied'
+                        reason = {'final_grade': grade_summary['percent']}
+                else:
+                    # Submission after deadline
+
+                    if passing_grade:
+                        # Grade was good, but submission arrived too late
+                        status = 'failed'
+                        reason = {
+                            'current_date': now,
+                            'deadline': deadline
+                        }
+                    else:
+                        # Student failed to receive minimum grade
+                        status = 'failed'
+                        reason = {
+                            'final_grade': grade_summary['percent'],
+                            'minimum_grade': min_grade
+                        }
+
+                # We do not record a status if the user has not yet earned the minimum grade, but still has
+                # time to do so.
+                if status and reason:
                     api.set_credit_requirement_status(
-                        username, course_id, 'grade', 'grade', status="failed", reason={}
+                        username, course_id, 'grade', 'grade', status=status, reason=reason
                     )

openedx/core/djangoapps/credit/tests/test_api.py

@@ -33,7 +33,8 @@ from openedx.core.djangoapps.credit.models import (
     CreditProvider,
     CreditRequirement,
     CreditRequirementStatus,
-    CreditEligibility
+    CreditEligibility,
+    CreditRequest
 )
 from student.tests.factories import UserFactory
 from util.date_utils import from_timestamp
@@ -371,8 +372,16 @@ class CreditRequirementApiTests(CreditApiTestBase):
         eligibilities = api.get_eligibilities_for_user("staff")
         self.assertEqual(eligibilities, [])
 
+    def assert_grade_requirement_status(self, expected_status, expected_order):
+        """ Assert the status and order of the grade requirement. """
+        req_status = api.get_credit_requirement_status(self.course_key, 'staff', namespace="grade", name="grade")
+        self.assertEqual(req_status[0]["status"], expected_status)
+        self.assertEqual(req_status[0]["order"], expected_order)
+        return req_status
+
     def test_set_credit_requirement_status(self):
-        self.add_credit_course()
+        username = "staff"
+        credit_course = self.add_credit_course()
         requirements = [
             {
                 "namespace": "grade",
@@ -395,40 +404,44 @@ class CreditRequirementApiTests(CreditApiTestBase):
         self.assertEqual(len(course_requirements), 2)
 
         # Initially, the status should be None
-        req_status = api.get_credit_requirement_status(self.course_key, "staff", namespace="grade", name="grade")
-        self.assertEqual(req_status[0]["status"], None)
-        self.assertEqual(req_status[0]["order"], 0)
+        self.assert_grade_requirement_status(None, 0)
+
+        # Requirement statuses cannot be changed if a CreditRequest exists
+        credit_request = CreditRequest.objects.create(
+            course=credit_course,
+            provider=CreditProvider.objects.first(),
+            username=username,
+        )
+        api.set_credit_requirement_status(username, self.course_key, "grade", "grade")
+        self.assert_grade_requirement_status(None, 0)
+        credit_request.delete()
 
         # Set the requirement to "satisfied" and check that it's actually set
-        api.set_credit_requirement_status("staff", self.course_key, "grade", "grade")
-        req_status = api.get_credit_requirement_status(self.course_key, "staff", namespace="grade", name="grade")
-        self.assertEqual(req_status[0]["status"], "satisfied")
-        self.assertEqual(req_status[0]["order"], 0)
+        api.set_credit_requirement_status(username, self.course_key, "grade", "grade")
+        self.assert_grade_requirement_status('satisfied', 0)
 
         # Set the requirement to "failed" and check that it's actually set
-        api.set_credit_requirement_status("staff", self.course_key, "grade", "grade", status="failed")
-        req_status = api.get_credit_requirement_status(self.course_key, "staff", namespace="grade", name="grade")
-        self.assertEqual(req_status[0]["status"], "failed")
-        self.assertEqual(req_status[0]["order"], 0)
+        api.set_credit_requirement_status(username, self.course_key, "grade", "grade", status="failed")
+        self.assert_grade_requirement_status('failed', 0)
 
         req_status = api.get_credit_requirement_status(self.course_key, "staff")
         self.assertEqual(req_status[0]["status"], "failed")
         self.assertEqual(req_status[0]["order"], 0)
 
-        # make sure the 'order' on the 2nd requiemtn is set correctly (aka 1)
+        # make sure the 'order' on the 2nd requirement is set correctly (aka 1)
         self.assertEqual(req_status[1]["status"], None)
         self.assertEqual(req_status[1]["order"], 1)
 
         # Set the requirement to "declined" and check that it's actually set
         api.set_credit_requirement_status(
-            "staff", self.course_key,
+            username, self.course_key,
             "reverification",
             "i4x://edX/DemoX/edx-reverification-block/assessment_uuid",
             status="declined"
         )
         req_status = api.get_credit_requirement_status(
             self.course_key,
-            "staff",
+            username,
             namespace="reverification",
             name="i4x://edX/DemoX/edx-reverification-block/assessment_uuid"
         )
@@ -530,7 +543,7 @@ class CreditRequirementApiTests(CreditApiTestBase):
         user = UserFactory.create(username=self.USER_INFO['username'], password=self.USER_INFO['password'])
 
         # Satisfy one of the requirements, but not the other
-        with self.assertNumQueries(11):
+        with self.assertNumQueries(12):
             api.set_credit_requirement_status(
                 user.username,
                 self.course_key,
@@ -542,7 +555,7 @@ class CreditRequirementApiTests(CreditApiTestBase):
         self.assertFalse(api.is_user_eligible_for_credit("bob", self.course_key))
 
         # Satisfy the other requirement
-        with self.assertNumQueries(19):
+        with self.assertNumQueries(20):
             api.set_credit_requirement_status(
                 "bob",
                 self.course_key,
@@ -596,7 +609,7 @@ class CreditRequirementApiTests(CreditApiTestBase):
         # Delete the eligibility entries and satisfy the user's eligibility
         # requirement again to trigger eligibility notification
         CreditEligibility.objects.all().delete()
-        with self.assertNumQueries(15):
+        with self.assertNumQueries(16):
             api.set_credit_requirement_status(
                 "bob",
                 self.course_key,

openedx/core/djangoapps/credit/tests/test_signals.py

@@ -2,24 +2,23 @@
 Tests for minimum grade requirement status
 """
 
-import pytz
 import ddt
+import pytz
 from datetime import timedelta, datetime
-from nose.plugins.attrib import attr
+from unittest import skipUnless
 
 from django.conf import settings
 from django.test.client import RequestFactory
-from unittest import skipUnless
+from nose.plugins.attrib import attr
+from student.tests.factories import UserFactory
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
 
 from openedx.core.djangoapps.credit.api import (
     set_credit_requirements, get_credit_requirement_status
 )
-
 from openedx.core.djangoapps.credit.models import CreditCourse, CreditProvider
 from openedx.core.djangoapps.credit.signals import listen_for_grade_calculation
-from student.tests.factories import UserFactory
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-from xmodule.modulestore.tests.factories import CourseFactory
 
 
 @attr('shard_2')
@@ -67,20 +66,35 @@ class TestMinGradedRequirementStatus(ModuleStoreTestCase):
         # Add a single credit requirement (final grade)
         set_credit_requirements(self.course.id, requirements)
 
+    def assert_requirement_status(self, grade, due_date, expected_status):
+        """ Verify the user's credit requirement status is as expected after simulating a grading calculation. """
+        listen_for_grade_calculation(None, self.user.username, {'percent': grade}, self.course.id, due_date)
+        req_status = get_credit_requirement_status(self.course.id, self.request.user.username, 'grade', 'grade')
+
+        self.assertEqual(req_status[0]['status'], expected_status)
+
+        if expected_status == 'satisfied':
+            expected_reason = {'final_grade': grade}
+            self.assertEqual(req_status[0]['reason'], expected_reason)
+
     @ddt.data(
         (0.6, VALID_DUE_DATE),
-        (0.52, VALID_DUE_DATE),
-        (0.70, EXPIRED_DUE_DATE),
+        (0.52, None),
     )
     @ddt.unpack
-    def test_min_grade_requirement_with_valid_grade(self, grade_achieved, due_date):
-        """Test with valid grades. Deadline date does not effect in case
-        of valid grade.
-        """
+    def test_min_grade_requirement_with_valid_grade(self, grade, due_date):
+        """Test with valid grades submitted before deadline"""
+        self.assert_requirement_status(grade, due_date, 'satisfied')
 
-        listen_for_grade_calculation(None, self.user.username, {'percent': grade_achieved}, self.course.id, due_date)
-        req_status = get_credit_requirement_status(self.course.id, self.request.user.username, 'grade', 'grade')
-        self.assertEqual(req_status[0]["status"], 'satisfied')
+    def test_grade_changed(self):
+        """ Verify successive calls to update a satisfied grade requirement are recorded. """
+        self.assert_requirement_status(0.6, self.VALID_DUE_DATE, 'satisfied')
+        self.assert_requirement_status(0.75, self.VALID_DUE_DATE, 'satisfied')
+        self.assert_requirement_status(0.70, self.VALID_DUE_DATE, 'satisfied')
+
+    def test_min_grade_requirement_with_valid_grade_and_expired_deadline(self):
+        """ Verify the status is set to failure if a passing grade is received past the submission deadline. """
+        self.assert_requirement_status(0.70, self.EXPIRED_DUE_DATE, 'failed')
 
     @ddt.data(
         (0.50, None),
@@ -88,16 +102,10 @@ class TestMinGradedRequirementStatus(ModuleStoreTestCase):
         (0.40, VALID_DUE_DATE),
     )
     @ddt.unpack
-    def test_min_grade_requirement_failed_grade_valid_deadline(self, grade_achieved, due_date):
+    def test_min_grade_requirement_failed_grade_valid_deadline(self, grade, due_date):
         """Test with failed grades and deadline is still open or not defined."""
-
-        listen_for_grade_calculation(None, self.user.username, {'percent': grade_achieved}, self.course.id, due_date)
-        req_status = get_credit_requirement_status(self.course.id, self.request.user.username, 'grade', 'grade')
-        self.assertEqual(req_status[0]["status"], None)
+        self.assert_requirement_status(grade, due_date, None)
 
     def test_min_grade_requirement_failed_grade_expired_deadline(self):
         """Test with failed grades and deadline expire"""
-
-        listen_for_grade_calculation(None, self.user.username, {'percent': 0.22}, self.course.id, self.EXPIRED_DUE_DATE)
-        req_status = get_credit_requirement_status(self.course.id, self.request.user.username, 'grade', 'grade')
-        self.assertEqual(req_status[0]["status"], 'failed')
+        self.assert_requirement_status(0.22, self.EXPIRED_DUE_DATE, 'failed')