From ff3232a6862a7a9f37b2261921002570387313eb Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Fri, 23 Oct 2015 17:05:26 -0400 Subject: [PATCH] Updated CreditCourse creation The credit endpoint has been updated to use PUT to create new CreditCourse objects. The publisher has been updated to use this method, greatly simplifying the code necessary to create these objects. ECOM-2524 --- ecommerce/courses/publishers.py | 71 ++++------ ecommerce/courses/tests/factories.py | 6 + ecommerce/courses/tests/test_publishers.py | 146 +++++++-------------- requirements/base.txt | 1 + 4 files changed, 81 insertions(+), 143 deletions(-) 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