Response to Dave & Diana's CR

common/djangoapps/course_modes/models.py

@@ -11,7 +11,6 @@ from django.db.models import Q
 
 Mode = namedtuple('Mode', ['slug', 'name', 'min_price', 'suggested_prices', 'currency', 'expiration_date'])
 
-
 class CourseMode(models.Model):
     """
     We would like to offer a course in a variety of modes.
@@ -72,8 +71,8 @@ class CourseMode(models.Model):
     @classmethod
     def modes_for_course_dict(cls, course_id):
         """
-        Returns the modes for a particular course as a dictionary with
-        the mode slug as the key
+        Returns the non-expired modes for a particular course as a
+        dictionary with the mode slug as the key
         """
         return {mode.slug: mode for mode in cls.modes_for_course(course_id)}
 
@@ -82,6 +81,8 @@ class CourseMode(models.Model):
         """
         Returns the mode for the course corresponding to mode_slug.
 
+        Returns only non-expired modes.
+
         If this particular mode is not set for the course, returns None
         """
         modes = cls.modes_for_course(course_id)
@@ -95,7 +96,8 @@ class CourseMode(models.Model):
     @classmethod
     def min_course_price_for_currency(cls, course_id, currency):
         """
-        Returns the minimum price of the course in the appropriate currency over all the course's modes.
+        Returns the minimum price of the course in the appropriate currency over all the course's
+        non-expired modes.
         If there is no mode found, will return the price of DEFAULT_MODE, which is 0
         """
         modes = cls.modes_for_course(course_id)

common/djangoapps/course_modes/tests/factories.py

@@ -11,3 +11,4 @@ class CourseModeFactory(DjangoModelFactory):
     mode_display_name = 'audit course'
     min_price = 0
     currency = 'usd'
+    expiration_date = None

common/djangoapps/student/models.py

@@ -23,14 +23,13 @@ from django.contrib.auth.signals import user_logged_in, user_logged_out
 from django.db import models
 from django.db.models.signals import post_save
 from django.dispatch import receiver
+import django.dispatch
 from django.forms import ModelForm, forms
 
 import comment_client as cc
 from pytz import UTC
 
-import django.dispatch
-
-verified_unenroll_done = django.dispatch.Signal(providing_args=["user", "user_email", "course_id"])
+unenroll_done = django.dispatch.Signal(providing_args=["course_enrollment"])
 
 log = logging.getLogger(__name__)
 AUDIT_LOG = logging.getLogger("audit")
@@ -823,10 +822,12 @@ class CourseEnrollment(models.Model):
 
         `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall)
         """
+        refund_error = "Refund Error"
         try:
             record = CourseEnrollment.objects.get(user=user, course_id=course_id)
             record.is_active = False
             record.save()
+            unenroll_done.send(sender=cls, course_enrollment=record)
         except cls.DoesNotExist:
             err_msg = u"Tried to unenroll student {} from {} but they were not enrolled"
             log.error(err_msg.format(user, course_id))

common/djangoapps/student/tests/tests.py

@@ -36,8 +36,6 @@ from student.tests.test_email import mock_render_to_string
 
 import shoppingcart
 
-from course_modes.models import CourseMode
-
 COURSE_1 = 'edX/toy/2012_Fall'
 COURSE_2 = 'edx/full/6.002_Spring_2012'
 
@@ -424,50 +422,3 @@ class PaidRegistrationTest(ModuleStoreTestCase):
         self.assertEqual(response.content, reverse('shoppingcart.views.show_cart'))
         self.assertTrue(shoppingcart.models.PaidCourseRegistration.contained_in_order(
             shoppingcart.models.Order.get_cart_for_user(self.user), self.course.id))
