diff --git a/lms/djangoapps/commerce/signals.py b/lms/djangoapps/commerce/signals.py index 52be46758d..0d54ab4e7d 100644 --- a/lms/djangoapps/commerce/signals.py +++ b/lms/djangoapps/commerce/signals.py @@ -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) diff --git a/lms/djangoapps/commerce/tests/test_signals.py b/lms/djangoapps/commerce/tests/test_signals.py index 8495f073e2..85eb7cb591 100644 --- a/lms/djangoapps/commerce/tests/test_signals.py +++ b/lms/djangoapps/commerce/tests/test_signals.py @@ -4,6 +4,8 @@ Tests for signal handling in commerce djangoapp. from django.test import TestCase from django.test.utils import override_settings +from course_modes.models import CourseMode +import ddt import mock from opaque_keys.edx.keys import CourseKey from student.models import UNENROLL_DONE @@ -14,6 +16,7 @@ from commerce.tests import TEST_PUBLIC_URL_ROOT, TEST_API_URL, TEST_API_SIGNING_ from commerce.tests.mocks import mock_create_refund +@ddt.ddt @override_settings( ECOMMERCE_PUBLIC_URL_ROOT=TEST_PUBLIC_URL_ROOT, ECOMMERCE_API_URL=TEST_API_URL, @@ -31,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) @@ -111,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') @@ -125,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): @@ -144,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):