Merge pull request #3371 from carsongee/cg/pdf_textbook_urls

Switch PDF Textbooks to use portable URLs

cms/djangoapps/contentstore/features/textbooks.feature

@@ -19,9 +19,9 @@ Feature: CMS.Textbooks
     And I upload the textbook "textbook.pdf"
     And I wait for "2" seconds
     And I save the textbook
-    Then I should see a textbook named "Economics" with a chapter path containing "/c4x/MITx/999/asset/textbook.pdf"
+    Then I should see a textbook named "Economics" with a chapter path containing "/static/textbook.pdf"
     And I reload the page
-    Then I should see a textbook named "Economics" with a chapter path containing "/c4x/MITx/999/asset/textbook.pdf"
+    Then I should see a textbook named "Economics" with a chapter path containing "/static/textbook.pdf"
 
   Scenario: Create a textbook with multiple chapters
     Given I have opened a new course in Studio

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -1809,10 +1809,10 @@ class ContentStoreTest(ModuleStoreTestCase):
         # first check PDF textbooks, to make sure the url paths got updated
         course_module = module_store.get_instance(target_course_id, target_location)
 
-        self.assertEquals(len(course_module.pdf_textbooks), 1)
-        self.assertEquals(len(course_module.pdf_textbooks[0]["chapters"]), 2)
-        self.assertEquals(course_module.pdf_textbooks[0]["chapters"][0]["url"], '/c4x/MITx/999/asset/Chapter1.pdf')
-        self.assertEquals(course_module.pdf_textbooks[0]["chapters"][1]["url"], '/c4x/MITx/999/asset/Chapter2.pdf')
+        self.assertEqual(len(course_module.pdf_textbooks), 1)
+        self.assertEqual(len(course_module.pdf_textbooks[0]["chapters"]), 2)
+        self.assertEqual(course_module.pdf_textbooks[0]["chapters"][0]["url"], '/static/Chapter1.pdf')
+        self.assertEqual(course_module.pdf_textbooks[0]["chapters"][1]["url"], '/static/Chapter2.pdf')
 
     def test_import_into_new_course_id_wiki_slug_renamespacing(self):
         module_store = modulestore('direct')

cms/static/js/views/edit_chapter.js

@@ -65,7 +65,7 @@ define(["js/views/baseview", "underscore", "underscore.string", "jquery", "gette
                     if(!that.model.get('name')) {
                         options.name = response.asset.displayname;
                     }
-                    options.asset_path = response.asset.url;
+                    options.asset_path = response.asset.portable_url;
                     that.model.set(options);
                 }
             });

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -517,12 +517,13 @@ def remap_namespace(module, target_location_namespace):
         # There is more re-namespacing work we have to do when
         # importing course modules
 
-        # remap pdf_textbook urls
+        # remap pdf_textbook urls to portable static URLs
         for entry in module.pdf_textbooks:
             for chapter in entry.get('chapters', []):
                 if StaticContent.is_c4x_path(chapter.get('url', '')):
-                    chapter['url'] = StaticContent.renamespace_c4x_path(
-                        chapter['url'], target_location_namespace
+                    chapter_loc = StaticContent.get_location_from_path(chapter['url'])
+                    chapter['url'] = StaticContent.get_static_path_from_location(
+                        chapter_loc
                     )
 
         # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'.

lms/djangoapps/staticbook/tests.py

@@ -22,8 +22,17 @@ PDF_BOOK = {
     "tab_title": "Textbook",
     "title": "A PDF Textbook",
     "chapters": [
-        { "title": "Chapter 1 for PDF", "url": "https://somehost.com/the_book/chap1.pdf" },
-        { "title": "Chapter 2 for PDF", "url": "https://somehost.com/the_book/chap2.pdf" },
+        {"title": "Chapter 1 for PDF", "url": "https://somehost.com/the_book/chap1.pdf"},
+        {"title": "Chapter 2 for PDF", "url": "https://somehost.com/the_book/chap2.pdf"},
+    ],
+}
+
+PORTABLE_PDF_BOOK = {
+    "tab_title": "Textbook",
+    "title": "A PDF Textbook",
+    "chapters": [
+        {"title": "Chapter 1 for PDF", "url": "/static/chap1.pdf"},
+        {"title": "Chapter 2 for PDF", "url": "/static/chap2.pdf"},
     ],
 }
 
@@ -31,8 +40,8 @@ HTML_BOOK = {
     "tab_title": "Textbook",
     "title": "An HTML Textbook",
     "chapters": [
-        { "title": "Chapter 1 for HTML", "url": "https://somehost.com/the_book/chap1.html" },
-        { "title": "Chapter 2 for HTML", "url": "https://somehost.com/the_book/chap2.html" },
+        {"title": "Chapter 1 for HTML", "url": "https://somehost.com/the_book/chap1.html"},
+        {"title": "Chapter 2 for HTML", "url": "https://somehost.com/the_book/chap2.html"},
     ],
 }
 
@@ -194,6 +203,33 @@ class StaticPdfBookTest(StaticBookTest):
         with self.assertRaises(NoReverseMatch):
             self.make_url('pdf_book', book_index=0, chapter='fooey', page='xyzzy')
 
+    def test_static_url_map_contentstore(self):
+        """
+        This ensure static  URL mapping is happening properly for
+        a course that uses the contentstore
+        """
+        self.make_course(pdf_textbooks=[PORTABLE_PDF_BOOK])
+        url = self.make_url('pdf_book', book_index=0, chapter=1)
+        response = self.client.get(url)
+        self.assertNotContains(response, 'file={}'.format(PORTABLE_PDF_BOOK['chapters'][0]['url']))
+        self.assertContains(response, 'file=/c4x/{0.org}/{0.course}/asset/{1}'.format(
+            self.course.location,
+            PORTABLE_PDF_BOOK['chapters'][0]['url'].replace('/static/', '')))
+
+    def test_static_url_map_static_asset_path(self):
+        """
+        Like above, but used when the course has set a static_asset_path
+        """
+        self.make_course(pdf_textbooks=[PORTABLE_PDF_BOOK], static_asset_path='awesomesauce')
+        url = self.make_url('pdf_book', book_index=0, chapter=1)
+        response = self.client.get(url)
+        self.assertNotContains(response, 'file={}'.format(PORTABLE_PDF_BOOK['chapters'][0]['url']))
+        self.assertNotContains(response, 'file=/c4x/{0.org}/{0.course}/asset/{1}'.format(
+            self.course.location,
+            PORTABLE_PDF_BOOK['chapters'][0]['url'].replace('/static/', '')))
+        self.assertContains(response, 'file=/static/awesomesauce/{}'.format(
+            PORTABLE_PDF_BOOK['chapters'][0]['url'].replace('/static/', '')))
+
 
 class StaticHtmlBookTest(StaticBookTest):
     """

lms/djangoapps/staticbook/views.py

@@ -51,6 +51,7 @@ def remap_static_url(original_url, course):
         input_url,
         getattr(course, 'data_dir', None),
         course_id=course.location.course_id,
+        static_asset_path=course.static_asset_path
     )
     # strip off the quotes again...
     return output_url[1:-1]