diff --git a/openedx/core/djangoapps/credit/admin.py b/openedx/core/djangoapps/credit/admin.py index 3fd6d89943..054a399e04 100644 --- a/openedx/core/djangoapps/credit/admin.py +++ b/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) 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/models.py b/openedx/core/djangoapps/credit/models.py index c657e7118b..8bd3071a5e 100644 --- a/openedx/core/djangoapps/credit/models.py +++ b/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): """ diff --git a/openedx/core/djangoapps/credit/signals.py b/openedx/core/djangoapps/credit/signals.py index fbec5c9bd8..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__) @@ -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']} + 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 d68fe77b6a..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 @@ -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, diff --git a/openedx/core/djangoapps/credit/tests/test_signals.py b/openedx/core/djangoapps/credit/tests/test_signals.py index f2480e9d5b..1121b27eed 100644 --- a/openedx/core/djangoapps/credit/tests/test_signals.py +++ b/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')