From 7ab8142e82e8b77c233c0aa23b52831ec6a0977a Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Wed, 30 Oct 2013 13:33:34 +0000 Subject: [PATCH] Response to Dave & Diana's CR --- common/djangoapps/course_modes/models.py | 10 ++- .../course_modes/tests/factories.py | 1 + common/djangoapps/student/models.py | 7 +- common/djangoapps/student/tests/tests.py | 49 ----------- common/djangoapps/student/views.py | 34 +++----- lms/djangoapps/courseware/access.py | 16 ++-- .../courseware/tests/test_access.py | 31 ++++--- lms/djangoapps/shoppingcart/models.py | 66 ++++++++------- .../shoppingcart/tests/test_models.py | 82 +++++++++++++++++-- lms/envs/dev.py | 4 +- .../dashboard/_dashboard_course_listing.html | 4 +- 11 files changed, 165 insertions(+), 139 deletions(-) diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 8e475d1ec1..ec2cbf1245 100644 --- a/common/djangoapps/course_modes/models.py +++ b/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) diff --git a/common/djangoapps/course_modes/tests/factories.py b/common/djangoapps/course_modes/tests/factories.py index 3e35b2f05c..0d519b3ad5 100644 --- a/common/djangoapps/course_modes/tests/factories.py +++ b/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 diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index ae5e10dc75..36affc0133 100644 --- a/common/djangoapps/student/models.py +++ b/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)) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 3366296aa0..315b6e9285 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/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) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 00480a7401..af2d3e73ac 100644 --- a/common/djangoapps/student/views.py +++ b/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'): diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 980e86f27d..6c8e7ec8ee 100644 --- a/lms/djangoapps/courseware/access.py +++ b/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: + course_mode = CourseMode.mode_for_course(course.id, 'verified') + if course_mode is None: + return False + else: 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()): - return True - return False checkers = { diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index b2c1a14bdf..e537c60d21 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/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() - # 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_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() - course.enrollment_start = (today - grace_period) - 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() + + # 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')) diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index f75b31e1ce..e995fe356f 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/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. """ + mode = kwargs['course_enrollment'].mode + course_id = kwargs['course_enrollment'].course_id + user = kwargs['course_enrollment'].user + + # Only refund verified cert unenrollments that are within bounds of the expiration date + if (mode != 'verified'): + return + + if (CourseMode.mode_for_course(course_id, 'verified') is None): + return + + target_certs = CertificateItem.objects.filter(course_id=course_id, user_id=user, status='purchased', mode='verified') try: - course_id = kwargs['course_id'] - user = kwargs['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() - - order_number = target_cert.order_id - - # 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" - send_mail(subject, message, from_email, to_email, fail_silently=False) - - return target_cert - except IndexError: - log.exception("Matching CertificateItem not found while trying to refund. User %s, Course %s", user, course_id) - raise 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 @classmethod @transaction.commit_on_success diff --git a/lms/djangoapps/shoppingcart/tests/test_models.py b/lms/djangoapps/shoppingcart/tests/test_models.py index 6891cf140d..7cd6ca061c 100644 --- a/lms/djangoapps/shoppingcart/tests/test_models.py +++ b/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) diff --git a/lms/envs/dev.py b/lms/envs/dev.py index e1eadf1331..c61347bdd6 100644 --- a/lms/envs/dev.py +++ b/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 ######################## diff --git a/lms/templates/dashboard/_dashboard_course_listing.html b/lms/templates/dashboard/_dashboard_course_listing.html index 0896797a0f..6229a5472d 100644 --- a/lms/templates/dashboard/_dashboard_course_listing.html +++ b/lms/templates/dashboard/_dashboard_course_listing.html @@ -144,9 +144,9 @@ % if enrollment.mode != "verified": ${_('Unregister')} % elif show_refund_option: - ${_('Unregister')} + ${_('Unregister')} % else: - ${_('Unregister')} + ${_('Unregister')} % endif % if show_email_settings: