diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 162d800291..f2fe149564 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -128,6 +128,9 @@ class CourseMode(models.Model): # Modes that allow a student to earn credit with a university partner CREDIT_MODES = [CREDIT_MODE] + # Modes that are eligible to purchase credit + CREDIT_ELIGIBLE_MODES = [VERIFIED, PROFESSIONAL, NO_ID_PROFESSIONAL_MODE] + # Modes that are allowed to upsell UPSELL_TO_VERIFIED_MODES = [HONOR, AUDIT] @@ -488,6 +491,18 @@ class CourseMode(models.Model): """ return mode_slug in cls.VERIFIED_MODES + @classmethod + def is_credit_eligible_slug(cls, mode_slug): + """Check whether the given mode_slug is credit eligible or not. + + Args: + mode_slug(str): Mode Slug + + Returns: + bool: True iff the course mode slug is credit eligible else False. + """ + return mode_slug in cls.CREDIT_ELIGIBLE_MODES + @classmethod def is_credit_mode(cls, course_mode_tuple): """Check whether this is a credit mode. diff --git a/common/djangoapps/student/tests/test_credit.py b/common/djangoapps/student/tests/test_credit.py index bef6309fa5..bdf5412dbb 100644 --- a/common/djangoapps/student/tests/test_credit.py +++ b/common/djangoapps/student/tests/test_credit.py @@ -177,7 +177,7 @@ class CreditCourseDashboardTest(ModuleStoreTestCase): def _make_eligible(self): """Make the user eligible for credit in the course. """ credit_api.set_credit_requirement_status( - self.USERNAME, + self.user, self.course.id, # pylint: disable=no-member "grade", "grade", status="satisfied", diff --git a/lms/djangoapps/courseware/tests/test_credit_requirements.py b/lms/djangoapps/courseware/tests/test_credit_requirements.py index a0bcff6155..fb3d241ad9 100644 --- a/lms/djangoapps/courseware/tests/test_credit_requirements.py +++ b/lms/djangoapps/courseware/tests/test_credit_requirements.py @@ -97,16 +97,19 @@ class ProgressPageCreditRequirementsTest(SharedModuleStoreTestCase): ) def test_credit_requirements_eligible(self): - # Mark the user as eligible for all requirements + """ + Mark the user as eligible for all requirements. Requirements are only displayed + for credit and verified enrollments. + """ credit_api.set_credit_requirement_status( - self.user.username, self.course.id, + self.user, self.course.id, "grade", "grade", status="satisfied", reason={"final_grade": 0.95} ) credit_api.set_credit_requirement_status( - self.user.username, self.course.id, + self.user, self.course.id, "reverification", "midterm", status="satisfied", reason={} ) @@ -123,9 +126,12 @@ class ProgressPageCreditRequirementsTest(SharedModuleStoreTestCase): self.assertNotContains(response, "95%") def test_credit_requirements_not_eligible(self): - # Mark the user as having failed both requirements + """ + Mark the user as having failed both requirements. Requirements are only displayed + for credit and verified enrollments. + """ credit_api.set_credit_requirement_status( - self.user.username, self.course.id, + self.user, self.course.id, "reverification", "midterm", status="failed", reason={} ) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index f7cf6ef35f..40a0c6abf1 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -940,7 +940,6 @@ class TestProctoringRendering(SharedModuleStoreTestCase): Verifies gated content from the student view rendering of a sequence this is labeled as a proctored exam """ - usage_key = self._setup_test_data(enrollment_mode, is_practice_exam, attempt_status) # initialize some credit requirements, if so then specify @@ -966,7 +965,7 @@ class TestProctoringRendering(SharedModuleStoreTestCase): ) set_credit_requirement_status( - self.request.user.username, + self.request.user, self.course_key, 'reverification', 'ICRV1' @@ -1021,7 +1020,6 @@ class TestProctoringRendering(SharedModuleStoreTestCase): if attempt_status: create_exam_attempt(exam_id, self.request.user.id, taking_as_proctored=True) update_attempt_status(exam_id, self.request.user.id, attempt_status) - return usage_key def _find_url_name(self, toc, url_name): diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index eff45cb53d..78296d0312 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -2,6 +2,7 @@ """ Integration tests for submitting problem responses and getting grades. """ +import ddt import json import os from textwrap import dedent @@ -19,10 +20,11 @@ from capa.tests.response_xml_factory import ( CodeResponseXMLFactory, ) from lms.djangoapps.grades import course_grades, progress +from course_modes.models import CourseMode from courseware.models import StudentModule, BaseStudentModuleHistory from courseware.tests.helpers import LoginEnrollmentTestCase from lms.djangoapps.lms_xblock.runtime import quote_slashes -from student.models import anonymous_id_for_user +from student.models import anonymous_id_for_user, CourseEnrollment from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.partitions.partitions import Group, UserPartition @@ -304,6 +306,7 @@ class TestSubmittingProblems(ModuleStoreTestCase, LoginEnrollmentTestCase, Probl @attr(shard=3) +@ddt.ddt class TestCourseGrader(TestSubmittingProblems): """ Suite of tests for the course grader. @@ -315,7 +318,6 @@ class TestCourseGrader(TestSubmittingProblems): """ Set up a simple course for testing basic grading functionality. """ - grading_policy = { "GRADER": [{ "type": "Homework", @@ -646,20 +648,26 @@ class TestCourseGrader(TestSubmittingProblems): self.assertEqual(self.earned_hw_scores(), [1.0, 2.0, 2.0]) # Order matters self.assertEqual(self.score_for_hw('homework3'), [1.0, 1.0]) - def test_min_grade_credit_requirements_status(self): + @ddt.data( + *CourseMode.CREDIT_ELIGIBLE_MODES + ) + def test_min_grade_credit_requirements_status(self, mode): """ Test for credit course. If user passes minimum grade requirement then status will be updated as satisfied in requirement status table. """ self.basic_setup() + + # Enroll student in credit eligible mode. + # Note that we can't call self.enroll here since that goes through + # the Django student views, and does not update enrollment if it already exists. + CourseEnrollment.enroll(self.student_user, self.course.id, mode) + self.submit_question_answer('p1', {'2_1': 'Correct'}) self.submit_question_answer('p2', {'2_1': 'Correct'}) # Enable the course for credit - credit_course = CreditCourse.objects.create( - course_key=self.course.id, - enabled=True, - ) + CreditCourse.objects.create(course_key=self.course.id, enabled=True) # Configure a credit provider for the course CreditProvider.objects.create( diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index e1dcf21954..269fea808c 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/lms/djangoapps/grades/new/course_grade.py @@ -155,7 +155,7 @@ class CourseGrade(object): """ responses = GRADES_UPDATED.send_robust( sender=None, - username=self.student.username, + user=self.student, grade_summary=self.summary, course_key=self.course.id, deadline=self.course.end diff --git a/lms/djangoapps/verify_student/services.py b/lms/djangoapps/verify_student/services.py index 8caf7d5057..6758c0614b 100644 --- a/lms/djangoapps/verify_student/services.py +++ b/lms/djangoapps/verify_student/services.py @@ -112,7 +112,7 @@ class ReverificationService(object): # As a user skips the reverification it declines to fulfill the requirement so # requirement sets to declined. set_credit_requirement_status( - user.username, + user, course_key, 'reverification', checkpoint.checkpoint_location, diff --git a/lms/djangoapps/verify_student/tests/test_services.py b/lms/djangoapps/verify_student/tests/test_services.py index 7000fd462b..59c4016146 100644 --- a/lms/djangoapps/verify_student/tests/test_services.py +++ b/lms/djangoapps/verify_student/tests/test_services.py @@ -124,7 +124,10 @@ class TestReverificationService(ModuleStoreTestCase): 'skipped' ) - def test_declined_verification_on_skip(self): + @ddt.data( + *CourseMode.CREDIT_ELIGIBLE_MODES + ) + def test_declined_verification_on_skip(self, mode): """Test that status with value 'declined' is added in credit requirement status model when a user skip's an ICRV. """ @@ -135,6 +138,8 @@ class TestReverificationService(ModuleStoreTestCase): ) # Create credit course and set credit requirements. CreditCourse.objects.create(course_key=self.course_key, enabled=True) + self.enrollment.update_enrollment(mode=mode) + set_credit_requirements( self.course_key, [ diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 12414985c0..4959f3dcf6 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -1245,7 +1245,7 @@ def _set_user_requirement_status(attempt, namespace, status, reason=None): if checkpoint is not None: try: set_credit_requirement_status( - attempt.user.username, + attempt.user, checkpoint.course_id, namespace, checkpoint.checkpoint_location, diff --git a/openedx/core/djangoapps/credit/api/eligibility.py b/openedx/core/djangoapps/credit/api/eligibility.py index e086b3eb74..8f37f8d29e 100644 --- a/openedx/core/djangoapps/credit/api/eligibility.py +++ b/openedx/core/djangoapps/credit/api/eligibility.py @@ -13,6 +13,9 @@ from openedx.core.djangoapps.credit.models import ( CreditCourse, CreditRequirement, CreditRequirementStatus, CreditEligibility, CreditRequest ) +from course_modes.models import CourseMode +from student.models import CourseEnrollment + # TODO: Cleanup this mess! ECOM-2908 log = logging.getLogger(__name__) @@ -196,7 +199,7 @@ def get_eligibilities_for_user(username, course_key=None): ] -def set_credit_requirement_status(username, course_key, req_namespace, req_name, status="satisfied", reason=None): +def set_credit_requirement_status(user, course_key, req_namespace, req_name, status="satisfied", reason=None): """ Update the user's requirement status. @@ -205,7 +208,7 @@ def set_credit_requirement_status(username, course_key, req_namespace, req_name, as eligible for credit in the course. Args: - username (str): Username of the user + user(User): User object to set credit requirement for. 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) @@ -225,22 +228,30 @@ def set_credit_requirement_status(username, course_key, req_namespace, req_name, ) """ + # Check whether user has credit eligible enrollment. + enrollment_mode, is_active = CourseEnrollment.enrollment_mode_for_user(user, course_key) + has_credit_eligible_enrollment = (CourseMode.is_credit_eligible_slug(enrollment_mode) and is_active) + + # Refuse to set status of requirement if the user enrollment is not credit eligible. + if not has_credit_eligible_enrollment: + return + # Do not allow students who have requested credit to change their eligibility - if CreditRequest.get_user_request_status(username, course_key): + if CreditRequest.get_user_request_status(user.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 + req_namespace, req_name, user.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) + eligible_before_update = CreditEligibility.is_user_eligible_for_credit(course_key, user.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 + req_namespace, req_name, user.username, course_key ) return @@ -269,22 +280,22 @@ def set_credit_requirement_status(username, course_key, req_namespace, req_name, 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 + unicode(course_key), req_namespace, req_name, user.username, status ) return # Update the requirement status CreditRequirementStatus.add_or_update_requirement_status( - username, req_to_update, status=status, reason=reason + user.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, 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) + is_eligible, eligibility_record_created = CreditEligibility.update_eligibility(reqs, user.username, course_key) if eligibility_record_created and is_eligible: try: - send_credit_notifications(username, course_key) + send_credit_notifications(user.username, course_key) except Exception: # pylint: disable=broad-except log.error("Error sending email") diff --git a/openedx/core/djangoapps/credit/services.py b/openedx/core/djangoapps/credit/services.py index 77e7e7fa89..e38db41f22 100644 --- a/openedx/core/djangoapps/credit/services.py +++ b/openedx/core/djangoapps/credit/services.py @@ -155,7 +155,7 @@ class CreditService(object): return None api_set_credit_requirement_status( - user.username, + user, course_key, req_namespace, req_name, diff --git a/openedx/core/djangoapps/credit/signals.py b/openedx/core/djangoapps/credit/signals.py index 92973ad1f0..a239589600 100644 --- a/openedx/core/djangoapps/credit/signals.py +++ b/openedx/core/djangoapps/credit/signals.py @@ -53,12 +53,12 @@ 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 +def listen_for_grade_calculation(sender, user, grade_summary, course_key, deadline, **kwargs): # pylint: disable=unused-argument """Receive 'MIN_GRADE_REQUIREMENT_STATUS' signal and update minimum grade requirement status. Args: sender: None - username(string): user name + user(User): User Model object grade_summary(dict): Dict containing output from the course grader course_key(CourseKey): The key for the course deadline(datetime): Course end date or None @@ -70,7 +70,6 @@ def listen_for_grade_calculation(sender, username, grade_summary, course_key, de # This needs to be imported here to avoid a circular dependency # that can cause syncdb to fail. from openedx.core.djangoapps.credit import api - course_id = CourseKey.from_string(unicode(course_key)) is_credit = api.is_credit_course(course_id) if is_credit: @@ -113,5 +112,5 @@ def listen_for_grade_calculation(sender, username, grade_summary, course_key, de # time to do so. if status and reason: api.set_credit_requirement_status( - username, course_id, 'grade', 'grade', status=status, reason=reason + user, 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 e90121d8db..b5e0386e04 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/openedx/core/djangoapps/credit/tests/test_api.py @@ -36,6 +36,8 @@ from openedx.core.djangoapps.credit.models import ( CreditEligibility, CreditRequest ) +from course_modes.models import CourseMode +from student.models import CourseEnrollment from student.tests.factories import UserFactory from util.date_utils import from_timestamp from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -177,6 +179,19 @@ class CreditApiTestBase(ModuleStoreTestCase): return credit_course + def create_and_enroll_user(self, username, password, course_id=None, mode=CourseMode.VERIFIED): + """ Create and enroll the user in the given course's and given mode.""" + if course_id is None: + course_id = self.course_key + + user = UserFactory.create(username=username, password=password) + self.enroll(user, course_id, mode) + return user + + def enroll(self, user, course_id, mode): + """Enroll user in given course and mode""" + return CourseEnrollment.enroll(user, course_id, mode=mode) + def _mock_ecommerce_courses_api(self, course_key, body, status=200): """ Mock GET requests to the ecommerce course API endpoint. """ httpretty.reset() @@ -330,14 +345,54 @@ class CreditRequirementApiTests(CreditApiTestBase): def test_is_user_eligible_for_credit(self): credit_course = self.add_credit_course() CreditEligibility.objects.create( - course=credit_course, username="staff" + course=credit_course, username=self.user.username ) - is_eligible = api.is_user_eligible_for_credit('staff', credit_course.course_key) + is_eligible = api.is_user_eligible_for_credit(self.user.username, credit_course.course_key) self.assertTrue(is_eligible) is_eligible = api.is_user_eligible_for_credit('abc', credit_course.course_key) self.assertFalse(is_eligible) + @ddt.data( + CourseMode.AUDIT, + CourseMode.HONOR, + CourseMode.CREDIT_MODE + ) + def test_user_eligibility_with_non_verified_enrollment(self, mode): + """ + Tests that user do not become credit eligible even after meeting the credit requirements. + + User can not become credit eligible if he does not has credit eligible enrollment in the course. + """ + self.add_credit_course() + + # Enroll user and verify his enrollment. + self.enroll(self.user, self.course_key, mode) + self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course_key)) + self.assertTrue(CourseEnrollment.enrollment_mode_for_user(self.user, self.course_key), (mode, True)) + + requirements = [ + { + "namespace": "grade", + "name": "grade", + "display_name": "Grade", + "criteria": { + "min_grade": 0.6 + }, + } + ] + # Set & verify course credit requirements. + api.set_credit_requirements(self.course_key, requirements) + requirements = api.get_credit_requirements(self.course_key) + self.assertEqual(len(requirements), 1) + + # Set the requirement to "satisfied" and check that they are not set for non-credit eligible enrollment. + api.set_credit_requirement_status(self.user, self.course_key, "grade", "grade", status='satisfied') + self.assert_grade_requirement_status(None, 0) + + # Verify user is not eligible for credit. + self.assertFalse(api.is_user_eligible_for_credit(self.user.username, self.course_key)) + def test_eligibility_expired(self): # Configure a credit eligibility that expired yesterday credit_course = self.add_credit_course() @@ -376,14 +431,23 @@ class CreditRequirementApiTests(CreditApiTestBase): 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") + req_status = api.get_credit_requirement_status(self.course_key, self.user, 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): - username = "staff" + @ddt.data( + *CourseMode.CREDIT_ELIGIBLE_MODES + ) + def test_set_credit_requirement_status(self, mode): + username = self.user.username credit_course = self.add_credit_course() + + # Enroll user and verify his enrollment. + self.enroll(self.user, self.course_key, mode) + self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course_key)) + self.assertTrue(CourseEnrollment.enrollment_mode_for_user(self.user, self.course_key), (mode, True)) + requirements = [ { "namespace": "grade", @@ -400,7 +464,6 @@ class CreditRequirementApiTests(CreditApiTestBase): "criteria": {}, } ] - api.set_credit_requirements(self.course_key, requirements) course_requirements = api.get_credit_requirements(self.course_key) self.assertEqual(len(course_requirements), 2) @@ -414,19 +477,19 @@ class CreditRequirementApiTests(CreditApiTestBase): provider=CreditProvider.objects.first(), username=username, ) - api.set_credit_requirement_status(username, self.course_key, "grade", "grade") + api.set_credit_requirement_status(self.user, 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") + api.set_credit_requirement_status(self.user, 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(username, self.course_key, "grade", "grade", status="failed") + api.set_credit_requirement_status(self.user, 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") + req_status = api.get_credit_requirement_status(self.course_key, username) self.assertEqual(req_status[0]["status"], "failed") self.assertEqual(req_status[0]["order"], 0) @@ -436,7 +499,7 @@ class CreditRequirementApiTests(CreditApiTestBase): # Set the requirement to "declined" and check that it's actually set api.set_credit_requirement_status( - username, self.course_key, + self.user, self.course_key, "reverification", "i4x://edX/DemoX/edx-reverification-block/assessment_uuid", status="declined" @@ -449,8 +512,13 @@ class CreditRequirementApiTests(CreditApiTestBase): ) self.assertEqual(req_status[0]["status"], "declined") - def test_remove_credit_requirement_status(self): + @ddt.data( + *CourseMode.CREDIT_ELIGIBLE_MODES + ) + def test_remove_credit_requirement_status(self, mode): self.add_credit_course() + self.enroll(self.user, self.course_key, mode) + username = self.user.username requirements = [ { "namespace": "grade", @@ -467,27 +535,26 @@ class CreditRequirementApiTests(CreditApiTestBase): "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") + api.remove_credit_requirement_status(username, self.course_key, "grade", "grade") + req_status = api.get_credit_requirement_status(self.course_key, username, 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") + api.set_credit_requirement_status(self.user, self.course_key, "grade", "grade") + req_status = api.get_credit_requirement_status(self.course_key, username, 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") + api.remove_credit_requirement_status(self.user.username, self.course_key, "grade", "grade") + req_status = api.get_credit_requirement_status(self.course_key, username, namespace="grade", name="grade") self.assertIsNone(req_status[0]["status"]) self.assertIsNone(req_status[0]["status_date"]) self.assertIsNone(req_status[0]["reason"]) @@ -522,6 +589,7 @@ class CreditRequirementApiTests(CreditApiTestBase): # Configure a course with two credit requirements self.add_credit_course() + user = self.create_and_enroll_user(username=self.USER_INFO['username'], password=self.USER_INFO['password']) CourseFactory.create(org='edX', number='DemoX', display_name='Demo_Course') requirements = [ @@ -542,31 +610,29 @@ class CreditRequirementApiTests(CreditApiTestBase): ] api.set_credit_requirements(self.course_key, requirements) - 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(12): + with self.assertNumQueries(13): api.set_credit_requirement_status( - user.username, + user, 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)) + self.assertFalse(api.is_user_eligible_for_credit(user.username, self.course_key)) # Satisfy the other requirement - with self.assertNumQueries(21): + with self.assertNumQueries(22): api.set_credit_requirement_status( - "bob", + user, self.course_key, requirements[1]["namespace"], requirements[1]["name"] ) # Now the user should be eligible - self.assertTrue(api.is_user_eligible_for_credit("bob", self.course_key)) + self.assertTrue(api.is_user_eligible_for_credit(user.username, self.course_key)) # Credit eligibility email should be sent self.assertEqual(len(mail.outbox), 1) @@ -611,9 +677,9 @@ 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(16): + with self.assertNumQueries(17): api.set_credit_requirement_status( - "bob", + user, self.course_key, requirements[1]["namespace"], requirements[1]["name"] @@ -629,26 +695,27 @@ class CreditRequirementApiTests(CreditApiTestBase): # The user should remain eligible even if the requirement status is later changed api.set_credit_requirement_status( - "bob", + user, self.course_key, requirements[0]["namespace"], requirements[0]["name"], status="failed" ) - self.assertTrue(api.is_user_eligible_for_credit("bob", self.course_key)) + self.assertTrue(api.is_user_eligible_for_credit(user.username, self.course_key)) def test_set_credit_requirement_status_req_not_configured(self): # Configure a credit course with no requirements + username = self.user.username 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") + api.set_credit_requirement_status(self.user, 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") + req_status = api.get_credit_requirement_status(self.course_key, username, namespace="grade", name="grade") self.assertEqual(req_status, []) # Now add the requirements, simulating what happens when a course is published. @@ -672,7 +739,7 @@ class CreditRequirementApiTests(CreditApiTestBase): # 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") + req_status = api.get_credit_requirement_status(self.course_key, username) self.assertEqual(len(req_status), 2) self.assertEqual(req_status[0]["status"], None) self.assertEqual(req_status[0]["status"], None) @@ -680,7 +747,7 @@ class CreditRequirementApiTests(CreditApiTestBase): # The user should *not* have satisfied the reverification requirement req_status = api.get_credit_requirement_status( self.course_key, - "bob", + username, namespace=requirements[1]["namespace"], name=requirements[1]["name"] ) @@ -713,6 +780,7 @@ class CreditRequirementApiTests(CreditApiTestBase): """ # Configure a course with two credit requirements self.add_credit_course() + user = self.create_and_enroll_user(username=self.USER_INFO['username'], password=self.USER_INFO['password']) CourseFactory.create(org='edX', number='DemoX', display_name='Demo_Course') requirements = [ { @@ -732,11 +800,9 @@ class CreditRequirementApiTests(CreditApiTestBase): ] api.set_credit_requirements(self.course_key, requirements) - user = UserFactory.create(username=self.USER_INFO['username'], password=self.USER_INFO['password']) - # Satisfy one of the requirements, but not the other api.set_credit_requirement_status( - user.username, + user, self.course_key, requirements[0]["namespace"], requirements[0]["name"] @@ -745,13 +811,13 @@ class CreditRequirementApiTests(CreditApiTestBase): with mock.patch('openedx.core.djangoapps.credit.email_utils.get_credit_provider_display_names') as mock_method: mock_method.return_value = providers_list api.set_credit_requirement_status( - "bob", + user, self.course_key, requirements[1]["namespace"], requirements[1]["name"] ) # Now the user should be eligible - self.assertTrue(api.is_user_eligible_for_credit("bob", self.course_key)) + self.assertTrue(api.is_user_eligible_for_credit(user.username, self.course_key)) # Credit eligibility email should be sent self.assertEqual(len(mail.outbox), 1) diff --git a/openedx/core/djangoapps/credit/tests/test_services.py b/openedx/core/djangoapps/credit/tests/test_services.py index 0575261a2a..60b0f83521 100644 --- a/openedx/core/djangoapps/credit/tests/test_services.py +++ b/openedx/core/djangoapps/credit/tests/test_services.py @@ -2,7 +2,9 @@ Tests for the Credit xBlock service """ +import ddt from nose.plugins.attrib import attr +from course_modes.models import CourseMode from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -15,6 +17,7 @@ from student.models import CourseEnrollment, UserProfile @attr(shard=2) +@ddt.ddt class CreditServiceTests(ModuleStoreTestCase): """ Tests for the Credit xBlock service @@ -28,14 +31,14 @@ class CreditServiceTests(ModuleStoreTestCase): self.credit_course = CreditCourse.objects.create(course_key=self.course.id, enabled=True) self.profile = UserProfile.objects.create(user_id=self.user.id, name='Foo Bar') - def enroll(self, course_id=None): + def enroll(self, course_id=None, mode=CourseMode.VERIFIED): """ - Enroll the test user in the given course's honor mode, or the test - course if not provided. + Enroll the test user in the given course's mode. Use course/mode if they are + provided. """ if course_id is None: course_id = self.course.id - return CourseEnrollment.enroll(self.user, course_id, mode='honor') + return CourseEnrollment.enroll(self.user, course_id, mode=mode) def test_user_not_found(self): """ @@ -127,7 +130,7 @@ class CreditServiceTests(ModuleStoreTestCase): self.assertIsNotNone(credit_state) self.assertTrue(credit_state['is_credit_course']) - self.assertEqual(credit_state['enrollment_mode'], 'honor') + self.assertEqual(credit_state['enrollment_mode'], 'verified') self.assertEqual(credit_state['profile_fullname'], 'Foo Bar') self.assertEqual(len(credit_state['credit_requirement_status']), 1) self.assertEqual(credit_state['credit_requirement_status'][0]['name'], 'grade') @@ -286,6 +289,48 @@ class CreditServiceTests(ModuleStoreTestCase): self.assertFalse(credit_state['is_credit_course']) self.assertEqual(len(credit_state['credit_requirement_status']), 0) + @ddt.data( + CourseMode.AUDIT, + CourseMode.HONOR, + CourseMode.CREDIT_MODE + ) + def test_set_status_non_verified_enrollment(self, mode): + """ + Test that we can still try to update a credit status but return quickly if + user has non-credit eligible enrollment. + """ + self.enroll(mode=mode) + + # set course requirements + set_credit_requirements( + self.course.id, + [ + { + "namespace": "grade", + "name": "grade", + "display_name": "Grade", + "criteria": { + "min_grade": 0.8 + }, + }, + ] + ) + + # this should be a no-op + self.service.set_credit_requirement_status( + self.user.id, + self.course.id, + 'grade', + 'grade' + ) + # Verify credit requirement status for user in the course should be None. + credit_state = self.service.get_credit_state(self.user.id, self.course.id) + self.assertIsNotNone(credit_state) + self.assertEqual(credit_state['enrollment_mode'], mode) + self.assertEqual(len(credit_state['credit_requirement_status']), 1) + self.assertIsNone(credit_state['credit_requirement_status'][0]['status']) + self.assertIsNone(credit_state['credit_requirement_status'][0]['status_date']) + def test_bad_user(self): """ Try setting requirements status with a bad user_id @@ -348,7 +393,7 @@ class CreditServiceTests(ModuleStoreTestCase): credit_state = self.service.get_credit_state(self.user.id, unicode(self.course.id)) self.assertIsNotNone(credit_state) - self.assertEqual(credit_state['enrollment_mode'], 'honor') + self.assertEqual(credit_state['enrollment_mode'], 'verified') self.assertEqual(credit_state['profile_fullname'], 'Foo Bar') self.assertEqual(len(credit_state['credit_requirement_status']), 1) self.assertEqual(credit_state['credit_requirement_status'][0]['name'], 'grade') diff --git a/openedx/core/djangoapps/credit/tests/test_signals.py b/openedx/core/djangoapps/credit/tests/test_signals.py index 376b8d8769..f3d55529a2 100644 --- a/openedx/core/djangoapps/credit/tests/test_signals.py +++ b/openedx/core/djangoapps/credit/tests/test_signals.py @@ -10,6 +10,8 @@ from unittest import skipUnless from django.conf import settings from django.test.client import RequestFactory from nose.plugins.attrib import attr +from course_modes.models import CourseMode +from student.models import CourseEnrollment from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -66,9 +68,12 @@ class TestMinGradedRequirementStatus(ModuleStoreTestCase): # Add a single credit requirement (final grade) set_credit_requirements(self.course.id, requirements) + # Enroll user in verified mode. + self.enrollment = CourseEnrollment.enroll(self.user, self.course.id, mode=CourseMode.VERIFIED) + 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) + listen_for_grade_calculation(None, self.user, {'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) @@ -109,3 +114,13 @@ class TestMinGradedRequirementStatus(ModuleStoreTestCase): def test_min_grade_requirement_failed_grade_expired_deadline(self): """Test with failed grades and deadline expire""" self.assert_requirement_status(0.22, self.EXPIRED_DUE_DATE, 'failed') + + @ddt.data( + CourseMode.AUDIT, + CourseMode.HONOR, + CourseMode.CREDIT_MODE + ) + def test_requirement_failed_for_non_verified_enrollment(self, mode): + """Test with valid grades submitted before deadline with non-verified enrollment.""" + self.enrollment.update_enrollment(mode, True) + self.assert_requirement_status(0.8, self.VALID_DUE_DATE, None) diff --git a/openedx/core/djangoapps/signals/signals.py b/openedx/core/djangoapps/signals/signals.py index a953ee1c3e..cfbf23969b 100644 --- a/openedx/core/djangoapps/signals/signals.py +++ b/openedx/core/djangoapps/signals/signals.py @@ -6,7 +6,7 @@ from django.dispatch import Signal # Signal that fires when a user is graded (in lms/grades/course_grades.py) -GRADES_UPDATED = Signal(providing_args=["username", "grade_summary", "course_key", "deadline"]) +GRADES_UPDATED = Signal(providing_args=["user", "grade_summary", "course_key", "deadline"]) # Signal that fires when a user is awarded a certificate in a course (in the certificates django app) # TODO: runtime coupling between apps will be reduced if this event is changed to carry a username