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):