From 7c39978b06a26edf609d54f307c237a4e89f94f5 Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Fri, 27 Jan 2017 17:10:53 -0500 Subject: [PATCH] Automated refund approval process Refunds are now processed automatically. If the automated processing fails, the system falls back to the previous behavior of notifying the Support Team. Additionally, these calls are all made by the service user rather than the learner. ECOM-6541 --- ...ration_enable_automatic_refund_approval.py | 19 +++ lms/djangoapps/commerce/models.py | 4 + lms/djangoapps/commerce/signals.py | 121 +++++++++--------- lms/djangoapps/commerce/tests/mocks.py | 21 ++- lms/djangoapps/commerce/tests/test_signals.py | 75 ++++++++--- 5 files changed, 156 insertions(+), 84 deletions(-) create mode 100644 lms/djangoapps/commerce/migrations/0005_commerceconfiguration_enable_automatic_refund_approval.py diff --git a/lms/djangoapps/commerce/migrations/0005_commerceconfiguration_enable_automatic_refund_approval.py b/lms/djangoapps/commerce/migrations/0005_commerceconfiguration_enable_automatic_refund_approval.py new file mode 100644 index 0000000000..1f46a5e979 --- /dev/null +++ b/lms/djangoapps/commerce/migrations/0005_commerceconfiguration_enable_automatic_refund_approval.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('commerce', '0004_auto_20160531_0950'), + ] + + operations = [ + migrations.AddField( + model_name='commerceconfiguration', + name='enable_automatic_refund_approval', + field=models.BooleanField(default=True, help_text='Automatically approve valid refund requests, without manual processing'), + ), + ] diff --git a/lms/djangoapps/commerce/models.py b/lms/djangoapps/commerce/models.py index 0f47655250..5cf535ca00 100644 --- a/lms/djangoapps/commerce/models.py +++ b/lms/djangoapps/commerce/models.py @@ -39,6 +39,10 @@ class CommerceConfiguration(ConfigurationModel): default=DEFAULT_RECEIPT_PAGE_URL, help_text=_('Path to order receipt page.') ) + enable_automatic_refund_approval = models.BooleanField( + default=True, + help_text=_('Automatically approve valid refund requests, without manual processing') + ) def __unicode__(self): return "Commerce configuration" diff --git a/lms/djangoapps/commerce/signals.py b/lms/djangoapps/commerce/signals.py index 401a16f8b8..b288553b09 100644 --- a/lms/djangoapps/commerce/signals.py +++ b/lms/djangoapps/commerce/signals.py @@ -9,23 +9,24 @@ from urlparse import urljoin import requests from django.conf import settings +from django.contrib.auth import get_user_model from django.contrib.auth.models import AnonymousUser from django.dispatch import receiver from django.utils.translation import ugettext as _ -from edx_rest_api_client.exceptions import HttpClientError -from request_cache.middleware import RequestCache -from student.models import UNENROLL_DONE +from commerce.models import CommerceConfiguration from openedx.core.djangoapps.commerce.utils import ecommerce_api_client, is_commerce_service_configured from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming import helpers as theming_helpers +from request_cache.middleware import RequestCache +from student.models import UNENROLL_DONE log = logging.getLogger(__name__) +# pylint: disable=unused-argument @receiver(UNENROLL_DONE) -def handle_unenroll_done(sender, course_enrollment=None, skip_refund=False, - **kwargs): # pylint: disable=unused-argument +def handle_unenroll_done(sender, course_enrollment=None, skip_refund=False, **kwargs): """ Signal receiver for unenrollments, used to automatically initiate refunds when applicable. @@ -40,12 +41,12 @@ def handle_unenroll_done(sender, course_enrollment=None, skip_refund=False, request_user = get_request_user() or course_enrollment.user if isinstance(request_user, AnonymousUser): # Assume the request was initiated via server-to-server - # api call (presumably Otto). In this case we cannot + # API call (presumably Otto). In this case we cannot # construct a client to call Otto back anyway, because # the client does not work anonymously, and furthermore, # there's certainly no need to inform Otto about this request. return - refund_seat(course_enrollment, request_user) + refund_seat(course_enrollment) except: # pylint: disable=bare-except # don't assume the signal was fired with `send_robust`. # avoid blowing up other signal handlers by gracefully @@ -69,78 +70,76 @@ def get_request_user(): return getattr(request, 'user', None) -def refund_seat(course_enrollment, request_user): +def refund_seat(course_enrollment): """ - Attempt to initiate a refund for any orders associated with the seat being - unenrolled, using the commerce service. + Attempt to initiate a refund for any orders associated with the seat being unenrolled, using the commerce service. Arguments: course_enrollment (CourseEnrollment): a student enrollment - request_user: the user as whom to authenticate to the commerce service - when attempting to initiate the refund. Returns: A list of the external service's IDs for any refunds that were initiated (may be empty). Raises: - exceptions.SlumberBaseException: for any unhandled HTTP error during - communication with the commerce service. - exceptions.Timeout: if the attempt to reach the commerce service timed - out. - + exceptions.SlumberBaseException: for any unhandled HTTP error during communication with the E-Commerce Service. + exceptions.Timeout: if the attempt to reach the commerce service timed out. """ + User = get_user_model() # pylint:disable=invalid-name course_key_str = unicode(course_enrollment.course_id) - unenrolled_user = course_enrollment.user + enrollee = course_enrollment.user - try: - refund_ids = ecommerce_api_client(request_user or unenrolled_user).refunds.post( - {'course_id': course_key_str, 'username': unenrolled_user.username} - ) - except HttpClientError, exc: - if exc.response.status_code == 403 and request_user != unenrolled_user: - # this is a known limitation; commerce service does not presently - # support the case of a non-superusers initiating a refund on - # behalf of another user. - log.warning("User [%s] was not authorized to initiate a refund for user [%s] " - "upon unenrollment from course [%s]", request_user.id, unenrolled_user.id, course_key_str) - return [] - else: - # no other error is anticipated, so re-raise the Exception - raise exc + service_user = User.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) + api_client = ecommerce_api_client(service_user) + + log.info('Attempting to create a refund for user [%s], course [%s]...', enrollee.id, course_key_str) + + refund_ids = api_client.refunds.post({'course_id': course_key_str, 'username': enrollee.username}) if refund_ids: - # at least one refundable order was found. - log.info( - "Refund successfully opened for user [%s], course [%s]: %r", - unenrolled_user.id, - course_key_str, - refund_ids, - ) + log.info('Refund successfully opened for user [%s], course [%s]: %r', enrollee.id, course_key_str, refund_ids) - # XCOM-371: this is a temporary measure to suppress refund-related email - # notifications to students and support@) for free enrollments. This - # condition should be removed when the CourseEnrollment.refundable() logic - # is updated to be more correct, or when we implement better handling (and - # notifications) in Otto for handling reversal of $0 transactions. - if course_enrollment.mode != 'verified': - # 'verified' is the only enrollment mode that should presently - # result in opening a refund request. - log.info( - "Skipping refund email notification for non-verified mode for user [%s], course [%s], mode: [%s]", - course_enrollment.user.id, - course_enrollment.course_id, - course_enrollment.mode, - ) + config = CommerceConfiguration.current() + + if config.enable_automatic_refund_approval: + refunds_requiring_approval = [] + + for refund_id in refund_ids: + try: + # NOTE: Approve payment only because the user has already been unenrolled. Additionally, this + # ensures we don't tie up an additional web worker when the E-Commerce Service tries to unenroll + # the learner + api_client.refunds(refund_id).process.put({'action': 'approve_payment_only'}) + log.info('Refund [%d] successfully approved.', refund_id) + except: # pylint: disable=bare-except + log.exception('Failed to automatically approve refund [%d]!', refund_id) + refunds_requiring_approval.append(refund_id) else: - try: - send_refund_notification(course_enrollment, refund_ids) - except: # pylint: disable=bare-except - # don't break, just log a warning - log.warning("Could not send email notification for refund.", exc_info=True) + refunds_requiring_approval = refund_ids + + if refunds_requiring_approval: + # XCOM-371: this is a temporary measure to suppress refund-related email + # notifications to students and support for free enrollments. This + # condition should be removed when the CourseEnrollment.refundable() logic + # is updated to be more correct, or when we implement better handling (and + # notifications) in Otto for handling reversal of $0 transactions. + if course_enrollment.mode != 'verified': + # 'verified' is the only enrollment mode that should presently + # result in opening a refund request. + log.info( + 'Skipping refund email notification for non-verified mode for user [%s], course [%s], mode: [%s]', + course_enrollment.user.id, + course_enrollment.course_id, + course_enrollment.mode, + ) + else: + try: + send_refund_notification(course_enrollment, refunds_requiring_approval) + except: # pylint: disable=bare-except + # don't break, just log a warning + log.warning('Could not send email notification for refund.', exc_info=True) else: - # no refundable orders were found. - log.debug("No refund opened for user [%s], course [%s]", unenrolled_user.id, course_key_str) + log.info('No refund opened for user [%s], course [%s]', enrollee.id, course_key_str) return refund_ids diff --git a/lms/djangoapps/commerce/tests/mocks.py b/lms/djangoapps/commerce/tests/mocks.py index b890df0977..d4a4489f40 100644 --- a/lms/djangoapps/commerce/tests/mocks.py +++ b/lms/djangoapps/commerce/tests/mocks.py @@ -26,7 +26,7 @@ class mock_ecommerce_api_endpoint(object): host = settings.ECOMMERCE_API_URL.strip('/') - def __init__(self, response=None, status=200, expect_called=True, exception=None): + def __init__(self, response=None, status=200, expect_called=True, exception=None, reset_on_exit=True): """ Keyword Arguments: response: a JSON-serializable Python type representing the desired response body. @@ -34,11 +34,13 @@ class mock_ecommerce_api_endpoint(object): expect_called: a boolean indicating whether an API request was expected; set to False if we should ensure that no request arrived. exception: raise this exception instead of returning an HTTP response when called. + reset_on_exit (bool): Indicates if `httpretty` should be reset after the decorator exits. """ self.response = response or self.default_response self.status = status self.expect_called = expect_called self.exception = exception + self.reset_on_exit = reset_on_exit def get_uri(self): """ @@ -74,7 +76,8 @@ class mock_ecommerce_api_endpoint(object): def __exit__(self, exc_type, exc_val, exc_tb): assert self.expect_called == (httpretty.last_request().headers != {}) httpretty.disable() - httpretty.reset() + if self.reset_on_exit: + httpretty.reset() class mock_create_basket(mock_ecommerce_api_endpoint): @@ -119,6 +122,20 @@ class mock_create_refund(mock_ecommerce_api_endpoint): return '/refunds/' +class mock_process_refund(mock_ecommerce_api_endpoint): + """ Mocks calls to E-Commerce API client refund process method. """ + + default_response = [] + method = httpretty.PUT + + def __init__(self, refund_id, **kwargs): + super(mock_process_refund, self).__init__(**kwargs) + self.refund_id = refund_id + + def get_path(self): + return '/refunds/{}/process/'.format(self.refund_id) + + class mock_order_endpoint(mock_ecommerce_api_endpoint): """ Mocks calls to E-Commerce API client basket order method. """ diff --git a/lms/djangoapps/commerce/tests/test_signals.py b/lms/djangoapps/commerce/tests/test_signals.py index 75ea4c55f0..7901fb4ce0 100644 --- a/lms/djangoapps/commerce/tests/test_signals.py +++ b/lms/djangoapps/commerce/tests/test_signals.py @@ -11,17 +11,17 @@ from urlparse import urljoin import ddt import httpretty import mock +from django.conf import settings from django.contrib.auth.models import AnonymousUser from django.test import TestCase from django.test.utils import override_settings from opaque_keys.edx.keys import CourseKey from requests import Timeout -from commerce.signals import ( - refund_seat, send_refund_notification, generate_refund_notification_body, create_zendesk_ticket -) +from commerce.models import CommerceConfiguration +from commerce.signals import send_refund_notification, generate_refund_notification_body, create_zendesk_ticket from commerce.tests import JSON -from commerce.tests.mocks import mock_create_refund +from commerce.tests.mocks import mock_create_refund, mock_process_refund from course_modes.models import CourseMode from student.models import UNENROLL_DONE from student.tests.factories import UserFactory, CourseEnrollmentFactory @@ -40,6 +40,10 @@ class TestRefundSignal(TestCase): def setUp(self): super(TestRefundSignal, self).setUp() + + # Ensure the E-Commerce service user exists + UserFactory(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME, is_staff=True) + self.requester = UserFactory(username="test-requester") self.student = UserFactory( username="test-student", @@ -52,6 +56,10 @@ class TestRefundSignal(TestCase): ) self.course_enrollment.refundable = mock.Mock(return_value=True) + self.config = CommerceConfiguration.current() + self.config.enable_automatic_refund_approval = True + self.config.save() + def send_signal(self, skip_refund=False): """ DRY helper: emit the UNENROLL_DONE signal, as is done in @@ -84,7 +92,7 @@ class TestRefundSignal(TestCase): """ self.send_signal() self.assertTrue(mock_refund_seat.called) - self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment, self.student)) + self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment,)) # if skip_refund is set to True in the signal, we should not try to initiate a refund. mock_refund_seat.reset_mock() @@ -106,21 +114,21 @@ class TestRefundSignal(TestCase): # no HTTP request/user: auth to commerce service as the unenrolled student. self.send_signal() self.assertTrue(mock_refund_seat.called) - self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment, self.student)) + self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment,)) # HTTP user is the student: auth to commerce service as the unenrolled student. mock_get_request_user.return_value = self.student mock_refund_seat.reset_mock() self.send_signal() self.assertTrue(mock_refund_seat.called) - self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment, self.student)) + self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment,)) # HTTP user is another user: auth to commerce service as the requester. mock_get_request_user.return_value = self.requester mock_refund_seat.reset_mock() self.send_signal() self.assertTrue(mock_refund_seat.called) - self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment, self.requester)) + self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment,)) # HTTP user is another server (AnonymousUser): do not try to initiate a refund at all. mock_get_request_user.return_value = AnonymousUser() @@ -128,15 +136,6 @@ class TestRefundSignal(TestCase): self.send_signal() self.assertFalse(mock_refund_seat.called) - @mock.patch('commerce.signals.log.warning') - def test_not_authorized_warning(self, mock_log_warning): - """ - Ensure that expected authorization issues are logged as warnings. - """ - with mock_create_refund(status=403): - refund_seat(self.course_enrollment, UserFactory()) - self.assertTrue(mock_log_warning.called) - @mock.patch('commerce.signals.log.exception') def test_error_logging(self, mock_log_exception): """ @@ -148,14 +147,48 @@ class TestRefundSignal(TestCase): self.assertTrue(mock_log_exception.called) @mock.patch('commerce.signals.send_refund_notification') - def test_notification(self, mock_send_notification): + def test_notification_when_approval_fails(self, mock_send_notification): """ - Ensure the notification function is triggered when refunds are - initiated + Ensure the notification function is triggered when refunds are initiated, and cannot be automatically approved. """ - with mock_create_refund(status=200, response=[1, 2, 3]): + refund_id = 1 + failed_refund_id = 2 + + with mock_create_refund(status=201, response=[refund_id, failed_refund_id]): + with mock_process_refund(refund_id, reset_on_exit=False): + with mock_process_refund(failed_refund_id, status=500, reset_on_exit=False): + self.send_signal() + self.assertTrue(mock_send_notification.called) + mock_send_notification.assert_called_with(self.course_enrollment, [failed_refund_id]) + + @mock.patch('commerce.signals.send_refund_notification') + def test_notification_if_automatic_approval_disabled(self, mock_send_notification): + """ + Ensure the notification is always sent if the automatic approval functionality is disabled. + """ + refund_id = 1 + self.config.enable_automatic_refund_approval = False + self.config.save() + + with mock_create_refund(status=201, response=[refund_id]): self.send_signal() self.assertTrue(mock_send_notification.called) + mock_send_notification.assert_called_with(self.course_enrollment, [refund_id]) + + @mock.patch('commerce.signals.send_refund_notification') + def test_no_notification_after_approval(self, mock_send_notification): + """ + Ensure the notification function is triggered when refunds are initiated, and cannot be automatically approved. + """ + refund_id = 1 + + with mock_create_refund(status=201, response=[refund_id]): + with mock_process_refund(refund_id, reset_on_exit=False): + self.send_signal() + self.assertFalse(mock_send_notification.called) + + last_request = httpretty.last_request() + self.assertDictEqual(json.loads(last_request.body), {'action': 'approve_payment_only'}) @mock.patch('commerce.signals.send_refund_notification') def test_notification_no_refund(self, mock_send_notification):