From 0d3490374ef250728f9362d690af78d6970b3816 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 25 Nov 2013 15:06:06 -0500 Subject: [PATCH 1/4] Refactor textbooks to use locator URLs STUD-945 --- .../contentstore/tests/test_contentstore.py | 9 +- .../contentstore/tests/test_textbooks.py | 63 +--- cms/djangoapps/contentstore/tests/utils.py | 3 +- cms/djangoapps/contentstore/views/course.py | 287 +++++++++--------- cms/static/coffee/spec/main_spec.coffee | 2 +- .../coffee/spec/models/textbook_spec.coffee | 17 +- .../coffee/spec/views/textbook_spec.coffee | 2 + cms/static/js/collections/textbook.js | 5 +- cms/static/js/models/textbook.js | 11 +- cms/templates/widgets/header.html | 5 +- cms/urls.py | 9 +- 11 files changed, 174 insertions(+), 239 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 45689f8725..b8dbd378e7 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1659,14 +1659,7 @@ class ContentStoreTest(ModuleStoreTestCase): test_get_html('settings/details') test_get_html('settings/grading') test_get_html('settings/advanced') - - # textbook index - resp = self.client.get_html(reverse('textbook_index', - kwargs={'org': loc.org, - 'course': loc.course, - 'name': loc.name})) - self.assertEqual(resp.status_code, 200) - _test_no_locations(self, resp) + test_get_html('textbooks') # go look at a subsection page subsection_location = loc.replace(category='sequential', name='test_sequence') diff --git a/cms/djangoapps/contentstore/tests/test_textbooks.py b/cms/djangoapps/contentstore/tests/test_textbooks.py index 950d0f780e..d1fe606354 100644 --- a/cms/djangoapps/contentstore/tests/test_textbooks.py +++ b/cms/djangoapps/contentstore/tests/test_textbooks.py @@ -14,11 +14,7 @@ class TextbookIndexTestCase(CourseTestCase): def setUp(self): "Set the URL for tests" super(TextbookIndexTestCase, self).setUp() - self.url = reverse('textbook_index', kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - }) + self.url = self.course_locator.url_reverse('textbooks') def test_view_index(self): "Basic check that the textbook index page responds correctly" @@ -77,13 +73,13 @@ class TextbookIndexTestCase(CourseTestCase): obj = json.loads(resp.content) self.assertEqual(content, obj) - def test_view_index_xhr_post(self): + def test_view_index_xhr_put(self): "Check that you can save information to the server" textbooks = [ {"tab_title": "Hi, mom!"}, {"tab_title": "Textbook 2"}, ] - resp = self.client.post( + resp = self.client.put( self.url, data=json.dumps(textbooks), content_type="application/json", @@ -102,9 +98,9 @@ class TextbookIndexTestCase(CourseTestCase): no_ids.append(textbook) self.assertEqual(no_ids, textbooks) - def test_view_index_xhr_post_invalid(self): + def test_view_index_xhr_put_invalid(self): "Check that you can't save invalid JSON" - resp = self.client.post( + resp = self.client.put( self.url, data="invalid", content_type="application/json", @@ -122,11 +118,7 @@ class TextbookCreateTestCase(CourseTestCase): def setUp(self): "Set up a url and some textbook content for tests" super(TextbookCreateTestCase, self).setUp() - self.url = reverse('create_textbook', kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - }) + self.url = self.course_locator.url_reverse('textbooks') self.textbook = { "tab_title": "Economics", "chapters": { @@ -151,15 +143,6 @@ class TextbookCreateTestCase(CourseTestCase): del textbook["id"] self.assertEqual(self.textbook, textbook) - def test_get(self): - "Test that GET is not allowed" - resp = self.client.get( - self.url, - HTTP_ACCEPT="application/json", - HTTP_X_REQUESTED_WITH="XMLHttpRequest", - ) - self.assertEqual(resp.status_code, 405) - def test_valid_id(self): "Textbook IDs must begin with a number; try a valid one" self.textbook["id"] = "7x5" @@ -188,12 +171,12 @@ class TextbookCreateTestCase(CourseTestCase): self.assertNotIn("Location", resp) -class TextbookByIdTestCase(CourseTestCase): - "Test cases for the `textbook_by_id` view" +class TextbookDetailTestCase(CourseTestCase): + "Test cases for the `textbook_detail_handler` view" def setUp(self): "Set some useful content and URLs for tests" - super(TextbookByIdTestCase, self).setUp() + super(TextbookDetailTestCase, self).setUp() self.textbook1 = { "tab_title": "Economics", "id": 1, @@ -202,12 +185,7 @@ class TextbookByIdTestCase(CourseTestCase): "url": "/a/b/c/ch1.pdf", } } - self.url1 = reverse('textbook_by_id', kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - 'tid': 1, - }) + self.url1 = self.course_locator.url_reverse("textbooks", "1") self.textbook2 = { "tab_title": "Algebra", "id": 2, @@ -216,24 +194,14 @@ class TextbookByIdTestCase(CourseTestCase): "url": "/a/b/ch11.pdf", } } - self.url2 = reverse('textbook_by_id', kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - 'tid': 2, - }) + self.url2 = self.course_locator.url_reverse("textbooks", "2") self.course.pdf_textbooks = [self.textbook1, self.textbook2] # Save the data that we've just changed to the underlying # MongoKeyValueStore before we update the mongo datastore. self.course.save() self.store = get_modulestore(self.course.location) self.store.update_metadata(self.course.location, own_metadata(self.course)) - self.url_nonexist = reverse('textbook_by_id', kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - 'tid': 20, - }) + self.url_nonexist = self.course_locator.url_reverse("textbooks", "20") def test_get_1(self): "Get the first textbook" @@ -275,12 +243,7 @@ class TextbookByIdTestCase(CourseTestCase): "url": "supercool.pdf", "id": "1supercool", } - url = reverse("textbook_by_id", kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - 'tid': "1supercool", - }) + url = self.course_locator.url_reverse("textbooks", "1supercool") resp = self.client.post( url, data=json.dumps(textbook), diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 0e716cc878..2957291f72 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -57,6 +57,7 @@ class AjaxEnabledTestClient(Client): """ return self.get(path, data or {}, follow, HTTP_ACCEPT="application/json", **extra) + @override_settings(MODULESTORE=TEST_MODULESTORE) class CourseTestCase(ModuleStoreTestCase): def setUp(self): @@ -111,7 +112,7 @@ class CourseTestCase(ModuleStoreTestCase): client = Client() client.login(username=uname, password=password) return client, nonstaff - + def populateCourse(self): """ Add 2 chapters, 4 sections, 8 verticals, 16 problems to self.course (branching 2) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 61131fcc11..f5549d1456 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -38,7 +38,7 @@ from models.settings.course_metadata import CourseMetadata from auth.authz import create_all_course_groups, is_user_in_creator_group from util.json_request import expect_json -from .access import has_access, get_location_and_verify_access +from .access import has_access from .tabs import initialize_course_tabs from .component import ( OPEN_ENDED_COMPONENT_TYPES, NOTE_COMPONENT_TYPES, @@ -57,8 +57,20 @@ __all__ = ['course_info_handler', 'course_handler', 'course_info_update_handler' 'settings_handler', 'grading_handler', 'advanced_settings_handler', - 'textbook_index', 'textbook_by_id', - 'create_textbook'] + 'textbooks_list_handler', 'textbooks_detail_handler'] + + +def _get_locator_and_course(course_id, branch, version_guid, usage_id, user, depth=0): + """ + Internal method used to calculate and return the locator and course module + for the view functions in this file. + """ + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=usage_id) + if not has_access(user, locator): + raise PermissionDenied() + course_location = loc_mapper().translate_locator_to_location(locator) + course_module = modulestore().get_item(course_location, depth=depth) + return locator, course_module # pylint: disable=unused-argument @@ -168,17 +180,10 @@ def course_index(request, course_id, branch, version_guid, block): org, course, name: Attributes of the Location for the item to edit """ - location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) - # TODO: when converting to split backend, if location does not have a usage_id, - # we'll need to get the course's root block_id - if not has_access(request.user, location): - raise PermissionDenied() - - old_location = loc_mapper().translate_locator_to_location(location) - - lms_link = get_lms_link_for_item(old_location) - - course = modulestore().get_item(old_location, depth=3) + locator, course = _get_locator_and_course( + course_id, branch, version_guid, block, request.user, depth=3 + ) + lms_link = get_lms_link_for_item(course.location) sections = course.get_children() return render_to_response('overview.html', { @@ -186,9 +191,9 @@ def course_index(request, course_id, branch, version_guid, block): 'lms_link': lms_link, 'sections': sections, 'course_graders': json.dumps( - CourseGradingModel.fetch(location).graders + CourseGradingModel.fetch(locator).graders ), - 'parent_locator': location, + 'parent_locator': locator, 'new_section_category': 'chapter', 'new_subsection_category': 'sequential', 'new_unit_category': 'vertical', @@ -314,22 +319,18 @@ def course_info_handler(request, tag=None, course_id=None, branch=None, version_ GET html: return html for editing the course info handouts and updates. """ - course_location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) - course_old_location = loc_mapper().translate_locator_to_location(course_location) + __, course_module = _get_locator_and_course( + course_id, branch, version_guid, block, request.user + ) if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): - if not has_access(request.user, course_location): - raise PermissionDenied() - - course_module = modulestore().get_item(course_old_location) - - handouts_old_location = course_old_location.replace(category='course_info', name='handouts') + handouts_old_location = course_module.location.replace(category='course_info', name='handouts') handouts_locator = loc_mapper().translate_location( - course_old_location.course_id, handouts_old_location, False, True + course_module.location.course_id, handouts_old_location, False, True ) - update_location = course_old_location.replace(category='course_info', name='updates') + update_location = course_module.location.replace(category='course_info', name='updates') update_locator = loc_mapper().translate_location( - course_old_location.course_id, update_location, False, True + course_module.location.course_id, update_location, False, True ) return render_to_response( @@ -338,7 +339,7 @@ def course_info_handler(request, tag=None, course_id=None, branch=None, version_ 'context_course': course_module, 'updates_url': update_locator.url_reverse('course_info_update/'), 'handouts_locator': handouts_locator, - 'base_asset_url': StaticContent.get_base_url_path_for_course_assets(course_old_location) + '/' + 'base_asset_url': StaticContent.get_base_url_path_for_course_assets(course_module.location) + '/' } ) else: @@ -407,20 +408,16 @@ def settings_handler(request, tag=None, course_id=None, branch=None, version_gui PUT json: update the Course and About xblocks through the CourseDetails model """ - locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) - if not has_access(request.user, locator): - raise PermissionDenied() - + locator, course_module = _get_locator_and_course( + course_id, branch, version_guid, block, request.user + ) if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': - course_old_location = loc_mapper().translate_locator_to_location(locator) - course_module = modulestore().get_item(course_old_location) - upload_asset_url = locator.url_reverse('assets/') return render_to_response('settings.html', { 'context_course': course_module, 'course_locator': locator, - 'lms_link_for_about_page': utils.get_lms_link_for_about_page(course_old_location), + 'lms_link_for_about_page': utils.get_lms_link_for_about_page(course_module.location), 'course_image_url': utils.course_image_url(course_module), 'details_url': locator.url_reverse('/settings/details/'), 'about_page_editable': not settings.FEATURES.get( @@ -457,13 +454,11 @@ def grading_handler(request, tag=None, course_id=None, branch=None, version_guid json no grader_index: update the Course through the CourseGrading model json w/ grader_index: create or update the specific grader (create if index out of range) """ - locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) - if not has_access(request.user, locator): - raise PermissionDenied() + locator, course_module = _get_locator_and_course( + course_id, branch, version_guid, block, request.user + ) if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': - course_old_location = loc_mapper().translate_locator_to_location(locator) - course_module = modulestore().get_item(course_old_location) course_details = CourseGradingModel.fetch(locator) return render_to_response('settings_graders.html', { @@ -514,8 +509,8 @@ def _config_course_advanced_components(request, course_module): filter_tabs = True # Exceptional conditions will pull this to False if ADVANCED_COMPONENT_POLICY_KEY in request.json: # Maps tab types to components tab_component_map = { - 'open_ended':OPEN_ENDED_COMPONENT_TYPES, - 'notes':NOTE_COMPONENT_TYPES, + 'open_ended': OPEN_ENDED_COMPONENT_TYPES, + 'notes': NOTE_COMPONENT_TYPES, } # Check to see if the user instantiated any notes or open ended # components @@ -565,13 +560,9 @@ def advanced_settings_handler(request, course_id=None, branch=None, version_guid metadata dicts. The dict can include a "unsetKeys" entry which is a list of keys whose values to unset: i.e., revert to default """ - locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) - if not has_access(request.user, locator): - raise PermissionDenied() - - course_old_location = loc_mapper().translate_locator_to_location(locator) - course_module = modulestore().get_item(course_old_location) - + locator, course_module = _get_locator_and_course( + course_id, branch, version_guid, block, request.user + ) if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': return render_to_response('settings_advanced.html', { @@ -657,113 +648,109 @@ def assign_textbook_id(textbook, used_ids=()): return tid +@require_http_methods(("GET", "POST", "PUT")) @login_required @ensure_csrf_cookie -def textbook_index(request, org, course, name): +def textbooks_list_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): """ - Display an editable textbook overview. + A RESTful handler for textbook collections. - org, course, name: Attributes of the Location for the item to edit + GET + html: return textbook list page (Backbone application) + json: return JSON representation of all textbooks in this course + POST + json: create a new textbook for this course + PUT + json: overwrite all textbooks in the course with the given list """ - location = get_location_and_verify_access(request, org, course, name) - store = get_modulestore(location) - course_module = store.get_item(location, depth=3) + locator, course = _get_locator_and_course( + course_id, branch, version_guid, block, request.user + ) + store = get_modulestore(course.location) - if request.is_ajax(): - if request.method == 'GET': - return JsonResponse(course_module.pdf_textbooks) - # can be either and sometimes django is rewriting one to the other: - elif request.method in ('POST', 'PUT'): - try: - textbooks = validate_textbooks_json(request.body) - except TextbookValidationError as err: - return JsonResponse({"error": err.message}, status=400) - - tids = set(t["id"] for t in textbooks if "id" in t) - for textbook in textbooks: - if not "id" in textbook: - tid = assign_textbook_id(textbook, tids) - textbook["id"] = tid - tids.add(tid) - - if not any(tab['type'] == 'pdf_textbooks' for tab in course_module.tabs): - course_module.tabs.append({"type": "pdf_textbooks"}) - course_module.pdf_textbooks = textbooks - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course_module.save() - store.update_metadata( - course_module.location, - own_metadata(course_module) - ) - return JsonResponse(course_module.pdf_textbooks) - else: - new_loc = loc_mapper().translate_location(location.course_id, location, False, True) - upload_asset_url = new_loc.url_reverse('assets/', '') - textbook_url = reverse('textbook_index', kwargs={ - 'org': org, - 'course': course, - 'name': name, - }) + if not "application/json" in request.META.get('HTTP_ACCEPT', 'text/html'): + # return HTML page + upload_asset_url = locator.url_reverse('assets/', '') + textbook_url = locator.url_reverse('/textbooks') return render_to_response('textbooks.html', { - 'context_course': course_module, - 'course': course_module, + 'context_course': course, + 'course': course, 'upload_asset_url': upload_asset_url, 'textbook_url': textbook_url, }) + # from here on down, we know the client has requested JSON + if request.method == 'GET': + return JsonResponse(course.pdf_textbooks) + elif request.method == 'PUT': + try: + textbooks = validate_textbooks_json(request.body) + except TextbookValidationError as err: + return JsonResponse({"error": err.message}, status=400) -@require_POST -@login_required -@ensure_csrf_cookie -def create_textbook(request, org, course, name): - """ - JSON API endpoint for creating a textbook. Used by the Backbone application. - """ - location = get_location_and_verify_access(request, org, course, name) - store = get_modulestore(location) - course_module = store.get_item(location, depth=0) + tids = set(t["id"] for t in textbooks if "id" in t) + for textbook in textbooks: + if not "id" in textbook: + tid = assign_textbook_id(textbook, tids) + textbook["id"] = tid + tids.add(tid) - try: - textbook = validate_textbook_json(request.body) - except TextbookValidationError as err: - return JsonResponse({"error": err.message}, status=400) - if not textbook.get("id"): - tids = set(t["id"] for t in course_module.pdf_textbooks if "id" in t) - textbook["id"] = assign_textbook_id(textbook, tids) - existing = course_module.pdf_textbooks - existing.append(textbook) - course_module.pdf_textbooks = existing - if not any(tab['type'] == 'pdf_textbooks' for tab in course_module.tabs): - tabs = course_module.tabs - tabs.append({"type": "pdf_textbooks"}) - course_module.tabs = tabs - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course_module.save() - store.update_metadata(course_module.location, own_metadata(course_module)) - resp = JsonResponse(textbook, status=201) - resp["Location"] = reverse("textbook_by_id", kwargs={ - 'org': org, - 'course': course, - 'name': name, - 'tid': textbook["id"], - }) - return resp + if not any(tab['type'] == 'pdf_textbooks' for tab in course.tabs): + course.tabs.append({"type": "pdf_textbooks"}) + course.pdf_textbooks = textbooks + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course.save() + store.update_metadata( + course.location, + own_metadata(course) + ) + return JsonResponse(course.pdf_textbooks) + elif request.method == 'POST': + # create a new textbook for the course + try: + textbook = validate_textbook_json(request.body) + except TextbookValidationError as err: + return JsonResponse({"error": err.message}, status=400) + if not textbook.get("id"): + tids = set(t["id"] for t in course.pdf_textbooks if "id" in t) + textbook["id"] = assign_textbook_id(textbook, tids) + existing = course.pdf_textbooks + existing.append(textbook) + course.pdf_textbooks = existing + if not any(tab['type'] == 'pdf_textbooks' for tab in course.tabs): + tabs = course.tabs + tabs.append({"type": "pdf_textbooks"}) + course.tabs = tabs + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course.save() + store.update_metadata(course.location, own_metadata(course)) + resp = JsonResponse(textbook, status=201) + resp["Location"] = locator.url_reverse('textbooks', textbook["id"]) + return resp @login_required @ensure_csrf_cookie @require_http_methods(("GET", "POST", "PUT", "DELETE")) -def textbook_by_id(request, org, course, name, tid): +def textbooks_detail_handler(request, tid, tag=None, course_id=None, branch=None, version_guid=None, block=None): """ JSON API endpoint for manipulating a textbook via its internal ID. Used by the Backbone application. + + GET + json: return JSON representation of textbook + POST or PUT + json: update textbook based on provided information + DELETE + json: remove textbook """ - location = get_location_and_verify_access(request, org, course, name) - store = get_modulestore(location) - course_module = store.get_item(location, depth=3) - matching_id = [tb for tb in course_module.pdf_textbooks + __, course = _get_locator_and_course( + course_id, branch, version_guid, block, request.user + ) + store = get_modulestore(course.location) + matching_id = [tb for tb in course.pdf_textbooks if str(tb.get("id")) == str(tid)] if matching_id: textbook = matching_id[0] @@ -782,32 +769,32 @@ def textbook_by_id(request, org, course, name, tid): return JsonResponse({"error": err.message}, status=400) new_textbook["id"] = tid if textbook: - i = course_module.pdf_textbooks.index(textbook) - new_textbooks = course_module.pdf_textbooks[0:i] + i = course.pdf_textbooks.index(textbook) + new_textbooks = course.pdf_textbooks[0:i] new_textbooks.append(new_textbook) - new_textbooks.extend(course_module.pdf_textbooks[i + 1:]) - course_module.pdf_textbooks = new_textbooks + new_textbooks.extend(course.pdf_textbooks[i + 1:]) + course.pdf_textbooks = new_textbooks else: - course_module.pdf_textbooks.append(new_textbook) + course.pdf_textbooks.append(new_textbook) # Save the data that we've just changed to the underlying # MongoKeyValueStore before we update the mongo datastore. - course_module.save() + course.save() store.update_metadata( - course_module.location, - own_metadata(course_module) + course.location, + own_metadata(course) ) return JsonResponse(new_textbook, status=201) elif request.method == 'DELETE': if not textbook: return JsonResponse(status=404) - i = course_module.pdf_textbooks.index(textbook) - new_textbooks = course_module.pdf_textbooks[0:i] - new_textbooks.extend(course_module.pdf_textbooks[i + 1:]) - course_module.pdf_textbooks = new_textbooks - course_module.save() + i = course.pdf_textbooks.index(textbook) + new_textbooks = course.pdf_textbooks[0:i] + new_textbooks.extend(course.pdf_textbooks[i + 1:]) + course.pdf_textbooks = new_textbooks + course.save() store.update_metadata( - course_module.location, - own_metadata(course_module) + course.location, + own_metadata(course) ) return JsonResponse() diff --git a/cms/static/coffee/spec/main_spec.coffee b/cms/static/coffee/spec/main_spec.coffee index 6811b3d96a..6c01d4b2d0 100644 --- a/cms/static/coffee/spec/main_spec.coffee +++ b/cms/static/coffee/spec/main_spec.coffee @@ -1,4 +1,4 @@ -require ["jquery", "backbone", "coffee/src/main", "sinon", "jasmine-stealth"], +require ["jquery", "backbone", "coffee/src/main", "sinon", "jasmine-stealth", "jquery.cookie"], ($, Backbone, main, sinon) -> describe "CMS", -> it "should initialize URL", -> diff --git a/cms/static/coffee/spec/models/textbook_spec.coffee b/cms/static/coffee/spec/models/textbook_spec.coffee index 0899641cc3..fa4f86742c 100644 --- a/cms/static/coffee/spec/models/textbook_spec.coffee +++ b/cms/static/coffee/spec/models/textbook_spec.coffee @@ -11,6 +11,10 @@ define ["backbone", "js/models/textbook", "js/collections/textbook", "js/models/ beforeEach -> main() @model = new Textbook() + CMS.URL.TEXTBOOKS = "/textbooks" + + afterEach -> + delete CMS.URL.TEXTBOOKS describe "Basic", -> it "should have an empty name by default", -> @@ -28,8 +32,9 @@ define ["backbone", "js/models/textbook", "js/collections/textbook", "js/models/ it "should be empty by default", -> expect(@model.isEmpty()).toBeTruthy() - it "should have a URL set", -> - expect(@model.url()).toBeTruthy() + it "should have a URL root", -> + urlRoot = _.result(@model, 'urlRoot') + expect(urlRoot).toBeTruthy() it "should be able to reset itself", -> @model.set("name", "foobar") @@ -135,12 +140,8 @@ define ["backbone", "js/models/textbook", "js/collections/textbook", "js/models/ delete CMS.URL.TEXTBOOKS it "should have a url set", -> - expect(@collection.url()).toEqual("/textbooks") - - it "can call save", -> - spyOn(@collection, "sync") - @collection.save() - expect(@collection.sync).toHaveBeenCalledWith("update", @collection, undefined) + url = _.result(@collection, 'url') + expect(url).toEqual("/textbooks") describe "Chapter model", -> diff --git a/cms/static/coffee/spec/views/textbook_spec.coffee b/cms/static/coffee/spec/views/textbook_spec.coffee index 15c2ff2782..e76736b155 100644 --- a/cms/static/coffee/spec/views/textbook_spec.coffee +++ b/cms/static/coffee/spec/views/textbook_spec.coffee @@ -81,9 +81,11 @@ define ["js/models/textbook", "js/models/chapter", "js/collections/chapter", "js @savingSpies = spyOnConstructor(Notification, "Mini", ["show", "hide"]) @savingSpies.show.andReturn(@savingSpies) + CMS.URL.TEXTBOOKS = "/textbooks" afterEach -> @xhr.restore() + delete CMS.URL.TEXTBOOKS it "should destroy itself on confirmation", -> @view.render().$(".delete").click() diff --git a/cms/static/js/collections/textbook.js b/cms/static/js/collections/textbook.js index ce06891739..81674cfb04 100644 --- a/cms/static/js/collections/textbook.js +++ b/cms/static/js/collections/textbook.js @@ -2,10 +2,7 @@ define(["backbone", "js/models/textbook"], function(Backbone, TextbookModel) { var TextbookCollection = Backbone.Collection.extend({ model: TextbookModel, - url: function() { return CMS.URL.TEXTBOOKS; }, - save: function(options) { - return this.sync('update', this, options); - } + url: function() { return CMS.URL.TEXTBOOKS; } }); return TextbookCollection; }); diff --git a/cms/static/js/models/textbook.js b/cms/static/js/models/textbook.js index 2042d6d4dc..9412c809d7 100644 --- a/cms/static/js/models/textbook.js +++ b/cms/static/js/models/textbook.js @@ -1,4 +1,5 @@ -define(["backbone", "underscore", "js/models/chapter", "js/collections/chapter", "backbone.associations"], +define(["backbone", "underscore", "js/models/chapter", "js/collections/chapter", + "backbone.associations", "coffee/src/main"], function(Backbone, _, ChapterModel, ChapterCollection) { var Textbook = Backbone.AssociatedModel.extend({ @@ -32,13 +33,7 @@ define(["backbone", "underscore", "js/models/chapter", "js/collections/chapter", isEmpty: function() { return !this.get('name') && this.get('chapters').isEmpty(); }, - url: function() { - if(this.isNew()) { - return CMS.URL.TEXTBOOKS + "/new"; - } else { - return CMS.URL.TEXTBOOKS + "/" + this.id; - } - }, + urlRoot: function() { return CMS.URL.TEXTBOOKS; }, parse: function(response) { var ret = $.extend(true, {}, response); if("tab_title" in ret && !("name" in ret)) { diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index b5d6c80d9c..ac9067e6be 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -13,13 +13,14 @@

