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/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/__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..d4a4489f40 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,7 +24,9 @@ class mock_ecommerce_api_endpoint(object): # pylint: disable=invalid-name # override this in subclasses, using one of httpretty's method constants method = None - def __init__(self, response=None, status=200, expect_called=True, exception=None): + host = settings.ECOMMERCE_API_URL.strip('/') + + 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. @@ -29,17 +34,28 @@ class mock_ecommerce_api_endpoint(object): # pylint: disable=invalid-name 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): """ - 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 +64,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 +76,11 @@ 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() + if self.reset_on_exit: + 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 +94,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 +108,35 @@ 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_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. """ default_response = {'number': 'EDX-100001'} @@ -115,11 +146,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 +169,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..7901fb4ce0 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.conf import settings 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 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, mock_process_refund +from course_modes.models import CourseMode 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.tests.mocks import mock_create_refund -from course_modes.models import CourseMode 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. @@ -43,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", @@ -55,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 @@ -65,7 +70,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): """ @@ -88,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() @@ -110,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() @@ -132,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): """ @@ -152,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): 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)