From 3498eeb1f62fa1210c614a4ef73cb696e6a292a9 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 7 Jan 2015 22:19:14 -0500 Subject: [PATCH] MA-212 Mobile API support for backward compatibility of course updates. --- .../contentstore/course_info_model.py | 68 +------------------ common/djangoapps/xmodule_modifiers.py | 62 +++++++++++++++++ .../mobile_api/course_info/tests.py | 38 ++++++++--- .../mobile_api/course_info/views.py | 3 +- 4 files changed, 95 insertions(+), 76 deletions(-) diff --git a/cms/djangoapps/contentstore/course_info_model.py b/cms/djangoapps/contentstore/course_info_model.py index f2714af540..692f7a9dd4 100644 --- a/cms/djangoapps/contentstore/course_info_model.py +++ b/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": "
    [

  1. date

    content
    2. ]
"} - Old Format: {"data": "
    [

  1. date

    content
    2. ]
"} - """ - 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("
  1. " + escaped + "
") - - # confirm that root is
    , iterate over
  1. , pull out

    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 diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 4d3bb974cc..8e4a305595 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/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": "
      [

    1. date

      content
      2. ]
    "} + Old Format: {"data": "
      [

    1. date

      content
      2. ]
    "} + """ + 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("
    1. " + escaped + "
    ") + + # confirm that root is
      , iterate over
    1. , pull out

      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 diff --git a/lms/djangoapps/mobile_api/course_info/tests.py b/lms/djangoapps/mobile_api/course_info/tests.py index 2c8b989b5a..ad8892533b 100644 --- a/lms/djangoapps/mobile_api/course_info/tests.py +++ b/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": "foo", - "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": "foo", + "status": CourseInfoModule.STATUS_VISIBLE + } + course_updates.items = [course_update_data] + else: + update_data = u"

      1. Date

        foo
      " + 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): diff --git a/lms/djangoapps/mobile_api/course_info/views.py b/lms/djangoapps/mobile_api/course_info/views.py index 0e6a9f7861..76defffe71 100644 --- a/lms/djangoapps/mobile_api/course_info/views.py +++ b/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