From 4d0efa289312e86bf52a62761f8d990d25f1da5a Mon Sep 17 00:00:00 2001 From: Muhammad Shoaib Date: Mon, 31 Aug 2015 16:37:09 +0500 Subject: [PATCH] PHX-122 Fixed the issue when deleting a proctored exam attempt does not remove the record in the CreditRequirementStatus table --- .../core/djangoapps/credit/api/eligibility.py | 53 +++++++++ openedx/core/djangoapps/credit/models.py | 18 +++ openedx/core/djangoapps/credit/services.py | 51 ++++++++ .../core/djangoapps/credit/tests/test_api.py | 57 +++++++++ .../djangoapps/credit/tests/test_services.py | 111 ++++++++++++++++++ 5 files changed, 290 insertions(+) diff --git a/openedx/core/djangoapps/credit/api/eligibility.py b/openedx/core/djangoapps/credit/api/eligibility.py index 91aa3f79e4..758617bad5 100644 --- a/openedx/core/djangoapps/credit/api/eligibility.py +++ b/openedx/core/djangoapps/credit/api/eligibility.py @@ -284,6 +284,59 @@ def set_credit_requirement_status(username, course_key, req_namespace, req_name, log.error("Error sending email") +# pylint: disable=invalid-name +def remove_credit_requirement_status(username, course_key, req_namespace, req_name): + """ + Remove the user's requirement status. + + This will remove the record from the credit requirement status table. + The user will still be eligible for the credit in a course. + + Args: + 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) + + Example: + >>> remove_credit_requirement_status( + "staff", + CourseKey.from_string("course-v1-edX-DemoX-1T2015"), + "reverification", + "i4x://edX/DemoX/edx-reverification-block/assessment_uuid". + ) + + """ + + # Find the requirement we're trying to remove + req_to_remove = CreditRequirement.get_course_requirements(course_key, namespace=req_namespace, name=req_name) + + # If we can't find the requirement, then the most likely explanation + # is that there was a lag removing the credit requirements after the course + # was published. We *could* attempt to remove 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 removing the requirement and log an error. + if req_to_remove is None: + log.error( + ( + u'Could not remove credit requirement in course "%s" ' + u'with namespace "%s" and name "%s" ' + u'because the requirement does not exist. ' + ), + unicode(course_key), req_namespace, req_name + ) + return + + # Remove the requirement status + CreditRequirementStatus.remove_requirement_status( + username, req_to_remove + ) + + def get_credit_requirement_status(course_key, username, namespace=None, name=None): """ Retrieve the user's status for each credit requirement in the course. diff --git a/openedx/core/djangoapps/credit/models.py b/openedx/core/djangoapps/credit/models.py index 45d2ab812f..b77505daec 100644 --- a/openedx/core/djangoapps/credit/models.py +++ b/openedx/core/djangoapps/credit/models.py @@ -467,6 +467,24 @@ class CreditRequirementStatus(TimeStampedModel): requirement_status.reason = reason if reason else {} requirement_status.save() + @classmethod + @transaction.commit_on_success + def remove_requirement_status(cls, username, requirement): + """ + Remove credit requirement status for given username. + + Args: + username(str): Username of the user + requirement(CreditRequirement): 'CreditRequirement' object + """ + + try: + requirement_status = cls.objects.get(username=username, requirement=requirement) + requirement_status.delete() + except cls.DoesNotExist: + log.exception(u'The requirement status does not exist against the username %s.', username) + return + class CreditEligibility(TimeStampedModel): """ diff --git a/openedx/core/djangoapps/credit/services.py b/openedx/core/djangoapps/credit/services.py index 67e80d1610..423f3e4b25 100644 --- a/openedx/core/djangoapps/credit/services.py +++ b/openedx/core/djangoapps/credit/services.py @@ -159,3 +159,54 @@ class CreditService(object): status, reason ) + + def remove_credit_requirement_status(self, user_id, course_key_or_id, req_namespace, req_name): + """ + A simple wrapper around the method of the same name in + api.eligibility.py. The only difference is that a user_id + is passed in. + + For more information, see documentation on this method name + in api.eligibility.py + """ + + # This seems to need to be here otherwise we get + # circular references when starting up the app + from openedx.core.djangoapps.credit.api.eligibility import ( + is_credit_course, + remove_credit_requirement_status as api_remove_credit_requirement_status + ) + + course_key = _get_course_key(course_key_or_id) + + # quick exit, if course is not credit enabled + if not is_credit_course(course_key): + return + + # always log any deleted activity to the credit requirements + # table. This will be to help debug any issues that might + # arise in production + log_msg = ( + 'remove_credit_requirement_status was called with ' + 'user_id={user_id}, course_key_or_id={course_key_or_id} ' + 'req_namespace={req_namespace}, req_name={req_name}, '.format( + user_id=user_id, + course_key_or_id=course_key_or_id, + req_namespace=req_namespace, + req_name=req_name + ) + ) + log.info(log_msg) + + # need to get user_name from the user object + try: + user = User.objects.get(id=user_id) + except ObjectDoesNotExist: + return None + + api_remove_credit_requirement_status( + user.username, + course_key, + req_namespace, + req_name + ) diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index b86b74fff2..2a5ee8af1c 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/openedx/core/djangoapps/credit/tests/test_api.py @@ -318,6 +318,63 @@ class CreditRequirementApiTests(CreditApiTestBase): req_status = api.get_credit_requirement_status(self.course_key, "staff", namespace="grade", name="grade") self.assertEqual(req_status[0]["status"], "failed") + def test_remove_credit_requirement_status(self): + 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) + course_requirements = api.get_credit_requirements(self.course_key) + self.assertEqual(len(course_requirements), 2) + + # before setting credit_requirement_status + api.remove_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.assertIsNone(req_status[0]["status"]) + self.assertIsNone(req_status[0]["status_date"]) + self.assertIsNone(req_status[0]["reason"]) + + # 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(len(req_status), 1) + self.assertEqual(req_status[0]["status"], "satisfied") + + # remove the credit requirement status and check that it's actually removed + api.remove_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.assertIsNone(req_status[0]["status"]) + self.assertIsNone(req_status[0]["status_date"]) + self.assertIsNone(req_status[0]["reason"]) + + def test_remove_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 removed + # after the course is published. + api.remove_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(len(req_status), 0) + def test_satisfy_all_requirements(self): """ Test the credit requirements, eligibility notification, email content caching for a credit course. diff --git a/openedx/core/djangoapps/credit/tests/test_services.py b/openedx/core/djangoapps/credit/tests/test_services.py index 8cc471eba5..c250ce0074 100644 --- a/openedx/core/djangoapps/credit/tests/test_services.py +++ b/openedx/core/djangoapps/credit/tests/test_services.py @@ -121,6 +121,117 @@ class CreditServiceTests(ModuleStoreTestCase): self.assertEqual(credit_state['credit_requirement_status'][0]['name'], 'grade') self.assertEqual(credit_state['credit_requirement_status'][0]['status'], 'satisfied') + def test_remove_credit_requirement_status(self): + """ + Happy path when deleting the requirement status. + """ + self.assertTrue(self.service.is_credit_course(self.course.id)) + + CourseEnrollment.enroll(self.user, self.course.id) + + # set course requirements + set_credit_requirements( + self.course.id, + [ + { + "namespace": "grade", + "name": "grade", + "display_name": "Grade", + "criteria": { + "min_grade": 0.8 + }, + }, + ] + ) + + # mark the grade as satisfied + self.service.set_credit_requirement_status( + self.user.id, + self.course.id, + 'grade', + 'grade' + ) + + # now the status should be "satisfied" when looking at the credit_requirement_status list + credit_state = self.service.get_credit_state(self.user.id, self.course.id) + self.assertEqual(credit_state['credit_requirement_status'][0]['status'], "satisfied") + + # remove the requirement status. + self.service.remove_credit_requirement_status( + self.user.id, + self.course.id, + 'grade', + 'grade' + ) + + # now the status should be None when looking at the credit_requirement_status list + credit_state = self.service.get_credit_state(self.user.id, self.course.id) + self.assertEqual(credit_state['credit_requirement_status'][0]['status'], None) + + def test_invalid_user(self): + """ + Try removing requirement status with a invalid user_id + """ + + # set course requirements + set_credit_requirements( + self.course.id, + [ + { + "namespace": "grade", + "name": "grade", + "display_name": "Grade", + "criteria": { + "min_grade": 0.8 + }, + }, + ] + ) + + # mark the grade as satisfied + retval = self.service.set_credit_requirement_status( + self.user.id, + self.course.id, + 'grade', + 'grade' + ) + self.assertIsNone(retval) + + # remove the requirement status with the invalid user id + retval = self.service.remove_credit_requirement_status( + 0, + self.course.id, + 'grade', + 'grade' + ) + self.assertIsNone(retval) + + def test_remove_status_non_credit(self): + """ + assert that we can still try to update a credit status but return quickly if + a course is not credit eligible + """ + + no_credit_course = CourseFactory.create(org='NoCredit', number='NoCredit', display_name='Demo_Course') + + self.assertFalse(self.service.is_credit_course(no_credit_course.id)) + + CourseEnrollment.enroll(self.user, no_credit_course.id) + + # this should be a no-op + self.service.remove_credit_requirement_status( + self.user.id, + no_credit_course.id, + 'grade', + 'grade' + ) + + credit_state = self.service.get_credit_state(self.user.id, no_credit_course.id) + + self.assertIsNotNone(credit_state) + self.assertFalse(credit_state['is_credit_course']) + self.assertEqual(len(credit_state['credit_requirement_status']), 0) + def test_course_name(self): """ Make sure we can get back the optional course name