From af136e75ca77e0ec3c4dfb343102ba549e4918ae Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 19 Mar 2013 18:07:46 -0400 Subject: [PATCH 1/5] Fix bug 249 the ugly way (decerebrate the moronically impotent model) --- cms/static/coffee/src/views/unit.coffee | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cms/static/coffee/src/views/unit.coffee b/cms/static/coffee/src/views/unit.coffee index 7f5fa4adce..3bf049d31b 100644 --- a/cms/static/coffee/src/views/unit.coffee +++ b/cms/static/coffee/src/views/unit.coffee @@ -109,7 +109,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) -> From bd2dd9408d2402e09b51f9f53faa304eb351cef7 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 20 Mar 2013 17:49:03 -0400 Subject: [PATCH 2/5] Unit tests and minor fixes to course_info_updates. Main fix was returning the content in same form as it was saved rather than parroting it back. --- .../contentstore/course_info_model.py | 52 +++++--- .../contentstore/tests/test_course_updates.py | 116 ++++++++++++++++-- cms/djangoapps/contentstore/views.py | 40 +++--- 3 files changed, 163 insertions(+), 45 deletions(-) diff --git a/cms/djangoapps/contentstore/course_info_model.py b/cms/djangoapps/contentstore/course_info_model.py index 8c8aed549d..589db4ac56 100644 --- 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("
    ") + 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("
    1. " + escaped + "
    ") # Confirm that root is
      , iterate over
    1. , pull out

      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("
        ") + 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("
        1. " + escaped + "
        ") # No try/catch b/c failure generates an error back to client new_html_parsed = html.fromstring('

      1. ' + update['date'] + '

        ' + update['content'] + '
        2. ') @@ -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("
          ") + 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("
          1. " + escaped + "
          ") 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)) diff --git a/cms/djangoapps/contentstore/tests/test_course_updates.py b/cms/djangoapps/contentstore/tests/test_course_updates.py index 6a3a1e21f7..29f387b3e6 100644 --- a/cms/djangoapps/contentstore/tests/test_course_updates.py +++ b/cms/djangoapps/contentstore/tests/test_course_updates.py @@ -1,31 +1,127 @@ 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 = '' + init_content = '' 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

          p

          ' + first_update_url = reverse('course_info_json', + kwargs={'org': self.course_location.org, + 'course': self.course_location.course, + 'provided_id': payload['id']}) + content += '
          div

          p

          ' 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 = '
            ' + 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) + + # 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 = 'error' + 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"), + ' Date: Wed, 20 Mar 2013 17:59:24 -0400 Subject: [PATCH 3/5] Also clean up children after sort saved. --- cms/static/coffee/src/views/unit.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cms/static/coffee/src/views/unit.coffee b/cms/static/coffee/src/views/unit.coffee index 3bf049d31b..13c7e53f02 100644 --- 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' From 9413364e55a8546c36a4f4a21f74b218a5b4818b Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 20 Mar 2013 18:00:25 -0400 Subject: [PATCH 4/5] Disambiguate the course_info url names. --- cms/urls.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/urls.py b/cms/urls.py index 69ce4a540d..4d46325a68 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -43,7 +43,7 @@ urlpatterns = ('', url(r'^(?P[^/]+)/(?P[^/]+)/course/(?P[^/]+)/remove_user$', 'contentstore.views.remove_user', name='remove_user'), url(r'^(?P[^/]+)/(?P[^/]+)/info/(?P[^/]+)$', 'contentstore.views.course_info', name='course_info'), - url(r'^(?P[^/]+)/(?P[^/]+)/course_info/updates/(?P.*)$', 'contentstore.views.course_info_updates', name='course_info'), + url(r'^(?P[^/]+)/(?P[^/]+)/course_info/updates/(?P.*)$', 'contentstore.views.course_info_updates', name='course_info_json'), url(r'^(?P[^/]+)/(?P[^/]+)/settings-details/(?P[^/]+)$', 'contentstore.views.get_course_settings', name='course_settings'), url(r'^(?P[^/]+)/(?P[^/]+)/settings-grading/(?P[^/]+)$', 'contentstore.views.course_config_graders_page', name='course_settings'), url(r'^(?P[^/]+)/(?P[^/]+)/settings-details/(?P[^/]+)/section/(?P
            [^/]+).*$', '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' From ae55cd7533d178cffaa2eb3c57d190d9341ece6f Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 21 Mar 2013 09:40:57 -0400 Subject: [PATCH 5/5] One more unit test for json not containing expected fields. --- .../contentstore/tests/test_course_updates.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_updates.py b/cms/djangoapps/contentstore/tests/test_course_updates.py index 29f387b3e6..38608ee94d 100644 --- a/cms/djangoapps/contentstore/tests/test_course_updates.py +++ b/cms/djangoapps/contentstore/tests/test_course_updates.py @@ -63,10 +63,18 @@ class CourseUpdateTest(CourseTestCase): 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'}) + 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'}