Merge pull request #251 from edx/renzo/audit-migration

Support migration of legacy audit modes

ecommerce/courses/models.py

@@ -72,13 +72,16 @@ class Course(models.Model):
 
         if mode == 'no-id-professional':
             return 'professional'
+        elif mode == 'audit':
+            # Historically, users enrolled in an 'audit' mode have not received a certificate.
+            return ''
 
         return mode
 
     @property
     def type(self):
         """ Returns the type of the course (based on the available seat types). """
-        seat_types = [(seat.attr.certificate_type or '').lower() for seat in self.seat_products]
+        seat_types = [getattr(seat.attr, 'certificate_type', '').lower() for seat in self.seat_products]
         if 'credit' in seat_types:
             return 'credit'
         elif 'professional' in seat_types or 'no-id-professional' in seat_types:
@@ -100,12 +103,13 @@ class Course(models.Model):
 
     def _get_course_seat_name(self, certificate_type, id_verification_required):
         """ Returns the name for a course seat. """
-        name = 'Seat in {course_name} with {certificate_type} certificate'.format(
-            course_name=self.name,
-            certificate_type=certificate_type)
+        name = u'Seat in {}'.format(self.name)
 
-        if id_verification_required:
-            name += ' (and ID verification)'
+        if certificate_type != '':
+            name += u' with {} certificate'.format(certificate_type)
+
+            if id_verification_required:
+                name += u' (and ID verification)'
 
         return name
 
