Merge pull request #15878 from edx/ret/foreign-key-course-key

Ret/foreign key course key

common/djangoapps/course_modes/admin.py

@@ -59,6 +59,9 @@ class CourseModeForm(forms.ModelForm):
     def __init__(self, *args, **kwargs):
         super(CourseModeForm, self).__init__(*args, **kwargs)
 
+        if self.data.get('course'):
+            self.data['course'] = CourseKey.from_string(self.data['course'])
+
         default_tz = timezone(settings.TIME_ZONE)
 
         if self.instance._expiration_datetime:  # pylint: disable=protected-access
@@ -81,7 +84,7 @@ class CourseModeForm(forms.ModelForm):
             )
 
     def clean_course_id(self):
-        course_id = self.cleaned_data['course_id']
+        course_id = self.cleaned_data['course']
         try:
             course_key = CourseKey.from_string(course_id)
         except InvalidKeyError:
@@ -150,7 +153,7 @@ class CourseModeForm(forms.ModelForm):
         """
         # Trigger validation so we can access cleaned data
         if self.is_valid():
-            course_key = self.cleaned_data.get("course_id")
+            course = self.cleaned_data.get("course")
             verification_deadline = self.cleaned_data.get("verification_deadline")
             mode_slug = self.cleaned_data.get("mode_slug")
 
@@ -158,8 +161,11 @@ class CourseModeForm(forms.ModelForm):
             # we need to handle saving this ourselves.
             # Note that verification deadline can be `None` here if
             # the deadline is being disabled.
-            if course_key is not None and mode_slug in CourseMode.VERIFIED_MODES:
-                verification_models.VerificationDeadline.set_deadline(course_key, verification_deadline)
+            if course is not None and mode_slug in CourseMode.VERIFIED_MODES:
+                verification_models.VerificationDeadline.set_deadline(
+                    course.id,
+                    verification_deadline
+                )
 
         return super(CourseModeForm, self).save(commit=commit)
 
@@ -169,7 +175,7 @@ class CourseModeAdmin(admin.ModelAdmin):
     form = CourseModeForm
 
     fields = (
-        'course_id',
+        'course',
         'mode_slug',
         'mode_display_name',
         'min_price',
@@ -180,11 +186,11 @@ class CourseModeAdmin(admin.ModelAdmin):
         'bulk_sku'
     )
 
-    search_fields = ('course_id',)
+    search_fields = ('course',)
 
     list_display = (
         'id',
-        'course_id',
+        'course',
         'mode_slug',
         'min_price',
         'expiration_datetime_custom',

common/djangoapps/course_modes/migrations/0008_course_key_field_to_foreign_key.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import openedx.core.djangoapps.xmodule_django.models
+
+
+# This should only be used for migrations that have be verified to have a net-neutral sql
+# change generated by Django
+class NoSqlAlterField(migrations.AlterField):
+    def database_forwards(self, app_label, schema_editor, from_state, to_state):
+        return
+
+    def database_backwards(self, app_label, schema_editor, from_state, to_state):
+        return
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('course_overviews', '0013_courseoverview_language'),
+        ('course_modes', '0007_coursemode_bulk_sku'),
+    ]
+
+    operations = [
+        # Pin the name of the column in the database so that we can rename the field
+        # in Django without generating any sql changes
+        migrations.AlterField(
+            model_name='coursemode',
+            name='course_id',
+            field=openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255, db_index=True, verbose_name="Course", db_column='course_id'),
+        ),
+        # Change the field name in Django to match our target field name
+        migrations.RenameField(
+            model_name='coursemode',
+            old_name='course_id',
+            new_name='course',
+        ),
+        # Change the type of the field in Django to be a foreign key
+        # N.B. we don't need the db_column declaration because the default
+        # for Django is to use ${field_name}_id (which is what we pinned the column
+        # name to above).
+        # We deliberately leave db_constraint set to False because the column
+        # isn't currently constrained
+        NoSqlAlterField(
+            model_name='coursemode',
+            name='course',
+            field=models.ForeignKey(related_name='modes', db_constraint=False, default=None, to='course_overviews.CourseOverview'),
+            preserve_default=False,
+        ),
+        # Change the Django unique-together constraint (this is Django-level only
+        # since the database column constraint already exists).
+        migrations.AlterUniqueTogether(
+            name='coursemode',
+            unique_together=set([('course', 'mode_slug', 'currency')]),
+        ),
+    ]

common/djangoapps/course_modes/models.py

@@ -13,7 +13,9 @@ from django.db.models import Q
 from django.dispatch import receiver
 from django.utils.translation import ugettext_lazy as _
 from django.utils.encoding import force_text
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 
+from opaque_keys.edx.keys import CourseKey
 from openedx.core.djangoapps.xmodule_django.models import CourseKeyField
 from request_cache.middleware import RequestCache, ns_request_cached
 
@@ -39,8 +41,25 @@ class CourseMode(models.Model):
     class Meta(object):
         app_label = "course_modes"
 
-    # the course that this mode is attached to
-    course_id = CourseKeyField(max_length=255, db_index=True, verbose_name=_("Course"))
+    course = models.ForeignKey(
+        CourseOverview,
+        db_constraint=False,
+        db_index=True,
+        related_name='modes',
+    )
+
+    # Django sets the `course_id` property in __init__ with the value from the database
+    # This pair of properties converts that into a proper CourseKey
+    @property
+    def course_id(self):
+        return self._course_id
+
+    @course_id.setter
+    def course_id(self, value):
+        if isinstance(value, basestring):
+            self._course_id = CourseKey.from_string(value)
+        else:
+            self._course_id = value
 
     # the reference to this mode that can be used by Enrollments to generate
     # similar behavior for the same slug across courses
@@ -164,7 +183,7 @@ class CourseMode(models.Model):
     CACHE_NAMESPACE = u"course_modes.CourseMode.cache."
 
     class Meta(object):
-        unique_together = ('course_id', 'mode_slug', 'currency')
+        unique_together = ('course', 'mode_slug', 'currency')
 
     def clean(self):
         """