-
-
-@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
-class RefundUnenrollmentTests(ModuleStoreTestCase):
-    """
-    Tests views for unenrollment with refunds
-    """
-    # test data
-    COURSE_SLUG = "100"
-    COURSE_NAME = "test_course"
-    COURSE_ORG = "EDX"
-
-    def setUp(self):
-        # Create course, user, and enroll them as a verified student
-        self.user = UserFactory.create()
-        self.course_id = "org/test/Test_Course"
-        self.cost = 40
-        CourseFactory.create(org='org', number='test', run='course', display_name='Test Course')
-        course_mode = CourseMode(course_id=self.course_id,
-                                 mode_slug="honor",
-                                 mode_display_name="honor cert",
-                                 min_price=self.cost)
-        course_mode.save()
-        course_mode = CourseMode(course_id=self.course_id,
-                                 mode_slug="verified",
-                                 mode_display_name="verified cert",
-                                 min_price=self.cost)
-        course_mode.save()
-
-        self.req_factory = RequestFactory()
-
-        course_enrollment = CourseEnrollment.create_enrollment(self.user, self.course_id, 'verified', is_active=True)
-        course_enrollment.save()
-
-    # Student is verified and paid; we should be able to refund them
-    def test_unenroll_and_refund(self):
-        request = self.req_factory.post(reverse('change_enrollment'), {'course_id': self.course_id, 'enrollment_action': 'unenroll'})
-        request.user = self.user
-        response = change_enrollment(request)
-        self.assertEqual(response.status_code, 200)
-        self.assertFalse(CourseEnrollment.is_enrolled(self.user, self.course_id))
-
-    def test_unenroll_but_no_course(self):
-        request = self.req_factory.post(reverse('change_enrollment'), {'course_id': 'non/existent/course', 'enrollment_action': 'unenroll'})
-        request.user = self.user
-        response = change_enrollment(request)
-        self.assertEqual(response.status_code, 400)

common/djangoapps/student/views.py

@@ -43,7 +43,6 @@ from student.models import (
     TestCenterRegistration, TestCenterRegistrationForm, PendingNameChange,
     PendingEmailChange, CourseEnrollment, unique_id_for_user,
     get_testcenter_registration, CourseEnrollmentAllowed, UserStanding,
-    verified_unenroll_done
 )
 from student.forms import PasswordResetFormNoActive
 
@@ -472,29 +471,18 @@ def change_enrollment(request):
 
     elif action == "unenroll":
         try:
-            course = course_from_id(course_id)
-            enrollment_mode = CourseEnrollment.enrollment_mode_for_user(user, course_id)
-
-            # did they sign up for verified certs?
-            if(enrollment_mode == 'verified'):
-                # If the user is allowed a refund, do so
-                if has_access(user, course, 'refund'):
-                    # triggers the callback to mark the certificate as refunded
-                    verified_unenroll_done.send(sender=request, user=user, user_email=user.email, course_id=course_id)
-            CourseEnrollment.unenroll(user, course_id)
-            org, course_num, run = course_id.split("/")
-            dog_stats_api.increment(
-                "common.student.unenrollment",
-                tags=["org:{0}".format(org),
-                      "course:{0}".format(course_num),
-                      "run:{0}".format(run)]
-            )
-            return HttpResponse()
+            CourseEnrollment.enrollment_mode_for_user(user, course_id)
         except CourseEnrollment.DoesNotExist:
             return HttpResponseBadRequest(_("You are not enrolled in this course"))
-        except ItemNotFoundError:
-            log.warning("User {0} tried to unenroll from non-existent course {1}".format(user.username, course_id))
-            return HttpResponseBadRequest(_("Course id is invalid"))
+        CourseEnrollment.unenroll(user, course_id)
+        org, course_num, run = course_id.split("/")
+        dog_stats_api.increment(
+            "common.student.unenrollment",
+            tags=["org:{0}".format(org),
+                  "course:{0}".format(course_num),
+                  "run:{0}".format(run)]
+        )
+        return HttpResponse()
     else:
         return HttpResponseBadRequest(_("Enrollment action is invalid"))
 
@@ -909,7 +897,7 @@ def create_account(request, post_override=None):
     subject = ''.join(subject.splitlines())
     message = render_to_string('emails/activation_email.txt', d)
 
