From 6f9a3911e5662fd459e397a2fa6e2e834f55446a Mon Sep 17 00:00:00 2001 From: Kevin Falcone <kevin@edx.org> Date: Thu, 28 Jan 2016 10:32:28 -0500 Subject: [PATCH] Implement a BaseStudentModuleHistory This abstract class contains most of the fields (aside from the id and foreign key to StudentModule that the subclasses need to manage). It also provides a get_history method that abstracts searching across multiple backends. Move router code to openedx/core We need to use it from cms and lms. Ensure aws_migrate can be used for migrating both the lms and cms. Handle queries directed to student_module_history vs default and the extra queries generated by Django 1.8 (SAVEPOINTS, etc). Additionally, flag testing classes as multi_db so that Django will flush the non-default database between unit tests. Further decouple the foreignkey relation between csm and csmhe When calling StudentModule().delete() Django will try to delete CSMHE objects, but naively does so in the database, not by consulting the database router. Instead, we disable django cascading deletes and listen for post_delete signals and clean up CSMHE by hand. Add feature flags for CSMHE One to turn it on/off so we can control the deploy. The other will control whether or not we read from two database tables or one when searching. Update tests to explicitly use this get_history method rather than looking directly into StudentModuleHistory or StudentModuleHistoryExtended. Inform lettuce to avoid the coursewarehistoryextended app Otherwise it fails when it can't find features/ in that app. Add Pg support, this is not tested automatically. --- cms/envs/aws_migrate.py | 4 +- cms/envs/common.py | 5 + .../xmodule/modulestore/tests/django_utils.py | 4 + .../tests/test_field_override_performance.py | 74 ++++++++------- lms/djangoapps/courseware/models.py | 94 ++++++++++--------- .../courseware/tests/test_access.py | 1 + .../courseware/tests/test_grades.py | 3 + lms/djangoapps/courseware/tests/test_i18n.py | 1 + .../courseware/tests/test_model_data.py | 30 ++++-- .../tests/test_submitting_problems.py | 27 +++--- .../tests/test_user_state_client.py | 2 + .../courseware/user_state_client.py | 10 +- lms/djangoapps/courseware/views.py | 13 +-- .../coursewarehistoryextended/__init__.py | 0 .../fields.py | 6 +- .../migrations/0001_initial.py} | 18 ++-- .../migrations/__init__.py | 0 .../coursewarehistoryextended/models.py | 64 +++++++++++++ .../coursewarehistoryextended/tests.py | 81 ++++++++++++++++ lms/envs/acceptance.py | 4 +- lms/envs/aws.py | 4 + lms/envs/bok_choy.py | 5 + lms/envs/common.py | 19 +++- lms/envs/test.py | 4 + .../core/lib/django_courseware_routers.py | 20 ++-- 25 files changed, 355 insertions(+), 138 deletions(-) create mode 100644 lms/djangoapps/coursewarehistoryextended/__init__.py rename lms/djangoapps/{courseware => coursewarehistoryextended}/fields.py (71%) rename lms/djangoapps/{courseware/migrations/0002_csmh-extended-keyspace.py => coursewarehistoryextended/migrations/0001_initial.py} (67%) create mode 100644 lms/djangoapps/coursewarehistoryextended/migrations/__init__.py create mode 100644 lms/djangoapps/coursewarehistoryextended/models.py create mode 100644 lms/djangoapps/coursewarehistoryextended/tests.py rename lms/djangoapps/courseware/routers.py => openedx/core/lib/django_courseware_routers.py (58%) diff --git a/cms/envs/aws_migrate.py b/cms/envs/aws_migrate.py index e14834ec2d1..92b2d51c9e9 100644 --- a/cms/envs/aws_migrate.py +++ b/cms/envs/aws_migrate.py @@ -26,5 +26,5 @@ if DB_OVERRIDES['PASSWORD'] is None: raise ImproperlyConfigured("No database password was provided for running " "migrations. This is fatal.") -for override, value in DB_OVERRIDES.iteritems(): - DATABASES['default'][override] = value +DATABASES['default'].update(DB_OVERRIDES) +DATABASES['student_module_history'].update(DB_OVERRIDES) diff --git a/cms/envs/common.py b/cms/envs/common.py index bb99b01a984..40dd6c5ac0f 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1114,6 +1114,11 @@ PROCTORING_BACKEND_PROVIDER = { } PROCTORING_SETTINGS = {} +############################ Global Database Configuration ##################### + +DATABASE_ROUTERS = [ + 'openedx.core.lib.django_courseware_routers.StudentModuleHistoryExtendedRouter', +] ############################ OAUTH2 Provider ################################### diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 5ed4ce201e1..8120590122a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -265,6 +265,8 @@ class SharedModuleStoreTestCase(TestCase): for Django ORM models that will get cleaned up properly. """ MODULESTORE = mixed_store_config(mkdtemp_clean(), {}, include_xml=False) + # Tell Django to clean out all databases, not just default + multi_db = True @classmethod def setUpClass(cls): @@ -392,6 +394,8 @@ class ModuleStoreTestCase(TestCase): """ MODULESTORE = mixed_store_config(mkdtemp_clean(), {}, include_xml=False) + # Tell Django to clean out all databases, not just default + multi_db = True def setUp(self, **kwargs): """ diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index ffad914bee5..f8c58ff6c53 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -46,6 +46,8 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, providers. """ __test__ = False + # Tell Django to clean out all databases, not just default + multi_db = True # TEST_DATA must be overridden by subclasses TEST_DATA = None @@ -227,24 +229,24 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of mongo queries, # # of xblocks # ) - ('no_overrides', 1, True, False): (23, 1, 6, 13), - ('no_overrides', 2, True, False): (53, 16, 6, 84), - ('no_overrides', 3, True, False): (183, 81, 6, 335), - ('ccx', 1, True, False): (23, 1, 6, 13), - ('ccx', 2, True, False): (53, 16, 6, 84), - ('ccx', 3, True, False): (183, 81, 6, 335), - ('ccx', 1, True, True): (23, 1, 6, 13), - ('ccx', 2, True, True): (53, 16, 6, 84), - ('ccx', 3, True, True): (183, 81, 6, 335), - ('no_overrides', 1, False, False): (23, 1, 6, 13), - ('no_overrides', 2, False, False): (53, 16, 6, 84), - ('no_overrides', 3, False, False): (183, 81, 6, 335), - ('ccx', 1, False, False): (23, 1, 6, 13), - ('ccx', 2, False, False): (53, 16, 6, 84), - ('ccx', 3, False, False): (183, 81, 6, 335), - ('ccx', 1, False, True): (23, 1, 6, 13), - ('ccx', 2, False, True): (53, 16, 6, 84), - ('ccx', 3, False, True): (183, 81, 6, 335), + ('no_overrides', 1, True, False): (47, 1, 6, 13), + ('no_overrides', 2, True, False): (119, 16, 6, 84), + ('no_overrides', 3, True, False): (399, 81, 6, 335), + ('ccx', 1, True, False): (47, 1, 6, 13), + ('ccx', 2, True, False): (119, 16, 6, 84), + ('ccx', 3, True, False): (399, 81, 6, 335), + ('ccx', 1, True, True): (47, 1, 6, 13), + ('ccx', 2, True, True): (119, 16, 6, 84), + ('ccx', 3, True, True): (399, 81, 6, 335), + ('no_overrides', 1, False, False): (47, 1, 6, 13), + ('no_overrides', 2, False, False): (119, 16, 6, 84), + ('no_overrides', 3, False, False): (399, 81, 6, 335), + ('ccx', 1, False, False): (47, 1, 6, 13), + ('ccx', 2, False, False): (119, 16, 6, 84), + ('ccx', 3, False, False): (399, 81, 6, 335), + ('ccx', 1, False, True): (47, 1, 6, 13), + ('ccx', 2, False, True): (119, 16, 6, 84), + ('ccx', 3, False, True): (399, 81, 6, 335), } @@ -256,22 +258,22 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (23, 1, 4, 9), - ('no_overrides', 2, True, False): (53, 16, 19, 54), - ('no_overrides', 3, True, False): (183, 81, 84, 215), - ('ccx', 1, True, False): (23, 1, 4, 9), - ('ccx', 2, True, False): (53, 16, 19, 54), - ('ccx', 3, True, False): (183, 81, 84, 215), - ('ccx', 1, True, True): (25, 1, 4, 13), - ('ccx', 2, True, True): (55, 16, 19, 84), - ('ccx', 3, True, True): (185, 81, 84, 335), - ('no_overrides', 1, False, False): (23, 1, 4, 9), - ('no_overrides', 2, False, False): (53, 16, 19, 54), - ('no_overrides', 3, False, False): (183, 81, 84, 215), - ('ccx', 1, False, False): (23, 1, 4, 9), - ('ccx', 2, False, False): (53, 16, 19, 54), - ('ccx', 3, False, False): (183, 81, 84, 215), - ('ccx', 1, False, True): (23, 1, 4, 9), - ('ccx', 2, False, True): (53, 16, 19, 54), - ('ccx', 3, False, True): (183, 81, 84, 215), + ('no_overrides', 1, True, False): (47, 1, 4, 9), + ('no_overrides', 2, True, False): (119, 16, 19, 54), + ('no_overrides', 3, True, False): (399, 81, 84, 215), + ('ccx', 1, True, False): (47, 1, 4, 9), + ('ccx', 2, True, False): (119, 16, 19, 54), + ('ccx', 3, True, False): (399, 81, 84, 215), + ('ccx', 1, True, True): (49, 1, 4, 13), + ('ccx', 2, True, True): (121, 16, 19, 84), + ('ccx', 3, True, True): (401, 81, 84, 335), + ('no_overrides', 1, False, False): (47, 1, 4, 9), + ('no_overrides', 2, False, False): (119, 16, 19, 54), + ('no_overrides', 3, False, False): (399, 81, 84, 215), + ('ccx', 1, False, False): (47, 1, 4, 9), + ('ccx', 2, False, False): (119, 16, 19, 54), + ('ccx', 3, False, False): (399, 81, 84, 215), + ('ccx', 1, False, True): (47, 1, 4, 9), + ('ccx', 2, False, True): (119, 16, 19, 54), + ('ccx', 3, False, True): (399, 81, 84, 215), } diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index c67bb1fd16c..b6581f2817e 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -24,9 +24,9 @@ from django.dispatch import receiver, Signal from model_utils.models import TimeStampedModel from student.models import user_by_anonymous_id from submissions.models import score_set, score_reset +import coursewarehistoryextended from xmodule_django.models import CourseKeyField, LocationKeyField, BlockTypeKeyField -from courseware.fields import UnsignedBigIntAutoField log = logging.getLogger("edx.courseware") @@ -149,18 +149,15 @@ class StudentModule(models.Model): return unicode(repr(self)) -class StudentModuleHistory(models.Model): - """Keeps a complete history of state changes for a given XModule for a given - Student. Right now, we restrict this to problems so that the table doesn't - explode in size.""" +class BaseStudentModuleHistory(models.Model): + """Abstract class containing most fields used by any class + storing Student Module History""" objects = ChunkingManager() HISTORY_SAVING_TYPES = {'problem'} class Meta(object): - app_label = "courseware" - get_latest_by = "created" + abstract = True - student_module = models.ForeignKey(StudentModule, db_index=True) version = models.CharField(max_length=255, null=True, blank=True, db_index=True) # This should be populated from the modified field in StudentModule @@ -169,59 +166,63 @@ class StudentModuleHistory(models.Model): grade = models.FloatField(null=True, blank=True) max_grade = models.FloatField(null=True, blank=True) - @receiver(post_save, sender=StudentModule) - def save_history(sender, instance, **kwargs): # pylint: disable=no-self-argument, unused-argument + @property + def csm(self): """ - Checks the instance's module_type, and creates & saves a - StudentModuleHistory entry if the module_type is one that - we save. + Finds the StudentModule object for this history record, even if our data is split + across multiple data stores. Django does not handle this correctly with the built-in + student_module property. """ - if instance.module_type in StudentModuleHistory.HISTORY_SAVING_TYPES: - history_entry = StudentModuleHistory(student_module=instance, - version=None, - created=instance.modified, - state=instance.state, - grade=instance.grade, - max_grade=instance.max_grade) - history_entry.save() + return StudentModule.objects.get(pk=self.student_module_id) - def __unicode__(self): - return unicode(repr(self)) + @staticmethod + def get_history(student_modules): + """ + Find history objects across multiple backend stores for a given StudentModule + """ + + history_entries = [] -class StudentModuleHistoryExtended(models.Model): + if settings.FEATURES.get('ENABLE_CSMH_EXTENDED'): + history_entries += coursewarehistoryextended.models.StudentModuleHistoryExtended.objects.filter( + # Django will sometimes try to join to courseware_studentmodule + # so just do an in query + student_module__in=[module.id for module in student_modules] + ).order_by('-id') + + # If we turn off reading from multiple history tables, then we don't want to read from + # StudentModuleHistory anymore, we believe that all history is in the Extended table. + if settings.FEATURES.get('ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES'): + # we want to save later SQL queries on the model which allows us to prefetch + history_entries += StudentModuleHistory.objects.prefetch_related('student_module').filter( + student_module__in=student_modules + ).order_by('-id') + + return history_entries + + +class StudentModuleHistory(BaseStudentModuleHistory): """Keeps a complete history of state changes for a given XModule for a given Student. Right now, we restrict this to problems so that the table doesn't - explode in size. - - This new extended CSMH has a larger primary key that won't run out of space - so quickly.""" - objects = ChunkingManager() - HISTORY_SAVING_TYPES = {'problem'} + explode in size.""" class Meta(object): app_label = "courseware" get_latest_by = "created" - id = UnsignedBigIntAutoField(primary_key=True) # pylint: disable=invalid-name - - student_module = models.ForeignKey(StudentModule, db_index=True, db_constraint=False) - version = models.CharField(max_length=255, null=True, blank=True, db_index=True) + student_module = models.ForeignKey(StudentModule, db_index=True) - # This should be populated from the modified field in StudentModule - created = models.DateTimeField(db_index=True) - state = models.TextField(null=True, blank=True) - grade = models.FloatField(null=True, blank=True) - max_grade = models.FloatField(null=True, blank=True) + def __unicode__(self): + return unicode(repr(self)) - @receiver(post_save, sender=StudentModule) def save_history(sender, instance, **kwargs): # pylint: disable=no-self-argument, unused-argument """ Checks the instance's module_type, and creates & saves a - StudentModuleHistory entry if the module_type is one that + StudentModuleHistoryExtended entry if the module_type is one that we save. """ - if instance.module_type in StudentModuleHistoryExtended.HISTORY_SAVING_TYPES: - history_entry = StudentModuleHistoryExtended(student_module=instance, + if instance.module_type in StudentModuleHistory.HISTORY_SAVING_TYPES: + history_entry = StudentModuleHistory(student_module=instance, version=None, created=instance.modified, state=instance.state, @@ -229,8 +230,11 @@ class StudentModuleHistoryExtended(models.Model): max_grade=instance.max_grade) history_entry.save() - def __unicode__(self): - return unicode(repr(self)) + # When the extended studentmodulehistory table exists, don't save + # duplicate history into courseware_studentmodulehistory, just retain + # data for reading. + if not settings.FEATURES.get('ENABLE_CSMH_EXTENDED'): + post_save.connect(save_history, sender=StudentModule) class XBlockFieldBase(models.Model): diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 67566fd3f11..add8c738ee7 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -520,6 +520,7 @@ class UserRoleTestCase(TestCase): """ Tests for user roles. """ + def setUp(self): super(UserRoleTestCase, self).setUp() self.course_key = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') diff --git a/lms/djangoapps/courseware/tests/test_grades.py b/lms/djangoapps/courseware/tests/test_grades.py index 84ef2498f64..5fce9572846 100644 --- a/lms/djangoapps/courseware/tests/test_grades.py +++ b/lms/djangoapps/courseware/tests/test_grades.py @@ -240,6 +240,9 @@ class TestProgressSummary(TestCase): (2/5) (3/5) (0/1) - (1/3) - (3/10) """ + # Tell Django to clean out all databases, not just default + multi_db = True + def setUp(self): super(TestProgressSummary, self).setUp() self.course_key = CourseLocator( diff --git a/lms/djangoapps/courseware/tests/test_i18n.py b/lms/djangoapps/courseware/tests/test_i18n.py index e5ae1b6f194..345dc21ddac 100644 --- a/lms/djangoapps/courseware/tests/test_i18n.py +++ b/lms/djangoapps/courseware/tests/test_i18n.py @@ -20,6 +20,7 @@ class BaseI18nTestCase(TestCase): """ Base utilities for i18n test classes to derive from """ + def assert_tag_has_attr(self, content, tag, attname, value): """Assert that a tag in `content` has a certain value in a certain attribute.""" regex = r"""<{tag} [^>]*\b{attname}=['"]([\w\d\- ]+)['"][^>]*>""".format(tag=tag, attname=attname) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index b80f05d2a12..9f62de0875f 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -103,6 +103,8 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): """Tests for user_state storage via StudentModule""" other_key_factory = partial(DjangoKeyValueStore.Key, Scope.user_state, 2, location('usage_id')) # user_id=2, not 1 existing_field_name = "a_field" + # Tell Django to clean out all databases, not just default + multi_db = True def setUp(self): super(TestStudentModuleStorage, self).setUp() @@ -137,8 +139,9 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # to discover if something other than the DjangoXBlockUserStateClient # has written to the StudentModule (such as UserStateCache setting the score # on the StudentModule). - with self.assertNumQueries(3): - self.kvs.set(user_state_key('a_field'), 'new_value') + with self.assertNumQueries(2, using='default'): + with self.assertNumQueries(1, using='student_module_history'): + self.kvs.set(user_state_key('a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) @@ -149,8 +152,9 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # to discover if something other than the DjangoXBlockUserStateClient # has written to the StudentModule (such as UserStateCache setting the score # on the StudentModule). - with self.assertNumQueries(3): - self.kvs.set(user_state_key('not_a_field'), 'new_value') + with self.assertNumQueries(2, using='default'): + with self.assertNumQueries(1, using='student_module_history'): + self.kvs.set(user_state_key('not_a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) @@ -161,8 +165,9 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # to discover if something other than the DjangoXBlockUserStateClient # has written to the StudentModule (such as UserStateCache setting the score # on the StudentModule). - with self.assertNumQueries(3): - self.kvs.delete(user_state_key('a_field')) + with self.assertNumQueries(2, using='default'): + with self.assertNumQueries(1, using='student_module_history'): + self.kvs.delete(user_state_key('a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) @@ -201,8 +206,9 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # We also need to read the database to discover if something other than the # DjangoXBlockUserStateClient has written to the StudentModule (such as # UserStateCache setting the score on the StudentModule). - with self.assertNumQueries(3): - self.kvs.set_many(kv_dict) + with self.assertNumQueries(2, using="default"): + with self.assertNumQueries(1, using="student_module_history"): + self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -223,6 +229,9 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): @attr('shard_1') class TestMissingStudentModule(TestCase): + # Tell Django to clean out all databases, not just default + multi_db = True + def setUp(self): super(TestMissingStudentModule, self).setUp() @@ -244,12 +253,13 @@ class TestMissingStudentModule(TestCase): self.assertEquals(0, len(self.field_data_cache)) self.assertEquals(0, StudentModule.objects.all().count()) - # We are updating a problem, so we write to courseware_studentmodulehistory + # We are updating a problem, so we write to courseware_studentmodulehistoryextended # as well as courseware_studentmodule. We also need to read the database # to discover if something other than the DjangoXBlockUserStateClient # has written to the StudentModule (such as UserStateCache setting the score # on the StudentModule). - with self.assertNumQueries(2, using='default'): + # Django 1.8 also has a number of other BEGIN and SAVESTATE queries. + with self.assertNumQueries(4, using='default'): with self.assertNumQueries(1, using='student_module_history'): self.kvs.set(user_state_key('a_field'), 'a_value') diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 23a7ae2aa60..8a7761a322d 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -19,7 +19,7 @@ from capa.tests.response_xml_factory import ( CodeResponseXMLFactory, ) from courseware import grades -from courseware.models import StudentModule, StudentModuleHistory +from courseware.models import StudentModule, BaseStudentModuleHistory from courseware.tests.helpers import LoginEnrollmentTestCase from lms.djangoapps.lms_xblock.runtime import quote_slashes from student.tests.factories import UserFactory @@ -121,6 +121,8 @@ class TestSubmittingProblems(ModuleStoreTestCase, LoginEnrollmentTestCase, Probl Check that a course gets graded properly. """ + # Tell Django to clean out all databases, not just default + multi_db = True # arbitrary constant COURSE_SLUG = "100" COURSE_NAME = "test_course" @@ -319,6 +321,9 @@ class TestCourseGrader(TestSubmittingProblems): """ Suite of tests for the course grader. """ + # Tell Django to clean out all databases, not just default + multi_db = True + def basic_setup(self, late=False, reset=False, showanswer=False): """ Set up a simple course for testing basic grading functionality. @@ -451,26 +456,20 @@ class TestCourseGrader(TestSubmittingProblems): self.submit_question_answer('p1', {'2_1': u'Correct'}) # Now fetch the state entry for that problem. - student_module = StudentModule.objects.get( + student_module = StudentModule.objects.filter( course_id=self.course.id, student=self.student_user ) # count how many state history entries there are - baseline = StudentModuleHistory.objects.filter( - student_module=student_module - ) - baseline_count = baseline.count() - self.assertEqual(baseline_count, 3) + baseline = BaseStudentModuleHistory.get_history(student_module) + self.assertEqual(len(baseline), 3) # now click "show answer" self.show_question_answer('p1') # check that we don't have more state history entries - csmh = StudentModuleHistory.objects.filter( - student_module=student_module - ) - current_count = csmh.count() - self.assertEqual(current_count, 3) + csmh = BaseStudentModuleHistory.get_history(student_module) + self.assertEqual(len(csmh), 3) def test_grade_with_max_score_cache(self): """ @@ -713,6 +712,8 @@ class TestCourseGrader(TestSubmittingProblems): @attr('shard_1') class ProblemWithUploadedFilesTest(TestSubmittingProblems): """Tests of problems with uploaded files.""" + # Tell Django to clean out all databases, not just default + multi_db = True def setUp(self): super(ProblemWithUploadedFilesTest, self).setUp() @@ -768,6 +769,8 @@ class TestPythonGradedResponse(TestSubmittingProblems): """ Check that we can submit a schematic and custom response, and it answers properly. """ + # Tell Django to clean out all databases, not just default + multi_db = True SCHEMATIC_SCRIPT = dedent(""" # for a schematic response, submission[i] is the json representation diff --git a/lms/djangoapps/courseware/tests/test_user_state_client.py b/lms/djangoapps/courseware/tests/test_user_state_client.py index 5bb9a0682b2..143bd7f644c 100644 --- a/lms/djangoapps/courseware/tests/test_user_state_client.py +++ b/lms/djangoapps/courseware/tests/test_user_state_client.py @@ -18,6 +18,8 @@ class TestDjangoUserStateClient(UserStateClientTestBase, TestCase): Tests of the DjangoUserStateClient backend. """ __test__ = True + # Tell Django to clean out all databases, not just default + multi_db = True def _user(self, user_idx): return self.users[user_idx].username diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index e90eeb8f474..b965a15ebe0 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -15,7 +15,7 @@ except ImportError: import dogstats_wrapper as dog_stats_api from django.contrib.auth.models import User from xblock.fields import Scope, ScopeBase -from courseware.models import StudentModule, StudentModuleHistory +from courseware.models import StudentModule, BaseStudentModuleHistory from edx_user_state_client.interface import XBlockUserStateClient, XBlockUserState @@ -312,9 +312,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): if len(student_modules) == 0: raise self.DoesNotExist() - history_entries = StudentModuleHistory.objects.prefetch_related('student_module').filter( - student_module__in=student_modules - ).order_by('-id') + history_entries = BaseStudentModuleHistory.get_history(student_modules) # If no history records exist, raise an error if not history_entries: @@ -332,9 +330,9 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): if state == {}: state = None - block_key = history_entry.student_module.module_state_key + block_key = history_entry.csm.module_state_key block_key = block_key.map_into_course( - history_entry.student_module.course_id + history_entry.csm.course_id ) yield XBlockUserState(username, block_key, state, history_entry.created, scope) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 73a43345c10..7d4cde32abb 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -59,7 +59,7 @@ from courseware.courses import ( ) from courseware.masquerade import setup_masquerade from courseware.model_data import FieldDataCache, ScoresClient -from courseware.models import StudentModuleHistory +from courseware.models import StudentModule, BaseStudentModuleHistory from courseware.url_helpers import get_redirect_url from courseware.user_state_client import DjangoXBlockUserStateClient from edxmako.shortcuts import render_to_response, render_to_string, marketing_link @@ -1173,11 +1173,12 @@ def submission_history(request, course_id, student_username, location): # This is ugly, but until we have a proper submissions API that we can use to provide # the scores instead, it will have to do. - scores = list(StudentModuleHistory.objects.filter( - student_module__module_state_key=usage_key, - student_module__student__username=student_username, - student_module__course_id=course_key - ).order_by('-id')) + csm = StudentModule.objects.filter( + module_state_key=usage_key, + student__username=student_username, + course_id=course_key) + + scores = BaseStudentModuleHistory.get_history(csm) if len(scores) != len(history_entries): log.warning( diff --git a/lms/djangoapps/coursewarehistoryextended/__init__.py b/lms/djangoapps/coursewarehistoryextended/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/lms/djangoapps/courseware/fields.py b/lms/djangoapps/coursewarehistoryextended/fields.py similarity index 71% rename from lms/djangoapps/courseware/fields.py rename to lms/djangoapps/coursewarehistoryextended/fields.py index 6e34c4e635f..a001543d319 100644 --- a/lms/djangoapps/courseware/fields.py +++ b/lms/djangoapps/coursewarehistoryextended/fields.py @@ -1,5 +1,5 @@ """ -Custom fields for use in the courseware django app. +Custom fields for use in the coursewarehistoryextended django app. """ from django.db.models.fields import AutoField @@ -17,5 +17,9 @@ class UnsignedBigIntAutoField(AutoField): # is an alias for that (https://www.sqlite.org/autoinc.html). An unsigned integer # isn't an alias for ROWID, so we have to give up on the unsigned part. return "integer" + elif connection.settings_dict['ENGINE'] == 'django.db.backends.postgresql_psycopg2': + # Pg's bigserial is implicitly unsigned (doesn't allow negative numbers) and + # goes 1-9.2x10^18 + return "BIGSERIAL" else: return None diff --git a/lms/djangoapps/courseware/migrations/0002_csmh-extended-keyspace.py b/lms/djangoapps/coursewarehistoryextended/migrations/0001_initial.py similarity index 67% rename from lms/djangoapps/courseware/migrations/0002_csmh-extended-keyspace.py rename to lms/djangoapps/coursewarehistoryextended/migrations/0001_initial.py index 8ee63b6f3b8..370327f9f2a 100644 --- a/lms/djangoapps/courseware/migrations/0002_csmh-extended-keyspace.py +++ b/lms/djangoapps/coursewarehistoryextended/migrations/0001_initial.py @@ -2,18 +2,21 @@ from __future__ import unicode_literals from django.db import migrations, models -import courseware.fields from django.conf import settings - +import django.db.models.deletion +import coursewarehistoryextended.fields def bump_pk_start(apps, schema_editor): if not schema_editor.connection.alias == 'student_module_history': return StudentModuleHistory = apps.get_model("courseware", "StudentModuleHistory") - initial_id = settings.STUDENTMODULEHISTORYEXTENDED_OFFSET + StudentModuleHistory.objects.all().latest('id').id + biggest_id = StudentModuleHistory.objects.all().order_by('-id').first() + initial_id = settings.STUDENTMODULEHISTORYEXTENDED_OFFSET + if biggest_id is not None: + initial_id += biggest_id.id if schema_editor.connection.vendor == 'mysql': - schema_editor.execute('ALTER TABLE courseware_studentmodulehistoryextended AUTO_INCREMENT=%s', [initial_id]) + schema_editor.execute('ALTER TABLE coursewarehistoryextended_studentmodulehistoryextended AUTO_INCREMENT=%s', [initial_id]) elif schema_editor.connection.vendor == 'sqlite3': # This is a hack to force sqlite to add new rows after the earlier rows we # want to migrate. @@ -25,7 +28,8 @@ def bump_pk_start(apps, schema_editor): version="", created=datetime.datetime.now(), ).save() - + elif schema_editor.connection.vendor == 'postgresql': + schema_editor.execute("SELECT setval('coursewarehistoryextended_studentmodulehistoryextended_seq', %s)", [initial_id]) class Migration(migrations.Migration): @@ -37,13 +41,13 @@ class Migration(migrations.Migration): migrations.CreateModel( name='StudentModuleHistoryExtended', fields=[ - ('id', courseware.fields.UnsignedBigIntAutoField(serialize=False, primary_key=True)), ('version', models.CharField(db_index=True, max_length=255, null=True, blank=True)), ('created', models.DateTimeField(db_index=True)), ('state', models.TextField(null=True, blank=True)), ('grade', models.FloatField(null=True, blank=True)), ('max_grade', models.FloatField(null=True, blank=True)), - ('student_module', models.ForeignKey(to='courseware.StudentModule', db_constraint=False)), + ('id', coursewarehistoryextended.fields.UnsignedBigIntAutoField(serialize=False, primary_key=True)), + ('student_module', models.ForeignKey(to='courseware.StudentModule', on_delete=django.db.models.deletion.DO_NOTHING, db_constraint=False)), ], options={ 'get_latest_by': 'created', diff --git a/lms/djangoapps/coursewarehistoryextended/migrations/__init__.py b/lms/djangoapps/coursewarehistoryextended/migrations/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/lms/djangoapps/coursewarehistoryextended/models.py b/lms/djangoapps/coursewarehistoryextended/models.py new file mode 100644 index 00000000000..8c3630b0e3d --- /dev/null +++ b/lms/djangoapps/coursewarehistoryextended/models.py @@ -0,0 +1,64 @@ +""" +WE'RE USING MIGRATIONS! + +If you make changes to this model, be sure to create an appropriate migration +file and check it in at the same time as your model changes. To do that, + +1. Go to the edx-platform dir +2. ./manage.py schemamigration courseware --auto description_of_your_change +3. Add the migration file created in edx-platform/lms/djangoapps/coursewarehistoryextended/migrations/ + + +ASSUMPTIONS: modules have unique IDs, even across different module_types + +""" +from django.db import models +from django.db.models.signals import post_save, post_delete +from django.dispatch import receiver + +from coursewarehistoryextended.fields import UnsignedBigIntAutoField +from courseware.models import StudentModule, BaseStudentModuleHistory + + +class StudentModuleHistoryExtended(BaseStudentModuleHistory): + """Keeps a complete history of state changes for a given XModule for a given + Student. Right now, we restrict this to problems so that the table doesn't + explode in size. + + This new extended CSMH has a larger primary key that won't run out of space + so quickly.""" + + class Meta(object): + app_label = 'coursewarehistoryextended' + get_latest_by = "created" + + id = UnsignedBigIntAutoField(primary_key=True) # pylint: disable=invalid-name + + student_module = models.ForeignKey(StudentModule, db_index=True, db_constraint=False, on_delete=models.DO_NOTHING) + + @receiver(post_save, sender=StudentModule) + def save_history(sender, instance, **kwargs): # pylint: disable=no-self-argument, unused-argument + """ + Checks the instance's module_type, and creates & saves a + StudentModuleHistoryExtended entry if the module_type is one that + we save. + """ + if instance.module_type in StudentModuleHistoryExtended.HISTORY_SAVING_TYPES: + history_entry = StudentModuleHistoryExtended(student_module=instance, + version=None, + created=instance.modified, + state=instance.state, + grade=instance.grade, + max_grade=instance.max_grade) + history_entry.save() + + @receiver(post_delete, sender=StudentModule) + def delete_history(sender, instance, **kwargs): # pylint: disable=no-self-argument, unused-argument + """ + Django can't cascade delete across databases, so we tell it at the model level to + on_delete=DO_NOTHING and then listen for post_delete so we can clean up the CSMHE rows. + """ + StudentModuleHistoryExtended.objects.filter(student_module=instance).all().delete() + + def __unicode__(self): + return unicode(repr(self)) diff --git a/lms/djangoapps/coursewarehistoryextended/tests.py b/lms/djangoapps/coursewarehistoryextended/tests.py new file mode 100644 index 00000000000..759fdfe4be2 --- /dev/null +++ b/lms/djangoapps/coursewarehistoryextended/tests.py @@ -0,0 +1,81 @@ +""" +Tests for coursewarehistoryextended +Many aspects of this app are covered by the courseware tests, +but these are specific to the new storage model with multiple +backend tables. +""" + +import json +from mock import patch +from django.test import TestCase +from django.conf import settings +from unittest import skipUnless +from nose.plugins.attrib import attr + +from courseware.models import BaseStudentModuleHistory, StudentModuleHistory, StudentModule + +from courseware.tests.factories import StudentModuleFactory, location, course_id + + +@attr('shard_1') +@skipUnless(settings.FEATURES["ENABLE_CSMH_EXTENDED"], "CSMH Extended needs to be enabled") +class TestStudentModuleHistoryBackends(TestCase): + """ Tests of data in CSMH and CSMHE """ + # Tell Django to clean out all databases, not just default + multi_db = True + + def setUp(self): + super(TestStudentModuleHistoryBackends, self).setUp() + for record in (1, 2, 3): + # This will store into CSMHE via the post_save signal + csm = StudentModuleFactory.create(module_state_key=location('usage_id'), + course_id=course_id, + state=json.dumps({'type': 'csmhe', 'order': record})) + # This manually gets us a CSMH record to compare + csmh = StudentModuleHistory(student_module=csm, + version=None, + created=csm.modified, + state=json.dumps({'type': 'csmh', 'order': record}), + grade=csm.grade, + max_grade=csm.max_grade) + csmh.save() + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_CSMH_EXTENDED": True}) + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES": True}) + def test_get_history_true_true(self): + student_module = StudentModule.objects.all() + history = BaseStudentModuleHistory.get_history(student_module) + self.assertEquals(len(history), 6) + self.assertEquals({'type': 'csmhe', 'order': 3}, json.loads(history[0].state)) + self.assertEquals({'type': 'csmhe', 'order': 2}, json.loads(history[1].state)) + self.assertEquals({'type': 'csmhe', 'order': 1}, json.loads(history[2].state)) + self.assertEquals({'type': 'csmh', 'order': 3}, json.loads(history[3].state)) + self.assertEquals({'type': 'csmh', 'order': 2}, json.loads(history[4].state)) + self.assertEquals({'type': 'csmh', 'order': 1}, json.loads(history[5].state)) + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_CSMH_EXTENDED": True}) + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES": False}) + def test_get_history_true_false(self): + student_module = StudentModule.objects.all() + history = BaseStudentModuleHistory.get_history(student_module) + self.assertEquals(len(history), 3) + self.assertEquals({'type': 'csmhe', 'order': 3}, json.loads(history[0].state)) + self.assertEquals({'type': 'csmhe', 'order': 2}, json.loads(history[1].state)) + self.assertEquals({'type': 'csmhe', 'order': 1}, json.loads(history[2].state)) + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_CSMH_EXTENDED": False}) + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES": True}) + def test_get_history_false_true(self): + student_module = StudentModule.objects.all() + history = BaseStudentModuleHistory.get_history(student_module) + self.assertEquals(len(history), 3) + self.assertEquals({'type': 'csmh', 'order': 3}, json.loads(history[0].state)) + self.assertEquals({'type': 'csmh', 'order': 2}, json.loads(history[1].state)) + self.assertEquals({'type': 'csmh', 'order': 1}, json.loads(history[2].state)) + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_CSMH_EXTENDED": False}) + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES": False}) + def test_get_history_false_false(self): + student_module = StudentModule.objects.all() + history = BaseStudentModuleHistory.get_history(student_module) + self.assertEquals(len(history), 0) diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py index 41c64afe56d..101e7bb7ecb 100644 --- a/lms/envs/acceptance.py +++ b/lms/envs/acceptance.py @@ -153,7 +153,9 @@ LETTUCE_APPS = ('courseware', 'instructor') # This causes some pretty cryptic errors as lettuce tries # to parse files in `instructor_task` as features. # As a quick workaround, explicitly exclude the `instructor_task` app. -LETTUCE_AVOID_APPS = ('instructor_task',) +# The coursewarehistoryextended app also falls prey to this fuzzy +# for the courseware app. +LETTUCE_AVOID_APPS = ('instructor_task', 'coursewarehistoryextended') LETTUCE_BROWSER = os.environ.get('LETTUCE_BROWSER', 'chrome') diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 6f593d139c0..d5935394af0 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -771,3 +771,7 @@ if ENV_TOKENS.get('AUDIT_CERT_CUTOFF_DATE', None): ################################ Settings for Credentials Service ################################ CREDENTIALS_GENERATION_ROUTING_KEY = HIGH_PRIORITY_QUEUE + +# The extended StudentModule history table +if FEATURES.get('ENABLE_CSMH_EXTENDED'): + INSTALLED_APPS += ('coursewarehistoryextended',) diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index 1990473417e..cb74f9a87f1 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -178,6 +178,11 @@ PROFILE_IMAGE_BACKEND = { 'base_url': os.path.join(MEDIA_URL, 'profile-images/'), }, } + +# Make sure we test with the extended history table +FEATURES['ENABLE_CSMH_EXTENDED'] = True +INSTALLED_APPS += ('coursewarehistoryextended',) + ##################################################################### # Lastly, see if the developer has any local overrides. try: diff --git a/lms/envs/common.py b/lms/envs/common.py index fa7cbe68fc6..c73eaae4517 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -363,6 +363,18 @@ FEATURES = { # Show Language selector. 'SHOW_LANGUAGE_SELECTOR': False, + + # Write new CSM history to the extended table. + # This will eventually default to True and may be + # removed since all installs should have the separate + # extended history table. + 'ENABLE_CSMH_EXTENDED': False, + + # Read from both the CSMH and CSMHE history tables. + # This is the default, but can be disabled if all history + # lives in the Extended table, saving the frontend from + # making multiple queries. + 'ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES': True } # Ignore static asset files on import which match this pattern @@ -416,7 +428,7 @@ STATUS_MESSAGE_PATH = ENV_ROOT / "status_message.json" ############################ Global Database Configuration ##################### DATABASE_ROUTERS = [ - 'courseware.routers.StudentModuleHistoryRouter', + 'openedx.core.lib.django_courseware_routers.StudentModuleHistoryExtendedRouter', ] ############################ OpenID Provider ################################## @@ -2766,7 +2778,10 @@ MOBILE_APP_USER_AGENT_REGEXES = [ ] # Offset for courseware.StudentModuleHistoryExtended which is used to -# calculate the starting primary key for the underlying table. +# calculate the starting primary key for the underlying table. This gap +# should be large enough that you do not generate more than N courseware.StudentModuleHistory +# records before you have deployed the app to write to coursewarehistoryextended.StudentModuleHistoryExtended +# if you want to avoid an overlap in ids while searching for history across the two tables. STUDENTMODULEHISTORYEXTENDED_OFFSET = 10000 # Deprecated xblock types diff --git a/lms/envs/test.py b/lms/envs/test.py index 111de79ca9f..83f55c337cc 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -197,6 +197,10 @@ if os.environ.get('DISABLE_MIGRATIONS'): # to Django 1.9, which allows setting MIGRATION_MODULES to None in order to skip migrations. MIGRATION_MODULES = NoOpMigrationModules() +# Make sure we test with the extended history table +FEATURES['ENABLE_CSMH_EXTENDED'] = True +INSTALLED_APPS += ('coursewarehistoryextended',) + CACHES = { # This is the cache used for most things. # In staging/prod envs, the sessions also live here. diff --git a/lms/djangoapps/courseware/routers.py b/openedx/core/lib/django_courseware_routers.py similarity index 58% rename from lms/djangoapps/courseware/routers.py rename to openedx/core/lib/django_courseware_routers.py index 1514c102597..4665efe47f4 100644 --- a/lms/djangoapps/courseware/routers.py +++ b/openedx/core/lib/django_courseware_routers.py @@ -1,27 +1,27 @@ """ -Database Routers for use with the courseware django app. +Database Routers for use with the coursewarehistoryextended django app. """ -class StudentModuleHistoryRouter(object): +class StudentModuleHistoryExtendedRouter(object): """ - A Database Router that separates StudentModuleHistory into its own database. + A Database Router that separates StudentModuleHistoryExtended into its own database. """ DATABASE_NAME = 'student_module_history' def _is_csmh(self, model): """ - Return True if ``model`` is courseware.StudentModuleHistory. + Return True if ``model`` is courseware.StudentModuleHistoryExtended. """ return ( - model._meta.app_label == 'courseware' and # pylint: disable=protected-access - model.__name__ == 'StudentModuleHistory' + model._meta.app_label == 'coursewarehistoryextended' and # pylint: disable=protected-access + model.__name__ == 'StudentModuleHistoryExtended' ) def db_for_read(self, model, **hints): # pylint: disable=unused-argument """ - Use the StudentModuleHistoryRouter.DATABASE_NAME if the model is StudentModuleHistory. + Use the StudentModuleHistoryExtendedRouter.DATABASE_NAME if the model is StudentModuleHistoryExtended. """ if self._is_csmh(model): return self.DATABASE_NAME @@ -30,7 +30,7 @@ class StudentModuleHistoryRouter(object): def db_for_write(self, model, **hints): # pylint: disable=unused-argument """ - Use the StudentModuleHistoryRouter.DATABASE_NAME if the model is StudentModuleHistory. + Use the StudentModuleHistoryExtendedRouter.DATABASE_NAME if the model is StudentModuleHistoryExtended. """ if self._is_csmh(model): return self.DATABASE_NAME @@ -39,7 +39,7 @@ class StudentModuleHistoryRouter(object): def allow_relation(self, obj1, obj2, **hints): # pylint: disable=unused-argument """ - Disable relations if the model is StudentModuleHistory. + Disable relations if the model is StudentModuleHistoryExtended. """ if self._is_csmh(obj1) or self._is_csmh(obj2): return False @@ -47,7 +47,7 @@ class StudentModuleHistoryRouter(object): def allow_migrate(self, db, model): # pylint: disable=unused-argument """ - Only sync StudentModuleHistory to StudentModuleHistoryRouter.DATABASE_Name + Only sync StudentModuleHistoryExtended to StudentModuleHistoryExtendedRouter.DATABASE_Name """ if self._is_csmh(model): return db == self.DATABASE_NAME -- GitLab