From f44d794e631cdbddca02f889e73fc6ebfe504c2b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 20 Aug 2012 16:39:52 -0400 Subject: [PATCH] Add course_id to StudentModule * Update all uses. --- lms/djangoapps/courseware/grades.py | 9 +- .../management/commands/check_course.py | 1 + .../0004_add_field_studentmodule_course_id.py | 112 ++++++++++++++++++ lms/djangoapps/courseware/models.py | 59 +++++---- lms/djangoapps/courseware/module_render.py | 26 ++-- lms/djangoapps/courseware/views.py | 6 +- 6 files changed, 172 insertions(+), 41 deletions(-) create mode 100644 lms/djangoapps/courseware/migrations/0004_add_field_studentmodule_course_id.py diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 3d8d8b0ebe..c913f2d5e5 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -42,7 +42,7 @@ def grade(student, request, course, student_module_cache=None): grading_context = course.grading_context if student_module_cache == None: - student_module_cache = StudentModuleCache(student, grading_context['all_descriptors']) + student_module_cache = StudentModuleCache(course.id, student, grading_context['all_descriptors']) totaled_scores = {} # This next complicated loop is just to collect the totaled_scores, which is @@ -56,7 +56,8 @@ def grade(student, request, course, student_module_cache=None): should_grade_section = False # If we haven't seen a single problem in the section, we don't have to grade it at all! We can assume 0% for moduledescriptor in section['xmoduledescriptors']: - if student_module_cache.lookup(moduledescriptor.category, moduledescriptor.location.url() ): + if student_module_cache.lookup( + course.id, moduledescriptor.category, moduledescriptor.location.url()): should_grade_section = True break @@ -64,10 +65,9 @@ def grade(student, request, course, student_module_cache=None): scores = [] # TODO: We need the request to pass into here. If we could forgo that, our arguments # would be simpler - course_id = CourseDescriptor.location_to_id(course.location) section_module = get_module(student, request, section_descriptor.location, student_module_cache, - course_id) + course.id) if section_module is None: # student doesn't have access to this module, or something else # went wrong. @@ -219,6 +219,7 @@ def get_score(user, problem, student_module_cache): # instance_module = student_module_cache.lookup(problem.category, problem.id) # if instance_module is None: # instance_module = StudentModule(module_type=problem.category, + # course_id=????, # module_state_key=problem.id, # student=user, # state=None, diff --git a/lms/djangoapps/courseware/management/commands/check_course.py b/lms/djangoapps/courseware/management/commands/check_course.py index 4b44581c3f..adb8bff709 100644 --- a/lms/djangoapps/courseware/management/commands/check_course.py +++ b/lms/djangoapps/courseware/management/commands/check_course.py @@ -84,6 +84,7 @@ class Command(BaseCommand): # TODO (cpennington): Get coursename in a legitimate way course_location = 'i4x://edx/6002xs12/course/6.002_Spring_2012' student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( + course_id, sample_user, modulestore().get_item(course_location)) course = get_module(sample_user, None, course_location, student_module_cache) diff --git a/lms/djangoapps/courseware/migrations/0004_add_field_studentmodule_course_id.py b/lms/djangoapps/courseware/migrations/0004_add_field_studentmodule_course_id.py new file mode 100644 index 0000000000..71154e17fd --- /dev/null +++ b/lms/djangoapps/courseware/migrations/0004_add_field_studentmodule_course_id.py @@ -0,0 +1,112 @@ +# -*- coding: utf-8 -*- +import datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding field 'StudentModule.course_id' + db.add_column('courseware_studentmodule', 'course_id', + self.gf('django.db.models.fields.CharField')(default="", max_length=255, db_index=True), + keep_default=False) + + # Removing unique constraint on 'StudentModule', fields ['module_id', 'student'] + db.delete_unique('courseware_studentmodule', ['module_id', 'student_id']) + + # Adding unique constraint on 'StudentModule', fields ['course_id', 'module_state_key', 'student'] + db.create_unique('courseware_studentmodule', ['course_id', 'module_id', 'student_id']) + + + def backwards(self, orm): + # Removing unique constraint on 'StudentModule', fields ['course_id', 'module_state_key', 'student'] + db.delete_unique('courseware_studentmodule', ['course_id', 'module_id', 'student_id']) + + # Deleting field 'StudentModule.course_id' + db.delete_column('courseware_studentmodule', 'course_id') + + # Adding unique constraint on 'StudentModule', fields ['module_id', 'student'] + db.create_unique('courseware_studentmodule', ['module_id', 'student_id']) + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'about': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'avatar_type': ('django.db.models.fields.CharField', [], {'default': "'n'", 'max_length': '1'}), + 'bronze': ('django.db.models.fields.SmallIntegerField', [], {'default': '0'}), + 'consecutive_days_visit_count': ('django.db.models.fields.IntegerField', [], {'default': '0'}), + 'country': ('django_countries.fields.CountryField', [], {'max_length': '2', 'blank': 'True'}), + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'date_of_birth': ('django.db.models.fields.DateField', [], {'null': 'True', 'blank': 'True'}), + 'display_tag_filter_strategy': ('django.db.models.fields.SmallIntegerField', [], {'default': '0'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'email_isvalid': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'email_key': ('django.db.models.fields.CharField', [], {'max_length': '32', 'null': 'True'}), + 'email_tag_filter_strategy': ('django.db.models.fields.SmallIntegerField', [], {'default': '1'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'gold': ('django.db.models.fields.SmallIntegerField', [], {'default': '0'}), + 'gravatar': ('django.db.models.fields.CharField', [], {'max_length': '32'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'ignored_tags': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'interesting_tags': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'last_seen': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'location': ('django.db.models.fields.CharField', [], {'max_length': '100', 'blank': 'True'}), + 'new_response_count': ('django.db.models.fields.IntegerField', [], {'default': '0'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'questions_per_page': ('django.db.models.fields.SmallIntegerField', [], {'default': '10'}), + 'real_name': ('django.db.models.fields.CharField', [], {'max_length': '100', 'blank': 'True'}), + 'reputation': ('django.db.models.fields.PositiveIntegerField', [], {'default': '1'}), + 'seen_response_count': ('django.db.models.fields.IntegerField', [], {'default': '0'}), + 'show_country': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'silver': ('django.db.models.fields.SmallIntegerField', [], {'default': '0'}), + 'status': ('django.db.models.fields.CharField', [], {'default': "'w'", 'max_length': '2'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}), + 'website': ('django.db.models.fields.URLField', [], {'max_length': '200', 'blank': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, + 'courseware.studentmodule': { + 'Meta': {'unique_together': "(('course_id', 'student', 'module_state_key'),)", 'object_name': 'StudentModule'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'db_index': 'True', 'blank': 'True'}), + 'done': ('django.db.models.fields.CharField', [], {'default': "'na'", 'max_length': '8', 'db_index': 'True'}), + 'grade': ('django.db.models.fields.FloatField', [], {'db_index': 'True', 'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'max_grade': ('django.db.models.fields.FloatField', [], {'null': 'True', 'blank': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'db_index': 'True', 'blank': 'True'}), + 'module_state_key': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_column': "'module_id'", 'db_index': 'True'}), + 'module_type': ('django.db.models.fields.CharField', [], {'default': "'problem'", 'max_length': '32', 'db_index': 'True'}), + 'state': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'student': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) + } + } + + complete_apps = ['courseware'] diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index 4389a5f169..4fdedfcdd3 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -22,6 +22,9 @@ from django.contrib.auth.models import User class StudentModule(models.Model): + """ + Keeps student state for a particular module in a particular course. + """ # For a homework problem, contains a JSON # object consisting of state MODULE_TYPES = (('problem', 'problem'), @@ -37,9 +40,10 @@ class StudentModule(models.Model): # Filename for homeworks, etc. module_state_key = models.CharField(max_length=255, db_index=True, db_column='module_id') student = models.ForeignKey(User, db_index=True) + course_id = models.CharField(max_length=255, db_index=True) class Meta: - unique_together = (('student', 'module_state_key'),) + unique_together = (('course_id', 'student', 'module_state_key'),) ## Internal state of the object state = models.TextField(null=True, blank=True) @@ -57,7 +61,8 @@ class StudentModule(models.Model): modified = models.DateTimeField(auto_now=True, db_index=True) def __unicode__(self): - return '/'.join([self.module_type, self.student.username, self.module_state_key, str(self.state)[:20]]) + return '/'.join([self.course_id, self.module_type, + self.student.username, self.module_state_key, str(self.state)[:20]]) # TODO (cpennington): Remove these once the LMS switches to using XModuleDescriptors @@ -67,20 +72,20 @@ class StudentModuleCache(object): """ A cache of StudentModules for a specific student """ - def __init__(self, user, descriptors, select_for_update=False): + def __init__(self, course_id, user, descriptors, select_for_update=False): ''' Find any StudentModule objects that are needed by any descriptor in descriptors. Avoids making multiple queries to the database. Note: Only modules that have store_state = True or have shared state will have a StudentModule. - + Arguments user: The user for which to fetch maching StudentModules descriptors: An array of XModuleDescriptors. select_for_update: Flag indicating whether the rows should be locked until end of transaction ''' if user.is_authenticated(): - module_ids = self._get_module_state_keys(descriptors) + module_ids = self._get_module_state_keys(descriptors) # This works around a limitation in sqlite3 on the number of parameters # that can be put into a single query @@ -89,78 +94,86 @@ class StudentModuleCache(object): for id_chunk in [module_ids[i:i + chunk_size] for i in xrange(0, len(module_ids), chunk_size)]: if select_for_update: self.cache.extend(StudentModule.objects.select_for_update().filter( + course_id=course_id, student=user, module_state_key__in=id_chunk) ) else: self.cache.extend(StudentModule.objects.filter( + course_id=course_id, student=user, module_state_key__in=id_chunk) ) else: self.cache = [] - - + + @classmethod - def cache_for_descriptor_descendents(cls, user, descriptor, depth=None, descriptor_filter=lambda descriptor: True, select_for_update=False): + def cache_for_descriptor_descendents(cls, course_id, user, descriptor, depth=None, + descriptor_filter=lambda descriptor: True, + select_for_update=False): """ + course_id: the course in the context of which we want StudentModules. + user: the django user for whom to load modules. descriptor: An XModuleDescriptor depth is the number of levels of descendent modules to load StudentModules for, in addition to the supplied descriptor. If depth is None, load all descendent StudentModules - descriptor_filter is a function that accepts a descriptor and return wether the StudentModule + descriptor_filter is a function that accepts a descriptor and return wether the StudentModule should be cached select_for_update: Flag indicating whether the rows should be locked until end of transaction """ - + def get_child_descriptors(descriptor, depth, descriptor_filter): if descriptor_filter(descriptor): descriptors = [descriptor] else: descriptors = [] - + if depth is None or depth > 0: new_depth = depth - 1 if depth is not None else depth for child in descriptor.get_children(): descriptors.extend(get_child_descriptors(child, new_depth, descriptor_filter)) - + return descriptors - - + + descriptors = get_child_descriptors(descriptor, depth, descriptor_filter) - - return StudentModuleCache(user, descriptors, select_for_update) - + + return StudentModuleCache(course_id, user, descriptors, select_for_update) + def _get_module_state_keys(self, descriptors): ''' Get a list of the state_keys needed for StudentModules required for this module descriptor - - descriptor_filter is a function that accepts a descriptor and return wether the StudentModule + + descriptor_filter is a function that accepts a descriptor and return wether the StudentModule should be cached ''' keys = [] for descriptor in descriptors: if descriptor.stores_state: keys.append(descriptor.location.url()) - + shared_state_key = getattr(descriptor, 'shared_state_key', None) if shared_state_key is not None: keys.append(shared_state_key) return keys - def lookup(self, module_type, module_state_key): + def lookup(self, course_id, module_type, module_state_key): ''' - Look for a student module with the given type and id in the cache. + Look for a student module with the given course_id, type, and id in the cache. cache -- list of student modules returns first found object, or None ''' for o in self.cache: - if o.module_type == module_type and o.module_state_key == module_state_key: + if (o.course_id == course_id and + o.module_type == module_type and + o.module_state_key == module_state_key): return o return None diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index fbd43b8247..a61e8b9e23 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -70,7 +70,8 @@ def toc_for_course(user, request, course, active_chapter, active_section, course None if this is not the case. ''' - student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(user, course, depth=2) + student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( + course_id, user, course, depth=2) course = get_module(user, request, course.location, student_module_cache, course_id) chapters = list() @@ -159,12 +160,13 @@ def get_module(user, request, location, student_module_cache, course_id, positio shared_module = None if user.is_authenticated(): if descriptor.stores_state: - instance_module = student_module_cache.lookup(descriptor.category, - descriptor.location.url()) + instance_module = student_module_cache.lookup( + course_id, descriptor.category, descriptor.location.url()) shared_state_key = getattr(descriptor, 'shared_state_key', None) if shared_state_key is not None: - shared_module = student_module_cache.lookup(descriptor.category, + shared_module = student_module_cache.lookup(course_id, + descriptor.category, shared_state_key) @@ -241,7 +243,7 @@ def get_module(user, request, location, student_module_cache, course_id, positio return module -def get_instance_module(user, module, student_module_cache): +def get_instance_module(course_id, user, module, student_module_cache): """ Returns instance_module is a StudentModule specific to this module for this student, or None if this is an anonymous user @@ -252,11 +254,12 @@ def get_instance_module(user, module, student_module_cache): + str(module.id) + " which does not store state.") return None - instance_module = student_module_cache.lookup(module.category, - module.location.url()) + instance_module = student_module_cache.lookup( + course_id, module.category, module.location.url()) if not instance_module: instance_module = StudentModule( + course_id=course_id, student=user, module_type=module.category, module_state_key=module.id, @@ -285,6 +288,7 @@ def get_shared_instance_module(course_id, user, module, student_module_cache): shared_state_key) if not shared_module: shared_module = StudentModule( + course_id=course_id, student=user, module_type=descriptor.category, module_state_key=shared_state_key, @@ -317,14 +321,14 @@ def xqueue_callback(request, course_id, userid, id, dispatch): # Retrieve target StudentModule user = User.objects.get(id=userid) - student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( + student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(course_id, user, modulestore().get_instance(course_id, id), depth=0, select_for_update=True) instance = get_module(user, request, id, student_module_cache, course_id) if instance is None: log.debug("No module {0} for user {1}--access denied?".format(id, user)) raise Http404 - instance_module = get_instance_module(user, instance, student_module_cache) + instance_module = get_instance_module(course_id, user, instance, student_module_cache) if instance_module is None: log.debug("Couldn't find instance of module '%s' for user '%s'", id, user) @@ -387,7 +391,7 @@ def modx_dispatch(request, dispatch, location, course_id): return HttpResponse(json.dumps({'success': file_too_big_msg})) p[fileinput_id] = inputfiles - student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( + student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(course_id, request.user, modulestore().get_instance(course_id, location)) instance = get_module(request.user, request, location, student_module_cache, course_id) @@ -397,7 +401,7 @@ def modx_dispatch(request, dispatch, location, course_id): log.debug("No module {0} for user {1}--access denied?".format(location, user)) raise Http404 - instance_module = get_instance_module(request.user, instance, student_module_cache) + instance_module = get_instance_module(course_id, request.user, instance, student_module_cache) shared_module = get_shared_instance_module(course_id, request.user, instance, student_module_cache) # Don't track state for anonymous users (who don't have student modules) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 583628d1f2..6e8fe6fd23 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -149,8 +149,7 @@ def index(request, course_id, chapter=None, section=None, section_descriptor = get_section(course, chapter, section) if section_descriptor is not None: student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( - request.user, - section_descriptor) + course_id, request.user, section_descriptor) module = get_module(request.user, request, section_descriptor.location, student_module_cache, course_id) @@ -310,7 +309,8 @@ def progress(request, course_id, student_id=None): raise Http404 student = User.objects.get(id=int(student_id)) - student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(request.user, course) + student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( + course_id, request.user, course) course_module = get_module(request.user, request, course.location, student_module_cache, course_id)