Merge pull request #8648 from edx/will/eligibility-provider-refactor

Update credit eligibility

lms/djangoapps/courseware/views.py

@@ -1066,27 +1066,6 @@ def _progress(request, course_key, student_id):
     # checking certificate generation configuration
     show_generate_cert_btn = certs_api.cert_generation_enabled(course_key)
 
-    credit_course_requirements = None
-    is_course_credit = settings.FEATURES.get("ENABLE_CREDIT_ELIGIBILITY", False) and is_credit_course(course_key)
-    if is_course_credit:
-        requirement_statuses = get_credit_requirement_status(course_key, student.username)
-        if any(requirement['status'] == 'failed' for requirement in requirement_statuses):
-            eligibility_status = "not_eligible"
-        elif is_user_eligible_for_credit(student.username, course_key):
-            eligibility_status = "eligible"
-        else:
-            eligibility_status = "partial_eligible"
-
-        paired_requirements = {}
-        for requirement in requirement_statuses:
-            namespace = requirement.pop("namespace")
-            paired_requirements.setdefault(namespace, []).append(requirement)
-
-        credit_course_requirements = {
-            'eligibility_status': eligibility_status,
-            'requirements': OrderedDict(sorted(paired_requirements.items(), reverse=True))
-        }
-
     context = {
         'course': course,
         'courseware_summary': courseware_summary,
@@ -1096,8 +1075,7 @@ def _progress(request, course_key, student_id):
         'student': student,
         'passed': is_course_passed(course, grade_summary),
         'show_generate_cert_btn': show_generate_cert_btn,
-        'credit_course_requirements': credit_course_requirements,
-        'is_credit_course': is_course_credit,
+        'credit_course_requirements': _credit_course_requirements(course_key, student),
     }
 
     if show_generate_cert_btn:
@@ -1124,6 +1102,64 @@ def _progress(request, course_key, student_id):
     return response
 
 
+def _credit_course_requirements(course_key, student):
+    """Return information about which credit requirements a user has satisfied.
+
+    Arguments:
+        course_key (CourseKey): Identifier for the course.
+        student (User): Currently logged in user.
+
+    Returns: dict
+
+    """
+    # If credit eligibility is not enabled or this is not a credit course,
+    # short-circuit and return `None`.  This indicates that credit requirements
+    # should NOT be displayed on the progress page.
+    if not (settings.FEATURES.get("ENABLE_CREDIT_ELIGIBILITY", False) and is_credit_course(course_key)):
+        return None
+
+    # Retrieve the status of the user for each eligibility requirement in the course.
+    # For each requirement, the user's status is either "satisfied", "failed", or None.
+    # In this context, `None` means that we don't know the user's status, either because
+    # the user hasn't done something (for example, submitting photos for verification)
+    # or we're waiting on more information (for example, a response from the photo
+    # verification service).
+    requirement_statuses = get_credit_requirement_status(course_key, student.username)
+
+    # If the user has been marked as "eligible", then they are *always* eligible
+    # unless someone manually intervenes.  This could lead to some strange behavior
+    # if the requirements change post-launch.  For example, if the user was marked as eligible
+    # for credit, then a new requirement was added, the user will see that they're eligible
+    # AND that one of the requirements is still pending.
+    # We're assuming here that (a) we can mitigate this by properly training course teams,
+    # and (b) it's a better user experience to allow students who were at one time
+    # marked as eligible to continue to be eligible.
+    # If we need to, we can always manually move students back to ineligible by
+    # deleting CreditEligibility records in the database.
+    if is_user_eligible_for_credit(student.username, course_key):
+        eligibility_status = "eligible"
+
+    # If the user has *failed* any requirements (for example, if a photo verification is denied),
+    # then the user is NOT eligible for credit.
+    elif any(requirement['status'] == 'failed' for requirement in requirement_statuses):
+        eligibility_status = "not_eligible"
+
+    # Otherwise, the user may be eligible for credit, but the user has not
+    # yet completed all the requirements.
+    else:
+        eligibility_status = "partial_eligible"
+
+    paired_requirements = {}
+    for requirement in requirement_statuses:
+        namespace = requirement.pop("namespace")
+        paired_requirements.setdefault(namespace, []).append(requirement)
+
+    return {
+        'eligibility_status': eligibility_status,
+        'requirements': OrderedDict(sorted(paired_requirements.items(), reverse=True))
+    }
+
+
 @login_required
 @ensure_valid_course_key
 def submission_history(request, course_id, student_username, location):

