Merge pull request #404 from edx/clintonb/cat-credit-fix

WAIT-LMS: Updated CreditCourse creation

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