From 6e85ec0739838d70508c3f1d9e765d28ed13960b Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Fri, 24 Oct 2014 15:47:01 +0000 Subject: [PATCH] Changing event suppression to signal suppression. Code review changes. Fix test and pylint. pylint error. Updating the test to verify the signal is fired. Test for all analytics events Import pylint error. Pylint error --- .../management/commands/transfer_students.py | 4 +- .../tests/test_transfer_students.py | 95 +++++++++++++++++-- common/djangoapps/student/models.py | 14 +-- lms/djangoapps/shoppingcart/models.py | 4 +- .../shoppingcart/tests/test_models.py | 18 ++++ 5 files changed, 118 insertions(+), 17 deletions(-) diff --git a/common/djangoapps/student/management/commands/transfer_students.py b/common/djangoapps/student/management/commands/transfer_students.py index 041402e19e..8e8557ff88 100644 --- a/common/djangoapps/student/management/commands/transfer_students.py +++ b/common/djangoapps/student/management/commands/transfer_students.py @@ -83,7 +83,7 @@ class Command(TrackedCommand): # Move the Student between the classes. mode = enrollment.mode old_is_active = enrollment.is_active - CourseEnrollment.unenroll(user, source_key, emit_unenrollment_event=False) + CourseEnrollment.unenroll(user, source_key, skip_refund=True) print(u"Unenrolled {} from {}".format(user.username, unicode(source_key))) for dest_key in dest_keys: @@ -98,7 +98,7 @@ class Command(TrackedCommand): # Un-enroll from the new course if the user had un-enrolled # form the old course. if not old_is_active: - new_enrollment.update_enrollment(is_active=False, emit_unenrollment_event=False) + new_enrollment.update_enrollment(is_active=False, skip_refund=True) if transfer_certificates: self._transfer_certificate_item(source_key, enrollment, user, dest_keys, new_enrollment) diff --git a/common/djangoapps/student/management/tests/test_transfer_students.py b/common/djangoapps/student/management/tests/test_transfer_students.py index caebeeace2..45376eb148 100644 --- a/common/djangoapps/student/management/tests/test_transfer_students.py +++ b/common/djangoapps/student/management/tests/test_transfer_students.py @@ -2,11 +2,16 @@ Tests the transfer student management command """ from django.conf import settings +from mock import patch, call from opaque_keys.edx import locator import unittest import ddt + +from shoppingcart.models import Order, CertificateItem # pylint: disable=F0401 +from course_modes.models import CourseMode from student.management.commands import transfer_students -from student.models import CourseEnrollment +from student.models import CourseEnrollment, UNENROLL_DONE, EVENT_NAME_ENROLLMENT_DEACTIVATED, \ + EVENT_NAME_ENROLLMENT_ACTIVATED, EVENT_NAME_ENROLLMENT_MODE_CHANGED from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -18,18 +23,40 @@ class TestTransferStudents(ModuleStoreTestCase): """Tests for transferring students between courses.""" PASSWORD = 'test' + signal_fired = False + + def setUp(self, **kwargs): + """Connect a stub receiver, and analytics event tracking.""" + UNENROLL_DONE.connect(self.assert_unenroll_signal) + patcher = patch('student.models.tracker') + self.mock_tracker = patcher.start() + self.addCleanup(patcher.stop) + + def tearDown(self): + """Disconnects the UNENROLL stub receiver.""" + UNENROLL_DONE.disconnect(self.assert_unenroll_signal) + + def assert_unenroll_signal(self, skip_refund=False, **kwargs): # pylint: disable=W0613 + """ Signal Receiver stub for testing that the unenroll signal was fired. """ + self.assertFalse(self.signal_fired) + self.assertTrue(skip_refund) + self.signal_fired = True def test_transfer_students(self): - student = UserFactory() + """ Verify the transfer student command works as intended. """ + student = UserFactory.create() student.set_password(self.PASSWORD) # pylint: disable=E1101 student.save() # pylint: disable=E1101 - + mode = 'verified' # Original Course original_course_location = locator.CourseLocator('Org0', 'Course0', 'Run0') course = self._create_course(original_course_location) # Enroll the student in 'verified' CourseEnrollment.enroll(student, course.id, mode="verified") + # Create and purchase a verified cert for the original course. + self._create_and_purchase_verified(student, course.id) + # New Course 1 course_location_one = locator.CourseLocator('Org1', 'Course1', 'Run1') new_course_one = self._create_course(course_location_one) @@ -45,11 +72,55 @@ class TestTransferStudents(ModuleStoreTestCase): transfer_students.Command().handle( source_course=original_key, dest_course_list=new_key_one + "," + new_key_two ) + self.assertTrue(self.signal_fired) + + # Confirm the analytics event was emitted. + self.mock_tracker.emit.assert_has_calls( # pylint: disable=E1103 + [ + call( + EVENT_NAME_ENROLLMENT_ACTIVATED, + {'course_id': original_key, 'user_id': student.id, 'mode': mode} + ), + call( + EVENT_NAME_ENROLLMENT_MODE_CHANGED, + {'course_id': original_key, 'user_id': student.id, 'mode': mode} + ), + call( + EVENT_NAME_ENROLLMENT_DEACTIVATED, + {'course_id': original_key, 'user_id': student.id, 'mode': mode} + ), + call( + EVENT_NAME_ENROLLMENT_ACTIVATED, + {'course_id': new_key_one, 'user_id': student.id, 'mode': mode} + ), + call( + EVENT_NAME_ENROLLMENT_MODE_CHANGED, + {'course_id': new_key_one, 'user_id': student.id, 'mode': mode} + ), + call( + EVENT_NAME_ENROLLMENT_ACTIVATED, + {'course_id': new_key_two, 'user_id': student.id, 'mode': mode} + ), + call( + EVENT_NAME_ENROLLMENT_MODE_CHANGED, + {'course_id': new_key_two, 'user_id': student.id, 'mode': mode} + ) + ] + ) + self.mock_tracker.reset_mock() # Confirm the enrollment mode is verified on the new courses, and enrollment is enabled as appropriate. - self.assertEquals(('verified', False), CourseEnrollment.enrollment_mode_for_user(student, course.id)) - self.assertEquals(('verified', True), CourseEnrollment.enrollment_mode_for_user(student, new_course_one.id)) - self.assertEquals(('verified', True), CourseEnrollment.enrollment_mode_for_user(student, new_course_two.id)) + self.assertEquals((mode, False), CourseEnrollment.enrollment_mode_for_user(student, course.id)) + self.assertEquals((mode, True), CourseEnrollment.enrollment_mode_for_user(student, new_course_one.id)) + self.assertEquals((mode, True), CourseEnrollment.enrollment_mode_for_user(student, new_course_two.id)) + + # Confirm the student has not be refunded. + target_certs = CertificateItem.objects.filter( + course_id=course.id, user_id=student, status='purchased', mode=mode + ) + self.assertTrue(target_certs[0]) + self.assertFalse(target_certs[0].refund_requested_time) + self.assertEquals(target_certs[0].order.status, 'purchased') def _create_course(self, course_location): """ Creates a course """ @@ -58,3 +129,15 @@ class TestTransferStudents(ModuleStoreTestCase): number=course_location.course, run=course_location.run ) + + def _create_and_purchase_verified(self, student, course_id): + """ Creates a verified mode for the course and purchases it for the student. """ + course_mode = CourseMode(course_id=course_id, + mode_slug="verified", + mode_display_name="verified cert", + min_price=50) + course_mode.save() + # When there is no expiration date on a verified mode, the user can always get a refund + cart = Order.get_cart_for_user(user=student) + CertificateItem.add_to_order(cart, course_id, 50, 'verified') + cart.purchase() diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index dda87a80e5..311becddbc 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -56,7 +56,7 @@ from ratelimitbackend import admin import analytics -UNENROLL_DONE = Signal(providing_args=["course_enrollment"]) +UNENROLL_DONE = Signal(providing_args=["course_enrollment", "skip_refund"]) log = logging.getLogger(__name__) AUDIT_LOG = logging.getLogger("audit") SessionStore = import_module(settings.SESSION_ENGINE).SessionStore # pylint: disable=invalid-name @@ -776,7 +776,7 @@ class CourseEnrollment(models.Model): is_course_full = cls.num_enrolled_in(course.id) >= course.max_student_enrollments_allowed return is_course_full - def update_enrollment(self, mode=None, is_active=None, emit_unenrollment_event=True): + def update_enrollment(self, mode=None, is_active=None, skip_refund=False): """ Updates an enrollment for a user in a class. This includes options like changing the mode, toggling is_active True/False, etc. @@ -814,8 +814,8 @@ class CourseEnrollment(models.Model): u"mode:{}".format(self.mode)] ) - elif emit_unenrollment_event: - UNENROLL_DONE.send(sender=None, course_enrollment=self) + else: + UNENROLL_DONE.send(sender=None, course_enrollment=self, skip_refund=skip_refund) self.emit_event(EVENT_NAME_ENROLLMENT_DEACTIVATED) @@ -988,7 +988,7 @@ class CourseEnrollment(models.Model): raise @classmethod - def unenroll(cls, user, course_id, emit_unenrollment_event=True): + def unenroll(cls, user, course_id, skip_refund=False): """ Remove the user from a given course. If the relevant `CourseEnrollment` object doesn't exist, we log an error but don't throw an exception. @@ -999,11 +999,11 @@ class CourseEnrollment(models.Model): `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) - `emit_unenrollment_events` can be set to False to suppress events firing. + `skip_refund` can be set to True to avoid the refund process. """ try: record = CourseEnrollment.objects.get(user=user, course_id=course_id) - record.update_enrollment(is_active=False, emit_unenrollment_event=emit_unenrollment_event) + record.update_enrollment(is_active=False, skip_refund=skip_refund) except cls.DoesNotExist: err_msg = u"Tried to unenroll student {} from {} but they were not enrolled" diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 37a32dfe0c..d06624b2c1 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -1038,7 +1038,7 @@ class CertificateItem(OrderItem): mode = models.SlugField() @receiver(UNENROLL_DONE) - def refund_cert_callback(sender, course_enrollment=None, **kwargs): + def refund_cert_callback(sender, course_enrollment=None, skip_refund=False, **kwargs): # pylint: disable=E0213,W0613 """ When a CourseEnrollment object calls its unenroll method, this function checks to see if that unenrollment occurred in a verified certificate that was within the refund deadline. If so, it actually performs the @@ -1048,7 +1048,7 @@ class CertificateItem(OrderItem): """ # Only refund verified cert unenrollments that are within bounds of the expiration date - if not course_enrollment.refundable(): + if (not course_enrollment.refundable()) or skip_refund: return target_certs = CertificateItem.objects.filter(course_id=course_enrollment.course_id, user_id=course_enrollment.user, status='purchased', mode='verified') diff --git a/lms/djangoapps/shoppingcart/tests/test_models.py b/lms/djangoapps/shoppingcart/tests/test_models.py index d2d992b89f..15385062be 100644 --- a/lms/djangoapps/shoppingcart/tests/test_models.py +++ b/lms/djangoapps/shoppingcart/tests/test_models.py @@ -495,6 +495,24 @@ class CertificateItemTest(ModuleStoreTestCase): self.assertTrue(target_certs[0].refund_requested_time) self.assertEquals(target_certs[0].order.status, 'refunded') + def test_no_refund_on_cert_callback(self): + # If we explicitly skip refunds, the unenroll action should not modify the purchase. + CourseEnrollment.enroll(self.user, self.course_key, 'verified') + cart = Order.get_cart_for_user(user=self.user) + CertificateItem.add_to_order(cart, self.course_key, self.cost, 'verified') + cart.purchase() + + CourseEnrollment.unenroll(self.user, self.course_key, skip_refund=True) + target_certs = CertificateItem.objects.filter( + course_id=self.course_key, + user_id=self.user, + status='purchased', + mode='verified' + ) + self.assertTrue(target_certs[0]) + self.assertFalse(target_certs[0].refund_requested_time) + self.assertEquals(target_certs[0].order.status, 'purchased') + def test_refund_cert_callback_before_expiration(self): # If the expiration date has not yet passed on a verified mode, the user can be refunded many_days = datetime.timedelta(days=60)