@@ -117,7 +121,6 @@ class Course(models.Model):
         Returns:
             Product:  The seat that has been created or updated.
         """
-
         certificate_type = certificate_type.lower()
         course_id = unicode(self.id)
 
@@ -136,12 +139,18 @@ class Course(models.Model):
 
         try:
             seat = Product.objects.get(slug__in=slugs)
-            logger.info('Retrieved [%s] course seat child product for [%s] from database.', certificate_type,
-                        course_id)
+            logger.info(
+                'Retrieved course seat child product with certificate type [%s] for [%s] from database.',
+                certificate_type,
+                course_id
+            )
         except Product.DoesNotExist:
             seat = Product(slug=slug)
-            logger.info('[%s] course seat product for [%s] does not exist. Instantiated a new instance.',
-                        certificate_type, course_id)
+            logger.info(
+                'Course seat product with certificate type [%s] for [%s] does not exist. Instantiated a new instance.',
+                certificate_type,
+                course_id
+            )
 
         seat.course = self
         seat.parent = self.parent_seat_product
@@ -149,7 +158,11 @@ class Course(models.Model):
         seat.structure = Product.CHILD
         seat.title = self._get_course_seat_name(certificate_type, id_verification_required)
         seat.expires = expires
+
+        # If a ProductAttribute is saved with a value of None or the empty string, the ProductAttribute is deleted.
+        # As a consequence, Seats derived from a migrated "audit" mode do not have a certificate_type attribute.
         seat.attr.certificate_type = certificate_type
+
         seat.attr.course_key = course_id
         seat.attr.id_verification_required = id_verification_required
 
@@ -165,14 +178,20 @@ class Course(models.Model):
         partner = Partner.objects.get(code='edx')
         try:
             stock_record = StockRecord.objects.get(product=seat, partner=partner)
-            logger.info('Retrieved [%s] course seat child product stock record for [%s] from database.',
-                        certificate_type, course_id)
+            logger.info(
+                'Retrieved course seat product stock record with certificate type [%s] for [%s] from database.',
+                certificate_type,
+                course_id
+            )
         except StockRecord.DoesNotExist:
             partner_sku = generate_sku(seat)
             stock_record = StockRecord(product=seat, partner=partner, partner_sku=partner_sku)
             logger.info(
-                '[%s] course seat product stock record for [%s] does not exist. Instantiated a new instance.',
-                certificate_type, course_id)
+                'Course seat product stock record with certificate type [%s] for [%s] does not exist. '
+                'Instantiated a new instance.',
+                certificate_type,
+                course_id
+            )
 
         stock_record.price_excl_tax = price
 

ecommerce/courses/publishers.py

@@ -11,7 +11,7 @@ logger = logging.getLogger(__name__)
 
 class LMSPublisher(object):
     def get_seat_expiration(self, seat):
-        if not seat.expires or 'professional' in seat.attr.certificate_type:
+        if not seat.expires or 'professional' in getattr(seat.attr, 'certificate_type', ''):
             return None
 
         return seat.expires.isoformat()

ecommerce/courses/tests/test_models.py

@@ -59,7 +59,7 @@ class CourseTests(CourseCatalogTestMixin, TestCase):
         ('professional', 'professional'),
         ('honor', 'honor'),
         ('no-id-professional', 'professional'),
-        ('audit', 'audit'),
+        ('audit', ''),
         ('unknown', 'unknown'),
     )
     @ddt.unpack
@@ -92,7 +92,7 @@ class CourseTests(CourseCatalogTestMixin, TestCase):
         # pylint: disable=protected-access
         self.assertEqual(seat.title, course._get_course_seat_name(certificate_type, id_verification_required))
         self.assertEqual(seat.get_product_class(), self.seat_product_class)
-        self.assertEqual(seat.attr.certificate_type, certificate_type)
+        self.assertEqual(getattr(seat.attr, 'certificate_type', ''), certificate_type)
         self.assertEqual(seat.attr.course_key, course.id)
         self.assertEqual(seat.attr.id_verification_required, id_verification_required)
         self.assertEqual(seat.stockrecords.first().price_excl_tax, price)

ecommerce/courses/tests/test_publishers.py

@@ -98,7 +98,7 @@ class LMSPublisherTests(CourseCatalogTestMixin, TestCase):
     def test_serialize_seat_for_commerce_api(self):
         """ The method should convert a seat to a JSON-serializable dict consumable by the Commerce API. """
         # Grab the verified seat
-        seat = sorted(self.course.seat_products, key=lambda p: p.attr.certificate_type)[1]
+        seat = sorted(self.course.seat_products, key=lambda p: getattr(p.attr, 'certificate_type', ''))[1]
         stock_record = seat.stockrecords.first()
 
         actual = self.publisher.serialize_seat_for_commerce_api(seat)

ecommerce/courses/tests/test_utils.py

@@ -11,6 +11,7 @@ from ecommerce.extensions.catalogue.tests.mixins import CourseCatalogTestMixin
 class UtilsTests(CourseCatalogTestMixin, TestCase):
     @ddt.unpack
     @ddt.data(
+        ('', False, 'audit'),
         ('honor', True, 'honor'),
         ('honor', False, 'honor'),
         ('verified', True, 'verified'),

ecommerce/courses/utils.py

 def mode_for_seat(seat):
     """ Returns the Enrollment mode for a given seat product. """
-    certificate_type = seat.attr.certificate_type
+    certificate_type = getattr(seat.attr, 'certificate_type', '')
 
     if certificate_type == 'professional' and not seat.attr.id_verification_required:
         return 'no-id-professional'
+    elif certificate_type == '':
+        return 'audit'
 
     return certificate_type

ecommerce/credit/views.py

@@ -39,7 +39,7 @@ class Checkout(TemplateView):
                 processors_dict[processor] = 'Checkout with {}'.format(processor)
 
         credit_seats = [
-            seat for seat in course.seat_products if seat.attr.certificate_type == self.CREDIT_MODE
+            seat for seat in course.seat_products if getattr(seat.attr, 'certificate_type', '') == self.CREDIT_MODE
         ]
         provider_ids = None
         if credit_seats:

ecommerce/extensions/catalogue/management/commands/migrate_course.py

@@ -138,9 +138,14 @@ class Command(BaseCommand):
 
                     for seat in course.seat_products:
                         stock_record = seat.stockrecords.first()
-                        data = (seat.attr.certificate_type, seat.attr.id_verification_required,
-                                '{0} {1}'.format(stock_record.price_currency, stock_record.price_excl_tax),
-                                stock_record.partner_sku, seat.slug, seat.expires)
+                        data = (
+                            getattr(seat.attr, 'certificate_type', ''),
+                            seat.attr.id_verification_required,
+                            '{0} {1}'.format(stock_record.price_currency, stock_record.price_excl_tax),
+                            stock_record.partner_sku,
+                            seat.slug,
+                            seat.expires
+                        )
                         msg += '\t{}\n'.format(data)
 
                     logger.info(msg)

ecommerce/extensions/catalogue/tests/test_migrate_course.py

@@ -19,6 +19,7 @@ from waffle import Switch
 from ecommerce.core.constants import ISO_8601_FORMAT
 from ecommerce.courses.models import Course
 from ecommerce.courses.publishers import LMSPublisher
+from ecommerce.courses.utils import mode_for_seat
 from ecommerce.extensions.catalogue.management.commands.migrate_course import MigratedCourse
 from ecommerce.extensions.catalogue.tests.mixins import CourseCatalogTestMixin
 from ecommerce.extensions.catalogue.utils import generate_sku
@@ -44,7 +45,9 @@ class CourseMigrationTestMixin(CourseCatalogTestMixin):
         'honor': 0,
         'verified': 10,
         'no-id-professional': 100,
-        'professional': 1000
+        'professional': 1000,
+        'audit': 0,
+        'credit': 0,
     }
 
     def _mock_lms_api(self):
@@ -60,8 +63,8 @@ class CourseMigrationTestMixin(CourseCatalogTestMixin):
         # Mock Enrollment API
         body = {
             'course_id': self.course_id,
-            'course_modes': [{'slug': seat_type, 'min_price': price, 'expiration_datetime': EXPIRES_STRING} for
-                             seat_type, price in self.prices.iteritems()]
+            'course_modes': [{'slug': mode, 'min_price': price, 'expiration_datetime': EXPIRES_STRING} for
+                             mode, price in self.prices.iteritems()]
         }
         httpretty.register_uri(httpretty.GET, self.enrollment_api_url, body=json.dumps(body), content_type=JSON)
 
@@ -76,12 +79,15 @@ class CourseMigrationTestMixin(CourseCatalogTestMixin):
         """ Verify the given seat is configured correctly. """
         certificate_type = Course.certificate_type_for_mode(mode)
 
-        expected_title = 'Seat in A Tést Côurse with {} certificate'.format(certificate_type)
-        if seat.attr.id_verification_required:
-            expected_title += u' (and ID verification)'
+        expected_title = 'Seat in A Tést Côurse'
+        if certificate_type != '':
+            expected_title += ' with {} certificate'.format(certificate_type)
+
+            if seat.attr.id_verification_required:
+                expected_title += u' (and ID verification)'
 
         self.assertEqual(seat.title, expected_title)
-        self.assertEqual(seat.attr.certificate_type, certificate_type)
+        self.assertEqual(getattr(seat.attr, 'certificate_type', ''), certificate_type)
         self.assertEqual(seat.expires, EXPIRES)
         self.assertEqual(seat.attr.course_key, self.course_id)
         self.assertEqual(seat.attr.id_verification_required, Course.is_mode_verified(mode))
@@ -90,18 +96,20 @@ class CourseMigrationTestMixin(CourseCatalogTestMixin):
         """ Verify the course was migrated and saved to the database. """
         course = Course.objects.get(id=self.course_id)
         seats = course.seat_products
-        self.assertEqual(len(seats), 4)
+
+        # Verify that all modes are migrated.
+        self.assertEqual(len(seats), len(self.prices))
+
         parent = course.products.get(structure=Product.PARENT)
         self.assertEqual(list(parent.categories.all()), [self.category])
+
         for seat in seats:
-            seat_type = seat.attr.certificate_type
-            if seat_type == 'professional' and not seat.attr.id_verification_required:
-                seat_type = 'no-id-professional'
-            logger.info('Validating objects for %s certificate type...', seat_type)
+            mode = mode_for_seat(seat)
+            logger.info('Validating objects for [%s] mode...', mode)
 
             stock_record = self.partner.stockrecords.get(product=seat)
-            self.assert_seat_valid(seat, seat_type)
-            self.assert_stock_record_valid(stock_record, seat, self.prices[seat_type])
+            self.assert_seat_valid(seat, mode)
+            self.assert_stock_record_valid(stock_record, seat, self.prices[mode])
 
     def assert_lms_api_headers(self, request):
         self.assertEqual(request.headers['Accept'], JSON)
@@ -143,11 +151,10 @@ class MigratedCourseTests(CourseMigrationTestMixin, TestCase):
         self.assertEqual(course.verification_deadline, EXPIRES)
 
         for seat in course.seat_products:
-            certificate_type = seat.attr.certificate_type
-            if certificate_type == 'professional' and not seat.attr.id_verification_required:
-                certificate_type = 'no-id-professional'
-            logger.info('Validating objects for %s certificate type...', certificate_type)
-            self.assert_stock_record_valid(seat.stockrecords.first(), seat, Decimal(self.prices[certificate_type]))
+            mode = mode_for_seat(seat)
+            logger.info('Validating objects for [%s] mode...', mode)
+
+            self.assert_stock_record_valid(seat.stockrecords.first(), seat, Decimal(self.prices[mode]))
 
     @httpretty.activate
     def test_course_name_missing(self):

ecommerce/extensions/catalogue/utils.py

@@ -9,9 +9,12 @@ def generate_sku(product):
     """
     # Note: This currently supports seats. In the future, this should
     # be updated to accommodate other product classes.