@@ -738,8 +757,6 @@ def get_course_prices(course, verified_only=False):
             settings.PAID_COURSE_REGISTRATION_CURRENCY[0]
         )
 
-    currency_symbol = settings.PAID_COURSE_REGISTRATION_CURRENCY[1]
-
     if registration_price > 0:
         price = registration_price
     # Handle course overview objects which have no cosmetic_display_price
@@ -748,6 +765,15 @@ def get_course_prices(course, verified_only=False):
     else:
         price = None
 
+    return registration_price, format_course_price(price)
+
+
+def format_course_price(price):
+    """
+    Return a formatted price for a course (a string preceded by correct currency, or 'Free').
+    """
+    currency_symbol = settings.PAID_COURSE_REGISTRATION_CURRENCY[1]
+
     if price:
         # Translators: This will look like '$50', where {currency_symbol} is a symbol such as '$' and {price} is a
         # numerical amount in that currency. Adjust this display as needed for your language.
@@ -756,7 +782,7 @@ def get_course_prices(course, verified_only=False):
         # Translators: This refers to the cost of the course. In this case, the course costs nothing so it is free.
         cosmetic_display_price = _('Free')
 
-    return registration_price, force_text(cosmetic_display_price)
+    return cosmetic_display_price
 
 
 class CourseModesArchive(models.Model):

common/djangoapps/course_modes/tests/test_admin.py

@@ -12,6 +12,7 @@ from pytz import UTC, timezone
 from course_modes.admin import CourseModeForm
 from course_modes.models import CourseMode
 from course_modes.tests.factories import CourseModeFactory
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 # Technically, we shouldn't be importing verify_student, since it's
 # defined in the LMS and course_modes is in common.  However, the benefits
 # of putting all this configuration in one place outweigh the downsides.
