From bcd762ef91c119f9cac7e41d6f70183888ccbe68 Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Fri, 6 May 2016 02:16:09 -0400 Subject: [PATCH] Updated credit requirements logic Credit eligibility can be updated up to the point that the course is closed or the student submits a request for credit. Credit eligibility cannot be removed. ECOM-4379 --- .../core/djangoapps/credit/api/eligibility.py | 33 +++++++------ openedx/core/djangoapps/credit/signals.py | 46 +++++++++++++++---- .../core/djangoapps/credit/tests/test_api.py | 21 +++++++-- .../djangoapps/credit/tests/test_signals.py | 22 +++++++-- 4 files changed, 88 insertions(+), 34 deletions(-) diff --git a/openedx/core/djangoapps/credit/api/eligibility.py b/openedx/core/djangoapps/credit/api/eligibility.py index 9ad0abff9e..e086b3eb74 100644 --- a/openedx/core/djangoapps/credit/api/eligibility.py +++ b/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'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" 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'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: diff --git a/openedx/core/djangoapps/credit/signals.py b/openedx/core/djangoapps/credit/signals.py index 30f40a0f2f..92973ad1f0 100644 --- a/openedx/core/djangoapps/credit/signals.py +++ b/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__) @@ -80,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']} + 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="satisfied", reason=reason_dict - ) - elif deadline and deadline < timezone.now(): - api.set_credit_requirement_status( - username, course_id, 'grade', 'grade', status="failed", reason={} + username, course_id, 'grade', 'grade', status=status, reason=reason ) diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index 02c9be41a2..755e95467e 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/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 @@ -380,7 +381,7 @@ class CreditRequirementApiTests(CreditApiTestBase): def test_set_credit_requirement_status(self): username = "staff" - self.add_credit_course() + credit_course = self.add_credit_course() requirements = [ { "namespace": "grade", @@ -405,6 +406,16 @@ class CreditRequirementApiTests(CreditApiTestBase): # Initially, the status should be None 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(username, self.course_key, "grade", "grade") self.assert_grade_requirement_status('satisfied', 0) @@ -532,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, @@ -544,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, @@ -598,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, diff --git a/openedx/core/djangoapps/credit/tests/test_signals.py b/openedx/core/djangoapps/credit/tests/test_signals.py index 0dc8102b5a..1121b27eed 100644 --- a/openedx/core/djangoapps/credit/tests/test_signals.py +++ b/openedx/core/djangoapps/credit/tests/test_signals.py @@ -70,20 +70,32 @@ class TestMinGradedRequirementStatus(ModuleStoreTestCase): """ 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, due_date): - """Test with valid grades. Deadline date does not effect in case - of valid grade. - """ + """Test with valid grades submitted before deadline""" self.assert_requirement_status(grade, due_date, '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), (0.51, None),