lms/djangoapps/verify_student/views.py

@@ -38,7 +38,7 @@ from microsite_configuration import microsite
 from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
 from openedx.core.djangoapps.user_api.accounts.api import get_account_settings, update_account_settings
 from openedx.core.djangoapps.user_api.errors import UserNotFound, AccountValidationError
-from openedx.core.djangoapps.credit.api import get_credit_requirement, set_credit_requirement_status
+from openedx.core.djangoapps.credit.api import set_credit_requirement_status
 from student.models import CourseEnrollment
 from shoppingcart.models import Order, CertificateItem
 from shoppingcart.processors import (
@@ -897,14 +897,14 @@ def _set_user_requirement_status(attempt, namespace, status, reason=None):
         log.error("Unable to find checkpoint for user with id %d", attempt.user.id)
 
     if checkpoint is not None:
-        course_key = checkpoint.course_id
-        credit_requirement = get_credit_requirement(
-            course_key, namespace, checkpoint.checkpoint_location
-        )
-        if credit_requirement is not None:
         try:
             set_credit_requirement_status(
-                    attempt.user.username, credit_requirement, status, reason
+                attempt.user.username,
+                checkpoint.course_id,
+                namespace,
+                checkpoint.checkpoint_location,
+                status=status,
+                reason=reason,
             )
         except Exception:  # pylint: disable=broad-except
             # Catch exception if unable to add credit requirement

lms/templates/courseware/progress.html

@@ -103,7 +103,7 @@ from django.utils.http import urlquote_plus
         <div class="grade-detail-graph" id="grade-detail-graph" aria-hidden="true"></div>
       %endif
 
-      % if is_credit_course:
+      % if credit_course_requirements:
         <section class="credit-eligibility">
             <div class="credit-eligibility-container">
                 <div class="eligibility-heading">

openedx/core/djangoapps/credit/api.py

@@ -274,12 +274,12 @@ def create_credit_request(course_key, provider_id, username):
 
     # Retrieve the final grade from the eligibility table
     try:
-        final_grade = CreditRequirementStatus.objects.filter(
+        final_grade = CreditRequirementStatus.objects.get(
             username=username,
             requirement__namespace="grade",
             requirement__name="grade",
             status="satisfied"
-        ).latest().reason["final_grade"]
+        ).reason["final_grade"]
     except (CreditRequirementStatus.DoesNotExist, TypeError, KeyError):
         log.exception(
             "Could not retrieve final grade from the credit eligibility table "
@@ -410,7 +410,7 @@ def get_credit_requests_for_user(username):
     return CreditRequest.credit_requests_for_user(username)
 
 
-def get_credit_requirement_status(course_key, username):
+def get_credit_requirement_status(course_key, username, namespace=None, name=None):
     """ Retrieve the user's status for each credit requirement in the course.
 
     Args:
@@ -447,7 +447,7 @@ def get_credit_requirement_status(course_key, username):
     Returns:
         list of requirement statuses
     """
-    requirements = CreditRequirement.get_course_requirements(course_key)
+    requirements = CreditRequirement.get_course_requirements(course_key, namespace=namespace, name=name)
     requirement_statuses = CreditRequirementStatus.get_statuses(requirements, username)
     requirement_statuses = dict((o.requirement, o) for o in requirement_statuses)
     statuses = []
@@ -511,36 +511,80 @@ def get_credit_requirement(course_key, namespace, name):
     } if requirement else None
 
 
-def set_credit_requirement_status(username, requirement, status="satisfied", reason=None):
-    """Update Credit Requirement Status for given username and requirement
-        if exists else add new.
+def set_credit_requirement_status(username, course_key, req_namespace, req_name, status="satisfied", reason=None):
+    """
+    Update the user's requirement status.
+
+    This will record whether the user satisfied or failed a particular requirement
+    in a course.  If the user has satisfied all requirements, the user will be marked
+    as eligible for credit in the course.
 
     Args:
-        username(str): Username of the user
-        requirement(dict): requirement dict
-        status(str): Status of the requirement
-        reason(dict): Reason of the status
+        username (str): Username of the user
+        course_key (CourseKey): Identifier for the course associated with the requirement.
+        req_namespace (str): Namespace of the requirement (e.g. "grade" or "reverification")
+        req_name (str): Name of the requirement (e.g. "grade" or the location of the ICRV XBlock)
+
+    Keyword Arguments:
+        status (str): Status of the requirement (either "satisfied" or "failed")
+        reason (dict): Reason of the status
 
     Example:
         >>> set_credit_requirement_status(
                 "staff",
-            {
-                "course_key": "course-v1-edX-DemoX-1T2015"
-                "namespace": "reverification",
-                "name": "i4x://edX/DemoX/edx-reverification-block/assessment_uuid",
-            },
-            "satisfied",
-            {}
+                CourseKey.from_string("course-v1-edX-DemoX-1T2015"),
+                "reverification",
+                "i4x://edX/DemoX/edx-reverification-block/assessment_uuid",
+                status="satisfied",
+                reason={}
             )
 
     """
-    credit_requirement = CreditRequirement.get_course_requirement(
-        requirement['course_key'], requirement['namespace'], requirement['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):
+        return
+
+    # Retrieve all credit requirements for the course
+    # We retrieve all of them to avoid making a second query later when
+    # we need to check whether all requirements have been satisfied.
+    reqs = CreditRequirement.get_course_requirements(course_key)
+
+    # Find the requirement we're trying to set
+    req_to_update = next((
+        req for req in reqs
+        if req.namespace == req_namespace
+        and req.name == req_name
+    ), None)
+
+    # If we can't find the requirement, then the most likely explanation
+    # is that there was a lag updating the credit requirements after the course
+    # was published.  We *could* attempt to create the requirement here,
+    # but that could cause serious performance issues if many users attempt to
+    # lock the row at the same time.
+    # Instead, we skip updating the requirement and log an error.
+    if req_to_update is None:
+        log.error(
+            (
+                u'Could not update credit requirement in course "%s" '
+                u'with namespace "%s" and name "%s" '
+                u'because the requirement does not exist. '
+                u'The user "%s" should have had his/her status updated to "%s".'
+            ),
+            unicode(course_key), req_namespace, req_name, username, status
         )
+        return
+
+    # Update the requirement status
     CreditRequirementStatus.add_or_update_requirement_status(
-        username, credit_requirement, status, reason
+        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":
+        CreditEligibility.update_eligibility(reqs, username, course_key)
+
 
 def _get_requirements_to_disable(old_requirements, new_requirements):
     """

openedx/core/djangoapps/credit/models.py

@@ -6,10 +6,10 @@ Credit courses allow students to receive university credit for
 successful completion of a course on EdX
 """
 
+from collections import defaultdict
 import logging
 
-from django.db import models
-from django.db import transaction
+from django.db import models, transaction, IntegrityError
 from django.core.validators import RegexValidator
 from simple_history.models import HistoricalRecords
 
@@ -168,8 +168,11 @@ class CreditRequirement(TimeStampedModel):
             course=credit_course,
             namespace=requirement["namespace"],
             name=requirement["name"],
-            display_name=requirement["display_name"],
-            defaults={"criteria": requirement["criteria"], "active": True}
+            defaults={
+                "display_name": requirement["display_name"],
+                "criteria": requirement["criteria"],
+                "active": True
+            }
         )
         if not created:
             credit_requirement.criteria = requirement["criteria"]
@@ -179,20 +182,29 @@ class CreditRequirement(TimeStampedModel):
         return credit_requirement, created
 
     @classmethod
-    def get_course_requirements(cls, course_key, namespace=None):
+    def get_course_requirements(cls, course_key, namespace=None, name=None):
         """
         Get credit requirements of a given course.
 
         Args:
-            course_key(CourseKey): The identifier for a course
-            namespace(str): Namespace of credit course requirements
+            course_key (CourseKey): The identifier for a course
+
+        Keyword Arguments
+            namespace (str): Optionally filter credit requirements by namespace.
+            name (str): Optionally filter credit requirements by name.
 
         Returns:
             QuerySet of CreditRequirement model
+
         """
         requirements = CreditRequirement.objects.filter(course__course_key=course_key, active=True)
-        if namespace:
+
+        if namespace is not None:
             requirements = requirements.filter(namespace=namespace)
+
+        if name is not None:
+            requirements = requirements.filter(name=name)
+
         return requirements
 
     @classmethod
@@ -261,8 +273,11 @@ class CreditRequirementStatus(TimeStampedModel):
     # the grade to users later and to send the information to credit providers.
     reason = JSONField(default={})
 
+    # Maintain a history of requirement status updates for auditing purposes
+    history = HistoricalRecords()
+
     class Meta(object):  # pylint: disable=missing-docstring
-        get_latest_by = "created"
+        unique_together = ('username', 'requirement')
 
     @classmethod
     def get_statuses(cls, requirements, username):
@@ -325,6 +340,40 @@ class CreditEligibility(TimeStampedModel):
         return cls.objects.filter(username=username).select_related('course').prefetch_related('course__providers')
 
     @classmethod
+    def update_eligibility(cls, requirements, username, course_key):
+        """
+        Update the user's credit eligibility for a course.
+
+        A user is eligible for credit when the user has satisfied
+        all requirements for credit in the course.
+
+        Arguments:
+            requirements (Queryset): Queryset of `CreditRequirement`s to check.
+            username (str): Identifier of the user being updated.
+            course_key (CourseKey): Identifier of the course.
+
+        """
+        # Check all requirements for the course to determine if the user
+        # is eligible.  We need to check all the *requirements*
+        # (not just the *statuses*) in case the user doesn't yet have
+        # a status for a particular requirement.
+        status_by_req = defaultdict(lambda: False)
+        for status in CreditRequirementStatus.get_statuses(requirements, username):
+            status_by_req[status.requirement.id] = status.status
+
+        is_eligible = all(status_by_req[req.id] == "satisfied" for req in requirements)
+
+        # If we're eligible, then mark the user as being eligible for credit.
+        if is_eligible:
+            try:
+                CreditEligibility.objects.create(
+                    username=username,
+                    course=CreditCourse.objects.get(course_key=course_key),
+                )
+            except IntegrityError:
+                pass
+
+    @classmethod
     def is_user_eligible_for_credit(cls, course_key, username):
         """Check if the given user is eligible for the provided credit course
 

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

@@ -30,11 +30,6 @@ from openedx.core.djangoapps.credit.models import (
     CreditRequirementStatus,
     CreditEligibility
 )
-from openedx.core.djangoapps.credit.api import (
-    set_credit_requirements,
-    set_credit_requirement_status,
-    get_credit_requirement
-)
 from student.models import CourseEnrollment
 from student.views import _create_credit_availability_message
 from student.tests.factories import UserFactory
@@ -240,7 +235,7 @@ class CreditRequirementApiTests(CreditApiTestBase):
                 }
             }
         ]