-    # dont send email if we are doing load testing or random user generation for some reason
+    # don't send email if we are doing load testing or random user generation for some reason
     if not (settings.MITX_FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING')):
         try:
             if settings.MITX_FEATURES.get('REROUTE_ACTIVATION_EMAIL'):

lms/djangoapps/courseware/access.py

@@ -14,6 +14,7 @@ from xmodule.modulestore import Location
 from xmodule.x_module import XModule, XModuleDescriptor
 
 from student.models import CourseEnrollmentAllowed
+from course_modes.models import CourseMode
 from external_auth.models import ExternalAuthMap
 from courseware.masquerade import is_masquerading_as_student
 from django.utils.timezone import UTC
@@ -209,18 +210,11 @@ def _has_access_course_desc(user, course, action):
         or not that deadline has passed; checking whether the student actually *purchased*
         a paid/verified certificate must be done elsewhere.
         """
-        now = datetime.now(UTC())
-        course_start = course.enrollment_start
-        # If there *is* no start date, user can be refunded
-        if course_start is None:
-            return True
-        # Presently, refunds are only allowed up to two weeks after the course
-        # start date.
-        grace_period = timedelta(days=14)
-        refund_end = course_start + grace_period
-        if (now.date() <= refund_end.date()):
+        course_mode = CourseMode.mode_for_course(course.id, 'verified')
+        if course_mode is None:
+            return False
+        else:
             return True
-        return False
 
 
     checkers = {

lms/djangoapps/courseware/tests/test_access.py

@@ -12,10 +12,14 @@ from django.utils.timezone import UTC
 
 from student.tests.factories import UserFactory
 from xmodule.modulestore.tests.factories import CourseFactory
+from course_modes.tests.factories import CourseModeFactory
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class AccessTestCase(TestCase):
+    """
+    Tests for the various access controls on the student dashboard
+    """
     def test__has_global_staff_access(self):
         u = Mock(is_staff=False)
         self.assertFalse(access._has_global_staff_access(u))
@@ -114,19 +118,26 @@ class AccessTestCase(TestCase):
         # Non-staff cannot enroll outside the open enrollment period if not specifically allowed
 
     def test__has_access_refund(self):
-        user = UserFactory.create()
-        course = CourseFactory.create(org='org', number='test', run='course', display_name='Test Course')
         today = datetime.datetime.now(UTC())
-        grace_period = datetime.timedelta(days=14)
         one_day_extra = datetime.timedelta(days=1)
+        user = UserFactory.create()
+
+        course_nonrefundable_id = 'nonrefundable/test/Test_Course'
+        course_nonrefundable = CourseFactory.create(org='nonrefundable', number='test', run='course', display_name='Test Course')
+        course_mode_nonrefundable = CourseModeFactory.create(course_id=course_nonrefundable_id,
+                                                             mode_slug='verified',
+                                                             expiration_date=(today - one_day_extra))
+        course_mode_nonrefundable.save()
 
-        # User is allowed to receive refund if it is within two weeks of course start date
-        course.enrollment_start = (today - one_day_extra)
-        self.assertTrue(access._has_access_course_desc(user, course, 'refund'))
+        course_refundable_id = 'refundable/test/Test_Course'
+        course_refundable = CourseFactory.create(org='refundable', number='test', run='course', display_name='Test Course')
+        course_mode_refundable = CourseModeFactory.create(course_id=course_refundable_id,
+                                                          mode_slug='verified',
+                                                          expiration_date=(today + one_day_extra))
+        course_mode_refundable.save()
 
-        course.enrollment_start = (today - grace_period)
-        self.assertTrue(access._has_access_course_desc(user, course, 'refund'))
+        # User cannot receive a refund one day after the expiration date
+        self.assertFalse(access._has_access_course_desc(user, course_nonrefundable, 'refund'))
 
         # After two weeks, user may no longer receive a refund
-        course.enrollment_start = (today - grace_period - one_day_extra)
-        self.assertFalse(access._has_access_course_desc(user, course, 'refund'))
+        self.assertTrue(access._has_access_course_desc(user, course_refundable, 'refund'))

lms/djangoapps/shoppingcart/models.py

@@ -8,8 +8,6 @@ from collections import namedtuple
 from boto.exception import BotoServerError  # this is a super-class of SESError and catches connection errors
 
 from django.dispatch import receiver
-from student.models import verified_unenroll_done
-
 from django.db import models
 from django.conf import settings
 from django.core.exceptions import ObjectDoesNotExist
@@ -26,7 +24,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 from course_modes.models import CourseMode
 from mitxmako.shortcuts import render_to_string
 from student.views import course_from_id
-from student.models import CourseEnrollment
+from student.models import CourseEnrollment, unenroll_done
 
 from verify_student.models import SoftwareSecurePhotoVerification
 
@@ -402,38 +400,48 @@ class CertificateItem(OrderItem):
     course_enrollment = models.ForeignKey(CourseEnrollment)
     mode = models.SlugField()
 
-    @classmethod
-    @receiver(verified_unenroll_done)
+    @receiver(unenroll_done, sender=CourseEnrollment)
     def refund_cert_callback(sender, **kwargs):
         """
-        When a CourseEnrollment object whose mode is 'verified' has its is_active field set to false (i.e. when a student
-        is unenrolled), this callback ensures that the associated CertificateItem is marked as refunded, and that an
-        appropriate email is sent to billing.
+        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
+        refund.
+
+        Returns the refunded certificate on a successful refund.  If no refund is necessary, it returns nothing.
+        If an error that the user should see occurs, it returns a string specifying the error.
         """
-        try:
-            course_id = kwargs['course_id']
-            user = kwargs['user']
+        mode = kwargs['course_enrollment'].mode
+        course_id = kwargs['course_enrollment'].course_id
+        user = kwargs['course_enrollment'].user
 
-            # If there's duplicate entries, just grab the first one and refund it (though in most cases we should only get one)
-            target_certs = CertificateItem.objects.filter(course_id=course_id, user_id=user, status='purchased', mode='verified')
-            target_cert = target_certs[0]
-            target_cert.status = 'refunded'
-            target_cert.save()
+        # Only refund verified cert unenrollments that are within bounds of the expiration date
+        if (mode != 'verified'):
+            return
 
-            order_number = target_cert.order_id
+        if (CourseMode.mode_for_course(course_id, 'verified') is None):
+            return
 
-            # send billing an email so they can handle refunding
-            subject = _("[Refund] User-Requested Refund")
-            message = "User " + str(user) + "(" + str(user.email) + ") has requested a refund on Order #" + str(order_number) + "."
-            to_email = [settings.PAYMENT_SUPPORT_EMAIL]
-            from_email = "support@edx.org"
+        target_certs = CertificateItem.objects.filter(course_id=course_id, user_id=user, status='purchased', mode='verified')
+        try:
+            target_cert = target_certs[0]
+        except IndexError:
+            log.error("Matching CertificateItem not found while trying to refund.  User %s, Course %s", user, course_id)
+            return
+        target_cert.status = 'refunded'
+        target_cert.save()
+
+        order_number = target_cert.order_id
+        # send billing an email so they can handle refunding
+        subject = _("[Refund] User-Requested Refund")
+        message = "User {user} ({user_email}) has requested a refund on Order #{order_number}.".format(user=user, user_email=user.email, order_number=order_number)
+        to_email = [settings.PAYMENT_SUPPORT_EMAIL]
+        from_email = [settings.PAYMENT_SUPPORT_EMAIL]
+        try:
             send_mail(subject, message, from_email, to_email, fail_silently=False)
+        except (smtplib.SMTPException, BotoServerError):
+            log.error('Failed sending email to billing request a refund for verified certiciate (User %s, Course %s)', user, course_id)
 
-            return target_cert
-
-        except IndexError:
-            log.exception("Matching CertificateItem not found while trying to refund.  User %s, Course %s", user, course_id)
-            raise IndexError
+        return target_cert
 
     @classmethod
     @transaction.commit_on_success

lms/djangoapps/shoppingcart/tests/test_models.py

@@ -17,9 +17,11 @@ from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
 from shoppingcart.models import (Order, OrderItem, CertificateItem, InvalidCartItem, PaidCourseRegistration,
                                  OrderItemSubclassPK)
 from student.tests.factories import UserFactory
-from student.models import CourseEnrollment, verified_unenroll_done
+from student.models import CourseEnrollment
 from course_modes.models import CourseMode
 from shoppingcart.exceptions import PurchasedCallbackException
+from django.utils.timezone import UTC
+import datetime
 
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
@@ -361,17 +363,85 @@ class CertificateItemTest(ModuleStoreTestCase):
         self.assertEquals(cert_item.single_item_receipt_template,
                           'shoppingcart/receipt.html')
 
-    def test_refund_cert_callback(self):
+    def test_refund_cert_callback_no_expiration(self):
         # enroll and buy; dup from test_existing_enrollment
-        CourseEnrollment.enroll(self.user, self.course_id)
+        CourseEnrollment.enroll(self.user, self.course_id, 'verified')
         cart = Order.get_cart_for_user(user=self.user)
         CertificateItem.add_to_order(cart, self.course_id, self.cost, 'verified')
         cart.purchase()
         # now that it's there, let's try refunding it
-        verified_unenroll_done.send(sender=self, user=self.user, user_email=self.user.email, course_id=self.course_id)
+        CourseEnrollment.unenroll(self.user, self.course_id)
         target_certs = CertificateItem.objects.filter(course_id=self.course_id, user_id=self.user, status='refunded', mode='verified')
         self.assertTrue(target_certs[0])
 
+    def test_refund_cert_callback_before_expiration(self):
+        # enroll and buy; dup from test_existing_enrollment
+        course_id = "refund_before_expiration/test/one"
+        many_days = datetime.timedelta(days=60)
+
+        CourseFactory.create(org='refund_before_expiration', number='test', run='course', display_name='one')
+        course_mode = CourseMode(course_id=course_id,
+                                 mode_slug="verified",
+                                 mode_display_name="verified cert",
+                                 min_price=self.cost,
+                                 expiration_date=(datetime.datetime.now(UTC()).date() + many_days))
+        course_mode.save()
+
+        CourseEnrollment.enroll(self.user, course_id, 'verified')
+        cart = Order.get_cart_for_user(user=self.user)
+        CertificateItem.add_to_order(cart, course_id, self.cost, 'verified')
+        cart.purchase()
+
+        # now that it's there, let's try refunding it
+        CourseEnrollment.unenroll(self.user, course_id)
+        target_certs = CertificateItem.objects.filter(course_id=course_id, user_id=self.user, status='refunded', mode='verified')
+        self.assertTrue(target_certs[0])
+
+    @patch('shoppingcart.models.log.error')
+    def test_refund_cert_callback_before_expiration_email_error(self, error_logger):
+        course_id = "refund_before_expiration/test/one"
+        many_days = datetime.timedelta(days=60)
+
+        CourseFactory.create(org='refund_before_expiration', number='test', run='course', display_name='one')
+        course_mode = CourseMode(course_id=course_id,
+                                 mode_slug="verified",
+                                 mode_display_name="verified cert",
+                                 min_price=self.cost,
+                                 expiration_date=(datetime.datetime.now(UTC()).date() + many_days))
+        course_mode.save()
+
+        CourseEnrollment.enroll(self.user, course_id, 'verified')
+        cart = Order.get_cart_for_user(user=self.user)
+        CertificateItem.add_to_order(cart, course_id, self.cost, 'verified')
+        cart.purchase()
+
+        # now that it's there, let's try refunding it
+        with patch('shoppingcart.models.send_mail', side_effect=smtplib.SMTPException):
+            CourseEnrollment.unenroll(self.user, course_id)
+            self.assertTrue(error_logger.called)
+
+    def test_refund_cert_callback_after_expiration(self):
+        # Enroll and buy
+        course_id = "refund_after_expiration/test/two"
+        CourseFactory.create(org='refund_after_expiration', number='test', run='course', display_name='two')
+        course_mode = CourseMode(course_id=course_id,
+                                 mode_slug="verified",
+                                 mode_display_name="verified cert",
+                                 min_price=self.cost,)
+        course_mode.save()
+
+        CourseEnrollment.enroll(self.user, course_id, 'verified')
+        cart = Order.get_cart_for_user(user=self.user)
+        CertificateItem.add_to_order(cart, course_id, self.cost, 'verified')
+        cart.purchase()
+        course_mode.save()
+
+        # now that it's there, let's try refunding it
+        CourseEnrollment.unenroll(self.user, course_id)
+        target_certs = CertificateItem.objects.filter(course_id=course_id, user_id=self.user, status='refunded', mode='verified')
+        self.assertTrue(target_certs[0])
+
     def test_refund_cert_no_cert_exists(self):
-        with self.assertRaises(IndexError):
-            verified_unenroll_done.send(sender=self, user=self.user, user_email=self.user.email, course_id=self.course_id)
+        CourseEnrollment.enroll(self.user, self.course_id, 'verified')
+        ret_val = CourseEnrollment.unenroll(self.user, self.course_id)
+        self.assertFalse(ret_val)

lms/envs/dev.py

@@ -19,7 +19,7 @@ DEBUG = True
 USE_I18N = True
 TEMPLATE_DEBUG = True
 
-
+MITX_FEATURES['ENABLE_PAYMENT_FAKE'] = True
 MITX_FEATURES['DISABLE_START_DATES'] = False
 MITX_FEATURES['ENABLE_SQL_TRACKING_LOGS'] = True
 MITX_FEATURES['SUBDOMAIN_COURSE_LISTINGS'] = False  # Enable to test subdomains--otherwise, want all courses to show up
@@ -269,7 +269,7 @@ if SEGMENT_IO_LMS_KEY:
 CC_PROCESSOR['CyberSource']['SHARED_SECRET'] = os.environ.get('CYBERSOURCE_SHARED_SECRET', '')
 CC_PROCESSOR['CyberSource']['MERCHANT_ID'] = os.environ.get('CYBERSOURCE_MERCHANT_ID', '')
 CC_PROCESSOR['CyberSource']['SERIAL_NUMBER'] = os.environ.get('CYBERSOURCE_SERIAL_NUMBER', '')
-CC_PROCESSOR['CyberSource']['PURCHASE_ENDPOINT'] = os.environ.get('CYBERSOURCE_PURCHASE_ENDPOINT', '')
+CC_PROCESSOR['CyberSource']['PURCHASE_ENDPOINT'] = '/shoppingcart/payment_fake/'
 
 
 ########################## USER API ########################

lms/templates/dashboard/_dashboard_course_listing.html

@@ -144,9 +144,9 @@
       % if enrollment.mode != "verified":
       <a href="#unenroll-modal" class="unenroll" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}" onclick="document.getElementById('refund-info').innerHTML=''">${_('Unregister')}</a>
       % elif show_refund_option:
-      <a href="#unenroll-modal" class="unenroll" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}" onclick="document.getElementById('refund-info').innerHTML=gettext('You will be refunded for the amount you paid')">${_('Unregister')}</a>
+      <a href="#unenroll-modal" class="unenroll" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}" onclick="document.getElementById('refund-info').innerHTML=gettext('You will be refunded the amount you paid.')">${_('Unregister')}</a>
       % else:
-      <a href="#unenroll-modal" class="unenroll" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}" onclick="document.getElementById('refund-info').innerHTML=gettext('The refund deadline for this course has passed, so you will not receive money back')">${_('Unregister')}</a>
+      <a href="#unenroll-modal" class="unenroll" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}" onclick="document.getElementById('refund-info').innerHTML=gettext('The refund deadline for this course has passed, so you will not receive a refund.')">${_('Unregister')}</a>
       % endif
 
 % if show_email_settings: