From b23169560f415c6eb197b2fd6b574ab0fe46984d Mon Sep 17 00:00:00 2001 From: "Albert (AJ) St. Aubin" Date: Thu, 24 Jun 2021 08:18:17 -0400 Subject: [PATCH] fix: Corrects issue with refund logic and certificates. [MICROBA-1307] Before this change a user would not be auto refunded if they had a certificate in a course with any status. This had unintended consequences. This change updates the logic to only block auto refund for statuses that we do not want to refund on such as downloadable. --- common/djangoapps/entitlements/models.py | 10 ++++--- .../entitlements/tests/test_models.py | 9 +++++-- common/djangoapps/student/models.py | 26 +++++++++--------- .../djangoapps/student/tests/test_refunds.py | 16 ++++++----- lms/djangoapps/certificates/api.py | 24 ++++++++++------- lms/djangoapps/certificates/data.py | 15 +++++++++++ lms/djangoapps/certificates/tests/test_api.py | 13 +++++++++ .../certificates/tests/test_data.py | 27 +++++++++++++++++++ openedx/core/djangoapps/programs/utils.py | 5 ++-- .../tests/views/test_learner_profile.py | 4 +-- 10 files changed, 109 insertions(+), 40 deletions(-) create mode 100644 lms/djangoapps/certificates/tests/test_data.py diff --git a/common/djangoapps/entitlements/models.py b/common/djangoapps/entitlements/models.py index eeb028066a..5090832637 100644 --- a/common/djangoapps/entitlements/models.py +++ b/common/djangoapps/entitlements/models.py @@ -18,7 +18,8 @@ from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.entitlements.utils import is_course_run_entitlement_fulfillable from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentException from common.djangoapps.util.date_utils import strftime_localized -from lms.djangoapps.certificates.models import GeneratedCertificate +from lms.djangoapps.certificates import api as certificates_api +from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.commerce.utils import refund_entitlement from openedx.core.djangoapps.catalog.utils import get_course_uuid_for_course from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -95,8 +96,11 @@ class CourseEntitlementPolicy(models.Model): return False if entitlement.enrollment_course_run: - if GeneratedCertificate.certificate_for_student( - entitlement.user_id, entitlement.enrollment_course_run.course_id) is not None: + certificate = certificates_api.get_certificate_for_user_id( + entitlement.user, + entitlement.enrollment_course_run.course_id + ) + if certificate and not CertificateStatuses.is_refundable_status(certificate.status): return False # This is >= because a days_until_expiration 0 means that the expiration day has not fully passed yet diff --git a/common/djangoapps/entitlements/tests/test_models.py b/common/djangoapps/entitlements/tests/test_models.py index 3fb1d8e3b8..da15c09e26 100644 --- a/common/djangoapps/entitlements/tests/test_models.py +++ b/common/djangoapps/entitlements/tests/test_models.py @@ -189,14 +189,14 @@ class TestModels(TestCase): def test_is_entitlement_regainable(self): """ - Test that the entitlement is not expired when created now, and is expired when created20 days + Test that the entitlement is not expired when created now, and is expired when created 20 days ago with a policy that sets the expiration period to 14 days """ entitlement = CourseEntitlementFactory.create(enrollment_course_run=self.enrollment) assert entitlement.is_entitlement_regainable() is True # Create and associate a GeneratedCertificate for a user and course and make sure it isn't regainable - GeneratedCertificateFactory( + certificate = GeneratedCertificateFactory( user=entitlement.user, course_id=entitlement.enrollment_course_run.course_id, mode=MODES.verified, @@ -205,6 +205,11 @@ class TestModels(TestCase): assert entitlement.is_entitlement_regainable() is False + certificate.status = CertificateStatuses.notpassing + certificate.save() + + assert entitlement.is_entitlement_regainable() is True + # Create a date 20 days in the past (greater than the policy expire period of 14 days) # and apply it to both the entitlement and the course past_datetime = now() - timedelta(days=20) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 9b20142039..c529df7c1d 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -66,7 +66,7 @@ from common.djangoapps.student.signals import ENROLL_STATUS_CHANGE, ENROLLMENT_T from common.djangoapps.track import contexts, segment from common.djangoapps.util.model_utils import emit_field_changed_events, get_changed_fields_dict from common.djangoapps.util.query import use_read_replica_if_available -from lms.djangoapps.certificates.models import GeneratedCertificate +from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.courseware.models import ( CourseDynamicUpgradeDeadlineConfiguration, DynamicUpgradeDeadlineConfiguration, @@ -1835,7 +1835,7 @@ class CourseEnrollment(models.Model): """Changes this `CourseEnrollment` record's mode to `mode`. Saves immediately.""" self.update_enrollment(mode=mode) - def refundable(self, user_already_has_certs_for=None): + def refundable(self): """ For paid/verified certificates, students may always receive a refund if this CourseEnrollment's `can_refund` attribute is not `None` (that @@ -1848,11 +1848,6 @@ class CourseEnrollment(models.Model): * 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. """ @@ -1865,13 +1860,16 @@ class CourseEnrollment(models.Model): if getattr(self, 'can_refund', None) is not None: return True - # If the student has already been given a certificate they should not be refunded - 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 + # Due to circular import issues this import was placed close to usage. To move this to the + # top of the file would require a large scale refactor of the refund code. + import lms.djangoapps.certificates.api + # If the student has already been given a certificate in a non refundable status they should not be refunded + certificate = lms.djangoapps.certificates.api.get_certificate_for_user_id( + self.user, + self.course_id + ) + if certificate and not CertificateStatuses.is_refundable_status(certificate.status): + 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 67fc4f64da..ccc1a27fc3 100644 --- a/common/djangoapps/student/tests/test_refunds.py +++ b/common/djangoapps/student/tests/test_refunds.py @@ -25,7 +25,6 @@ from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentAttribute, EnrollmentRefundConfiguration from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.certificates.data import CertificateStatuses -from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory from openedx.core.djangoapps.commerce.utils import ECOMMERCE_DATE_FORMAT from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -88,7 +87,7 @@ class RefundableTest(SharedModuleStoreTestCase): assert self.enrollment.refundable() - GeneratedCertificateFactory.create( + certificate = GeneratedCertificateFactory.create( user=self.user, course_id=self.course.id, status=CertificateStatuses.downloadable, @@ -96,15 +95,18 @@ class RefundableTest(SharedModuleStoreTestCase): ) assert not self.enrollment.refundable() - assert not self.enrollment.\ - refundable(user_already_has_certs_for=GeneratedCertificate.course_ids_with_certs_for_user(self.user)) + + # Certificates that are not in the PASSED_STATUSES should allow a refund + certificate.status = CertificateStatuses.notpassing + certificate.save() + + assert self.enrollment.refundable() # Assert that can_refund overrides this and allows refund + certificate.status = CertificateStatuses.downloadable + certificate.save() self.enrollment.can_refund = True assert self.enrollment.refundable() - assert self.enrollment.refundable( - user_already_has_certs_for=GeneratedCertificate.course_ids_with_certs_for_user(self.user) - ) @patch('common.djangoapps.student.models.CourseEnrollment.refund_cutoff_date') def test_refundable_with_cutoff_date(self, cutoff_date): diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 180f67b2ff..f0d3d3d51e 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -54,15 +54,6 @@ User = get_user_model() MODES = GeneratedCertificate.MODES -def is_passing_status(cert_status): - """ - Given the status of a certificate, return a boolean indicating whether - the student passed the course. This just proxies to the classmethod - defined in models.py - """ - return CertificateStatuses.is_passing_status(cert_status) - - def _format_certificate_for_user(username, cert): """ Helper function to serialize an user certificate. @@ -82,7 +73,7 @@ def _format_certificate_for_user(username, cert): "grade": cert.grade, "created": cert.created_date, "modified": cert.modified_date, - "is_passing": is_passing_status(cert.status), + "is_passing": CertificateStatuses.is_passing_status(cert.status), "is_pdf_certificate": bool(cert.download_url), "download_url": ( cert.download_url or get_certificate_url(cert.user.id, cert.course_id, uuid=cert.verify_uuid, @@ -154,6 +145,19 @@ def get_certificate_for_user(username, course_key, format_results=True): return cert +def get_certificate_for_user_id(user, course_id): + """ + Retrieve certificate information for a user in a specific course. + + Arguments: + user (User): A Django User. + course_id (CourseKey): Course ID + Returns: + A GeneratedCertificate object. + """ + return GeneratedCertificate.certificate_for_student(user, course_id) + + def get_certificates_for_user_by_course_keys(user, course_keys): """ Retrieve certificate information for a particular user for a set of courses. diff --git a/lms/djangoapps/certificates/data.py b/lms/djangoapps/certificates/data.py index 5d6381d55f..f8c670fffb 100644 --- a/lms/djangoapps/certificates/data.py +++ b/lms/djangoapps/certificates/data.py @@ -58,6 +58,7 @@ class CertificateStatuses: } PASSED_STATUSES = (downloadable, generating) + NON_REFUNDABLE_STATUSES = (downloadable, generating, unavailable) @classmethod def is_passing_status(cls, status): @@ -66,3 +67,17 @@ class CertificateStatuses: the student passed the course. """ return status in cls.PASSED_STATUSES + + @classmethod + def is_refundable_status(cls, status): + """ + Given the status of a certificate, check to see if that certificate status can + be refunded. + + Arguments: + status (str): The status of the certificate that you are checking + + Returns: + bool: True if the status is refundable. + """ + return status not in cls.NON_REFUNDABLE_STATUSES diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index bf737c1881..b3d18f89cb 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -44,6 +44,7 @@ from lms.djangoapps.certificates.api import ( get_allowlisted_users, get_certificate_footer_context, get_certificate_for_user, + get_certificate_for_user_id, get_certificate_header_context, get_certificate_invalidation_entry, get_certificate_url, @@ -429,6 +430,18 @@ class CertificateGetTests(SharedModuleStoreTestCase): assert cert['is_passing'] is True assert cert['download_url'] == 'www.google.com' + def test_get_certificate_for_user_id(self): + """ + Test to get a certificate for a user id for a specific course. + """ + cert = get_certificate_for_user_id(self.student, self.web_cert_course.id) + + assert cert is not None + assert cert.course_id == self.web_cert_course.id + assert cert.mode == CourseMode.VERIFIED + assert cert.status == CertificateStatuses.downloadable + assert cert.grade == '0.88' + def test_get_certificates_for_user(self): """ Test to get all the certificates for a user diff --git a/lms/djangoapps/certificates/tests/test_data.py b/lms/djangoapps/certificates/tests/test_data.py new file mode 100644 index 0000000000..828633e812 --- /dev/null +++ b/lms/djangoapps/certificates/tests/test_data.py @@ -0,0 +1,27 @@ +"""Tests for the certificates Python Data Class. """ +from django.test import TestCase +from lms.djangoapps.certificates.data import CertificateStatuses + + +class CertificateStatusAPITests(TestCase): + """ + Test the APIs related to certificate status. + """ + + def test_is_refundable_status(self): + """ + Test is a certificate has a refundable status. + """ + assert not CertificateStatuses.is_refundable_status(CertificateStatuses.downloadable) + assert CertificateStatuses.is_refundable_status(CertificateStatuses.notpassing) + + def test_is_passing_status(self): + """ + Test is a certificate has a refundable status. + """ + assert not CertificateStatuses.is_passing_status( + CertificateStatuses.notpassing + ) + assert CertificateStatuses.is_passing_status( + CertificateStatuses.downloadable + ) diff --git a/openedx/core/djangoapps/programs/utils.py b/openedx/core/djangoapps/programs/utils.py index 62ac9fb314..133c05e19a 100644 --- a/openedx/core/djangoapps/programs/utils.py +++ b/openedx/core/djangoapps/programs/utils.py @@ -25,6 +25,7 @@ from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.entitlements.api import get_active_entitlement_list_for_user from common.djangoapps.entitlements.models import CourseEntitlement from lms.djangoapps.certificates import api as certificate_api +from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.commerce.utils import EcommerceService from openedx.core.djangoapps.catalog.api import get_programs_by_type @@ -328,7 +329,7 @@ class ProgramProgressMeter: modes_match = course_run_mode == certificate_mode # Grab the available date and keep it if it's the earliest one for this catalog course. - if modes_match and certificate_api.is_passing_status(certificate.status): + if modes_match and CertificateStatuses.is_passing_status(certificate.status): course_overview = CourseOverview.get_from_id(key) available_date = available_date_for_certificate(course_overview, certificate) earliest_course_run_date = min( @@ -447,7 +448,7 @@ class ProgramProgressMeter: except CourseOverview.DoesNotExist: may_certify = True if ( - certificate_api.is_passing_status(certificate['status']) + CertificateStatuses.is_passing_status(certificate['status']) and may_certify ): completed_runs.append(course_data) diff --git a/openedx/features/learner_profile/tests/views/test_learner_profile.py b/openedx/features/learner_profile/tests/views/test_learner_profile.py index 63299a7695..97d42a5cb9 100644 --- a/openedx/features/learner_profile/tests/views/test_learner_profile.py +++ b/openedx/features/learner_profile/tests/views/test_learner_profile.py @@ -14,7 +14,7 @@ from opaque_keys.edx.locator import CourseLocator from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from common.djangoapps.util.testing import UrlResetMixin -from lms.djangoapps.certificates.api import is_passing_status +from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory from lms.envs.test import CREDENTIALS_PUBLIC_SERVICE_URL from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -171,7 +171,7 @@ class LearnerProfileViewTest(SiteMixin, UrlResetMixin, ModuleStoreTestCase): cert.save() # Ensure that this test is actually using both passing and non-passing certs. - assert is_passing_status(cert.status) == is_passed_status + assert CertificateStatuses.is_passing_status(cert.status) == is_passed_status response = self.client.get(f'/u/{self.user.username}')