From f6731342de5b7bea22d9b450b484a53c5234e4e2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 13 Dec 2013 07:50:33 -0500 Subject: [PATCH] Make Studio load XBlock fragment js and css on the client-side [LMS-1421][LMS-1517] --- .../contentstore/tests/test_contentstore.py | 9 ++- cms/djangoapps/contentstore/tests/utils.py | 7 ++ cms/djangoapps/contentstore/views/item.py | 80 +++++++++++++------ cms/djangoapps/contentstore/views/preview.py | 12 +-- .../coffee/spec/views/module_edit_spec.coffee | 57 ++++++++++++- .../coffee/src/views/module_edit.coffee | 34 +++++++- cms/static/sass/elements/_xmodules.scss | 2 +- cms/static/sass/views/_static-pages.scss | 6 +- cms/static/sass/views/_unit.scss | 2 +- .../coffee/spec/xblock/core_spec.coffee | 19 +++-- .../coffee/spec/xblock/runtime.v1_spec.coffee | 6 +- lms/lib/xblock/test/test_runtime.py | 30 ++++--- 12 files changed, 197 insertions(+), 67 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index d25ffe0376..d28a3ab3de 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1,7 +1,8 @@ #pylint: disable=E1101 -import shutil +import json import mock +import shutil from textwrap import dedent @@ -503,7 +504,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): This verifies that a video caption url is as we expect it to be """ resp = self._test_preview(Location('i4x', 'edX', 'toy', 'video', 'sample_video', None)) - self.assertContains(resp, 'data-caption-asset-path="/c4x/edX/toy/asset/subs_"') + self.assertEquals(resp.status_code, 200) + content = json.loads(resp.content) + self.assertIn('data-caption-asset-path="/c4x/edX/toy/asset/subs_"', content['html']) def _test_preview(self, location): """ Preview test case. """ @@ -514,7 +517,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): locator = loc_mapper().translate_location( course_items[0].location.course_id, location, True, True ) - resp = self.client.get_html(locator.url_reverse('xblock')) + resp = self.client.get_fragment(locator.url_reverse('xblock')) self.assertEqual(resp.status_code, 200) # TODO: uncomment when preview no longer has locations being returned. # _test_no_locations(self, resp) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 19d1f174d2..39acda3d8a 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -57,6 +57,13 @@ class AjaxEnabledTestClient(Client): """ return self.get(path, data or {}, follow, HTTP_ACCEPT="application/json", **extra) + def get_fragment(self, path, data=None, follow=False, **extra): + """ + Convenience method for client.get which sets the accept type to application/x-fragment+json + """ + return self.get(path, data or {}, follow, HTTP_ACCEPT="application/x-fragment+json", **extra) + + @override_settings(MODULESTORE=TEST_MODULESTORE) class CourseTestCase(ModuleStoreTestCase): diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 725f394d14..8d80d9e0bb 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -1,27 +1,31 @@ """Views for items (modules).""" +import hashlib import logging from uuid import uuid4 +from collections import OrderedDict from functools import partial from static_replace import replace_static_urls from xmodule_modifiers import wrap_xblock +from django.conf import settings from django.core.exceptions import PermissionDenied from django.contrib.auth.decorators import login_required -from django.http import HttpResponseBadRequest +from django.http import HttpResponseBadRequest, HttpResponse from django.utils.translation import ugettext as _ from django.views.decorators.http import require_http_methods from xblock.fields import Scope +from xblock.fragment import Fragment from xblock.core import XBlock +import xmodule.x_module from xmodule.modulestore.django import modulestore, loc_mapper from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.locator import BlockUsageLocator from xmodule.modulestore import Location -from xmodule.x_module import prefer_xmodules from util.json_request import expect_json, JsonResponse from util.string_utils import str_to_bool @@ -32,8 +36,8 @@ from ..utils import get_modulestore from .access import has_course_access from .helpers import _xmodule_recurse -from preview import handler_prefix, get_preview_html -from edxmako.shortcuts import render_to_response, render_to_string +from contentstore.views.preview import get_preview_fragment +from edxmako.shortcuts import render_to_string from models.settings.course_grading import CourseGradingModel from cms.lib.xblock.runtime import handler_url @@ -50,6 +54,16 @@ CREATE_IF_NOT_FOUND = ['course_info'] xmodule.x_module.descriptor_global_handler_url = handler_url +def hash_resource(resource): + """ + Hash a :class:`xblock.fragment.FragmentResource + """ + md5 = hashlib.md5() + for data in resource: + md5.update(data) + return md5.hexdigest() + + # pylint: disable=unused-argument @require_http_methods(("DELETE", "GET", "PUT", "POST")) @login_required @@ -95,7 +109,42 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid old_location = loc_mapper().translate_locator_to_location(locator) if request.method == 'GET': - if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + accept_header = request.META.get('HTTP_ACCEPT', 'application/json') + + if 'application/x-fragment+json' in accept_header: + component = modulestore().get_item(old_location) + + # Wrap the generated fragment in the xmodule_editor div so that the javascript + # can bind to it correctly + component.runtime.wrappers.append(partial(wrap_xblock, 'StudioRuntime')) + + try: + editor_fragment = component.render('studio_view') + # catch exceptions indiscriminately, since after this point they escape the + # dungeon and surface as uneditable, unsaveable, and undeletable + # component-goblins. + except Exception as exc: # pylint: disable=W0703 + log.debug("Unable to render studio_view for %r", component, exc_info=True) + editor_fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)})) + + modulestore().save_xmodule(component) + + preview_fragment = get_preview_fragment(request, component) + + hashed_resources = OrderedDict() + for resource in editor_fragment.resources + preview_fragment.resources: + hashed_resources[hash_resource(resource)] = resource + + return JsonResponse({ + 'html': render_to_string('component.html', { + 'preview': preview_fragment.content, + 'editor': editor_fragment.content, + 'label': component.display_name or component.scope_ids.block_type, + }), + 'resources': hashed_resources.items() + }) + + elif 'application/json' in accept_header: fields = request.REQUEST.get('fields', '').split(',') if 'graderType' in fields: # right now can't combine output of this w/ output of _get_module_info, but worthy goal @@ -104,25 +153,8 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid rsp = _get_module_info(locator) return JsonResponse(rsp) else: - component = modulestore().get_item(old_location) - # Wrap the generated fragment in the xmodule_editor div so that the javascript - # can bind to it correctly - component.runtime.wrappers.append(partial(wrap_xblock, handler_prefix)) + return HttpResponse(status=406) - try: - content = component.render('studio_view').content - # catch exceptions indiscriminately, since after this point they escape the - # dungeon and surface as uneditable, unsaveable, and undeletable - # component-goblins. - except Exception as exc: # pylint: disable=W0703 - log.debug("Unable to render studio_view for %r", component, exc_info=True) - content = render_to_string('html_error.html', {'message': str(exc)}) - - return render_to_response('component.html', { - 'preview': get_preview_html(request, component), - 'editor': content, - 'label': component.display_name or component.category, - }) elif request.method == 'DELETE': delete_children = str_to_bool(request.REQUEST.get('recurse', 'False')) delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', 'False')) @@ -288,7 +320,7 @@ def _create_item(request): data = None template_id = request.json.get('boilerplate') if template_id is not None: - clz = XBlock.load_class(category, select=prefer_xmodules) + clz = parent.runtime.load_block_type(category) if clz is not None: template = clz.get_template(template_id) if template is not None: diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 732323e924..53de39dcf2 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -1,11 +1,12 @@ import logging +import hashlib from functools import partial from django.conf import settings from django.core.urlresolvers import reverse from django.http import Http404, HttpResponseBadRequest from django.contrib.auth.decorators import login_required -from edxmako.shortcuts import render_to_response, render_to_string +from edxmako.shortcuts import render_to_string from xmodule_modifiers import replace_static_urls, wrap_xblock from xmodule.error_module import ErrorDescriptor @@ -15,6 +16,7 @@ from xmodule.x_module import ModuleSystem from xblock.runtime import KvsFieldData from xblock.django.request import webob_to_django_response, django_to_webob_request from xblock.exceptions import NoSuchHandlerError +from xblock.fragment import Fragment from lms.lib.xblock.field_data import LmsFieldData from lms.lib.xblock.runtime import quote_slashes, unquote_slashes @@ -143,15 +145,15 @@ def _load_preview_module(request, descriptor): return descriptor -def get_preview_html(request, descriptor): +def get_preview_fragment(request, descriptor): """ Returns the HTML returned by the XModule's student_view, specified by the descriptor and idx. """ module = _load_preview_module(request, descriptor) try: - content = module.render("student_view").content + fragment = module.render("student_view") except Exception as exc: # pylint: disable=W0703 log.debug("Unable to render student_view for %r", module, exc_info=True) - content = render_to_string('html_error.html', {'message': str(exc)}) - return content + fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)})) + return fragment diff --git a/cms/static/coffee/spec/views/module_edit_spec.coffee b/cms/static/coffee/spec/views/module_edit_spec.coffee index 36716668d3..e9e3db2a29 100644 --- a/cms/static/coffee/spec/views/module_edit_spec.coffee +++ b/cms/static/coffee/spec/views/module_edit_spec.coffee @@ -1,4 +1,4 @@ -define ["coffee/src/views/module_edit", "js/models/module_info", "xmodule"], (ModuleEdit, ModuleModel) -> +define ["jquery", "coffee/src/views/module_edit", "js/models/module_info", "xmodule"], ($, ModuleEdit, ModuleModel) -> describe "ModuleEdit", -> beforeEach -> @@ -24,7 +24,7 @@ define ["coffee/src/views/module_edit", "js/models/module_info", "xmodule"], (Mo """ - spyOn($.fn, 'load').andReturn(@moduleData) + spyOn($, 'ajax').andReturn(@moduleData) @moduleEdit = new ModuleEdit( el: $(".component") @@ -56,14 +56,63 @@ define ["coffee/src/views/module_edit", "js/models/module_info", "xmodule"], (Mo beforeEach -> spyOn(@moduleEdit, 'loadDisplay') spyOn(@moduleEdit, 'delegateEvents') + spyOn($.fn, 'append') + spyOn($, 'getScript') + + window.loadedXBlockResources = undefined + @moduleEdit.render() + $.ajax.mostRecentCall.args[0].success( + html: '
Response html
' + resources: [ + ['hash1', {kind: 'text', mimetype: 'text/css', data: 'inline-css'}], + ['hash2', {kind: 'url', mimetype: 'text/css', data: 'css-url'}], + ['hash3', {kind: 'text', mimetype: 'application/javascript', data: 'inline-js'}], + ['hash4', {kind: 'url', mimetype: 'application/javascript', data: 'js-url'}], + ['hash5', {placement: 'head', mimetype: 'text/html', data: 'head-html'}], + ['hash6', {placement: 'not-head', mimetype: 'text/html', data: 'not-head-html'}], + ] + ) it "loads the module preview and editor via ajax on the view element", -> - expect(@moduleEdit.$el.load).toHaveBeenCalledWith("/xblock/#{@moduleEdit.model.id}", jasmine.any(Function)) - @moduleEdit.$el.load.mostRecentCall.args[1]() + expect($.ajax).toHaveBeenCalledWith( + url: "/xblock/#{@moduleEdit.model.id}" + type: "GET" + headers: + Accept: 'application/x-fragment+json' + success: jasmine.any(Function) + ) expect(@moduleEdit.loadDisplay).toHaveBeenCalled() expect(@moduleEdit.delegateEvents).toHaveBeenCalled() + it "loads inline css from fragments", -> + expect($('head').append).toHaveBeenCalledWith("") + + it "loads css urls from fragments", -> + expect($('head').append).toHaveBeenCalledWith("") + + it "loads inline js from fragments", -> + expect($('head').append).toHaveBeenCalledWith("") + + it "loads js urls from fragments", -> + expect($.getScript).toHaveBeenCalledWith("js-url") + + it "loads head html", -> + expect($('head').append).toHaveBeenCalledWith("head-html") + + it "doesn't load body html", -> + expect($.fn.append).not.toHaveBeenCalledWith('not-head-html') + + it "doesn't reload resources", -> + count = $('head').append.callCount + $.ajax.mostRecentCall.args[0].success( + html: '
Response html 2
' + resources: [ + ['hash1', {kind: 'text', mimetype: 'text/css', data: 'inline-css'}], + ] + ) + expect($('head').append.callCount).toBe(count) + describe "loadDisplay", -> beforeEach -> spyOn(XBlock, 'initializeBlock') diff --git a/cms/static/coffee/src/views/module_edit.coffee b/cms/static/coffee/src/views/module_edit.coffee index a30fbe9db1..3ef164d5d1 100644 --- a/cms/static/coffee/src/views/module_edit.coffee +++ b/cms/static/coffee/src/views/module_edit.coffee @@ -75,9 +75,37 @@ define ["backbone", "jquery", "underscore", "gettext", "xblock/runtime.v1", render: -> if @model.id - @$el.load(@model.url(), => - @loadDisplay() - @delegateEvents() + $.ajax( + url: @model.url() + type: 'GET' + headers: + Accept: 'application/x-fragment+json' + success: (data) => + @$el.html(data.html) + + for value in data.resources + do (value) => + hash = value[0] + if not window.loadedXBlockResources? + window.loadedXBlockResources = [] + + if hash not in window.loadedXBlockResources + resource = value[1] + switch resource.mimetype + when "text/css" + switch resource.kind + when "text" then $('head').append("") + when "url" then $('head').append("") + when "application/javascript" + switch resource.kind + when "text" then $('head').append("") + when "url" then $.getScript(resource.data) + when "text/html" + switch resource.placement + when "head" then $('head').append(resource.data) + window.loadedXBlockResources.push(hash) + @loadDisplay() + @delegateEvents() ) clickSaveButton: (event) => diff --git a/cms/static/sass/elements/_xmodules.scss b/cms/static/sass/elements/_xmodules.scss index 8ffccfef2d..c889f0fde4 100644 --- a/cms/static/sass/elements/_xmodules.scss +++ b/cms/static/sass/elements/_xmodules.scss @@ -16,7 +16,7 @@ .xmodule_VideoModule { // display mode - &.xmodule_display { + &.xblock-student_view { // full screen .video-controls .add-fullscreen { diff --git a/cms/static/sass/views/_static-pages.scss b/cms/static/sass/views/_static-pages.scss index 5acd9f536d..dd2a897d2f 100644 --- a/cms/static/sass/views/_static-pages.scss +++ b/cms/static/sass/views/_static-pages.scss @@ -183,16 +183,16 @@ border-left: 1px solid $mediumGrey; border-right: 1px solid $mediumGrey; - .xmodule_display { + .xblock-student_view { display: none; } } - .new .xmodule_display { + .new .xblock-student_view { background: $yellow; } - .xmodule_display { + .xblock-student_view { @include transition(background-color $tmg-s3 linear 0s); padding: 20px 20px 22px; font-size: 24px; diff --git a/cms/static/sass/views/_unit.scss b/cms/static/sass/views/_unit.scss index 1cd74e12a1..5f8b9a897d 100644 --- a/cms/static/sass/views/_unit.scss +++ b/cms/static/sass/views/_unit.scss @@ -420,7 +420,7 @@ body.course.unit,.view-unit { } } - .xmodule_display { + .xblock-student_view { padding: 2*$baseline $baseline $baseline; overflow-x: auto; diff --git a/common/static/coffee/spec/xblock/core_spec.coffee b/common/static/coffee/spec/xblock/core_spec.coffee index 3db4190790..efc6d15807 100644 --- a/common/static/coffee/spec/xblock/core_spec.coffee +++ b/common/static/coffee/spec/xblock/core_spec.coffee @@ -2,9 +2,9 @@ describe "XBlock", -> beforeEach -> setFixtures """
-
+
-
+
@@ -13,8 +13,11 @@ describe "XBlock", -> describe "initializeBlock", -> beforeEach -> - XBlock.runtime.vA = jasmine.createSpy().andReturn('runtimeA') - XBlock.runtime.vZ = jasmine.createSpy().andReturn('runtimeZ') + window.TestRuntime = {} + @runtimeA = {name: 'runtimeA'} + @runtimeZ = {name: 'runtimeZ'} + TestRuntime.vA = jasmine.createSpy().andReturn(@runtimeA) + TestRuntime.vZ = jasmine.createSpy().andReturn(@runtimeZ) window.initFnA = jasmine.createSpy() window.initFnZ = jasmine.createSpy() @@ -28,12 +31,12 @@ describe "XBlock", -> @missingInitBlock = XBlock.initializeBlock($('#missing-init')[0]) it "loads the right runtime version", -> - expect(XBlock.runtime.vA).toHaveBeenCalledWith($('#vA')[0], @fakeChildren) - expect(XBlock.runtime.vZ).toHaveBeenCalledWith($('#vZ')[0], @fakeChildren) + expect(TestRuntime.vA).toHaveBeenCalledWith($('#vA')[0], @fakeChildren) + expect(TestRuntime.vZ).toHaveBeenCalledWith($('#vZ')[0], @fakeChildren) it "loads the right init function", -> - expect(window.initFnA).toHaveBeenCalledWith('runtimeA', $('#vA')[0]) - expect(window.initFnZ).toHaveBeenCalledWith('runtimeZ', $('#vZ')[0]) + expect(window.initFnA).toHaveBeenCalledWith(@runtimeA, $('#vA')[0]) + expect(window.initFnZ).toHaveBeenCalledWith(@runtimeZ, $('#vZ')[0]) it "loads when missing versions", -> expect(@missingVersionBlock.element).toBe($('#missing-version')) diff --git a/common/static/coffee/spec/xblock/runtime.v1_spec.coffee b/common/static/coffee/spec/xblock/runtime.v1_spec.coffee index e9c71e9b32..f13d69154c 100644 --- a/common/static/coffee/spec/xblock/runtime.v1_spec.coffee +++ b/common/static/coffee/spec/xblock/runtime.v1_spec.coffee @@ -1,4 +1,4 @@ -describe "XBlock.runtime.v1", -> +describe "XBlock.Runtime.v1", -> beforeEach -> setFixtures """
@@ -10,9 +10,7 @@ describe "XBlock.runtime.v1", -> @element = $('.xblock')[0] - @runtime = XBlock.runtime.v1(@element, @children) - it "provides a handler url", -> - expect(@runtime.handlerUrl(@element, 'foo')).toBe('/xblock/fake-usage-id/handler/foo') + @runtime = new XBlock.Runtime.v1(@element, @children) it "provides a list of children", -> expect(@runtime.children).toBe(@children) diff --git a/lms/lib/xblock/test/test_runtime.py b/lms/lib/xblock/test/test_runtime.py index 5b3efc171c..14987a87d3 100644 --- a/lms/lib/xblock/test/test_runtime.py +++ b/lms/lib/xblock/test/test_runtime.py @@ -6,7 +6,7 @@ from ddt import ddt, data from mock import Mock from unittest import TestCase from urlparse import urlparse -from lms.lib.xblock.runtime import quote_slashes, unquote_slashes, handler_url +from lms.lib.xblock.runtime import quote_slashes, unquote_slashes, LmsModuleSystem TEST_STRINGS = [ '', @@ -41,23 +41,31 @@ class TestHandlerUrl(TestCase): def setUp(self): self.block = Mock() self.course_id = "org/course/run" + self.runtime = LmsModuleSystem( + static_url='/static', + track_function=Mock(), + get_module=Mock(), + render_template=Mock(), + replace_urls=str, + course_id=self.course_id, + ) def test_trailing_characters(self): - self.assertFalse(handler_url(self.course_id, self.block, 'handler').endswith('?')) - self.assertFalse(handler_url(self.course_id, self.block, 'handler').endswith('/')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler').endswith('?')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler').endswith('/')) - self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix').endswith('?')) - self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix').endswith('/')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler', 'suffix').endswith('?')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler', 'suffix').endswith('/')) - self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix', 'query').endswith('?')) - self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix', 'query').endswith('/')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler', 'suffix', 'query').endswith('?')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler', 'suffix', 'query').endswith('/')) - self.assertFalse(handler_url(self.course_id, self.block, 'handler', query='query').endswith('?')) - self.assertFalse(handler_url(self.course_id, self.block, 'handler', query='query').endswith('/')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler', query='query').endswith('?')) + self.assertFalse(self.runtime.handler_url(self.block, 'handler', query='query').endswith('/')) def _parsed_query(self, query_string): """Return the parsed query string from a handler_url generated with the supplied query_string""" - return urlparse(handler_url(self.course_id, self.block, 'handler', query=query_string)).query + return urlparse(self.runtime.handler_url(self.block, 'handler', query=query_string)).query def test_query_string(self): self.assertIn('foo=bar', self._parsed_query('foo=bar')) @@ -66,7 +74,7 @@ class TestHandlerUrl(TestCase): def _parsed_path(self, handler_name='handler', suffix=''): """Return the parsed path from a handler_url with the supplied handler_name and suffix""" - return urlparse(handler_url(self.course_id, self.block, handler_name, suffix=suffix)).path + return urlparse(self.runtime.handler_url(self.block, handler_name, suffix=suffix)).path def test_suffix(self): self.assertTrue(self._parsed_path(suffix="foo").endswith('foo'))