diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index 9dd4a0bcb7..4b9ddde8d5 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -7,7 +7,6 @@ from contentstore.utils import reverse_course_url, reverse_usage_url from contentstore.views.component import SPLIT_TEST_COMPONENT_TYPE from contentstore.views.course import GroupConfiguration from contentstore.tests.utils import CourseTestCase -from util.testing import UrlResetMixin from xmodule.partitions.partitions import Group, UserPartition from xmodule.modulestore.tests.factories import ItemFactory from xmodule.split_test_module import ValidationMessage, ValidationMessageType @@ -165,11 +164,10 @@ class GroupConfigurationsBaseTestCase(object): # pylint: disable=no-member -class GroupConfigurationsListHandlerTestCase(UrlResetMixin, CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): +class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): """ Test cases for group_configurations_list_handler. """ - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_GROUP_CONFIGURATIONS": True}) def setUp(self): """ Set up GroupConfigurationsListHandlerTestCase. @@ -261,14 +259,13 @@ class GroupConfigurationsListHandlerTestCase(UrlResetMixin, CourseTestCase, Grou # pylint: disable=no-member -class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): +class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): """ Test cases for group_configurations_detail_handler. """ ID = 0 - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_GROUP_CONFIGURATIONS": True}) def setUp(self): """ Set up GroupConfigurationsDetailHandlerTestCase. @@ -420,12 +417,11 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr # pylint: disable=no-member -class GroupConfigurationsUsageInfoTestCase(UrlResetMixin, CourseTestCase, HelperMethods): +class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): """ Tests for usage information of configurations. """ - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_GROUP_CONFIGURATIONS": True}) def setUp(self): super(GroupConfigurationsUsageInfoTestCase, self).setUp() @@ -542,7 +538,6 @@ class GroupConfigurationsValidationTestCase(CourseTestCase, HelperMethods): """ Tests for validation in Group Configurations. """ - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_GROUP_CONFIGURATIONS": True}) def setUp(self): super(GroupConfigurationsValidationTestCase, self).setUp() diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index ef2bba6d08..c613ab59d6 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -180,7 +180,6 @@ class GetItem(ItemTest): self.assertIn('Zooming', html) - @skipUnless(os.environ.get('FEATURE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature') def test_split_test_edited(self): """ Test that rename of a group changes display name of child vertical. @@ -194,7 +193,7 @@ class GetItem(ItemTest): resp = self.create_xblock(category='split_test', parent_usage_key=root_usage_key) split_test_usage_key = self.response_usage_key(resp) self.client.ajax_post( - reverse_usage_url("xblock_handler", split_test_usage_key), + reverse_usage_url("xblock_handler", split_test_usage_key), data={'metadata': {'user_partition_id': str(0)}} ) html, __ = self._get_container_preview(split_test_usage_key) diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index f716f717bc..09d87a2b21 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -24,6 +24,7 @@ class CourseMetadata(object): 'graded', 'hide_from_toc', 'pdf_textbooks', + 'user_partitions', 'name', # from xblock 'tags', # from xblock 'visible_to_staff_only' diff --git a/cms/envs/common.py b/cms/envs/common.py index ee4a1969a9..5f8ebfdb17 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -104,9 +104,6 @@ FEATURES = { # Turn off Advanced Security by default 'ADVANCED_SECURITY': False, - # Toggles Group Configuration editing functionality - 'ENABLE_GROUP_CONFIGURATIONS': os.environ.get('FEATURE_GROUP_CONFIGURATIONS'), - # Modulestore to use for new courses 'DEFAULT_STORE_FOR_NEW_COURSE': 'mongo', } diff --git a/cms/static/js/spec/views/group_configuration_spec.js b/cms/static/js/spec/views/group_configuration_spec.js index 3b3d75be9a..c9fa5e7925 100644 --- a/cms/static/js/spec/views/group_configuration_spec.js +++ b/cms/static/js/spec/views/group_configuration_spec.js @@ -381,7 +381,7 @@ define([ this.view.$('form').submit(); // See error message expect(this.view.$(SELECTORS.errorMessage)).toContainText( - 'Group Configuration name is required' + 'Group Configuration name is required.' ); // No request expect(requests.length).toBe(0); diff --git a/cms/templates/settings.html b/cms/templates/settings.html index b0facad5de..9065103cf4 100644 --- a/cms/templates/settings.html +++ b/cms/templates/settings.html @@ -330,7 +330,7 @@ require(["domReady!", "jquery", "js/models/settings/course_details", "js/views/s - % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules: + % if "split_test" in context_course.advanced_modules: % endif diff --git a/cms/templates/settings_advanced.html b/cms/templates/settings_advanced.html index 386c72adfa..4cbdf4cfed 100644 --- a/cms/templates/settings_advanced.html +++ b/cms/templates/settings_advanced.html @@ -126,7 +126,7 @@ require(["domReady!", "jquery", "gettext", "js/models/settings/advanced", "js/vi - % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules: + % if "split_test" in context_course.advanced_modules: % endif diff --git a/cms/templates/settings_graders.html b/cms/templates/settings_graders.html index 506bedbe69..c8687b1a8c 100644 --- a/cms/templates/settings_graders.html +++ b/cms/templates/settings_graders.html @@ -148,7 +148,7 @@ require(["domReady!", "jquery", "js/views/settings/grading", "js/models/settings - % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules: + % if "split_test" in context_course.advanced_modules: % endif diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index 1b6a6fff0d..3f42d06ace 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -84,7 +84,7 @@ - % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules: + % if "split_test" in context_course.advanced_modules: diff --git a/cms/urls.py b/cms/urls.py index ceea5f039f..c71a766c33 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -91,15 +91,11 @@ urlpatterns += patterns( url(r'^settings/advanced/{}$'.format(settings.COURSE_KEY_PATTERN), 'advanced_settings_handler'), url(r'^textbooks/{}$'.format(settings.COURSE_KEY_PATTERN), 'textbooks_list_handler'), url(r'^textbooks/{}/(?P\d[^/]*)$'.format(settings.COURSE_KEY_PATTERN), 'textbooks_detail_handler'), + url(r'^group_configurations/{}$'.format(settings.COURSE_KEY_PATTERN), 'group_configurations_list_handler'), + url(r'^group_configurations/{}/(?P\d+)/?$'.format(settings.COURSE_KEY_PATTERN), + 'group_configurations_detail_handler'), ) -if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS'): - urlpatterns += patterns('contentstore.views', - url(r'^group_configurations/{}$'.format(settings.COURSE_KEY_PATTERN), 'group_configurations_list_handler'), - url(r'^group_configurations/{}/(?P\d+)/?$'.format(settings.COURSE_KEY_PATTERN), - 'group_configurations_detail_handler'), - ) - js_info_dict = { 'domain': 'djangojs', # We need to explicitly include external Django apps that are not in LOCALE_PATHS. diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index 4bcd9241d6..c867d0dae1 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -6,6 +6,7 @@ import logging import json from webob import Response from uuid import uuid4 +from operator import itemgetter from xmodule.progress import Progress from xmodule.seq_module import SequenceDescriptor @@ -235,24 +236,41 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): Render the staff view for a split test module. """ fragment = Fragment() - contents = [] + active_contents = [] + inactive_contents = [] - for group_id in self.group_id_to_child: - child_location = self.group_id_to_child[group_id] + for child_location in self.children: # pylint: disable=no-member child_descriptor = self.get_child_descriptor_by_location(child_location) child = self.system.get_module(child_descriptor) rendered_child = child.render(STUDENT_VIEW, context) fragment.add_frag_resources(rendered_child) + group_name, updated_group_id = self.get_data_for_vertical(child) - contents.append({ - 'group_id': group_id, + if updated_group_id is None: # inactive group + group_name = child.display_name + updated_group_id = [g_id for g_id, loc in self.group_id_to_child.items() if loc == child_location][0] + inactive_contents.append({ + 'group_name': _(u'{group_name} (inactive)').format(group_name=group_name), + 'id': child.location.to_deprecated_string(), + 'content': rendered_child.content, + 'group_id': updated_group_id, + }) + continue + + active_contents.append({ + 'group_name': group_name, 'id': child.location.to_deprecated_string(), - 'content': rendered_child.content + 'content': rendered_child.content, + 'group_id': updated_group_id, }) + # Sort active and inactive contents by group name. + sorted_active_contents = sorted(active_contents, key=itemgetter('group_name')) + sorted_inactive_contents = sorted(inactive_contents, key=itemgetter('group_name')) + # Use the new template fragment.add_content(self.system.render_template('split_test_staff_view.html', { - 'items': contents, + 'items': sorted_active_contents + sorted_inactive_contents, })) fragment.add_css('.split-test-child { display: none; }') fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/split_test_staff.js')) diff --git a/common/test/acceptance/tests/test_studio_split_test.py b/common/test/acceptance/tests/test_studio_split_test.py index 20eb056288..01bfc01160 100644 --- a/common/test/acceptance/tests/test_studio_split_test.py +++ b/common/test/acceptance/tests/test_studio_split_test.py @@ -7,6 +7,7 @@ import os import math from unittest import skip, skipUnless from nose.plugins.attrib import attr +from selenium.webdriver.support.ui import Select from xmodule.partitions.partitions import Group, UserPartition from bok_choy.promise import Promise, EmptyPromise @@ -17,8 +18,9 @@ from ..pages.studio.overview import CourseOutlinePage, CourseOutlineUnit from ..pages.studio.settings_advanced import AdvancedSettingsPage from ..pages.studio.container import ContainerPage from ..pages.studio.settings_group_configurations import GroupConfigurationsPage -from ..pages.studio.utils import add_advanced_component +from ..pages.studio.utils import add_advanced_component, click_css from ..pages.xblock.utils import wait_for_xblock_initialization +from ..pages.lms.courseware import CoursewarePage from .base_studio_test import StudioCourseTest @@ -187,7 +189,6 @@ class SplitTest(ContainerBase, SplitTestMixin): @attr('shard_1') -@skipUnless(os.environ.get('FEATURE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature') class SettingsMenuTest(StudioCourseTest): """ Tests that Setting menu is rendered correctly in Studio @@ -236,7 +237,6 @@ class SettingsMenuTest(StudioCourseTest): @attr('shard_1') -@skipUnless(os.environ.get('FEATURE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature') class GroupConfigurationsTest(ContainerBase, SplitTestMixin): """ Tests that Group Configurations page works correctly with previously @@ -291,7 +291,7 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): def _add_split_test_to_vertical(self, number, group_configuration_metadata=None): """ - Add split test to vertical #`number`. + Add split test to vertical #`number`. If `group_configuration_metadata` is not None, use it to assign group configuration to split test. """ @@ -319,6 +319,8 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): """ Creates a Group Configuration containing a list of groups. Optionally creates a Content Experiment and associates it with previous Group Configuration. + + Returns group configuration or (group configuration, experiment xblock) """ # Create a new group configurations self.course_fixture._update_xblock(self.course_fixture._course_location, { @@ -332,16 +334,40 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): if associate_experiment: # Assign newly created group configuration to experiment vertical = self.course_fixture.get_nested_xblocks(category="vertical")[0] - self.course_fixture.create_xblock( - vertical.locator, - XBlockFixtureDesc('split_test', 'Test Content Experiment', metadata={'user_partition_id': 0}) - ) + split_test = XBlockFixtureDesc('split_test', 'Test Content Experiment', metadata={'user_partition_id': 0}) + self.course_fixture.create_xblock(vertical.locator, split_test) # Go to the Group Configuration Page self.page.visit() config = self.page.group_configurations[0] + + if associate_experiment: + return config, split_test return config + def publish_unit_in_LMS_and_view(self, courseware_page): + """ + Given course outline page, publish first unit and view it in LMS + """ + self.outline_page.visit() + self.outline_page.expand_all_subsections() + section = self.outline_page.section_at(0) + unit = section.subsection_at(0).unit_at(0).go_to() + + # I publish and view in LMS and it is rendered correctly + unit.publish_action.click() + unit.view_published_version() + self.assertEqual(len(self.browser.window_handles), 2) + courseware_page.wait_for_page() + + def get_select_options(self, page, selector): + """ + Get list of options of dropdown that is specified by selector on a given page. + """ + select_element = page.q(css=selector) + self.assertTrue(select_element.is_present()) + return [option.text for option in Select(select_element[0]).options] + def test_no_group_configurations_added(self): """ Scenario: Ensure that message telling me to create a new group configuration is @@ -614,12 +640,12 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): Given I have a course without group configurations And I create new group configuration with 2 default groups When I set only description and try to save - Then I see error message "Group Configuration name is required" + Then I see error message "Group Configuration name is required." When I set a name And I delete the name of one of the groups and try to save Then I see error message "All groups must have a name" When I delete the group without name and try to save - Then I see error message "Please add at least two groups" + Then I see error message "Please add at least two groups." When I add new group and try to save Then I see the group configuration is saved successfully """ @@ -638,14 +664,13 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): config = self.page.group_configurations[0] config.description = "Description of the group configuration." - try_to_save_and_verify_error_message("Group Configuration name is required") + try_to_save_and_verify_error_message("Group Configuration name is required.") # Set required field config.name = "Name of the Group Configuration" config.groups[1].name = '' - try_to_save_and_verify_error_message("All groups must have a name") + try_to_save_and_verify_error_message("All groups must have a name.") config.groups[1].remove() - try_to_save_and_verify_error_message("There must be at least two groups") config.add_group() # Save the configuration @@ -860,7 +885,7 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): """ # Create group configuration and associated experiment - config = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B")], True) + config, _ = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B")], True) # Display details view config.toggle() @@ -898,7 +923,7 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): """ # Create group configuration and associated experiment - config = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B"), Group("2", "Group C")], True) + config, _ = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B"), Group("2", "Group C")], True) # Display details view config.toggle() @@ -948,7 +973,7 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): """ # Create a group configuration with an associated experiment and display edit view - config = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B")], True) + config, _ = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B")], True) config.edit() # Check that warning icon and message are present self.assertTrue(config.edit_warning_icon_is_present) @@ -957,3 +982,60 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): "This configuration is currently used in content experiments. If you make changes to the groups, you may need to edit those experiments.", config.edit_warning_message_text ) + + def publish_unit_and_verify_groups_in_LMS(self, courseware_page, group_names): + """ + Publish first unit in LMS and verify that Courseware page has given Groups + """ + self.publish_unit_in_LMS_and_view(courseware_page) + self.assertEqual(u'split_test', courseware_page.xblock_component_type()) + self.assertTrue(courseware_page.q(css=".split-test-select").is_present()) + rendered_group_names = self.get_select_options(page=courseware_page, selector=".split-test-select") + self.assertListEqual(group_names, rendered_group_names) + + def test_split_test_LMS_staff_view(self): + """ + Scenario: Ensure that split test is correctly rendered in LMS staff mode as it is + and after inactive group removal. + + Given I have a course with group configurations and split test that assigned to first group configuration + Then I publish split test and view it in LMS in staff view + And it is rendered correctly + Then I go to group configuration and delete group + Then I publish split test and view it in LMS in staff view + And it is rendered correctly + Then I go to split test and delete inactive vertical + Then I publish unit and view unit in LMS in staff view + And it is rendered correctly + """ + + config, split_test = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B"), Group("2", "Group C")], True) + container = ContainerPage(self.browser, split_test.locator) + + # render in LMS correctly + courseware_page = CoursewarePage(self.browser, self.course_id) + self.publish_unit_and_verify_groups_in_LMS(courseware_page, [u'Group A', u'Group B', u'Group C']) + + # I go to group configuration and delete group + self.page.visit() + self.page.q(css='.group-toggle').first.click() + config.edit() + config.groups[2].remove() + config.save() + self.page.q(css='.group-toggle').first.click() + self._assert_fields(config, name="Name", description="Description", groups=["Group A", "Group B"]) + self.browser.close() + self.browser.switch_to_window(self.browser.window_handles[0]) + + # render in LMS to see how inactive vertical is rendered + self.publish_unit_and_verify_groups_in_LMS(courseware_page, [u'Group A', u'Group B', u'Group ID 2 (inactive)']) + + self.browser.close() + self.browser.switch_to_window(self.browser.window_handles[0]) + + # I go to split test and delete inactive vertical + container.visit() + container.delete(0) + + # render in LMS again + self.publish_unit_and_verify_groups_in_LMS(courseware_page, [u'Group A', u'Group B']) diff --git a/lms/templates/split_test_author_view.html b/lms/templates/split_test_author_view.html index 49477874a5..ece87663c8 100644 --- a/lms/templates/split_test_author_view.html +++ b/lms/templates/split_test_author_view.html @@ -5,7 +5,7 @@ split_test = context.get('split_test') user_partition = split_test.descriptor.get_selected_partition() messages = split_test.descriptor.validation_messages() -show_link = settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and group_configuration_url is not None +show_link = group_configuration_url is not None %> % if is_root and not is_configured: diff --git a/lms/templates/split_test_staff_view.html b/lms/templates/split_test_staff_view.html index e66a3d7452..28bbd66534 100644 --- a/lms/templates/split_test_staff_view.html +++ b/lms/templates/split_test_staff_view.html @@ -4,7 +4,7 @@