Merge pull request #138 from open-craft/mtyaka/copy-course-keys-migration

Copy course_id into course_key and remove temporary transition code

circle.yml

@@ -11,7 +11,7 @@ dependencies:
     - "pip install -r requirements.txt"
     - "pip install -r $VIRTUAL_ENV/src/xblock-sdk/requirements/base.txt"
     - "pip install -r $VIRTUAL_ENV/src/xblock-sdk/requirements/test.txt"
-    - "pip uninstall -y xblock-problem-builder && python setup.py sdist && pip install dist/xblock-problem-builder-2.6.5.tar.gz"
+    - "pip uninstall -y xblock-problem-builder && python setup.py sdist && pip install dist/xblock-problem-builder-2.6.6.tar.gz"
     - "pip install -r test_requirements.txt"
     - "mkdir var"
 test:

problem_builder/answer.py

@@ -23,11 +23,6 @@
 import logging
 from lazy import lazy
 
-from django.core.exceptions import ValidationError
-from django.db import IntegrityError
-from django.db.models import Q
-from django.utils.crypto import get_random_string
-
 from .models import Answer
 
 from xblock.core import XBlock
@@ -69,35 +64,6 @@ class AnswerMixin(XBlockWithPreviewMixin, XBlockWithTranslationServiceMixin):
         except AttributeError:
             return self.scope_ids.user_id
 
-    @staticmethod
-    def _fetch_model_object(name, student_id, course_id):
-        answer = Answer.objects.get(
-            Q(name=name),
-            Q(student_id=student_id),
-            Q(course_key=course_id) | Q(course_id=course_id)
-        )
-        if not answer.course_key:
-            answer.course_key = answer.course_id
-        return answer
-
-    @staticmethod
-    def _create_model_object(name, student_id, course_id):
-        # Try to store the course_id into the deprecated course_id field if it fits into
-        # the 50 character limit for compatibility with old code. If it does not fit,
-        # use a random temporary value until the column gets removed in next release.
-        # This should not create any issues with old code running alongside the new code,
-        # since Answer blocks don't work with old code when course_id is longer than 50 chars anyway.
-        if len(course_id) > 50:
-            # The deprecated course_id field cannot be blank. It also needs to be unique together with
-            # the name and student_id fields, so we cannot use a static placeholder value, we generate
-            # a random value instead, to make the database happy.
-            deprecated_course_id = get_random_string(24)
-        else:
-            deprecated_course_id = course_id
-        answer = Answer(student_id=student_id, name=name, course_key=course_id, course_id=deprecated_course_id)
-        answer.save()
-        return answer
-
     def get_model_object(self, name=None):
         """
         Fetches the Answer model object for the answer named `name`
@@ -110,18 +76,13 @@ class AnswerMixin(XBlockWithPreviewMixin, XBlockWithTranslationServiceMixin):
             raise ValueError('AnswerBlock.name field need to be set to a non-null/empty value')
 
         student_id = self._get_student_id()
-        course_id = self._get_course_id()
+        course_key = self._get_course_id()
 
-        try:
-            answer_data = self._fetch_model_object(name, student_id, course_id)
-        except Answer.DoesNotExist:
-            try:
-                # Answer object does not exist, try to create it.
-                answer_data = self._create_model_object(name, student_id, course_id)
-            except (IntegrityError, ValidationError):
-                # Integrity/validation error means the object must have been created in the meantime,
-                # so fetch the new object from the db.
-                answer_data = self._fetch_model_object(name, student_id, course_id)
+        answer_data, _ = Answer.objects.get_or_create(
+            student_id=student_id,
+            course_key=course_key,
+            name=name,
+        )
 
         return answer_data
 

problem_builder/migrations/0004_copy_course_ids.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+def copy_course_id_to_course_key(apps, schema_editor):
+    """
+    Iterates over all Answer records for which course_key is not set
+    and copies the value of the course_id column into course_key.
+    """
+    Answer = apps.get_model('problem_builder', 'Answer')
+    for answer in Answer.objects.filter(course_key__isnull=True).iterator():
+        answer.course_key = answer.course_id
+        answer.save()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('problem_builder', '0003_auto_20161124_0755'),
+    ]
+
+    operations = [
+        migrations.RunPython(code=copy_course_id_to_course_key, reverse_code=migrations.RunPython.noop),
+    ]

problem_builder/migrations/0005_auto_20170112_1021.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('problem_builder', '0004_copy_course_ids'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='answer',
+            name='course_id',
+            field=models.CharField(default=None, max_length=50, null=True, db_index=True, blank=True),
+        ),
+        migrations.AlterField(
+            model_name='answer',
+            name='course_key',
+            field=models.CharField(max_length=255, db_index=True),
+        ),
+    ]

problem_builder/models.py

@@ -43,11 +43,9 @@ class Answer(models.Model):
     name = models.CharField(max_length=50, db_index=True)
     student_id = models.CharField(max_length=32, db_index=True)
     # course_id is deprecated; it will be removed in next release.
-    course_id = models.CharField(max_length=50, db_index=True)
+    course_id = models.CharField(max_length=50, db_index=True, blank=True, null=True, default=None)
     # course_key is the new course_id replacement with extended max_length.
-    # We need to allow NULL values during the transition period,
-    # but we will remove the null=True and default=None parameters in next release.
-    course_key = models.CharField(max_length=255, db_index=True, null=True, default=None)
+    course_key = models.CharField(max_length=255, db_index=True)
     student_input = models.TextField(blank=True, default='')
     created_on = models.DateTimeField('created on', auto_now_add=True)
     modified_on = models.DateTimeField('modified on', auto_now=True)

problem_builder/tests/unit/test_answer_mixin.py

 """
