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

@@ -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,

ecommerce/courses/tests/factories.py

@@ -1,8 +1,14 @@
+from __future__ import unicode_literals
+
 import factory
+from factory.fuzzy import FuzzyText
 
 from ecommerce.courses.models import Course
 
 
 class CourseFactory(factory.DjangoModelFactory):
     class Meta(object):
         model = Course
+
+    id = FuzzyText(prefix='course-id-')
+    name = FuzzyText(prefix='course-name-')

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)

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