@@ -40,16 +41,16 @@ class AdminCourseModePageTest(ModuleStoreTestCase):
         user.save()
         course = CourseFactory.create()
         expiration = datetime(2015, 10, 20, 1, 10, 23, tzinfo=timezone(settings.TIME_ZONE))
+        CourseOverview.load_from_module_store(course.id)
 
         data = {
-            'course_id': unicode(course.id),
+            'course': unicode(course.id),
             'mode_slug': 'verified',
             'mode_display_name': 'verified',
             'min_price': 10,
             'currency': 'usd',
             '_expiration_datetime_0': expiration.date(),  # due to django admin datetime widget passing as separate vals
             '_expiration_datetime_1': expiration.time(),
-
         }
 
         self.client.login(username=user.username, password='test')
@@ -89,6 +90,7 @@ class AdminCourseModeFormTest(ModuleStoreTestCase):
         """
         super(AdminCourseModeFormTest, self).setUp()
         self.course = CourseFactory.create()
+        CourseOverview.load_from_module_store(self.course.id)
 
     @ddt.data(
         ("honor", False),
@@ -197,7 +199,7 @@ class AdminCourseModeFormTest(ModuleStoreTestCase):
             mode_slug=mode,
         )
         return CourseModeForm({
-            "course_id": unicode(self.course.id),
+            "course": unicode(self.course.id),
             "mode_slug": mode,
             "mode_display_name": mode,
             "_expiration_datetime": upgrade_deadline,

common/djangoapps/student/migrations/0011_course_key_field_to_foreign_key.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+import openedx.core.djangoapps.xmodule_django.models
+
+
+# This should only be used for migrations that have be verified to have a net-neutral sql
+# change generated by Django
+class NoSqlAlterField(migrations.AlterField):
+    def database_forwards(self, app_label, schema_editor, from_state, to_state):
+        return
+
+    def database_backwards(self, app_label, schema_editor, from_state, to_state):
+        return
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('course_overviews', '0013_courseoverview_language'),
+        ('student', '0010_auto_20170207_0458'),
+    ]
+
+    operations = [
+        # Pin the db_columns to the names already in the database
+        migrations.AlterField(
+            model_name='courseenrollment',
+            name='course_id',
+            field=openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255, db_index=True, db_column='course_id'),
+        ),
+        migrations.AlterField(
+            model_name='historicalcourseenrollment',
+            name='course_id',
+            field=openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255, db_index=True, db_column='course_id'),
+        ),
+
+        # Rename the fields in Django to the new names that we want them to have
+        migrations.RenameField(
+            model_name='courseenrollment',
+            old_name='course_id',
+            new_name='course',
+        ),
+        migrations.RenameField(
+            model_name='historicalcourseenrollment',
+            old_name='course_id',
+            new_name='course',
+        ),
+
+        # Alter the fields to make them ForeignKeys (leaving off the db_constraint so
+        # that we don't create it at migration time). The db_column is left off because
+        # it defaults to ${field_name}_id, which we pinned it to up above.
+        NoSqlAlterField(
+            model_name='courseenrollment',
+            name='course',
+            field=models.ForeignKey(db_constraint=False, to='course_overviews.CourseOverview'),
+            preserve_default=True,
+        ),
+        NoSqlAlterField(
+            model_name='historicalcourseenrollment',
+            name='course',
+            field=models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.DO_NOTHING, db_constraint=False, blank=True, to='course_overviews.CourseOverview', null=True),
+            preserve_default=True,
+        ),
+
+        # Set the Django-side unique-together and ordering configuration (no SQL required)
+        migrations.AlterModelOptions(
+            name='courseenrollment',
+            options={'ordering': ('user', 'course')},
+        ),
+        migrations.AlterUniqueTogether(
+            name='courseenrollment',
+            unique_together=set([('user', 'course')]),
+        ),
+    ]

common/djangoapps/student/models.py

@@ -992,10 +992,26 @@ class CourseEnrollment(models.Model):
     checking course dates, user permissions, etc.) This logic is currently
     scattered across our views.
     """
