Merge pull request #8255 from edx/patch/2015-05-29

patch/2015-05-29

lms/djangoapps/commerce/signals.py

@@ -2,7 +2,7 @@
 Signal handling functions for use with external commerce service.
 """
 import logging
-from urlparse import urlparse
+from urlparse import urljoin
 
 from django.conf import settings
 from django.core.mail import EmailMultiAlternatives
@@ -39,7 +39,7 @@ def handle_unenroll_done(sender, course_enrollment=None, skip_refund=False, **kw
             # trapping the Exception and logging an error.
             log.exception(
                 "Unexpected exception while attempting to initiate refund for user [%s], course [%s]",
-                course_enrollment.user,
+                course_enrollment.user.id,
                 course_enrollment.course_id,
             )
 
@@ -90,7 +90,7 @@ def refund_seat(course_enrollment, request_user):
             # 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, unenrolled_user, course_key_str)
+                        "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
@@ -104,11 +104,27 @@ def refund_seat(course_enrollment, request_user):
             course_key_str,
             refund_ids,
         )
-        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)
+
+        # 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, 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)
     else:
         # no refundable orders were found.
         log.debug("No refund opened for user [%s], course [%s]", unenrolled_user.id, course_key_str)
@@ -131,14 +147,14 @@ def send_refund_notification(course_enrollment, refund_ids):
     for_user = course_enrollment.user
     subject = _("[Refund] User-Requested Refund")
     message = _(
-        "A refund request has been initiated for {username} ({email}). To process this request, please visit the link(s) below."  # pylint: disable=line-too-long
+        "A refund request has been initiated for {username} ({email}). "
+        "To process this request, please visit the link(s) below."
     ).format(username=for_user.username, email=for_user.email)
 
-    # TODO: this manipulation of API url is temporary, pending the introduction
-    # of separate configuration items for the service's base url and api path.
-    ecommerce_url = '://'.join(urlparse(settings.ECOMMERCE_API_URL)[:2])
-
-    refund_urls = ["{}/dashboard/refunds/{}/".format(ecommerce_url, refund_id) for refund_id in refund_ids]
+    refund_urls = [
+        urljoin(settings.ECOMMERCE_PUBLIC_URL_ROOT, '/dashboard/refunds/{}/'.format(refund_id))
+        for refund_id in refund_ids
+    ]
     text_body = '\r\n'.join([message] + refund_urls + [''])
     refund_links = ['<a href="{0}">{0}</a>'.format(url) for url in refund_urls]
     html_body = '<p>{}</p>'.format('<br>'.join([message] + refund_links))

lms/djangoapps/commerce/tests/__init__.py

@@ -12,7 +12,8 @@ from commerce import ecommerce_api_client
 from student.tests.factories import UserFactory
 
 
-TEST_API_URL = 'http://example.com/api'
+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'

lms/djangoapps/commerce/tests/test_signals.py

@@ -3,23 +3,25 @@ Tests for signal handling in commerce djangoapp.
 """
 from django.test import TestCase
 from django.test.utils import override_settings
-from urlparse import urlparse
 
+from course_modes.models import CourseMode
+import ddt
 import mock
 from opaque_keys.edx.keys import CourseKey
 from student.models import UNENROLL_DONE
 from student.tests.factories import UserFactory, CourseEnrollmentFactory
 
 from commerce.signals import refund_seat, send_refund_notification
-from commerce.tests import TEST_API_URL, TEST_API_SIGNING_KEY
+from commerce.tests import TEST_PUBLIC_URL_ROOT, TEST_API_URL, TEST_API_SIGNING_KEY
 from commerce.tests.mocks import mock_create_refund
 
 
-# TODO: this is temporary.  See the corresponding TODO in signals.send_refund_notification
-TEST_BASE_URL = '://'.join(urlparse(TEST_API_URL)[:2])
-
-
-@override_settings(ECOMMERCE_API_URL=TEST_API_URL, ECOMMERCE_API_SIGNING_KEY=TEST_API_SIGNING_KEY)
+@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,
+)
 class TestRefundSignal(TestCase):
     """
     Exercises logic triggered by the UNENROLL_DONE signal.
@@ -32,6 +34,7 @@ class TestRefundSignal(TestCase):
         self.course_enrollment = CourseEnrollmentFactory(
             user=self.student,
             course_id=CourseKey.from_string('course-v1:org+course+run'),
+            mode=CourseMode.VERIFIED,
         )
         self.course_enrollment.refundable = mock.Mock(return_value=True)
 
@@ -42,7 +45,11 @@ class TestRefundSignal(TestCase):
         """
         UNENROLL_DONE.send(sender=None, course_enrollment=self.course_enrollment, skip_refund=skip_refund)
 