-    _hash = u' '.join((product.attr.certificate_type.lower(),
-                       product.attr.course_key.lower(),
-                       unicode(product.attr.id_verification_required)))
+    _hash = u' '.join((
+        getattr(product.attr, 'certificate_type', '').lower(),
+        product.attr.course_key.lower(),
+        unicode(product.attr.id_verification_required)
+    ))
     _hash = md5(_hash)
     _hash = _hash.hexdigest()[-7:]
+
     return _hash.upper()

ecommerce/extensions/checkout/signals.py

@@ -3,6 +3,7 @@ import waffle
 from django.dispatch import receiver
 from oscar.core.loading import get_class
 
+from ecommerce.courses.utils import mode_for_seat
 from ecommerce.extensions.analytics.utils import is_segment_configured, parse_tracking_context, log_exceptions
 from ecommerce.notifications.notifications import send_notification
 
@@ -33,7 +34,7 @@ def track_completed_order(sender, order=None, **kwargs):  # pylint: disable=unus
                     # SKU. Marketing is aware that this approach will not scale once we start selling
                     # products other than courses, and will need to change in the future.
                     'id': line.partner_sku,
-                    'sku': line.product.attr.certificate_type,
+                    'sku': mode_for_seat(line.product),
                     'name': line.product.title,
                     'price': str(line.line_price_excl_tax),
                     'quantity': line.quantity,

