Merge pull request #6495 from edx/mobile/MA-212

MA-212 Mobile API support for backward compatibility of course updates.

cms/djangoapps/contentstore/course_info_model.py

@@ -16,14 +16,14 @@ import re
 import logging
 
 from django.http import HttpResponseBadRequest
-import django.utils
 from django.utils.translation import ugettext as _
-from lxml import html, etree
 
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.django import modulestore
 from xmodule.html_module import CourseInfoModule
 
+from xmodule_modifiers import get_course_update_items
+
 # # This should be in a class which inherits from XmlDescriptor
 log = logging.getLogger(__name__)
 
@@ -38,7 +38,7 @@ def get_course_updates(location, provided_id, user_id):
     except ItemNotFoundError:
         course_updates = modulestore().create_item(user_id, location.course_key, location.block_type, location.block_id)
 
-    course_update_items = get_course_update_items(course_updates, provided_id)
+    course_update_items = get_course_update_items(course_updates, _get_index(provided_id))
     return _get_visible_update(course_update_items)
 
 
@@ -82,19 +82,6 @@ def update_course_updates(location, update, passed_id=None, user=None):
     return course_update_dict
 
 
-def _course_info_content(html_parsed):
-    """
-    Constructs the HTML for the course info update, not including the header.
-    """
-    if len(html_parsed) == 1:
-        # could enforce that update[0].tag == 'h2'
-        content = html_parsed[0].tail
-    else:
-        content = html_parsed[0].tail if html_parsed[0].tail is not None else ""
-        content += "\n".join([html.tostring(ele) for ele in html_parsed[1:]])
-    return content
-
-
 def _make_update_dict(update):
     """
     Return course update item as a dictionary with required keys ('id', "date" and "content").
@@ -167,55 +154,6 @@ def _get_index(passed_id=None):
     return 0
 
 
-def get_course_update_items(course_updates, provided_id=None):
-    """
-    Returns list of course_updates data dictionaries either from new format if available or
-    from old. This function don't modify old data to new data (in db), instead returns data
-    in common old dictionary format.
-    New Format: {"items" : [{"id": computed_id, "date": date, "content": html-string}],
-                 "data": "<ol>[<li><h2>date</h2>content</li>]</ol>"}
-    Old Format: {"data": "<ol>[<li><h2>date</h2>content</li>]</ol>"}
-    """
-    if course_updates and getattr(course_updates, "items", None):
-        provided_id = _get_index(provided_id)
-        if provided_id and 0 < provided_id <= len(course_updates.items):
-            return course_updates.items[provided_id - 1]
-
-        # return list in reversed order (old format: [4,3,2,1]) for compatibility
-        return list(reversed(course_updates.items))
-    else:
-        # old method to get course updates
-        # purely to handle free formed updates not done via editor. Actually kills them, but at least doesn't break.
-        try:
-            course_html_parsed = html.fromstring(course_updates.data)
-        except (etree.XMLSyntaxError, etree.ParserError):
-            log.error("Cannot parse: " + course_updates.data)
-            escaped = django.utils.html.escape(course_updates.data)
-            course_html_parsed = html.fromstring("<ol><li>" + escaped + "</li></ol>")
-
-        # confirm that root is <ol>, iterate over <li>, pull out <h2> subs and then rest of val
-        course_update_items = []
-        provided_id = _get_index(provided_id)
-        if course_html_parsed.tag == 'ol':
-            # 0 is the newest
-            for index, update in enumerate(course_html_parsed):
-                if len(update) > 0:
-                    content = _course_info_content(update)
-                    # make the id on the client be 1..len w/ 1 being the oldest and len being the newest
-                    computed_id = len(course_html_parsed) - index
-                    payload = {
-                        "id": computed_id,
-                        "date": update.findtext("h2"),
-                        "content": content
-                    }
-                    if provided_id == 0:
-                        course_update_items.append(payload)
-                    elif provided_id == computed_id:
-                        return payload
-
-        return course_update_items
-
-
 def _get_html(course_updates_items):
     """
     Method to create course_updates_html from course_updates items

common/djangoapps/xmodule_modifiers.py

@@ -8,9 +8,11 @@ import logging
 import static_replace
 import uuid
 import markupsafe
+from lxml import html, etree
 
 from django.conf import settings
 from django.utils.timezone import UTC
+from django.utils.html import escape
 from edxmako.shortcuts import render_to_string
 from xblock.exceptions import InvalidScopeError
 from xblock.fragment import Fragment
@@ -278,3 +280,63 @@ def add_staff_markup(user, has_instructor_access, block, view, frag, context):
                      'has_instructor_access': has_instructor_access,
                      }
     return wrap_fragment(frag, render_to_string("staff_problem_info.html", staff_context))
