From 1189867dd2c2ab53396d1fa4bca50dbf941fc3d0 Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Fri, 27 Jan 2017 16:19:37 -0500 Subject: [PATCH 1/2] Removed references to ECOMMERCE_API_SIGNING_KEY We should not be using custom signing keys for each service at this time. We may want to return to this strategy in the future; but, this is not the direction any of our other services are going in. ECOM-6541 --- common/djangoapps/student/models.py | 4 +- .../djangoapps/student/tests/test_refunds.py | 5 +- .../third_party_auth/tests/specs/base.py | 4 +- .../commerce/api/v0/tests/test_views.py | 8 ++- .../commerce/api/v1/tests/test_views.py | 2 - lms/djangoapps/commerce/tests/__init__.py | 53 +++++-------------- lms/djangoapps/commerce/tests/mocks.py | 52 +++++++++++------- lms/djangoapps/commerce/tests/test_signals.py | 22 ++++---- .../student_account/test/test_views.py | 3 +- .../verify_student/tests/test_views.py | 9 ++-- lms/envs/aws.py | 1 - lms/envs/bok_choy.py | 1 - lms/envs/common.py | 1 - lms/envs/test.py | 2 + openedx/core/djangoapps/commerce/utils.py | 31 +++++------ .../core/djangoapps/credit/tests/test_api.py | 8 ++- openedx/core/lib/tests/test_edx_api_utils.py | 4 +- openedx/core/lib/token_utils.py | 12 ++++- 18 files changed, 98 insertions(+), 124 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index dd718b1843..18a82bdaed 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -49,7 +49,6 @@ import request_cache from certificates.models import GeneratedCertificate from course_modes.models import CourseMode from enrollment.api import _default_course_mode -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client, ECOMMERCE_DATE_FORMAT from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.xmodule_django.models import CourseKeyField, NoneToEmptyManager @@ -1537,6 +1536,9 @@ class CourseEnrollment(models.Model): def refund_cutoff_date(self): """ Calculate and return the refund window end date. """ + # NOTE: This is here to avoid circular references + from openedx.core.djangoapps.commerce.utils import ecommerce_api_client, ECOMMERCE_DATE_FORMAT + try: attribute = self.attributes.get(namespace='order', name='order_number') except ObjectDoesNotExist: diff --git a/common/djangoapps/student/tests/test_refunds.py b/common/djangoapps/student/tests/test_refunds.py index 6e8e568489..4dd6ba2ecc 100644 --- a/common/djangoapps/student/tests/test_refunds.py +++ b/common/djangoapps/student/tests/test_refunds.py @@ -30,7 +30,6 @@ from config_models.models import cache log = logging.getLogger(__name__) TEST_API_URL = 'http://www-internal.example.com/api' -TEST_API_SIGNING_KEY = 'edx' JSON = 'application/json' @@ -131,7 +130,7 @@ class RefundableTest(SharedModuleStoreTestCase): ) @ddt.unpack @httpretty.activate - @override_settings(ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, ECOMMERCE_API_URL=TEST_API_URL) + @override_settings(ECOMMERCE_API_URL=TEST_API_URL) def test_refund_cutoff_date(self, order_date_delta, course_start_delta, expected_date_delta, days): """ Assert that the later date is used with the configurable refund period in calculating the returned cutoff date. @@ -172,7 +171,7 @@ class RefundableTest(SharedModuleStoreTestCase): self.assertIsNone(self.enrollment.refund_cutoff_date()) @httpretty.activate - @override_settings(ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, ECOMMERCE_API_URL=TEST_API_URL) + @override_settings(ECOMMERCE_API_URL=TEST_API_URL) def test_multiple_refunds_dashbaord_page_error(self): """ Order with mutiple refunds will not throw 500 error when dashboard page will access.""" now = datetime.now(pytz.UTC).replace(microsecond=0) diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py index c6eab3d0af..f821f56f7e 100644 --- a/common/djangoapps/third_party_auth/tests/specs/base.py +++ b/common/djangoapps/third_party_auth/tests/specs/base.py @@ -18,7 +18,7 @@ from social import actions, exceptions from social.apps.django_app import utils as social_utils from social.apps.django_app import views as social_views -from lms.djangoapps.commerce.tests import TEST_API_URL, TEST_API_SIGNING_KEY +from lms.djangoapps.commerce.tests import TEST_API_URL from student import models as student_models from student import views as student_views from student.tests.factories import UserFactory @@ -911,7 +911,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase): # pylint: disable=test-inherits-tests, abstract-method -@django_utils.override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) +@django_utils.override_settings(ECOMMERCE_API_URL=TEST_API_URL) class Oauth2IntegrationTest(IntegrationTest): """Base test case for integration tests of Oauth2 providers.""" diff --git a/lms/djangoapps/commerce/api/v0/tests/test_views.py b/lms/djangoapps/commerce/api/v0/tests/test_views.py index ac3a66a458..b2dc7981ae 100644 --- a/lms/djangoapps/commerce/api/v0/tests/test_views.py +++ b/lms/djangoapps/commerce/api/v0/tests/test_views.py @@ -17,7 +17,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from commerce.constants import Messages -from commerce.tests import TEST_BASKET_ID, TEST_ORDER_NUMBER, TEST_PAYMENT_DATA, TEST_API_URL, TEST_API_SIGNING_KEY +from commerce.tests import TEST_BASKET_ID, TEST_ORDER_NUMBER, TEST_PAYMENT_DATA from commerce.tests.mocks import mock_basket_order, mock_create_basket from commerce.tests.test_views import UserMixin from course_modes.models import CourseMode @@ -39,7 +39,6 @@ UTM_COOKIE_CONTENTS = { @attr(shard=1) @ddt.ddt -@override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) class BasketsViewTests(EnrollmentEventTestMixin, UserMixin, ModuleStoreTestCase): """ Tests for the commerce orders view. @@ -276,7 +275,7 @@ class BasketsViewTests(EnrollmentEventTestMixin, UserMixin, ModuleStoreTestCase) # We should be enrolled in honor mode self._test_course_without_sku(enrollment_mode=CourseMode.HONOR) - @override_settings(ECOMMERCE_API_URL=None, ECOMMERCE_API_SIGNING_KEY=None) + @override_settings(ECOMMERCE_API_URL=None) def test_ecommerce_service_not_configured(self): """ If the E-Commerce Service is not configured, the view should enroll the user. @@ -313,7 +312,7 @@ class BasketsViewTests(EnrollmentEventTestMixin, UserMixin, ModuleStoreTestCase) """ Verifies that the view behaves appropriately when the course only has a professional mode. """ self.assertProfessionalModeBypassed() - @override_settings(ECOMMERCE_API_URL=None, ECOMMERCE_API_SIGNING_KEY=None) + @override_settings(ECOMMERCE_API_URL=None) def test_professional_mode_only_and_ecommerce_service_not_configured(self): """ Verifies that the view behaves appropriately when the course only has a professional mode and @@ -390,7 +389,6 @@ class BasketsViewTests(EnrollmentEventTestMixin, UserMixin, ModuleStoreTestCase) @attr(shard=1) -@override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) class BasketOrderViewTests(UserMixin, TestCase): """ Tests for the basket order view. """ view_name = 'commerce_api:v0:baskets:retrieve_order' diff --git a/lms/djangoapps/commerce/api/v1/tests/test_views.py b/lms/djangoapps/commerce/api/v1/tests/test_views.py index ca6c4d31dd..501d3b38b7 100644 --- a/lms/djangoapps/commerce/api/v1/tests/test_views.py +++ b/lms/djangoapps/commerce/api/v1/tests/test_views.py @@ -17,7 +17,6 @@ from rest_framework.utils.encoders import JSONEncoder from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from commerce.tests import TEST_API_URL, TEST_API_SIGNING_KEY from commerce.tests.mocks import mock_order_endpoint from commerce.tests.test_views import UserMixin from course_modes.models import CourseMode @@ -391,7 +390,6 @@ class CourseRetrieveUpdateViewTests(CourseApiViewTestMixin, ModuleStoreTestCase) @attr(shard=1) -@override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) class OrderViewTests(UserMixin, TestCase): """ Tests for the basket order view. """ view_name = 'commerce_api:v1:orders:detail' diff --git a/lms/djangoapps/commerce/tests/__init__.py b/lms/djangoapps/commerce/tests/__init__.py index 959d182efe..922a2b3d85 100644 --- a/lms/djangoapps/commerce/tests/__init__.py +++ b/lms/djangoapps/commerce/tests/__init__.py @@ -1,25 +1,19 @@ # -*- coding: utf-8 -*- """ Commerce app tests package. """ -import datetime -import json -from django.conf import settings -from django.contrib.auth.models import User -from django.test import TestCase -from django.test.utils import override_settings -from freezegun import freeze_time import httpretty -import jwt import mock +from django.conf import settings +from django.test import TestCase +from freezegun import freeze_time -from edx_rest_api_client import auth from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.lib.token_utils import JwtBuilder from student.tests.factories import UserFactory JSON = 'application/json' TEST_PUBLIC_URL_ROOT = 'http://www.example.com' TEST_API_URL = 'http://www-internal.example.com/api' -TEST_API_SIGNING_KEY = 'edx' TEST_BASKET_ID = 7 TEST_ORDER_NUMBER = '100004' TEST_PAYMENT_DATA = { @@ -29,33 +23,27 @@ TEST_PAYMENT_DATA = { } -@override_settings(ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, ECOMMERCE_API_URL=TEST_API_URL) class EdxRestApiClientTest(TestCase): """ Tests to ensure the client is initialized properly. """ - TEST_USER_EMAIL = 'test@example.com' TEST_CLIENT_ID = 'test-client-id' def setUp(self): super(EdxRestApiClientTest, self).setUp() - self.user = UserFactory() - self.user.email = self.TEST_USER_EMAIL - self.user.save() # pylint: disable=no-member @httpretty.activate @freeze_time('2015-7-2') - @override_settings(JWT_AUTH={'JWT_ISSUER': 'http://example.com/oauth', 'JWT_EXPIRATION': 30}) def test_tracking_context(self): """ Ensure the tracking context is set up in the api client correctly and automatically. """ - # fake an ecommerce api request. + # fake an E-Commerce API request. httpretty.register_uri( httpretty.POST, - '{}/baskets/1/'.format(TEST_API_URL), + '{}/baskets/1/'.format(settings.ECOMMERCE_API_URL.strip('/')), status=200, body='{}', adding_headers={'Content-Type': JSON} ) @@ -65,23 +53,18 @@ class EdxRestApiClientTest(TestCase): with mock.patch('openedx.core.djangoapps.commerce.utils.tracker.get_tracker', return_value=mock_tracker): ecommerce_api_client(self.user).baskets(1).post() - # make sure the request's JWT token payload included correct tracking context values. + # Verify the JWT includes the tracking context for the user actual_header = httpretty.last_request().headers['Authorization'] - expected_payload = { - 'username': self.user.username, - 'full_name': self.user.profile.name, - 'email': self.user.email, - 'iss': settings.JWT_AUTH['JWT_ISSUER'], - 'iat': datetime.datetime.utcnow(), - 'exp': datetime.datetime.utcnow() + datetime.timedelta(seconds=settings.JWT_AUTH['JWT_EXPIRATION']), + + claims = { 'tracking_context': { 'lms_user_id': self.user.id, # pylint: disable=no-member 'lms_client_id': self.TEST_CLIENT_ID, 'lms_ip': '127.0.0.1', - }, + } } - - expected_header = 'JWT {}'.format(jwt.encode(expected_payload, TEST_API_SIGNING_KEY)) + expected_jwt = JwtBuilder(self.user).build_token(['email', 'profile'], additional_claims=claims) + expected_header = 'JWT {}'.format(expected_jwt) self.assertEqual(actual_header, expected_header) @httpretty.activate @@ -95,19 +78,9 @@ class EdxRestApiClientTest(TestCase): expected_content = '{"result": "Préparatoire"}' httpretty.register_uri( httpretty.GET, - '{}/baskets/1/order/'.format(TEST_API_URL), + '{}/baskets/1/order/'.format(settings.ECOMMERCE_API_URL.strip('/')), status=200, body=expected_content, adding_headers={'Content-Type': JSON}, ) actual_object = ecommerce_api_client(self.user).baskets(1).order.get() self.assertEqual(actual_object, {u"result": u"Préparatoire"}) - - def test_client_with_user_without_profile(self): - """ - Verify client initialize successfully for users having no profile. - """ - worker = User.objects.create_user(username='test_worker', email='test@example.com') - api_client = ecommerce_api_client(worker) - - self.assertEqual(api_client._store['session'].auth.__dict__['username'], worker.username) # pylint: disable=protected-access - self.assertIsNone(api_client._store['session'].auth.__dict__['full_name']) # pylint: disable=protected-access diff --git a/lms/djangoapps/commerce/tests/mocks.py b/lms/djangoapps/commerce/tests/mocks.py index 933e4bbb01..b890df0977 100644 --- a/lms/djangoapps/commerce/tests/mocks.py +++ b/lms/djangoapps/commerce/tests/mocks.py @@ -2,11 +2,14 @@ import json import httpretty +from django.conf import settings -from commerce.tests import TEST_API_URL, factories +from commerce.tests import factories -class mock_ecommerce_api_endpoint(object): # pylint: disable=invalid-name +# pylint: disable=invalid-name + +class mock_ecommerce_api_endpoint(object): """ Base class for contextmanagers used to mock calls to api endpoints. @@ -21,6 +24,8 @@ class mock_ecommerce_api_endpoint(object): # pylint: disable=invalid-name # override this in subclasses, using one of httpretty's method constants method = None + host = settings.ECOMMERCE_API_URL.strip('/') + def __init__(self, response=None, status=200, expect_called=True, exception=None): """ Keyword Arguments: @@ -37,9 +42,18 @@ class mock_ecommerce_api_endpoint(object): # pylint: disable=invalid-name def get_uri(self): """ - Return the uri to register with httpretty for this contextmanager. + Returns the uri to register with httpretty for this contextmanager. + """ + return self.host + '/' + self.get_path().lstrip('/') + + def get_path(self): + """ + Returns the path of the URI to register with httpretty for this contextmanager. Subclasses must override this method. + + Returns: + str """ raise NotImplementedError @@ -48,7 +62,6 @@ class mock_ecommerce_api_endpoint(object): # pylint: disable=invalid-name raise self.exception # pylint: disable=raising-bad-type def __enter__(self): - httpretty.reset() httpretty.enable() httpretty.register_uri( self.method, @@ -61,9 +74,10 @@ class mock_ecommerce_api_endpoint(object): # pylint: disable=invalid-name def __exit__(self, exc_type, exc_val, exc_tb): assert self.expect_called == (httpretty.last_request().headers != {}) httpretty.disable() + httpretty.reset() -class mock_create_basket(mock_ecommerce_api_endpoint): # pylint: disable=invalid-name +class mock_create_basket(mock_ecommerce_api_endpoint): """ Mocks calls to E-Commerce API client basket creation method. """ default_response = { @@ -77,11 +91,11 @@ class mock_create_basket(mock_ecommerce_api_endpoint): # pylint: disable=invali } method = httpretty.POST - def get_uri(self): - return TEST_API_URL + '/baskets/' + def get_path(self): + return '/baskets/' -class mock_basket_order(mock_ecommerce_api_endpoint): # pylint: disable=invalid-name +class mock_basket_order(mock_ecommerce_api_endpoint): """ Mocks calls to E-Commerce API client basket order method. """ default_response = {'number': 1} @@ -91,21 +105,21 @@ class mock_basket_order(mock_ecommerce_api_endpoint): # pylint: disable=invalid super(mock_basket_order, self).__init__(**kwargs) self.basket_id = basket_id - def get_uri(self): - return TEST_API_URL + '/baskets/{}/order/'.format(self.basket_id) + def get_path(self): + return '/baskets/{}/order/'.format(self.basket_id) -class mock_create_refund(mock_ecommerce_api_endpoint): # pylint: disable=invalid-name +class mock_create_refund(mock_ecommerce_api_endpoint): """ Mocks calls to E-Commerce API client refund creation method. """ default_response = [] method = httpretty.POST - def get_uri(self): - return TEST_API_URL + '/refunds/' + def get_path(self): + return '/refunds/' -class mock_order_endpoint(mock_ecommerce_api_endpoint): # pylint: disable=invalid-name +class mock_order_endpoint(mock_ecommerce_api_endpoint): """ Mocks calls to E-Commerce API client basket order method. """ default_response = {'number': 'EDX-100001'} @@ -115,11 +129,11 @@ class mock_order_endpoint(mock_ecommerce_api_endpoint): # pylint: disable=inval super(mock_order_endpoint, self).__init__(**kwargs) self.order_number = order_number - def get_uri(self): - return TEST_API_URL + '/orders/{}/'.format(self.order_number) + def get_path(self): + return '/orders/{}/'.format(self.order_number) -class mock_get_orders(mock_ecommerce_api_endpoint): # pylint: disable=invalid-name +class mock_get_orders(mock_ecommerce_api_endpoint): """ Mocks calls to E-Commerce API client order get method. """ default_response = { @@ -138,5 +152,5 @@ class mock_get_orders(mock_ecommerce_api_endpoint): # pylint: disable=invalid-n } method = httpretty.GET - def get_uri(self): - return TEST_API_URL + '/orders/' + def get_path(self): + return '/orders/' diff --git a/lms/djangoapps/commerce/tests/test_signals.py b/lms/djangoapps/commerce/tests/test_signals.py index 4f36ffbbb2..75ea4c55f0 100644 --- a/lms/djangoapps/commerce/tests/test_signals.py +++ b/lms/djangoapps/commerce/tests/test_signals.py @@ -9,21 +9,22 @@ import json from urlparse import urljoin import ddt +import httpretty +import mock from django.contrib.auth.models import AnonymousUser from django.test import TestCase from django.test.utils import override_settings -import httpretty -import mock from opaque_keys.edx.keys import CourseKey from requests import Timeout -from student.models import UNENROLL_DONE -from student.tests.factories import UserFactory, CourseEnrollmentFactory -from commerce.signals import (refund_seat, send_refund_notification, generate_refund_notification_body, - create_zendesk_ticket) -from commerce.tests import TEST_PUBLIC_URL_ROOT, TEST_API_URL, TEST_API_SIGNING_KEY, JSON +from commerce.signals import ( + refund_seat, send_refund_notification, generate_refund_notification_body, create_zendesk_ticket +) +from commerce.tests import JSON from commerce.tests.mocks import mock_create_refund from course_modes.models import CourseMode +from student.models import UNENROLL_DONE +from student.tests.factories import UserFactory, CourseEnrollmentFactory ZENDESK_URL = 'http://zendesk.example.com/' ZENDESK_USER = 'test@example.com' @@ -31,11 +32,7 @@ ZENDESK_API_KEY = 'abc123' @ddt.ddt -@override_settings( - ECOMMERCE_PUBLIC_URL_ROOT=TEST_PUBLIC_URL_ROOT, - ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, - ZENDESK_URL=ZENDESK_URL, ZENDESK_USER=ZENDESK_USER, ZENDESK_API_KEY=ZENDESK_API_KEY -) +@override_settings(ZENDESK_URL=ZENDESK_URL, ZENDESK_USER=ZENDESK_USER, ZENDESK_API_KEY=ZENDESK_API_KEY) class TestRefundSignal(TestCase): """ Exercises logic triggered by the UNENROLL_DONE signal. @@ -65,7 +62,6 @@ class TestRefundSignal(TestCase): @override_settings( ECOMMERCE_PUBLIC_URL_ROOT=None, ECOMMERCE_API_URL=None, - ECOMMERCE_API_SIGNING_KEY=None, ) def test_no_service(self): """ diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index d199dcac2d..7b83c16398 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -32,7 +32,7 @@ from provider.oauth2.models import ( from testfixtures import LogCapture from commerce.models import CommerceConfiguration -from commerce.tests import TEST_API_URL, TEST_API_SIGNING_KEY, factories +from commerce.tests import factories from commerce.tests.mocks import mock_get_orders from course_modes.models import CourseMode from openedx.core.djangoapps.oauth_dispatch.tests import factories as dot_factories @@ -506,7 +506,6 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi }) -@override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) class AccountSettingsViewTest(ThirdPartyAuthTestMixin, TestCase, ProgramsApiConfigMixin): """ Tests for the account settings view. """ diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 9add06d8b2..c22e49ccf5 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -36,7 +36,7 @@ from course_modes.tests.factories import CourseModeFactory from courseware.url_helpers import get_redirect_url from common.test.utils import XssTestMixin from commerce.models import CommerceConfiguration -from commerce.tests import TEST_PAYMENT_DATA, TEST_API_URL, TEST_API_SIGNING_KEY, TEST_PUBLIC_URL_ROOT +from commerce.tests import TEST_PAYMENT_DATA, TEST_API_URL, TEST_PUBLIC_URL_ROOT from openedx.core.djangoapps.embargo.test_utils import restrict_course from openedx.core.djangoapps.user_api.accounts.api import get_account_settings from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme @@ -140,7 +140,6 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase, XssTestMixin): @httpretty.activate @override_settings( ECOMMERCE_API_URL=TEST_API_URL, - ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, ECOMMERCE_PUBLIC_URL_ROOT=TEST_PUBLIC_URL_ROOT ) def test_start_flow_with_ecommerce(self): @@ -1053,7 +1052,7 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase, XssTestMixin): self.assertEqual(response_dict['course_name'], mode_display_name) @httpretty.activate - @override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) + @override_settings(ECOMMERCE_API_URL=TEST_API_URL) @ddt.data("verify_student_start_flow", "verify_student_begin_flow") def test_processors_api(self, payment_flow): """ @@ -1223,7 +1222,7 @@ class TestCreateOrderShoppingCart(CheckoutTestMixin, ModuleStoreTestCase): @attr(shard=2) -@override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) +@override_settings(ECOMMERCE_API_URL=TEST_API_URL) @patch( 'lms.djangoapps.verify_student.views.checkout_with_ecommerce_service', return_value=TEST_PAYMENT_DATA, @@ -1248,7 +1247,7 @@ class TestCheckoutWithEcommerceService(ModuleStoreTestCase): """ @httpretty.activate - @override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY) + @override_settings(ECOMMERCE_API_URL=TEST_API_URL) def test_create_basket(self): """ Check that when working with a product being processed by the diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 3e85617b0f..4d7e52e541 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -776,7 +776,6 @@ ONLOAD_BEACON_SAMPLE_RATE = ENV_TOKENS.get('ONLOAD_BEACON_SAMPLE_RATE', ONLOAD_B ##### ECOMMERCE API CONFIGURATION SETTINGS ##### ECOMMERCE_PUBLIC_URL_ROOT = ENV_TOKENS.get('ECOMMERCE_PUBLIC_URL_ROOT', ECOMMERCE_PUBLIC_URL_ROOT) ECOMMERCE_API_URL = ENV_TOKENS.get('ECOMMERCE_API_URL', ECOMMERCE_API_URL) -ECOMMERCE_API_SIGNING_KEY = AUTH_TOKENS.get('ECOMMERCE_API_SIGNING_KEY', ECOMMERCE_API_SIGNING_KEY) ECOMMERCE_API_TIMEOUT = ENV_TOKENS.get('ECOMMERCE_API_TIMEOUT', ECOMMERCE_API_TIMEOUT) COURSE_CATALOG_API_URL = ENV_TOKENS.get('COURSE_CATALOG_API_URL', COURSE_CATALOG_API_URL) diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index 51b2d680ce..62785a1eb7 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -218,7 +218,6 @@ BADGING_BACKEND = 'lms.djangoapps.badges.backends.tests.dummy_backend.DummyBacke # Configure the LMS to use our stub eCommerce implementation ECOMMERCE_API_URL = 'http://localhost:8043/api/v2/' -ECOMMERCE_API_SIGNING_KEY = 'ecommerce-key' LMS_ROOT_URL = "http://localhost:8000" DOC_LINK_BASE_URL = 'http://edx.readthedocs.io/projects/edx-guide-for-students' diff --git a/lms/envs/common.py b/lms/envs/common.py index d66716004c..42d61ad4f2 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2831,7 +2831,6 @@ ACCOUNT_VISIBILITY_CONFIGURATION = { # E-Commerce API Configuration ECOMMERCE_PUBLIC_URL_ROOT = None ECOMMERCE_API_URL = None -ECOMMERCE_API_SIGNING_KEY = None ECOMMERCE_API_TIMEOUT = 5 ECOMMERCE_SERVICE_WORKER_USERNAME = 'ecommerce_worker' ENTERPRISE_SERVICE_WORKER_USERNAME = 'enterprise_worker' diff --git a/lms/envs/test.py b/lms/envs/test.py index 6e2e61fac5..8075dfca0f 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -588,3 +588,5 @@ COMPREHENSIVE_THEME_DIRS = [REPO_ROOT / "themes", REPO_ROOT / "common/test"] COMPREHENSIVE_THEME_LOCALE_PATHS = [REPO_ROOT / "themes/conf/locale", ] LMS_ROOT_URL = "http://localhost:8000" + +ECOMMERCE_API_URL = 'https://ecommerce.example.com/api/v2/' diff --git a/openedx/core/djangoapps/commerce/utils.py b/openedx/core/djangoapps/commerce/utils.py index 2400cbb721..1f86373878 100644 --- a/openedx/core/djangoapps/commerce/utils.py +++ b/openedx/core/djangoapps/commerce/utils.py @@ -4,13 +4,14 @@ from edx_rest_api_client.client import EdxRestApiClient from eventtracking import tracker from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.lib.token_utils import JwtBuilder -ECOMMERCE_DATE_FORMAT = "%Y-%m-%dT%H:%M:%SZ" +ECOMMERCE_DATE_FORMAT = '%Y-%m-%dT%H:%M:%SZ' def create_tracking_context(user): """ Assembles attributes from user and request objects to be sent along - in ecommerce api calls for tracking purposes. """ + in E-Commerce API calls for tracking purposes. """ context_tracker = tracker.get_tracker().resolve_context() return { @@ -22,27 +23,19 @@ def create_tracking_context(user): def is_commerce_service_configured(): """ - Return a Boolean indicating whether or not configuration is present to use - the external commerce service. + Return a Boolean indicating whether or not configuration is present to use the external commerce service. """ - ecommerce_api_url = configuration_helpers.get_value("ECOMMERCE_API_URL", settings.ECOMMERCE_API_URL) - ecommerce_api_signing_key = configuration_helpers.get_value( - "ECOMMERCE_API_SIGNING_KEY", settings.ECOMMERCE_API_SIGNING_KEY, - ) - return bool(ecommerce_api_url and ecommerce_api_signing_key) + ecommerce_api_url = configuration_helpers.get_value('ECOMMERCE_API_URL', settings.ECOMMERCE_API_URL) + return bool(ecommerce_api_url) -def ecommerce_api_client(user, session=None): +def ecommerce_api_client(user, session=None, token_expiration=None): """ Returns an E-Commerce API client setup with authentication for the specified user. """ - jwt_auth = configuration_helpers.get_value("JWT_AUTH", settings.JWT_AUTH) + claims = {'tracking_context': create_tracking_context(user)} + jwt = JwtBuilder(user).build_token(['email', 'profile'], expires_in=token_expiration, additional_claims=claims) + return EdxRestApiClient( - configuration_helpers.get_value("ECOMMERCE_API_URL", settings.ECOMMERCE_API_URL), - configuration_helpers.get_value("ECOMMERCE_API_SIGNING_KEY", settings.ECOMMERCE_API_SIGNING_KEY), - user.username, - user.profile.name if hasattr(user, 'profile') else None, - user.email, - tracking_context=create_tracking_context(user), - issuer=jwt_auth['JWT_ISSUER'], - expires_in=jwt_auth['JWT_EXPIRATION'], + configuration_helpers.get_value('ECOMMERCE_API_URL', settings.ECOMMERCE_API_URL), + jwt=jwt, session=session ) diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index da7f374965..95c3fe51a2 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/openedx/core/djangoapps/credit/tests/test_api.py @@ -11,7 +11,7 @@ from django.test.utils import override_settings from django.db import connection from nose.plugins.attrib import attr import httpretty -from lms.djangoapps.commerce.tests import TEST_API_SIGNING_KEY, TEST_API_URL +from lms.djangoapps.commerce.tests import TEST_API_URL import mock import pytz from opaque_keys.edx.keys import CourseKey @@ -575,7 +575,6 @@ class CreditRequirementApiTests(CreditApiTestBase): @httpretty.activate @override_settings( ECOMMERCE_API_URL=TEST_API_URL, - ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, ECOMMERCE_SERVICE_WORKER_USERNAME=TEST_ECOMMERCE_WORKER ) def test_satisfy_all_requirements(self): @@ -622,7 +621,7 @@ class CreditRequirementApiTests(CreditApiTestBase): self.assertFalse(api.is_user_eligible_for_credit(user.username, self.course_key)) # Satisfy the other requirement - with self.assertNumQueries(21): + with self.assertNumQueries(25): api.set_credit_requirement_status( user, self.course_key, @@ -676,7 +675,7 @@ class CreditRequirementApiTests(CreditApiTestBase): # Delete the eligibility entries and satisfy the user's eligibility # requirement again to trigger eligibility notification CreditEligibility.objects.all().delete() - with self.assertNumQueries(16): + with self.assertNumQueries(17): api.set_credit_requirement_status( user, self.course_key, @@ -1167,7 +1166,6 @@ class CreditProviderIntegrationApiTests(CreditApiTestBase): @skip_unless_lms @override_settings( ECOMMERCE_API_URL=TEST_API_URL, - ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, ECOMMERCE_SERVICE_WORKER_USERNAME=TEST_ECOMMERCE_WORKER ) @ddt.ddt diff --git a/openedx/core/lib/tests/test_edx_api_utils.py b/openedx/core/lib/tests/test_edx_api_utils.py index e4878b08a8..6b0e29c4f0 100644 --- a/openedx/core/lib/tests/test_edx_api_utils.py +++ b/openedx/core/lib/tests/test_edx_api_utils.py @@ -19,7 +19,6 @@ from student.tests.factories import UserFactory UTILITY_MODULE = 'openedx.core.lib.edx_api_utils' TEST_API_URL = 'http://www-internal.example.com/api' -TEST_API_SIGNING_KEY = 'edx' @skip_unless_lms @@ -200,8 +199,7 @@ class TestGetEdxApiData(ProgramsApiConfigMixin, CacheIsolationTestCase): self.assertTrue(mock_exception.called) self.assertEqual(actual, []) - @override_settings(JWT_AUTH={'JWT_ISSUER': 'http://example.com/oauth', 'JWT_EXPIRATION': 30}, - ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY, ECOMMERCE_API_URL=TEST_API_URL) + @override_settings(ECOMMERCE_API_URL=TEST_API_URL) def test_client_passed(self): """ Verify that when API client is passed edx_rest_api_client is not used. diff --git a/openedx/core/lib/token_utils.py b/openedx/core/lib/token_utils.py index 3191d8828b..74e40a0c5a 100644 --- a/openedx/core/lib/token_utils.py +++ b/openedx/core/lib/token_utils.py @@ -33,17 +33,22 @@ class JwtBuilder(object): self.secret = secret self.jwt_auth = configuration_helpers.get_value('JWT_AUTH', settings.JWT_AUTH) - def build_token(self, scopes, expires_in, aud=None): + def build_token(self, scopes, expires_in=None, aud=None, additional_claims=None): """Returns a JWT access token. Arguments: scopes (list): Scopes controlling which optional claims are included in the token. - expires_in (int): Time to token expiry, specified in seconds. Keyword Arguments: + expires_in (int): Time to token expiry, specified in seconds. aud (string): Overrides configured JWT audience claim. + additional_claims (dict): Additional claims to include in the token. + + Returns: + str: Encoded JWT """ now = int(time()) + expires_in = expires_in or self.jwt_auth['JWT_EXPIRATION'] payload = { 'aud': aud if aud else self.jwt_auth['JWT_AUDIENCE'], 'exp': now + expires_in, @@ -54,6 +59,9 @@ class JwtBuilder(object): 'sub': anonymous_id_for_user(self.user, None), } + if additional_claims: + payload.update(additional_claims) + for scope in scopes: handler = self.claim_handlers.get(scope) From 7c39978b06a26edf609d54f307c237a4e89f94f5 Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Fri, 27 Jan 2017 17:10:53 -0500 Subject: [PATCH 2/2] 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):