Merge pull request #1772 from edx/db/locator-refactor-textbooks

Refactor textbooks to use locator URLs

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')

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),

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)

cms/static/coffee/spec/main_spec.coffee

-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", ->

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", ->

cms/static/coffee/spec/views/textbook_spec.coffee

-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", ->
@@ -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()
@@ -283,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()

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;
 });

cms/static/js/models/textbook.js

-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)) {

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']
             });

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;
         },

cms/templates/textbooks.html

@@ -23,14 +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) {
-  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 textbooks = new TextbookCollection(${json.dumps(textbooks)}, {parse: true});
   var tbView = new ListTextbooksView({collection: textbooks});
 
   $(function() {

cms/templates/widgets/header.html

@@ -13,13 +13,14 @@
       <h1 class="branding"><a href="/"><img src="${static.url("img/logo-edx-studio.png")}" alt="edX Studio" /></a></h1>
 
       % 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 @@
                     <a href="${assets_url}">${_("Files &amp; Uploads")}</a>
                   </li>
                   <li class="nav-item nav-course-courseware-textbooks">
-                    <a href="${reverse('textbook_index', kwargs=dict(org=ctx_loc.org, course=ctx_loc.course, name=ctx_loc.name))}">${_("Textbooks")}</a>
+                    <a href="${textbooks_url}">${_("Textbooks")}</a>
                   </li>
                 </ul>
               </div>

cms/urls.py

@@ -23,13 +23,6 @@ urlpatterns = patterns('',  # nopep8
     url(r'^preview/xblock/(?P<usage_id>.*?)/handler/(?P<handler>[^/]*)(?:/(?P<suffix>[^/]*))?$',
         'contentstore.views.preview_handler', name='preview_handler'),
 
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/textbooks/(?P<name>[^/]+)$',
-        'contentstore.views.textbook_index', name='textbook_index'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/textbooks/(?P<name>[^/]+)/new$',
-        'contentstore.views.create_textbook', name='create_textbook'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/textbooks/(?P<name>[^/]+)/(?P<tid>\d[^/]*)$',
-        'contentstore.views.textbook_by_id', name='textbook_by_id'),
-
     # temporary landing page for a course
     url(r'^edge/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$',
         '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<grader_index>\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<tid>\d[^/]*)$'.format(parsers.URL_RE_SOURCE), 'textbooks_detail_handler'),
 )
 
 js_info_dict = {