Merge pull request #131 from open-craft/mtyaka/extended-course-key

Introduce Answer.course_key and deprecate course_id.

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.3.tar.gz"
+    - "pip uninstall -y xblock-problem-builder && python setup.py sdist && pip install dist/xblock-problem-builder-2.6.4.tar.gz"
     - "pip install -r test_requirements.txt"
     - "mkdir var"
 test:

problem_builder/answer.py

@@ -23,6 +23,10 @@
 import logging
 from lazy import lazy
 
+from django.core.exceptions import ValidationError
+from django.db import IntegrityError
+from django.utils.crypto import get_random_string
+
 from .models import Answer
 
 from xblock.core import XBlock
@@ -64,6 +68,40 @@ class AnswerMixin(XBlockWithPreviewMixin, XBlockWithTranslationServiceMixin):
         except AttributeError:
             return self.scope_ids.user_id
 
+    @staticmethod
+    def _fetch_model_object(name, student_id, course_id):
+        sql_query = '''SELECT * FROM problem_builder_answer
+                         WHERE name = %s
+                           AND student_id = %s
+                           AND (course_key = %s
+                                OR course_id = %s)'''
+        params = [name, student_id, course_id, course_id]
+        try:
+            answer = Answer.objects.raw(sql_query, params)[0]
+        except IndexError:
+            raise Answer.DoesNotExist()
+        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`
@@ -78,11 +116,17 @@ class AnswerMixin(XBlockWithPreviewMixin, XBlockWithTranslationServiceMixin):
         student_id = self._get_student_id()
         course_id = self._get_course_id()
 
-        answer_data, _ = Answer.objects.get_or_create(
-            student_id=student_id,
-            course_id=course_id,
-            name=name,
-        )
+        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)
+
         return answer_data
 
     @property

problem_builder/migrations/0003_auto_20161124_0755.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('problem_builder', '0002_auto_20160121_1525'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='answer',
+            name='course_key',
+            field=models.CharField(default=None, max_length=255, null=True, db_index=True),
+        ),
+        migrations.AlterUniqueTogether(
+            name='answer',
+            unique_together=set([('student_id', 'course_key', 'name'), ('student_id', 'course_id', 'name')]),
+        ),
+    ]

problem_builder/models.py

@@ -35,11 +35,19 @@ class Answer(models.Model):
     """
 
     class Meta:
-        unique_together = (('student_id', 'course_id', 'name'),)
+        unique_together = (
+            ('student_id', 'course_id', 'name'),
+            ('student_id', 'course_key', 'name'),
+        )
 
     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_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)
     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.
+"""
+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
+
+
+class TestAnswerMixin(unittest.TestCase):
+    """ Unit tests for AnswerMixin. """
+
+    FakeRuntime = namedtuple('FakeRuntime', ['course_id', 'anonymous_student_id'])
+
+    def setUp(self):
+        self.course_id = 'course-v1:edX+DemoX+Demo_Course'
+        self.anonymous_student_id = '12345678987654321'
+
+    def make_answer_mixin(self, name=None, course_id=None, student_id=None):
+        if name is None:
+            name = get_random_string()
+        if course_id is None:
+            course_id = self.course_id
+        if student_id is None:
+            student_id = self.anonymous_student_id
+        answer_mixin = AnswerMixin()
+        answer_mixin.name = name
+        answer_mixin.runtime = self.FakeRuntime(course_id, student_id)
+        return answer_mixin
+
+    def test_creates_model_instance(self):
+        name = 'test-model-creation'
+        answer_mixin = self.make_answer_mixin(name=name)
+        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)
+
+    def test_finds_instance_by_course_key(self):
+        name = 'test-course-key'
+        existing_model = Answer(
+            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
+        answer_mixin = self.make_answer_mixin(course_id=course_id)
+        model = answer_mixin.get_model_object()
+        self.assertEqual(model.course_key, course_id)

setup.py

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