Merge pull request #1701 from MITx/bug/orphan

Fix bug 249 the ugly way (decerebrate the moronically impotent model)
2013-03-21 07:21:16 -07:00
parent 93eebdcd97 ae55cd7533
commit 37a673012f
5 changed files with 186 additions and 50 deletions
--- a/cms/djangoapps/contentstore/course_info_model.py
+++ b/cms/djangoapps/contentstore/course_info_model.py
@@ -1,13 +1,15 @@
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
-from lxml import html, etree
+from lxml import html
 import re
 from django.http import HttpResponseBadRequest
 import logging
+import django.utils

-## TODO store as array of { date, content } and override  course_info_module.definition_from_xml
-## This should be in a class which inherits from XmlDescriptor
+# # TODO store as array of { date, content } and override  course_info_module.definition_from_xml
+# # This should be in a class which inherits from XmlDescriptor
+log = logging.getLogger(__name__)


 def get_course_updates(location):
@@ -26,9 +28,11 @@ def get_course_updates(location):

    # purely to handle free formed updates not done via editor. Actually kills them, but at least doesn't break.
    try:
-        course_html_parsed = etree.fromstring(course_updates.data)
-    except etree.XMLSyntaxError:
-        course_html_parsed = etree.fromstring("<ol></ol>")
+        course_html_parsed = html.fromstring(course_updates.data)
+    except:
+        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_upd_collection = []
@@ -64,9 +68,11 @@ def update_course_updates(location, update, passed_id=None):

    # purely to handle free formed updates not done via editor. Actually kills them, but at least doesn't break.
    try:
-        course_html_parsed = etree.fromstring(course_updates.data)
-    except etree.XMLSyntaxError:
-        course_html_parsed = etree.fromstring("<ol></ol>")
+        course_html_parsed = html.fromstring(course_updates.data)
+    except:
+        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>")

    # No try/catch b/c failure generates an error back to client
    new_html_parsed = html.fromstring('<li><h2>' + update['date'] + '</h2>' + update['content'] + '</li>')
@@ -85,12 +91,19 @@ def update_course_updates(location, update, passed_id=None):
            passed_id = course_updates.location.url() + "/" + str(idx)

        # update db record
-        course_updates.data = etree.tostring(course_html_parsed)
+        course_updates.data = html.tostring(course_html_parsed)
        modulestore('direct').update_item(location, course_updates.data)

