Merge pull request #2238 from zubair-arbi/zub/bugfix/std154-courseinfohtml

update course info module to save content without modifying

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -679,6 +679,62 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         return content_store, trash_store, thumbnail_location
 
+    def test_course_info_updates_import_export(self):
+        """
+        Test that course info updates are imported and exported with all content fields ('data', 'items')
+        """
+        content_store = contentstore()
+        module_store = modulestore('direct')
+        data_dir = "common/test/data/"
+        import_from_xml(module_store, data_dir, ['course_info_updates'],
+                        static_content_store=content_store, verbose=True)
+
+        course_location = CourseDescriptor.id_to_location('edX/course_info_updates/2014_T1')
+        course = module_store.get_item(course_location)
+
+        self.assertIsNotNone(course)
+
+        course_updates = module_store.get_item(
+            Location(['i4x', 'edX', 'course_info_updates', 'course_info', 'updates', None]))
+
+        self.assertIsNotNone(course_updates)
+
+         # check that course which is imported has files 'updates.html' and 'updates.items.json'
+        filesystem = OSFS(data_dir + 'course_info_updates/info')
+        self.assertTrue(filesystem.exists('updates.html'))
+        self.assertTrue(filesystem.exists('updates.items.json'))
+
+        # verify that course info update module has same data content as in data file from which it is imported
+        # check 'data' field content
+        with filesystem.open('updates.html', 'r') as course_policy:
+            on_disk = course_policy.read()
+            self.assertEqual(course_updates.data, on_disk)
+
+        # check 'items' field content
+        with filesystem.open('updates.items.json', 'r') as course_policy:
+            on_disk = loads(course_policy.read())
+            self.assertEqual(course_updates.items, on_disk)
+
+        # now export the course to a tempdir and test that it contains files 'updates.html' and 'updates.items.json'
+        # with same content as in course 'info' directory
+        root_dir = path(mkdtemp_clean())
+        print 'Exporting to tempdir = {0}'.format(root_dir)
+        export_to_xml(module_store, content_store, course_location, root_dir, 'test_export')
+
+        # check that exported course has files 'updates.html' and 'updates.items.json'
+        filesystem = OSFS(root_dir / 'test_export/info')
+        self.assertTrue(filesystem.exists('updates.html'))
+        self.assertTrue(filesystem.exists('updates.items.json'))
+
+        # verify that exported course has same data content as in course_info_update module
+        with filesystem.open('updates.html', 'r') as grading_policy:
+            on_disk = grading_policy.read()
+            self.assertEqual(on_disk, course_updates.data)
+
+        with filesystem.open('updates.items.json', 'r') as grading_policy:
+            on_disk = loads(grading_policy.read())
+            self.assertEqual(on_disk, course_updates.items)
+
     def test_empty_trashcan(self):
         '''
         This test will exercise the emptying of the asset trashcan

cms/djangoapps/contentstore/views/course.py

@@ -490,7 +490,11 @@ def course_info_update_handler(request, tag=None, package_id=None, branch=None,
         raise PermissionDenied()
 
     if request.method == 'GET':
-        return JsonResponse(get_course_updates(updates_location, provided_id))
+        course_updates = get_course_updates(updates_location, provided_id)
+        if isinstance(course_updates, dict) and course_updates.get('error'):
+            return JsonResponse(get_course_updates(updates_location, provided_id), course_updates.get('status', 400))
+        else:
+            return JsonResponse(get_course_updates(updates_location, provided_id))
     elif request.method == 'DELETE':
         try:
             return JsonResponse(delete_course_update(updates_location, request.json, provided_id, request.user))

cms/djangoapps/contentstore/views/tests/test_course_updates.py

@@ -120,6 +120,86 @@ class CourseUpdateTest(CourseTestCase):
         payload = json.loads(resp.content)
         self.assertTrue(len(payload) == before_delete - 1)
 
+    def test_course_updates_compatibility(self):
+        '''
+        Test that course updates doesn't break on old data (content in 'data' field).
+        Note: new data will save as list in 'items' field.
+        '''
+        # get the updates and populate 'data' field with some data.
+        location = self.course.location.replace(category='course_info', name='updates')
+        modulestore('direct').create_and_save_xmodule(location)
+        course_updates = modulestore('direct').get_item(location)
+        update_date = u"January 23, 2014"
+        update_content = u"Hello world!"
+        update_data = u"<ol><li><h2>" + update_date + "</h2>" + update_content + "</li></ol>"
+        course_updates.data = update_data
+        modulestore('direct').update_item(course_updates, self.user)
+
+        update_locator = loc_mapper().translate_location(
+            self.course.location.course_id, location, False, True
+        )
+        # test getting all updates list
+        course_update_url = update_locator.url_reverse('course_info_update/')
+        resp = self.client.get_json(course_update_url)
+        payload = json.loads(resp.content)
+        self.assertEqual(payload, [{u'date': update_date, u'content': update_content, u'id': 1}])
+        self.assertTrue(len(payload) == 1)
+
+        # test getting single update item
+        first_update_url = update_locator.url_reverse('course_info_update', str(payload[0]['id']))
+        resp = self.client.get_json(first_update_url)
+        payload = json.loads(resp.content)
+        self.assertEqual(payload, {u'date': u'January 23, 2014', u'content': u'Hello world!', u'id': 1})
+        self.assertHTMLEqual(update_date, payload['date'])
+        self.assertHTMLEqual(update_content, payload['content'])
+
+        # test that while updating it converts old data (with string format in 'data' field)
+        # to new data (with list format in 'items' field) and respectively updates 'data' field.
+        course_updates = modulestore('direct').get_item(location)
+        self.assertEqual(course_updates.items, [])
+        # now try to update first update item
+        update_content = 'Testing'
+        payload = {'content': update_content, 'date': update_date}
+        resp = self.client.ajax_post(
+            course_update_url + '/1', payload, HTTP_X_HTTP_METHOD_OVERRIDE="PUT", REQUEST_METHOD="POST"
+        )
+        self.assertHTMLEqual(update_content, json.loads(resp.content)['content'])
+        course_updates = modulestore('direct').get_item(location)
+        self.assertEqual(course_updates.items, [{u'date': update_date, u'content': update_content, u'id': 1}])
+        # course_updates 'data' field should update accordingly
+        update_data = u"<ol><li><h2>" + update_date + "</h2>" + update_content + "</li></ol>"
+        self.assertEqual(course_updates.data, update_data)
+
+        # test delete course update item (soft delete)
+        course_updates = modulestore('direct').get_item(location)
+        self.assertEqual(course_updates.items, [{u'date': update_date, u'content': update_content, u'id': 1}])
+        # now try to delete first update item
+        resp = self.client.delete(course_update_url + '/1')
+        self.assertEqual(json.loads(resp.content), [])
+        # confirm that course update is soft deleted ('status' flag set to 'deleted') in db
+        course_updates = modulestore('direct').get_item(location)
+        self.assertEqual(course_updates.items,
+                         [{u'date': update_date, u'content': update_content, u'id': 1, u'status': 'deleted'}])
+
+        # now try to get deleted update
+        resp = self.client.get_json(course_update_url + '/1')
+        payload = json.loads(resp.content)
+        self.assertEqual(payload.get('error'), u"Course update not found.")
+        self.assertEqual(resp.status_code, 404)
+
+        # now check that course update don't munges html
+        update_content = u"""&lt;problem>
+                           &lt;p>&lt;/p>
+                           &lt;multiplechoiceresponse>
+                           <pre>&lt;problem>
+                               &lt;p>&lt;/p></pre>
+                           <div><foo>bar</foo></div>"""
+        payload = {'content': update_content, 'date': update_date}
+        resp = self.client.ajax_post(
+            course_update_url, payload, REQUEST_METHOD="POST"
+        )
+        self.assertHTMLEqual(update_content, json.loads(resp.content)['content'])
+
     def test_no_ol_course_update(self):
         '''Test trying to add to a saved course_update which is not an ol.'''
         # get the updates and set to something wrong
@@ -143,10 +223,10 @@ class CourseUpdateTest(CourseTestCase):
 
         self.assertHTMLEqual(payload['content'], content)
 
-        # now confirm that the bad news and the iframe make up 2 updates
+        # now confirm that the bad news and the iframe make up single update
         resp = self.client.get_json(course_update_url)
         payload = json.loads(resp.content)
-        self.assertTrue(len(payload) == 2)
+        self.assertTrue(len(payload) == 1)
 
     def test_post_course_update(self):
         """

common/lib/xmodule/xmodule/html_module.py

@@ -7,7 +7,7 @@ from lxml import etree
 from path import path
 
 from pkg_resources import resource_string
-from xblock.fields import Scope, String, Boolean
+from xblock.fields import Scope, String, Boolean, List
 from xmodule.editing_module import EditingDescriptor
 from xmodule.html_checker import check_html
 from xmodule.stringify import stringify_children
@@ -293,6 +293,11 @@ class CourseInfoFields(object):
     """
     Field overrides
     """
+    items = List(
+        help="List of course update items",
+        default=[],
+        scope=Scope.content
+    )
     data = String(
         help="Html contents to display for this module",
         default="<ol></ol>",
@@ -305,7 +310,9 @@ class CourseInfoModule(CourseInfoFields, HtmlModule):
     """
     Just to support xblock field overrides
     """
-    pass
+    # statuses
+    STATUS_VISIBLE = 'visible'
+    STATUS_DELETED = 'deleted'
 
 
 @XBlock.tag("detached")

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -19,6 +19,7 @@ from xmodule.errortracker import make_error_tracker, exc_info_to_str
 from xmodule.course_module import CourseDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
 from xmodule.x_module import XMLParsingSystem, policy_key
+from xmodule.modulestore.xml_exporter import DEFAULT_CONTENT_FIELDS
 
 from xblock.fields import ScopeIds
 from xblock.field_data import DictFieldData
@@ -582,9 +583,35 @@ class XMLModuleStore(ModuleStoreReadBase):
         if os.path.isdir(base_dir / url_name):
             self._load_extra_content(system, course_descriptor, category, base_dir / url_name, course_dir)
 
-    def _load_extra_content(self, system, course_descriptor, category, path, course_dir):
-
-        for filepath in glob.glob(path / '*'):
+    def _import_field_content(self, course_descriptor, category, file_path):
+        """
+        Import field data content for field other than 'data' or 'metadata' form json file and
+        return field data content as dictionary
+        """
+        slug, location, data_content = None, None, None
+        try:
+            # try to read json file
+            # file_path format: {dirname}.{field_name}.json
+            dirname, field, file_suffix = file_path.split('/')[-1].split('.')
+            if file_suffix == 'json' and field not in DEFAULT_CONTENT_FIELDS:
+                slug = os.path.splitext(os.path.basename(dirname))[0]
+                location = course_descriptor.scope_ids.usage_id.replace(category=category, name=slug)
+                with open(file_path) as field_content_file:
+                    field_data = json.load(field_content_file)
+                    data_content = {field: field_data}
+        except (IOError, ValueError):
+            # ignore this exception
+            # only new exported courses which use content fields other than 'metadata' and 'data'
+            # will have this file '{dirname}.{field_name}.json'
+            data_content = None
+
+        return slug, location, data_content
+
+    def _load_extra_content(self, system, course_descriptor, category, content_path, course_dir):
+        """
+        Import fields data content from files
+        """
+        for filepath in glob.glob(content_path / '*'):
             if not os.path.isfile(filepath):
                 continue
 
@@ -593,28 +620,55 @@ class XMLModuleStore(ModuleStoreReadBase):
 
             with open(filepath) as f:
                 try:
-                    html = f.read().decode('utf-8')
-                    # tabs are referenced in policy.json through a 'slug' which is just the filename without the .html suffix
-                    slug = os.path.splitext(os.path.basename(filepath))[0]
-                    loc = course_descriptor.scope_ids.usage_id.replace(category=category, name=slug)
-                    module = system.construct_xblock(
-                        category,
-                        # We're loading a descriptor, so student_id is meaningless
-                        # We also don't have separate notions of definition and usage ids yet,
-                        # so we use the location for both
-                        ScopeIds(None, category, loc, loc),
-                        DictFieldData({'data': html, 'location': loc, 'category': category}),
-                    )
-                    # VS[compat]:
-                    # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them)
-                    # from the course policy
-                    if category == "static_tab":
-                        for tab in course_descriptor.tabs or []:
-                            if tab.get('url_slug') == slug:
-                                module.display_name = tab['name']
-                    module.data_dir = course_dir
-                    module.save()
-                    self.modules[course_descriptor.id][module.scope_ids.usage_id] = module
+                    if filepath.find('.json') != -1:
+                        # json file with json data content
+                        slug, loc, data_content = self._import_field_content(course_descriptor, category, filepath)
+                        if data_content is None:
+                            continue
+                        else:
+                            try:
+                                # get and update data field in xblock runtime
+                                module = system.load_item(loc)
+                                for key, value in data_content.iteritems():
+                                    setattr(module, key, value)
+                                module.save()
+                            except ItemNotFoundError:
+                                module = None
+                                data_content['location'] = loc
+                                data_content['category'] = category
+                    else:
+                        slug = os.path.splitext(os.path.basename(filepath))[0]
+                        loc = course_descriptor.scope_ids.usage_id.replace(category=category, name=slug)
+                        # html file with html data content
+                        html = f.read().decode('utf-8')
+                        try:
+                            module = system.load_item(loc)
+                            module.data = html
+                            module.save()
+                        except ItemNotFoundError:
+                            module = None
+                            data_content = {'data': html, 'location': loc, 'category': category}
+
+                    if module is None:
+                        module = system.construct_xblock(
+                            category,
+                            # We're loading a descriptor, so student_id is meaningless
+                            # We also don't have separate notions of definition and usage ids yet,
+                            # so we use the location for both
+                            ScopeIds(None, category, loc, loc),
+                            DictFieldData(data_content),
+                        )
+                        # VS[compat]:
+                        # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them)
+                        # from the course policy
+                        if category == "static_tab":
+                            for tab in course_descriptor.tabs or []:
+                                if tab.get('url_slug') == slug:
+                                    module.display_name = tab['name']
+                        module.data_dir = course_dir
+                        module.save()
+
+                        self.modules[course_descriptor.id][module.scope_ids.usage_id] = module
                 except Exception, e:
                     logging.exception("Failed to load %s. Skipping... \
                             Exception: %s", filepath, unicode(e))

common/lib/xmodule/xmodule/modulestore/xml_exporter.py

@@ -4,6 +4,7 @@ Methods for exporting course data to XML
 
 import logging
 import lxml.etree
+from xblock.fields import Scope
 from xmodule.modulestore import Location
 from xmodule.modulestore.inheritance import own_metadata
 from fs.osfs import OSFS
@@ -19,6 +20,9 @@ PUBLISHED_DIR = "published"
 EXPORT_VERSION_FILE = "format.json"
 EXPORT_VERSION_KEY = "export_format"
 
+DEFAULT_CONTENT_FIELDS = ['metadata', 'data']
+
+
 class EdxJSONEncoder(json.JSONEncoder):
     """
     Custom JSONEncoder that handles `Location` and `datetime.datetime` objects.
@@ -120,6 +124,20 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d
                     draft_vertical.add_xml_to_node(node)
 
 
+def _export_field_content(xblock_item, item_dir):
+    """
+    Export all fields related to 'xblock_item' other than 'metadata' and 'data' to json file in provided directory
+    """
+    module_data = xblock_item.get_explicitly_set_fields_by_scope(Scope.content)
+    if isinstance(module_data, dict):
+        for field_name in module_data:
+            if field_name not in DEFAULT_CONTENT_FIELDS:
+                # filename format: {dirname}.{field_name}.json
+                with item_dir.open('{0}.{1}.{2}'.format(xblock_item.location.name, field_name, 'json'),
+                                   'w') as field_content_file:
+                    field_content_file.write(dumps(module_data.get(field_name, {}), cls=EdxJSONEncoder))
+
+
 def export_extra_content(export_fs, modulestore, course_id, course_location, category_type, dirname, file_suffix=''):
     query_loc = Location('i4x', course_location.org, course_location.course, category_type, None)
     items = modulestore.get_items(query_loc, course_id)
@@ -130,6 +148,9 @@ def export_extra_content(export_fs, modulestore, course_id, course_location, cat
             with item_dir.open(item.location.name + file_suffix, 'w') as item_file:
                 item_file.write(item.data.encode('utf8'))
 
+                # export content fields other then metadata and data in json format in current directory
+                _export_field_content(item, item_dir)
+
 
 def convert_between_versions(source_dir, target_dir):
     """

common/test/data/course_info_updates/about/overview.html

+<section class="about">
+  <h2>About This Course</h2>
+  <p>Include your long course description here. The long course description should contain 150-400 words.</p>
+
+  <p>This is paragraph 2 of the long course description. Add more paragraphs as needed. Make sure to enclose them in paragraph tags.</p>
+</section>
+
+<section class="prerequisites">
+  <h2>Prerequisites</h2>
+  <p>Add information about course prerequisites here.</p>
+</section>
+
+<section class="course-staff">
+  <h2>Course Staff</h2>
+  <article class="teacher">
+    <div class="teacher-image">
+      <img src="/static/images/pl-faculty.png" align="left" style="margin:0 20 px 0" alt="Course Staff Image #1">
+    </div>
+
+    <h3>Staff Member #1</h3>
+    <p>Biography of instructor/staff member #1</p>
+  </article>
+
+  <article class="teacher">
+    <div class="teacher-image">
+      <img src="/static/images/pl-faculty.png" align="left" style="margin:0 20 px 0" alt="Course Staff Image #2">
+    </div>
+
+    <h3>Staff Member #2</h3>
+    <p>Biography of instructor/staff member #2</p>
+  </article>
+</section>
+
+<section class="faq">
+  <section class="responses">
+    <h2>Frequently Asked Questions</h2>
+    <article class="response">
+      <h3>Do I need to buy a textbook?</h3>
+      <p>No, a free online version of Chemistry: Principles, Patterns, and Applications, First Edition by Bruce Averill and Patricia Eldredge will be available, though you can purchase a printed version (published by FlatWorld Knowledge) if you’d like.</p>
+    </article>
+
+    <article class="response">
+      <h3>Question #2</h3>
+      <p>Your answer would be displayed here.</p>
+    </article>
+  </section>
+</section>

common/test/data/course_info_updates/course.xml

+<course url_name="2014_T1" org="edX" course="course_info_updates"/>

common/test/data/course_info_updates/course/2014_T1.xml

+<course display_name="Toy Course"/>

common/test/data/course_info_updates/info/handouts.html

+<ol></ol>
\ No newline at end of file

common/test/data/course_info_updates/info/updates.html

+<ol><li><h2>February 13, 2014</h2>Sample update</li></ol>
\ No newline at end of file

common/test/data/course_info_updates/info/updates.items.json

+[{"date": "February 13, 2014", "content": "Sample update", "status": "visible", "id": 1}]
\ No newline at end of file

common/test/data/course_info_updates/policies/2014_T1/grading_policy.json

+{"GRADER": [{"short_label": "HW", "min_count": 12, "type": "Homework", "drop_count": 2, "weight": 0.15}, {"min_count": 12, "type": "Lab", "drop_count": 2, "weight": 0.15}, {"short_label": "Midterm", "min_count": 1, "type": "Midterm Exam", "drop_count": 0, "weight": 0.3}, {"short_label": "Final", "min_count": 1, "type": "Final Exam", "drop_count": 0, "weight": 0.4}], "GRADE_CUTOFFS": {"Pass": 0.5}}
\ No newline at end of file

common/test/data/course_info_updates/policies/2014_T1/policy.json

+{"course/2014_T1": {"tabs": [{"type": "courseware", "name": "Courseware"}, {"type": "course_info", "name": "Course Info"}, {"type": "discussion", "name": "Discussion"}, {"type": "wiki", "name": "Wiki"}, {"type": "progress", "name": "Progress"}], "display_name": "Toy Course", "discussion_topics": {"General": {"id": "i4x-edX-course_info_updates-course-2014_T1"}}}}
\ No newline at end of file

common/test/data/course_info_updates/policies/assets.json

+{}
\ No newline at end of file