diff --git a/ecommerce/courses/publishers.py b/ecommerce/courses/publishers.py index 5f70fe35d71..9d1b7393d86 100644 --- a/ecommerce/courses/publishers.py +++ b/ecommerce/courses/publishers.py @@ -1,14 +1,16 @@ +from __future__ import unicode_literals import json import logging -import requests from django.conf import settings from django.utils.translation import ugettext_lazy as _ +from edx_rest_api_client.client import EdxRestApiClient +from edx_rest_api_client.exceptions import SlumberHttpBaseException +import requests from ecommerce.courses.utils import mode_for_seat from ecommerce.settings import get_lms_url - logger = logging.getLogger(__name__) @@ -37,31 +39,19 @@ def serialize_seat_for_commerce_api(self, seat): def _publish_creditcourse(self, course_id, access_token): """Creates or updates a CreditCourse object on the LMS.""" - url = get_lms_url('api/credit/v1/courses/') + + api = EdxRestApiClient( + get_lms_url('api/credit/v1/'), + oauth_access_token=access_token, + timeout=self.timeout + ) data = { 'course_key': course_id, 'enabled': True } - headers = { - 'Content-Type': 'application/json', - 'Authorization': 'Bearer ' + access_token - } - - kwargs = { - 'url': url, - 'data': json.dumps(data), - 'headers': headers, - 'timeout': self.timeout - } - response = requests.post(**kwargs) - if response.status_code == 400: - # The CreditCourse already exists. Try updating it. - kwargs['url'] += course_id.strip('/') + '/' - response = requests.put(**kwargs) - - return response + api.courses(course_id).put(data) def publish(self, course, access_token=None): """ Publish course commerce data to LMS. @@ -94,32 +84,23 @@ def publish(self, course, access_token=None): has_credit = 'credit' in [mode['name'] for mode in modes] if has_credit: - if access_token is not None: - try: - response = self._publish_creditcourse(course_id, access_token) - if response.status_code in (200, 201): - logger.info(u'Successfully published CreditCourse for [%s] to LMS.', course_id) - else: - # Note that %r is used to log the repr() of the response content, which may sometimes - # contain non-ASCII Unicode. %s would call str() on the response content, sometimes - # resulting in a UnicodeDecodeError. - logger.error( - u'Failed to publish CreditCourse for [%s] to LMS. Status was [%d]. Body was %r.', - course_id, - response.status_code, - response.content - ) - - return self._parse_error(response, error_message) - except Exception: # pylint: disable=broad-except - logger.exception(u'Failed to publish CreditCourse for [%s] to LMS.', course_id) - return error_message - else: - logger.error( - u'Unable to publish CreditCourse for [%s] to LMS. No access token available.', - course_id + try: + self._publish_creditcourse(course_id, access_token) + logger.info(u'Successfully published CreditCourse for [%s] to LMS.', course_id) + except SlumberHttpBaseException as e: + # Note that %r is used to log the repr() of the response content, which may sometimes + # contain non-ASCII Unicode. We don't know (or want to guess) the encoding, so using %r will log the + # raw bytes of the message, freeing us from the possibility of encoding errors. + logger.exception( + u'Failed to publish CreditCourse for [%s] to LMS. Status was [%d]. Body was %r.', + course_id, + e.response.status_code, + e.content ) return error_message + except Exception: # pylint: disable=broad-except + logger.exception(u'Failed to publish CreditCourse for [%s] to LMS.', course_id) + return error_message data = { 'id': course_id, diff --git a/ecommerce/courses/tests/factories.py b/ecommerce/courses/tests/factories.py index a213e6bf08c..042f7d117b7 100644 --- a/ecommerce/courses/tests/factories.py +++ b/ecommerce/courses/tests/factories.py @@ -1,4 +1,7 @@ +from __future__ import unicode_literals + import factory +from factory.fuzzy import FuzzyText from ecommerce.courses.models import Course @@ -6,3 +9,6 @@ class CourseFactory(factory.DjangoModelFactory): class Meta(object): model = Course + + id = FuzzyText(prefix='course-id-') + name = FuzzyText(prefix='course-name-') diff --git a/ecommerce/courses/tests/test_publishers.py b/ecommerce/courses/tests/test_publishers.py index a011800cfe3..54a7f88ee3c 100644 --- a/ecommerce/courses/tests/test_publishers.py +++ b/ecommerce/courses/tests/test_publishers.py @@ -41,28 +41,18 @@ def _mock_commerce_api(self, status, body=None): httpretty.register_uri(httpretty.PUT, url, status=status, body=json.dumps(body), content_type=JSON) - def _mock_credit_api(self, creation_status, update_status, body=None): + def mock_creditcourse_endpoint(self, course_id, status, body=None): self.assertTrue(httpretty.is_enabled, 'httpretty must be enabled to mock Credit API calls.') - url = get_lms_url('api/credit/v1/courses/') + url = get_lms_url('/api/credit/v1/courses/{}/'.format(course_id)) httpretty.register_uri( - httpretty.POST, + httpretty.PUT, url, - status=creation_status, + status=status, body=json.dumps(body), content_type=JSON ) - if update_status is not None: - url += self.course.id.strip('/') + '/' - httpretty.register_uri( - httpretty.PUT, - url, - status=update_status, - body=json.dumps(body), - content_type=JSON - ) - @ddt.data('', None) def test_commerce_api_url_not_set(self, setting_value): """ If the Commerce API is not setup, the method should log an INFO message and return """ @@ -71,12 +61,7 @@ def test_commerce_api_url_not_set(self, setting_value): response = self.publisher.publish(self.course) l.check((LOGGER_NAME, 'ERROR', 'COMMERCE_API_URL is not set. Commerce data will not be published!')) self.assertIsNotNone(response) - self.assertEqual( - response, - self.error_message.format( - course_id=self.course.id - ) - ) + self.assertEqual(response, self.error_message) def test_api_exception(self): """ If an exception is raised when communicating with the Commerce API, an ERROR message should be logged. """ @@ -196,91 +181,56 @@ def test_serialize_seat_for_commerce_api_with_professional(self, is_verified, ex } self.assertDictEqual(actual, expected) - @httpretty.activate - @ddt.data( - (201, None, 201), - (400, 200, 200) - ) - @ddt.unpack - def test_credit_publication_success(self, creation_status, update_status, commerce_status): + def attempt_credit_publication(self, api_status): """ - Verify that course publication succeeds if the Credit API responds - with 2xx status codes when publishing CreditCourse data to the LMS. - """ - self.course.create_or_update_seat('credit', True, 100, self.partner, credit_provider='Harvard', credit_hours=1) - - self._mock_credit_api(creation_status, update_status) - self._mock_commerce_api(commerce_status) + Sets up a credit seat and attempts to publish it to LMS. - access_token = 'access_token' - error_message = self.publisher.publish(self.course, access_token=access_token) - self.assertIsNone(error_message) - - # Retrieve the latest request to the Credit API. - if creation_status == 400: - latest_request = httpretty.httpretty.latest_requests[1] - else: - latest_request = httpretty.httpretty.latest_requests[0] - - # Verify the headers passed to the Credit API were correct. - expected = { - 'Content-Type': JSON, - 'Authorization': 'Bearer ' + access_token - } - self.assertDictContainsSubset(expected, latest_request.headers) - - # Verify the data passed to the Credit API was correct. - expected = { - 'course_key': self.course.id, - 'enabled': True - } - actual = json.loads(latest_request.body) - self.assertEqual(expected, actual) - - @httpretty.activate - @ddt.unpack - @ddt.data( - ({'non_field_errors': ['deadline issue']}, 'deadline issue'), - ('page not found', 'page not found'), - ({'detail': 'Authentication'}, 'Authentication'), - ({}, ''), - ) - def test_credit_publication_failure(self, error_message, expected_message): - """ - Verify that course publication fails if the Credit API does not respond - with 2xx status codes when publishing CreditCourse data to the LMS. - """ - self.course.create_or_update_seat('credit', True, 100, self.partner, credit_provider='Harvard', credit_hours=1) - - self._mock_credit_api(400, 418, error_message) - - response = self.publisher.publish(self.course, access_token='access_token') - self.assert_response_message(response, expected_message) - - def test_credit_publication_no_access_token(self): - """ - Verify that course publication fails if no access token is provided - when publishing CreditCourse data to the LMS. + Returns + String - Publish error message. """ - self.course.create_or_update_seat('credit', True, 100, self.partner, credit_provider='Harvard', credit_hours=1) + # Setup the course and mock the API endpoints + self.course.create_or_update_seat('credit', True, 100, self.partner, credit_provider='acme', credit_hours=1) + self.mock_creditcourse_endpoint(self.course.id, api_status) + self._mock_commerce_api(201) - response = self.publisher.publish(self.course, access_token=None) - self.assertIsNotNone(response) - self.assertEqual(self.error_message, response) + # Attempt to publish the course + return self.publisher.publish(self.course, access_token='access_token') - def test_credit_publication_exception(self): - """ - Verify that course publication fails if an exception is raised - while publishing CreditCourse data to the LMS. - """ - self.course.create_or_update_seat('credit', True, 100, self.partner, credit_provider='Harvard', credit_hours=1) + def assert_creditcourse_endpoint_called(self): + """ Verify the Credit API's CreditCourse endpoint was called. """ + last_request = httpretty.httpretty.latest_requests[0] + self.assertEqual(last_request.path, '/api/credit/v1/courses/{}/'.format(self.course.id)) - with mock.patch.object(LMSPublisher, '_publish_creditcourse') as mock_publish_creditcourse: - mock_publish_creditcourse.side_effect = Exception(self.error_message) + @httpretty.activate + def test_credit_publication_success(self): + """ Verify the endpoint returns successfully when credit publication succeeds. """ + error_message = self.attempt_credit_publication(201) + self.assertIsNone(error_message) + self.assert_creditcourse_endpoint_called() - response = self.publisher.publish(self.course, access_token='access_token') - self.assertIsNotNone(response) - self.assertEqual(self.error_message, response) + @httpretty.activate + def test_credit_publication_api_failure(self): + """ Verify the endpoint fails appropriately when Credit API calls return an error. """ + course_id = self.course.id + with LogCapture(LOGGER_NAME) as l: + status = 400 + actual = self.attempt_credit_publication(status) + + # Ensure the HTTP status and response are logged + expected_log = 'Failed to publish CreditCourse for [{course_id}] to LMS. ' \ + 'Status was [{status}]. Body was \'null\'.'.format(course_id=course_id, status=status) + l.check((LOGGER_NAME, 'ERROR', expected_log)) + + expected = 'Failed to publish commerce data for {} to LMS.'.format(course_id) + self.assertEqual(actual, expected) + self.assert_creditcourse_endpoint_called() + + @mock.patch('requests.get', mock.Mock(side_effect=Exception)) + def test_credit_publication_uncaught_exception(self): + """ Verify the endpoint fails appropriately when the Credit API fails unexpectedly. """ + actual = self.attempt_credit_publication(500) + expected = 'Failed to publish commerce data for {} to LMS.'.format(self.course.id) + self.assertEqual(actual, expected) def assert_response_message(self, api_response, expected_error_msg): self.assertIsNotNone(api_response) diff --git a/requirements/base.txt b/requirements/base.txt index ad96c237f1f..7c6c8467f5d 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -13,6 +13,7 @@ djangorestframework-jwt==1.7.2 drf-extensions==0.2.8 edx-auth-backends==0.1.3 edx-ecommerce-worker==0.2.0 +edx-rest-api-client==1.2.1 jsonfield==1.0.3 libsass==0.8.2 paypalrestsdk==1.9.0