Merge pull request #4948 from edx/christina/last-group-config-pr

Unflag Group Configurations, Render only existing children in staff view...

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()
 

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)

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'

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',
 }

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);

cms/templates/settings.html

@@ -330,7 +330,7 @@ require(["domReady!", "jquery", "js/models/settings/course_details", "js/views/s
             <li class="nav-item"><a href="${grading_config_url}">${_("Grading")}</a></li>
             <li class="nav-item"><a href="${course_team_url}">${_("Course Team")}</a></li>
             <li class="nav-item"><a href="${advanced_config_url}">${_("Advanced Settings")}</a></li>
-          % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules:
+          % if "split_test" in context_course.advanced_modules:
             <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
           % endif
           </ul>

cms/templates/settings_advanced.html

@@ -126,7 +126,7 @@ require(["domReady!", "jquery", "gettext", "js/models/settings/advanced", "js/vi
             <li class="nav-item"><a href="${details_url}">${_("Details &amp; Schedule")}</a></li>
             <li class="nav-item"><a href="${grading_url}">${_("Grading")}</a></li>
             <li class="nav-item"><a href="${course_team_url}">${_("Course Team")}</a></li>
-          % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules:
+          % if "split_test" in context_course.advanced_modules:
             <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
           % endif
           </ul>

cms/templates/settings_graders.html

@@ -148,7 +148,7 @@ require(["domReady!", "jquery", "js/views/settings/grading", "js/models/settings
             <li class="nav-item"><a href="${detailed_settings_url}">${_("Details &amp; Schedule")}</a></li>
             <li class="nav-item"><a href="${course_team_url}">${_("Course Team")}</a></li>
             <li class="nav-item"><a href="${advanced_settings_url}">${_("Advanced Settings")}</a></li>
-          % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules:
+          % if "split_test" in context_course.advanced_modules:
             <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
           % endif
           </ul>

cms/templates/widgets/header.html

@@ -84,7 +84,7 @@
                   <li class="nav-item nav-course-settings-advanced">
                     <a href="${advanced_settings_url}">${_("Advanced Settings")}</a>
                   </li>
-                  % if settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and "split_test" in context_course.advanced_modules:
+                  % if "split_test" in context_course.advanced_modules:
                   <li class="nav-item nav-course-settings-group-configurations">
                     <a href="${reverse('contentstore.views.group_configurations_list_handler', kwargs={'course_key_string': unicode(course_key)})}">${_("Group Configurations")}</a>
                   </li>

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<textbook_id>\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<group_configuration_id>\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<group_configuration_id>\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.

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)
-
-            contents.append({
-                'group_id': group_id,
+            group_name, updated_group_id = self.get_data_for_vertical(child)
+
+            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'))

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'])

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:

lms/templates/split_test_staff_view.html

@@ -4,7 +4,7 @@
     <select class="split-test-select">
         % for idx, item in enumerate(items):
         ## Translators: The 'Group' here refers to the group of users that has been sorted into group_id
-        <option value="${item['group_id']}">${_("Group {group_id}").format(group_id=item['group_id'])}</option>
+        <option value="${item['group_id']}">${item['group_name']}</option>
         %endfor
     </select>