diff --git a/cms/envs/aws_migrate.py b/cms/envs/aws_migrate.py index e14834ec2d..92b2d51c9e 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 bb99b01a98..40dd6c5ac0 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 5ed4ce201e..8120590122 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 ffad914bee..f8c58ff6c5 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 c67bb1fd16..b6581f2817 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,11 +166,59 @@ class StudentModuleHistory(models.Model): grade = models.FloatField(null=True, blank=True) max_grade = models.FloatField(null=True, blank=True) - @receiver(post_save, sender=StudentModule) + @property + def csm(self): + """ + 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. + """ + return StudentModule.objects.get(pk=self.student_module_id) + + @staticmethod + def get_history(student_modules): + """ + Find history objects across multiple backend stores for a given StudentModule + """ + + history_entries = [] + + 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.""" + + class Meta(object): + app_label = "courseware" + get_latest_by = "created" + + student_module = models.ForeignKey(StudentModule, db_index=True) + + def __unicode__(self): + return unicode(repr(self)) + 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 StudentModuleHistory.HISTORY_SAVING_TYPES: @@ -185,52 +230,11 @@ class StudentModuleHistory(models.Model): max_grade=instance.max_grade) history_entry.save() - def __unicode__(self): - return unicode(repr(self)) - -class StudentModuleHistoryExtended(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. - - This new extended CSMH has a larger primary key that won't run out of space - so quickly.""" - objects = ChunkingManager() - HISTORY_SAVING_TYPES = {'problem'} - - 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) - - # 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) - - @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 - 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() - - 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 67566fd3f1..add8c738ee 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 84ef2498f6..5fce957284 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 e5ae1b6f19..345dc21dda 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 b80f05d2a1..9f62de0875 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 23a7ae2aa6..8a7761a322 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 5bb9a0682b..143bd7f644 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 e90eeb8f47..b965a15ebe 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 73a43345c1..7d4cde32ab 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 0000000000..e69de29bb2 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 6e34c4e635..a001543d31 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 8ee63b6f3b..370327f9f2 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 0000000000..e69de29bb2 diff --git a/lms/djangoapps/coursewarehistoryextended/models.py b/lms/djangoapps/coursewarehistoryextended/models.py new file mode 100644 index 0000000000..8c3630b0e3 --- /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 0000000000..759fdfe4be --- /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 41c64afe56..101e7bb7ec 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 6f593d139c..d5935394af 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 1990473417..cb74f9a87f 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 fa7cbe68fc..c73eaae451 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 111de79ca9..83f55c337c 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 1514c10259..4665efe47f 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