diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8ed5a0114a..4ba7c6b9c9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,9 @@ the top. Include a label indicating the component affected. Studio: Move Peer Assessment into advanced problems menu. +Studio: Support creation and editing of split_test instances (Content Experiments) +entirely in Studio. STUD-1658. + Blades: Add context-aware video index. BLD-933 Blades: Fix bug with incorrect link format and redirection. BLD-1049 diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index f6ef0ab61c..46ef237772 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -23,6 +23,7 @@ import xmodule from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, DuplicateItemError from xmodule.modulestore.inheritance import own_metadata +from xmodule.x_module import PREVIEW_VIEWS, STUDIO_VIEW from util.json_request import expect_json, JsonResponse from util.string_utils import str_to_bool @@ -180,15 +181,15 @@ def xblock_view_handler(request, usage_key_string, view_name): xblock = store.get_item(usage_key) is_read_only = _is_xblock_read_only(xblock) container_views = ['container_preview', 'reorderable_container_child_preview'] - unit_views = ['student_view'] + unit_views = PREVIEW_VIEWS # wrap the generated fragment in the xmodule_editor div so that the javascript # can bind to it correctly xblock.runtime.wrappers.append(partial(wrap_xblock, 'StudioRuntime', usage_id_serializer=unicode)) - if view_name == 'studio_view': + if view_name == STUDIO_VIEW: try: - fragment = xblock.render('studio_view') + fragment = xblock.render(STUDIO_VIEW) # catch exceptions indiscriminately, since after this point they escape the # dungeon and surface as uneditable, unsaveable, and undeletable # component-goblins. @@ -213,7 +214,6 @@ def xblock_view_handler(request, usage_key_string, view_name): # Note: this special case logic can be removed once the unit page is replaced # with the new container view. context = { - 'runtime_type': 'studio', 'container_view': is_container_view, 'read_only': is_read_only, 'root_xblock': xblock if (view_name == 'container_preview') else None, diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 5b0ef4fa48..8401935c3d 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -10,6 +10,7 @@ from django.contrib.auth.decorators import login_required from edxmako.shortcuts import render_to_string from xmodule_modifiers import replace_static_urls, wrap_xblock, wrap_fragment +from xmodule.x_module import PREVIEW_VIEWS, STUDENT_VIEW, AUTHOR_VIEW from xmodule.error_module import ErrorDescriptor from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import modulestore, ModuleI18nService @@ -21,13 +22,14 @@ from xblock.exceptions import NoSuchHandlerError from xblock.fragment import Fragment from lms.lib.xblock.field_data import LmsFieldData +from cms.lib.xblock.field_data import CmsFieldData from cms.lib.xblock.runtime import local_resource_url from util.sandboxing import can_execute_unsafe_code import static_replace from .session_kv_store import SessionKeyValueStore -from .helpers import render_from_lms, xblock_has_own_studio_page +from .helpers import render_from_lms from contentstore.views.access import get_user_role @@ -143,15 +145,20 @@ def _preview_module_system(request, descriptor): def _load_preview_module(request, descriptor): """ - Return a preview XModule instantiated from the supplied descriptor. + Return a preview XModule instantiated from the supplied descriptor. Will use mutable fields + if XModule supports an author_view. Otherwise, will use immutable fields and student_view. request: The active django request descriptor: An XModuleDescriptor """ student_data = KvsFieldData(SessionKeyValueStore(request)) + if _has_author_view(descriptor): + field_data = CmsFieldData(descriptor._field_data, student_data) # pylint: disable=protected-access + else: + field_data = LmsFieldData(descriptor._field_data, student_data) # pylint: disable=protected-access descriptor.bind_for_student( _preview_module_system(request, descriptor), - LmsFieldData(descriptor._field_data, student_data), # pylint: disable=protected-access + field_data ) return descriptor @@ -169,7 +176,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): Wraps the results of rendering an XBlock view in a div which adds a header and Studio action buttons. """ # Only add the Studio wrapper when on the container page. The unit page will remain as is for now. - if context.get('container_view', None) and view == 'student_view': + if context.get('container_view', None) and view in PREVIEW_VIEWS: root_xblock = context.get('root_xblock') is_root = root_xblock and xblock.location == root_xblock.location is_reorderable = _is_xblock_reorderable(xblock, context) @@ -187,14 +194,25 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): def get_preview_fragment(request, descriptor, context): """ - Returns the HTML returned by the XModule's student_view, + Returns the HTML returned by the XModule's student_view or author_view (if available), specified by the descriptor and idx. """ module = _load_preview_module(request, descriptor) + preview_view = AUTHOR_VIEW if _has_author_view(module) else STUDENT_VIEW + try: - fragment = module.render("student_view", context) + fragment = module.render(preview_view, context) except Exception as exc: # pylint: disable=W0703 - log.warning("Unable to render student_view for %r", module, exc_info=True) + log.warning("Unable to render %s for %r", preview_view, module, exc_info=True) fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)})) return fragment + + +def _has_author_view(descriptor): + """ + Returns True if the xmodule linked to the descriptor supports "author_view". + + If False, "student_view" and LmsFieldData should be used. + """ + return getattr(descriptor, 'has_author_view', False) diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index ff26ba8176..ebadd986f9 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -22,6 +22,7 @@ from student.tests.factories import UserFactory from xmodule.capa_module import CapaDescriptor from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.x_module import STUDIO_VIEW, STUDENT_VIEW from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locations import Location from xmodule.partitions.partitions import Group, UserPartition @@ -106,7 +107,7 @@ class GetItem(ItemTest): self.assertNotIn('wrapper-xblock', html) # Verify that the header and article tags are still added - self.assertIn('
', html) + self.assertIn('
', html) self.assertIn('
', html) def test_get_container_fragment(self): @@ -122,7 +123,7 @@ class GetItem(ItemTest): # Verify that the Studio nesting wrapper has been added self.assertIn('level-nesting', html) - self.assertIn('
', html) + self.assertIn('
', html) self.assertIn('
', html) # Verify that the Studio element wrapper has been added @@ -712,12 +713,12 @@ class TestEditItem(ItemTest): self.assertNotEqual(draft.data, published.data) # Get problem by 'xblock_handler' - view_url = reverse_usage_url("xblock_view_handler", self.problem_usage_key, {"view_name": "student_view"}) + view_url = reverse_usage_url("xblock_view_handler", self.problem_usage_key, {"view_name": STUDENT_VIEW}) resp = self.client.get(view_url, HTTP_ACCEPT='application/json') self.assertEqual(resp.status_code, 200) # Activate the editing view - view_url = reverse_usage_url("xblock_view_handler", self.problem_usage_key, {"view_name": "studio_view"}) + view_url = reverse_usage_url("xblock_view_handler", self.problem_usage_key, {"view_name": STUDIO_VIEW}) resp = self.client.get(view_url, HTTP_ACCEPT='application/json') self.assertEqual(resp.status_code, 200) @@ -811,7 +812,15 @@ class TestEditSplitModule(ItemTest): self.assertEqual(partition_id, split_test.user_partition_id) return split_test - def test_split_create_groups(self): + def _assert_children(self, expected_number): + """ + Verifies the number of children of the split_test instance. + """ + split_test = self.get_item_from_modulestore(self.split_test_usage_key, True) + self.assertEqual(expected_number, len(split_test.children)) + return split_test + + def test_create_groups(self): """ Test that verticals are created for the experiment groups when a spit test module is edited. @@ -833,16 +842,14 @@ class TestEditSplitModule(ItemTest): self.assertEqual("alpha", vertical_0.display_name) self.assertEqual("beta", vertical_1.display_name) - # Verify that the group_id_to child mapping is correct. + # Verify that the group_id_to_child mapping is correct. self.assertEqual(2, len(split_test.group_id_to_child)) - split_test.group_id_to_child['0'] = vertical_0.location - split_test.group_id_to_child['1'] = vertical_1.location + self.assertEqual(vertical_0.location, split_test.group_id_to_child['0']) + self.assertEqual(vertical_1.location, split_test.group_id_to_child['1']) - def test_split_change_user_partition_id(self): + def test_change_user_partition_id(self): """ Test what happens when the user_partition_id is changed to a different experiment. - - This is not currently supported by the Studio UI. """ # Set to first experiment. split_test = self._update_partition_id(0) @@ -852,21 +859,23 @@ class TestEditSplitModule(ItemTest): # Set to second experiment split_test = self._update_partition_id(1) - # We don't currently remove existing children. + # We don't remove existing children. self.assertEqual(5, len(split_test.children)) + self.assertEqual(initial_vertical_0_location, split_test.children[0]) + self.assertEqual(initial_vertical_1_location, split_test.children[1]) vertical_0 = self.get_item_from_modulestore(split_test.children[2], True) vertical_1 = self.get_item_from_modulestore(split_test.children[3], True) vertical_2 = self.get_item_from_modulestore(split_test.children[4], True) # Verify that the group_id_to child mapping is correct. self.assertEqual(3, len(split_test.group_id_to_child)) - split_test.group_id_to_child['0'] = vertical_0.location - split_test.group_id_to_child['1'] = vertical_1.location - split_test.group_id_to_child['2'] = vertical_2.location + self.assertEqual(vertical_0.location, split_test.group_id_to_child['0']) + self.assertEqual(vertical_1.location, split_test.group_id_to_child['1']) + self.assertEqual(vertical_2.location, split_test.group_id_to_child['2']) self.assertNotEqual(initial_vertical_0_location, vertical_0.location) self.assertNotEqual(initial_vertical_1_location, vertical_1.location) - def test_split_same_user_partition_id(self): + def test_change_same_user_partition_id(self): """ Test that nothing happens when the user_partition_id is set to the same value twice. """ @@ -880,7 +889,7 @@ class TestEditSplitModule(ItemTest): self.assertEqual(2, len(split_test.children)) self.assertEqual(initial_group_id_to_child, split_test.group_id_to_child) - def test_split_non_existent_user_partition_id(self): + def test_change_non_existent_user_partition_id(self): """ Test that nothing happens when the user_partition_id is set to a value that doesn't exist. @@ -896,6 +905,80 @@ class TestEditSplitModule(ItemTest): self.assertEqual(2, len(split_test.children)) self.assertEqual(initial_group_id_to_child, split_test.group_id_to_child) + def test_delete_children(self): + """ + Test that deleting a child in the group_id_to_child map updates the map. + + Also test that deleting a child not in the group_id_to_child_map behaves properly. + """ + # Set to first experiment. + self._update_partition_id(0) + split_test = self._assert_children(2) + vertical_1_usage_key = split_test.children[1] + + # Add an extra child to the split_test + resp = self.create_xblock(category='html', parent_usage_key=self.split_test_usage_key) + extra_child_usage_key = self.response_usage_key(resp) + self._assert_children(3) + + # Remove the first child (which is part of the group configuration). + resp = self.client.ajax_post( + self.split_test_update_url, + data={'children': [unicode(vertical_1_usage_key), unicode(extra_child_usage_key)]} + ) + self.assertEqual(resp.status_code, 200) + split_test = self._assert_children(2) + + # Check that group_id_to_child was updated appropriately + group_id_to_child = split_test.group_id_to_child + self.assertEqual(1, len(group_id_to_child)) + self.assertEqual(vertical_1_usage_key, group_id_to_child['1']) + + # Remove the "extra" child and make sure that group_id_to_child did not change. + resp = self.client.ajax_post( + self.split_test_update_url, + data={'children': [unicode(vertical_1_usage_key)]} + ) + self.assertEqual(resp.status_code, 200) + split_test = self._assert_children(1) + self.assertEqual(group_id_to_child, split_test.group_id_to_child) + + def test_add_groups(self): + """ + Test the "fix up behavior" when groups are missing (after a group is added to a group configuration). + + This test actually belongs over in common, but it relies on a mutable modulestore. + TODO: move tests that can go over to common after the mixed modulestore work is done. # pylint: disable=fixme + """ + # Set to first group configuration. + split_test = self._update_partition_id(0) + + # Add a group to the first group configuration. + split_test.user_partitions = [ + UserPartition( + 0, 'first_partition', 'First Partition', + [Group("0", 'alpha'), Group("1", 'beta'), Group("2", 'pie')] + ) + ] + self.store.update_item(split_test, self.user.id) + + # group_id_to_child and children have not changed yet. + split_test = self._assert_children(2) + group_id_to_child = split_test.group_id_to_child + self.assertEqual(2, len(group_id_to_child)) + + # Call add_missing_groups method to add the missing group. + split_test.add_missing_groups(None) + split_test = self._assert_children(3) + self.assertNotEqual(group_id_to_child, split_test.group_id_to_child) + group_id_to_child = split_test.group_id_to_child + self.assertEqual(split_test.children[2], group_id_to_child["2"]) + + # Call add_missing_groups again -- it should be a no-op. + split_test.add_missing_groups(None) + split_test = self._assert_children(3) + self.assertEqual(group_id_to_child, split_test.group_id_to_child) + @ddt.ddt class TestComponentHandler(TestCase): diff --git a/cms/djangoapps/contentstore/views/tests/test_tabs.py b/cms/djangoapps/contentstore/views/tests/test_tabs.py index f938ba8880..2d84d41269 100644 --- a/cms/djangoapps/contentstore/views/tests/test_tabs.py +++ b/cms/djangoapps/contentstore/views/tests/test_tabs.py @@ -4,7 +4,7 @@ import json from contentstore.views import tabs from contentstore.tests.utils import CourseTestCase from django.test import TestCase -from xmodule.modulestore.django import loc_mapper +from xmodule.x_module import STUDENT_VIEW from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.tabs import CourseTabList, WikiTab from contentstore.utils import reverse_course_url @@ -178,7 +178,7 @@ class TabsPageTests(CourseTestCase): """ Verify that the static tab renders itself with the correct HTML """ - preview_url = '/xblock/{}/student_view'.format(self.test_tab.location) + preview_url = '/xblock/{}/{}'.format(self.test_tab.location, STUDENT_VIEW) resp = self.client.get(preview_url, HTTP_ACCEPT='application/json') self.assertEqual(resp.status_code, 200) diff --git a/cms/djangoapps/contentstore/views/tests/test_unit_page.py b/cms/djangoapps/contentstore/views/tests/test_unit_page.py index 569d7ab9d5..695ca090b1 100644 --- a/cms/djangoapps/contentstore/views/tests/test_unit_page.py +++ b/cms/djangoapps/contentstore/views/tests/test_unit_page.py @@ -5,6 +5,7 @@ Unit tests for the unit page. from contentstore.views.tests.utils import StudioPageTestCase from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import ItemFactory +from xmodule.x_module import STUDENT_VIEW class UnitPageTestCase(StudioPageTestCase): @@ -38,7 +39,7 @@ class UnitPageTestCase(StudioPageTestCase): """ Verify that a public xblock's preview returns the expected HTML. """ - self.validate_preview_html(self.video, 'student_view', + self.validate_preview_html(self.video, STUDENT_VIEW, can_edit=True, can_reorder=True, can_add=False) def test_draft_component_preview_html(self): @@ -47,7 +48,7 @@ class UnitPageTestCase(StudioPageTestCase): """ modulestore('draft').convert_to_draft(self.vertical.location) draft_video = modulestore('draft').convert_to_draft(self.video.location) - self.validate_preview_html(draft_video, 'student_view', + self.validate_preview_html(draft_video, STUDENT_VIEW, can_edit=True, can_reorder=True, can_add=False) def test_public_child_container_preview_html(self): @@ -59,7 +60,7 @@ class UnitPageTestCase(StudioPageTestCase): category='split_test', display_name='Split Test') ItemFactory.create(parent_location=child_container.location, category='html', display_name='grandchild') - self.validate_preview_html(child_container, 'student_view', + self.validate_preview_html(child_container, STUDENT_VIEW, can_reorder=True, can_edit=True, can_add=False) def test_draft_child_container_preview_html(self): @@ -73,5 +74,5 @@ class UnitPageTestCase(StudioPageTestCase): category='html', display_name='grandchild') modulestore('draft').convert_to_draft(self.vertical.location) draft_child_container = modulestore('draft').get_item(child_container.location) - self.validate_preview_html(draft_child_container, 'student_view', + self.validate_preview_html(draft_child_container, STUDENT_VIEW, can_reorder=True, can_edit=True, can_add=False) diff --git a/cms/lib/xblock/field_data.py b/cms/lib/xblock/field_data.py new file mode 100644 index 0000000000..67c5023ee8 --- /dev/null +++ b/cms/lib/xblock/field_data.py @@ -0,0 +1,32 @@ +""" +:class:`~xblock.field_data.FieldData` subclasses used by the CMS +""" + +from xblock.field_data import SplitFieldData +from xblock.fields import Scope + + +class CmsFieldData(SplitFieldData): + """ + A :class:`~xblock.field_data.FieldData` that + reads all UserScope.ONE and UserScope.ALL fields from `student_data` + and all UserScope.NONE fields from `authored_data`. It allows writing to`authored_data`. + """ + def __init__(self, authored_data, student_data): + # Make sure that we don't repeatedly nest CmsFieldData instances + if isinstance(authored_data, CmsFieldData): + authored_data = authored_data._authored_data # pylint: disable=protected-access + + self._authored_data = authored_data + self._student_data = student_data + + super(CmsFieldData, self).__init__({ + Scope.content: authored_data, + Scope.settings: authored_data, + Scope.parent: authored_data, + Scope.children: authored_data, + Scope.user_state_summary: student_data, + Scope.user_state: student_data, + Scope.user_info: student_data, + Scope.preferences: student_data, + }) diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 2bae437547..e55a28dda3 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -233,6 +233,8 @@ define([ "js/spec/views/modals/base_modal_spec", "js/spec/views/modals/edit_xblock_spec", + "js/spec/xblock/cms.runtime.v1_spec", + # these tests are run separately in the cms-squire suite, due to process # isolation issues with Squire.js # "coffee/spec/views/assets_spec" diff --git a/cms/static/coffee/src/xblock/cms.runtime.v1.coffee b/cms/static/coffee/src/xblock/cms.runtime.v1.coffee index 60291c43b8..d38640d091 100644 --- a/cms/static/coffee/src/xblock/cms.runtime.v1.coffee +++ b/cms/static/coffee/src/xblock/cms.runtime.v1.coffee @@ -1,80 +1,96 @@ define [ - "jquery", "xblock/runtime.v1", "URI", "gettext", + "jquery", "backbone", "xblock/runtime.v1", "URI", "gettext", "js/utils/modal", "js/views/feedback_notification" -], ($, XBlock, URI, gettext, ModalUtils, NotificationView) -> - @PreviewRuntime = {} +], ($, Backbone, XBlock, URI, gettext, ModalUtils, NotificationView) -> - class PreviewRuntime.v1 extends XBlock.Runtime.v1 + @BaseRuntime = {} + + class BaseRuntime.v1 extends XBlock.Runtime.v1 handlerUrl: (element, handlerName, suffix, query, thirdparty) -> - uri = URI("/preview/xblock").segment($(element).data('usage-id')) + uri = URI(@handlerPrefix).segment($(element).data('usage-id')) .segment('handler') .segment(handlerName) if suffix? then uri.segment(suffix) if query? then uri.search(query) uri.toString() - @StudioRuntime = {} - - class StudioRuntime.v1 extends XBlock.Runtime.v1 constructor: () -> super() - @savingNotification = new NotificationView.Mini - title: gettext('Saving…') - @alert = new NotificationView.Error - title: "OpenAssessment Save Error", - closeIcon: false, - shown: false + @dispatcher = _.clone(Backbone.Events) + @listenTo('save', @_handleSave) + @listenTo('cancel', @_handleCancel) + @listenTo('error', @_handleError) + @listenTo('modal-shown', (data) -> + @modal = data) + @listenTo('modal-hidden', () -> + @modal = null) + @listenTo('page-shown', (data) -> + @page = data) - handlerUrl: (element, handlerName, suffix, query, thirdparty) -> - uri = URI("/xblock").segment($(element).data('usage-id')) - .segment('handler') - .segment(handlerName) - if suffix? then uri.segment(suffix) - if query? then uri.search(query) - uri.toString() - - # Notify the Studio client-side runtime so it can update - # the UI in a consistent way. Currently, this is used - # for save / cancel when editing an XBlock. - # Although native XBlocks should handle their own persistence, - # Studio still needs to update the UI in a consistent way - # (showing the "Saving..." notification, closing the modal editing dialog, etc.) + # Notify the Studio client-side runtime of an event so that it can update the UI in a consistent way. notify: (name, data) -> - if name == 'save' - if 'state' of data + @dispatcher.trigger(name, data) - # Starting to save, so show the "Saving..." notification - if data.state == 'start' - @savingNotification.show() + # Listen to a Studio event and invoke the specified callback when it is triggered. + listenTo: (name, callback) -> + @dispatcher.bind(name, callback, this) - # Finished saving, so hide the "Saving..." notification - else if data.state == 'end' - @_hideAlerts() + # Refresh the view for the xblock represented by the specified element. + refreshXBlock: (element) -> + if @page + @page.refreshXBlock(element) - # Notify the modal that the save has completed so that it can hide itself - # and then refresh the xblock. - if @modal - @modal.onSave() + _handleError: (data) -> + message = data.message || data.msg + if message + # TODO: remove 'Open Assessment' specific default title + title = data.title || gettext("OpenAssessment Save Error") + @alert = new NotificationView.Error + title: title + message: message + closeIcon: false + shown: false + @alert.show() - @savingNotification.hide() + _handleSave: (data) -> + # Starting to save, so show a notification + if data.state == 'start' + message = data.message || gettext('Saving…') + @notification = new NotificationView.Mini + title: message + @notification.show() - else if name == 'edit-modal-shown' - @modal = data - - else if name == 'edit-modal-hidden' - @modal = null - - else if name == 'cancel' + # Finished saving, so hide the notification and refresh appropriately + else if data.state == 'end' @_hideAlerts() - if @modal - @modal.cancel() - else if name == 'error' - if 'msg' of data - @alert.options.message = data.msg - @alert.show() + # Notify the modal that the save has completed so that it can hide itself + # and then refresh the xblock. + if @modal and @modal.onSave + @modal.onSave() + # ... else ask it to refresh the newly saved xblock + else if data.element + @refreshXBlock(data.element) + + @notification.hide() + + _handleCancel: () -> + @_hideAlerts() + if @modal + @modal.cancel() + @notify('modal-hidden') _hideAlerts: () -> # Hide any alerts that are being shown - if @alert.options.shown + if @alert && @alert.options.shown @alert.hide() + + @PreviewRuntime = {} + + class PreviewRuntime.v1 extends BaseRuntime.v1 + handlerPrefix: '/preview/xblock' + + @StudioRuntime = {} + + class StudioRuntime.v1 extends BaseRuntime.v1 + handlerPrefix: '/xblock' diff --git a/cms/static/js/spec/views/pages/container_spec.js b/cms/static/js/spec/views/pages/container_spec.js index d30521c4d5..35cb2e4579 100644 --- a/cms/static/js/spec/views/pages/container_spec.js +++ b/cms/static/js/spec/views/pages/container_spec.js @@ -1,6 +1,6 @@ -define(["jquery", "underscore", "js/spec_helpers/create_sinon", "js/spec_helpers/edit_helpers", +define(["jquery", "underscore", "underscore.string", "js/spec_helpers/create_sinon", "js/spec_helpers/edit_helpers", "js/views/feedback_prompt", "js/views/pages/container", "js/models/xblock_info"], - function ($, _, create_sinon, edit_helpers, Prompt, ContainerPage, XBlockInfo) { + function ($, _, str, create_sinon, edit_helpers, Prompt, ContainerPage, XBlockInfo) { describe("ContainerPage", function() { var lastRequest, renderContainerPage, expectComponents, respondWithHtml, @@ -96,7 +96,7 @@ define(["jquery", "underscore", "js/spec_helpers/create_sinon", "js/spec_helpers editButtons.first().click(); // Expect a request to be made to show the studio view for the container - expect(lastRequest().url.startsWith('/xblock/locator-container/studio_view')).toBeTruthy(); + expect(str.startsWith(lastRequest().url, '/xblock/locator-container/studio_view')).toBeTruthy(); create_sinon.respondWithJson(requests, { html: mockContainerXBlockHtml, resources: [] @@ -112,7 +112,7 @@ define(["jquery", "underscore", "js/spec_helpers/create_sinon", "js/spec_helpers expect(edit_helpers.isShowingModal()).toBeFalsy(); // Expect the last request be to refresh the container page - expect(lastRequest().url.startsWith('/xblock/locator-container/container_preview')).toBeTruthy(); + expect(str.startsWith(lastRequest().url, '/xblock/locator-container/container_preview')).toBeTruthy(); create_sinon.respondWithJson(requests, { html: mockUpdatedContainerXBlockHtml, resources: [] @@ -149,7 +149,7 @@ define(["jquery", "underscore", "js/spec_helpers/create_sinon", "js/spec_helpers expect(editButtons.length).toBe(6); editButtons[0].click(); // Make sure that the correct xblock is requested to be edited - expect(lastRequest().url.startsWith('/xblock/locator-component-A1/studio_view')).toBeTruthy(); + expect(str.startsWith(lastRequest().url, '/xblock/locator-component-A1/studio_view')).toBeTruthy(); create_sinon.respondWithJson(requests, { html: mockXBlockEditorHtml, resources: [] diff --git a/cms/static/js/spec/views/unit_spec.js b/cms/static/js/spec/views/unit_spec.js index d8b34ea121..2464fa2653 100644 --- a/cms/static/js/spec/views/unit_spec.js +++ b/cms/static/js/spec/views/unit_spec.js @@ -1,6 +1,6 @@ -define(["jquery", "underscore", "jasmine", "coffee/src/views/unit", "js/models/module_info", +define(["jquery", "underscore.string", "jasmine", "coffee/src/views/unit", "js/models/module_info", "js/spec_helpers/create_sinon", "js/spec_helpers/edit_helpers", "jasmine-stealth"], - function ($, _, jasmine, UnitEditView, ModuleModel, create_sinon, edit_helpers) { + function ($, str, jasmine, UnitEditView, ModuleModel, create_sinon, edit_helpers) { var requests, unitView, initialize, lastRequest, respondWithHtml, verifyComponents, i, mockXBlockEditorHtml = readFixtures('mock/mock-xblock-editor.underscore'); @@ -150,7 +150,7 @@ define(["jquery", "underscore", "jasmine", "coffee/src/views/unit", "js/models/m }); describe("Disabled edit/publish links during ajax call", function() { - var link, i, + var link, draft_states = [ { state: "draft", @@ -204,7 +204,7 @@ define(["jquery", "underscore", "jasmine", "coffee/src/views/unit", "js/models/m expect(editButtons.length).toBe(2); editButtons[1].click(); // Make sure that the correct xblock is requested to be edited - expect(lastRequest().url.startsWith('/xblock/loc_2/studio_view')).toBeTruthy(); + expect(str.startsWith(lastRequest().url, '/xblock/loc_2/studio_view')).toBeTruthy(); create_sinon.respondWithJson(requests, { html: mockXBlockEditorHtml, resources: [] diff --git a/cms/static/js/spec/xblock/cms.runtime.v1_spec.js b/cms/static/js/spec/xblock/cms.runtime.v1_spec.js new file mode 100644 index 0000000000..1eb5fa1d1b --- /dev/null +++ b/cms/static/js/spec/xblock/cms.runtime.v1_spec.js @@ -0,0 +1,80 @@ +define(["js/spec_helpers/edit_helpers", "js/views/modals/base_modal", "xblock/cms.runtime.v1"], + function (edit_helpers, BaseModal) { + + describe("Studio Runtime v1", function() { + var runtime; + + beforeEach(function () { + edit_helpers.installEditTemplates(); + runtime = new window.StudioRuntime.v1(); + }); + + it('allows events to be listened to', function() { + var canceled = false; + runtime.listenTo('cancel', function() { + canceled = true; + }); + expect(canceled).toBeFalsy(); + runtime.notify('cancel', {}); + expect(canceled).toBeTruthy(); + }); + + it('shows save notifications', function() { + var title = "Mock saving...", + notificationSpy = edit_helpers.createNotificationSpy(); + runtime.notify('save', { + state: 'start', + message: title + }); + edit_helpers.verifyNotificationShowing(notificationSpy, title); + runtime.notify('save', { + state: 'end' + }); + edit_helpers.verifyNotificationHidden(notificationSpy); + }); + + it('shows error messages', function() { + var title = "Mock Error", + message = "This is a mock error.", + notificationSpy = edit_helpers.createNotificationSpy("Error"); + runtime.notify('error', { + title: title, + message: message + }); + edit_helpers.verifyNotificationShowing(notificationSpy, title); + }); + + describe("Modal Dialogs", function() { + var MockModal, modal, showMockModal; + + MockModal = BaseModal.extend({ + getContentHtml: function() { + return readFixtures('mock/mock-modal.underscore'); + } + }); + + showMockModal = function() { + modal = new MockModal({ + title: "Mock Modal" + }); + modal.show(); + }; + + beforeEach(function () { + edit_helpers.installEditTemplates(); + }); + + afterEach(function() { + edit_helpers.hideModalIfShowing(modal); + }); + + it('cancels a modal dialog', function () { + showMockModal(); + runtime.notify('modal-shown', modal); + expect(edit_helpers.isShowingModal(modal)).toBeTruthy(); + runtime.notify('cancel'); + expect(edit_helpers.isShowingModal(modal)).toBeFalsy(); + }); + }); + }); + }); diff --git a/cms/static/js/spec_helpers/view_helpers.js b/cms/static/js/spec_helpers/view_helpers.js index e0ebf4d528..1cbbd1e5d4 100644 --- a/cms/static/js/spec_helpers/view_helpers.js +++ b/cms/static/js/spec_helpers/view_helpers.js @@ -21,8 +21,8 @@ define(["jquery", "js/views/feedback_notification", "js/spec_helpers/create_sino appendSetFixtures('
'); }; - createNotificationSpy = function() { - var notificationSpy = spyOnConstructor(NotificationView, "Mini", ["show", "hide"]); + createNotificationSpy = function(type) { + var notificationSpy = spyOnConstructor(NotificationView, type || "Mini", ["show", "hide"]); notificationSpy.show.andReturn(notificationSpy); return notificationSpy; }; diff --git a/cms/static/js/views/container.js b/cms/static/js/views/container.js index 85b3dc7fee..7a979a4496 100644 --- a/cms/static/js/views/container.js +++ b/cms/static/js/views/container.js @@ -1,4 +1,5 @@ -define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", "js/views/feedback_notification"], +define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", "js/views/feedback_notification", + "jquery.ui"], // The container view uses sortable, which is provided by jquery.ui. function ($, _, XBlockView, ModuleUtils, gettext, NotificationView) { var reorderableClass = '.reorderable-container', sortableInitializedClass = '.ui-sortable', diff --git a/cms/static/js/views/modals/edit_xblock.js b/cms/static/js/views/modals/edit_xblock.js index efe9acff2c..41718f7c24 100644 --- a/cms/static/js/views/modals/edit_xblock.js +++ b/cms/static/js/views/modals/edit_xblock.js @@ -71,7 +71,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", // Notify the runtime that the modal has been shown if (runtime) { this.runtime = runtime; - runtime.notify("edit-modal-shown", this); + runtime.notify('modal-shown', this); } // Update the modal's header @@ -166,12 +166,8 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", // Notify the runtime that the modal has been hidden if (this.runtime) { - this.runtime.notify('edit-modal-hidden'); + this.runtime.notify('modal-hidden'); } - - // Completely clear the contents of the modal - this.undelegateEvents(); - this.$el.html(""); }, findXBlockInfo: function(xblockWrapperElement, defaultXBlockInfo) { @@ -180,7 +176,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", displayName; if (xblockWrapperElement.length > 0) { xblockElement = xblockWrapperElement.find('.xblock'); - displayName = xblockWrapperElement.find('.xblock-header .header-details').text().trim(); + displayName = xblockWrapperElement.find('.xblock-header .header-details .xblock-display-name').text().trim(); // If not found, try looking for the old unit page style rendering if (!displayName) { displayName = this.xblockElement.find('.component-header').text().trim(); diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 414a040e9b..4f8fd18c94 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -2,11 +2,9 @@ * XBlockContainerPage is used to display Studio's container page for an xblock which has children. * This page allows the user to understand and manipulate the xblock and its children. */ -define(["jquery", "underscore", "gettext", "js/views/feedback_notification", - "js/views/baseview", "js/views/container", "js/views/xblock", "js/views/components/add_xblock", - "js/views/modals/edit_xblock", "js/models/xblock_info"], - function ($, _, gettext, NotificationView, BaseView, ContainerView, XBlockView, AddXBlockComponent, - EditXBlockModal, XBlockInfo) { +define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/container", + "js/views/xblock", "js/views/components/add_xblock", "js/views/modals/edit_xblock", "js/models/xblock_info"], + function ($, _, gettext, BaseView, ContainerView, XBlockView, AddXBlockComponent, EditXBlockModal, XBlockInfo) { var XBlockContainerPage = BaseView.extend({ // takes XBlockInfo as a model @@ -30,12 +28,16 @@ define(["jquery", "underscore", "gettext", "js/views/feedback_notification", // Hide both blocks until we know which one to show xblockView.$el.addClass('is-hidden'); - // Add actions to any top level buttons, e.g. "Edit" of the container itself - self.addButtonActions(this.$el); + if (!options || !options.refresh) { + // Add actions to any top level buttons, e.g. "Edit" of the container itself. + // Do not add the actions on "refresh" though, as the handlers are already registered. + self.addButtonActions(this.$el); + } // Render the xblock xblockView.render({ success: function(xblock) { + xblockView.xblock.runtime.notify("page-shown", self); xblockView.$el.removeClass('is-hidden'); self.renderAddXBlockComponents(); self.onXBlockRefresh(xblockView); @@ -55,7 +57,7 @@ define(["jquery", "underscore", "gettext", "js/views/feedback_notification", }, refreshTitle: function() { - var title = this.$('.xblock-header .header-details span').first().text().trim(); + var title = this.$('.xblock-header .header-details .xblock-display-name').first().text().trim(); this.$('.page-header-title').text(title); this.$('.page-header .subtitle a').last().text(title); }, @@ -112,12 +114,16 @@ define(["jquery", "underscore", "gettext", "js/views/feedback_notification", buttonPanel = target.closest('.add-xblock-component'), listPanel = buttonPanel.prev(), scrollOffset = this.getScrollOffset(buttonPanel), - placeholderElement = $('
').appendTo(listPanel), + placeholderElement = $('
').appendTo(listPanel), requestData = _.extend(template, { parent_locator: parentLocator }); return $.postJSON(this.getURLRoot() + '/', requestData, - _.bind(this.onNewXBlock, this, placeholderElement, scrollOffset)); + _.bind(this.onNewXBlock, this, placeholderElement, scrollOffset)) + .fail(function() { + // Remove the placeholder if the update failed + placeholderElement.remove(); + }); }, duplicateComponent: function(xblockElement) { @@ -129,14 +135,18 @@ define(["jquery", "underscore", "gettext", "js/views/feedback_notification", this.runOperationShowingMessage(gettext('Duplicating…'), function() { var scrollOffset = self.getScrollOffset(xblockElement), - placeholderElement = $('
').insertAfter(xblockElement), + placeholderElement = $('
').insertAfter(xblockElement), parentElement = self.findXBlockElement(parent), requestData = { duplicate_source_locator: xblockElement.data('locator'), parent_locator: parentElement.data('locator') }; return $.postJSON(self.getURLRoot() + '/', requestData, - _.bind(self.onNewXBlock, self, placeholderElement, scrollOffset)); + _.bind(self.onNewXBlock, self, placeholderElement, scrollOffset)) + .fail(function() { + // Remove the placeholder if the update failed + placeholderElement.remove(); + }); }); }, @@ -153,16 +163,21 @@ define(["jquery", "underscore", "gettext", "js/views/feedback_notification", url: self.getURLRoot() + "/" + xblockElement.data('locator') + "?" + $.param({recurse: true, all_versions: false}) - }).success(function() { - // get the parent so we can remove this component from its parent. - var parent = self.findXBlockElement(xblockElement.parent()); - xblockElement.remove(); - self.xblockView.updateChildren(parent); - }); + }).success(_.bind(self.onDelete, self, xblockElement)); }); }); }, + onDelete: function(xblockElement) { + // get the parent so we can remove this component from its parent. + var xblockView = this.xblockView, + xblock = xblockView.xblock, + parent = this.findXBlockElement(xblockElement.parent()); + xblockElement.remove(); + xblockView.updateChildren(parent); + xblock.runtime.notify('deleted-child', parent.data('locator')); + }, + onNewXBlock: function(xblockElement, scrollOffset, data) { this.setScrollOffset(xblockElement, scrollOffset); xblockElement.data('locator', data.locator); @@ -173,13 +188,14 @@ define(["jquery", "underscore", "gettext", "js/views/feedback_notification", * Refreshes the specified xblock's display. If the xblock is an inline child of a * reorderable container then the element will be refreshed inline. If not, then the * parent container will be refreshed instead. - * @param xblockElement The element representing the xblock to be refreshed. + * @param element An element representing the xblock to be refreshed. */ - refreshXBlock: function(xblockElement) { - var parentElement = xblockElement.parent(), + refreshXBlock: function(element) { + var xblockElement = this.findXBlockElement(element), + parentElement = xblockElement.parent(), rootLocator = this.xblockView.model.id; if (xblockElement.length === 0 || xblockElement.data('locator') === rootLocator) { - this.render({ }); + this.render({refresh: true}); } else if (parentElement.hasClass('reorderable-container')) { this.refreshChildXBlock(xblockElement); } else { diff --git a/cms/static/sass/elements/_controls.scss b/cms/static/sass/elements/_controls.scss index 6af4514079..fc3cadc339 100644 --- a/cms/static/sass/elements/_controls.scss +++ b/cms/static/sass/elements/_controls.scss @@ -233,7 +233,7 @@ @include transition(all $tmg-f3 linear 0s); display: block; border-radius: 3px; - padding: ($baseline/4) ($baseline/2); + padding: 3px ($baseline/2); color: $gray-l1; &:hover { diff --git a/cms/static/sass/elements/_xblocks.scss b/cms/static/sass/elements/_xblocks.scss index 2602bcca61..90e116c0cd 100644 --- a/cms/static/sass/elements/_xblocks.scss +++ b/cms/static/sass/elements/_xblocks.scss @@ -28,6 +28,11 @@ display: inline-block; width: 50%; vertical-align: middle; + + .xblock-display-name { + display: inline-block; + vertical-align: middle; + } } .header-actions { @@ -147,47 +152,51 @@ padding: ($baseline/2) ($baseline*.75); color: $white; - .message-text { - display: inline-block; - width: 93%; - vertical-align: top; - } - [class^="icon-"] { font-style: normal; } &.information { + @extend %t-copy-sub1; background-color: $gray-l5; color: $gray-d2; } - &.warning { + &.validation { background-color: $gray-d2; - padding: ($baseline/2) $baseline; color: $white; - .icon-warning-sign { - margin-right: ($baseline/2); - color: $orange; + a { + color: $blue-l2; } - .message-text { - display: inline-block; - width: 93%; - vertical-align: top; + &.has-warnings { + border-bottom: 3px solid $orange; + + .icon-warning-sign { + margin-right: ($baseline/2); + color: $orange; + } } - } - &.error { - background-color: $gray-d2; - padding: ($baseline/2) $baseline; - color: $white; + &.has-errors { + border-bottom: 3px solid $red-l2; - .icon-exclamation-sign { - margin-right: ($baseline/2); - color: $red-l2; + .icon-exclamation-sign { + margin-right: ($baseline/2); + color: $red-l2; + } } } } + + .xblock-message-list { + margin-bottom: 0; + } + + .xblock-message-actions { + @extend %actions-header; + padding: ($baseline/2) $baseline; + background-color: $gray-d1; + } } diff --git a/cms/static/sass/views/_container.scss b/cms/static/sass/views/_container.scss index 299c5ff731..0238122820 100644 --- a/cms/static/sass/views/_container.scss +++ b/cms/static/sass/views/_container.scss @@ -64,7 +64,7 @@ .no-container-content { @extend %ui-well; - padding: ($baseline*2); + padding: $baseline; background-color: $gray-l4; text-align: center; color: $gray; @@ -78,7 +78,7 @@ @extend %t-action4; padding: 8px 20px 10px; text-align: center; - margin-left: $baseline; + margin: ($baseline/2) 0 ($baseline/2) $baseline; [class^="icon-"] { margin-right: ($baseline/2); @@ -158,6 +158,8 @@ body.view-container .content-primary { // CASE: page level xblock rendering &.level-page { margin: 0; + box-shadow: none; + border: 0; .xblock-header { display: none; @@ -166,15 +168,36 @@ body.view-container .content-primary { .xblock-message { border-radius: 3px 3px 0 0; + &.validation { + padding-top: ($baseline*.75); + } + + .xblock-message-list { + margin: ($baseline/5) ($baseline*2.5); + list-style-type: disc; + color: $gray-l3; + } + + .xblock-message-item { + padding-bottom: ($baseline/4); + } + &.information { - @extend %t-copy-base; - margin-bottom: $baseline; - border-bottom: 1px solid $gray-l4; - padding: ($baseline/2) ($baseline*.75); + padding: 0 0 ($baseline/2) 0; background-color: $gray-l5; color: $gray-d1; } } + + .no-container-content { + + .xblock-message-list { + margin: 0; + list-style-type: none; + color: $gray-d2; + } + } + } // CASE: nesting level xblock rendering @@ -250,6 +273,74 @@ body.view-container .content-primary { display: none; } } + + .wrapper-xblock-message { + + .xblock-message { + border-radius: 0 0 3px 3px; + + .xblock-message-list { + margin: 0; + list-style-type: none; + } + + &.information { + @extend %t-copy-sub2; + padding: 0 0 ($baseline/2) $baseline; + color: $gray-l1; + } + + &.validation.has-warnings { + border: 0; + border-top: 3px solid $orange; + } + + &.validation.has-errors { + border: 0; + border-top: 3px solid $red-l2; + } + } + } + } + } + + // groups in experiments + .wrapper-groups { + + .title { + @extend %t-title7; + margin-left: ($baseline/2); + color: $gray-l1; + } + + &.is-active { + + // Don't show delete buttons on active groups + .wrapper-xblock.level-nesting > .xblock-header .action-delete { + display: none; + } + + + } + + &.is-inactive { + margin: ($baseline*1.5) 0 0 0; + border-top: 2px dotted $gray-l2; + padding: ($baseline*.75) 0; + background-color: $gray-l4; + + .wrapper-xblock.level-nesting { + @include transition(all $tmg-f2 linear 0s); + opacity: .7; + + &:hover { + opacity: 1; + } + } + + .new-component-item { + display: none; + } } } diff --git a/cms/static/sass/views/_unit.scss b/cms/static/sass/views/_unit.scss index ce28a6d263..4b86f9e398 100644 --- a/cms/static/sass/views/_unit.scss +++ b/cms/static/sass/views/_unit.scss @@ -203,6 +203,16 @@ body.course.unit, padding-top: 0; color: $gray-l1; } + + &.has-warnings { + border: 0; + border-top: 3px solid $orange; + } + + &.has-errors { + border: 0; + border-top: 3px solid $red-l2; + } } } diff --git a/cms/templates/js/mock/mock-container-xblock.underscore b/cms/templates/js/mock/mock-container-xblock.underscore index 1a13251f02..8d6adac912 100644 --- a/cms/templates/js/mock/mock-container-xblock.underscore +++ b/cms/templates/js/mock/mock-container-xblock.underscore @@ -1,7 +1,7 @@
- Test Container + Test Container
    @@ -22,7 +22,7 @@ Expand or Collapse - Group A + Group A
    @@ -131,7 +131,7 @@ Expand or Collapse - Group B + Group B
    diff --git a/cms/templates/js/mock/mock-empty-container-xblock.underscore b/cms/templates/js/mock/mock-empty-container-xblock.underscore index d0477756b1..1ba44ccee7 100644 --- a/cms/templates/js/mock/mock-empty-container-xblock.underscore +++ b/cms/templates/js/mock/mock-empty-container-xblock.underscore @@ -4,7 +4,7 @@ Expand or Collapse - Empty Vertical Test + Empty Vertical Test
    diff --git a/cms/templates/js/mock/mock-updated-container-xblock.underscore b/cms/templates/js/mock/mock-updated-container-xblock.underscore index 50a56efddc..141daa5526 100644 --- a/cms/templates/js/mock/mock-updated-container-xblock.underscore +++ b/cms/templates/js/mock/mock-updated-container-xblock.underscore @@ -1,7 +1,7 @@
    - Updated Test Container + Updated Test Container
      diff --git a/cms/templates/js/mock/mock-updated-xblock.underscore b/cms/templates/js/mock/mock-updated-xblock.underscore index 64579555bb..c5ef38394e 100644 --- a/cms/templates/js/mock/mock-updated-xblock.underscore +++ b/cms/templates/js/mock/mock-updated-xblock.underscore @@ -1,7 +1,7 @@
    • - Mock XBlock + Mock XBlock
        diff --git a/cms/templates/js/mock/mock-xblock.underscore b/cms/templates/js/mock/mock-xblock.underscore index f57ff8cfa3..652aa17001 100644 --- a/cms/templates/js/mock/mock-xblock.underscore +++ b/cms/templates/js/mock/mock-xblock.underscore @@ -1,7 +1,7 @@
      • - Mock XBlock + Mock XBlock
          diff --git a/cms/templates/studio_vertical_wrapper.html b/cms/templates/studio_vertical_wrapper.html index f7cadf3f53..ae4ac8aa05 100644 --- a/cms/templates/studio_vertical_wrapper.html +++ b/cms/templates/studio_vertical_wrapper.html @@ -8,7 +8,7 @@ ${_('Expand or Collapse')} - ${xblock.display_name_with_default | h} + ${xblock.display_name_with_default | h}
          diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index e9c9450bc1..07de5e6732 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -20,7 +20,7 @@ collapsible_class = "is-collapsible" if xblock.has_children else ""
          % endif -
          +
          % if show_inline: @@ -29,7 +29,7 @@ collapsible_class = "is-collapsible" if xblock.has_children else "" ${_('Expand or Collapse')} % endif - ${xblock.display_name_with_default | h} + ${xblock.display_name_with_default | h}
            @@ -47,13 +47,13 @@ collapsible_class = "is-collapsible" if xblock.has_children else "" ${_("Duplicate")} -
          • - - - ${_("Delete")} - -
            • % endif +
          • + + + ${_("Delete")} + +
            • % if is_reorderable:
          • diff --git a/cms/templates/widgets/split-edit.html b/cms/templates/widgets/split-edit.html deleted file mode 100644 index 3ac72ac12f..0000000000 --- a/cms/templates/widgets/split-edit.html +++ /dev/null @@ -1,12 +0,0 @@ -<%! from django.utils.translation import ugettext as _ %> -<%include file="metadata-edit.html" /> -% if disable_user_partition_editing: -
            - % if not selected_partition: -

            ${_("This content experiment refers to a group configuration that has been deleted.")}

            - % else: -

            ${_("This content experiment uses group configuration '{0}'.".format(""+str(selected_partition.name)+""))}

            - % endif -

            ${_("After you select the group configuration and save the content experiment, you cannot change this setting.")}

            -
            -% endif diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 403a50fab4..1baffd2e59 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -15,8 +15,7 @@ from xblock.fragment import Fragment from xmodule.seq_module import SequenceModule from xmodule.vertical_module import VerticalModule -from xmodule.x_module import shim_xmodule_js, XModuleDescriptor, XModule -from lms.lib.xblock.runtime import quote_slashes +from xmodule.x_module import shim_xmodule_js, XModuleDescriptor, XModule, PREVIEW_VIEWS, STUDIO_VIEW from xmodule.modulestore import MONGO_MODULESTORE_TYPE from xmodule.modulestore.django import modulestore @@ -60,10 +59,10 @@ def wrap_xblock(runtime_class, block, view, frag, context, usage_id_serializer, css_classes = ['xblock', 'xblock-' + view] if isinstance(block, (XModule, XModuleDescriptor)): - if view == 'student_view': + if view in PREVIEW_VIEWS: # The block is acting as an XModule css_classes.append('xmodule_display') - elif view == 'studio_view': + elif view == STUDIO_VIEW: # The block is acting as an XModuleDescriptor css_classes.append('xmodule_edit') diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index d68dd00741..c887670a7a 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/common/lib/xmodule/xmodule/conditional_module.py @@ -8,7 +8,7 @@ from lazy import lazy from lxml import etree from pkg_resources import resource_string -from xmodule.x_module import XModule +from xmodule.x_module import XModule, STUDENT_VIEW from xmodule.seq_module import SequenceDescriptor from xblock.fields import Scope, ReferenceList from xmodule.modulestore.exceptions import ItemNotFoundError @@ -160,7 +160,7 @@ class ConditionalModule(ConditionalFields, XModule): context) return json.dumps({'html': [html], 'message': bool(message)}) - html = [child.render('student_view').content for child in self.get_display_items()] + html = [child.render(STUDENT_VIEW).content for child in self.get_display_items()] return json.dumps({'html': html}) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 0ccd7d6613..1d0d0c56a5 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -13,7 +13,7 @@ from pkg_resources import resource_string from lxml import etree -from xmodule.x_module import XModule +from xmodule.x_module import XModule, STUDENT_VIEW from xmodule.raw_module import RawDescriptor from xblock.fields import Scope, String, Integer, Boolean, Dict, List @@ -113,7 +113,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): try: child = self.get_display_items()[0] - out = child.render('student_view').content + out = child.render(STUDENT_VIEW).content # The event listener uses the ajax url to find the child. child_id = child.id except IndexError: diff --git a/common/lib/xmodule/xmodule/css/split_test/edit.scss b/common/lib/xmodule/xmodule/css/split_test/edit.scss deleted file mode 100644 index adbf3ad7c6..0000000000 --- a/common/lib/xmodule/xmodule/css/split_test/edit.scss +++ /dev/null @@ -1,11 +0,0 @@ -.setting-message { - margin: ($baseline/2) $baseline; - border-top: 3px solid $gray-l2; - background-color: $gray-l5; - padding: $baseline; -} - -.setting-help { - @include font-size(12); - font-color: $gray-l6; -} diff --git a/common/lib/xmodule/xmodule/public/js/split_test_author_view.js b/common/lib/xmodule/xmodule/public/js/split_test_author_view.js new file mode 100644 index 0000000000..8c4cc866f3 --- /dev/null +++ b/common/lib/xmodule/xmodule/public/js/split_test_author_view.js @@ -0,0 +1,29 @@ +/* JavaScript for editing operations that can be done on the split test author view. */ +window.SplitTestAuthorView = function (runtime, element) { + var $element = $(element); + + $element.find('.add-missing-groups-button').click(function () { + runtime.notify('save', { + state: 'start', + element: element, + message: gettext('Creating missing groups…') + }); + $.post(runtime.handlerUrl(element, 'add_missing_groups')).done(function() { + runtime.notify('save', { + state: 'end', + element: element + }); + }); + }); + + // Listen to delete events so that the view can refresh when the last inactive group is removed. + runtime.listenTo('deleted-child', function(parentLocator) { + var splitTestLocator = $element.closest('.studio-xblock-wrapper').data('locator'), + inactiveGroups = $element.find('.is-inactive .studio-xblock-wrapper'); + if (splitTestLocator === parentLocator && inactiveGroups.length === 0) { + runtime.refreshXBlock($element); + } + }); + + return {}; +}; diff --git a/common/lib/xmodule/xmodule/randomize_module.py b/common/lib/xmodule/xmodule/randomize_module.py index 30febac9b2..6827a68c35 100644 --- a/common/lib/xmodule/xmodule/randomize_module.py +++ b/common/lib/xmodule/xmodule/randomize_module.py @@ -1,7 +1,7 @@ import logging import random -from xmodule.x_module import XModule +from xmodule.x_module import XModule, STUDENT_VIEW from xmodule.seq_module import SequenceDescriptor from lxml import etree @@ -83,7 +83,7 @@ class RandomizeModule(RandomizeFields, XModule): # raise error instead? In fact, could complain on descriptor load... return Fragment(content=u"
            Nothing to randomize between
            ") - return self.child.render('student_view', context) + return self.child.render(STUDENT_VIEW, context) def get_icon_class(self): return self.child.get_icon_class() if self.child else 'other' diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 6cb25884a4..b449763a6f 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -11,7 +11,7 @@ from .exceptions import NotFoundError from .fields import Date from .mako_module import MakoModuleDescriptor from .progress import Progress -from .x_module import XModule +from .x_module import XModule, STUDENT_VIEW from .xml_module import XmlDescriptor log = logging.getLogger(__name__) @@ -85,7 +85,7 @@ class SequenceModule(SequenceFields, XModule): for child in self.get_display_items(): progress = child.get_progress() - rendered_child = child.render('student_view', context) + rendered_child = child.render(STUDENT_VIEW, context) fragment.add_frag_resources(rendered_child) titles = child.get_content_titles() diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index 69e0629ef2..d909743217 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -6,12 +6,11 @@ import logging import json from webob import Response from uuid import uuid4 -from pkg_resources import resource_string from xmodule.progress import Progress from xmodule.seq_module import SequenceDescriptor -from xmodule.studio_editable import StudioEditableModule -from xmodule.x_module import XModule, module_attr +from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor +from xmodule.x_module import XModule, module_attr, STUDENT_VIEW from xmodule.modulestore.inheritance import UserPartitionList from lxml import etree @@ -51,6 +50,23 @@ class ValidationMessageType(object): return None +# TODO: move this into the xblock repo once it has a formal validation contract +class ValidationMessage(object): + """ + Represents a single validation message for an xblock. + """ + def __init__(self, xblock, message_text, message_type, action_class=None, action_label=None): + assert isinstance(message_text, unicode) + self.xblock = xblock + self.message_text = message_text + self.message_type = message_type + self.action_class = action_class + self.action_label = action_label + + def __unicode__(self): + return self.message_text + + class SplitTestFields(object): """Fields needed for split test module""" has_children = True @@ -62,12 +78,15 @@ class SplitTestFields(object): no_partition_selected = {'display_name': _("Not Selected"), 'value': -1} @staticmethod - def build_partition_values(all_user_partitions): + def build_partition_values(all_user_partitions, selected_user_partition): """ This helper method builds up the user_partition values that will be passed to the Studio editor """ - SplitTestFields.user_partition_values = [SplitTestFields.no_partition_selected] + SplitTestFields.user_partition_values = [] + # Add "No selection" value if there is not a valid selected user partition. + if not selected_user_partition: + SplitTestFields.user_partition_values.append(SplitTestFields.no_partition_selected) for user_partition in all_user_partitions: SplitTestFields.user_partition_values.append({"display_name": user_partition.name, "value": user_partition.id}) return SplitTestFields.user_partition_values @@ -87,7 +106,7 @@ class SplitTestFields(object): ) user_partition_id = Integer( - help=_("The configuration for how users are grouped for this content experiment. After you select the group configuration and save the content experiment, you cannot change this setting."), + help=_("The configuration defines how users are grouped for this content experiment. Caution: Changing the group configuration of a student-visible experiment will impact the experiment data."), scope=Scope.content, display_name=_("Group Configuration"), default=no_partition_selected["value"], @@ -108,7 +127,6 @@ class SplitTestFields(object): scope=Scope.content ) - @XBlock.needs('user_tags') # pylint: disable=abstract-method @XBlock.wants('partitions') class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): @@ -220,7 +238,7 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): child_location = self.group_id_to_child[group_id] child_descriptor = self.get_child_descriptor_by_location(child_location) child = self.system.get_module(child_descriptor) - rendered_child = child.render('student_view', context) + rendered_child = child.render(STUDENT_VIEW, context) fragment.add_frag_resources(rendered_child) contents.append({ @@ -238,35 +256,55 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): fragment.initialize_js('ABTestSelector') return fragment - def studio_preview_view(self, context): + def author_view(self, context): """ Renders the Studio preview by rendering each child so that they can all be seen and edited. """ fragment = Fragment() root_xblock = context.get('root_xblock') + is_configured = not self.user_partition_id == SplitTestFields.no_partition_selected['value'] is_root = root_xblock and root_xblock.location == self.location + active_groups_preview = None + inactive_groups_preview = None + if is_root: + [active_children, inactive_children] = self.descriptor.active_and_inactive_children() + active_groups_preview = self.studio_render_children( + fragment, active_children, context + ) + inactive_groups_preview = self.studio_render_children( + fragment, inactive_children, context + ) - # First render a header at the top of the split test module... - fragment.add_content(self.system.render_template('split_test_studio_header.html', { + fragment.add_content(self.system.render_template('split_test_author_view.html', { 'split_test': self, 'is_root': is_root, + 'is_configured': is_configured, + 'active_groups_preview': active_groups_preview, + 'inactive_groups_preview': inactive_groups_preview, })) - - # ... then render the children only when this block is being shown as the container - if is_root: - self.render_children(context, fragment, can_reorder=False) + fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/split_test_author_view.js')) + fragment.initialize_js('SplitTestAuthorView') return fragment + def studio_render_children(self, fragment, children, context): + """ + Renders the specified children and returns it as an HTML string. In addition, any + dependencies are added to the specified fragment. + """ + html = "" + for active_child_descriptor in children: + active_child = self.system.get_module(active_child_descriptor) + rendered_child = active_child.render(StudioEditableModule.get_preview_view_name(active_child), context) + fragment.add_frag_resources(rendered_child) + html = html + rendered_child.content + return html + def student_view(self, context): """ - Render the contents of the chosen condition for students, and all the + Renders the contents of the chosen condition for students, and all the conditions for staff. """ - # When rendering a Studio preview, render all of the block's children - if context and context.get('runtime_type', None) == 'studio': - return self.studio_preview_view(context) - if self.child is None: # raise error instead? In fact, could complain on descriptor load... return Fragment(content=u"
            Nothing here. Move along.
            ") @@ -274,7 +312,7 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): if self.system.user_is_staff: return self._staff_view(context) else: - child_fragment = self.child.render('student_view', context) + child_fragment = self.child.render(STUDENT_VIEW, context) fragment = Fragment(self.system.render_template('split_test_student_view.html', { 'child_content': child_fragment.content, 'child_id': self.child.scope_ids.usage_id, @@ -305,14 +343,13 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): @XBlock.needs('user_tags') # pylint: disable=abstract-method @XBlock.wants('partitions') -class SplitTestDescriptor(SplitTestFields, SequenceDescriptor): +class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDescriptor): # the editing interface can be the same as for sequences -- just a container module_class = SplitTestModule filename_extension = "xml" - mako_template = "widgets/split-edit.html" - css = {'scss': [resource_string(__name__, 'css/split_test/edit.scss')]} + mako_template = "widgets/metadata-only-edit.html" child_descriptor = module_attr('child_descriptor') log_child_render = module_attr('log_child_render') @@ -359,8 +396,7 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor): def get_context(self): _context = super(SplitTestDescriptor, self).get_context() _context.update({ - 'disable_user_partition_editing': self._disable_user_partition_editing(), - 'selected_partition': self._get_selected_partition() + 'selected_partition': self.get_selected_partition() }) return _context @@ -380,41 +416,31 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor): # Any existing value of user_partition_id will be in "old_content" instead of "old_metadata" # because it is Scope.content. if 'user_partition_id' not in old_content or old_content['user_partition_id'] != self.user_partition_id: - selected_partition = self._get_selected_partition() + selected_partition = self.get_selected_partition() if selected_partition is not None: - assert hasattr(self.system, 'modulestore') and hasattr(self.system.modulestore, 'create_and_save_xmodule'), \ - "editor_saved should only be called when a mutable modulestore is available" - modulestore = self.system.modulestore - group_id_mapping = {} + self.group_id_mapping = {} # pylint: disable=attribute-defined-outside-init for group in selected_partition.groups: - dest_usage_key = self.location.replace(category="vertical", name=uuid4().hex) - metadata = {'display_name': group.name} - modulestore.create_and_save_xmodule( - dest_usage_key, - definition_data=None, - metadata=metadata, - system=self.system, - ) - self.children.append(dest_usage_key) # pylint: disable=no-member - group_id_mapping[unicode(group.id)] = dest_usage_key - - self.group_id_to_child = group_id_mapping + self._create_vertical_for_group(group) # Don't need to call update_item in the modulestore because the caller of this method will do it. + else: + # If children referenced in group_id_to_child have been deleted, remove them from the map. + for str_group_id, usage_key in self.group_id_to_child.items(): + if usage_key not in self.children: # pylint: disable=no-member + del self.group_id_to_child[str_group_id] @property def editable_metadata_fields(self): # Update the list of partitions based on the currently available user_partitions. - SplitTestFields.build_partition_values(self.user_partitions) + SplitTestFields.build_partition_values(self.user_partitions, self.get_selected_partition()) editable_fields = super(SplitTestDescriptor, self).editable_metadata_fields - if not self._disable_user_partition_editing(): - # Explicitly add user_partition_id, which does not automatically get picked up because it is Scope.content. - # Note that this means it will be saved by the Studio editor as "metadata", but the field will - # still update correctly. - editable_fields[SplitTestFields.user_partition_id.name] = self._create_metadata_editor_info( - SplitTestFields.user_partition_id - ) + # Explicitly add user_partition_id, which does not automatically get picked up because it is Scope.content. + # Note that this means it will be saved by the Studio editor as "metadata", but the field will + # still update correctly. + editable_fields[SplitTestFields.user_partition_id.name] = self._create_metadata_editor_info( + SplitTestFields.user_partition_id + ) return editable_fields @@ -427,13 +453,7 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor): ]) return non_editable_fields - def _disable_user_partition_editing(self): - """ - If user_partition_id has been set to anything besides the default value, disable editing. - """ - return self.user_partition_id != SplitTestFields.user_partition_id.default - - def _get_selected_partition(self): + def get_selected_partition(self): """ Returns the partition that this split module is currently using, or None if the currently selected partition ID does not match any of the defined partitions. @@ -444,23 +464,121 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor): return None - def validation_message(self): + def active_and_inactive_children(self): """ - Returns a validation message describing the current state of the block, as well as a message type - indicating whether the message represents information, a warning or an error. + Returns two values: + 1. The active children of this split test, in the order of the groups. + 2. The remaining (inactive) children, in the order they were added to the split test. + """ + children = self.get_children() + + user_partition = self.get_selected_partition() + if not user_partition: + return [], children + + def get_child_descriptor(location): + """ + Returns the child descriptor which matches the specified location, or None if one is not found. + """ + for child in children: + if child.location == location: + return child + return None + + # Compute the active children in the order specified by the user partition + active_children = [] + for group in user_partition.groups: + group_id = unicode(group.id) + child_location = self.group_id_to_child.get(group_id, None) + child = get_child_descriptor(child_location) + if child: + active_children.append(child) + + # Compute the inactive children in the order they were added to the split test + inactive_children = [child for child in children if child not in active_children] + + return active_children, inactive_children + + def validation_messages(self): + """ + Returns a list of validation messages describing the current state of the block. Each message + includes a message type indicating whether the message represents information, a warning or an error. """ _ = self.runtime.service(self, "i18n").ugettext # pylint: disable=redefined-outer-name + messages = [] if self.user_partition_id < 0: - return _(u"You must select a group configuration for this content experiment."), ValidationMessageType.warning - user_partition = self._get_selected_partition() - if not user_partition: - return \ - _(u"This content experiment will not be shown to students because it refers to a group configuration that has been deleted. You can delete this experiment or reinstate the group configuration to repair it."), \ - ValidationMessageType.error - groups = user_partition.groups - if not len(groups) == len(self.get_children()): - return _(u"This content experiment is in an invalid state and cannot be repaired. Please delete and recreate."), ValidationMessageType.error + messages.append(ValidationMessage( + self, + _(u"The experiment is not associated with a group configuration."), + ValidationMessageType.warning, + 'edit-button', + _(u"Select a Group Configuration") + )) + else: + user_partition = self.get_selected_partition() + if not user_partition: + messages.append(ValidationMessage( + self, + _(u"The experiment uses a deleted group configuration. Select a valid group configuration or delete this experiment."), + ValidationMessageType.error + )) + else: + [active_children, inactive_children] = self.active_and_inactive_children() + if len(active_children) < len(user_partition.groups): + messages.append(ValidationMessage( + self, + _(u"The experiment does not contain all of the groups in the configuration."), + ValidationMessageType.error, + 'add-missing-groups-button', + _(u"Add Missing Groups") + )) + if len(inactive_children) > 0: + messages.append(ValidationMessage( + self, + _(u"The experiment has an inactive group. Move content into active groups, then delete the inactive group."), + ValidationMessageType.warning + )) + return messages - return _(u"This content experiment uses group configuration '{experiment_name}'.").format( - experiment_name=user_partition.name - ), ValidationMessageType.information + @XBlock.handler + def add_missing_groups(self, request, suffix=''): # pylint: disable=unused-argument + """ + Create verticals for any missing groups in the split test instance. + + Called from Studio view. + """ + user_partition = self.get_selected_partition() + + changed = False + for group in user_partition.groups: + str_group_id = unicode(group.id) + if str_group_id not in self.group_id_to_child: + self._create_vertical_for_group(group) + changed = True + + if changed: + # request does not have a user attribute, so pass None for user. + self.system.modulestore.update_item(self, None) + return Response() + + def _create_vertical_for_group(self, group): + """ + Creates a vertical to associate with the group. + + This appends the new vertical to the end of children, and updates group_id_to_child. + A mutable modulestore is needed to call this method (will need to update after mixed + modulestore work, currently relies on mongo's create_and_save_xmodule method). + """ + assert hasattr(self.system, 'modulestore') and hasattr(self.system.modulestore, 'create_and_save_xmodule'), \ + "editor_saved should only be called when a mutable modulestore is available" + modulestore = self.system.modulestore + dest_usage_key = self.location.replace(category="vertical", name=uuid4().hex) + metadata = {'display_name': group.name} + modulestore.create_and_save_xmodule( + dest_usage_key, + definition_data=None, + metadata=metadata, + system=self.system, + ) + self.children.append(dest_usage_key) # pylint: disable=no-member + self.group_id_to_child[unicode(group.id)] = dest_usage_key diff --git a/common/lib/xmodule/xmodule/studio_editable.py b/common/lib/xmodule/xmodule/studio_editable.py index 81284cd6e1..da29b19c6a 100644 --- a/common/lib/xmodule/xmodule/studio_editable.py +++ b/common/lib/xmodule/xmodule/studio_editable.py @@ -1,17 +1,18 @@ """ Mixin to support editing in Studio. """ +from xmodule.x_module import module_attr, STUDENT_VIEW, AUTHOR_VIEW class StudioEditableModule(object): """ - Helper methods for supporting Studio editing of xblocks/xmodules. + Helper methods for supporting Studio editing of xmodules. This class is only intended to be used with an XModule, as it assumes the existence of self.descriptor and self.system. """ - def render_children(self, context, fragment, can_reorder=False, can_add=False, view_name='student_view'): + def render_children(self, context, fragment, can_reorder=False, can_add=False): """ Renders the children of the module with HTML appropriate for Studio. If can_reorder is True, then the children will be rendered to support drag and drop. @@ -22,7 +23,7 @@ class StudioEditableModule(object): if can_reorder: context['reorderable_items'].add(child.location) child_module = self.system.get_module(child) # pylint: disable=E1101 - rendered_child = child_module.render(view_name, context) + rendered_child = child_module.render(StudioEditableModule.get_preview_view_name(child_module), context) fragment.add_frag_resources(rendered_child) contents.append({ @@ -36,3 +37,21 @@ class StudioEditableModule(object): 'can_add': can_add, 'can_reorder': can_reorder, })) + + @staticmethod + def get_preview_view_name(block): + """ + Helper method for getting preview view name (student_view or author_view) for a given module. + """ + return AUTHOR_VIEW if hasattr(block, AUTHOR_VIEW) else STUDENT_VIEW + + +class StudioEditableDescriptor(object): + """ + Helper mixin for supporting Studio editing of xmodules. + + This class is only intended to be used with an XModule Descriptor. This class assumes that the associated + XModule will have an "author_view" method for returning an editable preview view of the module. + """ + author_view = module_attr(AUTHOR_VIEW) + has_author_view = True diff --git a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py index e74d6d16c2..502de99884 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -25,6 +25,7 @@ from xmodule.combined_open_ended_module import CombinedOpenEndedModule from opaque_keys.edx.locations import Location from xmodule.tests import get_test_system, test_util_open_ended from xmodule.progress import Progress +from xmodule.x_module import STUDENT_VIEW from xmodule.tests.test_util_open_ended import ( DummyModulestore, TEST_STATE_SA_IN, MOCK_INSTANCE_STATE, TEST_STATE_SA, TEST_STATE_AI, TEST_STATE_AI2, TEST_STATE_AI2_INVALID, @@ -1041,7 +1042,7 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): self._handle_ajax("next_problem", {}) self.assertEqual(self._module().current_task_number, 0) - html = self._module().render('student_view').content + html = self._module().render(STUDENT_VIEW).content self.assertIsInstance(html, basestring) rubric = self._handle_ajax("get_combined_rubric", {}) @@ -1098,7 +1099,7 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): # Move to the next step in the problem self._handle_ajax("next_problem", {}) self.assertEqual(self._module().current_task_number, 1) - self._module().render('student_view') + self._module().render(STUDENT_VIEW) # Try to get the rubric from the module self._handle_ajax("get_combined_rubric", {}) @@ -1131,7 +1132,7 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): self.assertEqual(module.current_task_number, 1) # Get html and other data client will request - module.render('student_view') + module.render(STUDENT_VIEW) self._handle_ajax("skip_post_assessment", {}) @@ -1167,7 +1168,7 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): # Move to the next step in the problem self._handle_ajax("next_problem", {}) self.assertEqual(self._module().current_task_number, 1) - self._module().render('student_view') + self._module().render(STUDENT_VIEW) # Try to get the rubric from the module self._handle_ajax("get_combined_rubric", {}) @@ -1198,7 +1199,7 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): self.assertEqual(module.current_task_number, 1) # Get html and other data client will request - module.render('student_view') + module.render(STUDENT_VIEW) self._handle_ajax("skip_post_assessment", {}) @@ -1268,7 +1269,7 @@ class OpenEndedModuleXmlAttemptTest(unittest.TestCase, DummyModulestore): self._handle_ajax("next_problem", {}) self.assertEqual(self._module().current_task_number, 0) - html = self._module().render('student_view').content + html = self._module().render(STUDENT_VIEW).content self.assertIsInstance(html, basestring) # Module should now be done diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index c083877abc..bc20075704 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -11,6 +11,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location from xmodule.modulestore.xml import ImportSystem, XMLModuleStore, CourseLocationGenerator from xmodule.conditional_module import ConditionalDescriptor from xmodule.tests import DATA_DIR, get_test_system, get_test_descriptor_system +from xmodule.x_module import STUDENT_VIEW ORG = 'test_org' @@ -129,7 +130,7 @@ class ConditionalModuleBasicTest(unittest.TestCase): modules = ConditionalFactory.create(self.test_system) # because get_test_system returns the repr of the context dict passed to render_template, # we reverse it here - html = modules['cond_module'].render('student_view').content + html = modules['cond_module'].render(STUDENT_VIEW).content expected = modules['cond_module'].xmodule_runtime.render_template('conditional_ajax.html', { 'ajax_url': modules['cond_module'].xmodule_runtime.ajax_url, 'element_id': u'i4x-edX-conditional_test-conditional-SampleConditional', @@ -219,7 +220,7 @@ class ConditionalModuleXmlTest(unittest.TestCase): print "module children: ", module.get_children() print "module display items (children): ", module.get_display_items() - html = module.render('student_view').content + html = module.render(STUDENT_VIEW).content print "html type: ", type(html) print "html: ", html html_expect = module.xmodule_runtime.render_template( diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index 0521b32c94..f32b9c1a83 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py @@ -8,6 +8,7 @@ import copy from xmodule.crowdsource_hinter import CrowdsourceHinterModule from xmodule.vertical_module import VerticalModule, VerticalDescriptor +from xmodule.x_module import STUDENT_VIEW from xblock.field_data import DictFieldData from xblock.fragment import Fragment from xblock.core import XBlock @@ -245,7 +246,7 @@ class CrowdsourceHinterTest(unittest.TestCase): """ return [FakeChild()] mock_module.get_display_items = fake_get_display_items - out_html = mock_module.render('student_view').content + out_html = mock_module.render(STUDENT_VIEW).content self.assertTrue('This is supposed to be test html.' in out_html) self.assertTrue('i4x://this/is/a/fake/id' in out_html) @@ -262,7 +263,7 @@ class CrowdsourceHinterTest(unittest.TestCase): """ return [] mock_module.get_display_items = fake_get_display_items - out_html = mock_module.render('student_view').content + out_html = mock_module.render(STUDENT_VIEW).content self.assertTrue('Error in loading crowdsourced hinter' in out_html) @unittest.skip("Needs to be finished.") @@ -273,7 +274,7 @@ class CrowdsourceHinterTest(unittest.TestCase): NOT WORKING RIGHT NOW """ mock_module = VerticalWithModulesFactory.create() - out_html = mock_module.render('student_view').content + out_html = mock_module.render(STUDENT_VIEW).content self.assertTrue('Test numerical problem.' in out_html) self.assertTrue('Another test numerical problem.' in out_html) diff --git a/common/lib/xmodule/xmodule/tests/test_error_module.py b/common/lib/xmodule/xmodule/tests/test_error_module.py index cb7cebb935..85c28de7ca 100644 --- a/common/lib/xmodule/xmodule/tests/test_error_module.py +++ b/common/lib/xmodule/xmodule/tests/test_error_module.py @@ -6,7 +6,7 @@ from xmodule.tests import get_test_system from xmodule.error_module import ErrorDescriptor, ErrorModule, NonStaffErrorDescriptor from xmodule.modulestore.xml import CourseLocationGenerator from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location -from xmodule.x_module import XModuleDescriptor, XModule +from xmodule.x_module import XModuleDescriptor, XModule, STUDENT_VIEW from mock import MagicMock, Mock, patch from xblock.runtime import Runtime, IdReader from xblock.field_data import FieldData @@ -39,7 +39,7 @@ class TestErrorModule(unittest.TestCase, SetupTestErrorModules): ) self.assertIsInstance(descriptor, ErrorDescriptor) descriptor.xmodule_runtime = self.system - context_repr = self.system.render(descriptor, 'student_view').content + context_repr = self.system.render(descriptor, STUDENT_VIEW).content self.assertIn(self.error_msg, context_repr) self.assertIn(repr(self.valid_xml), context_repr) @@ -53,7 +53,7 @@ class TestErrorModule(unittest.TestCase, SetupTestErrorModules): descriptor, self.error_msg) self.assertIsInstance(error_descriptor, ErrorDescriptor) error_descriptor.xmodule_runtime = self.system - context_repr = self.system.render(error_descriptor, 'student_view').content + context_repr = self.system.render(error_descriptor, STUDENT_VIEW).content self.assertIn(self.error_msg, context_repr) self.assertIn(repr(descriptor), context_repr) @@ -80,7 +80,7 @@ class TestNonStaffErrorModule(unittest.TestCase, SetupTestErrorModules): CourseLocationGenerator(self.course_id) ) descriptor.xmodule_runtime = self.system - context_repr = self.system.render(descriptor, 'student_view').content + context_repr = self.system.render(descriptor, STUDENT_VIEW).content self.assertNotIn(self.error_msg, context_repr) self.assertNotIn(repr(self.valid_xml), context_repr) @@ -94,7 +94,7 @@ class TestNonStaffErrorModule(unittest.TestCase, SetupTestErrorModules): descriptor, self.error_msg) self.assertIsInstance(error_descriptor, ErrorDescriptor) error_descriptor.xmodule_runtime = self.system - context_repr = self.system.render(error_descriptor, 'student_view').content + context_repr = self.system.render(error_descriptor, STUDENT_VIEW).content self.assertNotIn(self.error_msg, context_repr) self.assertNotIn(str(descriptor), context_repr) diff --git a/common/lib/xmodule/xmodule/tests/test_split_test_module.py b/common/lib/xmodule/xmodule/tests/test_split_test_module.py index e9eee2cef6..20e4aa73b5 100644 --- a/common/lib/xmodule/xmodule/tests/test_split_test_module.py +++ b/common/lib/xmodule/xmodule/tests/test_split_test_module.py @@ -9,6 +9,7 @@ from fs.memoryfs import MemoryFS from xmodule.tests.xml import factories as xml from xmodule.tests.xml import XModuleXmlImportTest from xmodule.tests import get_test_system +from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW from xmodule.split_test_module import SplitTestDescriptor, SplitTestFields, ValidationMessageType from xmodule.partitions.partitions import Group, UserPartition from xmodule.partitions.test_partitions import StaticPartitionService, MemoryUserTagsService @@ -113,7 +114,7 @@ class SplitTestModuleLMSTest(SplitTestModuleTest): self.assertIn( child_content, - self.module_system.render(self.split_test_module, 'student_view').content + self.module_system.render(self.split_test_module, STUDENT_VIEW).content ) @ddt.data(('0',), ('1',)) @@ -159,35 +160,43 @@ class SplitTestModuleStudioTest(SplitTestModuleTest): Unit tests for how split test interacts with Studio. """ - def test_render_studio_view(self): + def test_render_author_view(self): """ - Test the rendering of the Studio view. + Test the rendering of the Studio author view. """ + def create_studio_context(root_xblock): + """ + Context for rendering the studio "author_view". + """ + return { + 'container_view': True, + 'reorderable_items': set(), + 'root_xblock': root_xblock, + } + # The split_test module should render both its groups when it is the root - reorderable_items = set() - context = { - 'runtime_type': 'studio', - 'container_view': True, - 'reorderable_items': reorderable_items, - 'root_xblock': self.split_test_module, - } - html = self.module_system.render(self.split_test_module, 'student_view', context).content + context = create_studio_context(self.split_test_module) + html = self.module_system.render(self.split_test_module, AUTHOR_VIEW, context).content self.assertIn('HTML FOR GROUP 0', html) self.assertIn('HTML FOR GROUP 1', html) # When rendering as a child, it shouldn't render either of its groups - reorderable_items = set() - context = { - 'runtime_type': 'studio', - 'container_view': True, - 'reorderable_items': reorderable_items, - 'root_xblock': self.course_sequence, - } - html = self.module_system.render(self.split_test_module, 'student_view', context).content + context = create_studio_context(self.course_sequence) + html = self.module_system.render(self.split_test_module, AUTHOR_VIEW, context).content self.assertNotIn('HTML FOR GROUP 0', html) self.assertNotIn('HTML FOR GROUP 1', html) + # The "Create Missing Groups" button should be rendered when groups are missing + context = create_studio_context(self.split_test_module) + self.split_test_module.user_partitions = [ + UserPartition(0, 'first_partition', 'First Partition', + [Group("0", 'alpha'), Group("1", 'beta'), Group("2", 'gamma')]) + ] + html = self.module_system.render(self.split_test_module, AUTHOR_VIEW, context).content + self.assertIn('HTML FOR GROUP 0', html) + self.assertIn('HTML FOR GROUP 1', html) + def test_editable_settings(self): """ Test the setting information passed back from editable_metadata_fields. @@ -197,14 +206,8 @@ class SplitTestModuleStudioTest(SplitTestModuleTest): self.assertNotIn(SplitTestDescriptor.due.name, editable_metadata_fields) self.assertNotIn(SplitTestDescriptor.user_partitions.name, editable_metadata_fields) - # user_partition_id will only appear in the editable settings if the value is the - # default "unselected" value. This split instance has user_partition_id = 0, so - # user_partition_id will not be editable. - self.assertNotIn(SplitTestDescriptor.user_partition_id.name, editable_metadata_fields) - - # Explicitly set user_partition_id to the default value. Now user_partition_id will be editable. - self.split_test_module.user_partition_id = SplitTestFields.no_partition_selected['value'] - editable_metadata_fields = self.split_test_module.editable_metadata_fields + # user_partition_id will always appear in editable_metadata_settings, regardless + # of the selected value. self.assertIn(SplitTestDescriptor.user_partition_id.name, editable_metadata_fields) def test_non_editable_settings(self): @@ -223,6 +226,7 @@ class SplitTestModuleStudioTest(SplitTestModuleTest): self.assertEqual([], SplitTestDescriptor.user_partition_id.values) # user_partitions is empty, only the "Not Selected" item will appear. + self.split_test_module.user_partition_id = SplitTestFields.no_partition_selected['value'] self.split_test_module.editable_metadata_fields # pylint: disable=pointless-statement partitions = SplitTestDescriptor.user_partition_id.values self.assertEqual(1, len(partitions)) @@ -239,6 +243,67 @@ class SplitTestModuleStudioTest(SplitTestModuleTest): self.assertEqual(0, partitions[1]['value']) self.assertEqual("first_partition", partitions[1]['display_name']) + # Try again with a selected partition and verify that there is no option for "No Selection" + self.split_test_module.user_partition_id = 0 + self.split_test_module.editable_metadata_fields # pylint: disable=pointless-statement + partitions = SplitTestDescriptor.user_partition_id.values + self.assertEqual(1, len(partitions)) + self.assertEqual(0, partitions[0]['value']) + self.assertEqual("first_partition", partitions[0]['display_name']) + + # Finally try again with an invalid selected partition and verify that "No Selection" is an option + self.split_test_module.user_partition_id = 999 + self.split_test_module.editable_metadata_fields # pylint: disable=pointless-statement + partitions = SplitTestDescriptor.user_partition_id.values + self.assertEqual(2, len(partitions)) + self.assertEqual(SplitTestFields.no_partition_selected['value'], partitions[0]['value']) + self.assertEqual(0, partitions[1]['value']) + self.assertEqual("first_partition", partitions[1]['display_name']) + + def test_active_and_inactive_children(self): + """ + Tests the active and inactive children returned for different split test configurations. + """ + split_test_module = self.split_test_module + children = split_test_module.get_children() + + # Verify that a split test has no active children if it has no specified user partition. + split_test_module.user_partition_id = -1 + [active_children, inactive_children] = split_test_module.active_and_inactive_children() + self.assertEqual(active_children, []) + self.assertEqual(inactive_children, children) + + # Verify that all the children are returned as active for a correctly configured split_test + split_test_module.user_partition_id = 0 + split_test_module.user_partitions = [ + UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta')]) + ] + [active_children, inactive_children] = split_test_module.active_and_inactive_children() + self.assertEqual(active_children, children) + self.assertEqual(inactive_children, []) + + # Verify that a split_test does not return inactive children in the active children + self.split_test_module.user_partitions = [ + UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha')]) + ] + [active_children, inactive_children] = split_test_module.active_and_inactive_children() + self.assertEqual(active_children, [children[0]]) + self.assertEqual(inactive_children, [children[1]]) + + # Verify that a split_test ignores misconfigured children + self.split_test_module.user_partitions = [ + UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("2", 'gamma')]) + ] + [active_children, inactive_children] = split_test_module.active_and_inactive_children() + self.assertEqual(active_children, [children[0]]) + self.assertEqual(inactive_children, [children[1]]) + + # Verify that a split_test referring to a non-existent user partition has no active children + self.split_test_module.user_partition_id = 2 + [active_children, inactive_children] = split_test_module.active_and_inactive_children() + self.assertEqual(active_children, []) + self.assertEqual(inactive_children, children) + def test_validation_message_types(self): """ Test the behavior of validation message types. @@ -249,46 +314,95 @@ class SplitTestModuleStudioTest(SplitTestModuleTest): def test_validation_messages(self): """ - Test the validation messages produced for different split_test configurations. + Test the validation messages produced for different split test configurations. """ + split_test_module = self.split_test_module - def verify_validation_message(split_test_module, expected_message, expected_message_type): + def verify_validation_message(message, expected_message, expected_message_type, + expected_action_class=None, expected_action_label=None): """ - Verify that the module has the expected validation message and type. + Verify that the validation message has the expected validation message and type. """ - (message, message_type) = split_test_module.validation_message() - self.assertEqual(message, expected_message) - self.assertEqual(message_type, expected_message_type) + self.assertEqual(unicode(message), expected_message) + self.assertEqual(message.message_type, expected_message_type) + self.assertEqual(message.action_class, expected_action_class) + self.assertEqual(message.action_label, expected_action_label) - # Verify the message for an unconfigured experiment - self.split_test_module.user_partition_id = -1 - verify_validation_message(self.split_test_module, - u"You must select a group configuration for this content experiment.", - ValidationMessageType.warning) + # Verify the messages for an unconfigured user partition + split_test_module.user_partition_id = -1 + messages = split_test_module.validation_messages() + self.assertEqual(len(messages), 1) + verify_validation_message( + messages[0], + u"The experiment is not associated with a group configuration.", + ValidationMessageType.warning, + 'edit-button', + u"Select a Group Configuration", + ) - # Verify the message for a correctly configured experiment - self.split_test_module.user_partition_id = 0 - self.split_test_module.user_partitions = [ + # Verify the messages for a correctly configured split_test + split_test_module.user_partition_id = 0 + split_test_module.user_partitions = [ UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta')]) ] - verify_validation_message(self.split_test_module, - u"This content experiment uses group configuration 'first_partition'.", - ValidationMessageType.information) + messages = split_test_module.validation_messages() + self.assertEqual(len(messages), 0) - # Verify the message for a block with the wrong number of groups - self.split_test_module.user_partitions = [ + # Verify the messages for a split test with too few groups + split_test_module.user_partitions = [ UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta'), Group("2", 'gamma')]) ] - verify_validation_message(self.split_test_module, - u"This content experiment is in an invalid state and cannot be repaired. " - u"Please delete and recreate.", - ValidationMessageType.error) + messages = split_test_module.validation_messages() + self.assertEqual(len(messages), 1) + verify_validation_message( + messages[0], + u"The experiment does not contain all of the groups in the configuration.", + ValidationMessageType.error, + 'add-missing-groups-button', + u"Add Missing Groups" + ) - # Verify the message for a block referring to a non-existent experiment - self.split_test_module.user_partition_id = 2 - verify_validation_message(self.split_test_module, - u"This content experiment will not be shown to students because it refers " - u"to a group configuration that has been deleted. " - u"You can delete this experiment or reinstate the group configuration to repair it.", - ValidationMessageType.error) + # Verify the messages for a split test with children that are not associated with any group + split_test_module.user_partitions = [ + UserPartition(0, 'first_partition', 'First Partition', + [Group("0", 'alpha')]) + ] + messages = split_test_module.validation_messages() + self.assertEqual(len(messages), 1) + verify_validation_message( + messages[0], + u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.", + ValidationMessageType.warning + ) + + # Verify the messages for a split test with both missing and inactive children + split_test_module.user_partitions = [ + UserPartition(0, 'first_partition', 'First Partition', + [Group("0", 'alpha'), Group("2", 'gamma')]) + ] + messages = split_test_module.validation_messages() + self.assertEqual(len(messages), 2) + verify_validation_message( + messages[0], + u"The experiment does not contain all of the groups in the configuration.", + ValidationMessageType.error, + 'add-missing-groups-button', + u"Add Missing Groups" + ) + verify_validation_message( + messages[1], + u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.", + ValidationMessageType.warning + ) + + # Verify the messages for a split test referring to a non-existent user partition + split_test_module.user_partition_id = 2 + messages = split_test_module.validation_messages() + self.assertEqual(len(messages), 1) + verify_validation_message( + messages[0], + u"The experiment uses a deleted group configuration. " + u"Select a valid group configuration or delete this experiment.", + ValidationMessageType.error + ) diff --git a/common/lib/xmodule/xmodule/tests/test_studio_editable.py b/common/lib/xmodule/xmodule/tests/test_studio_editable.py index 62916fa8b3..327e82a75a 100644 --- a/common/lib/xmodule/xmodule/tests/test_studio_editable.py +++ b/common/lib/xmodule/xmodule/tests/test_studio_editable.py @@ -3,6 +3,7 @@ Tests for StudioEditableModule. """ from xmodule.tests.test_vertical import BaseVerticalModuleTest +from xmodule.x_module import AUTHOR_VIEW class StudioEditableModuleTestCase(BaseVerticalModuleTest): @@ -12,7 +13,6 @@ class StudioEditableModuleTestCase(BaseVerticalModuleTest): """ reorderable_items = set() context = { - 'runtime_type': 'studio', 'container_view': True, 'reorderable_items': reorderable_items, 'read_only': False, @@ -20,6 +20,6 @@ class StudioEditableModuleTestCase(BaseVerticalModuleTest): } # Both children of the vertical should be rendered as reorderable - self.module_system.render(self.vertical, 'student_view', context).content + self.module_system.render(self.vertical, AUTHOR_VIEW, context).content # pylint: disable=expression-not-assigned self.assertIn(self.vertical.get_children()[0].location, reorderable_items) self.assertIn(self.vertical.get_children()[1].location, reorderable_items) diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index 600a37d224..d1e0047313 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -6,6 +6,7 @@ from fs.memoryfs import MemoryFS from xmodule.tests import get_test_system from xmodule.tests.xml import XModuleXmlImportTest from xmodule.tests.xml import factories as xml +from xmodule.x_module import STUDENT_VIEW, AUTHOR_VIEW class BaseVerticalModuleTest(XModuleXmlImportTest): @@ -46,30 +47,28 @@ class VerticalModuleTestCase(BaseVerticalModuleTest): """ Test the rendering of the student view. """ - html = self.module_system.render(self.vertical, 'student_view', {}).content + html = self.module_system.render(self.vertical, STUDENT_VIEW, {}).content self.assertIn(self.test_html_1, html) self.assertIn(self.test_html_2, html) def test_render_studio_view(self): """ - Test the rendering of the Studio view + Test the rendering of the Studio author view """ # Vertical shouldn't render children on the unit page context = { - 'runtime_type': 'studio', 'container_view': False, } - html = self.module_system.render(self.vertical, 'student_view', context).content + html = self.module_system.render(self.vertical, AUTHOR_VIEW, context).content self.assertNotIn(self.test_html_1, html) self.assertNotIn(self.test_html_2, html) # Vertical should render reorderable children on the container page reorderable_items = set() context = { - 'runtime_type': 'studio', 'container_view': True, 'reorderable_items': reorderable_items, } - html = self.module_system.render(self.vertical, 'student_view', context).content + html = self.module_system.render(self.vertical, AUTHOR_VIEW, context).content self.assertIn(self.test_html_1, html) self.assertIn(self.test_html_2, html) diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index 37c812a5c2..62add6cca3 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py @@ -26,7 +26,7 @@ from xblock.fields import ScopeIds from opaque_keys.edx.locations import Location -from xmodule.x_module import ModuleSystem, XModule, XModuleDescriptor, DescriptorSystem +from xmodule.x_module import ModuleSystem, XModule, XModuleDescriptor, DescriptorSystem, STUDENT_VIEW, STUDIO_VIEW from xmodule.annotatable_module import AnnotatableDescriptor from xmodule.capa_module import CapaDescriptor from xmodule.course_module import CourseDescriptor @@ -324,7 +324,7 @@ class TestStudentView(XBlockWrapperTestMixin, TestCase): """ self.assertEqual( descriptor._xmodule.get_html(), - descriptor.render('student_view').content + descriptor.render(STUDENT_VIEW).content ) @@ -343,7 +343,7 @@ class TestStudioView(XBlockWrapperTestMixin, TestCase): """ Assert that studio_view and get_html render the same. """ - self.assertEqual(descriptor.get_html(), descriptor.render('studio_view').content) + self.assertEqual(descriptor.get_html(), descriptor.render(STUDIO_VIEW).content) class TestXModuleHandler(TestCase): diff --git a/common/lib/xmodule/xmodule/vertical_module.py b/common/lib/xmodule/xmodule/vertical_module.py index cfb8081bea..292553ab6f 100644 --- a/common/lib/xmodule/xmodule/vertical_module.py +++ b/common/lib/xmodule/xmodule/vertical_module.py @@ -1,8 +1,8 @@ from xblock.fragment import Fragment -from xmodule.x_module import XModule +from xmodule.x_module import XModule, STUDENT_VIEW from xmodule.seq_module import SequenceDescriptor from xmodule.progress import Progress -from xmodule.studio_editable import StudioEditableModule +from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor from pkg_resources import resource_string from copy import copy @@ -19,13 +19,28 @@ class VerticalModule(VerticalFields, XModule, StudioEditableModule): ''' Layout module for laying out submodules vertically.''' def student_view(self, context): - # When rendering a Studio preview, use a different template to support drag and drop. - if context and context.get('runtime_type', None) == 'studio': - return self.studio_preview_view(context) + fragment = Fragment() + contents = [] - return self.render_view(context, 'vert_module.html') + child_context = {} if not context else copy(context) + child_context['child_of_vertical'] = True - def studio_preview_view(self, context): + for child in self.get_display_items(): + rendered_child = child.render(STUDENT_VIEW, child_context) + fragment.add_frag_resources(rendered_child) + + contents.append({ + 'id': child.location.to_deprecated_string(), + 'content': rendered_child.content + }) + + fragment.add_content(self.system.render_template('vert_module.html', { + 'items': contents, + 'xblock_context': context, + })) + return fragment + + def author_view(self, context): """ Renders the Studio preview view, which supports drag and drop. """ @@ -36,31 +51,6 @@ class VerticalModule(VerticalFields, XModule, StudioEditableModule): self.render_children(context, fragment, can_reorder=True, can_add=True) return fragment - def render_view(self, context, template_name): - """ - Helper method for rendering student_view and the Studio version. - """ - fragment = Fragment() - contents = [] - - child_context = {} if not context else copy(context) - child_context['child_of_vertical'] = True - - for child in self.get_display_items(): - rendered_child = child.render('student_view', child_context) - fragment.add_frag_resources(rendered_child) - - contents.append({ - 'id': child.location.to_deprecated_string(), - 'content': rendered_child.content - }) - - fragment.add_content(self.system.render_template(template_name, { - 'items': contents, - 'xblock_context': context, - })) - return fragment - def get_progress(self): # TODO: Cache progress or children array? children = self.get_children() @@ -77,7 +67,10 @@ class VerticalModule(VerticalFields, XModule, StudioEditableModule): return new_class -class VerticalDescriptor(VerticalFields, SequenceDescriptor): +class VerticalDescriptor(VerticalFields, SequenceDescriptor, StudioEditableDescriptor): + """ + Descriptor class for editing verticals. + """ module_class = VerticalModule js = {'coffee': [resource_string(__name__, 'js/src/vertical/edit.coffee')]} diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 305d83730c..dd4af325ef 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -32,6 +32,25 @@ log = logging.getLogger(__name__) XMODULE_METRIC_NAME = 'edxapp.xmodule' +# xblock view names + +# This is the view that will be rendered to display the XBlock in the LMS. +# It will also be used to render the block in "preview" mode in Studio, unless +# the XBlock also implements author_view. +STUDENT_VIEW = 'student_view' + +# An optional view of the XBlock similar to student_view, but with possible inline +# editing capabilities. This view differs from studio_view in that it should be as similar to student_view +# as possible. When previewing XBlocks within Studio, Studio will prefer author_view to student_view. +AUTHOR_VIEW = 'author_view' + +# The view used to render an editor in Studio. The editor rendering can be completely different +# from the LMS student_view, and it is only shown when the author selects "Edit". +STUDIO_VIEW = 'studio_view' + +# Views that present a "preview" view of an xblock (as opposed to an editing view). +PREVIEW_VIEWS = [STUDENT_VIEW, AUTHOR_VIEW] + class OpaqueKeyReader(IdReader): """ @@ -934,7 +953,7 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): get_score = module_attr('get_score') handle_ajax = module_attr('handle_ajax') max_score = module_attr('max_score') - student_view = module_attr('student_view') + student_view = module_attr(STUDENT_VIEW) get_child_descriptors = module_attr('get_child_descriptors') xmodule_handler = module_attr('xmodule_handler') @@ -1138,7 +1157,7 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # p return result def render(self, block, view_name, context=None): - if view_name == 'student_view': + if view_name in PREVIEW_VIEWS: assert block.xmodule_runtime is not None if isinstance(block, (XModule, XModuleDescriptor)): to_render = block._xmodule diff --git a/common/test/acceptance/fixtures/course.py b/common/test/acceptance/fixtures/course.py index 76fbb69b54..5d14c2d27c 100644 --- a/common/test/acceptance/fixtures/course.py +++ b/common/test/acceptance/fixtures/course.py @@ -200,6 +200,7 @@ class CourseFixture(StudioApiFixture): self._handouts = [] self._children = [] self._assets = [] + self._advanced_settings = {} def __str__(self): """ @@ -236,6 +237,12 @@ class CourseFixture(StudioApiFixture): """ self._assets.extend(asset_name) + def add_advanced_settings(self, settings): + """ + Adds advanced settings to be set on the course when the install method is called. + """ + self._advanced_settings.update(settings) + def install(self): """ Create the course and XBlocks within the course. @@ -248,6 +255,7 @@ class CourseFixture(StudioApiFixture): self._install_course_handouts() self._configure_course() self._upload_assets() + self._add_advanced_settings() self._create_xblock_children(self._course_location, self._children) return self @@ -415,6 +423,23 @@ class CourseFixture(StudioApiFixture): raise CourseFixtureError('Could not upload {asset_name} with {url}. Status code: {code}'.format( asset_name=asset_name, url=url, code=upload_response.status_code)) + def _add_advanced_settings(self): + """ + Add advanced settings. + """ + url = STUDIO_BASE_URL + "/settings/advanced/" + self._course_key + + # POST advanced settings to Studio + response = self.session.post( + url, data=self._encode_post_dict(self._advanced_settings), + headers=self.headers, + ) + + if not response.ok: + raise CourseFixtureError( + "Could not update advanced details to '{0}' with {1}: Status was {2}.".format( + self._advanced_settings, url, response.status_code)) + def _create_xblock_children(self, parent_loc, xblock_descriptions): """ Recursively create XBlock children. @@ -489,6 +514,6 @@ class CourseFixture(StudioApiFixture): Encode `post_dict` (a dictionary) as UTF-8 encoded JSON. """ return json.dumps({ - k: v.encode('utf-8') if v is not None else v + k: v.encode('utf-8') if isinstance(v, basestring) else v for k, v in post_dict.items() }) diff --git a/common/test/acceptance/pages/studio/component_editor.py b/common/test/acceptance/pages/studio/component_editor.py index 57990b93b1..ce87d1d6ad 100644 --- a/common/test/acceptance/pages/studio/component_editor.py +++ b/common/test/acceptance/pages/studio/component_editor.py @@ -2,6 +2,7 @@ from bok_choy.page_object import PageObject from selenium.webdriver.common.keys import Keys from selenium.webdriver.common.action_chains import ActionChains from utils import click_css +from selenium.webdriver.support.ui import Select class ComponentEditorView(PageObject): @@ -40,7 +41,7 @@ class ComponentEditorView(PageObject): """ return None - def get_setting_entry_index(self, label): + def get_setting_element(self, label): """ Returns the index of the setting entry with given label (display name) within the Settings modal. """ @@ -48,15 +49,14 @@ class ComponentEditorView(PageObject): setting_labels = self.q(css=self._bounded_selector('.metadata_edit .wrapper-comp-setting .setting-label')) for index, setting in enumerate(setting_labels): if setting.text == label: - return index + return self.q(css=self._bounded_selector('.metadata_edit div.wrapper-comp-setting .setting-input'))[index] return None def set_field_value_and_save(self, label, value): """ - Set the field with given label (display name) to the specified value, and presses Save. + Sets the text field with given label (display name) to the specified value, and presses Save. """ - index = self.get_setting_entry_index(label) - elem = self.q(css=self._bounded_selector('.metadata_edit div.wrapper-comp-setting input.setting-input'))[index] + elem = self.get_setting_element(label) # Click in the field, delete the value there. action = ActionChains(self.browser).click(elem) for _x in range(0, len(elem.get_attribute('value'))): @@ -64,3 +64,12 @@ class ComponentEditorView(PageObject): # Send the new text, then Tab to move to the next field (so change event is triggered). action.send_keys(value).send_keys(Keys.TAB).perform() click_css(self, 'a.action-save') + + def set_select_value_and_save(self, label, value): + """ + Sets the select with given label (display name) to the specified value, and presses Save. + """ + elem = self.get_setting_element(label) + select = Select(elem) + select.select_by_value(value) + click_css(self, 'a.action-save') diff --git a/common/test/acceptance/pages/studio/container.py b/common/test/acceptance/pages/studio/container.py index 82baa3b673..6fee6d8b73 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -54,7 +54,24 @@ class ContainerPage(PageObject): """ Return a list of xblocks loaded on the container page. """ - return self.q(css=XBlockWrapper.BODY_SELECTOR).map( + return self._get_xblocks() + + @property + def inactive_xblocks(self): + """ + Return a list of inactive xblocks loaded on the container page. + """ + return self._get_xblocks(".is-inactive ") + + @property + def active_xblocks(self): + """ + Return a list of active xblocks loaded on the container page. + """ + return self._get_xblocks(".is-active ") + + def _get_xblocks(self, prefix=""): + return self.q(css=prefix + XBlockWrapper.BODY_SELECTOR).map( lambda el: XBlockWrapper(self.browser, el.get_attribute('data-locator'))).results def drag(self, source_index, target_index): @@ -77,15 +94,6 @@ class ContainerPage(PageObject): ).release().perform() wait_for_notification(self) - def add_discussion(self, menu_index): - """ - Add a new instance of the discussion category. - - menu_index specifies which instance of the menus should be used (based on vertical - placement within the page). - """ - click_css(self, 'a>span.large-discussion-icon', menu_index) - def duplicate(self, source_index): """ Duplicate the item with index source_index (based on vertical placement in page). @@ -101,6 +109,11 @@ class ContainerPage(PageObject): click_css(self, 'a.button.action-primary', 0) def edit(self): + """ + Clicks the "edit" button for the first component on the page. + + Same as the implementation in unit.py, unit and component pages will be merging. + """ self.q(css='.edit-button').first.click() EmptyPromise( lambda: self.q(css='.xblock-studio_view').present, @@ -109,6 +122,18 @@ class ContainerPage(PageObject): return self + def add_missing_groups(self): + """ + Click the "add missing groups" link. + """ + click_css(self, '.add-missing-groups-button') + + def missing_groups_button_present(self): + """ + Returns True if the "add missing groups" button is present. + """ + return self.q(css='.add-missing-groups-button').present + class XBlockWrapper(PageObject): """ @@ -161,4 +186,4 @@ class XBlockWrapper(PageObject): @property def preview_selector(self): - return self._bounded_selector('.xblock-student_view') + return self._bounded_selector('.xblock-student_view,.xblock-author_view') diff --git a/common/test/acceptance/pages/studio/unit.py b/common/test/acceptance/pages/studio/unit.py index 8e89a0d556..7eca47dfd3 100644 --- a/common/test/acceptance/pages/studio/unit.py +++ b/common/test/acceptance/pages/studio/unit.py @@ -27,7 +27,7 @@ class UnitPage(PageObject): def _is_finished_loading(): # Wait until all components have been loaded - number_of_leaf_xblocks = len(self.q(css='{} .xblock-student_view'.format(Component.BODY_SELECTOR)).results) + number_of_leaf_xblocks = len(self.q(css='{} .xblock-author_view,.xblock-student_view'.format(Component.BODY_SELECTOR)).results) is_done = len(self.q(css=Component.BODY_SELECTOR).results) == number_of_leaf_xblocks return (is_done, is_done) @@ -99,9 +99,14 @@ class Component(PageObject): @property def preview_selector(self): - return self._bounded_selector('.xblock-student_view') + return self._bounded_selector('.xblock-author_view,.xblock-student_view') def edit(self): + """ + Clicks the "edit" button for the first component on the page. + + Same as the implementation in unit.py, unit and component pages will be merging. + """ self.q(css=self._bounded_selector('.edit-button')).first.click() EmptyPromise( lambda: self.q(css='.xblock-studio_view').present, diff --git a/common/test/acceptance/pages/studio/utils.py b/common/test/acceptance/pages/studio/utils.py index 33bac1e8c1..b4e71cc02b 100644 --- a/common/test/acceptance/pages/studio/utils.py +++ b/common/test/acceptance/pages/studio/utils.py @@ -9,10 +9,12 @@ def click_css(page, css, source_index=0, require_notification=True): """ Click the button/link with the given css and index on the specified page (subclass of PageObject). + Will only consider buttons with a non-zero size. + If require_notification is False (default value is True), the method will return immediately. Otherwise, it will wait for the "mini-notification" to appear and disappear. """ - buttons = page.q(css=css) + buttons = page.q(css=css).filter(lambda el: el.size['width'] > 0) target = buttons[source_index] ActionChains(page.browser).click(target).release().perform() if require_notification: @@ -31,5 +33,36 @@ def wait_for_notification(page): num_notifications = len(page.q(css='.wrapper-notification-mini.is-hiding')) return (num_notifications == 1, num_notifications) - Promise(_is_saving, 'Notification showing.').fulfill() - Promise(_is_saving_done, 'Notification hidden.').fulfill() + Promise(_is_saving, 'Notification should have been shown.').fulfill() + Promise(_is_saving_done, 'Notification should have been hidden.').fulfill() + + +def add_discussion(page, menu_index): + """ + Add a new instance of the discussion category. + + menu_index specifies which instance of the menus should be used (based on vertical + placement within the page). + """ + click_css(page, 'a>span.large-discussion-icon', menu_index) + + +def add_advanced_component(page, menu_index, name): + """ + Adds an instance of the advanced component with the specified name. + + menu_index specifies which instance of the menus should be used (based on vertical + placement within the page). + """ + click_css(page, 'a>span.large-advanced-icon', menu_index, require_notification=False) + + # Sporadically, the advanced component was not getting created after the click_css call on the category (below). + # Try making sure that the menu of advanced components is visible before clicking (the HTML is always on the + # page, but will have display none until the large-advanced-icon is clicked). + def is_advanced_components_showing(): + advanced_buttons = page.q(css=".new-component-advanced").filter(lambda el: el.size['width'] > 0) + return (len(advanced_buttons) == 1, len(advanced_buttons)) + + Promise(is_advanced_components_showing, "Advanced component menu not showing").fulfill() + + click_css(page, 'a[data-category={}]'.format(name)) diff --git a/common/test/acceptance/tests/test_studio_container.py b/common/test/acceptance/tests/test_studio_container.py index ec1bfe3d50..66878b95e1 100644 --- a/common/test/acceptance/tests/test_studio_container.py +++ b/common/test/acceptance/tests/test_studio_container.py @@ -8,6 +8,7 @@ from ..fixtures.course import CourseFixture, XBlockFixtureDesc from .helpers import UniqueCourseTest from ..pages.studio.component_editor import ComponentEditorView +from ..pages.studio.utils import add_discussion from unittest import skip @@ -32,6 +33,87 @@ class ContainerBase(UniqueCourseTest): self.course_info['run'] ) + self.setup_fixtures() + + self.auth_page = AutoAuthPage( + self.browser, + staff=False, + username=self.user.get('username'), + email=self.user.get('email'), + password=self.user.get('password') + ) + + self.auth_page.visit() + + def setup_fixtures(self): + pass + + def go_to_container_page(self, make_draft=False): + """ + Go to the test container page. + + If make_draft is true, the unit page (accessed on way to container page) will be put into draft mode. + """ + unit = self.go_to_unit_page(make_draft) + container = unit.components[0].go_to_container() + return container + + def go_to_unit_page(self, make_draft=False): + """ + Go to the test unit page. + + If make_draft is true, the unit page will be put into draft mode. + """ + self.outline.visit() + subsection = self.outline.section('Test Section').subsection('Test Subsection') + unit = subsection.toggle_expand().unit('Test Unit').go_to() + if make_draft: + unit.edit_draft() + return unit + + def verify_ordering(self, container, expected_orderings): + """ + Verifies the expected ordering of xblocks on the page. + """ + xblocks = container.xblocks + blocks_checked = set() + for expected_ordering in expected_orderings: + for xblock in xblocks: + parent = expected_ordering.keys()[0] + if xblock.name == parent: + blocks_checked.add(parent) + children = xblock.children + expected_length = len(expected_ordering.get(parent)) + self.assertEqual( + expected_length, len(children), + "Number of children incorrect for group {0}. Expected {1} but got {2}.".format(parent, expected_length, len(children))) + for idx, expected in enumerate(expected_ordering.get(parent)): + self.assertEqual(expected, children[idx].name) + blocks_checked.add(expected) + break + self.assertEqual(len(blocks_checked), len(xblocks)) + + def do_action_and_verify(self, action, expected_ordering): + """ + Perform the supplied action and then verify the resulting ordering. + """ + container = self.go_to_container_page(make_draft=True) + action(container) + + self.verify_ordering(container, expected_ordering) + + # Reload the page to see that the change was persisted. + container = self.go_to_container_page() + self.verify_ordering(container, expected_ordering) + + +class NestedVerticalTest(ContainerBase): + __test__ = False + + """ + Sets up a course structure with nested verticals. + """ + def setup_fixtures(self): self.container_title = "" self.group_a = "Expand or Collapse\nGroup A" self.group_b = "Expand or Collapse\nGroup B" @@ -55,18 +137,6 @@ class ContainerBase(UniqueCourseTest): self.duplicate_label = "Duplicate of '{0}'" self.discussion_label = "Discussion" - self.setup_fixtures() - - self.auth_page = AutoAuthPage( - self.browser, - staff=False, - username=self.user.get('username'), - email=self.user.get('email'), - password=self.user.get('password') - ) - self.auth_page.visit() - - def setup_fixtures(self): course_fix = CourseFixture( self.course_info['org'], self.course_info['number'], @@ -96,46 +166,8 @@ class ContainerBase(UniqueCourseTest): self.user = course_fix.user - def go_to_container_page(self, make_draft=False): - unit = self.go_to_unit_page(make_draft) - container = unit.components[0].go_to_container() - return container - def go_to_unit_page(self, make_draft=False): - self.outline.visit() - subsection = self.outline.section('Test Section').subsection('Test Subsection') - unit = subsection.toggle_expand().unit('Test Unit').go_to() - if make_draft: - unit.edit_draft() - return unit - - def verify_ordering(self, container, expected_orderings): - xblocks = container.xblocks - for expected_ordering in expected_orderings: - for xblock in xblocks: - parent = expected_ordering.keys()[0] - if xblock.name == parent: - children = xblock.children - expected_length = len(expected_ordering.get(parent)) - self.assertEqual( - expected_length, len(children), - "Number of children incorrect for group {0}. Expected {1} but got {2}.".format(parent, expected_length, len(children))) - for idx, expected in enumerate(expected_ordering.get(parent)): - self.assertEqual(expected, children[idx].name) - break - - def do_action_and_verify(self, action, expected_ordering): - container = self.go_to_container_page(make_draft=True) - action(container) - - self.verify_ordering(container, expected_ordering) - - # Reload the page to see that the change was persisted. - container = self.go_to_container_page() - self.verify_ordering(container, expected_ordering) - - -class DragAndDropTest(ContainerBase): +class DragAndDropTest(NestedVerticalTest): """ Tests of reordering within the container page. """ @@ -196,7 +228,7 @@ class DragAndDropTest(ContainerBase): def add_new_components_and_rearrange(container): # Add a video component to Group 1 - container.add_discussion(group_a_menu) + add_discussion(container, group_a_menu) # Duplicate the first item in Group A container.duplicate(self.group_a_item_1_action_index) @@ -216,7 +248,7 @@ class DragAndDropTest(ContainerBase): self.do_action_and_verify(add_new_components_and_rearrange, expected_ordering) -class AddComponentTest(ContainerBase): +class AddComponentTest(NestedVerticalTest): """ Tests of adding a component to the container page. """ @@ -224,7 +256,7 @@ class AddComponentTest(ContainerBase): def add_and_verify(self, menu_index, expected_ordering): self.do_action_and_verify( - lambda (container): container.add_discussion(menu_index), + lambda (container): add_discussion(container, menu_index), expected_ordering ) @@ -256,7 +288,7 @@ class AddComponentTest(ContainerBase): self.add_and_verify(container_menu, expected_ordering) -class DuplicateComponentTest(ContainerBase): +class DuplicateComponentTest(NestedVerticalTest): """ Tests of duplicating a component on the container page. """ @@ -302,7 +334,7 @@ class DuplicateComponentTest(ContainerBase): self.do_action_and_verify(duplicate_twice, expected_ordering) -class DeleteComponentTest(ContainerBase): +class DeleteComponentTest(NestedVerticalTest): """ Tests of deleting a component from the container page. """ @@ -319,10 +351,13 @@ class DeleteComponentTest(ContainerBase): {self.group_a: [self.group_a_item_2]}, {self.group_b: [self.group_b_item_1, self.group_b_item_2]}, {self.group_empty: []}] - self.delete_and_verify(self.group_a_item_1_action_index, expected_ordering) + + # Group A itself has a delete icon now, so item_1 is index 1 instead of 0. + group_a_item_1_delete_index = 1 + self.delete_and_verify(group_a_item_1_delete_index, expected_ordering) -class EditContainerTest(ContainerBase): +class EditContainerTest(NestedVerticalTest): """ Tests of editing a container. """ diff --git a/common/test/acceptance/tests/test_studio_split_test.py b/common/test/acceptance/tests/test_studio_split_test.py new file mode 100644 index 0000000000..a4df7dc168 --- /dev/null +++ b/common/test/acceptance/tests/test_studio_split_test.py @@ -0,0 +1,156 @@ +""" +Acceptance tests for Studio related to the split_test module. +""" + +from unittest import skip + +from ..fixtures.course import CourseFixture, XBlockFixtureDesc + +from ..pages.studio.component_editor import ComponentEditorView +from test_studio_container import ContainerBase +from ..pages.studio.utils import add_advanced_component +from xmodule.partitions.partitions import Group, UserPartition +from bok_choy.promise import Promise + + +class SplitTest(ContainerBase): + """ + Tests for creating and editing split test instances in Studio. + """ + __test__ = True + + def setup_fixtures(self): + course_fix = CourseFixture( + self.course_info['org'], + self.course_info['number'], + self.course_info['run'], + self.course_info['display_name'] + ) + + course_fix.add_advanced_settings( + { + u"advanced_modules": ["split_test"], + u"user_partitions": [ + UserPartition(0, 'Configuration alpha,beta', 'first', [Group("0", 'alpha'), Group("1", 'beta')]).to_json(), + UserPartition(1, 'Configuration 0,1,2', 'second', [Group("0", 'Group 0'), Group("1", 'Group 1'), Group("2", 'Group 2')]).to_json() + ] + } + ) + + course_fix.add_children( + XBlockFixtureDesc('chapter', 'Test Section').add_children( + XBlockFixtureDesc('sequential', 'Test Subsection').add_children( + XBlockFixtureDesc('vertical', 'Test Unit') + ) + ) + ).install() + + self.course_fix = course_fix + + self.user = course_fix.user + + def verify_groups(self, container, active_groups, inactive_groups, verify_missing_groups_not_present=True): + """ + Check that the groups appear and are correctly categorized as to active and inactive. + + Also checks that the "add missing groups" button/link is not present unless a value of False is passed + for verify_missing_groups_not_present. + """ + def wait_for_xblocks_to_render(): + # First xblock is the container for the page, subtract 1. + return (len(active_groups) + len(inactive_groups) == len(container.xblocks) - 1, len(active_groups)) + + Promise(wait_for_xblocks_to_render, "Number of xblocks on the page are incorrect").fulfill() + + def check_xblock_names(expected_groups, actual_blocks): + self.assertEqual(len(expected_groups), len(actual_blocks)) + for idx, expected in enumerate(expected_groups): + self.assertEqual('Expand or Collapse\n{}'.format(expected), actual_blocks[idx].name) + + check_xblock_names(active_groups, container.active_xblocks) + check_xblock_names(inactive_groups, container.inactive_xblocks) + + # Verify inactive xblocks appear after active xblocks + check_xblock_names(active_groups + inactive_groups, container.xblocks[1:]) + + if verify_missing_groups_not_present: + self.verify_add_missing_groups_button_not_present(container) + + def verify_add_missing_groups_button_not_present(self, container): + """ + Checks that the "add missing gorups" button/link is not present. + """ + def missing_groups_button_not_present(): + button_present = container.missing_groups_button_present() + return (not button_present, not button_present) + + Promise(missing_groups_button_not_present, "Add missing groups button should not be showing.").fulfill() + + def create_poorly_configured_split_instance(self): + """ + Creates a split test instance with a missing group and an inactive group. + + Returns the container page. + """ + unit = self.go_to_unit_page(make_draft=True) + add_advanced_component(unit, 0, 'split_test') + container = self.go_to_container_page() + container.edit() + component_editor = ComponentEditorView(self.browser, container.locator) + component_editor.set_select_value_and_save('Group Configuration', 'Configuration alpha,beta') + self.course_fix.add_advanced_settings( + { + u"user_partitions": [ + UserPartition(0, 'Configuration alpha,beta', 'first', + [Group("0", 'alpha'), Group("2", 'gamma')]).to_json() + ] + } + ) + self.course_fix._add_advanced_settings() + return self.go_to_container_page() + + def test_create_and_select_group_configuration(self): + """ + Tests creating a split test instance on the unit page, and then + assigning the group configuration. + """ + unit = self.go_to_unit_page(make_draft=True) + add_advanced_component(unit, 0, 'split_test') + container = self.go_to_container_page() + container.edit() + component_editor = ComponentEditorView(self.browser, container.locator) + component_editor.set_select_value_and_save('Group Configuration', 'Configuration alpha,beta') + self.verify_groups(container, ['alpha', 'beta'], []) + + # Switch to the other group configuration. Must navigate again to the container page so + # that there is only a single "editor" on the page. + container = self.go_to_container_page() + container.edit() + component_editor = ComponentEditorView(self.browser, container.locator) + component_editor.set_select_value_and_save('Group Configuration', 'Configuration 0,1,2') + self.verify_groups(container, ['Group 0', 'Group 1', 'Group 2'], ['alpha', 'beta']) + + # Reload the page to make sure the groups were persisted. + container = self.go_to_container_page() + self.verify_groups(container, ['Group 0', 'Group 1', 'Group 2'], ['alpha', 'beta']) + + @skip("This fails periodically where it fails to trigger the add missing groups action.Dis") + def test_missing_group(self): + """ + The case of a split test with invalid configuration (missing group). + """ + container = self.create_poorly_configured_split_instance() + container.add_missing_groups() + self.verify_groups(container, ['alpha', 'gamma'], ['beta']) + + # Reload the page to make sure the groups were persisted. + container = self.go_to_container_page() + self.verify_groups(container, ['alpha', 'gamma'], ['beta']) + + def test_delete_inactive_group(self): + """ + Test deleting an inactive group. + """ + container = self.create_poorly_configured_split_instance() + container.delete(0) + self.verify_groups(container, ['alpha'], [], verify_missing_groups_not_present=False) diff --git a/docs/en_us/developers/source/xblocks.rst b/docs/en_us/developers/source/xblocks.rst index a540b21da8..84cc914ef4 100644 --- a/docs/en_us/developers/source/xblocks.rst +++ b/docs/en_us/developers/source/xblocks.rst @@ -40,8 +40,9 @@ Class Features These are class attributes or functions that can be provided by an XBlock to customize behaviour in the LMS. -* student_view (XBlock view): This is the view that will be rendered to display - the XBlock in the LMS. +* student_view (XBlock view): This is the view that will be rendered to display the XBlock + in the LMS. It will also be used to render the block in "preview" mode in Studio, unless + the XBlock also implements author_view. * has_score (class property): True if this block should appear in the LMS progress page. * get_progress (method): See documentation in x_module.py:XModuleMixin.get_progress. * icon_class (class property): This can be one of (``other``, ``video``, or ``problem``), and @@ -77,7 +78,11 @@ Studio Class Features ~~~~~~~~~~~~~~ -* studio_view (XBlock.view): The view used to render an editor in Studio. +* studio_view (XBlock.view): The view used to render an editor in Studio. The editor rendering can +be completely different from the LMS student_view, and it is only shown when the author selects "Edit". +* author_view (XBlock.view): An optional view of the XBlock similar to student_view, but with possible inline +editing capabilities. This view differs from studio_view in that it should be as similar to student_view +as possible. When previewing XBlocks within Studio, Studio will prefer author_view to student_view. * non_editable_metadata_fields (property): A list of :class:`~xblock.fields.Field` objects that shouldn't be displayed in the default editing view for Studio. diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 3d524699e7..81e7a2f072 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -15,6 +15,7 @@ from xmodule.contentstore.content import StaticContent from xmodule.modulestore.exceptions import ItemNotFoundError from static_replace import replace_static_urls from xmodule.modulestore import MONGO_MODULESTORE_TYPE +from xmodule.x_module import STUDENT_VIEW from courseware.access import has_access from courseware.model_data import FieldDataCache @@ -196,7 +197,7 @@ def get_course_about_section(course, section_key): if about_module is not None: try: - html = about_module.render('student_view').content + html = about_module.render(STUDENT_VIEW).content except Exception: # pylint: disable=broad-except html = render_to_string('courseware/error-message.html', None) log.exception( @@ -250,7 +251,7 @@ def get_course_info_section(request, course, section_key): if info_module is not None: try: - html = info_module.render('student_view').content + html = info_module.render(STUDENT_VIEW).content except Exception: # pylint: disable=broad-except html = render_to_string('courseware/error-message.html', None) log.exception( diff --git a/lms/djangoapps/courseware/tests/test_lti_integration.py b/lms/djangoapps/courseware/tests/test_lti_integration.py index 0d95fd0172..6b53ac7a3e 100644 --- a/lms/djangoapps/courseware/tests/test_lti_integration.py +++ b/lms/djangoapps/courseware/tests/test_lti_integration.py @@ -12,6 +12,7 @@ from django.conf import settings from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.x_module import STUDENT_VIEW from courseware.tests import BaseTestXmodule from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE @@ -108,7 +109,7 @@ class TestLTI(BaseTestXmodule): self.addCleanup(patcher.stop) def test_lti_constructor(self): - generated_content = self.item_descriptor.render('student_view').content + generated_content = self.item_descriptor.render(STUDENT_VIEW).content expected_content = self.runtime.render_template('lti.html', self.expected_context) self.assertEqual(generated_content, expected_content) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 4f58281c4a..c9770ff691 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -22,7 +22,7 @@ from xmodule.lti_module import LTIDescriptor from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory -from xmodule.x_module import XModuleDescriptor +from xmodule.x_module import XModuleDescriptor, STUDENT_VIEW from opaque_keys.edx.locations import SlashSeparatedCourseKey from courseware import module_render as render @@ -94,7 +94,7 @@ class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase): ) # get the rendered HTML output which should have the rewritten link - html = module.render('student_view').content + html = module.render(STUDENT_VIEW).content # See if the url got rewritten to the target link # note if the URL mapping changes then this assertion will break @@ -416,7 +416,7 @@ class TestHtmlModifiers(ModuleStoreTestCase): self.course.id, wrap_xmodule_display=True, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertIn('div class="xblock xblock-student_view xmodule_display xmodule_HtmlModule"', result_fragment.content) @@ -429,7 +429,7 @@ class TestHtmlModifiers(ModuleStoreTestCase): self.course.id, wrap_xmodule_display=False, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertNotIn('div class="xblock xblock-student_view xmodule_display xmodule_HtmlModule"', result_fragment.content) @@ -441,7 +441,7 @@ class TestHtmlModifiers(ModuleStoreTestCase): self.field_data_cache, self.course.id, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertIn( '/c4x/{org}/{course}/asset/foo_content'.format( @@ -459,7 +459,7 @@ class TestHtmlModifiers(ModuleStoreTestCase): self.field_data_cache, self.course.id, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertIn( '/c4x/{org}/{course}/asset/_file.jpg'.format( @@ -483,7 +483,7 @@ class TestHtmlModifiers(ModuleStoreTestCase): self.course.id, static_asset_path="toy_course_dir", ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertIn('href="/static/toy_course_dir', result_fragment.content) def test_course_image(self): @@ -509,7 +509,7 @@ class TestHtmlModifiers(ModuleStoreTestCase): self.field_data_cache, self.course.id, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertIn( '/courses/{course_id}/bar/content'.format( @@ -590,14 +590,14 @@ class MongoViewInStudioTest(ViewInStudioTest): def test_view_in_studio_link_studio_course(self): """Regular Studio courses should see 'View in Studio' links.""" self.setup_mongo_course() - result_fragment = self.module.render('student_view') + result_fragment = self.module.render(STUDENT_VIEW) self.assertIn('View Unit in Studio', result_fragment.content) def test_view_in_studio_link_only_in_top_level_vertical(self): """Regular Studio courses should not see 'View in Studio' for child verticals of verticals.""" self.setup_mongo_course() # Render the parent vertical, then check that there is only a single "View Unit in Studio" link. - result_fragment = self.module.render('student_view') + result_fragment = self.module.render(STUDENT_VIEW) # The single "View Unit in Studio" link should appear before the first xmodule vertical definition. parts = result_fragment.content.split('xmodule_VerticalModule') self.assertEqual(3, len(parts), "Did not find two vertical modules") @@ -608,7 +608,7 @@ class MongoViewInStudioTest(ViewInStudioTest): def test_view_in_studio_link_xml_authored(self): """Courses that change 'course_edit_method' setting can hide 'View in Studio' links.""" self.setup_mongo_course(course_edit_method='XML') - result_fragment = self.module.render('student_view') + result_fragment = self.module.render(STUDENT_VIEW) self.assertNotIn('View Unit in Studio', result_fragment.content) @@ -622,19 +622,19 @@ class MixedViewInStudioTest(ViewInStudioTest): def test_view_in_studio_link_mongo_backed(self): """Mixed mongo courses that are mongo backed should see 'View in Studio' links.""" self.setup_mongo_course() - result_fragment = self.module.render('student_view') + result_fragment = self.module.render(STUDENT_VIEW) self.assertIn('View Unit in Studio', result_fragment.content) def test_view_in_studio_link_xml_authored(self): """Courses that change 'course_edit_method' setting can hide 'View in Studio' links.""" self.setup_mongo_course(course_edit_method='XML') - result_fragment = self.module.render('student_view') + result_fragment = self.module.render(STUDENT_VIEW) self.assertNotIn('View Unit in Studio', result_fragment.content) def test_view_in_studio_link_xml_backed(self): """Course in XML only modulestore should not see 'View in Studio' links.""" self.setup_xml_course() - result_fragment = self.module.render('student_view') + result_fragment = self.module.render(STUDENT_VIEW) self.assertNotIn('View Unit in Studio', result_fragment.content) @@ -648,7 +648,7 @@ class XmlViewInStudioTest(ViewInStudioTest): def test_view_in_studio_link_xml_backed(self): """Course in XML only modulestore should not see 'View in Studio' links.""" self.setup_xml_course() - result_fragment = self.module.render('student_view') + result_fragment = self.module.render(STUDENT_VIEW) self.assertNotIn('View Unit in Studio', result_fragment.content) @@ -694,7 +694,7 @@ class TestStaffDebugInfo(ModuleStoreTestCase): self.field_data_cache, self.course.id, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertNotIn('Staff Debug', result_fragment.content) def test_staff_debug_info_enabled(self): @@ -705,7 +705,7 @@ class TestStaffDebugInfo(ModuleStoreTestCase): self.field_data_cache, self.course.id, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertIn('Staff Debug', result_fragment.content) @patch.dict('django.conf.settings.FEATURES', {'DISPLAY_HISTOGRAMS_TO_STAFF': False}) @@ -717,7 +717,7 @@ class TestStaffDebugInfo(ModuleStoreTestCase): self.field_data_cache, self.course.id, ) - result_fragment = module.render('student_view') + result_fragment = module.render(STUDENT_VIEW) self.assertNotIn('histrogram', result_fragment.content) def test_histogram_enabled_for_unscored_xmodules(self): @@ -741,7 +741,7 @@ class TestStaffDebugInfo(ModuleStoreTestCase): field_data_cache, self.course.id, ) - module.render('student_view') + module.render(STUDENT_VIEW) self.assertFalse(mock_grade_histogram.called) def test_histogram_enabled_for_scored_xmodules(self): @@ -764,7 +764,7 @@ class TestStaffDebugInfo(ModuleStoreTestCase): self.field_data_cache, self.course.id, ) - module.render('student_view') + module.render(STUDENT_VIEW) self.assertTrue(mock_grade_histogram.called) diff --git a/lms/djangoapps/courseware/tests/test_video_handlers.py b/lms/djangoapps/courseware/tests/test_video_handlers.py index 28126a4970..0a502cb77d 100644 --- a/lms/djangoapps/courseware/tests/test_video_handlers.py +++ b/lms/djangoapps/courseware/tests/test_video_handlers.py @@ -12,6 +12,7 @@ from webob import Request from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import editable_modulestore +from xmodule.x_module import STUDENT_VIEW from . import BaseTestXmodule from .test_video_xml import SOURCE_XML from cache_toolbox.core import del_cached_content @@ -175,7 +176,7 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo): def setUp(self): super(TestTranscriptAvailableTranslationsDispatch, self).setUp() - self.item_descriptor.render('student_view') + self.item_descriptor.render(STUDENT_VIEW) self.item = self.item_descriptor.xmodule_runtime.xmodule_instance self.subs = {"start": [10], "end": [100], "text": ["Hi, welcome to Edx."]} @@ -234,7 +235,7 @@ class TestTranscriptDownloadDispatch(TestVideo): def setUp(self): super(TestTranscriptDownloadDispatch, self).setUp() - self.item_descriptor.render('student_view') + self.item_descriptor.render(STUDENT_VIEW) self.item = self.item_descriptor.xmodule_runtime.xmodule_instance def test_download_transcript_not_exist(self): @@ -299,7 +300,7 @@ class TestTranscriptTranslationGetDispatch(TestVideo): def setUp(self): super(TestTranscriptTranslationGetDispatch, self).setUp() - self.item_descriptor.render('student_view') + self.item_descriptor.render(STUDENT_VIEW) self.item = self.item_descriptor.xmodule_runtime.xmodule_instance def test_translation_fails(self): @@ -609,7 +610,7 @@ class TestGetTranscript(TestVideo): def setUp(self): super(TestGetTranscript, self).setUp() - self.item_descriptor.render('student_view') + self.item_descriptor.render(STUDENT_VIEW) self.item = self.item_descriptor.xmodule_runtime.xmodule_instance def test_good_transcript(self): diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index d4053753e3..0a8beca65a 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -13,6 +13,7 @@ from xblock.field_data import DictFieldData from xmodule.video_module import create_youtube_string from xmodule.tests import get_test_descriptor_system from xmodule.video_module import VideoDescriptor +from xmodule.x_module import STUDENT_VIEW from opaque_keys.edx.locations import SlashSeparatedCourseKey from . import BaseTestXmodule @@ -25,7 +26,7 @@ class TestVideoYouTube(TestVideo): def test_video_constructor(self): """Make sure that all parameters extracted correctly from xml""" - context = self.item_descriptor.render('student_view').content + context = self.item_descriptor.render(STUDENT_VIEW).content sources = json.dumps([u'example.mp4', u'example.webm']) expected_context = { @@ -89,7 +90,7 @@ class TestVideoNonYouTube(TestVideo): """Make sure that if the 'youtube' attribute is omitted in XML, then the template generates an empty string for the YouTube streams. """ - context = self.item_descriptor.render('student_view').content + context = self.item_descriptor.render(STUDENT_VIEW).content sources = json.dumps([u'example.mp4', u'example.webm']) expected_context = { @@ -231,7 +232,7 @@ class TestGetHtmlMethod(BaseTestXmodule): self.item_descriptor, 'transcript', 'download' ).rstrip('/?') - context = self.item_descriptor.render('student_view').content + context = self.item_descriptor.render(STUDENT_VIEW).content expected_context.update({ 'transcript_download_format': None if self.item_descriptor.track and self.item_descriptor.download_track else 'srt', @@ -344,7 +345,7 @@ class TestGetHtmlMethod(BaseTestXmodule): sources=data['sources'] ) self.initialize_module(data=DATA) - context = self.item_descriptor.render('student_view').content + context = self.item_descriptor.render(STUDENT_VIEW).content expected_context = dict(initial_context) expected_context.update({ diff --git a/lms/djangoapps/courseware/tests/test_word_cloud.py b/lms/djangoapps/courseware/tests/test_word_cloud.py index 3d2a235e41..1721abd9b9 100644 --- a/lms/djangoapps/courseware/tests/test_word_cloud.py +++ b/lms/djangoapps/courseware/tests/test_word_cloud.py @@ -5,6 +5,7 @@ import json from operator import itemgetter from . import BaseTestXmodule +from xmodule.x_module import STUDENT_VIEW class TestWordCloud(BaseTestXmodule): @@ -242,7 +243,7 @@ class TestWordCloud(BaseTestXmodule): def test_word_cloud_constructor(self): """Make sure that all parameters extracted correclty from xml""" - fragment = self.runtime.render(self.item_descriptor, 'student_view') + fragment = self.runtime.render(self.item_descriptor, STUDENT_VIEW) expected_context = { 'ajax_url': self.item_descriptor.xmodule_runtime.ajax_url, diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index ce9a9d15a5..dc256dbc5d 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -42,6 +42,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from xmodule.modulestore.search import path_to_location from xmodule.tabs import CourseTabList, StaffGradingTab, PeerGradingTab, OpenEndedGradingTab +from xmodule.x_module import STUDENT_VIEW import shoppingcart from opaque_keys import InvalidKeyError @@ -377,7 +378,7 @@ def index(request, course_id, chapter=None, section=None, # Save where we are in the chapter save_child_position(chapter_module, section) - context['fragment'] = section_module.render('student_view') + context['fragment'] = section_module.render(STUDENT_VIEW) context['section_title'] = section_descriptor.display_name_with_default else: # section is none, so display a message @@ -865,7 +866,7 @@ def get_static_tab_contents(request, course, tab): html = '' if tab_module is not None: try: - html = tab_module.render('student_view').content + html = tab_module.render(STUDENT_VIEW).content except Exception: # pylint: disable=broad-except html = render_to_string('courseware/error-message.html', None) log.exception( diff --git a/lms/templates/split_test_author_view.html b/lms/templates/split_test_author_view.html new file mode 100644 index 0000000000..cd4e239811 --- /dev/null +++ b/lms/templates/split_test_author_view.html @@ -0,0 +1,89 @@ +<%! from django.utils.translation import ugettext as _ %> +<%! from xmodule.split_test_module import ValidationMessageType %> + +<% +split_test = context.get('split_test') +user_partition = split_test.descriptor.get_selected_partition() +messages = split_test.descriptor.validation_messages() +%> + +% if is_root and not is_configured: +
            +% else: +
            +% endif + +% if user_partition: +
            +

            + + ${_("This content experiment uses group configuration '{experiment_name}'.").format(experiment_name=user_partition.name)} + +

            +
            +% endif +% if len(messages) > 0: + <% + def get_validation_icon(validation_type): + if validation_type == 'error': + return 'icon-exclamation-sign' + elif validation_type == 'warning': + return 'icon-warning-sign' + return None + + error_messages = (message for message in messages if message.message_type==ValidationMessageType.error) + has_errors = next(error_messages, False) + aggregate_validation_class = 'has-errors' if has_errors else 'has-warnings' + aggregate_validation_type = 'error' if has_errors else 'warning' + %> +
            + % if is_configured: +

            + ${_("This content experiment has issues that affect content visibility.")} +

            + % endif + % if is_root or not is_configured: +
              + % for message in messages: + <% + message_type = message.message_type + message_type_display_name = ValidationMessageType.display_name(message_type) if message_type else None + %> +
            • + % if not is_configured: + + % endif + + % if message_type_display_name: + ${message_type_display_name}: + % endif + ${unicode(message)} + + % if message.action_class: + + ${message.action_label} + + % endif + +
              • + % endfor +
            + % endif +
            +% endif + +
            + +% if is_root: +
            +

            ${_("Active Groups")}

            + ${active_groups_preview} +
            + + % if inactive_groups_preview: +
            +

            ${_("Inactive Groups")}

            + ${inactive_groups_preview} +
            + % endif +% endif diff --git a/lms/templates/split_test_studio_header.html b/lms/templates/split_test_studio_header.html deleted file mode 100644 index a63182640c..0000000000 --- a/lms/templates/split_test_studio_header.html +++ /dev/null @@ -1,47 +0,0 @@ -<%! from django.utils.translation import ugettext as _ %> -<%! from xmodule.split_test_module import ValidationMessageType %> - -<% -split_test = context.get('split_test') -(message, message_type) = split_test.descriptor.validation_message() -message_type_display_name = ValidationMessageType.display_name(message_type) if message_type else None -is_configured = split_test.user_partition_id >= 0 -%> - -% if message or not is_configured: - % if is_root and not is_configured: -
            - % else: -
            -
            - % endif - - % if not is_configured: -

            ${_("You must select a group configuration for this content experiment.")} - - ${_("Select a Group Configuration")} - -

            - % else: -

            - % if message_type == 'warning': - - % elif message_type == 'error': - - % endif - - % if message_type_display_name: - ${message_type_display_name}: - % endif - ${message} - -

            - % endif - - % if is_root and not is_configured: -
            - % else: -
            -
            - % endif -% endif