+
+
+def get_course_update_items(course_updates, provided_index=0):
+    """
+    Returns list of course_updates data dictionaries either from new format if available or
+    from old. This function don't modify old data to new data (in db), instead returns data
+    in common old dictionary format.
+    New Format: {"items" : [{"id": computed_id, "date": date, "content": html-string}],
+                 "data": "<ol>[<li><h2>date</h2>content</li>]</ol>"}
+    Old Format: {"data": "<ol>[<li><h2>date</h2>content</li>]</ol>"}
+    """
+    def _course_info_content(html_parsed):
+        """
+        Constructs the HTML for the course info update, not including the header.
+        """
+        if len(html_parsed) == 1:
+            # could enforce that update[0].tag == 'h2'
+            content = html_parsed[0].tail
+        else:
+            content = html_parsed[0].tail if html_parsed[0].tail is not None else ""
+            content += "\n".join([html.tostring(ele) for ele in html_parsed[1:]])
+        return content
+
+    if course_updates and getattr(course_updates, "items", None):
+        if provided_index and 0 < provided_index <= len(course_updates.items):
+            return course_updates.items[provided_index - 1]
+        else:
+            # return list in reversed order (old format: [4,3,2,1]) for compatibility
+            return list(reversed(course_updates.items))
+
+    course_update_items = []
+    if course_updates:
+        # old method to get course updates
+        # purely to handle free formed updates not done via editor. Actually kills them, but at least doesn't break.
+        try:
+            course_html_parsed = html.fromstring(course_updates.data)
+        except (etree.XMLSyntaxError, etree.ParserError):
+            log.error("Cannot parse: " + course_updates.data)
+            escaped = escape(course_updates.data)
+            course_html_parsed = html.fromstring("<ol><li>" + escaped + "</li></ol>")
+
+        # confirm that root is <ol>, iterate over <li>, pull out <h2> subs and then rest of val
+        if course_html_parsed.tag == 'ol':
+            # 0 is the newest
+            for index, update in enumerate(course_html_parsed):
+                if len(update) > 0:
+                    content = _course_info_content(update)
+                    # make the id on the client be 1..len w/ 1 being the oldest and len being the newest
+                    computed_id = len(course_html_parsed) - index
+                    payload = {
+                        "id": computed_id,
+                        "date": update.findtext("h2"),
+                        "content": content
+                    }
+                    if provided_index == 0:
+                        course_update_items.append(payload)
+                    elif provided_index == computed_id:
+                        return payload
+
+    return course_update_items

lms/djangoapps/mobile_api/course_info/tests.py

@@ -2,6 +2,7 @@
 Tests for course_info
 """
 
+import ddt
 from django.conf import settings
 
 from xmodule.html_module import CourseInfoModule
@@ -43,6 +44,7 @@ class TestAbout(MobileAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMi
         self.assertNotIn('\"/static/', response.data['overview'])
 
 
+@ddt.ddt
 class TestUpdates(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin):
     """
     Tests for /api/mobile/v0.5/course_info/{course_id}/updates
@@ -53,9 +55,15 @@ class TestUpdates(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAc
         super(TestUpdates, self).verify_success(response)
         self.assertEqual(response.data, [])
 
-    def test_updates_static_rewrite(self):
+    @ddt.data(True, False)
+    def test_updates(self, new_format):
+        """
+        Tests updates endpoint with /static in the content.
+        Tests both new updates format (using "items") and old format (using "data").
+        """
         self.login_and_enroll()
 
+        # create course Updates item in modulestore
         updates_usage_key = self.course.id.make_usage_key('course_info', 'updates')
         course_updates = modulestore().create_item(
             self.user.id,
@@ -63,22 +71,32 @@ class TestUpdates(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAc
             updates_usage_key.block_type,
             block_id=updates_usage_key.block_id
         )
-        course_update_data = {
-            "id": 1,
-            "date": "Some date",
-            "content": "<a href=\"/static/\">foo</a>",
-            "status": CourseInfoModule.STATUS_VISIBLE
-        }
 
-        course_updates.items = [course_update_data]
+        # store content in Updates item (either new or old format)
+        if new_format:
+            course_update_data = {
+                "id": 1,
+                "date": "Some date",
+                "content": "<a href=\"/static/\">foo</a>",
+                "status": CourseInfoModule.STATUS_VISIBLE
+            }
+            course_updates.items = [course_update_data]
+        else:
+            update_data = u"<ol><li><h2>Date</h2><a href=\"/static/\">foo</a></li></ol>"
+            course_updates.data = update_data
         modulestore().update_item(course_updates, self.user.id)
 
+        # call API
         response = self.api_response()
         content = response.data[0]["content"]  # pylint: disable=maybe-no-member
+
+        # verify static URLs are replaced in the content returned by the API
         self.assertNotIn("\"/static/", content)
 
-        underlying_updates_module = modulestore().get_item(updates_usage_key)
-        self.assertIn("\"/static/", underlying_updates_module.items[0]['content'])
+        # verify static URLs remain in the underlying content
+        underlying_updates = modulestore().get_item(updates_usage_key)
+        underlying_content = underlying_updates.items[0]['content'] if new_format else underlying_updates.data
+        self.assertIn("\"/static/", underlying_content)
 
 
 class TestHandouts(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin):

lms/djangoapps/mobile_api/course_info/views.py

@@ -7,6 +7,7 @@ from rest_framework.response import Response
 
 from courseware.courses import get_course_about_section, get_course_info_section_module
 from static_replace import make_static_urls_absolute, replace_static_urls
+from xmodule_modifiers import get_course_update_items
 
 from ..utils import MobileView, mobile_course_access
 
@@ -38,7 +39,7 @@ class CourseUpdatesList(generics.ListAPIView):
     @mobile_course_access()
     def list(self, request, course, *args, **kwargs):
         course_updates_module = get_course_info_section_module(request, course, 'updates')
-        update_items = reversed(getattr(course_updates_module, 'items', []))
+        update_items = list(reversed(get_course_update_items(course_updates_module)))
 
         updates_to_show = [
             update for update in update_items