From 4896444d10ec650f1c44e4c9a4c01178b61114c7 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Fri, 2 Aug 2013 09:29:55 -0400 Subject: [PATCH 1/9] Clean up item views, use JsonResponse class --- cms/djangoapps/contentstore/views/item.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index efebded9b9..ff347a2878 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -1,15 +1,13 @@ -import json from uuid import uuid4 from django.core.exceptions import PermissionDenied -from django.http import HttpResponse from django.contrib.auth.decorators import login_required from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.modulestore.inheritance import own_metadata -from util.json_request import expect_json +from util.json_request import expect_json, JsonResponse from ..utils import get_modulestore from .access import has_access from .requests import _xmodule_recurse @@ -20,6 +18,7 @@ __all__ = ['save_item', 'create_item', 'delete_item'] # cdodge: these are categories which should not be parented, they are detached from the hierarchy DETACHED_CATEGORIES = ['about', 'static_tab', 'course_info'] + @login_required @expect_json def save_item(request): @@ -80,7 +79,7 @@ def save_item(request): # commit to datastore store.update_metadata(item_location, own_metadata(existing_item)) - return HttpResponse() + return JsonResponse() # [DHM] A hack until we implement a permanent soln. Proposed perm solution is to make namespace fields also top level @@ -139,13 +138,17 @@ def create_item(request): if display_name is not None: metadata['display_name'] = display_name - get_modulestore(category).create_and_save_xmodule(dest_location, definition_data=data, - metadata=metadata, system=parent.system) + get_modulestore(category).create_and_save_xmodule( + dest_location, + definition_data=data, + metadata=metadata, + system=parent.system, + ) if category not in DETACHED_CATEGORIES: get_modulestore(parent.location).update_children(parent_location, parent.children + [dest_location.url()]) - return HttpResponse(json.dumps({'id': dest_location.url()})) + return JsonResponse({'id': dest_location.url()}) @login_required @@ -184,4 +187,4 @@ def delete_item(request): parent.children = children modulestore('direct').update_children(parent.location, parent.children) - return HttpResponse() + return JsonResponse() From be103dfa0d3e758cec8c25eb08181d0570e315a9 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 31 Jul 2013 12:50:22 -0400 Subject: [PATCH 2/9] improving code style --- common/lib/xmodule/xmodule/x_module.py | 70 +++++++++++--------------- 1 file changed, 29 insertions(+), 41 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 3556f3f0f3..310a871b72 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -711,20 +711,20 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): # =============================== BUILTIN METHODS ========================== def __eq__(self, other): - eq = (self.__class__ == other.__class__ and + return (self.__class__ == other.__class__ and all(getattr(self, attr, None) == getattr(other, attr, None) for attr in self.equality_attributes)) - return eq - def __repr__(self): - return ("{class_}({system!r}, location={location!r}," - " model_data={model_data!r})".format( - class_=self.__class__.__name__, - system=self.system, - location=self.location, - model_data=self._model_data, - )) + return ( + "{class_}({system!r}, location={location!r}," + " model_data={model_data!r})".format( + class_=self.__class__.__name__, + system=self.system, + location=self.location, + model_data=self._model_data, + ) + ) @property def non_editable_metadata_fields(self): @@ -785,15 +785,17 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): editor_type = "Float" elif isinstance(field, List): editor_type = "List" - metadata_fields[field.name] = {'field_name': field.name, - 'type': editor_type, - 'display_name': field.display_name, - 'value': field.to_json(value), - 'options': [] if values is None else values, - 'default_value': field.to_json(default_value), - 'inheritable': inheritable, - 'explicitly_set': explicitly_set, - 'help': field.help} + metadata_fields[field.name] = { + 'field_name': field.name, + 'type': editor_type, + 'display_name': field.display_name, + 'value': field.to_json(value), + 'options': [] if values is None else values, + 'default_value': field.to_json(default_value), + 'inheritable': inheritable, + 'explicitly_set': explicitly_set, + 'help': field.help, + } return metadata_fields @@ -885,28 +887,14 @@ class ModuleSystem(Runtime): Note that these functions can be closures over e.g. a django request and user, or other environment-specific info. ''' - def __init__(self, - ajax_url, - track_function, - get_module, - render_template, - replace_urls, - xblock_model_data, - user=None, - filestore=None, - debug=False, - xqueue=None, - publish=None, - node_path="", - anonymous_student_id='', - course_id=None, - open_ended_grading_interface=None, - s3_interface=None, - cache=None, - can_execute_unsafe_code=None, - replace_course_urls=None, - replace_jump_to_id_urls=None - ): + def __init__( + self, ajax_url, track_function, get_module, render_template, + replace_urls, xblock_model_data, user=None, filestore=None, + debug=False, xqueue=None, publish=None, node_path="", + anonymous_student_id='', course_id=None, + open_ended_grading_interface=None, s3_interface=None, + cache=None, can_execute_unsafe_code=None, replace_course_urls=None, + replace_jump_to_id_urls=None): ''' Create a closure around the system environment. From 9634e222bed244d3310f449faa137a029493977b Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 31 Jul 2013 12:51:01 -0400 Subject: [PATCH 3/9] Refactored get_module_previews function --- cms/djangoapps/contentstore/views/preview.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index f2a07abe32..a9c9757d1d 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -163,6 +163,11 @@ def load_preview_module(request, preview_id, descriptor): return module +def get_preview_html(request, descriptor, idx): + module = load_preview_module(request, str(idx), descriptor) + return module.get_html() + + def get_module_previews(request, descriptor): """ Returns a list of preview XModule html contents. One preview is returned for each @@ -170,8 +175,5 @@ def get_module_previews(request, descriptor): descriptor: An XModuleDescriptor """ - preview_html = [] - for idx, (_instance_state, _shared_state) in enumerate(descriptor.get_sample_state()): - module = load_preview_module(request, str(idx), descriptor) - preview_html.append(module.get_html()) - return preview_html + return tuple(get_preview_html(request, descriptor, idx) + for idx in range(len(descriptor.get_sample_state()))) From 8a95d7e6f0c72fe9836f01a209e6eb1d4ca4bab4 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Fri, 2 Aug 2013 13:51:46 -0400 Subject: [PATCH 4/9] XBlock integration: replaced `get_html` with `runtime.render()` Currently calls the same machinery, but re-routes the logic in preparation of deeper integration with XBlock --- cms/djangoapps/contentstore/views/preview.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index a9c9757d1d..fa55cb2c24 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -165,7 +165,7 @@ def load_preview_module(request, preview_id, descriptor): def get_preview_html(request, descriptor, idx): module = load_preview_module(request, str(idx), descriptor) - return module.get_html() + return module.runtime.render(module, None, "student_view") def get_module_previews(request, descriptor): From 3fa990ea60dc3c704c82eea3539d1a71a5eafbd3 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 5 Aug 2013 09:38:51 -0400 Subject: [PATCH 5/9] Made some tests more general, less fragile --- .../contentstore/tests/test_contentstore.py | 30 +++++++++---------- .../contentstore/tests/test_item.py | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 838af2cafa..64fa53433e 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1237,7 +1237,7 @@ class ContentStoreTest(ModuleStoreTestCase): 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) self.assertContains(resp, 'Chapter 2') # go to various pages @@ -1247,92 +1247,92 @@ class ContentStoreTest(ModuleStoreTestCase): kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # export page resp = self.client.get(reverse('export_course', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # manage users resp = self.client.get(reverse('manage_users', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # course info resp = self.client.get(reverse('course_info', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # settings_details resp = self.client.get(reverse('settings_details', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # settings_details resp = self.client.get(reverse('settings_grading', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # static_pages resp = self.client.get(reverse('static_pages', kwargs={'org': loc.org, 'course': loc.course, 'coursename': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # static_pages resp = self.client.get(reverse('asset_index', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # go look at a subsection page subsection_location = loc.replace(category='sequential', name='test_sequence') resp = self.client.get(reverse('edit_subsection', kwargs={'location': subsection_location.url()})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # go look at the Edit page unit_location = loc.replace(category='vertical', name='test_vertical') resp = self.client.get(reverse('edit_unit', kwargs={'location': unit_location.url()})) - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # delete a component del_loc = loc.replace(category='html', name='test_html') resp = self.client.post(reverse('delete_item'), json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # delete a unit del_loc = loc.replace(category='vertical', name='test_vertical') resp = self.client.post(reverse('delete_item'), json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # delete a unit del_loc = loc.replace(category='sequential', name='test_sequence') resp = self.client.post(reverse('delete_item'), json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) # delete a chapter del_loc = loc.replace(category='chapter', name='chapter_2') resp = self.client.post(reverse('delete_item'), json.dumps({'id': del_loc.url()}), "application/json") - self.assertEqual(200, resp.status_code) + self.assert2XX(resp.status_code) def test_import_metadata_with_attempts_empty_string(self): module_store = modulestore('direct') diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index 827dd1b054..260444a8f7 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/cms/djangoapps/contentstore/tests/test_item.py @@ -34,7 +34,7 @@ class DeleteItem(CourseTestCase): resp.content, "application/json" ) - self.assertEqual(resp.status_code, 200) + self.assert2XX(resp.status_code) class TestCreateItem(CourseTestCase): From baa9bd5bdca69c358f2f4e81e4632febe2f6019f Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 5 Aug 2013 11:04:23 -0400 Subject: [PATCH 6/9] Make sure to return the content, not the fragment --- cms/djangoapps/contentstore/views/preview.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index fa55cb2c24..202927bdfb 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -165,7 +165,7 @@ def load_preview_module(request, preview_id, descriptor): def get_preview_html(request, descriptor, idx): module = load_preview_module(request, str(idx), descriptor) - return module.runtime.render(module, None, "student_view") + return module.runtime.render(module, None, "student_view").content def get_module_previews(request, descriptor): From a87a1bfcdab0fa42d169f630e9eab85137e50e29 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 7 Aug 2013 16:14:07 -0400 Subject: [PATCH 7/9] Docstrings --- cms/djangoapps/contentstore/views/preview.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 202927bdfb..1fef30dd99 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -164,13 +164,16 @@ def load_preview_module(request, preview_id, descriptor): def get_preview_html(request, descriptor, idx): + """ + Returns the HTML for the XModule specified by the descriptor and idx. + """ module = load_preview_module(request, str(idx), descriptor) return module.runtime.render(module, None, "student_view").content def get_module_previews(request, descriptor): """ - Returns a list of preview XModule html contents. One preview is returned for each + Returns a tuple of preview XModule html contents. One preview is returned for each pair of states returned by get_sample_state() for the supplied descriptor. descriptor: An XModuleDescriptor From 7aec95c3100132149e9f84f6d8e58927f78655e7 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Thu, 8 Aug 2013 09:52:39 -0400 Subject: [PATCH 8/9] Removed get_module_previews function According to @cpennington, no modules return anything for `get_sample_state`, so this function is extraneous. --- cms/djangoapps/contentstore/views/preview.py | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 1fef30dd99..a325dd3b34 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -76,7 +76,7 @@ def preview_component(request, location): component = modulestore().get_item(location) return render_to_response('component.html', { - 'preview': get_module_previews(request, component)[0], + 'preview': get_preview_html(request, component, 0), 'editor': wrap_xmodule(component.get_html, component, 'xmodule_edit.html')(), }) @@ -169,14 +169,3 @@ def get_preview_html(request, descriptor, idx): """ module = load_preview_module(request, str(idx), descriptor) return module.runtime.render(module, None, "student_view").content - - -def get_module_previews(request, descriptor): - """ - Returns a tuple of preview XModule html contents. One preview is returned for each - pair of states returned by get_sample_state() for the supplied descriptor. - - descriptor: An XModuleDescriptor - """ - return tuple(get_preview_html(request, descriptor, idx) - for idx in range(len(descriptor.get_sample_state()))) From 32f76988c6bb3128da5b3c804bc305f8bc7281d0 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Thu, 8 Aug 2013 09:53:19 -0400 Subject: [PATCH 9/9] Update docstring --- cms/djangoapps/contentstore/views/preview.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index a325dd3b34..121bf98393 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -165,7 +165,8 @@ def load_preview_module(request, preview_id, descriptor): def get_preview_html(request, descriptor, idx): """ - Returns the HTML for the XModule specified by the descriptor and idx. + Returns the HTML returned by the XModule's student_view, + specified by the descriptor and idx. """ module = load_preview_module(request, str(idx), descriptor) return module.runtime.render(module, None, "student_view").content