edX Studio

% if context_course: - <% + <% ctx_loc = context_course.location location = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True) index_url = location.url_reverse('course') checklists_url = location.url_reverse('checklists') course_team_url = location.url_reverse('course_team') assets_url = location.url_reverse('assets') + textbooks_url = location.url_reverse('textbooks') import_url = location.url_reverse('import') course_info_url = location.url_reverse('course_info') export_url = location.url_reverse('export') @@ -58,7 +59,7 @@ ${_("Files & Uploads")}
  • - ${_("Textbooks")} + ${_("Textbooks")}
    • diff --git a/cms/urls.py b/cms/urls.py index 07cd0d4bbb..28d526ca87 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -23,13 +23,6 @@ urlpatterns = patterns('', # nopep8 url(r'^preview/xblock/(?P.*?)/handler/(?P[^/]*)(?:/(?P[^/]*))?$', 'contentstore.views.preview_handler', name='preview_handler'), - url(r'^(?P[^/]+)/(?P[^/]+)/textbooks/(?P[^/]+)$', - 'contentstore.views.textbook_index', name='textbook_index'), - url(r'^(?P[^/]+)/(?P[^/]+)/textbooks/(?P[^/]+)/new$', - 'contentstore.views.create_textbook', name='create_textbook'), - url(r'^(?P[^/]+)/(?P[^/]+)/textbooks/(?P[^/]+)/(?P\d[^/]*)$', - 'contentstore.views.textbook_by_id', name='textbook_by_id'), - # temporary landing page for a course url(r'^edge/(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)$', 'contentstore.views.landing', name='landing'), @@ -89,6 +82,8 @@ urlpatterns += patterns( url(r'(?ix)^settings/details/{}$'.format(parsers.URL_RE_SOURCE), 'settings_handler'), url(r'(?ix)^settings/grading/{}(/)?(?P\d+)?$'.format(parsers.URL_RE_SOURCE), 'grading_handler'), url(r'(?ix)^settings/advanced/{}$'.format(parsers.URL_RE_SOURCE), 'advanced_settings_handler'), + url(r'(?ix)^textbooks/{}$'.format(parsers.URL_RE_SOURCE), 'textbooks_list_handler'), + url(r'(?ix)^textbooks/{}/(?P\d[^/]*)$'.format(parsers.URL_RE_SOURCE), 'textbooks_detail_handler'), ) js_info_dict = { From 06f400191f463936a620f2fdeb69681260cb5fd0 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Thu, 5 Dec 2013 13:17:39 -0500 Subject: [PATCH 2/4] Use window.course instead of window.section --- cms/static/coffee/spec/views/textbook_spec.coffee | 12 ++++++------ cms/static/js/views/edit_chapter.js | 2 +- cms/static/js/views/show_textbook.js | 2 +- cms/templates/textbooks.html | 7 ------- 4 files changed, 8 insertions(+), 15 deletions(-) diff --git a/cms/static/coffee/spec/views/textbook_spec.coffee b/cms/static/coffee/spec/views/textbook_spec.coffee index e76736b155..50fbd71c3f 100644 --- a/cms/static/coffee/spec/views/textbook_spec.coffee +++ b/cms/static/coffee/spec/views/textbook_spec.coffee @@ -1,8 +1,8 @@ -define ["js/models/textbook", "js/models/chapter", "js/collections/chapter", "js/models/section", +define ["js/models/textbook", "js/models/chapter", "js/collections/chapter", "js/models/course", "js/collections/textbook", "js/views/show_textbook", "js/views/edit_textbook", "js/views/list_textbooks", "js/views/edit_chapter", "js/views/feedback_prompt", "js/views/feedback_notification", "sinon", "jasmine-stealth"], -(Textbook, Chapter, ChapterSet, Section, TextbookSet, ShowTextbook, EditTextbook, ListTexbook, EditChapter, Prompt, Notification, sinon) -> +(Textbook, Chapter, ChapterSet, Course, TextbookSet, ShowTextbook, EditTextbook, ListTexbook, EditChapter, Prompt, Notification, sinon) -> feedbackTpl = readFixtures('system-feedback.underscore') beforeEach -> @@ -30,7 +30,7 @@ define ["js/models/textbook", "js/models/chapter", "js/collections/chapter", "js @promptSpies = spyOnConstructor(Prompt, "Warning", ["show", "hide"]) @promptSpies.show.andReturn(@promptSpies) - window.section = new Section({ + window.course = new Course({ id: "5", name: "Course Name", url_name: "course_name", @@ -40,7 +40,7 @@ define ["js/models/textbook", "js/models/chapter", "js/collections/chapter", "js }); afterEach -> - delete window.section + delete window.course describe "Basic", -> it "should render properly", -> @@ -285,11 +285,11 @@ define ["js/models/textbook", "js/models/chapter", "js/collections/chapter", "js @view = new EditChapter({model: @model}) spyOn(@view, "remove").andCallThrough() CMS.URL.UPLOAD_ASSET = "/upload" - window.section = new Section({name: "abcde"}) + window.course = new Course({name: "abcde"}) afterEach -> delete CMS.URL.UPLOAD_ASSET - delete window.section + delete window.course it "can render", -> @view.render() diff --git a/cms/static/js/views/edit_chapter.js b/cms/static/js/views/edit_chapter.js index 64169304e3..af6c5c61dd 100644 --- a/cms/static/js/views/edit_chapter.js +++ b/cms/static/js/views/edit_chapter.js @@ -53,7 +53,7 @@ define(["backbone", "underscore", "underscore.string", "jquery", "gettext", "js/ }); var msg = new FileUploadModel({ title: _.template(gettext("Upload a new PDF to “<%= name %>”"), - {name: section.escape('name')}), + {name: course.escape('name')}), message: "Files must be in PDF format.", mimeTypes: ['application/pdf'] }); diff --git a/cms/static/js/views/show_textbook.js b/cms/static/js/views/show_textbook.js index 7bae395fd5..580f4e9840 100644 --- a/cms/static/js/views/show_textbook.js +++ b/cms/static/js/views/show_textbook.js @@ -16,7 +16,7 @@ define(["backbone", "underscore", "gettext", "js/views/feedback_notification", " render: function() { var attrs = $.extend({}, this.model.attributes); attrs.bookindex = this.model.collection.indexOf(this.model); - attrs.course = window.section.attributes; + attrs.course = window.course.attributes; this.$el.html(this.template(attrs)); return this; }, diff --git a/cms/templates/textbooks.html b/cms/templates/textbooks.html index 238834da22..622f712d0b 100644 --- a/cms/templates/textbooks.html +++ b/cms/templates/textbooks.html @@ -23,13 +23,6 @@ CMS.URL.TEXTBOOKS = "${textbook_url}" CMS.URL.LMS_BASE = "${settings.LMS_BASE}" require(["js/models/section", "js/collections/textbook", "js/views/list_textbooks"], function(Section, TextbookCollection, ListTextbooksView) { - window.section = new Section({ - name: "${course.display_name_with_default | h}", - url_name: "${course.location.name | h}", - org: "${course.location.org | h}", - num: "${course.location.course | h}", - revision: "${course.location.revision | h}" - }); var textbooks = new TextbookCollection(${json.dumps(course.pdf_textbooks)}, {parse: true}); var tbView = new ListTextbooksView({collection: textbooks}); From c93198235ef92037bde0d59a86de36dbbdd6a2bd Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Thu, 5 Dec 2013 13:19:00 -0500 Subject: [PATCH 3/4] Pass PDF textbooks to template separately --- cms/djangoapps/contentstore/views/course.py | 2 +- cms/templates/textbooks.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index f5549d1456..163bf7c90d 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -674,7 +674,7 @@ def textbooks_list_handler(request, tag=None, course_id=None, branch=None, versi textbook_url = locator.url_reverse('/textbooks') return render_to_response('textbooks.html', { 'context_course': course, - 'course': course, + 'textbooks': course.pdf_textbooks, 'upload_asset_url': upload_asset_url, 'textbook_url': textbook_url, }) diff --git a/cms/templates/textbooks.html b/cms/templates/textbooks.html index 622f712d0b..a8d73319ec 100644 --- a/cms/templates/textbooks.html +++ b/cms/templates/textbooks.html @@ -23,7 +23,7 @@ CMS.URL.TEXTBOOKS = "${textbook_url}" CMS.URL.LMS_BASE = "${settings.LMS_BASE}" require(["js/models/section", "js/collections/textbook", "js/views/list_textbooks"], function(Section, TextbookCollection, ListTextbooksView) { - var textbooks = new TextbookCollection(${json.dumps(course.pdf_textbooks)}, {parse: true}); + var textbooks = new TextbookCollection(${json.dumps(textbooks)}, {parse: true}); var tbView = new ListTextbooksView({collection: textbooks}); $(function() { From d8103d437713b4996e9798cd2ffdf88042abcd14 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Thu, 5 Dec 2013 13:21:55 -0500 Subject: [PATCH 4/4] course.save() is no longer necessary --- cms/djangoapps/contentstore/views/course.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 163bf7c90d..888bfcc061 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -698,9 +698,6 @@ def textbooks_list_handler(request, tag=None, course_id=None, branch=None, versi if not any(tab['type'] == 'pdf_textbooks' for tab in course.tabs): course.tabs.append({"type": "pdf_textbooks"}) course.pdf_textbooks = textbooks - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course.save() store.update_metadata( course.location, own_metadata(course) @@ -722,9 +719,6 @@ def textbooks_list_handler(request, tag=None, course_id=None, branch=None, versi tabs = course.tabs tabs.append({"type": "pdf_textbooks"}) course.tabs = tabs - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course.save() store.update_metadata(course.location, own_metadata(course)) resp = JsonResponse(textbook, status=201) resp["Location"] = locator.url_reverse('textbooks', textbook["id"]) @@ -776,9 +770,6 @@ def textbooks_detail_handler(request, tid, tag=None, course_id=None, branch=None course.pdf_textbooks = new_textbooks else: course.pdf_textbooks.append(new_textbook) - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course.save() store.update_metadata( course.location, own_metadata(course) @@ -791,7 +782,6 @@ def textbooks_detail_handler(request, tid, tag=None, course_id=None, branch=None new_textbooks = course.pdf_textbooks[0:i] new_textbooks.extend(course.pdf_textbooks[i + 1:]) course.pdf_textbooks = new_textbooks - course.save() store.update_metadata( course.location, own_metadata(course)