diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 228067a67c..f984ce10c7 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1531,10 +1531,26 @@ class CourseEnrollment(models.Model): """Changes this `CourseEnrollment` record's mode to `mode`. Saves immediately.""" self.update_enrollment(mode=mode) - def refundable(self): + def refundable(self, user_already_has_certs_for=None): """ - For paid/verified certificates, students may receive a refund if they have - a verified certificate and the deadline for refunds has not yet passed. + For paid/verified certificates, students may always receive a refund if + this CourseEnrollment's `can_refund` attribute is not `None` (that + overrides all other rules). + + If the `.can_refund` attribute is `None` or doesn't exist, then ALL of + the following must be true for this enrollment to be refundable: + + * The user does not have a certificate issued for this course. + * We are not past the refund cutoff date + * There exists a 'verified' CourseMode for this course. + + Arguments: + `user_already_has_certs_for` (set of `CourseKey`): + An optional param that is a set of `CourseKeys` that the user + has already been issued certificates in. + + Returns: + bool: Whether is CourseEnrollment can be refunded. """ # In order to support manual refunds past the deadline, set can_refund on this object. # On unenrolling, the "UNENROLL_DONE" signal calls CertificateItem.refund_cert_callback(), @@ -1545,8 +1561,12 @@ class CourseEnrollment(models.Model): return True # If the student has already been given a certificate they should not be refunded - if GeneratedCertificate.certificate_for_student(self.user, self.course_id) is not None: - return False + if user_already_has_certs_for is not None: + if self.course_id in user_already_has_certs_for: + return False + else: + if GeneratedCertificate.certificate_for_student(self.user, self.course_id) is not None: + return False # If it is after the refundable cutoff date they should not be refunded. refund_cutoff_date = self.refund_cutoff_date() diff --git a/common/djangoapps/student/tests/test_refunds.py b/common/djangoapps/student/tests/test_refunds.py index 4dd6ba2ecc..c9f937b7f2 100644 --- a/common/djangoapps/student/tests/test_refunds.py +++ b/common/djangoapps/student/tests/test_refunds.py @@ -21,7 +21,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # These imports refer to lms djangoapps. # Their testcases are only run under lms. -from certificates.models import CertificateStatuses # pylint: disable=import-error +from certificates.models import CertificateStatuses, GeneratedCertificate # pylint: disable=import-error from certificates.tests.factories import GeneratedCertificateFactory # pylint: disable=import-error from openedx.core.djangoapps.commerce.utils import ECOMMERCE_DATE_FORMAT @@ -106,10 +106,20 @@ class RefundableTest(SharedModuleStoreTestCase): ) self.assertFalse(self.enrollment.refundable()) + self.assertFalse( + self.enrollment.refundable( + user_already_has_certs_for=GeneratedCertificate.course_ids_with_certs_for_user(self.user) + ) + ) # Assert that can_refund overrides this and allows refund self.enrollment.can_refund = True self.assertTrue(self.enrollment.refundable()) + self.assertTrue( + self.enrollment.refundable( + user_already_has_certs_for=GeneratedCertificate.course_ids_with_certs_for_user(self.user) + ) + ) def test_refundable_with_cutoff_date(self): """ Assert enrollment is refundable before cutoff and not refundable after.""" diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index dd24d47d9b..02eaf539e4 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -61,7 +61,9 @@ from student.tasks import send_activation_email from lms.djangoapps.commerce.utils import EcommerceService # pylint: disable=import-error from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification # pylint: disable=import-error from bulk_email.models import Optout, BulkEmailFlag # pylint: disable=import-error -from certificates.models import CertificateStatuses, certificate_status_for_student +from certificates.models import ( # pylint: disable=import-error + CertificateStatuses, GeneratedCertificate, certificate_status_for_student +) from certificates.api import ( # pylint: disable=import-error get_certificate_url, has_html_certificates_enabled, @@ -716,9 +718,12 @@ def dashboard(request): statuses = ["approved", "denied", "pending", "must_reverify"] reverifications = reverification_info(statuses) + user_already_has_certs_for = GeneratedCertificate.course_ids_with_certs_for_user(request.user) show_refund_option_for = frozenset( enrollment.course_id for enrollment in course_enrollments - if enrollment.refundable() + if enrollment.refundable( + user_already_has_certs_for=user_already_has_certs_for + ) ) block_courses = frozenset( diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index a375b980a7..c8fecf9de9 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -260,6 +260,23 @@ class GeneratedCertificate(models.Model): return None + @classmethod + def course_ids_with_certs_for_user(cls, user): + """ + Return a set of CourseKeys for which the user has certificates. + + Sometimes we just want to check if a user has already been issued a + certificate for a given course (e.g. to test refund elibigility). + Instead of checking if `certificate_for_student` returns `None` on each + course_id individually, we instead just return a set of all CourseKeys + for which this student has certificates all at once. + """ + return { + cert.course_id + for cert + in cls.objects.filter(user=user).only('course_id') # pylint: disable=no-member + } + @classmethod def get_unique_statuses(cls, course_key=None, flat=False): """ diff --git a/lms/djangoapps/certificates/tests/tests.py b/lms/djangoapps/certificates/tests/tests.py index 457b9c53d5..3ca9c5ef9e 100644 --- a/lms/djangoapps/certificates/tests/tests.py +++ b/lms/djangoapps/certificates/tests/tests.py @@ -91,6 +91,42 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): certificate_info = certificate_info_for_user(student, course.id, grade, whitelisted) self.assertEqual(certificate_info, output) + def test_course_ids_with_certs_for_user(self): + # Create one user with certs and one without + student_no_certs = UserFactory() + student_with_certs = UserFactory() + student_with_certs.profile.allow_certificate = True + student_with_certs.profile.save() + + # Set up a couple of courses + course_1 = CourseFactory.create() + course_2 = CourseFactory.create() + + # Generate certificates + GeneratedCertificateFactory.create( + user=student_with_certs, + course_id=course_1.id, + status=CertificateStatuses.downloadable, + mode='honor' + ) + GeneratedCertificateFactory.create( + user=student_with_certs, + course_id=course_2.id, + status=CertificateStatuses.downloadable, + mode='honor' + ) + + # User with no certs should return an empty set. + self.assertSetEqual( + GeneratedCertificate.course_ids_with_certs_for_user(student_no_certs), + set() + ) + # User with certs should return a set with the two course_ids + self.assertSetEqual( + GeneratedCertificate.course_ids_with_certs_for_user(student_with_certs), + {course_1.id, course_2.id} + ) + @patch.dict(settings.FEATURES, {'ENABLE_PREREQUISITE_COURSES': True}) def test_course_milestone_collected(self): student = UserFactory()