-        return {"id" : passed_id,
-                "date" : update['date'],
-                "content" :update['content']}
+        if (len(new_html_parsed) == 1):
+            content = new_html_parsed[0].tail
+        else:
+            content = "\n".join([html.tostring(ele)
+                for ele in new_html_parsed[1:]])
+
+        return {"id": passed_id,
+                "date": update['date'],
+                "content": content}
+

 def delete_course_update(location, update, passed_id):
    """
@@ -108,9 +121,11 @@ def delete_course_update(location, update, passed_id):
    # TODO use delete_blank_text parser throughout and cache as a static var in a class
    # purely to handle free formed updates not done via editor. Actually kills them, but at least doesn't break.
    try:
-        course_html_parsed = etree.fromstring(course_updates.data)
-    except etree.XMLSyntaxError:
-        course_html_parsed = etree.fromstring("<ol></ol>")
+        course_html_parsed = html.fromstring(course_updates.data)
+    except:
+        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>")

    if course_html_parsed.tag == 'ol':
        # ??? Should this use the id in the json or in the url or does it matter?
@@ -121,7 +136,7 @@ def delete_course_update(location, update, passed_id):
            course_html_parsed.remove(element_to_delete)

        # update db record
-        course_updates.data = etree.tostring(course_html_parsed)
+        course_updates.data = html.tostring(course_html_parsed)
        store = modulestore('direct')
        store.update_item(location, course_updates.data)

@@ -132,7 +147,6 @@ def get_idx(passed_id):
    """
    From the url w/ idx appended, get the idx.
    """
-    # TODO compile this regex into a class static and reuse for each call
-    idx_matcher = re.search(r'.*/(\d+)$', passed_id)
+    idx_matcher = re.search(r'.*?/?(\d+)$', passed_id)
    if idx_matcher:
        return int(idx_matcher.group(1))
--- a/cms/djangoapps/contentstore/tests/test_course_updates.py
+++ b/cms/djangoapps/contentstore/tests/test_course_updates.py
@@ -1,31 +1,135 @@
 from contentstore.tests.test_course_settings import CourseTestCase
 from django.core.urlresolvers import reverse
 import json
+from webob.exc import HTTPServerError
+from django.http import HttpResponseBadRequest


 class CourseUpdateTest(CourseTestCase):
    def test_course_update(self):
        # first get the update to force the creation
-        url = reverse('course_info', kwargs={'org': self.course_location.org, 'course': self.course_location.course,
-                                       'name': self.course_location.name})
+        url = reverse('course_info',
+            kwargs={'org': self.course_location.org,
+                'course': self.course_location.course,
+                'name': self.course_location.name})
        self.client.get(url)

-        content = '<iframe width="560" height="315" src="http://www.youtube.com/embed/RocY-Jd93XU" frameborder="0"></iframe>'
+        init_content = '<iframe width="560" height="315" src="http://www.youtube.com/embed/RocY-Jd93XU" frameborder="0">'
+        content = init_content + '</iframe>'
        payload = {'content': content,
                   'date': 'January 8, 2013'}
-        url = reverse('course_info', kwargs={'org': self.course_location.org, 'course': self.course_location.course,
-                                             'provided_id': ''})
+        url = reverse('course_info_json', kwargs={'org': self.course_location.org,
+            'course': self.course_location.course,
+            'provided_id': ''})

        resp = self.client.post(url, json.dumps(payload), "application/json")

        payload = json.loads(resp.content)

-        self.assertHTMLEqual(content, payload['content'], "single iframe")
+        self.assertHTMLEqual(payload['content'], content)

-        url = reverse('course_info', kwargs={'org': self.course_location.org, 'course': self.course_location.course,
-                                             'provided_id': payload['id']})
-        content += '<div>div <p>p</p></div>'
+        first_update_url = reverse('course_info_json',
+            kwargs={'org': self.course_location.org,
+                'course': self.course_location.course,
+                'provided_id': payload['id']})
+        content += '<div>div <p>p<br/></p></div>'
        payload['content'] = content
+        resp = self.client.post(first_update_url, json.dumps(payload),
+            "application/json")
+
+        self.assertHTMLEqual(content, json.loads(resp.content)['content'],
+            "iframe w/ div")
+
+        # now put in an evil update
+        content = '<ol/>'
+        payload = {'content': content,
+                   'date': 'January 11, 2013'}
+        url = reverse('course_info_json',
+            kwargs={'org': self.course_location.org,
+                'course': self.course_location.course,
+                'provided_id': ''})
+
        resp = self.client.post(url, json.dumps(payload), "application/json")

-        self.assertHTMLEqual(content, json.loads(resp.content)['content'], "iframe w/ div")
+        payload = json.loads(resp.content)
+
+        self.assertHTMLEqual(content, payload['content'], "self closing ol")
+
+        url = reverse('course_info_json',
+            kwargs={'org': self.course_location.org,
+                'course': self.course_location.course,
+                'provided_id': ''})
+        resp = self.client.get(url)
+        payload = json.loads(resp.content)
+        self.assertTrue(len(payload) == 2)
+
+        # can't test non-json paylod b/c expect_json throws error
+        # try json w/o required fields
+        self.assertContains(
+            self.client.post(url, json.dumps({'garbage': 1}),
+                "application/json"),
+            'Failed to save', status_code=400)
+
+        # now try to update a non-existent update
+        url = reverse('course_info_json',
+            kwargs={'org': self.course_location.org,
+                'course': self.course_location.course,
+                'provided_id': '9'})
+        content = 'blah blah'
+        payload = {'content': content,
+                   'date': 'January 21, 2013'}
+        self.assertContains(
+            self.client.post(url, json.dumps(payload), "application/json"),
+            'Failed to save', status_code=400)
+
+        # update w/ malformed html
+        content = '<garbage tag No closing brace to force <span>error</span>'
+        payload = {'content': content,
+                   'date': 'January 11, 2013'}
+        url = reverse('course_info_json', kwargs={'org': self.course_location.org,
+            'course': self.course_location.course,
+            'provided_id': ''})
+
+        resp = self.client.post(url, json.dumps(payload), "application/json")
+
+        payload = json.loads(resp.content)
+
+        self.assertContains(
+            self.client.post(url, json.dumps(payload), "application/json"),
+            '<garbage')
+
+        # now try to delete a non-existent update
+        url = reverse('course_info_json', kwargs={'org': self.course_location.org,
+            'course': self.course_location.course,
+            'provided_id': '19'})
+        payload = {'content': content,
+                   'date': 'January 21, 2013'}
+        self.assertContains(self.client.delete(url), "delete", status_code=400)
+
+        # now delete a real update
+        content = 'blah blah'
+        payload = {'content': content,
+                   'date': 'January 28, 2013'}
+        url = reverse('course_info_json', kwargs={'org': self.course_location.org,
+            'course': self.course_location.course,
+            'provided_id': ''})
+        resp = self.client.post(url, json.dumps(payload), "application/json")
+        payload = json.loads(resp.content)
+        this_id = payload['id']
+        self.assertHTMLEqual(content, payload['content'], "single iframe")
+        # first count the entries
+        url = reverse('course_info_json',
+            kwargs={'org': self.course_location.org,
+                'course': self.course_location.course,
+                'provided_id': ''})
+        resp = self.client.get(url)
+        payload = json.loads(resp.content)
+        before_delete = len(payload)
+
+        url = reverse('course_info_json',
+            kwargs={'org': self.course_location.org,
+                'course': self.course_location.course,
+                'provided_id': this_id})
+        resp = self.client.delete(url)
+        payload = json.loads(resp.content)
+        self.assertTrue(len(payload) == before_delete - 1)
--- a/cms/djangoapps/contentstore/views.py
+++ b/cms/djangoapps/contentstore/views.py
@@ -54,12 +54,12 @@ from auth.authz import INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME, create_all_course_
 from .utils import get_course_location_for_item, get_lms_link_for_item, compute_unit_state, get_date_display, UnitState, get_course_for_item

 from xmodule.modulestore.xml_importer import import_from_xml
-from contentstore.course_info_model import get_course_updates,\
+from contentstore.course_info_model import get_course_updates, \
    update_course_updates, delete_course_update
 from cache_toolbox.core import del_cached_content
 from xmodule.timeparse import stringify_time
 from contentstore.module_info_model import get_module_info, set_module_info
-from models.settings.course_details import CourseDetails,\
+from models.settings.course_details import CourseDetails, \
    CourseSettingsEncoder
 from models.settings.course_grading import CourseGradingModel
 from contentstore.utils import get_modulestore
@@ -73,7 +73,7 @@ log = logging.getLogger(__name__)

 COMPONENT_TYPES = ['customtag', 'discussion', 'html', 'problem', 'video']

-ADVANCED_COMPONENT_TYPES = ['annotatable','combinedopenended', 'peergrading']
+ADVANCED_COMPONENT_TYPES = ['annotatable', 'combinedopenended', 'peergrading']
 ADVANCED_COMPONENT_CATEGORY = 'advanced'
 ADVANCED_COMPONENT_POLICY_KEY = 'advanced_modules'

@@ -322,7 +322,7 @@ def edit_unit(request, location):
            category = ADVANCED_COMPONENT_CATEGORY

        if category in component_types:
-            #This is a hack to create categories for different xmodules
+            # This is a hack to create categories for different xmodules
            component_templates[category].append((
                template.display_name_with_default,
                template.location.url(),
@@ -417,7 +417,7 @@ def assignment_type_update(request, org, course, category, name):
    if request.method == 'GET':
        return HttpResponse(json.dumps(CourseGradingModel.get_section_grader_type(location)),
                            mimetype="application/json")
-    elif request.method == 'POST':   # post or put, doesn't matter.
+    elif request.method == 'POST':  # post or put, doesn't matter.
        return HttpResponse(json.dumps(CourseGradingModel.update_section_grader_type(location, request.POST)),
                            mimetype="application/json")

@@ -831,7 +831,7 @@ def upload_asset(request, org, course, coursename):
    if thumbnail_content is not None:
        content.thumbnail_location = thumbnail_location

-    #then commit the content
+    # then commit the content
    contentstore().save(content)
    del_cached_content(content.location)

@@ -874,7 +874,7 @@ def manage_users(request, location):
    })


-def create_json_response(errmsg = None):
+def create_json_response(errmsg=None):
    if errmsg is not None:
        resp = HttpResponse(json.dumps({'Status': 'Failed', 'ErrMsg': errmsg}))
    else:
@@ -1118,14 +1118,22 @@ def course_info_updates(request, org, course, provided_id=None):
        real_method = request.method

    if request.method == 'GET':
-        return HttpResponse(json.dumps(get_course_updates(location)), mimetype="application/json")
-    elif real_method == 'DELETE':  # coming as POST need to pull from Request Header X-HTTP-Method-Override    DELETE
-        return HttpResponse(json.dumps(delete_course_update(location, request.POST, provided_id)), mimetype="application/json")
+        return HttpResponse(json.dumps(get_course_updates(location)),
+            mimetype="application/json")
+    elif real_method == 'DELETE':
+        try:
+            return HttpResponse(json.dumps(delete_course_update(location,
+                request.POST, provided_id)), mimetype="application/json")
+        except:
+            return HttpResponseBadRequest("Failed to delete",
+                content_type="text/plain")
    elif request.method == 'POST':
        try:
-            return HttpResponse(json.dumps(update_course_updates(location, request.POST, provided_id)), mimetype="application/json")
+            return HttpResponse(json.dumps(update_course_updates(location,
+                request.POST, provided_id)), mimetype="application/json")
        except:
-            return HttpResponseBadRequest("Failed to save: malformed html", content_type="text/plain")
+            return HttpResponseBadRequest("Failed to save",
+                content_type="text/plain")


@expect_json
@@ -1260,7 +1268,7 @@ def course_settings_updates(request, org, course, name, section):
        # Cannot just do a get w/o knowing the course name :-(
        return HttpResponse(json.dumps(manager.fetch(Location(['i4x', org, course, 'course', name])), cls=CourseSettingsEncoder),
                            mimetype="application/json")
-    elif request.method == 'POST':   # post or put, doesn't matter.
+    elif request.method == 'POST':  # post or put, doesn't matter.
        return HttpResponse(json.dumps(manager.update_from_json(request.POST), cls=CourseSettingsEncoder),
                            mimetype="application/json")

@@ -1295,12 +1303,12 @@ def course_grader_updates(request, org, course, name, grader_index=None):
        # ??? Shoudl this return anything? Perhaps success fail?
        CourseGradingModel.delete_grader(Location(['i4x', org, course, 'course', name]), grader_index)
        return HttpResponse()
-    elif request.method == 'POST':   # post or put, doesn't matter.
+    elif request.method == 'POST':  # post or put, doesn't matter.
        return HttpResponse(json.dumps(CourseGradingModel.update_grader_from_json(Location(['i4x', org, course, 'course', name]), request.POST)),
                            mimetype="application/json")


-## NB: expect_json failed on ["key", "key2"] and json payload
+# # NB: expect_json failed on ["key", "key2"] and json payload
@login_required
@ensure_csrf_cookie
 def course_advanced_updates(request, org, course, name):
@@ -1558,7 +1566,7 @@ def generate_export_course(request, org, course, name):
    logging.debug('root = {0}'.format(root_dir))

    export_to_xml(modulestore('direct'), contentstore(), loc, root_dir, name)
-    #filename = root_dir / name + '.tar.gz'
+    # filename = root_dir / name + '.tar.gz'

    logging.debug('tar file being generated at {0}'.format(export_file.name))
    tf = tarfile.open(name=export_file.name, mode='w:gz')
--- a/cms/static/coffee/src/views/unit.coffee
+++ b/cms/static/coffee/src/views/unit.coffee
@@ -34,7 +34,10 @@ class CMS.Views.UnitEdit extends Backbone.View

    @$('.components').sortable(
      handle: '.drag-handle'
-      update: (event, ui) => @model.save(children: @components())
+      update: (event, ui) =>
+        payload = children : @components()
+        options = success : => @model.unset('children')
+        @model.save(payload, options)
      helper: 'clone'
      opacity: '0.5'
      placeholder: 'component-placeholder'
@@ -109,7 +112,14 @@ class CMS.Views.UnitEdit extends Backbone.View
      id: $component.data('id')
    }, =>
      $component.remove()
-      @model.save(children: @components())
+      # b/c we don't vigilantly keep children up to date
+      # get rid of it before it hurts someone
+      # sorry for the js, i couldn't figure out the coffee equivalent
+      `_this.model.save({children: _this.components()},
+          {success: function(model) {
+              model.unset('children');
+          }}
+      );`
    )

  deleteDraft: (event) ->
--- a/cms/urls.py
+++ b/cms/urls.py
@@ -43,7 +43,7 @@ urlpatterns = ('',
    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)/remove_user$',
        'contentstore.views.remove_user', name='remove_user'),
    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/info/(?P<name>[^/]+)$', 'contentstore.views.course_info', name='course_info'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course_info/updates/(?P<provided_id>.*)$', 'contentstore.views.course_info_updates', name='course_info'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course_info/updates/(?P<provided_id>.*)$', 'contentstore.views.course_info_updates', name='course_info_json'),
    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-details/(?P<name>[^/]+)$', 'contentstore.views.get_course_settings', name='course_settings'),
    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-grading/(?P<name>[^/]+)$', 'contentstore.views.course_config_graders_page', name='course_settings'),
    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-details/(?P<name>[^/]+)/section/(?P<section>[^/]+).*$', 'contentstore.views.course_settings_updates', name='course_settings'),
@@ -100,12 +100,12 @@ urlpatterns += (
    )

 if settings.ENABLE_JASMINE:
-    ## Jasmine
+    # # Jasmine
    urlpatterns = urlpatterns + (url(r'^_jasmine/', include('django_jasmine.urls')),)

 urlpatterns = patterns(*urlpatterns)

-#Custom error pages
+# Custom error pages
 handler404 = 'contentstore.views.render_404'
 handler500 = 'contentstore.views.render_500'