From 84075efc8188d31becabf141939bc2ef471222e6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 4 Aug 2017 11:41:44 -0400 Subject: [PATCH] Convert CourseKeyField to ForeignKey(CourseOverview) on enrollments and course modes --- common/djangoapps/course_modes/admin.py | 20 ++++-- .../0008_course_key_field_to_foreign_key.py | 47 +++++++++++++ common/djangoapps/course_modes/models.py | 33 ++++++++-- .../course_modes/tests/test_admin.py | 8 ++- .../0011_course_key_field_to_foreign_key.py | 66 +++++++++++++++++++ common/djangoapps/student/models.py | 31 +++++---- lms/djangoapps/experiments/utils.py | 2 +- 7 files changed, 179 insertions(+), 28 deletions(-) create mode 100644 common/djangoapps/course_modes/migrations/0008_course_key_field_to_foreign_key.py create mode 100644 common/djangoapps/student/migrations/0011_course_key_field_to_foreign_key.py diff --git a/common/djangoapps/course_modes/admin.py b/common/djangoapps/course_modes/admin.py index 086c19e213..fb6ffccb32 100644 --- a/common/djangoapps/course_modes/admin.py +++ b/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', diff --git a/common/djangoapps/course_modes/migrations/0008_course_key_field_to_foreign_key.py b/common/djangoapps/course_modes/migrations/0008_course_key_field_to_foreign_key.py new file mode 100644 index 0000000000..7b9ff2da41 --- /dev/null +++ b/common/djangoapps/course_modes/migrations/0008_course_key_field_to_foreign_key.py @@ -0,0 +1,47 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import openedx.core.djangoapps.xmodule_django.models + + +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 + migrations.AlterField( + 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')]), + ), + ] diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index ab39532e5c..cd51903041 100644 --- a/common/djangoapps/course_modes/models.py +++ b/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,20 @@ 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 +178,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 +752,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 +760,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 +777,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): diff --git a/common/djangoapps/course_modes/tests/test_admin.py b/common/djangoapps/course_modes/tests/test_admin.py index f81395b4f0..7bd230e7cb 100644 --- a/common/djangoapps/course_modes/tests/test_admin.py +++ b/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, diff --git a/common/djangoapps/student/migrations/0011_course_key_field_to_foreign_key.py b/common/djangoapps/student/migrations/0011_course_key_field_to_foreign_key.py new file mode 100644 index 0000000000..c2ecc2cd40 --- /dev/null +++ b/common/djangoapps/student/migrations/0011_course_key_field_to_foreign_key.py @@ -0,0 +1,66 @@ +# -*- 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 + + +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. + migrations.AlterField( + model_name='courseenrollment', + name='course', + field=models.ForeignKey(db_constraint=False, to='course_overviews.CourseOverview'), + preserve_default=True, + ), + migrations.AlterField( + 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')]), + ), + ] diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 02bba4e008..93c14d1f43 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -992,10 +992,24 @@ 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 +1031,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 +1195,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 +1447,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: @@ -1654,11 +1668,6 @@ class CourseEnrollment(models.Model): def username(self): 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): """ diff --git a/lms/djangoapps/experiments/utils.py b/lms/djangoapps/experiments/utils.py index 63d99b1203..a8e28f68ef 100644 --- a/lms/djangoapps/experiments/utils.py +++ b/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',