-    @override_settings(ECOMMERCE_API_URL=None, ECOMMERCE_API_SIGNING_KEY=None)
+    @override_settings(
+        ECOMMERCE_PUBLIC_URL_ROOT=None,
+        ECOMMERCE_API_URL=None,
+        ECOMMERCE_API_SIGNING_KEY=None,
+    )
     def test_no_service(self):
         """
         Ensure that the receiver quietly bypasses attempts to initiate
@@ -108,7 +115,7 @@ class TestRefundSignal(TestCase):
         Ensure that expected authorization issues are logged as warnings.
         """
         with mock_create_refund(status=403):
-            refund_seat(self.course_enrollment, None)
+            refund_seat(self.course_enrollment, UserFactory())
             self.assertTrue(mock_log_warning.called)
 
     @mock.patch('commerce.signals.log.exception')
@@ -122,14 +129,14 @@ class TestRefundSignal(TestCase):
             self.assertTrue(mock_log_exception.called)
 
     @mock.patch('commerce.signals.send_refund_notification')
-    def test_notification(self, mock_send_notificaton):
+    def test_notification(self, mock_send_notification):
         """
         Ensure the notification function is triggered when refunds are
         initiated
         """
         with mock_create_refund(status=200, response=[1, 2, 3]):
             self.send_signal()
-            self.assertTrue(mock_send_notificaton.called)
+            self.assertTrue(mock_send_notification.called)
 
     @mock.patch('commerce.signals.send_refund_notification')
     def test_notification_no_refund(self, mock_send_notification):
@@ -141,6 +148,27 @@ class TestRefundSignal(TestCase):
             self.send_signal()
             self.assertFalse(mock_send_notification.called)
 
+    @mock.patch('commerce.signals.send_refund_notification')
+    @ddt.data(
+        CourseMode.HONOR,
+        CourseMode.PROFESSIONAL,
+        CourseMode.AUDIT,
+        CourseMode.NO_ID_PROFESSIONAL_MODE,
+        CourseMode.CREDIT_MODE,
+    )
+    def test_notification_not_verified(self, mode, mock_send_notification):
+        """
+        Ensure the notification function is NOT triggered when the
+        unenrollment is for any mode other than verified (i.e. any mode other
+        than one for which refunds are presently supported).  See the
+        TODO associated with XCOM-371 in the signals module in the commerce
+        package for more information.
+        """
+        self.course_enrollment.mode = mode
+        with mock_create_refund(status=200, response=[1, 2, 3]):
+            self.send_signal()
+            self.assertFalse(mock_send_notification.called)
+
     @mock.patch('commerce.signals.send_refund_notification', side_effect=Exception("Splat!"))
     @mock.patch('commerce.signals.log.warning')
     def test_notification_error(self, mock_log_warning, mock_send_notification):
@@ -185,14 +213,15 @@ class TestRefundSignal(TestCase):
         )
         text_body = mock_email_class.call_args[0][1]
         # check for a URL for each refund
-        for exp in [r'{0}/dashboard/refunds/{1}/'.format(TEST_BASE_URL, refund_id) for refund_id in refund_ids]:
+        for exp in [r'{0}/dashboard/refunds/{1}/'.format(TEST_PUBLIC_URL_ROOT, refund_id)
+                    for refund_id in refund_ids]:
             self.assertRegexpMatches(text_body, exp)
 
         # check HTML content
         self.assertEqual(mock_message.attach_alternative.call_args[0], (mock.ANY, "text/html"))
         html_body = mock_message.attach_alternative.call_args[0][0]
         # check for a link to each refund
-        for exp in [r'a href="{0}/dashboard/refunds/{1}/"'.format(TEST_BASE_URL, refund_id)
+        for exp in [r'a href="{0}/dashboard/refunds/{1}/"'.format(TEST_PUBLIC_URL_ROOT, refund_id)
                     for refund_id in refund_ids]:
             self.assertRegexpMatches(html_body, exp)
 

lms/envs/aws.py

@@ -583,6 +583,7 @@ CDN_VIDEO_URLS = ENV_TOKENS.get('CDN_VIDEO_URLS', CDN_VIDEO_URLS)
 ONLOAD_BEACON_SAMPLE_RATE = ENV_TOKENS.get('ONLOAD_BEACON_SAMPLE_RATE', ONLOAD_BEACON_SAMPLE_RATE)
 
 ##### 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)

lms/envs/common.py

@@ -2317,6 +2317,7 @@ 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