ecommerce/extensions/fulfillment/modules.py

@@ -228,12 +228,12 @@ class EnrollmentFulfillmentModule(BaseFulfillmentModule):
         try:
             logger.info('Attempting to revoke fulfillment of Line [%d]...', line.id)
 
-            certificate_type = line.product.attr.certificate_type
+            mode = mode_for_seat(line.product)
             course_key = line.product.attr.course_key
             data = {
                 'user': line.order.user.username,
                 'is_active': False,
-                'mode': certificate_type,
+                'mode': mode,
                 'course_details': {
                     'course_id': course_key,
                 },
@@ -248,7 +248,7 @@ class EnrollmentFulfillmentModule(BaseFulfillmentModule):
                     order_number=line.order.number,
                     product_class=line.product.get_product_class().name,
                     course_id=course_key,
-                    certificate_type=certificate_type,
+                    certificate_type=getattr(line.product.attr, 'certificate_type', ''),
                     user_id=line.order.user.id
                 )
 

ecommerce/extensions/fulfillment/tests/test_modules.py

@@ -143,7 +143,7 @@ class EnrollmentFulfillmentModuleTests(EnrollmentFulfillmentTestMixin, TestCase)
                     'INFO',
                     'line_revoked: certificate_type="{}", course_id="{}", order_line_id="{}", order_number="{}", '
                     'product_class="{}", user_id="{}"'.format(
-                        line.product.attr.certificate_type,
+                        getattr(line.product.attr, 'certificate_type', ''),
                         line.product.attr.course_key,
                         line.id,
                         line.order.number,

ecommerce/extensions/payment/processors/cybersource.py

@@ -98,7 +98,7 @@ class Cybersource(BasePaymentProcessor):
         single_seat = self.get_single_seat(basket)
         if single_seat:
             parameters[u'merchant_defined_data1'] = single_seat.attr.course_key
-            parameters[u'merchant_defined_data2'] = single_seat.attr.certificate_type
+            parameters[u'merchant_defined_data2'] = getattr(single_seat.attr, 'certificate_type', '')
 
         # Sign all fields
         signed_field_names = parameters.keys()

ecommerce/extensions/refund/signals.py

 import analytics
 from django.dispatch import receiver, Signal
 
+from ecommerce.courses.utils import mode_for_seat
 from ecommerce.extensions.analytics.utils import is_segment_configured, parse_tracking_context, log_exceptions
 
 
@@ -34,7 +35,7 @@ def track_completed_refund(sender, refund=None, **kwargs):  # pylint: disable=un
                     # SKU. Marketing is aware that this approach will not scale once we start selling
                     # products other than courses, and will need to change in the future.
                     'id': line.order_line.partner_sku,
-                    'sku': line.order_line.product.attr.certificate_type,
+                    'sku': mode_for_seat(line.order_line.product),
                     'name': line.order_line.product.title,
                     'price': str(line.line_credit_excl_tax),
                     'quantity': -1 * line.quantity,

ecommerce/extensions/refund/tests/factories.py

@@ -7,8 +7,10 @@ from oscar.core.loading import get_model
 from oscar.test import factories
 from oscar.test.newfactories import UserFactory
 
+from ecommerce.courses.models import Course
 from ecommerce.extensions.refund.status import REFUND, REFUND_LINE
 
+
 Category = get_model("catalogue", "Category")
 Partner = get_model('partner', 'Partner')
 Product = get_model("catalogue", "Product")
@@ -87,14 +89,21 @@ class CourseFactory(object):
     def add_mode(self, name, price, id_verification_required=False):
         parent_product = self._get_parent_seat_product()
 
-        title = u'{mode_name} Seat in {course_name}'.format(mode_name=name, course_name=self.course_name)
-        slug = slugify(u'{course_name}-seat-{mode_name}'.format(course_name=self.course_name, mode_name=name))
+        certificate_type = Course.certificate_type_for_mode(name)
+        title = u'{certificate_type} Seat in {course_name}'.format(
+            certificate_type=certificate_type,
+            course_name=self.course_name
+        )
+        slug = slugify(u'{course_name}-seat-{certificate_type}'.format(
+            course_name=self.course_name,
+            certificate_type=certificate_type
+        ))
         child_product, created = Product.objects.get_or_create(parent=parent_product, title=title, slug=slug,
                                                                structure='child')
 
         if created:
             child_product.attr.course_key = self.course_id
-            child_product.attr.certificate_type = name
+            child_product.attr.certificate_type = certificate_type
             child_product.attr.id_verification_required = id_verification_required
             child_product.save()
 

ecommerce/tests/mixins.py

@@ -12,6 +12,7 @@ from mock import patch
 from oscar.test import factories
 from oscar.core.loading import get_model, get_class
 
+from ecommerce.courses.utils import mode_for_seat
 from ecommerce.extensions.api.constants import APIConstants as AC
 from ecommerce.extensions.fulfillment.mixins import FulfillmentMixin
 
@@ -201,7 +202,7 @@ class BusinessIntelligenceMixin(object):
                 self.assertEqual(line.product.title, tracked_product['name'])
                 self.assertEqual(str(line.line_price_excl_tax), tracked_product['price'])
                 self.assertEqual(line.quantity, tracked_product['quantity'])
-                self.assertEqual(line.product.attr.certificate_type, tracked_product['sku'])
+                self.assertEqual(mode_for_seat(line.product), tracked_product['sku'])
                 self.assertEqual(line.product.get_product_class().name, tracked_product['category'])
         elif model_name == 'Refund':
             self.assertEqual(event_payload['total'], '-{}'.format(total))
@@ -212,7 +213,7 @@ class BusinessIntelligenceMixin(object):
                 self.assertEqual(line.order_line.product.title, tracked_product['name'])
                 self.assertEqual(str(line.line_credit_excl_tax), tracked_product['price'])
                 self.assertEqual(-1 * line.quantity, tracked_product['quantity'])
-                self.assertEqual(line.order_line.product.attr.certificate_type, tracked_product['sku'])
+                self.assertEqual(mode_for_seat(line.order_line.product), tracked_product['sku'])
                 self.assertEqual(line.order_line.product.get_product_class().name, tracked_product['category'])
         else:
             # Payload validation is currently limited to order and refund events