From 87a146923201887527a6926674a13c75a200d815 Mon Sep 17 00:00:00 2001 From: Phillip Shiu Date: Wed, 27 Mar 2024 17:29:39 -0400 Subject: [PATCH 1/7] feat: waffle refunds for commerce-coordinator --- lms/djangoapps/commerce/utils.py | 93 +++++++++++++++++++++++++++++-- lms/djangoapps/commerce/waffle.py | 24 +++++++- lms/envs/common.py | 6 +- 3 files changed, 115 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/commerce/utils.py b/lms/djangoapps/commerce/utils.py index c5cb4315f635..6bdef1a250a6 100644 --- a/lms/djangoapps/commerce/utils.py +++ b/lms/djangoapps/commerce/utils.py @@ -14,7 +14,7 @@ from opaque_keys.edx.keys import CourseKey from common.djangoapps.course_modes.models import CourseMode -from lms.djangoapps.commerce.waffle import should_redirect_to_commerce_coordinator_checkout +from common.djangoapps.student.models import CourseEnrollmentAttribute from openedx.core.djangoapps.commerce.utils import ( get_ecommerce_api_base_url, get_ecommerce_api_client, @@ -24,6 +24,10 @@ from openedx.core.djangoapps.theming import helpers as theming_helpers from .models import CommerceConfiguration +from .waffle import ( + should_redirect_to_commerce_coordinator_checkout, + should_redirect_to_commerce_coordinator_refunds, +) log = logging.getLogger(__name__) @@ -252,6 +256,10 @@ def refund_seat(course_enrollment, change_mode=False): course_key_str = str(course_enrollment.course_id) enrollee = course_enrollment.user + if should_redirect_to_commerce_coordinator_refunds(): + if _refund_in_commerce_coordinator(course_enrollment, change_mode): + return + service_user = User.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) api_client = get_ecommerce_api_client(service_user) @@ -274,16 +282,91 @@ def refund_seat(course_enrollment, change_mode=False): mode=course_enrollment.mode, user=enrollee, ) - if change_mode and CourseMode.can_auto_enroll(course_id=CourseKey.from_string(course_key_str)): - course_enrollment.update_enrollment(mode=CourseMode.auto_enroll_mode(course_id=course_key_str), - is_active=False, skip_refund=True) - course_enrollment.save() + if change_mode: + _auto_enroll(course_enrollment) else: log.info('No refund opened for user [%s], course [%s]', enrollee.id, course_key_str) return refund_ids +def _refund_in_commerce_coordinator(course_enrollment, change_mode): + """ + Helper function to perform refund in Commerce Coordinator. + + Parameters: + course_enrollment (CourseEnrollment): the enrollment to refund. + change_mode (bool): whether the enrollment should be auto-enrolled into + the default course mode after refund. + + Returns: + bool: True if refund was performed. False if refund is not applicable + to Commerce Coordinator. + """ + enrollment_source_system = course_enrollment.get_order_attribute_value("source_system") + course_key_str = str(course_enrollment.course_id) + # Commerce Coordinator enrollments will have an orders.source_system enrollment attribute. + # Redirect to Coordinator only if the source_system is whitelisted as Coordinator's in settings. + if enrollment_source_system and enrollment_source_system in settings.COMMERCE_COORDINATOR_REFUND_SOURCE_SYSTEMS: + log.info('Redirecting refund to Commerce Coordinator for user [%s], course [%s]...', + course_enrollment.user_id, course_key_str) + # Re-use Ecommerce API client factory to build an API client for Commerce Coordinator.. + service_user = get_user_model().objects.get( + username=settings.COMMERCE_COORDINATOR_SERVICE_WORKER_USERNAME + ) + api_client = get_ecommerce_api_client(service_user) + refunds_url = urljoin( + settings.COMMERCE_COORDINATOR_URL_ROOT, + settings.COMMERCE_COORDINATOR_REFUND_PATH + ) + # Build request, raising exception if Coordinator returns non-200. + enrollment_attributes = CourseEnrollmentAttribute.get_enrollment_attributes(course_enrollment) + api_client.post( + refunds_url, + json={ + 'course_id': course_key_str, + 'username': course_enrollment.username, + 'enrollment_attributes': enrollment_attributes + } + ).raise_for_status() + log.info('Refund successfully sent to Commerce Coordinator for user [%s], course [%s].', + course_enrollment.user_id, course_key_str) + if change_mode: + _auto_enroll(course_enrollment) + return True + else: + log.info('Continuing refund without Commerce Coordinator redirect for user [%s], course [%s]...', + course_enrollment.user_id, course_key_str) + return False + + +def _auto_enroll(course_enrollment): + """ + Helper method to update an enrollment to a default course mode. + + Arguments: + course_enrollment (CourseEnrollment): The course_enrollment to update. + + Returns: + bool: True if auto-enroll is succesful. False if auto-enroll is not applicable. + """ + enrollment_course_id = course_enrollment.course_id + + if CourseMode.can_auto_enroll(course_id=enrollment_course_id): + auto_enroll_mode = CourseMode.auto_enroll_mode(course_id=enrollment_course_id) + course_enrollment.update_enrollment(mode=auto_enroll_mode, is_active=False, skip_refund=True) + course_enrollment.save() + log.info( + 'Auto-enrolled user [%s], course [%s] in mode [%s].', + course_enrollment.user_id, + enrollment_course_id, + auto_enroll_mode + ) + return True + else: + return False + + def _process_refund(refund_ids, api_client, mode, user, always_notify=False): """ Helper method to process a refund for a given course_product. This method assumes that the User has already diff --git a/lms/djangoapps/commerce/waffle.py b/lms/djangoapps/commerce/waffle.py index e1ee6f26456d..a36586a52d9b 100644 --- a/lms/djangoapps/commerce/waffle.py +++ b/lms/djangoapps/commerce/waffle.py @@ -20,10 +20,30 @@ __name__, ) +# .. toggle_name: commerce.transition_to_coordinator.refund +# .. toggle_implementation: WaffleFlag +# .. toggle_default: False +# .. toggle_description: Allows to redirect refunds to Commerce Coordinator API +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2024-03-26 +# .. toggle_target_removal_date: TBA +# .. toggle_tickets: SONIC-382 +# .. toggle_status: supported +ENABLE_TRANSITION_TO_COORDINATOR_REFUNDS = WaffleFlag( + f"{WAFFLE_FLAG_NAMESPACE}.transition_to_coordinator.refunds", + __name__, +) + def should_redirect_to_commerce_coordinator_checkout(): """ - Redirect learners to Commerce coordinator checkout. - + Redirect learners to Commerce Coordinator checkout. """ return ENABLE_TRANSITION_TO_COORDINATOR_CHECKOUT.is_enabled() + + +def should_redirect_to_commerce_coordinator_refunds(): + """ + Redirect learners to Commerce Coordinator refunds. + """ + return ENABLE_TRANSITION_TO_COORDINATOR_REFUNDS.is_enabled() diff --git a/lms/envs/common.py b/lms/envs/common.py index 3e651a915f49..47b1003bbd1f 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -4254,9 +4254,13 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring ECOMMERCE_API_SIGNING_KEY = 'SET-ME-PLEASE' # E-Commerce Commerce Coordinator Configuration -COMMERCE_COORDINATOR_URL_ROOT = 'http://localhost:8000' +COMMERCE_COORDINATOR_URL_ROOT = 'http://localhost:8140' +COMMERCE_COORDINATOR_REFUND_PATH = '/lms/refund/' +COMMERCE_COORDINATOR_REFUND_SOURCE_SYSTEMS = ('SET-ME-PLEASE',) +COMMERCE_COORDINATOR_SERVICE_WORKER_USERNAME = 'commerce_coordinator_worker' COORDINATOR_CHECKOUT_REDIRECT_PATH = '/lms/redirect/' + # Exam Service EXAMS_SERVICE_URL = 'http://localhost:18740/api/v1' From 5989d3e73622ae320ad482d0b063913e263ffa44 Mon Sep 17 00:00:00 2001 From: Phillip Shiu Date: Tue, 2 Apr 2024 04:49:42 -0400 Subject: [PATCH 2/7] chore: remove unused import --- lms/djangoapps/commerce/utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/djangoapps/commerce/utils.py b/lms/djangoapps/commerce/utils.py index 6bdef1a250a6..032174d574c5 100644 --- a/lms/djangoapps/commerce/utils.py +++ b/lms/djangoapps/commerce/utils.py @@ -11,7 +11,6 @@ from django.contrib.auth import get_user_model from django.urls import reverse from django.utils.translation import gettext as _ -from opaque_keys.edx.keys import CourseKey from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollmentAttribute From ed5270469a0582cbd2d2cb8e8f2150097143b8c8 Mon Sep 17 00:00:00 2001 From: Phillip Shiu Date: Tue, 2 Apr 2024 04:50:18 -0400 Subject: [PATCH 3/7] chore: bypass invalid-django-waffle-import This edx-lint check is to prevent: import waffle However, we are doing: import .waffle Where .waffle is importing correctly from edx_toggles instead of directly from the Django waffle library. See also: https://github.com/openedx/edx-lint/blob/be07c3739d73cd44d33d9d9dbf5f3119a906be9e/edx_lint/pylint/annotations_check.py#L379 --- lms/djangoapps/commerce/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/commerce/utils.py b/lms/djangoapps/commerce/utils.py index 032174d574c5..16768d341851 100644 --- a/lms/djangoapps/commerce/utils.py +++ b/lms/djangoapps/commerce/utils.py @@ -23,7 +23,7 @@ from openedx.core.djangoapps.theming import helpers as theming_helpers from .models import CommerceConfiguration -from .waffle import ( +from .waffle import ( # lint-amnesty, pylint: disable=invalid-django-waffle-import should_redirect_to_commerce_coordinator_checkout, should_redirect_to_commerce_coordinator_refunds, ) From 7535f27aa2c8731da7dfddc6cfe2e438cb3b9bc4 Mon Sep 17 00:00:00 2001 From: Phillip Shiu Date: Wed, 3 Apr 2024 07:21:15 -0400 Subject: [PATCH 4/7] fix: change default for COORDINATOR_CHECKOUT_REDIRECT_PATH --- lms/envs/common.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index 47b1003bbd1f..0241411df8fb 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -4258,8 +4258,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring COMMERCE_COORDINATOR_REFUND_PATH = '/lms/refund/' COMMERCE_COORDINATOR_REFUND_SOURCE_SYSTEMS = ('SET-ME-PLEASE',) COMMERCE_COORDINATOR_SERVICE_WORKER_USERNAME = 'commerce_coordinator_worker' -COORDINATOR_CHECKOUT_REDIRECT_PATH = '/lms/redirect/' - +COORDINATOR_CHECKOUT_REDIRECT_PATH = '/lms/payment_page_redirect/' # Exam Service EXAMS_SERVICE_URL = 'http://localhost:18740/api/v1' From fc3a8f7ebbe6006db498595cac8cfa6f84a0dba3 Mon Sep 17 00:00:00 2001 From: "Glenn R. Martin" Date: Mon, 8 Apr 2024 10:32:22 -0400 Subject: [PATCH 5/7] chore: exception handling --- lms/djangoapps/commerce/utils.py | 40 ++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/commerce/utils.py b/lms/djangoapps/commerce/utils.py index 16768d341851..1a2416231983 100644 --- a/lms/djangoapps/commerce/utils.py +++ b/lms/djangoapps/commerce/utils.py @@ -304,12 +304,15 @@ def _refund_in_commerce_coordinator(course_enrollment, change_mode): """ enrollment_source_system = course_enrollment.get_order_attribute_value("source_system") course_key_str = str(course_enrollment.course_id) + # Commerce Coordinator enrollments will have an orders.source_system enrollment attribute. - # Redirect to Coordinator only if the source_system is whitelisted as Coordinator's in settings. + # Redirect to Coordinator only if the source_system is safelisted as Coordinator's in settings. + if enrollment_source_system and enrollment_source_system in settings.COMMERCE_COORDINATOR_REFUND_SOURCE_SYSTEMS: log.info('Redirecting refund to Commerce Coordinator for user [%s], course [%s]...', course_enrollment.user_id, course_key_str) - # Re-use Ecommerce API client factory to build an API client for Commerce Coordinator.. + + # Re-use Ecommerce API client factory to build an API client for Commerce Coordinator... service_user = get_user_model().objects.get( username=settings.COMMERCE_COORDINATOR_SERVICE_WORKER_USERNAME ) @@ -318,22 +321,39 @@ def _refund_in_commerce_coordinator(course_enrollment, change_mode): settings.COMMERCE_COORDINATOR_URL_ROOT, settings.COMMERCE_COORDINATOR_REFUND_PATH ) + # Build request, raising exception if Coordinator returns non-200. enrollment_attributes = CourseEnrollmentAttribute.get_enrollment_attributes(course_enrollment) - api_client.post( - refunds_url, - json={ - 'course_id': course_key_str, - 'username': course_enrollment.username, - 'enrollment_attributes': enrollment_attributes - } - ).raise_for_status() + + try: + api_client.post( + refunds_url, + json={ + 'course_id': course_key_str, + 'username': course_enrollment.username, + 'enrollment_attributes': enrollment_attributes + } + ).raise_for_status() + + except Exception as exc: # pylint: disable=broad-except + # Catch any possible exceptions from the Commerce Coordinator service to ensure we fail gracefully + log.exception( + "Unexpected exception while attempting to refund user in Coordinator [%s], " + "course key [%s] message: [%s]", + course_enrollment.username, + course_key_str, + str(exc) + ) + return False + + # Refund was successfully sent to Commerce Coordinator log.info('Refund successfully sent to Commerce Coordinator for user [%s], course [%s].', course_enrollment.user_id, course_key_str) if change_mode: _auto_enroll(course_enrollment) return True else: + # Refund was not meant to be sent to Commerce Coordinator log.info('Continuing refund without Commerce Coordinator redirect for user [%s], course [%s]...', course_enrollment.user_id, course_key_str) return False From 9e56c1d04b7c09193dff0deae69fd3f89fdfb4c0 Mon Sep 17 00:00:00 2001 From: "Glenn R. Martin" Date: Tue, 9 Apr 2024 11:09:49 -0400 Subject: [PATCH 6/7] fix: broken lms test for CC switching in Ecommerce Service --- lms/djangoapps/commerce/tests/test_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/commerce/tests/test_utils.py b/lms/djangoapps/commerce/tests/test_utils.py index 96c5c1e2c35c..6c793433dff6 100644 --- a/lms/djangoapps/commerce/tests/test_utils.py +++ b/lms/djangoapps/commerce/tests/test_utils.py @@ -200,12 +200,12 @@ def test_get_add_to_basket_url(self, coordinator_flag_active): result = ecommerce_service.get_add_to_basket_url() if coordinator_flag_active: - expected_url = 'http://coordinator_url/lms/redirect/' + expected_url = 'http://coordinator_url/lms/payment_page_redirect/' else: expected_url = 'http://ecommerce_url/test_basket/add/' self.assertIsNotNone(result) - self.assertEqual(result, expected_url) + self.assertEqual(expected_url, result) @ddt.ddt From 294c6a77688855a74bfc5cd1522b9de6cc1f7f8b Mon Sep 17 00:00:00 2001 From: "Glenn R. Martin" Date: Tue, 9 Apr 2024 11:12:57 -0400 Subject: [PATCH 7/7] fix: feedback from Shafqat --- lms/djangoapps/commerce/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lms/djangoapps/commerce/utils.py b/lms/djangoapps/commerce/utils.py index 1a2416231983..a37cd2ab36b2 100644 --- a/lms/djangoapps/commerce/utils.py +++ b/lms/djangoapps/commerce/utils.py @@ -344,7 +344,6 @@ def _refund_in_commerce_coordinator(course_enrollment, change_mode): course_key_str, str(exc) ) - return False # Refund was successfully sent to Commerce Coordinator log.info('Refund successfully sent to Commerce Coordinator for user [%s], course [%s].', @@ -367,7 +366,7 @@ def _auto_enroll(course_enrollment): course_enrollment (CourseEnrollment): The course_enrollment to update. Returns: - bool: True if auto-enroll is succesful. False if auto-enroll is not applicable. + bool: True if auto-enroll is successful. False if auto-enroll is not applicable. """ enrollment_course_id = course_enrollment.course_id