-        requirement = get_credit_requirement(self.course_key, "grade", "grade")
+        requirement = api.get_credit_requirement(self.course_key, "grade", "grade")
         self.assertIsNone(requirement)
 
         expected_requirement = {
@@ -252,8 +247,8 @@ class CreditRequirementApiTests(CreditApiTestBase):
                 "min_grade": 0.8
             }
         }
-        set_credit_requirements(self.course_key, requirements)
-        requirement = get_credit_requirement(self.course_key, "grade", "grade")
+        api.set_credit_requirements(self.course_key, requirements)
+        requirement = api.get_credit_requirement(self.course_key, "grade", "grade")
         self.assertIsNotNone(requirement)
         self.assertEqual(requirement, expected_requirement)
 
@@ -276,25 +271,128 @@ class CreditRequirementApiTests(CreditApiTestBase):
             }
         ]
 
-        set_credit_requirements(self.course_key, requirements)
-        course_requirements = CreditRequirement.get_course_requirements(self.course_key)
+        api.set_credit_requirements(self.course_key, requirements)
+        course_requirements = api.get_credit_requirements(self.course_key)
         self.assertEqual(len(course_requirements), 2)
 
-        requirement = get_credit_requirement(self.course_key, "grade", "grade")
-        set_credit_requirement_status("staff", requirement, 'satisfied', {})
-        course_requirement = CreditRequirement.get_course_requirement(
-            requirement['course_key'], requirement['namespace'], requirement['name']
+        # 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)
+
+        # 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")
+
+        # 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")
+
+    def test_satisfy_all_requirements(self):
+        # Configure a course with two credit requirements
+        self.add_credit_course()
+        requirements = [
+            {
+                "namespace": "grade",
+                "name": "grade",
+                "display_name": "Grade",
+                "criteria": {
+                    "min_grade": 0.8
+                }
+            },
+            {
+                "namespace": "reverification",
+                "name": "i4x://edX/DemoX/edx-reverification-block/assessment_uuid",
+                "display_name": "Assessment 1",
+                "criteria": {}
+            }
+        ]
+        api.set_credit_requirements(self.course_key, requirements)
+
+        # Satisfy one of the requirements, but not the other
+        with self.assertNumQueries(7):
+            api.set_credit_requirement_status(
+                "bob",
+                self.course_key,
+                requirements[0]["namespace"],
+                requirements[0]["name"]
+            )
+
+        # The user should not be eligible (because only one requirement is satisfied)
+        self.assertFalse(api.is_user_eligible_for_credit("bob", self.course_key))
+
+        # Satisfy the other requirement
+        with self.assertNumQueries(10):
+            api.set_credit_requirement_status(
+                "bob",
+                self.course_key,
+                requirements[1]["namespace"],
+                requirements[1]["name"]
             )
-        status = CreditRequirementStatus.objects.get(username="staff", requirement=course_requirement)
-        self.assertEqual(status.requirement.namespace, requirement['namespace'])
-        self.assertEqual(status.status, "satisfied")
 
-        set_credit_requirement_status(
-            "staff", requirement, 'failed', {'failure_reason': "requirements not satisfied"}
+        # Now the user should be eligible
+        self.assertTrue(api.is_user_eligible_for_credit("bob", self.course_key))
+
+        # The user should remain eligible even if the requirement status is later changed
+        api.set_credit_requirement_status(
+            "bob",
+            self.course_key,
+            requirements[0]["namespace"],
+            requirements[0]["name"],
+            status="failed"
+        )
+        self.assertTrue(api.is_user_eligible_for_credit("bob", self.course_key))
+
+    def test_set_credit_requirement_status_req_not_configured(self):
+        # Configure a credit course with no requirements
+        self.add_credit_course()
+
+        # A user satisfies a requirement.  This could potentially
+        # happen if there's a lag when the requirements are updated
+        # after the course is published.
+        api.set_credit_requirement_status("bob", self.course_key, "grade", "grade")
+
+        # Since the requirement hasn't been published yet, it won't show
+        # up in the list of requirements.
+        req_status = api.get_credit_requirement_status(self.course_key, "bob", namespace="grade", name="grade")
+        self.assertEqual(req_status, [])
+
+        # Now add the requirements, simulating what happens when a course is published.
+        requirements = [
+            {
+                "namespace": "grade",
+                "name": "grade",
+                "display_name": "Grade",
+                "criteria": {
+                    "min_grade": 0.8
+                }
+            },
+            {
+                "namespace": "reverification",
+                "name": "i4x://edX/DemoX/edx-reverification-block/assessment_uuid",
+                "display_name": "Assessment 1",
+                "criteria": {}
+            }
+        ]
+        api.set_credit_requirements(self.course_key, requirements)
+
+        # The user should not have satisfied the requirements, since they weren't
+        # in effect when the user completed the requirement
+        req_status = api.get_credit_requirement_status(self.course_key, "bob")
+        self.assertEqual(len(req_status), 2)
+        self.assertEqual(req_status[0]["status"], None)
+        self.assertEqual(req_status[0]["status"], None)
+
+        # The user should *not* have satisfied the reverification requirement
+        req_status = api.get_credit_requirement_status(
+            self.course_key,
+            "bob",
+            namespace=requirements[1]["namespace"],
+            name=requirements[1]["name"]
         )
-        status = CreditRequirementStatus.objects.get(username="staff", requirement=course_requirement)
-        self.assertEqual(status.requirement.namespace, requirement['namespace'])
-        self.assertEqual(status.status, "failed")
+        self.assertEqual(len(req_status), 1)
+        self.assertEqual(req_status[0]["status"], None)
 
 
 @ddt.ddt