From b85556701b717157dffdb8da052753000245fd81 Mon Sep 17 00:00:00 2001 From: Matjaz Gregoric Date: Wed, 23 Nov 2016 09:18:09 +0100 Subject: [PATCH 1/2] Introduce Answer.course_key and deprecate course_id. This introduces a new 255-char course_key field to the Answer model. The old 50-char course_id field is still present, but is deprecated and will be removed in next release. We cannot simply extend the existing course_id field because we have some large problem_builder_answer tables in production and the migration to extend the column would lock the table causing issues in production. The code is updated so that it uses the new course_key column, but falls back to course_id. In next release, a data migration to copy course_id data to course_key will be provided and the course_id column dropped. --- problem_builder/answer.py | 54 +++++++++++-- .../migrations/0003_auto_20161124_0755.py | 23 ++++++ problem_builder/models.py | 10 ++- .../tests/unit/test_answer_mixin.py | 78 +++++++++++++++++++ 4 files changed, 159 insertions(+), 6 deletions(-) create mode 100644 problem_builder/migrations/0003_auto_20161124_0755.py create mode 100644 problem_builder/tests/unit/test_answer_mixin.py diff --git a/problem_builder/answer.py b/problem_builder/answer.py index cc3fc0a1..4a34c465 100644 --- a/problem_builder/answer.py +++ b/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 @@ def _get_student_id(self): 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 @@ def get_model_object(self, name=None): 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 diff --git a/problem_builder/migrations/0003_auto_20161124_0755.py b/problem_builder/migrations/0003_auto_20161124_0755.py new file mode 100644 index 00000000..1da75161 --- /dev/null +++ b/problem_builder/migrations/0003_auto_20161124_0755.py @@ -0,0 +1,23 @@ +# -*- 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')]), + ), + ] diff --git a/problem_builder/models.py b/problem_builder/models.py index 5afc3df7..275324e9 100644 --- a/problem_builder/models.py +++ b/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) diff --git a/problem_builder/tests/unit/test_answer_mixin.py b/problem_builder/tests/unit/test_answer_mixin.py new file mode 100644 index 00000000..872ce870 --- /dev/null +++ b/problem_builder/tests/unit/test_answer_mixin.py @@ -0,0 +1,78 @@ +""" +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) From 113d5d6f207db656904e88b3610e662cc4c7d8e2 Mon Sep 17 00:00:00 2001 From: Matjaz Gregoric Date: Fri, 25 Nov 2016 08:14:42 +0100 Subject: [PATCH 2/2] Bump version to 2.6.4. --- circle.yml | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/circle.yml b/circle.yml index a34a8086..a3a83ded 100644 --- a/circle.yml +++ b/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: diff --git a/setup.py b/setup.py index 2a97aa82..4a1b87cf 100644 --- a/setup.py +++ b/setup.py @@ -71,7 +71,7 @@ def package_data(pkg, root_list): 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=[