-    MODEL_TAGS = ['course_id', 'is_active', 'mode']
+    MODEL_TAGS = ['course', 'is_active', 'mode']
 
     user = models.ForeignKey(User)
-    course_id = CourseKeyField(max_length=255, db_index=True)
+
+    course = models.ForeignKey(
+        CourseOverview,
+        db_constraint=False,
+    )
+
+    @property
+    def course_id(self):
+        return self._course_id
+
+    @course_id.setter
+    def course_id(self, value):
+        if isinstance(value, basestring):
+            self._course_id = CourseKey.from_string(value)
+        else:
+            self._course_id = value
+
     created = models.DateTimeField(auto_now_add=True, null=True, db_index=True)
 
     # If is_active is False, then the student is not considered to be enrolled
@@ -1017,8 +1033,8 @@ class CourseEnrollment(models.Model):
     MODE_CACHE_NAMESPACE = u'CourseEnrollment.mode_and_active'
 
     class Meta(object):
-        unique_together = (('user', 'course_id'),)
-        ordering = ('user', 'course_id')
+        unique_together = (('user', 'course'),)
+        ordering = ('user', 'course')
 
     def __init__(self, *args, **kwargs):
         super(CourseEnrollment, self).__init__(*args, **kwargs)
@@ -1181,7 +1197,7 @@ class CourseEnrollment(models.Model):
                                   cost=cost, currency=currency)
 
     @classmethod
-    def send_signal_full(cls, event, user=user, mode=mode, course_id=course_id, cost=None, currency=None):
+    def send_signal_full(cls, event, user=user, mode=mode, course_id=None, cost=None, currency=None):
         """
         Sends a signal announcing changes in course enrollment status.
         This version should be used if you don't already have a CourseEnrollment object
@@ -1433,7 +1449,7 @@ class CourseEnrollment(models.Model):
         try:
             return cls.objects.filter(
                 user=user,
-                course_id__startswith=querystring,
+                course__id__startswith=querystring,
                 is_active=1
             ).exists()
         except cls.DoesNotExist:
@@ -1655,11 +1671,6 @@ class CourseEnrollment(models.Model):
         return self.user.username
 
     @property
-    def course(self):
-        # Deprecated. Please use the `course_overview` property instead.
-        return self.course_overview
-
-    @property
     def course_overview(self):
         """
         Returns a CourseOverview of the course to which this enrollment refers.
@@ -1669,7 +1680,7 @@ class CourseEnrollment(models.Model):
             If the course is re-published within the lifetime of this
             CourseEnrollment object, then the value of this property will
             become stale.
-       """
+        """
         if not self._course_overview:
             try:
                 self._course_overview = CourseOverview.get_from_id(self.course_id)

lms/djangoapps/certificates/signals.py

@@ -88,11 +88,11 @@ def _listen_for_track_change(sender, user, **kwargs):  # pylint: disable=unused-
     user_enrollments = CourseEnrollment.enrollments_for_user(user=user)
     grade_factory = CourseGradeFactory()
     for enrollment in user_enrollments:
-        if grade_factory.read(user=user, course=enrollment.course).passed:
-            if fire_ungenerated_certificate_task(user, enrollment.course.id):
+        if grade_factory.read(user=user, course=enrollment.course_overview).passed:
+            if fire_ungenerated_certificate_task(user, enrollment.course_id):
                 log.info(u'Certificate generation task initiated for {user} : {course} via track change'.format(
                     user=user.id,
-                    course=enrollment.course.id
+                    course=enrollment.course_id
                 ))
 
 

lms/djangoapps/experiments/utils.py

@@ -38,7 +38,7 @@ def get_experiment_user_metadata_context(course, user):
 
     return {
         'upgrade_link': upgrade_data and upgrade_data.link,
-        'upgrade_price': get_cosmetic_verified_display_price(course),
+        'upgrade_price': unicode(get_cosmetic_verified_display_price(course)),
         'enrollment_mode': enrollment_mode,
         'enrollment_time': enrollment_time,
         'pacing_type': 'self_paced' if course.self_paced else 'instructor_paced',