-Tests temporary AnswerMixin code that helps migrate course_id column to course_key.
+Unit tests for AnswerMixin.
 """
 import unittest
 from collections import namedtuple
 from django.utils.crypto import get_random_string
-from mock import patch
 
 from problem_builder.answer import AnswerMixin
 from problem_builder.models import Answer
@@ -37,7 +36,6 @@ class TestAnswerMixin(unittest.TestCase):
         model = answer_mixin.get_model_object()
         self.assertEqual(model.name, name)
         self.assertEqual(model.student_id, self.anonymous_student_id)
-        self.assertEqual(model.course_id, self.course_id)
         self.assertEqual(model.course_key, self.course_id)
         self.assertEqual(Answer.objects.get(pk=model.pk), model)
 
@@ -47,29 +45,12 @@ class TestAnswerMixin(unittest.TestCase):
             name=name,
             student_id=self.anonymous_student_id,
             course_key=self.course_id,
-            course_id='ignored'
         )
         existing_model.save()
         answer_mixin = self.make_answer_mixin(name=name)
         model = answer_mixin.get_model_object()
         self.assertEqual(model, existing_model)
 
-    def test_finds_instance_by_course_id(self):
-        name = 'test-course-id'
-        existing_model = Answer(
-            name=name,
-            student_id=self.anonymous_student_id,
-            course_id=self.course_id,
-            course_key=None
-        )
-        # Temporarily patch full_clean to allow saving object with blank course_key to the database.
-        with patch.object(Answer, 'full_clean', return_value=None):
-            existing_model.save()
-        answer_mixin = self.make_answer_mixin(name=name)
-        model = answer_mixin.get_model_object()
-        self.assertEqual(model, existing_model)
-        self.assertEqual(model.course_key, self.course_id)
-
     def test_works_with_long_course_keys(self):
         course_id = 'course-v1:VeryLongOrganizationName+VeryLongCourseNumber+VeryLongCourseRun'
         self.assertTrue(len(course_id) > 50)  # precondition check

setup.py

@@ -71,7 +71,7 @@ BLOCKS = [
 
 setup(
     name='xblock-problem-builder',
-    version='2.6.5',
+    version='2.6.6',
     description='XBlock - Problem Builder',
     packages=['problem_builder', 'problem_builder.v1'],
     install_requires=[