From dd5ac4cf34908c2c245b51668bb4455808ead2d6 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 26 Nov 2013 15:02:25 -0500 Subject: [PATCH 1/7] Change video transcripts to use locators instead of locations. Part of STUD-870 --- .../contentstore/tests/test_contentstore.py | 9 +- .../contentstore/tests/test_transcripts.py | 80 +++++++-------- .../contentstore/views/component.py | 10 +- .../contentstore/views/transcripts_ajax.py | 97 ++++++++++--------- .../js/spec/transcripts/file_uploader_spec.js | 2 +- .../spec/transcripts/message_manager_spec.js | 10 +- .../js/spec/transcripts/videolist_spec.js | 8 +- cms/static/js/views/transcripts/editor.js | 18 ++-- .../js/views/transcripts/file_uploader.js | 2 +- .../js/views/transcripts/message_manager.js | 10 +- .../views/transcripts/metadata_videolist.js | 6 +- cms/static/js/views/transcripts/utils.js | 6 +- .../js/transcripts/file-upload.underscore | 2 +- .../messages/transcripts-found.underscore | 2 +- .../messages/transcripts-uploaded.underscore | 2 +- cms/templates/unit.html | 4 +- 16 files changed, 130 insertions(+), 138 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 0aaf2dfb29..91dce0f6ee 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -137,8 +137,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): locator = loc_mapper().translate_location(course.location.course_id, descriptor.location, False, True) resp = self.client.get_html(locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) - # TODO: uncomment when video transcripts no longer require IDs. - # _test_no_locations(self, resp) + _test_no_locations(self, resp) for expected in expected_types: self.assertIn(expected, resp.content) @@ -1354,8 +1353,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): unit_locator = loc_mapper().translate_location(course_id, descriptor.location, False, True) resp = self.client.get_html(unit_locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) - # TODO: uncomment when video transcripts no longer require IDs. - # _test_no_locations(self, resp) + _test_no_locations(self, resp) @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE) @@ -1682,8 +1680,7 @@ class ContentStoreTest(ModuleStoreTestCase): unit_locator = loc_mapper().translate_location(loc.course_id, unit_location, False, True) resp = self.client.get_html(unit_locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) - # TODO: uncomment when video transcripts no longer require IDs. - # _test_no_locations(self, resp) + _test_no_locations(self, resp) def delete_item(category, name): """ Helper method for testing the deletion of an xblock item. """ diff --git a/cms/djangoapps/contentstore/tests/test_transcripts.py b/cms/djangoapps/contentstore/tests/test_transcripts.py index 4c481383ab..f4c7f773ad 100644 --- a/cms/djangoapps/contentstore/tests/test_transcripts.py +++ b/cms/djangoapps/contentstore/tests/test_transcripts.py @@ -60,7 +60,7 @@ class Basetranscripts(CourseTestCase): 'type': 'video' } resp = self.client.ajax_post('/xblock', data) - self.item_location = self._get_location(resp) + self.item_locator, self.item_location = self._get_locator(resp) self.assertEqual(resp.status_code, 200) # hI10vDNYz4M - valid Youtube ID with transcripts. @@ -73,10 +73,10 @@ class Basetranscripts(CourseTestCase): # Remove all transcripts for current module. self.clear_subs_content() - def _get_location(self, resp): - """ Returns the location (as a string) from the response returned by a create operation. """ + def _get_locator(self, resp): + """ Returns the locator and old-style location (as a string) from the response returned by a create operation. """ locator = json.loads(resp.content).get('locator') - return loc_mapper().translate_locator_to_location(BlockUsageLocator(locator)).url() + return locator, loc_mapper().translate_locator_to_location(BlockUsageLocator(locator)).url() def get_youtube_ids(self): """Return youtube speeds and ids.""" @@ -142,7 +142,7 @@ class TestUploadtranscripts(Basetranscripts): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -164,20 +164,20 @@ class TestUploadtranscripts(Basetranscripts): link = reverse('upload_transcripts') resp = self.client.post(link, {'file': self.good_srt_file}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), 'POST data without "id" form data.') + self.assertEqual(json.loads(resp.content).get('status'), 'POST data without "locator" form data.') def test_fail_data_without_file(self): link = reverse('upload_transcripts') - resp = self.client.post(link, {'id': self.item_location}) + resp = self.client.post(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 400) self.assertEqual(json.loads(resp.content).get('status'), 'POST data without "file" form data.') - def test_fail_data_with_bad_location(self): + def test_fail_data_with_bad_locator(self): # Test for raising `InvalidLocationError` exception. link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': 'BAD_LOCATION', + 'locator': 'BAD_LOCATOR', 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -186,13 +186,13 @@ class TestUploadtranscripts(Basetranscripts): }]) }) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") # Test for raising `ItemNotFoundError` exception. link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': '{0}_{1}'.format(self.item_location, 'BAD_LOCATION'), + 'locator': '{0}_{1}'.format(self.item_locator, 'BAD_LOCATOR'), 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -201,7 +201,7 @@ class TestUploadtranscripts(Basetranscripts): }]) }) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") def test_fail_for_non_video_module(self): # non_video module: setup @@ -211,7 +211,7 @@ class TestUploadtranscripts(Basetranscripts): 'type': 'non_video' } resp = self.client.ajax_post('/xblock', data) - item_location = self._get_location(resp) + item_locator, item_location = self._get_locator(resp) data = '' modulestore().update_item(item_location, data) @@ -220,7 +220,7 @@ class TestUploadtranscripts(Basetranscripts): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': item_location, + 'locator': item_locator, 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -238,7 +238,7 @@ class TestUploadtranscripts(Basetranscripts): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -255,7 +255,7 @@ class TestUploadtranscripts(Basetranscripts): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.bad_data_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.bad_data_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -270,7 +270,7 @@ class TestUploadtranscripts(Basetranscripts): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.bad_name_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.bad_name_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -297,7 +297,7 @@ class TestUploadtranscripts(Basetranscripts): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -359,7 +359,7 @@ class TestDownloadtranscripts(Basetranscripts): self.save_subs_to_store(subs, 'JMD_ifUUfsU') link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location, 'subs_id': "JMD_ifUUfsU"}) + resp = self.client.get(link, {'locator': self.item_locator, 'subs_id': "JMD_ifUUfsU"}) self.assertEqual(resp.status_code, 200) self.assertEqual(resp.content, """0\n00:00:00,100 --> 00:00:00,200\nsubs #1\n\n1\n00:00:00,200 --> 00:00:00,240\nsubs #2\n\n2\n00:00:00,240 --> 00:00:00,380\nsubs #3\n\n""") @@ -386,7 +386,7 @@ class TestDownloadtranscripts(Basetranscripts): self.save_subs_to_store(subs, subs_id) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location, 'subs_id': subs_id}) + resp = self.client.get(link, {'locator': self.item_locator, 'subs_id': subs_id}) self.assertEqual(resp.status_code, 200) self.assertEqual( resp.content, @@ -397,21 +397,21 @@ class TestDownloadtranscripts(Basetranscripts): def test_fail_data_without_file(self): link = reverse('download_transcripts') - resp = self.client.get(link, {'id': ''}) + resp = self.client.get(link, {'locator': ''}) self.assertEqual(resp.status_code, 404) resp = self.client.get(link, {}) self.assertEqual(resp.status_code, 404) - def test_fail_data_with_bad_location(self): + def test_fail_data_with_bad_locator(self): # Test for raising `InvalidLocationError` exception. link = reverse('download_transcripts') - resp = self.client.get(link, {'id': 'BAD_LOCATION'}) + resp = self.client.get(link, {'locator': 'BAD_LOCATOR'}) self.assertEqual(resp.status_code, 404) # Test for raising `ItemNotFoundError` exception. link = reverse('download_transcripts') - resp = self.client.get(link, {'id': '{0}_{1}'.format(self.item_location, 'BAD_LOCATION')}) + resp = self.client.get(link, {'locator': '{0}_{1}'.format(self.item_locator, 'BAD_LOCATOR')}) self.assertEqual(resp.status_code, 404) def test_fail_for_non_video_module(self): @@ -422,7 +422,7 @@ class TestDownloadtranscripts(Basetranscripts): 'type': 'videoalpha' } resp = self.client.ajax_post('/xblock', data) - item_location = self._get_location(resp) + item_locator, item_location = self._get_locator(resp) subs_id = str(uuid4()) data = textwrap.dedent(""" @@ -445,7 +445,7 @@ class TestDownloadtranscripts(Basetranscripts): self.save_subs_to_store(subs, subs_id) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': item_location}) + resp = self.client.get(link, {'locator': item_locator}) self.assertEqual(resp.status_code, 404) def test_fail_nonyoutube_subs_dont_exist(self): @@ -459,7 +459,7 @@ class TestDownloadtranscripts(Basetranscripts): modulestore().update_item(self.item_location, data) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location}) + resp = self.client.get(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 404) def test_empty_youtube_attr_and_sub_attr(self): @@ -473,7 +473,7 @@ class TestDownloadtranscripts(Basetranscripts): modulestore().update_item(self.item_location, data) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location}) + resp = self.client.get(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 404) @@ -498,7 +498,7 @@ class TestDownloadtranscripts(Basetranscripts): self.save_subs_to_store(subs, 'JMD_ifUUfsU') link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location}) + resp = self.client.get(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 404) @@ -553,7 +553,7 @@ class TestChecktranscripts(Basetranscripts): self.save_subs_to_store(subs, subs_id) data = { - 'id': self.item_location, + 'locator': self.item_locator, 'videos': [{ 'type': 'html5', 'video': subs_id, @@ -597,7 +597,7 @@ class TestChecktranscripts(Basetranscripts): self.save_subs_to_store(subs, 'JMD_ifUUfsU') link = reverse('check_transcripts') data = { - 'id': self.item_location, + 'locator': self.item_locator, 'videos': [{ 'type': 'youtube', 'video': 'JMD_ifUUfsU', @@ -625,7 +625,7 @@ class TestChecktranscripts(Basetranscripts): def test_fail_data_without_id(self): link = reverse('check_transcripts') data = { - 'id': '', + 'locator': '', 'videos': [{ 'type': '', 'video': '', @@ -634,13 +634,13 @@ class TestChecktranscripts(Basetranscripts): } resp = self.client.get(link, {'data': json.dumps(data)}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") - def test_fail_data_with_bad_location(self): + def test_fail_data_with_bad_locator(self): # Test for raising `InvalidLocationError` exception. link = reverse('check_transcripts') data = { - 'id': '', + 'locator': '', 'videos': [{ 'type': '', 'video': '', @@ -649,11 +649,11 @@ class TestChecktranscripts(Basetranscripts): } resp = self.client.get(link, {'data': json.dumps(data)}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") # Test for raising `ItemNotFoundError` exception. data = { - 'id': '{0}_{1}'.format(self.item_location, 'BAD_LOCATION'), + 'locator': '{0}_{1}'.format(self.item_locator, 'BAD_LOCATOR'), 'videos': [{ 'type': '', 'video': '', @@ -662,7 +662,7 @@ class TestChecktranscripts(Basetranscripts): } resp = self.client.get(link, {'data': json.dumps(data)}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") def test_fail_for_non_video_module(self): # Not video module: setup @@ -672,7 +672,7 @@ class TestChecktranscripts(Basetranscripts): 'type': 'not_video' } resp = self.client.ajax_post('/xblock', data) - item_location = self._get_location(resp) + item_locator, item_location = self._get_locator(resp) subs_id = str(uuid4()) data = textwrap.dedent(""" @@ -695,7 +695,7 @@ class TestChecktranscripts(Basetranscripts): self.save_subs_to_store(subs, subs_id) data = { - 'id': item_location, + 'locator': item_locator, 'videos': [{ 'type': '', 'video': '', diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 3742c7af20..ce20e004ee 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -223,13 +223,9 @@ def unit_handler(request, tag=None, course_id=None, branch=None, version_guid=No ) components = [ - [ - # TODO: old location needed for video transcripts. - component.location.url(), - loc_mapper().translate_location( - course.location.course_id, component.location, False, True - ) - ] + loc_mapper().translate_location( + course.location.course_id, component.location, False, True + ) for component in item.get_children() ] diff --git a/cms/djangoapps/contentstore/views/transcripts_ajax.py b/cms/djangoapps/contentstore/views/transcripts_ajax.py index 9db2cb9c13..9d5cdf8e80 100644 --- a/cms/djangoapps/contentstore/views/transcripts_ajax.py +++ b/cms/djangoapps/contentstore/views/transcripts_ajax.py @@ -18,11 +18,12 @@ from django.conf import settings from xmodule.contentstore.content import StaticContent from xmodule.exceptions import NotFoundError -from xmodule.modulestore.django import modulestore +from xmodule.modulestore.django import modulestore, loc_mapper from xmodule.contentstore.django import contentstore -from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError +from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError from util.json_request import JsonResponse +from xmodule.modulestore.locator import BlockUsageLocator from ..transcripts_utils import ( generate_subs_from_source, @@ -77,20 +78,14 @@ def upload_transcripts(request): 'subs': '', } - item_location = request.POST.get('id') - if not item_location: - return error_response(response, 'POST data without "id" form data.') + locator = request.POST.get('locator') + if not locator: + return error_response(response, 'POST data without "locator" form data.') - # This is placed before has_access() to validate item_location, - # because has_access() raises InvalidLocationError if location is invalid. try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - return error_response(response, "Can't find item by location.") - - # Check permissions for this user within this course. - if not has_access(request.user, item_location): - raise PermissionDenied() + item = _get_item(request, request.POST) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + return error_response(response, "Can't find item by locator.") if 'file' not in request.FILES: return error_response(response, 'POST data without "file" form data.') @@ -156,23 +151,17 @@ def download_transcripts(request): Raises Http404 if unsuccessful. """ - item_location = request.GET.get('id') - if not item_location: - log.debug('GET data without "id" property.') + locator = request.GET.get('locator') + if not locator: + log.debug('GET data without "locator" property.') raise Http404 - # This is placed before has_access() to validate item_location, - # because has_access() raises InvalidLocationError if location is invalid. try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - log.debug("Can't find item by location.") + item = _get_item(request, request.GET) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + log.debug("Can't find item by locator.") raise Http404 - # Check permissions for this user within this course. - if not has_access(request.user, item_location): - raise PermissionDenied() - subs_id = request.GET.get('subs_id') if not subs_id: log.debug('GET data without "subs_id" property.') @@ -240,7 +229,7 @@ def check_transcripts(request): 'status': 'Error', } try: - __, videos, item = validate_transcripts_data(request) + __, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(transcripts_presence, e.message) @@ -303,7 +292,7 @@ def check_transcripts(request): if len(html5_subs) == 2: # check html5 transcripts for equality transcripts_presence['html5_equal'] = json.loads(html5_subs[0]) == json.loads(html5_subs[1]) - command, subs_to_use = transcripts_logic(transcripts_presence, videos) + command, subs_to_use = _transcripts_logic(transcripts_presence, videos) transcripts_presence.update({ 'command': command, 'subs': subs_to_use, @@ -311,7 +300,7 @@ def check_transcripts(request): return JsonResponse(transcripts_presence) -def transcripts_logic(transcripts_presence, videos): +def _transcripts_logic(transcripts_presence, videos): """ By `transcripts_presence` content, figure what show to user: @@ -386,7 +375,7 @@ def choose_transcripts(request): } try: - data, videos, item = validate_transcripts_data(request) + data, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(response, e.message) @@ -416,7 +405,7 @@ def replace_transcripts(request): response = {'status': 'Error', 'subs': ''} try: - __, videos, item = validate_transcripts_data(request) + __, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(response, e.message) @@ -435,7 +424,7 @@ def replace_transcripts(request): return JsonResponse(response) -def validate_transcripts_data(request): +def _validate_transcripts_data(request): """ Validates, that request contains all proper data for transcripts processing. @@ -452,18 +441,10 @@ def validate_transcripts_data(request): if not data: raise TranscriptsRequestValidationException('Incoming video data is empty.') - item_location = data.get('id') - - # This is placed before has_access() to validate item_location, - # because has_access() raises InvalidLocationError if location is invalid. try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - raise TranscriptsRequestValidationException("Can't find item by location.") - - # Check permissions for this user within this course. - if not has_access(request.user, item_location): - raise PermissionDenied() + item = _get_item(request, data) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + raise TranscriptsRequestValidationException("Can't find item by locator.") if item.category != 'video': raise TranscriptsRequestValidationException('Transcripts are supported only for "video" modules.') @@ -492,7 +473,7 @@ def rename_transcripts(request): response = {'status': 'Error', 'subs': ''} try: - __, videos, item = validate_transcripts_data(request) + __, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(response, e.message) @@ -525,11 +506,10 @@ def save_transcripts(request): if not data: return error_response(response, 'Incoming video data is empty.') - item_location = data.get('id') try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - return error_response(response, "Can't find item by location.") + item = _get_item(request, data) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + return error_response(response, "Can't find item by locator.") metadata = data.get('metadata') if metadata is not None: @@ -553,3 +533,24 @@ def save_transcripts(request): response['status'] = 'Success' return JsonResponse(response) + + +def _get_item(request, data): + """ + Obtains from 'data' the locator for an item. + Next, gets that item from the modulestore (allowing any errors to raise up). + Finally, verifies that the user has access to the item. + + Returns the item. + """ + locator = BlockUsageLocator(data.get('locator')) + old_location = loc_mapper().translate_locator_to_location(locator) + + # This is placed before has_access() to validate the location, + # because has_access() raises InvalidLocationError if location is invalid. + item = modulestore().get_item(old_location) + + if not has_access(request.user, locator): + raise PermissionDenied() + + return item diff --git a/cms/static/js/spec/transcripts/file_uploader_spec.js b/cms/static/js/spec/transcripts/file_uploader_spec.js index c896ad811a..20b653527c 100644 --- a/cms/static/js/spec/transcripts/file_uploader_spec.js +++ b/cms/static/js/spec/transcripts/file_uploader_spec.js @@ -48,7 +48,7 @@ function ($, _, Utils, FileUploader) { el: $container, messenger: messenger, videoListObject: videoListObject, - component_id: 'component_id' + component_locator: 'component_locator' }); }); diff --git a/cms/static/js/spec/transcripts/message_manager_spec.js b/cms/static/js/spec/transcripts/message_manager_spec.js index c6d3bc7909..40eef7f02e 100644 --- a/cms/static/js/spec/transcripts/message_manager_spec.js +++ b/cms/static/js/spec/transcripts/message_manager_spec.js @@ -52,7 +52,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { view = new MessageManager({ el: $container, parent: videoList, - component_id: 'component_id' + component_locator: 'component_locator' }); }); @@ -60,7 +60,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { expect(fileUploader.initialize).toHaveBeenCalledWith({ el: view.$el, messenger: view, - component_id: view.component_id, + component_locator: view.component_locator, videoListObject: view.options.parent }); }); @@ -215,7 +215,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { function() { expect(Utils.command).toHaveBeenCalledWith( action, - view.component_id, + view.component_locator, videoList, void(0) ); @@ -245,7 +245,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { function () { expect(Utils.command).toHaveBeenCalledWith( action, - view.component_id, + view.component_locator, videoList, { html5_id: extraParamas @@ -268,7 +268,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { function () { expect(Utils.command).toHaveBeenCalledWith( action, - view.component_id, + view.component_locator, videoList, void(0) ); diff --git a/cms/static/js/spec/transcripts/videolist_spec.js b/cms/static/js/spec/transcripts/videolist_spec.js index cf189c2549..de850eb3ee 100644 --- a/cms/static/js/spec/transcripts/videolist_spec.js +++ b/cms/static/js/spec/transcripts/videolist_spec.js @@ -11,7 +11,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s 'transcripts/metadata-videolist-entry.underscore' ), abstractEditor = AbstractEditor.prototype, - component_id = 'component_id', + component_locator = 'component_locator', videoList = [ { mode: "youtube", @@ -62,7 +62,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s var tpl = sandbox({ 'class': 'component', - 'data-id': component_id + 'data-locator': component_locator }), model = new MetadataModel(modelStub), videoList, $el; @@ -157,7 +157,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s waitsForResponse(function () { expect(abstractEditor.initialize).toHaveBeenCalled(); expect(messenger.initialize).toHaveBeenCalled(); - expect(view.component_id).toBe(component_id); + expect(view.component_locator).toBe(component_locator); expect(view.$el).toHandle('input'); }); }); @@ -167,7 +167,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s expect(abstractEditor.render).toHaveBeenCalled(); expect(Utils.command).toHaveBeenCalledWith( 'check', - component_id, + component_locator, videoList ); diff --git a/cms/static/js/views/transcripts/editor.js b/cms/static/js/views/transcripts/editor.js index 21e48a03b0..78280a0d66 100644 --- a/cms/static/js/views/transcripts/editor.js +++ b/cms/static/js/views/transcripts/editor.js @@ -72,7 +72,7 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) { syncBasicTab: function (metadataCollection, metadataView) { var result = [], getField = Utils.getField, - component_id = this.$el.closest('.component').data('id'), + component_locator = this.$el.closest('.component').data('locator'), subs = getField(metadataCollection, 'sub'), values = {}, videoUrl, metadata, modifiedValues; @@ -99,7 +99,7 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) { if (isSubsModified) { metadata = $.extend(true, {}, modifiedValues); // Save module state - Utils.command('save', component_id, null, { + Utils.command('save', component_locator, null, { metadata: metadata, current_subs: _.pluck( Utils.getVideoList(videoUrl.getDisplayValue()), @@ -110,18 +110,16 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) { // Get values from `Advanced` tab fields (`html5_sources`, // `youtube_id_1_0`) that should be synchronized. - html5Sources = getField(metadataCollection, 'html5_sources') -                                    .getDisplayValue(); + var html5Sources = getField(metadataCollection, 'html5_sources').getDisplayValue(); -            values.youtube = getField(metadataCollection, 'youtube_id_1_0') -                                    .getDisplayValue(); + values.youtube = getField(metadataCollection, 'youtube_id_1_0').getDisplayValue(); -            values.html5Sources = _.filter(html5Sources, function (value) { -                var link = Utils.parseLink(value), + values.html5Sources = _.filter(html5Sources, function (value) { + var link = Utils.parseLink(value), mode = link && link.mode; -                return mode === 'html5' && mode; -            }); + return mode === 'html5' && mode; + }); // The length of youtube video_id should be 11 characters. diff --git a/cms/static/js/views/transcripts/file_uploader.js b/cms/static/js/views/transcripts/file_uploader.js index c9c33820e4..d307c7c85e 100644 --- a/cms/static/js/views/transcripts/file_uploader.js +++ b/cms/static/js/views/transcripts/file_uploader.js @@ -39,7 +39,7 @@ function($, Backbone, _, Utils) { tplContainer.html(this.template({ ext: this.validFileExtensions, - component_id: this.options.component_id, + component_locator: this.options.component_locator, video_list: videoList })); diff --git a/cms/static/js/views/transcripts/message_manager.js b/cms/static/js/views/transcripts/message_manager.js index 80f70752cf..506f3143ac 100644 --- a/cms/static/js/views/transcripts/message_manager.js +++ b/cms/static/js/views/transcripts/message_manager.js @@ -31,12 +31,12 @@ function($, Backbone, _, Utils, FileUploader, gettext) { initialize: function () { _.bindAll(this); - this.component_id = this.$el.closest('.component').data('id'); + this.component_locator = this.$el.closest('.component').data('locator'); this.fileUploader = new FileUploader({ el: this.$el, messenger: this, - component_id: this.component_id, + component_locator: this.component_locator, videoListObject: this.options.parent }); }, @@ -76,7 +76,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) { this.$el.find('.transcripts-status') .removeClass('is-invisible') .find(this.elClass).html(template({ - component_id: encodeURIComponent(this.component_id), + component_locator: encodeURIComponent(this.component_locator), html5_list: html5List, grouped_list: groupedList, subs_id: (params) ? params.subs: '' @@ -204,7 +204,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) { */ processCommand: function (action, errorMessage, videoId) { var self = this, - component_id = this.component_id, + component_locator = this.component_locator, videoList = this.options.parent.getVideoObjectsList(), extraParam, xhr; @@ -212,7 +212,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) { extraParam = { html5_id: videoId }; } - xhr = Utils.command(action, component_id, videoList, extraParam) + xhr = Utils.command(action, component_locator, videoList, extraParam) .done(function (resp) { var sub = resp.subs; diff --git a/cms/static/js/views/transcripts/metadata_videolist.js b/cms/static/js/views/transcripts/metadata_videolist.js index 6e7e82232b..7e7dac7f19 100644 --- a/cms/static/js/views/transcripts/metadata_videolist.js +++ b/cms/static/js/views/transcripts/metadata_videolist.js @@ -46,7 +46,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) { _.debounce(_.bind(this.inputHandler, this), this.inputDelay) ); - this.component_id = this.$el.closest('.component').data('id'); + this.component_locator = this.$el.closest('.component').data('locator'); }, render: function () { @@ -55,7 +55,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) { .apply(this, arguments); var self = this, - component_id = this.$el.closest('.component').data('id'), + component_locator = this.$el.closest('.component').data('locator'), videoList = this.getVideoObjectsList(), showServerError = function (response) { @@ -82,7 +82,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) { } // Check current state of Timed Transcripts. - Utils.command('check', component_id, videoList) + Utils.command('check', component_locator, videoList) .done(function (resp) { var params = resp, len = videoList.length, diff --git a/cms/static/js/views/transcripts/utils.js b/cms/static/js/views/transcripts/utils.js index 7965d0b9e3..a6865906e5 100644 --- a/cms/static/js/views/transcripts/utils.js +++ b/cms/static/js/views/transcripts/utils.js @@ -295,7 +295,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) { * * @param {string} action Action that will be invoked on server. Is a part * of url. - * @param {string} component_id Id of component. + * @param {string} component_locator the locator of component. * @param {array} videoList List of object with information about inserted * urls. * @param {object} extraParams Extra parameters that can be send to the @@ -314,7 +314,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) { // _command() function. var xhr = null; - return function (action, component_id, videoList, extraParams) { + return function (action, component_locator, videoList, extraParams) { var params, data; if (extraParams) { @@ -326,7 +326,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) { } data = $.extend( - { id: component_id }, + { locator: component_locator }, { videos: videoList }, params ); diff --git a/cms/templates/js/transcripts/file-upload.underscore b/cms/templates/js/transcripts/file-upload.underscore index 90dfb80db6..d48f9af887 100644 --- a/cms/templates/js/transcripts/file-upload.underscore +++ b/cms/templates/js/transcripts/file-upload.underscore @@ -5,6 +5,6 @@ method="post" enctype="multipart/form-data"> - + diff --git a/cms/templates/js/transcripts/messages/transcripts-found.underscore b/cms/templates/js/transcripts/messages/transcripts-found.underscore index 6a12edcd3f..286063ed6a 100644 --- a/cms/templates/js/transcripts/messages/transcripts-found.underscore +++ b/cms/templates/js/transcripts/messages/transcripts-found.underscore @@ -10,7 +10,7 @@ - "> + "> <%= gettext("Download to Edit") %> diff --git a/cms/templates/js/transcripts/messages/transcripts-uploaded.underscore b/cms/templates/js/transcripts/messages/transcripts-uploaded.underscore index fa328208fd..c17d83f710 100644 --- a/cms/templates/js/transcripts/messages/transcripts-uploaded.underscore +++ b/cms/templates/js/transcripts/messages/transcripts-uploaded.underscore @@ -10,7 +10,7 @@ - "> + "> <%= gettext("Download to Edit") %> diff --git a/cms/templates/unit.html b/cms/templates/unit.html index 9f3daf0b20..2318e9cd97 100644 --- a/cms/templates/unit.html +++ b/cms/templates/unit.html @@ -48,8 +48,8 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"

    - % for id, locator in components: -
  1. + % for locator in components: +
  2. % endfor
  3. From 050031f7d5bc04389a5018f75c92b7b25dbfec83 Mon Sep 17 00:00:00 2001 From: Zubair Afzal Date: Wed, 4 Dec 2013 16:43:17 +0500 Subject: [PATCH 2/7] Disable Peer Track Changes STUD-1008 --- common/lib/xmodule/xmodule/combined_open_ended_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index 68a0d65617..2eb8585d85 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -508,5 +508,5 @@ class CombinedOpenEndedDescriptor(CombinedOpenEndedFields, RawDescriptor): def non_editable_metadata_fields(self): non_editable_fields = super(CombinedOpenEndedDescriptor, self).non_editable_metadata_fields non_editable_fields.extend([CombinedOpenEndedDescriptor.due, CombinedOpenEndedDescriptor.graceperiod, - CombinedOpenEndedDescriptor.markdown, CombinedOpenEndedDescriptor.version]) + CombinedOpenEndedDescriptor.markdown, CombinedOpenEndedDescriptor.version, CombinedOpenEndedDescriptor.track_changes]) return non_editable_fields From 76c8a7a38dd87b771583d902730a931ac819815b Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Thu, 5 Dec 2013 14:50:05 -0500 Subject: [PATCH 3/7] add all clear message --- lms/templates/instructor/staff_grading.html | 11 +++++++++++ .../open_ended_problems/combined_notifications.html | 11 +++++++++++ .../open_ended_flagged_problems.html | 11 +++++++++++ .../open_ended_problems/open_ended_problems.html | 11 +++++++++++ lms/templates/peer_grading/peer_grading.html | 11 +++++++++++ 5 files changed, 55 insertions(+) diff --git a/lms/templates/instructor/staff_grading.html b/lms/templates/instructor/staff_grading.html index 59585308c1..ae6485ec36 100644 --- a/lms/templates/instructor/staff_grading.html +++ b/lms/templates/instructor/staff_grading.html @@ -22,6 +22,17 @@

    ${_("Staff grading")}

    +
    +
    + + The issue we had between Dec 3 and Dec 5 with the visibility + of student responses has now been resolved. All responses + should now be visible. Thank you for your patience! + --the edX team + +
    +
    +
    diff --git a/lms/templates/open_ended_problems/combined_notifications.html b/lms/templates/open_ended_problems/combined_notifications.html index 47bb01fc8e..1f20256479 100644 --- a/lms/templates/open_ended_problems/combined_notifications.html +++ b/lms/templates/open_ended_problems/combined_notifications.html @@ -16,6 +16,17 @@
    ${error_text}
    +
    +
    + + The issue we had between Dec 3 and Dec 5 with the visibility + of student responses has now been resolved. All responses + should now be visible. Thank you for your patience! + --the edX team + +
    +
    +

    ${_("Open Ended Console")}

    ${_("Instructions")}

    diff --git a/lms/templates/open_ended_problems/open_ended_flagged_problems.html b/lms/templates/open_ended_problems/open_ended_flagged_problems.html index 87d5a11bd8..3bad135c28 100644 --- a/lms/templates/open_ended_problems/open_ended_flagged_problems.html +++ b/lms/templates/open_ended_problems/open_ended_flagged_problems.html @@ -19,6 +19,17 @@
    ${error_text}
    +
    +
    + + The issue we had between Dec 3 and Dec 5 with the visibility + of student responses has now been resolved. All responses + should now be visible. Thank you for your patience! + --the edX team + +
    +
    +

    ${_("Flagged Open Ended Problems")}

    ${_("Instructions")}

    diff --git a/lms/templates/open_ended_problems/open_ended_problems.html b/lms/templates/open_ended_problems/open_ended_problems.html index d5edc8edaf..f9447f49a9 100644 --- a/lms/templates/open_ended_problems/open_ended_problems.html +++ b/lms/templates/open_ended_problems/open_ended_problems.html @@ -16,6 +16,17 @@
    ${error_text}
    +
    +
    + + The issue we had between Dec 3 and Dec 5 with the visibility + of student responses has now been resolved. All responses + should now be visible. Thank you for your patience! + --the edX team + +
    +
    +

    ${_("Open Ended Problems")}

    ${_("Instructions")}

    ${_("Here is a list of open ended problems for this course.")}

    diff --git a/lms/templates/peer_grading/peer_grading.html b/lms/templates/peer_grading/peer_grading.html index 101e6341f5..1b212bcc0f 100644 --- a/lms/templates/peer_grading/peer_grading.html +++ b/lms/templates/peer_grading/peer_grading.html @@ -15,6 +15,17 @@ criteria.{end_li_tag}
    ${error_text}
    +
    +
    + + The issue we had between Dec 3 and Dec 5 with the visibility + of student responses has now been resolved. All responses + should now be visible. Thank you for your patience! + --the edX team + +
    +
    +

    ${_("Peer Grading")}

    ${_("Instructions")}

    From 7e4820af520bf4f83c24ad3c6d72a70c3d56a755 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 9 Dec 2013 16:34:07 -0500 Subject: [PATCH 4/7] Reduce sql queries for groupname tests. (STUD-1039) Conflicts: cms/djangoapps/auth/authz.py --- cms/djangoapps/auth/authz.py | 15 ++++++--------- lms/djangoapps/courseware/roles.py | 1 - 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 1a1f138cb5..5ec7beba2e 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -63,7 +63,7 @@ def get_all_course_role_groupnames(location, role, use_filter=True): # filter to the ones which exist default = groupnames[0] if use_filter: - groupnames = [group for group in groupnames if Group.objects.filter(name=group).exists()] + groupnames = [group.name for group in Group.objects.filter(name__in=groupnames)] return groupnames, default @@ -203,12 +203,9 @@ def remove_user_from_course_group(caller, user, location, role): # see if the user is actually in that role, if not then we don't have to do anything groupnames, _ = get_all_course_role_groupnames(location, role) - for groupname in groupnames: - groups = user.groups.filter(name=groupname) - if groups: - # will only be one with that name - user.groups.remove(groups[0]) - user.save() + for group in user.groups.filter(name__in=groupnames): + user.groups.remove(group) + user.save() def remove_user_from_creator_group(caller, user): @@ -243,7 +240,7 @@ def is_user_in_course_group_role(user, location, role, check_staff=True): if check_staff and user.is_staff: return True groupnames, _ = get_all_course_role_groupnames(location, role) - return any(user.groups.filter(name=groupname).exists() for groupname in groupnames) + return user.groups.filter(name__in=groupnames).exists() return False @@ -266,7 +263,7 @@ def is_user_in_creator_group(user): # Feature flag for using the creator group setting. Will be removed once the feature is complete. if settings.MITX_FEATURES.get('ENABLE_CREATOR_GROUP', False): - return user.groups.filter(name=COURSE_CREATOR_GROUP_NAME).count() > 0 + return user.groups.filter(name=COURSE_CREATOR_GROUP_NAME).exists() return True diff --git a/lms/djangoapps/courseware/roles.py b/lms/djangoapps/courseware/roles.py index 110bb9f362..60853e8090 100644 --- a/lms/djangoapps/courseware/roles.py +++ b/lms/djangoapps/courseware/roles.py @@ -4,7 +4,6 @@ adding users, removing users, and listing members """ from abc import ABCMeta, abstractmethod -from functools import partial from django.contrib.auth.models import User, Group From aba25c2de7dbb8ec60c276bacf9dcd8019da7c26 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Mon, 25 Nov 2013 15:35:51 +0000 Subject: [PATCH 5/7] In OpenEndedModule latest_score and all_score calculate sum of rubric scores every time for ML grading. ORA-72 --- .../combined_open_ended_modulev1.py | 19 +- .../open_ended_module.py | 40 +- .../xmodule/tests/test_combined_open_ended.py | 182 ++++- .../xmodule/tests/test_util_open_ended.py | 670 ++++++++++++++++++ 4 files changed, 903 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py index 72915eb7b3..3158ff291e 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py @@ -258,8 +258,23 @@ class CombinedOpenEndedV1Module(): if not task_states: return (0, 0, state_values[OpenEndedChild.INITIAL], idx) - final_child_state = json.loads(task_states[-1]) - scores = [attempt.get('score', 0) for attempt in final_child_state.get('child_history', [])] + final_task_xml = self.task_xml[-1] + final_child_state_json = task_states[-1] + final_child_state = json.loads(final_child_state_json) + + tag_name = self.get_tag_name(final_task_xml) + children = self.child_modules() + task_descriptor = children['descriptors'][tag_name](self.system) + task_parsed_xml = task_descriptor.definition_from_xml(etree.fromstring(final_task_xml), self.system) + task = children['modules'][tag_name]( + self.system, + self.location, + task_parsed_xml, + task_descriptor, + self.static_data, + instance_state=final_child_state_json, + ) + scores = task.all_scores() if scores: best_score = max(scores) else: diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py index d020987fdf..35c1c361cc 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py @@ -679,7 +679,7 @@ class OpenEndedModule(openendedchild.OpenEndedChild): return { 'success': success, 'error': error_message, - 'student_response': data['student_answer'].replace("\n","
    ") + 'student_response': data['student_answer'].replace("\n", "
    ") } def update_score(self, data, system): @@ -738,6 +738,44 @@ class OpenEndedModule(openendedchild.OpenEndedChild): html = system.render_template('{0}/open_ended.html'.format(self.TEMPLATE_DIR), context) return html + def latest_score(self): + """None if not available""" + if not self.child_history: + return None + return self.score_for_attempt(-1) + + def all_scores(self): + """None if not available""" + if not self.child_history: + return None + return [self.score_for_attempt(index) for index in xrange(0, len(self.child_history))] + + def score_for_attempt(self, index): + """ + Return sum of rubric scores for ML grading otherwise return attempt["score"]. + """ + attempt = self.child_history[index] + score = attempt.get('score') + post_assessment_data = self._parse_score_msg(attempt.get('post_assessment'), self.system) + grader_types = post_assessment_data.get('grader_types') + + # According to _parse_score_msg in ML grading there should be only one grader type. + if len(grader_types) == 1 and grader_types[0] == 'ML': + rubric_scores = post_assessment_data.get("rubric_scores") + + # Similarly there should be only one list of rubric scores. + if len(rubric_scores) == 1: + rubric_scores_sum = sum(rubric_scores[0]) + log.debug("""Score normalized for location={loc}, old_score={old_score}, + new_score={new_score}, rubric_score={rubric_score}""".format( + loc=self.location_string, + old_score=score, + new_score=rubric_scores_sum, + rubric_score=rubric_scores + )) + return rubric_scores_sum + return score + class OpenEndedDescriptor(): """ diff --git a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py index e1b2a4ebe2..6498c82c4a 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -27,7 +27,9 @@ from xmodule.progress import Progress from xmodule.tests.test_util_open_ended import ( DummyModulestore, TEST_STATE_SA_IN, MOCK_INSTANCE_STATE, TEST_STATE_SA, TEST_STATE_AI, TEST_STATE_AI2, TEST_STATE_AI2_INVALID, - TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE, MockUploadedFile + TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE, MockUploadedFile, INSTANCE_INCONSISTENT_STATE, + INSTANCE_INCONSISTENT_STATE2, INSTANCE_INCONSISTENT_STATE3, INSTANCE_INCONSISTENT_STATE4, + INSTANCE_INCONSISTENT_STATE5 ) from xblock.field_data import DictFieldData @@ -358,7 +360,7 @@ class OpenEndedModuleTest(unittest.TestCase): # Create a module with no state yet. Important that this start off as a blank slate. test_module = OpenEndedModule(self.test_system, self.location, - self.definition, self.descriptor, self.static_data, self.metadata) + self.definition, self.descriptor, self.static_data, self.metadata) saved_response = "Saved response." submitted_response = "Submitted response." @@ -369,7 +371,7 @@ class OpenEndedModuleTest(unittest.TestCase): self.assertEqual(test_module.get_display_answer(), "") # Now, store an answer in the module. - test_module.handle_ajax("store_answer", {'student_answer' : saved_response}, get_test_system()) + test_module.handle_ajax("store_answer", {'student_answer': saved_response}, get_test_system()) # The stored answer should now equal our response. self.assertEqual(test_module.stored_answer, saved_response) self.assertEqual(test_module.get_display_answer(), saved_response) @@ -387,6 +389,7 @@ class OpenEndedModuleTest(unittest.TestCase): # Confirm that the answer is stored properly. self.assertEqual(test_module.latest_answer(), submitted_response) + class CombinedOpenEndedModuleTest(unittest.TestCase): """ Unit tests for the combined open ended xmodule @@ -610,7 +613,6 @@ class CombinedOpenEndedModuleTest(unittest.TestCase): metadata=self.metadata, instance_state={'task_states': TEST_STATE_SA_IN}) - def test_get_score_realistic(self): """ Try to parse the correct score from a json instance state @@ -717,6 +719,175 @@ class CombinedOpenEndedModuleTest(unittest.TestCase): self.ai_state_success(TEST_STATE_PE_SINGLE, iscore=0, tasks=[self.task_xml2]) +class CombinedOpenEndedModuleConsistencyTest(unittest.TestCase): + """ + Unit tests for the combined open ended xmodule rubric scores consistency. + """ + + # location, definition_template, prompt, rubric, max_score, metadata, oeparam, task_xml1, task_xml2 + # All these variables are used to construct the xmodule descriptor. + location = Location(["i4x", "edX", "open_ended", "combinedopenended", + "SampleQuestion"]) + definition_template = """ + + {rubric} + {prompt} + + {task1} + + + {task2} + + + """ + prompt = "This is a question prompt" + rubric = ''' + + Response Quality + + + + ''' + max_score = 10 + + metadata = {'attempts': '10', 'max_score': max_score} + + oeparam = etree.XML(''' + + Enter essay here. + This is the answer. + {"grader_settings" : "ml_grading.conf", "problem_id" : "6.002x/Welcome/OETest"} + + ''') + + task_xml1 = ''' + + + What hint about this problem would you give to someone? + + + Save Succcesful. Thanks for participating! + + + ''' + task_xml2 = ''' + + + Enter essay here. + This is the answer. + {"grader_settings" : "ml_grading.conf", "problem_id" : "6.002x/Welcome/OETest"} + + ''' + + + static_data = { + 'max_attempts': 20, + 'prompt': prompt, + 'rubric': rubric, + 'max_score': max_score, + 'display_name': 'Name', + 'accept_file_upload': False, + 'close_date': "", + 's3_interface': test_util_open_ended.S3_INTERFACE, + 'open_ended_grading_interface': test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE, + 'skip_basic_checks': False, + 'graded': True, + } + + definition = {'prompt': etree.XML(prompt), 'rubric': etree.XML(rubric), 'task_xml': [task_xml1, task_xml2]} + full_definition = definition_template.format(prompt=prompt, rubric=rubric, task1=task_xml1, task2=task_xml2) + descriptor = Mock(data=full_definition) + test_system = get_test_system() + test_system.open_ended_grading_interface = None + combinedoe_container = CombinedOpenEndedModule( + descriptor=descriptor, + runtime=test_system, + field_data=DictFieldData({ + 'data': full_definition, + 'weight': '1', + }), + scope_ids=ScopeIds(None, None, None, None), + ) + + def setUp(self): + self.combinedoe = CombinedOpenEndedV1Module(self.test_system, + self.location, + self.definition, + self.descriptor, + static_data=self.static_data, + metadata=self.metadata, + instance_state=json.loads(INSTANCE_INCONSISTENT_STATE)) + + def test_get_score(self): + """ + If grader type is ML score should be updated from rubric scores. Aggregate rubric scores = sum([3])*5. + """ + score_dict = self.combinedoe.get_score() + self.assertEqual(score_dict['score'], 15.0) + self.assertEqual(score_dict['total'], 5.0) + + def test_get_score_with_pe_grader(self): + """ + If grader type is PE score should not be updated from rubric scores. Aggregate rubric scores = sum([3])*5. + """ + combinedoe = CombinedOpenEndedV1Module(self.test_system, + self.location, + self.definition, + self.descriptor, + static_data=self.static_data, + metadata=self.metadata, + instance_state=json.loads(INSTANCE_INCONSISTENT_STATE2)) + score_dict = combinedoe.get_score() + self.assertNotEqual(score_dict['score'], 15.0) + + def test_get_score_with_different_score_value_in_rubric(self): + """ + If grader type is ML score should be updated from rubric scores. Aggregate rubric scores = sum([5])*5. + """ + combinedoe = CombinedOpenEndedV1Module(self.test_system, + self.location, + self.definition, + self.descriptor, + static_data=self.static_data, + metadata=self.metadata, + instance_state=json.loads(INSTANCE_INCONSISTENT_STATE3)) + score_dict = combinedoe.get_score() + self.assertEqual(score_dict['score'], 25.0) + self.assertEqual(score_dict['total'], 5.0) + + def test_get_score_with_old_task_states(self): + """ + If grader type is ML and old_task_states are present in instance inconsistent state score should be updated + from rubric scores. Aggregate rubric scores = sum([3])*5. + """ + combinedoe = CombinedOpenEndedV1Module(self.test_system, + self.location, + self.definition, + self.descriptor, + static_data=self.static_data, + metadata=self.metadata, + instance_state=json.loads(INSTANCE_INCONSISTENT_STATE4)) + score_dict = combinedoe.get_score() + self.assertEqual(score_dict['score'], 15.0) + self.assertEqual(score_dict['total'], 5.0) + + def test_get_score_with_score_missing(self): + """ + If grader type is ML and score field is missing in instance inconsistent state score should be updated from + rubric scores. Aggregate rubric scores = sum([3])*5. + """ + combinedoe = CombinedOpenEndedV1Module(self.test_system, + self.location, + self.definition, + self.descriptor, + static_data=self.static_data, + metadata=self.metadata, + instance_state=json.loads(INSTANCE_INCONSISTENT_STATE5)) + score_dict = combinedoe.get_score() + self.assertEqual(score_dict['score'], 15.0) + self.assertEqual(score_dict['total'], 5.0) + + class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): """ Test the student flow in the combined open ended xmodule @@ -948,6 +1119,7 @@ class OpenEndedModuleXmlAttemptTest(unittest.TestCase, DummyModulestore): reset_data = json.loads(self._handle_ajax("reset", {})) self.assertEqual(reset_data['success'], False) + class OpenEndedModuleXmlImageUploadTest(unittest.TestCase, DummyModulestore): """ Test if student is able to upload images properly. @@ -1018,7 +1190,7 @@ class OpenEndedModuleXmlImageUploadTest(unittest.TestCase, DummyModulestore): # Simulate a student saving an answer with a link. response = module.handle_ajax("save_answer", { "student_answer": "{0} {1}".format(self.answer_text, self.answer_link) - }) + }) response = json.loads(response) diff --git a/common/lib/xmodule/xmodule/tests/test_util_open_ended.py b/common/lib/xmodule/xmodule/tests/test_util_open_ended.py index bbb0653512..f4a607aecf 100644 --- a/common/lib/xmodule/xmodule/tests/test_util_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_util_open_ended.py @@ -1,3 +1,5 @@ +import json +from textwrap import dedent from xmodule.modulestore import Location from xmodule.modulestore.xml import XMLModuleStore from xmodule.tests import DATA_DIR, get_test_system @@ -98,12 +100,680 @@ class DummyModulestore(object): descriptor.xmodule_runtime = self.get_module_system(descriptor) return descriptor +def serialize_child_history(task_state): + """ + To json serialize feedback and post_assessment in child_history of task state. + """ + child_history = task_state.get("child_history", []) + for i, attempt in enumerate(child_history): + if "post_assessment" in attempt: + if "feedback" in attempt["post_assessment"]: + attempt["post_assessment"]["feedback"] = json.dumps(attempt["post_assessment"].get("feedback")) + task_state["child_history"][i]["post_assessment"] = json.dumps(attempt["post_assessment"]) + +def serialize_open_ended_instance_state(json_str): + """ + To json serialize task_states and old_task_states in instance state. + """ + json_data = json.loads(json_str) + task_states = json_data.get('task_states', []) + for i, task_state in enumerate(task_states): + serialize_child_history(task_state) + json_data['task_states'][i] = json.dumps(task_state) + + old_task_states = json_data.get('old_task_states', []) + for i, old_task in enumerate(old_task_states): + for j, task_state in enumerate(old_task): + old_task[j] = json.dumps(task_state) + json_data['old_task_states'][i] = old_task + + return json.dumps(json_data) + + # Task state for a module with self assessment then instructor assessment. TEST_STATE_SA_IN = ["{\"child_created\": false, \"child_attempts\": 2, \"version\": 1, \"child_history\": [{\"answer\": \"However venture pursuit he am mr cordial. Forming musical am hearing studied be luckily. Ourselves for determine attending how led gentleman sincerity. Valley afford uneasy joy she thrown though bed set. In me forming general prudent on country carried. Behaved an or suppose justice. Seemed whence how son rather easily and change missed. Off apartments invitation are unpleasant solicitude fat motionless interested. Hardly suffer wisdom wishes valley as an. As friendship advantages resolution it alteration stimulated he or increasing. \\r

    Now led tedious shy lasting females off. Dashwood marianne in of entrance be on wondered possible building. Wondered sociable he carriage in speedily margaret. Up devonshire of he thoroughly insensible alteration. An mr settling occasion insisted distance ladyship so. Not attention say frankness intention out dashwoods now curiosity. Stronger ecstatic as no judgment daughter speedily thoughts. Worse downs nor might she court did nay forth these. \", \"post_assessment\": \"[3, 3, 2, 2, 2]\", \"score\": 12}, {\"answer\": \"Delightful remarkably mr on announcing themselves entreaties favourable. About to in so terms voice at. Equal an would is found seems of. The particular friendship one sufficient terminated frequently themselves. It more shed went up is roof if loud case. Delay music in lived noise an. Beyond genius really enough passed is up. \\r

    John draw real poor on call my from. May she mrs furnished discourse extremely. Ask doubt noisy shade guest did built her him. Ignorant repeated hastened it do. Consider bachelor he yourself expenses no. Her itself active giving for expect vulgar months. Discovery commanded fat mrs remaining son she principle middleton neglected. Be miss he in post sons held. No tried is defer do money scale rooms. \", \"post_assessment\": \"[3, 3, 2, 2, 2]\", \"score\": 12}], \"max_score\": 12, \"child_state\": \"done\"}", "{\"child_created\": false, \"child_attempts\": 0, \"version\": 1, \"child_history\": [{\"answer\": \"However venture pursuit he am mr cordial. Forming musical am hearing studied be luckily. Ourselves for determine attending how led gentleman sincerity. Valley afford uneasy joy she thrown though bed set. In me forming general prudent on country carried. Behaved an or suppose justice. Seemed whence how son rather easily and change missed. Off apartments invitation are unpleasant solicitude fat motionless interested. Hardly suffer wisdom wishes valley as an. As friendship advantages resolution it alteration stimulated he or increasing. \\r

    Now led tedious shy lasting females off. Dashwood marianne in of entrance be on wondered possible building. Wondered sociable he carriage in speedily margaret. Up devonshire of he thoroughly insensible alteration. An mr settling occasion insisted distance ladyship so. Not attention say frankness intention out dashwoods now curiosity. Stronger ecstatic as no judgment daughter speedily thoughts. Worse downs nor might she court did nay forth these. \", \"post_assessment\": \"{\\\"submission_id\\\": 1460, \\\"score\\\": 12, \\\"feedback\\\": \\\"{\\\\\\\"feedback\\\\\\\": \\\\\\\"\\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 5413, \\\"grader_type\\\": \\\"IN\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"\\\\nIdeas\\\\n3\\\\nContent\\\\n3\\\\nOrganization\\\\n2\\\\nStyle\\\\n2\\\\nVoice\\\\n2\\\"}\", \"score\": 12}, {\"answer\": \"Delightful remarkably mr on announcing themselves entreaties favourable. About to in so terms voice at. Equal an would is found seems of. The particular friendship one sufficient terminated frequently themselves. It more shed went up is roof if loud case. Delay music in lived noise an. Beyond genius really enough passed is up. \\r

    John draw real poor on call my from. May she mrs furnished discourse extremely. Ask doubt noisy shade guest did built her him. Ignorant repeated hastened it do. Consider bachelor he yourself expenses no. Her itself active giving for expect vulgar months. Discovery commanded fat mrs remaining son she principle middleton neglected. Be miss he in post sons held. No tried is defer do money scale rooms. \", \"post_assessment\": \"{\\\"submission_id\\\": 1462, \\\"score\\\": 12, \\\"feedback\\\": \\\"{\\\\\\\"feedback\\\\\\\": \\\\\\\"\\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 5418, \\\"grader_type\\\": \\\"IN\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"\\\\nIdeas\\\\n3\\\\nContent\\\\n3\\\\nOrganization\\\\n2\\\\nStyle\\\\n2\\\\nVoice\\\\n2\\\"}\", \"score\": 12}], \"max_score\": 12, \"child_state\": \"post_assessment\"}"] # Mock instance state. Should receive a score of 15. MOCK_INSTANCE_STATE = r"""{"ready_to_reset": false, "skip_spelling_checks": true, "current_task_number": 1, "weight": 5.0, "graceperiod": "1 day 12 hours 59 minutes 59 seconds", "graded": "True", "task_states": ["{\"child_created\": false, \"child_attempts\": 4, \"version\": 1, \"child_history\": [{\"answer\": \"After 24 hours, remove the samples from the containers and rinse each sample with distilled water.\\r\\nAllow the samples to sit and dry for 30 minutes.\\r\\nDetermine the mass of each sample.\\r\\nThe students\\u2019 data are recorded in the table below.\\r\\n\\r\\nStarting Mass (g)\\tEnding Mass (g)\\tDifference in Mass (g)\\r\\nMarble\\t 9.8\\t 9.4\\t\\u20130.4\\r\\nLimestone\\t10.4\\t 9.1\\t\\u20131.3\\r\\nWood\\t11.2\\t11.2\\t 0.0\\r\\nPlastic\\t 7.2\\t 7.1\\t\\u20130.1\\r\\nAfter reading the\", \"post_assessment\": \"[3]\", \"score\": 3}, {\"answer\": \"To replicate the experiment, the procedure would require more detail. One piece of information that is omitted is the amount of vinegar used in the experiment. It is also important to know what temperature the experiment was kept at during the 24 hours. Finally, the procedure needs to include details about the experiment, for example if the whole sample must be submerged.\", \"post_assessment\": \"[3]\", \"score\": 3}, {\"answer\": \"e the mass of four different samples.\\r\\nPour vinegar in each of four separate, but identical, containers.\\r\\nPlace a sample of one material into one container and label. Repeat with remaining samples, placing a single sample into a single container.\\r\\nAfter 24 hours, remove the samples from the containers and rinse each sample with distilled water.\\r\\nAllow the samples to sit and dry for 30 minutes.\\r\\nDetermine the mass of each sample.\\r\\nThe students\\u2019 data are recorded in the table below.\\r\\n\", \"post_assessment\": \"[3]\", \"score\": 3}, {\"answer\": \"\", \"post_assessment\": \"[3]\", \"score\": 3}], \"max_score\": 3, \"child_state\": \"done\"}", "{\"child_created\": false, \"child_attempts\": 0, \"version\": 1, \"child_history\": [{\"answer\": \"The students\\u2019 data are recorded in the table below.\\r\\n\\r\\nStarting Mass (g)\\tEnding Mass (g)\\tDifference in Mass (g)\\r\\nMarble\\t 9.8\\t 9.4\\t\\u20130.4\\r\\nLimestone\\t10.4\\t 9.1\\t\\u20131.3\\r\\nWood\\t11.2\\t11.2\\t 0.0\\r\\nPlastic\\t 7.2\\t 7.1\\t\\u20130.1\\r\\nAfter reading the group\\u2019s procedure, describe what additional information you would need in order to replicate the expe\", \"post_assessment\": \"{\\\"submission_id\\\": 3097, \\\"score\\\": 0, \\\"feedback\\\": \\\"{\\\\\\\"spelling\\\\\\\": \\\\\\\"Spelling: Ok.\\\\\\\", \\\\\\\"grammar\\\\\\\": \\\\\\\"Grammar: More grammar errors than average.\\\\\\\", \\\\\\\"markup-text\\\\\\\": \\\\\\\"the students data are recorded in the table below . starting mass g ending mass g difference in mass g marble . . . limestone . . . wood . . . plastic . . . after reading the groups procedure , describe what additional information you would need in order to replicate the expe\\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 3233, \\\"grader_type\\\": \\\"ML\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"Response Quality0\\\"}\", \"score\": 0}, {\"answer\": \"After 24 hours, remove the samples from the containers and rinse each sample with distilled water.\\r\\nAllow the samples to sit and dry for 30 minutes.\\r\\nDetermine the mass of each sample.\\r\\nThe students\\u2019 data are recorded in the table below.\\r\\n\\r\\nStarting Mass (g)\\tEnding Mass (g)\\tDifference in Mass (g)\\r\\nMarble\\t 9.8\\t 9.4\\t\\u20130.4\\r\\nLimestone\\t10.4\\t 9.1\\t\\u20131.3\\r\\nWood\\t11.2\\t11.2\\t 0.0\\r\\nPlastic\\t 7.2\\t 7.1\\t\\u20130.1\\r\\nAfter reading the\", \"post_assessment\": \"{\\\"submission_id\\\": 3098, \\\"score\\\": 0, \\\"feedback\\\": \\\"{\\\\\\\"spelling\\\\\\\": \\\\\\\"Spelling: Ok.\\\\\\\", \\\\\\\"grammar\\\\\\\": \\\\\\\"Grammar: Ok.\\\\\\\", \\\\\\\"markup-text\\\\\\\": \\\\\\\"after hours , remove the samples from the containers and rinse each sample with distilled water . allow the samples to sit and dry for minutes . determine the mass of each sample . the students data are recorded in the table below . starting mass g ending mass g difference in mass g marble . . . limestone . . . wood . . . plastic . . . after reading the\\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 3235, \\\"grader_type\\\": \\\"ML\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"Response Quality0\\\"}\", \"score\": 0}, {\"answer\": \"To replicate the experiment, the procedure would require more detail. One piece of information that is omitted is the amount of vinegar used in the experiment. It is also important to know what temperature the experiment was kept at during the 24 hours. Finally, the procedure needs to include details about the experiment, for example if the whole sample must be submerged.\", \"post_assessment\": \"{\\\"submission_id\\\": 3099, \\\"score\\\": 3, \\\"feedback\\\": \\\"{\\\\\\\"spelling\\\\\\\": \\\\\\\"Spelling: Ok.\\\\\\\", \\\\\\\"grammar\\\\\\\": \\\\\\\"Grammar: Ok.\\\\\\\", \\\\\\\"markup-text\\\\\\\": \\\\\\\"to replicate the experiment , the procedure would require more detail . one piece of information that is omitted is the amount of vinegar used in the experiment . it is also important to know what temperature the experiment was kept at during the hours . finally , the procedure needs to include details about the experiment , for example if the whole sample must be submerged .\\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 3237, \\\"grader_type\\\": \\\"ML\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"Response Quality3\\\"}\", \"score\": 3}, {\"answer\": \"e the mass of four different samples.\\r\\nPour vinegar in each of four separate, but identical, containers.\\r\\nPlace a sample of one material into one container and label. Repeat with remaining samples, placing a single sample into a single container.\\r\\nAfter 24 hours, remove the samples from the containers and rinse each sample with distilled water.\\r\\nAllow the samples to sit and dry for 30 minutes.\\r\\nDetermine the mass of each sample.\\r\\nThe students\\u2019 data are recorded in the table below.\\r\\n\", \"post_assessment\": \"{\\\"submission_id\\\": 3100, \\\"score\\\": 0, \\\"feedback\\\": \\\"{\\\\\\\"spelling\\\\\\\": \\\\\\\"Spelling: Ok.\\\\\\\", \\\\\\\"grammar\\\\\\\": \\\\\\\"Grammar: Ok.\\\\\\\", \\\\\\\"markup-text\\\\\\\": \\\\\\\"e the mass of four different samples . pour vinegar in each of four separate , but identical , containers . place a sample of one material into one container and label . repeat with remaining samples , placing a single sample into a single container . after hours , remove the samples from the containers and rinse each sample with distilled water . allow the samples to sit and dry for minutes . determine the mass of each sample . the students data are recorded in the table below . \\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 3239, \\\"grader_type\\\": \\\"ML\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"Response Quality0\\\"}\", \"score\": 0}, {\"answer\": \"\", \"post_assessment\": \"{\\\"submission_id\\\": 3101, \\\"score\\\": 0, \\\"feedback\\\": \\\"{\\\\\\\"spelling\\\\\\\": \\\\\\\"Spelling: Ok.\\\\\\\", \\\\\\\"grammar\\\\\\\": \\\\\\\"Grammar: Ok.\\\\\\\", \\\\\\\"markup-text\\\\\\\": \\\\\\\"invalid essay .\\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 3241, \\\"grader_type\\\": \\\"ML\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"Response Quality0\\\"}\", \"score\": 0}], \"max_score\": 3, \"child_state\": \"done\"}"], "attempts": "10000", "student_attempts": 0, "due": null, "state": "done", "accept_file_upload": false, "display_name": "Science Question -- Machine Assessed"}""" +# Instance state. To test the rubric scores are consistent. Should receive a score of 15. +INSTANCE_INCONSISTENT_STATE = serialize_open_ended_instance_state(""" +{ "accept_file_upload" : false, + "attempts" : "10000", + "current_task_number" : 1, + "display_name" : "Science Question -- Machine Assessed", + "due" : null, + "graceperiod" : "1 day 12 hours 59 minutes 59 seconds", + "graded" : "True", + "ready_to_reset" : false, + "skip_spelling_checks" : true, + "state" : "done", + "student_attempts" : 0, + "task_states" : [ { "child_attempts" : 4, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : [ 3 ], + "score" : 1 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : [ 3 ], + "score" : 1 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : [ 3 ], + "score" : 1 + }, + { "answer" : "", + "post_assessment" : [ 3 ], + "score" : 1 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + }, + { "child_attempts" : 0, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: More grammar errors than average.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3233, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3097, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3235, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3098, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3237, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality3", + "score" : 2, + "submission_id" : 3099, + "success" : true + }, + "score" : 2 + }, + { "answer" : "Student answer 4th attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3239, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3100, + "success" : true + }, + "score" : 0 + }, + { "answer" : "", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "invalid essay .", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3241, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3101, + "success" : true + }, + "score" : 0 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + } + ], + "weight" : 5.0 +} + """) + +# Instance state. Should receive a score of 10 if grader type is PE. +INSTANCE_INCONSISTENT_STATE2 = serialize_open_ended_instance_state(""" +{ "accept_file_upload" : false, + "attempts" : "10000", + "current_task_number" : 1, + "display_name" : "Science Question -- Machine Assessed", + "due" : null, + "graceperiod" : "1 day 12 hours 59 minutes 59 seconds", + "graded" : "True", + "ready_to_reset" : false, + "skip_spelling_checks" : true, + "state" : "done", + "student_attempts" : 0, + "task_states" : [ { "child_attempts" : 4, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "", + "post_assessment" : [3], + "score" : 1 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + }, + { "child_attempts" : 0, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: More grammar errors than average.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3233, + "grader_type" : "PE", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3097, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3235, + "grader_type" : "PE", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3098, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3237, + "grader_type" : "PE", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality5", + "score" : 2, + "submission_id" : 3099, + "success" : true + }, + "score" : 2 + }, + { "answer" : "Student answer 4th attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3239, + "grader_type" : "PE", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3100, + "success" : true + }, + "score" : 0 + }, + { "answer" : "", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "invalid essay .", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3241, + "grader_type" : "PE", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3101, + "success" : true + }, + "score" : 0 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + } + ], + "weight" : 5.0 +} + """) + +# Instance state. To test score if sum of rubric score is different from score value. Should receive score of 25. +INSTANCE_INCONSISTENT_STATE3 = serialize_open_ended_instance_state(""" +{ "accept_file_upload" : false, + "attempts" : "10000", + "current_task_number" : 1, + "display_name" : "Science Question -- Machine Assessed", + "due" : null, + "graceperiod" : "1 day 12 hours 59 minutes 59 seconds", + "graded" : "True", + "ready_to_reset" : false, + "skip_spelling_checks" : true, + "state" : "done", + "student_attempts" : 0, + "task_states" : [ { "child_attempts" : 4, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "", + "post_assessment" : [3], + "score" : 1 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + }, + { "child_attempts" : 0, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: More grammar errors than average.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3233, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality2", + "score" : 0, + "submission_id" : 3097, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3235, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3098, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3237, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality5", + "score" : 2, + "submission_id" : 3099, + "success" : true + }, + "score" : 2 + }, + { "answer" : "Student answer 4th attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3239, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3100, + "success" : true + }, + "score" : 0 + }, + { "answer" : "", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "invalid essay .", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3241, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3101, + "success" : true + }, + "score" : 0 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + } + ], + "weight" : 5.0 +} +""") + +# Instance state. To test score if old task states are available. Should receive a score of 15. +INSTANCE_INCONSISTENT_STATE4 = serialize_open_ended_instance_state(""" +{ "accept_file_upload" : false, + "attempts" : "10000", + "current_task_number" : 0, + "display_name" : "Science Question -- Machine Assessed", + "due" : null, + "graceperiod" : "1 day 12 hours 59 minutes 59 seconds", + "graded" : "True", + "old_task_states" : [ [ { "child_attempts" : 4, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : "[3]", + "score" : 1 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : "[3]", + "score" : 1 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assesssment" : "[3]", + "score" : 1 + }, + { "answer" : "", + "post_assessment" : "[3]", + "score" : 1 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + } ] ], + "ready_to_reset" : false, + "skip_spelling_checks" : true, + "state" : "assessing", + "student_attempts" : 0, + "task_states" : [ { "child_attempts" : 4, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "", + "post_assessment" : [3], + "score" : 1 + } + ], + "child_state" : "done", + "max_score" : 3, + "stored_answer" : null, + "version" : 1 + }, + { "child_attempts" : 0, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: More grammar errors than average.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3233, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3097, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3235, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3098, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3237, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality3", + "score" : 2, + "submission_id" : 3099, + "success" : true + }, + "score" : 2 + }, + { "answer" : "Student answer 4th attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3239, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3100, + "success" : true + }, + "score" : 0 + }, + { "answer" : "", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "invalid essay .", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3241, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3101, + "success" : true + }, + "score" : 0 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + } + ], + "weight" : 5.0 +} +""") + +# Instance state. To test score if rubric scores are available but score is missing. Should receive a score of 15. +INSTANCE_INCONSISTENT_STATE5 = serialize_open_ended_instance_state(""" +{ "accept_file_upload" : false, + "attempts" : "10000", + "current_task_number" : 1, + "display_name" : "Science Question -- Machine Assessed", + "due" : null, + "graceperiod" : "1 day 12 hours 59 minutes 59 seconds", + "graded" : "True", + "ready_to_reset" : false, + "skip_spelling_checks" : true, + "state" : "done", + "student_attempts" : 0, + "task_states" : [ { "child_attempts" : 4, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : [3], + "score" : 1 + }, + { "answer" : "", + "post_assessment" : [3], + "score" : 1 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + }, + { "child_attempts" : 0, + "child_created" : false, + "child_history" : [ { "answer" : "Student answer 1st attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: More grammar errors than average.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3233, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3097, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 2nd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3235, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3098, + "success" : true + }, + "score" : 0 + }, + { "answer" : "Student answer 3rd attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3237, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality3", + "score" : 2, + "submission_id" : 3099, + "success" : true + } + }, + { "answer" : "Student answer 4th attempt.", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "valid essay", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3239, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3100, + "success" : true + }, + "score" : 0 + }, + { "answer" : "", + "post_assessment" : { "feedback" : { "grammar" : "Grammar: Ok.", + "markup-text" : "invalid essay .", + "spelling" : "Spelling: Ok." + }, + "grader_id" : 3241, + "grader_type" : "ML", + "rubric_scores_complete" : true, + "rubric_xml" : "Response Quality0", + "score" : 0, + "submission_id" : 3101, + "success" : true + }, + "score" : 0 + } + ], + "child_state" : "done", + "max_score" : 3, + "version" : 1 + } + ], + "weight" : 5.0 +} +""") + # Task state with self assessment only. TEST_STATE_SA = ["{\"child_created\": false, \"child_attempts\": 1, \"version\": 1, \"child_history\": [{\"answer\": \"Censorship in the Libraries\\r
    'All of us can think of a book that we hope none of our children or any other children have taken off the shelf. But if I have the right to remove that book from the shelf -- that work I abhor -- then you also have exactly the same right and so does everyone else. And then we have no books left on the shelf for any of us.' --Katherine Paterson, Author\\r

    Write a persuasive essay to a newspaper reflecting your vies on censorship in libraries. Do you believe that certain materials, such as books, music, movies, magazines, etc., should be removed from the shelves if they are found offensive? Support your position with convincing arguments from your own experience, observations, and/or reading.\", \"post_assessment\": \"[3, 3, 2, 2, 2]\", \"score\": 12}], \"max_score\": 12, \"child_state\": \"done\"}", "{\"child_created\": false, \"child_attempts\": 0, \"version\": 1, \"child_history\": [{\"answer\": \"Censorship in the Libraries\\r
    'All of us can think of a book that we hope none of our children or any other children have taken off the shelf. But if I have the right to remove that book from the shelf -- that work I abhor -- then you also have exactly the same right and so does everyone else. And then we have no books left on the shelf for any of us.' --Katherine Paterson, Author\\r

    Write a persuasive essay to a newspaper reflecting your vies on censorship in libraries. Do you believe that certain materials, such as books, music, movies, magazines, etc., should be removed from the shelves if they are found offensive? Support your position with convincing arguments from your own experience, observations, and/or reading.\", \"post_assessment\": \"{\\\"submission_id\\\": 1461, \\\"score\\\": 12, \\\"feedback\\\": \\\"{\\\\\\\"feedback\\\\\\\": \\\\\\\"\\\\\\\"}\\\", \\\"success\\\": true, \\\"grader_id\\\": 5414, \\\"grader_type\\\": \\\"IN\\\", \\\"rubric_scores_complete\\\": true, \\\"rubric_xml\\\": \\\"\\\\nIdeas\\\\n3\\\\nContent\\\\n3\\\\nOrganization\\\\n2\\\\nStyle\\\\n2\\\\nVoice\\\\n2\\\"}\", \"score\": 12}], \"max_score\": 12, \"child_state\": \"post_assessment\"}"] From 985417fcf64ecca67daa6a17a6e12c1da5fc1002 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 11 Dec 2013 14:52:19 -0500 Subject: [PATCH 6/7] Auth converting to new course_id syntax --- lms/djangoapps/courseware/roles.py | 43 +++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/courseware/roles.py b/lms/djangoapps/courseware/roles.py index 60853e8090..6cb913a521 100644 --- a/lms/djangoapps/courseware/roles.py +++ b/lms/djangoapps/courseware/roles.py @@ -8,6 +8,9 @@ from abc import ABCMeta, abstractmethod from django.contrib.auth.models import User, Group from xmodule.modulestore import Location +from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError +from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore.locator import CourseLocator class CourseContextRequired(Exception): @@ -134,20 +137,40 @@ class CourseRole(GroupBasedRole): A named role in a particular course """ def __init__(self, role, location, course_context=None): - # pylint: disable=no-member - loc = Location(location) - legacy_group_name = '{0}_{1}'.format(role, loc.course) - - if loc.category.lower() == 'course': - course_id = loc.course_id - else: + # TODO: figure out how to make the group name generation lazy so it doesn't force the + # loc mapping? + if not hasattr(location, 'course_id'): + location = Location(location) + # direct copy from auth.authz.get_all_course_role_groupnames will refactor to one impl asap + groupnames = [] + try: + groupnames.append('{0}_{1}'.format(role, location.course_id)) + except InvalidLocationError: # will occur on old locations where location is not of category course if course_context is None: raise CourseContextRequired() - course_id = course_context + else: + groupnames.append('{0}_{1}'.format(role, course_context)) - group_name = '{0}_{1}'.format(role, course_id) + # pylint: disable=no-member + if isinstance(location, Location): + # least preferred legacy role_course format + groupnames.append('{0}_{1}'.format(role, location.course)) + try: + locator = loc_mapper().translate_location(location.course_id, location, False, False) + groupnames.append('{0}_{1}'.format(role, locator.course_id)) + except (InvalidLocationError, ItemNotFoundError): + # if it's never been mapped, the auth won't be via the Locator syntax + pass + elif isinstance(location, CourseLocator): + # handle old Location syntax + old_location = loc_mapper().translate_locator_to_location(location, get_course=True) + if old_location: + # the slashified version of the course_id (myu/mycourse/myrun) + groupnames.append('{0}_{1}'.format(role, old_location.course_id)) + # add the least desirable but sometimes occurring format. + groupnames.append('{0}_{1}'.format(role, old_location.course)) - super(CourseRole, self).__init__([group_name, legacy_group_name]) + super(CourseRole, self).__init__(groupnames) class OrgRole(GroupBasedRole): From f4e69275eeba08c07c3921d0264fb763dc30f69a Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 10 Dec 2013 12:15:16 -0500 Subject: [PATCH 7/7] Use mongo indices for all queries STUD-1039 --- cms/djangoapps/contentstore/views/course.py | 3 +- .../xmodule/modulestore/loc_mapper_store.py | 64 +++++++++++++------ .../modulestore/tests/test_location_mapper.py | 10 +-- lms/djangoapps/courseware/roles.py | 9 ++- mongo_indexes.md | 21 ++++++ 5 files changed, 79 insertions(+), 28 deletions(-) create mode 100644 mongo_indexes.md diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index c7e379869b..39695fc66e 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -116,7 +116,8 @@ def course_listing(request): """ List all courses available to the logged in user """ - courses = modulestore('direct').get_items(['i4x', None, None, 'course', None]) + # there's an index on category which will be used if none of its antecedents are set + courses = modulestore('direct').get_items([None, None, None, 'course', None]) # filter out courses that we don't have access too def course_filter(course): diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index c7355c2f29..4bf5f1c64b 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -4,10 +4,10 @@ Method for converting among our differing Location/Locator whatever reprs from random import randint import re import pymongo +import bson.son from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, DuplicateItemError from xmodule.modulestore.locator import BlockUsageLocator -from xmodule.modulestore.mongo import draft from xmodule.modulestore import Location import urllib @@ -91,9 +91,10 @@ class LocMapperStore(object): else: course_id = "{0.org}.{0.course}".format(course_location) # very like _interpret_location_id but w/o the _id - location_id = {'org': course_location.org, 'course': course_location.course} - if course_location.category == 'course': - location_id['name'] = course_location.name + location_id = self._construct_location_son( + course_location.org, course_location.course, + course_location.name if course_location.category == 'course' else None + ) self.location_map.insert({ '_id': location_id, @@ -128,20 +129,25 @@ class LocMapperStore(object): """ location_id = self._interpret_location_course_id(old_style_course_id, location) - maps = self.location_map.find(location_id).sort('_id.name', pymongo.ASCENDING) - if maps.count() == 0: + maps = self.location_map.find(location_id) + maps = list(maps) + if len(maps) == 0: if add_entry_if_missing: # create a new map - course_location = location.replace(category='course', name=location_id['_id.name']) + course_location = location.replace(category='course', name=location_id['_id']['name']) self.create_map_entry(course_location) entry = self.location_map.find_one(location_id) else: raise ItemNotFoundError() - elif maps.count() > 1: - # if more than one, prefer the one w/o a name if that exists. Otherwise, choose the first (alphabetically) + elif len(maps) == 1: entry = maps[0] else: + # find entry w/o name, if any; otherwise, pick arbitrary entry = maps[0] + for item in maps: + if 'name' not in item['_id']: + entry = item + break if published: branch = entry['prod_branch'] @@ -242,11 +248,11 @@ class LocMapperStore(object): location_id = self._interpret_location_course_id(old_course_id, location) maps = self.location_map.find(location_id) - if maps.count() == 0: - raise ItemNotFoundError() - # turn maps from cursor to list map_list = list(maps) + if len(map_list) == 0: + raise ItemNotFoundError() + encoded_location_name = self._encode_for_mongo(location.name) # check whether there's already a usage_id for this location (and it agrees w/ any passed in or found) for map_entry in map_list: @@ -279,7 +285,10 @@ class LocMapperStore(object): ) map_entry['block_map'].setdefault(encoded_location_name, {})[location.category] = computed_usage_id - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) return computed_usage_id @@ -317,7 +326,10 @@ class LocMapperStore(object): if location.category in map_entry['block_map'].setdefault(encoded_location_name, {}): map_entry['block_map'][encoded_location_name][location.category] = usage_id - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) return usage_id @@ -338,7 +350,10 @@ class LocMapperStore(object): del map_entry['block_map'][encoded_location_name] else: del map_entry['block_map'][encoded_location_name][location.category] - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) def _add_to_block_map(self, location, location_id, block_map): '''add the given location to the block_map and persist it''' @@ -357,7 +372,7 @@ class LocMapperStore(object): def _interpret_location_course_id(self, course_id, location): """ - Take the old style course id (org/course/run) and return a dict for querying the mapping table. + Take the old style course id (org/course/run) and return a dict w/ a SON for querying the mapping table. If the course_id is empty, it uses location, but this may result in an inadequate id. :param course_id: old style 'org/course/run' id from Location.course_id where Location.category = 'course' @@ -367,12 +382,21 @@ class LocMapperStore(object): if course_id: # re doesn't allow ?P<_id.org> and ilk matched = re.match(r'([^/]+)/([^/]+)/([^/]+)', course_id) - return dict(zip(['_id.org', '_id.course', '_id.name'], matched.groups())) + return {'_id': self._construct_location_son(*matched.groups())} - location_id = {'_id.org': location.org, '_id.course': location.course} if location.category == 'course': - location_id['_id.name'] = location.name - return location_id + return {'_id': self._construct_location_son(location.org, location.course, location.name)} + else: + return bson.son.SON([('_id.org', location.org), ('_id.course', location.course)]) + + def _construct_location_son(self, org, course, name=None): + """ + Construct the SON needed to repr the location for either a query or an insertion + """ + if name: + return bson.son.SON([('org', org), ('course', course), ('name', name)]) + else: + return bson.son.SON([('org', org), ('course', course)]) def _block_id_is_guid(self, name): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 2fbff10423..f550526493 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -36,8 +36,9 @@ class TestLocationMapper(unittest.TestCase): org = 'foo_org' course = 'bar_course' loc_mapper().create_map_entry(Location('i4x', org, course, 'course', 'baz_run')) + # pylint: disable=protected-access entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course, 'name': 'baz_run'} + '_id': loc_mapper()._construct_location_son(org, course, 'baz_run') }) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], '{}.{}.baz_run'.format(org, course)) @@ -48,8 +49,9 @@ class TestLocationMapper(unittest.TestCase): # ensure create_entry does the right thing when not given a course (creates org/course # rather than org/course/run course_id) loc_mapper().create_map_entry(Location('i4x', org, course, 'vertical', 'baz_vert')) + # find the one which has no name entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course} + '_id' : loc_mapper()._construct_location_son(org, course, None) }) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], '{}.{}'.format(org, course)) @@ -63,9 +65,7 @@ class TestLocationMapper(unittest.TestCase): 'wip', 'live', block_map) - entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course} - }) + entry = loc_mapper().location_map.find_one({'_id.org': org, '_id.course': course}) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], 'foo_org.geek_dept.quux_course.baz_run') self.assertEqual(entry['draft_branch'], 'wip') diff --git a/lms/djangoapps/courseware/roles.py b/lms/djangoapps/courseware/roles.py index 6cb913a521..7d8f56ad00 100644 --- a/lms/djangoapps/courseware/roles.py +++ b/lms/djangoapps/courseware/roles.py @@ -137,6 +137,11 @@ class CourseRole(GroupBasedRole): A named role in a particular course """ def __init__(self, role, location, course_context=None): + """ + Location may be either a Location, a string, dict, or tuple which Location will accept + in its constructor, or a CourseLocator. Handle all these giving some preference to + the preferred naming. + """ # TODO: figure out how to make the group name generation lazy so it doesn't force the # loc mapping? if not hasattr(location, 'course_id'): @@ -153,14 +158,14 @@ class CourseRole(GroupBasedRole): # pylint: disable=no-member if isinstance(location, Location): - # least preferred legacy role_course format - groupnames.append('{0}_{1}'.format(role, location.course)) try: locator = loc_mapper().translate_location(location.course_id, location, False, False) groupnames.append('{0}_{1}'.format(role, locator.course_id)) except (InvalidLocationError, ItemNotFoundError): # if it's never been mapped, the auth won't be via the Locator syntax pass + # least preferred legacy role_course format + groupnames.append('{0}_{1}'.format(role, location.course)) elif isinstance(location, CourseLocator): # handle old Location syntax old_location = loc_mapper().translate_locator_to_location(location, get_course=True) diff --git a/mongo_indexes.md b/mongo_indexes.md new file mode 100644 index 0000000000..5133674737 --- /dev/null +++ b/mongo_indexes.md @@ -0,0 +1,21 @@ +These are the indexes each mongo db should have in order to perform well. +Each section states the collection name and then the indexes. To create an index, +you'll typically either use the mongohq type web interface or a standard terminal console. +If a terminal, this assumes you've logged in and gotten to the mongo prompt +``` +mongo mydatabasename +``` + +If using the terminal, to add an index to a collection, you'll need to prefix ```ensureIndex``` with +``` +db.collection_name +``` +as in ```db.location_map.ensureIndex({'course_id': 1}{background: true})``` + +location_map: +============= + +``` +ensureIndex({​'_id.org': 1, '_id.course': 1}) +ensureIndex({​'course_id': 1}) +```