Studio support for cohorted courseware

TNL-652

cms/djangoapps/contentstore/tests/test_utils.py

@@ -423,3 +423,63 @@ class InheritedStaffLockTest(StaffLockTest):
     def test_no_inheritance_for_orphan(self):
         """Tests that an orphaned xblock does not inherit staff lock"""
         self.assertFalse(utils.ancestor_has_staff_lock(self.orphan))
+
+
+class GroupVisibilityTest(CourseTestCase):
+    """
+    Test content group access rules.
+    """
+    def setUp(self):
+        super(GroupVisibilityTest, self).setUp()
+
+        chapter = ItemFactory.create(category='chapter', parent_location=self.course.location)
+        sequential = ItemFactory.create(category='sequential', parent_location=chapter.location)
+        vertical = ItemFactory.create(category='vertical', parent_location=sequential.location)
+        html = ItemFactory.create(category='html', parent_location=vertical.location)
+        problem = ItemFactory.create(
+            category='problem', parent_location=vertical.location, data="<problem></problem>"
+        )
+        self.sequential = self.store.get_item(sequential.location)
+        self.vertical = self.store.get_item(vertical.location)
+        self.html = self.store.get_item(html.location)
+        self.problem = self.store.get_item(problem.location)
+
+    def set_group_access(self, xblock, value):
+        """ Sets group_access to specified value and calls update_item to persist the change. """
+        xblock.group_access = value
+        self.store.update_item(xblock, self.user.id)
+
+    def test_no_visibility_set(self):
+        """ Tests when group_access has not been set on anything. """
+
+        def verify_all_components_visible_to_all():  # pylint: disable=invalid-name
+            """ Verifies when group_access has not been set on anything. """
+            for item in (self.sequential, self.vertical, self.html, self.problem):
+                self.assertFalse(utils.has_children_visible_to_specific_content_groups(item))
+                self.assertFalse(utils.is_visible_to_specific_content_groups(item))
+
+        verify_all_components_visible_to_all()
+
+        # Test with group_access set to Falsey values.
+        self.set_group_access(self.vertical, {1: []})
+        self.set_group_access(self.html, {2: None})
+
+        verify_all_components_visible_to_all()
+
+    def test_sequential_and_problem_have_group_access(self):
+        """ Tests when group_access is set on a few different components. """
+        self.set_group_access(self.sequential, {1: [0]})
+        # This is a no-op.
+        self.set_group_access(self.vertical, {1: []})
+        self.set_group_access(self.problem, {2: [3, 4]})
+
+        # Note that "has_children_visible_to_specific_content_groups" only checks immediate children.
+        self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.sequential))
+        self.assertTrue(utils.has_children_visible_to_specific_content_groups(self.vertical))
+        self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.html))
+        self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.problem))
+
+        self.assertTrue(utils.is_visible_to_specific_content_groups(self.sequential))
+        self.assertFalse(utils.is_visible_to_specific_content_groups(self.vertical))
+        self.assertFalse(utils.is_visible_to_specific_content_groups(self.html))
+        self.assertTrue(utils.is_visible_to_specific_content_groups(self.problem))

cms/djangoapps/contentstore/utils.py

@@ -179,6 +179,36 @@ def is_currently_visible_to_students(xblock):
     return True
 
 
+def has_children_visible_to_specific_content_groups(xblock):
+    """
+    Returns True if this xblock has children that are limited to specific content groups.
+    Note that this method is not recursive (it does not check grandchildren).
+    """
+    if not xblock.has_children:
+        return False
+
+    for child in xblock.get_children():
+        if is_visible_to_specific_content_groups(child):
+            return True
+
+    return False
+
+
+def is_visible_to_specific_content_groups(xblock):
+    """
+    Returns True if this xblock has visibility limited to specific content groups.
+    """
+    if not xblock.group_access:
+        return False
+    for __, value in xblock.group_access.iteritems():
+        # value should be a list of group IDs. If it is an empty list or None, the xblock is visible
+        # to all groups in that particular partition. So if value is a truthy value, the xblock is
+        # restricted in some way.
+        if value:
+            return True
+    return False
+
+
 def find_release_date_source(xblock):
     """
     Finds the ancestor of xblock that set its release date.

cms/djangoapps/contentstore/views/component.py

@@ -21,7 +21,7 @@ from xblock.runtime import Mixologist
 
 from contentstore.utils import get_lms_link_for_item
 from contentstore.views.helpers import get_parent_xblock, is_unit, xblock_type_display_name
-from contentstore.views.item import create_xblock_info
+from contentstore.views.item import create_xblock_info, add_container_page_publishing_info
 
 from opaque_keys.edx.keys import UsageKey
 
@@ -186,8 +186,9 @@ def container_handler(request, usage_key_string):
             # about the block's ancestors and siblings for use by the Unit Outline.
             xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page)
 
-            # Create the link for preview.
-            preview_lms_base = settings.FEATURES.get('PREVIEW_LMS_BASE')
+            if is_unit_page:
+                add_container_page_publishing_info(xblock, xblock_info)
+
             # need to figure out where this item is in the list of children as the
             # preview will need this
             index = 1

cms/djangoapps/contentstore/views/course.py

@@ -1410,7 +1410,7 @@ def group_configurations_list_handler(request, course_key_string):
                 'context_course': course,
                 'group_configuration_url': group_configuration_url,
                 'course_outline_url': course_outline_url,
-                'configurations': configurations if should_show_group_configurations_page(course) else None,
+                'configurations': configurations,
             })
         elif "application/json" in request.META.get('HTTP_ACCEPT'):
             if request.method == 'POST':
@@ -1489,16 +1489,6 @@ def group_configurations_detail_handler(request, course_key_string, group_config
             return JsonResponse(status=204)
 
 
-def should_show_group_configurations_page(course):
-    """
-    Returns true if Studio should show the "Group Configurations" page for the specified course.
-    """
-    return (
-        SPLIT_TEST_COMPONENT_TYPE in ADVANCED_COMPONENT_TYPES and
-        SPLIT_TEST_COMPONENT_TYPE in course.advanced_modules
-    )
-
-
 def _get_course_creator_status(user):
     """
     Helper method for returning the course creator status for a particular user,

cms/djangoapps/contentstore/views/tests/test_group_configurations.py

@@ -208,17 +208,6 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations
         self.assertContains(response, 'First name')
         self.assertContains(response, 'Group C')
 
-    def test_view_index_disabled(self):
-        """
-        Check that group configuration page is not displayed when turned off.
-        """
-        if SPLIT_TEST_COMPONENT_TYPE in self.course.advanced_modules:
-            self.course.advanced_modules.remove(SPLIT_TEST_COMPONENT_TYPE)
-            self.store.update_item(self.course, self.user.id)
-
-        resp = self.client.get(self._url())
-        self.assertContains(resp, "module is disabled")
-
     def test_unsupported_http_accept_header(self):
         """
         Test if not allowed header present in request.

cms/djangoapps/contentstore/views/tests/test_item.py

@@ -18,8 +18,9 @@ from contentstore.views.component import (
     component_handler, get_component_templates
 )
 
-
-from contentstore.views.item import create_xblock_info, ALWAYS, VisibilityState, _xblock_type_and_display_name
+from contentstore.views.item import (
+    create_xblock_info, ALWAYS, VisibilityState, _xblock_type_and_display_name, add_container_page_publishing_info
+)
 from contentstore.tests.utils import CourseTestCase
 from student.tests.factories import UserFactory
 from xmodule.capa_module import CapaDescriptor
@@ -116,20 +117,9 @@ class GetItemTest(ItemTest):
         return resp
 
     @ddt.data(
-        # chapter explanation:
-        # 1-3. get course, chapter, chapter's children,
-        # 4-7. chapter's published grandchildren, chapter's draft grandchildren, published & then draft greatgrand
-        # 8 compute chapter's parent
-        # 9 get chapter's parent
-        # 10-16. run queries 2-8 again
-        # 17-19. compute seq, vert, and problem's parents (odd since it's going down; so, it knows)
-        # 20-22. get course 3 times
-        # 23. get chapter
-        # 24. compute chapter's parent (course)
-        # 25. compute course's parent (None)
-        (1, 20, 20, 26, 26),
-        (2, 21, 21, 29, 28),
-        (3, 22, 22, 32, 30),
+        (1, 16, 14, 15, 11),
+        (2, 16, 14, 15, 11),
+        (3, 16, 14, 15, 11),
     )
     @ddt.unpack
     def test_get_query_count(self, branching_factor, chapter_queries, section_queries, unit_queries, problem_queries):
@@ -144,6 +134,17 @@ class GetItemTest(ItemTest):
         with check_mongo_calls(problem_queries):
             self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['problem'][-1]))
 
+    @ddt.data(
+        (1, 26),
+        (2, 28),
+        (3, 30),
+    )
+    @ddt.unpack
+    def test_container_get_query_count(self, branching_factor, unit_queries,):
+        self.populate_course(branching_factor)
+        with check_mongo_calls(unit_queries):
+            self.client.get(reverse_usage_url('xblock_container_handler', self.populated_usage_keys['vertical'][-1]))
+
     def test_get_vertical(self):
         # Add a vertical
         resp = self.create_xblock(category='vertical')
@@ -1403,6 +1404,7 @@ class TestXBlockInfo(ItemTest):
             include_children_predicate=ALWAYS,
             include_ancestor_info=True
         )
+        add_container_page_publishing_info(vertical, xblock_info)
         self.validate_vertical_xblock_info(xblock_info)
 
     def test_component_xblock_info(self):
@@ -1523,10 +1525,6 @@ class TestXBlockInfo(ItemTest):
                     )
         else:
             self.assertIsNone(xblock_info.get('child_info', None))
-        if xblock_info['category'] == 'vertical' and not course_outline:
-            self.assertEqual(xblock_info['edited_by'], 'testuser')
-        else:
-            self.assertIsNone(xblock_info.get('edited_by', None))
 
 
 class TestLibraryXBlockInfo(ModuleStoreTestCase):
@@ -1631,7 +1629,8 @@ class TestXBlockPublishingInfo(ItemTest):
         )
         if staff_only:
             self._enable_staff_only(child.location)
-        return child
+        # In case the staff_only state was set, return the updated xblock.
+        return modulestore().get_item(child.location)
 
     def _get_child_xblock_info(self, xblock_info, index):
         """
@@ -1720,12 +1719,6 @@ class TestXBlockPublishingInfo(ItemTest):
         """
         self._verify_xblock_info_state(xblock_info, 'has_explicit_staff_lock', expected_state, path, should_equal)
 
-    def _verify_staff_lock_from_state(self, xblock_info, expected_state, path=None, should_equal=True):
-        """
-        Verify the staff_lock_from state of an item in the xblock_info.
-        """
-        self._verify_xblock_info_state(xblock_info, 'staff_lock_from', expected_state, path, should_equal)
-
     def test_empty_chapter(self):
         empty_chapter = self._create_child(self.course, 'chapter', "Empty Chapter")
         xblock_info = self._get_xblock_info(empty_chapter.location)
@@ -1815,7 +1808,7 @@ class TestXBlockPublishingInfo(ItemTest):
         """
         chapter = self._create_child(self.course, 'chapter', "Test Chapter", staff_only=True)
         sequential = self._create_child(chapter, 'sequential', "Test Sequential")
-        self._create_child(sequential, 'vertical', "Unit")
+        vertical = self._create_child(sequential, 'vertical', "Unit")
         xblock_info = self._get_xblock_info(chapter.location)
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only)
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.FIRST_SUBSECTION_PATH)
@@ -1825,7 +1818,9 @@ class TestXBlockPublishingInfo(ItemTest):
         self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_SUBSECTION_PATH)
         self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_UNIT_PATH)
 
-        self._verify_staff_lock_from_state(xblock_info, _xblock_type_and_display_name(chapter), path=self.FIRST_UNIT_PATH)
+        vertical_info = self._get_xblock_info(vertical.location)
+        add_container_page_publishing_info(vertical, vertical_info)
+        self.assertEqual(_xblock_type_and_display_name(chapter), vertical_info["staff_lock_from"])
 
     def test_no_staff_only_section(self):
         """
@@ -1846,7 +1841,7 @@ class TestXBlockPublishingInfo(ItemTest):
         """
         chapter = self._create_child(self.course, 'chapter', "Test Chapter")
         sequential = self._create_child(chapter, 'sequential', "Test Sequential", staff_only=True)
-        self._create_child(sequential, 'vertical', "Unit")
+        vertical = self._create_child(sequential, 'vertical', "Unit")
         xblock_info = self._get_xblock_info(chapter.location)
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only)
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.FIRST_SUBSECTION_PATH)
@@ -1856,7 +1851,9 @@ class TestXBlockPublishingInfo(ItemTest):
         self._verify_explicit_staff_lock_state(xblock_info, True, path=self.FIRST_SUBSECTION_PATH)
         self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_UNIT_PATH)
 
-        self._verify_staff_lock_from_state(xblock_info, _xblock_type_and_display_name(sequential), path=self.FIRST_UNIT_PATH)
+        vertical_info = self._get_xblock_info(vertical.location)
+        add_container_page_publishing_info(vertical, vertical_info)
+        self.assertEqual(_xblock_type_and_display_name(sequential), vertical_info["staff_lock_from"])
 
     def test_no_staff_only_subsection(self):
         """
@@ -1874,7 +1871,7 @@ class TestXBlockPublishingInfo(ItemTest):
     def test_staff_only_unit(self):
         chapter = self._create_child(self.course, 'chapter', "Test Chapter")
         sequential = self._create_child(chapter, 'sequential', "Test Sequential")
-        unit = self._create_child(sequential, 'vertical', "Unit", staff_only=True)
+        vertical = self._create_child(sequential, 'vertical', "Unit", staff_only=True)
         xblock_info = self._get_xblock_info(chapter.location)
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only)
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.FIRST_SUBSECTION_PATH)
@@ -1884,7 +1881,9 @@ class TestXBlockPublishingInfo(ItemTest):
         self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_SUBSECTION_PATH)
         self._verify_explicit_staff_lock_state(xblock_info, True, path=self.FIRST_UNIT_PATH)
 
-        self._verify_staff_lock_from_state(xblock_info, _xblock_type_and_display_name(unit), path=self.FIRST_UNIT_PATH)
+        vertical_info = self._get_xblock_info(vertical.location)
+        add_container_page_publishing_info(vertical, vertical_info)
+        self.assertEqual(_xblock_type_and_display_name(vertical), vertical_info["staff_lock_from"])
 
     def test_unscheduled_section_with_live_subsection(self):
         chapter = self._create_child(self.course, 'chapter', "Test Chapter")

cms/envs/common.py

@@ -37,6 +37,7 @@ from path import path
 from warnings import simplefilter
 
 from lms.djangoapps.lms_xblock.mixin import LmsBlockMixin
+from cms.lib.xblock.authoring_mixin import AuthoringMixin
 import dealer.git
 from xmodule.modulestore.edit_info import EditInfoMixin
 
@@ -269,7 +270,13 @@ from xmodule.x_module import XModuleMixin
 
 # This should be moved into an XBlock Runtime/Application object
 # once the responsibility of XBlock creation is moved out of modulestore - cpennington
-XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin, XModuleMixin, EditInfoMixin)
+XBLOCK_MIXINS = (
+    LmsBlockMixin,
+    InheritanceMixin,
+    XModuleMixin,
+    EditInfoMixin,
+    AuthoringMixin,
+)
 
 # Allow any XBlock in Studio
 # You should also enable the ALLOW_ALL_ADVANCED_COMPONENTS feature flag, so that

cms/lib/xblock/authoring_mixin.py

+"""
+Mixin class that provides authoring capabilities for XBlocks.
+"""
+
+import logging
+
+from xblock.core import XBlock
+from xblock.fields import XBlockMixin
+from xblock.fragment import Fragment
+
+log = logging.getLogger(__name__)
+
+VISIBILITY_VIEW = 'visibility_view'
+
+
+@XBlock.needs("i18n")
+class AuthoringMixin(XBlockMixin):
+    """
+    Mixin class that provides authoring capabilities for XBlocks.
+    """
+    _services_requested = {
+        'i18n': 'need',
+    }
+
+    def _get_studio_resource_url(self, relative_url):
+        """
+        Returns the Studio URL to a static resource.
+        """
+        # TODO: is there a cleaner way to do this?
+        from cms.envs.common import STATIC_URL
+        return STATIC_URL + relative_url
+
+    def visibility_view(self, _context=None):
+        """
+        Render the view to manage an xblock's visibility settings in Studio.
+        Args:
+            _context: Not actively used for this view.
+        Returns:
+            (Fragment): An HTML fragment for editing the visibility of this XBlock.
+        """
+        fragment = Fragment()
+        from contentstore.utils import reverse_course_url
+        fragment.add_content(self.system.render_template('visibility_editor.html', {
+            'xblock': self,
+            'manage_groups_url': reverse_course_url('group_configurations_list_handler', self.location.course_key),
+        }))
+        fragment.add_javascript_url(self._get_studio_resource_url('/js/xblock/authoring.js'))
+        fragment.initialize_js('VisibilityEditorInit')
+        return fragment

cms/lib/xblock/test/test_authoring_mixin.py

+from openedx.core.djangoapps.course_groups.partition_scheme import CohortPartitionScheme
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
+from xmodule.partitions.partitions import Group, UserPartition
+
+
+class AuthoringMixinTestCase(ModuleStoreTestCase):
+    """
+    Tests the studio authoring XBlock mixin.
+    """
+    def setUp(self):
+        """
+        Create a simple course with a video component.
+        """
+        super(AuthoringMixinTestCase, self).setUp()
+        self.course = CourseFactory.create()
+        chapter = ItemFactory.create(
+            category='chapter',
+            parent_location=self.course.location,
+            display_name='Test Chapter'
+        )
+        sequential = ItemFactory.create(
+            category='sequential',
+            parent_location=chapter.location,
+            display_name='Test Sequential'
+        )
+        self.vertical = ItemFactory.create(
+            category='vertical',
+            parent_location=sequential.location,
+            display_name='Test Vertical'
+        )
+        self.video = ItemFactory.create(
+            category='video',
+            parent_location=self.vertical.location,
+            display_name='Test Vertical'
+        )
+        self.pet_groups = [Group(1, 'Cat Lovers'), Group(2, 'Dog Lovers')]
+
+    def create_cohorted_content_groups(self, groups):
+        """
+        Create a cohorted content partition with specified groups.
+        """
+        self.content_partition = UserPartition(
+            1,
+            'Content Groups',
+            'Contains Groups for Cohorted Courseware',
+            groups,
+            scheme_id='cohort'
+        )
+        self.course.user_partitions = [self.content_partition]
+        self.store.update_item(self.course, self.user.id)
+
+    def set_staff_only(self, item):
+        """Make an item visible to staff only."""
+        item.visible_to_staff_only = True
+        self.store.update_item(item, self.user.id)
+
+    def set_group_access(self, item, group_ids):
+        """
+        Set group_access for the specified item to the specified group
+        ids within the content partition.
+        """
+        item.group_access[self.content_partition.id] = group_ids
+        self.store.update_item(item, self.user.id)
+
+    def verify_visibility_view_contains(self, item, substrings):
+        """
+        Verify that an item's visibility view returns an html string
+        containing all the expected substrings.
+        """
+        html = item.visibility_view().body_html()
+        for string in substrings:
+            self.assertIn(string, html)
+
+    def test_html_no_partition(self):
+        self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.')
+
+    def test_html_empty_partition(self):
+        self.create_cohorted_content_groups([])
+        self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.')
+
+    def test_html_populated_partition(self):
+        self.create_cohorted_content_groups(self.pet_groups)
+        self.verify_visibility_view_contains(self.video, ['Cat Lovers', 'Dog Lovers'])
+
+    def test_html_no_partition_staff_locked(self):
+        self.set_staff_only(self.vertical)
+        self.verify_visibility_view_contains(self.video, ['You have not set up any groups to manage visibility with.'])
+
+    def test_html_empty_partition_staff_locked(self):
+        self.create_cohorted_content_groups([])
+        self.set_staff_only(self.vertical)
+        self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.')
+
+    def test_html_populated_partition_staff_locked(self):
+        self.create_cohorted_content_groups(self.pet_groups)
+        self.set_staff_only(self.vertical)
+        self.verify_visibility_view_contains(
+            self.video, ['The Unit this component is contained in is hidden from students.', 'Cat Lovers', 'Dog Lovers']
+        )
+
+    def test_html_false_content_group(self):
+        self.create_cohorted_content_groups(self.pet_groups)
+        self.set_group_access(self.video, ['false_group_id'])
+        self.verify_visibility_view_contains(
+            self.video, ['Cat Lovers', 'Dog Lovers', 'Content group no longer exists.']
+        )
+
+    def test_html_false_content_group_staff_locked(self):
+        self.create_cohorted_content_groups(self.pet_groups)
+        self.set_staff_only(self.vertical)
+        self.set_group_access(self.video, ['false_group_id'])
+        self.verify_visibility_view_contains(
+            self.video,
+            [
+                'Cat Lovers',
+                'Dog Lovers',
+                'The Unit this component is contained in is hidden from students.',
+                'Content group no longer exists.'
+            ]
+        )

cms/static/coffee/src/views/module_edit.coffee

@@ -52,7 +52,5 @@ define ["jquery", "underscore", "gettext", "xblock/runtime.v1",
 
     clickEditButton: (event) ->
       event.preventDefault()
-      modal = new EditXBlockModal({
-        view: 'student_view'
-      });
+      modal = new EditXBlockModal();
       modal.edit(this.$el, self.model, { refresh: _.bind(@render, this) })

cms/static/js/factories/container.js

 define([
-    'jquery', 'underscore', 'js/models/xblock_info', 'js/views/pages/container',
+    'jquery', 'underscore', 'js/models/xblock_container_info', 'js/views/pages/container',
     'js/collections/component_template', 'xmodule', 'coffee/src/main',
     'xblock/cms.runtime.v1'
 ],
-function($, _, XBlockInfo, ContainerPage, ComponentTemplates, xmoduleLoader) {
+function($, _, XBlockContainerInfo, ContainerPage, ComponentTemplates, xmoduleLoader) {
     'use strict';
     return function (componentTemplates, XBlockInfoJson, action, options) {
         var main_options = {
             el: $('#content'),
-            model: new XBlockInfo(XBlockInfoJson, {parse: true}),
+            model: new XBlockContainerInfo(XBlockInfoJson, {parse: true}),
             action: action,
             templates: new ComponentTemplates(componentTemplates, {parse: true})
         };

cms/static/js/models/custom_sync_xblock_info.js

+define(["js/models/xblock_info"],
+    function(XBlockInfo) {
+        var CustomSyncXBlockInfo = XBlockInfo.extend({
+            sync: function(method, model, options) {
+                options.url = (this.urlRoots[method] || this.urlRoot) + '/' + this.get('id');
+                return XBlockInfo.prototype.sync.call(this, method, model, options);
+            }
+        });
+        return CustomSyncXBlockInfo;
+    });

cms/static/js/models/xblock_container_info.js

+define(["js/models/custom_sync_xblock_info"],
+    function(CustomSyncXBlockInfo) {
+        var XBlockContainerInfo = CustomSyncXBlockInfo.extend({
+            urlRoots: {
+                'read': '/xblock/container'
+            }
+        });
+        return XBlockContainerInfo;
+    });

cms/static/js/models/xblock_info.js

@@ -32,7 +32,8 @@ function(Backbone, _, str, ModuleUtils) {
              */
             'edited_on':null,
             /**
-             * User who last edited the xblock or any of its descendants.
+             * User who last edited the xblock or any of its descendants. Will only be present if
+             * publishing info was explicitly requested.
              */
             'edited_by':null,
             /**
@@ -44,7 +45,8 @@ function(Backbone, _, str, ModuleUtils) {
              */
             'published_on': null,
             /**
-             * User who last published the xblock, or null if never published.
+             * User who last published the xblock, or null if never published. Will only be present if
+             * publishing info was explicitly requested.
              */
             'published_by': null,
             /**
@@ -70,12 +72,14 @@ function(Backbone, _, str, ModuleUtils) {
             /**
              * The xblock which is determining the release date. For instance, for a unit,
              * this will either be the parent subsection or the grandparent section.
-             * This can be null if the release date is unscheduled.
+             * This can be null if the release date is unscheduled. Will only be present if
+             * publishing info was explicitly requested.
              */
             'release_date_from':null,
             /**
              * True if this xblock is currently visible to students. This is computed server-side
-             * so that the logic isn't duplicated on the client.
+             * so that the logic isn't duplicated on the client. Will only be present if
+             * publishing info was explicitly requested.
              */
             'currently_visible_to_students': null,
             /**
@@ -114,13 +118,20 @@ function(Backbone, _, str, ModuleUtils) {
             /**
              * The xblock which is determining the staff lock value. For instance, for a unit,
              * this will either be the parent subsection or the grandparent section.
-             * This can be null if the xblock has no inherited staff lock.
+             * This can be null if the xblock has no inherited staff lock. Will only be present if
+             * publishing info was explicitly requested.
              */
             'staff_lock_from': null,
             /**
              * True iff this xblock should display a "Contains staff only content" message.
              */
-            'staff_only_message': null
+            'staff_only_message': null,
+            /**
+             * True iff this xblock is a unit, and it has children that are only visible to certain
+             * content groups. Note that this is not a recursive property. Will only be present if
+             * publishing info was explicitly requested.
+             */
+            'has_content_group_components': null
         },
 
         initialize: function () {

cms/static/js/models/xblock_outline_info.js

-define(["js/models/xblock_info"],
-    function(XBlockInfo) {
-        var XBlockOutlineInfo = XBlockInfo.extend({
+define(["js/models/custom_sync_xblock_info"],
+    function(CustomSyncXBlockInfo) {
+        var XBlockOutlineInfo = CustomSyncXBlockInfo.extend({
 
             urlRoots: {
                 'read': '/xblock/outline'
@@ -8,15 +8,6 @@ define(["js/models/xblock_info"],
 
             createChild: function(response) {
                 return new XBlockOutlineInfo(response, { parse: true });
-            },
-
-            sync: function(method, model, options) {
-                var urlRoot = this.urlRoots[method];
-                if (!urlRoot) {
-                    urlRoot = this.urlRoot;
-                }
-                options.url = urlRoot + '/' + this.get('id');
-                return XBlockInfo.prototype.sync.call(this, method, model, options);
             }
         });
         return XBlockOutlineInfo;

cms/static/js/spec/views/pages/container_spec.js

 define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_helpers",
         "js/common_helpers/template_helpers", "js/spec_helpers/edit_helpers",
-        "js/views/pages/container", "js/views/pages/paged_container", "js/models/xblock_info"],
+        "js/views/pages/container", "js/views/pages/paged_container", "js/models/xblock_info", "jquery.simulate"],
     function ($, _, str, AjaxHelpers, TemplateHelpers, EditHelpers, ContainerPage, PagedContainerPage, XBlockInfo) {
 
         function parameterized_suite(label, global_page_options, fixtures) {
@@ -14,6 +14,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                     mockBadXBlockContainerXBlockHtml = readFixtures('mock/mock-bad-xblock-container-xblock.underscore'),
                     mockUpdatedContainerXBlockHtml = readFixtures('mock/mock-updated-container-xblock.underscore'),
                     mockXBlockEditorHtml = readFixtures('mock/mock-xblock-editor.underscore'),
+                    mockXBlockVisibilityEditorHtml = readFixtures('mock/mock-xblock-visibility-editor.underscore'),
                     PageClass = fixtures.page;
 
                 beforeEach(function () {
@@ -219,6 +220,21 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                         });
                         expect(EditHelpers.isShowingModal()).toBeTruthy();
                     });
+
+                    it('can show a visibility modal for a child xblock', function() {
+                        var visibilityButtons;
+                        renderContainerPage(this, mockContainerXBlockHtml);
+                        visibilityButtons = containerPage.$('.wrapper-xblock .visibility-button');
+                        expect(visibilityButtons.length).toBe(6);
+                        visibilityButtons[0].click();
+                        expect(str.startsWith(lastRequest().url, '/xblock/locator-component-A1/visibility_view'))
+                            .toBeTruthy();
+                        AjaxHelpers.respondWithJson(requests, {
+                            html: mockXBlockVisibilityEditorHtml,
+                            resources: []
+                        });
+                        expect(EditHelpers.isShowingModal()).toBeTruthy();
+                    });
                 });
 
                 describe("Editing an xmodule", function () {

cms/static/js/spec/views/pages/container_subviews_spec.js

@@ -80,7 +80,8 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
 
             describe("PreviewActionController", function () {
                 var viewPublishedCss = '.button-view',
-                    previewCss = '.button-preview';
+                    previewCss = '.button-preview',
+                    visibilityNoteCss = '.note-visibility';
 
                 it('renders correctly for unscheduled unit', function () {
                     renderContainerPage(this, mockContainerXBlockHtml);
@@ -109,6 +110,18 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel
                     fetch({published: false, has_changes: false});
                     expect(containerPage.$(previewCss)).not.toHaveClass(disabledCss);
                 });
+
+                it('updates when has_content_group_components attribute changes', function () {
+                    renderContainerPage(this, mockContainerXBlockHtml);
+                    fetch({has_content_group_components: false});
+                    expect(containerPage.$(visibilityNoteCss).length).toBe(0);
+
+                    fetch({has_content_group_components: true});
+                    expect(containerPage.$(visibilityNoteCss).length).toBe(1);
+
+                    fetch({has_content_group_components: false});
+                    expect(containerPage.$(visibilityNoteCss).length).toBe(0);
+                });
             });
 
             describe("Publisher", function () {

cms/static/js/spec/views/xblock_editor_spec.js

@@ -85,7 +85,7 @@ define([ "jquery", "underscore", "js/common_helpers/ajax_helpers", "js/spec_help
                     });
                     // Give the mock xblock a save method...
                     editor.xblock.save = window.MockDescriptor.save;
-                    editor.model.save(editor.getXModuleData());
+                    editor.model.save(editor.getXBlockFieldData());
                     request = requests[requests.length - 1];
                     response = JSON.parse(request.requestBody);
                     expect(response.metadata.display_name).toBe(testDisplayName);

cms/static/js/views/metadata.js

@@ -49,7 +49,7 @@ function(BaseView, _, MetadataModel, AbstractEditor, FileUpload, UploadDialog, V
         },
 
         /**
-         * Returns the just the modified metadata values, in the format used to persist to the server.
+         * Returns just the modified metadata values, in the format used to persist to the server.
          */
         getModifiedMetadataValues: function () {
             var modified_values = {};

cms/static/js/views/modals/base_modal.js

 /**
  * This is a base modal implementation that provides common utilities.
+ *
+ * A modal implementation should override the following methods:
+ *
+ *   getTitle():
+ *     returns the title for the modal.
+ *   getHTMLContent():
+ *     returns the HTML content to be shown inside the modal.
+ *
+ * A modal implementation should also provide the following options:
+ *
+ *   modalName: A string identifying the modal.
+ *   modalType: A string identifying the type of the modal.
+ *   modalSize: A string, either 'sm', 'med', or 'lg' indicating the
+ *     size of the modal.
+ *   viewSpecificClasses: A string of CSS classes to be attached to
+ *     the modal window.
+ *   addSaveButton: A boolean indicating whether to include a save
+ *     button on the modal.
  */
 define(["jquery", "underscore", "gettext", "js/views/baseview"],
     function($, _, gettext, BaseView) {
@@ -41,7 +59,7 @@ define(["jquery", "underscore", "gettext", "js/views/baseview"],
                     name: this.options.modalName,
                     type: this.options.modalType,
                     size: this.options.modalSize,
-                    title: this.options.title,
+                    title: this.getTitle(),
                     viewSpecificClasses: this.options.viewSpecificClasses
                 }));
                 this.addActionButtons();
@@ -49,6 +67,10 @@ define(["jquery", "underscore", "gettext", "js/views/baseview"],
                 this.parentElement.append(this.$el);
             },
 
+            getTitle: function() {
+                return this.options.title;
+            },
+
             renderContents: function() {
                 var contentHtml = this.getContentHtml();
                 this.$('.modal-content').html(contentHtml);

cms/static/js/views/modals/edit_xblock.js

@@ -6,6 +6,8 @@
 define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/views/utils/view_utils",
     "js/models/xblock_info", "js/views/xblock_editor"],
     function($, _, gettext, BaseModal, ViewUtils, XBlockInfo, XBlockEditorView) {
+        "strict mode";
+
         var EditXBlockModal = BaseModal.extend({
             events : {
                 "click .action-save": "save",
@@ -15,7 +17,10 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie
             options: $.extend({}, BaseModal.prototype.options, {
                 modalName: 'edit-xblock',
                 addSaveButton: true,
-                viewSpecificClasses: 'modal-editor confirm'
+                view: 'studio_view',
+                viewSpecificClasses: 'modal-editor confirm',
+                // Translators: "title" is the name of the current component being edited.
+                titleFormat: gettext("Editing: %(title)s")
             }),
 
             initialize: function() {
@@ -56,7 +61,8 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie
             displayXBlock: function() {
                 this.editorView = new XBlockEditorView({
                     el: this.$('.xblock-editor'),
-                    model: this.xblockInfo
+                    model: this.xblockInfo,
+                    view: this.options.view
                 });
                 this.editorView.render({
                     success: _.bind(this.onDisplayXBlock, this)
@@ -66,7 +72,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie
             onDisplayXBlock: function() {
                 var editorView = this.editorView,
                     title = this.getTitle(),
-                    readOnlyView = (this.editOptions && this.editOptions.readOnlyView) || !editorView.xblock.save;
+                    readOnlyView = (this.editOptions && this.editOptions.readOnlyView) || !this.canSave();
 
                 // Notify the runtime that the modal has been shown
                 editorView.notifyRuntime('modal-shown', this);
@@ -99,6 +105,10 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie
                 this.resize();
             },
 
+            canSave: function() {
+                return this.editorView.xblock.save || this.editorView.xblock.collectFieldData;
+            },
+
             disableSave: function() {
                 var saveButton = this.getActionButton('save'),
                     cancelButton = this.getActionButton('cancel');
@@ -112,7 +122,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie
                 if (!displayName) {
                     displayName = gettext('Component');
                 }
-                return interpolate(gettext("Editing: %(title)s"), { title: displayName }, true);
+                return interpolate(this.options.titleFormat, { title: displayName }, true);
             },
 
             addDefaultModes: function() {
@@ -147,7 +157,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie
                 var self = this,
                     editorView = this.editorView,
                     xblockInfo = this.xblockInfo,
-                    data = editorView.getXModuleData();
+                    data = editorView.getXBlockFieldData();
                 event.preventDefault();
                 if (data) {
                     ViewUtils.runOperationShowingMessage(gettext('Saving'),

cms/static/js/views/pages/container.js

@@ -15,6 +15,7 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
 
             events: {
                 "click .edit-button": "editXBlock",
+                "click .visibility-button": "editVisibilitySettings",
                 "click .duplicate-button": "duplicateXBlock",
                 "click .delete-button": "deleteXBlock",
                 "click .new-component-button": "scrollToNewComponentButtons"
@@ -161,10 +162,10 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
                 }
             },
 
-            editXBlock: function(event) {
+            editXBlock: function(event, options) {
                 var xblockElement = this.findXBlockElement(event.target),
                     self = this,
-                    modal = new EditXBlockModal({ });
+                    modal = new EditXBlockModal(options);
                 event.preventDefault();
 
                 modal.edit(xblockElement, this.model, {
@@ -175,6 +176,16 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
                 });
             },
 
+            editVisibilitySettings: function(event) {
+                this.editXBlock(event, {
+                    view: 'visibility_view',
+                    // Translators: "title" is the name of the current component being edited.
+                    titleFormat: gettext("Editing visibility for: %(title)s"),
+                    viewSpecificClasses: '',
+                    modalSize: 'med'
+                });
+            },
+
             duplicateXBlock: function(event) {
                 event.preventDefault();
                 this.duplicateComponent(this.findXBlockElement(event.target));

cms/static/js/views/pages/container_subviews.js

@@ -100,7 +100,8 @@ define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/utils/
 
             onSync: function(model) {
                 if (ViewUtils.hasChangedAttributes(model, [
-                    'has_changes', 'published', 'edited_on', 'edited_by', 'visibility_state', 'has_explicit_staff_lock'
+                    'has_changes', 'published', 'edited_on', 'edited_by', 'visibility_state',
+                    'has_explicit_staff_lock', 'has_content_group_components'
                 ])) {
                    this.render();
                 }
@@ -120,7 +121,8 @@ define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/utils/
                     releaseDate: this.model.get('release_date'),
                     releaseDateFrom: this.model.get('release_date_from'),
                     hasExplicitStaffLock: this.model.get('has_explicit_staff_lock'),
-                    staffLockFrom: this.model.get('staff_lock_from')
+                    staffLockFrom: this.model.get('staff_lock_from'),
+                    hasContentGroupComponents: this.model.get('has_content_group_components')
                 }));
 
                 return this;

cms/static/js/views/pages/course_outline.js

@@ -26,7 +26,7 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views
                 });
                 this.model.on('change', this.setCollapseExpandVisibility, this);
                 $('.dismiss-button').bind('click', ViewUtils.deleteNotificationHandler(function () {
-                    $('.wrapper-alert-announcement').removeClass('is-shown').addClass('is-hidden')
+                    $('.wrapper-alert-announcement').removeClass('is-shown').addClass('is-hidden');
                 }));
             },
 

cms/static/js/views/xblock_editor.js

@@ -89,17 +89,23 @@ define(["jquery", "underscore", "gettext", "js/views/xblock", "js/views/metadata
             },
 
             /**
-             * Returns the data saved for the xmodule. Note that this *does not* work for XBlocks.
+             * Returns the updated field data for the xblock. Note that this works for all
+             * XModules as well as for XBlocks that provide a 'collectFieldData' API.
              */
-            getXModuleData: function() {
+            getXBlockFieldData: function() {
                 var xblock = this.xblock,
                     metadataEditor = this.getMetadataEditor(),
                     data = null;
-                if (xblock.save) {
+                // If the xblock supports returning its field data then collect it
+                if (xblock.collectFieldData) {
+                    data = xblock.collectFieldData();
+                // ... else if this is an XModule then call its save method
+                } else if (xblock.save) {
                     data = xblock.save();
                     if (metadataEditor) {
                         data.metadata = _.extend(data.metadata || {}, this.getChangedMetadata());
                     }
+                // ... else log an error
                 } else {
                     console.error('Cannot save xblock as it has no save method');
                 }

cms/static/js/xblock/authoring.js

+/**
+ * Client-side logic to support XBlock authoring.
+ */
+(function($) {
+    'use strict';
+
+    function VisibilityEditorView(runtime, element) {
+        this.getGroupAccess = function() {
+            var groupAccess, userPartitionId, selectedGroupIds;
+            if (element.find('.visibility-level-all').prop('checked')) {
+                return {};
+            }
+            userPartitionId = element.find('.wrapper-visibility-specific').data('user-partition-id').toString();
+            selectedGroupIds = [];
+            element.find('.field-visibility-content-group input:checked').each(function(index, input) {
+                selectedGroupIds.push(parseInt($(input).val()));
+            });
+            groupAccess = {};
+            groupAccess[userPartitionId] = selectedGroupIds;
+            return groupAccess;
+        };
+
+        element.find('.field-visibility-level input').change(function(event) {
+            if ($(event.target).hasClass('visibility-level-all')) {
+                element.find('.field-visibility-content-group input').prop('checked', false);
+            }
+        });
+        element.find('.field-visibility-content-group input').change(function(event) {
+            element.find('.visibility-level-all').prop('checked', false);
+            element.find('.visibility-level-specific').prop('checked', true);
+        });
+    }
+
+    VisibilityEditorView.prototype.collectFieldData = function collectFieldData() {
+        return {
+            metadata: {
+                "group_access": this.getGroupAccess()
+            }
+        };
+    };
+
+    function initializeVisibilityEditor(runtime, element) {
+        return new VisibilityEditorView(runtime, element);
+    }
+
+    // XBlock initialization functions must be global
+    window.VisibilityEditorInit = initializeVisibilityEditor;
+})($);

cms/static/sass/elements/_xblocks.scss

@@ -189,8 +189,8 @@
       }
     }
 
-    // CASE: xblock has specific visibility set
-    &.has-visibility-set {
+    // CASE: xblock has specific visibility based on content groups set
+    &.has-group-visibility-set {
 
       .action-visibility .visibility-button.visibility-button { // needed to cascade in front of overscoped header-actions CSS rule
         color: $color-visibility-set;

cms/templates/container.html

@@ -144,8 +144,3 @@ from django.utils.translation import ugettext as _
     </div>
 </div>
 </%block>
-
-<!-- NOTE: remove this HTML if you want to not see the fake visibility modal -->
-<%block name="modal_placeholder">
-    <%include file="ux/reference/modal_access-component.html" />
-</%block>

cms/templates/group_configurations.html

@@ -47,17 +47,9 @@
   <div class="wrapper-content wrapper">
     <section class="content">
       <article class="content-primary" role="main">
-      % if configurations is None:
-        <div class="notice notice-incontext notice-moduledisabled">
-            <p class="copy">
-                ${_("This module is disabled at the moment.")}
-            </p>
-        </div>
-      % else:
         <div class="ui-loading">
             <p><span class="spin"><i class="icon fa fa-refresh"></i></span> <span class="copy">${_("Loading")}</span></p>
         </div>
-      % endif
       </article>
       <aside class="content-supplementary" role="complementary">
       <div class="bit">

cms/templates/js/mock/mock-container-xblock.underscore

@@ -46,6 +46,9 @@
                                                         <li class="action-item action-edit">
                                                             <a href="#" class="edit-button action-button"></a>
                                                         </li>
+                                                        <li class="action-item action-visibility">
+                                                            <a href="#" class="visibility-button action-button"></a>
+                                                        </li>
                                                         <li class="action-item action-duplicate">
                                                             <a href="#" class="duplicate-button action-button"></a>
                                                         </li>
@@ -71,6 +74,9 @@
                                                         <li class="action-item action-edit">
                                                             <a href="#" class="edit-button action-button"></a>
                                                         </li>
+                                                        <li class="action-item action-visibility">
+                                                            <a href="#" class="visibility-button action-button"></a>
+                                                        </li>
                                                         <li class="action-item action-duplicate">
                                                             <a href="#" class="duplicate-button action-button"></a>
                                                         </li>
@@ -96,6 +102,9 @@
                                                         <li class="action-item action-edit">
                                                             <a href="#" class="edit-button action-button"></a>
                                                         </li>
+                                                        <li class="action-item action-visibility">
+                                                            <a href="#" class="visibility-button action-button"></a>
+                                                        </li>
                                                         <li class="action-item action-duplicate">
                                                             <a href="#" class="duplicate-button action-button"></a>
                                                         </li>
@@ -151,6 +160,9 @@
                                                         <li class="action-item action-edit">
                                                             <a href="#" class="edit-button action-button"></a>
                                                         </li>
+                                                        <li class="action-item action-visibility">
+                                                            <a href="#" class="visibility-button action-button"></a>
+                                                        </li>
                                                         <li class="action-item action-duplicate">
                                                             <a href="#" class="duplicate-button action-button"></a>
                                                         </li>
@@ -176,6 +188,9 @@
                                                         <li class="action-item action-edit">
                                                             <a href="#" class="edit-button action-button"></a>
                                                         </li>
+                                                        <li class="action-item action-visibility">
+                                                            <a href="#" class="visibility-button action-button"></a>
+                                                        </li>
                                                         <li class="action-item action-duplicate">
                                                             <a href="#" class="duplicate-button action-button"></a>
                                                         </li>
@@ -201,6 +216,9 @@
                                                         <li class="action-item action-edit">
                                                             <a href="#" class="edit-button action-button"></a>
                                                         </li>
+                                                        <li class="action-item action-visibility">
+                                                            <a href="#" class="visibility-button action-button"></a>
+                                                        </li>
                                                         <li class="action-item action-duplicate">
                                                             <a href="#" class="duplicate-button action-button"></a>
                                                         </li>

cms/templates/js/mock/mock-xblock-visibility-editor.underscore

+<div class="xblock xblock-visibility_view" data-runtime-version="1" data-usage-id="i4x:;_;_edX;_mock"
+     data-init="MockXBlock" data-runtime-class="StudioRuntime" tabindex="0">
+</div>

cms/templates/js/publish-xblock.underscore

@@ -77,13 +77,12 @@ var visibleToStaffOnly = visibilityState === 'staff_only';
         <% } else { %>
             <p class="visbility-copy copy"><%= gettext("Staff and Students") %></p>
         <% } %>
-
-            <!-- NOTE: @andyarmstrong, here's the new copy we're adding to the visibility summary UI to make sure we're as accurate as we can be at the final state of the unit/container's visibility -->
-            <p class="note-visibility">
-                <i class="icon icon-eye-open"></i>
-                <span class="note-copy"><%= gettext("Some content in this unit is only visible to particular groups") %></span>
-            </p>
-
+            <% if (hasContentGroupComponents) { %>
+                <p class="note-visibility">
+                    <i class="icon icon-eye-open"></i>
+                    <span class="note-copy"><%= gettext("Some content in this unit is only visible to particular groups") %></span>
+                </p>
+            <% } %>
         <ul class="actions-inline">
             <li class="action-inline">
                 <a href="" class="action-staff-lock" role="button" aria-pressed="<%= hasExplicitStaffLock %>">

cms/templates/settings.html

@@ -7,7 +7,6 @@
 <%!
   from django.utils.translation import ugettext as _
   from contentstore import utils
-  from contentstore.views.course import should_show_group_configurations_page
   import urllib
 %>
 
@@ -333,9 +332,7 @@ CMS.URL.UPLOAD_ASSET = '${upload_asset_url}';
           <ul>
             <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>
-            % if should_show_group_configurations_page(context_course):
-              <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
-            % endif
+            <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
             <li class="nav-item"><a href="${advanced_config_url}">${_("Advanced Settings")}</a></li>
           </ul>
         </nav>

cms/templates/settings_advanced.html

@@ -4,7 +4,6 @@
 <%!
   from django.utils.translation import ugettext as _
   from contentstore import utils
-  from contentstore.views.course import should_show_group_configurations_page
   from django.utils.html import escapejs
 %>
 <%block name="title">${_("Advanced Settings")}</%block>
@@ -92,9 +91,7 @@
             <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 should_show_group_configurations_page(context_course):
-              <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
-            % endif
+            <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
           </ul>
         </nav>
       % endif

cms/templates/settings_graders.html

@@ -6,7 +6,6 @@
 <%namespace name='static' file='static_content.html'/>
 <%!
   from contentstore import utils
-  from contentstore.views.course import should_show_group_configurations_page
   from django.utils.translation import ugettext as _
 %>
 
@@ -135,9 +134,7 @@
           <ul>
             <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>
-            % if should_show_group_configurations_page(context_course):
-              <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
-            % endif
+            <li class="nav-item"><a href="${utils.reverse_course_url('group_configurations_list_handler', context_course.id)}">${_("Group Configurations")}</a></li>
             <li class="nav-item"><a href="${advanced_settings_url}">${_("Advanced Settings")}</a></li>
           </ul>
         </nav>

cms/templates/studio_xblock_wrapper.html

 <%!
 from django.utils.translation import ugettext as _
 from contentstore.views.helpers import xblock_studio_url
+from contentstore.utils import is_visible_to_specific_content_groups
 import json
 %>
 <%
@@ -38,8 +39,11 @@ messages = json.dumps(xblock.validate().to_json())
     <div class="studio-xblock-wrapper" data-locator="${xblock.location | h}" data-course-key="${xblock.location.course_key | h}">
     % endif
 
-    <!-- NOTE: @andyarmstrong, in order to style the case when an access level is set, we need to add a class to each xblock's wrapper. How does .has-visiblity-set sound? -->
-    <section class="wrapper-xblock ${section_class} ${collapsible_class} has-visibility-set">
+    <section class="wrapper-xblock ${section_class} ${collapsible_class}
+    % if is_visible_to_specific_content_groups(xblock):
+        has-group-visibility-set
+    % endif
+    ">
 % endif
 
 <header class="xblock-header xblock-header-${xblock.category}">
@@ -77,7 +81,6 @@ messages = json.dumps(xblock.validate().to_json())
                             <span class="sr">${_("Delete")}</span>
                             </a>
                         </li>
-                        <!-- NOTE: @andyarmstrong, here's a static version of the new access control we're adding. Remember we wanted to update the tooltip if particular groups were set on a component -->
                         <li class="action-item action-visibility">
                             <a href="#" data-tooltip="${_("Visibility Settings")}" class="visibility-button action-button">
                                 <i class="icon-eye-open"></i>

cms/templates/visibility_editor.html

+<%
+from django.utils.translation import ugettext as _
+
+from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_user_partition
+from contentstore.utils import ancestor_has_staff_lock
+
+cohorted_user_partition = get_cohorted_user_partition(xblock.location.course_key)
+unsorted_groups = cohorted_user_partition.groups if cohorted_user_partition else []
+groups = sorted(unsorted_groups, key=lambda group: group.name)
+selected_group_ids = xblock.group_access.get(cohorted_user_partition.id, []) if cohorted_user_partition else []
+has_selected_groups = len(selected_group_ids) > 0
+is_staff_locked = ancestor_has_staff_lock(xblock)
+%>
+
+<div class="modal-section visibility-summary">
+    % if len(groups) == 0:
+        <div class="is-not-configured has-actions">
+            <h4 class="title">${_('You have not set up any groups to manage visibility with.')}</h4>
+
+            <div class="copy">
+                <p>${_('Groups are a way for you to organize content in your course with a particular student experience in mind. They are commonly used to facilitate content and pedagogical experiments as well as provide different tracks of content.')}</p>
+            </div>
+
+            <div class="actions">
+                <a href="${manage_groups_url}" class="action action-primary action-settings">${_('Manage groups in this course')}</a>
+            </div>
+        </div>
+    % elif is_staff_locked:
+        <div class="summary-message summary-message-warning visibility-summary-message">
+            <i class="icon icon-warning-sign"></i>
+            <p class="copy">${_('The Unit this component is contained in is hidden from students. Visibility settings here will be trumped by this.')}</p>
+        </div>
+    % endif
+</div>
+
+% if len(groups) > 0:
+    <form class="visibility-controls-form" method="post" action="">
+
+        <div class="modal-section visibility-controls">
+            <h3 class="modal-section-title">${_('Set visibility to:')}</h3>
+
+            <div class="modal-section-content">
+
+                <section class="visibility-controls-primary">
+                    <ul class="list-fields list-radio">
+                        <li class="field field-radio field-visibility-level">
+                            <input type="radio" id="visibility-level-all" name="visibility-level" value="" class="input input-radio visibility-level-all" ${'checked="checked"' if not has_selected_groups else ''} />
+                            <label for="visibility-level-all" class="label">${_('All Students and Staff')}</label>
+                        </li>
+
+                        <li class="field field-radio field-visibility-level">
+                            <input type="radio" id="visibility-level-specific" name="visibility-level" value="" class="input input-radio visibility-level-specific"  ${'checked="checked"' if has_selected_groups else ''} />
+                            <label for="visibility-level-specific" class="label">${_('Specific Content Groups')}</label>
+                        </li>
+                    </ul>
+                </section>
+
+                <div class="wrapper-visibility-specific" data-user-partition-id="${cohorted_user_partition.id}">
+                    <section class="visibility-controls-secondary">
+                        <div class="visibility-controls-group">
+                            <h4 class="visibility-controls-title modal-subsection-title sr">${_('Content Groups')}</h4>
+                            <ul class="list-fields list-checkbox">
+                                <%
+                                missing_group_ids = set(selected_group_ids)
+                                %>
+                                % for group in groups:
+                                    <%
+                                    is_group_selected = group.id in selected_group_ids
+                                    if is_group_selected:
+                                        missing_group_ids.remove(group.id)
+                                    %>
+                                    <li class="field field-checkbox field-visibility-content-group">
+                                        <input type="checkbox" id="visibility-content-group-${group.id}" name="visibility-content-group" value="${group.id}" class="input input-checkbox"  ${'checked="checked"' if group.id in selected_group_ids else ''}/>
+                                        <label for="visibility-content-group-${group.id}" class="label">${group.name | h}</label>
+                                    </li>
+                                % endfor
+
+                                % for group_id in missing_group_ids:
+                                    <li class="field field-checkbox field-visibility-content-group was-removed">
+                                        <input type="checkbox" id="visibility-content-group-${group_id}" name="visibility-content-group" value="${group_id}" class="input input-checkbox" checked="checked" />
+                                        <label for="visibility-content-group-${group_id}" class="label">
+                                            ${_('Deleted Content Group')}
+                                            <span class="note">${_('Content group no longer exists. Please choose another or allow access to All Students and staff')}</span>
+                                        </label>
+                                    </li>
+                                % endfor
+                            </ul>
+                        </div>
+                    </section>
+                </div>
+            </div>
+        </div>
+    </form>
+% endif

cms/templates/widgets/header.html

@@ -3,7 +3,6 @@
   from django.core.urlresolvers import reverse
   from django.utils.translation import ugettext as _
   from contentstore.context_processors import doc_url
-  from contentstore.views.course import should_show_group_configurations_page
 %>
 <%page args="online_help_token"/>
 
@@ -93,11 +92,9 @@
                   <li class="nav-item nav-course-settings-team">
                     <a href="${course_team_url}">${_("Course Team")}</a>
                   </li>
-                  % if should_show_group_configurations_page(context_course):
-                    <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>
-                  % endif
+                  <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>
                   <li class="nav-item nav-course-settings-advanced">
                     <a href="${advanced_settings_url}">${_("Advanced Settings")}</a>
                   </li>

cms/urls.py

@@ -91,6 +91,7 @@ urlpatterns += patterns(
     url(r'^import_status/{}/(?P<filename>.+)$'.format(settings.COURSE_KEY_PATTERN), 'import_status_handler'),
     url(r'^export/{}$'.format(settings.COURSE_KEY_PATTERN), 'export_handler'),
     url(r'^xblock/outline/{}$'.format(settings.USAGE_KEY_PATTERN), 'xblock_outline_handler'),
+    url(r'^xblock/container/{}$'.format(settings.USAGE_KEY_PATTERN), 'xblock_container_handler'),
     url(r'^xblock/{}/(?P<view_name>[^/]+)$'.format(settings.USAGE_KEY_PATTERN), 'xblock_view_handler'),
     url(r'^xblock/{}?$'.format(settings.USAGE_KEY_PATTERN), 'xblock_handler'),
     url(r'^tabs/{}$'.format(settings.COURSE_KEY_PATTERN), 'tabs_handler'),

common/test/acceptance/pages/studio/component_editor.py

@@ -4,9 +4,9 @@ from utils import click_css
 from selenium.webdriver.support.ui import Select
 
 
-class ComponentEditorView(PageObject):
+class BaseComponentEditorView(PageObject):
     """
-    A :class:`.PageObject` representing the rendered view of a component editor.
+    A base :class:`.PageObject` for the component and visibility editors.
 
     This class assumes that the editor is our default editor as displayed for xmodules.
     """
@@ -18,7 +18,7 @@ class ComponentEditorView(PageObject):
             browser (selenium.webdriver): The Selenium-controlled browser that this page is loaded in.
             locator (str): The locator that identifies which xblock this :class:`.xblock-editor` relates to.
         """
-        super(ComponentEditorView, self).__init__(browser)
+        super(BaseComponentEditorView, self).__init__(browser)
         self.locator = locator
 
     def is_browser_on_page(self):
@@ -40,6 +40,23 @@ class ComponentEditorView(PageObject):
         """
         return None
 
+    def save(self):
+        """
+        Clicks save button.
+        """
+        click_css(self, 'a.action-save')
+
+    def cancel(self):
+        """
+        Clicks cancel button.
+        """
+        click_css(self, 'a.action-cancel', require_notification=False)
+
+
+class ComponentEditorView(BaseComponentEditorView):
+    """
+    A :class:`.PageObject` representing the rendered view of a component editor.
+    """
     def get_setting_element(self, label):
         """
         Returns the index of the setting entry with given label (display name) within the Settings modal.
@@ -86,14 +103,48 @@ class ComponentEditorView(PageObject):
         else:
             return None
 
-    def save(self):
+
+class ComponentVisibilityEditorView(BaseComponentEditorView):
+    """
+    A :class:`.PageObject` representing the rendered view of a component visibility editor.
+    """
+    OPTION_SELECTOR = '.modal-section-content li.field'
+
+    @property
+    def all_options(self):
         """
-        Clicks save button.
+        Return all visibility 'li' options.
         """
-        click_css(self, 'a.action-save')
+        return self.q(css=self._bounded_selector(self.OPTION_SELECTOR)).results
 
-    def cancel(self):
+    @property
+    def selected_options(self):
         """
-        Clicks cancel button.
+        Return all selected visibility 'li' options.
         """
-        click_css(self, 'a.action-cancel', require_notification=False)
+        results = []
+        for option in self.all_options:
+            button = option.find_element_by_css_selector('input.input')
+            if button.is_selected():
+                results.append(option)
+        return results
+
+    def select_option(self, label_text, save=True):
+        """
+        Click the first li which has a label matching `label_text`.
+
+        Arguments:
+            label_text (str): Text of a label accompanying the input
+                which should be clicked.
+            save (boolean): Whether the "save" button should be clicked
+                afterwards.
+        Returns:
+            bool: Whether the label was found and clicked.
+        """
+        for option in self.all_options:
+            if label_text in option.text:
+                option.click()
+                if save:
+                    self.save()
+                return True
+        return False

common/test/acceptance/pages/studio/container.py

@@ -153,6 +153,13 @@ class ContainerPage(PageObject):
         return self.q(css='.bit-publishing .wrapper-visibility .copy .inherited-from').visible
 
     @property
+    def sidebar_visibility_message(self):
+        """
+        Returns the text within the sidebar visibility section.
+        """
+        return self.q(css='.bit-publishing .wrapper-visibility').first.text[0]
+
+    @property
     def publish_action(self):
         """
         Returns the link for publishing a unit.
@@ -243,7 +250,7 @@ class ContainerPage(PageObject):
         """
         Clicks the "edit" button for the first component on the page.
         """
-        return _click_edit(self)
+        return _click_edit(self, '.edit-button', '.xblock-studio_view')
 
     def add_missing_groups(self):
         """
@@ -282,6 +289,7 @@ class XBlockWrapper(PageObject):
     url = None
     BODY_SELECTOR = '.studio-xblock-wrapper'
     NAME_SELECTOR = '.xblock-display-name'
+    VALIDATION_SELECTOR = '.xblock-message.validation'
     COMPONENT_BUTTONS = {
         'basic_tab': '.editor-tabs li.inner_tab_wrap:nth-child(1) > a',
         'advanced_tab': '.editor-tabs li.inner_tab_wrap:nth-child(2) > a',
@@ -348,11 +356,11 @@ class XBlockWrapper(PageObject):
     @property
     def has_validation_message(self):
         """ Is a validation warning/error/message shown? """
-        return self.q(css=self._bounded_selector('.xblock-message.validation')).present
+        return self.q(css=self._bounded_selector(self.VALIDATION_SELECTOR)).present
 
     def _validation_paragraph(self, css_class):
         """ Helper method to return the <p> element of a validation warning """
-        return self.q(css=self._bounded_selector('.xblock-message.validation p.{}'.format(css_class)))
+        return self.q(css=self._bounded_selector('{} p.{}'.format(self.VALIDATION_SELECTOR, css_class)))
 
     @property
     def has_validation_warning(self):
@@ -381,6 +389,10 @@ class XBlockWrapper(PageObject):
         return self._validation_paragraph('error').text[0]
 
     @property
+    def validation_error_messages(self):
+        return self.q(css=self._bounded_selector('{} .xblock-message-item.error'.format(self.VALIDATION_SELECTOR))).text
+
+    @property
     # pylint: disable=invalid-name
     def validation_not_configured_warning_text(self):
         """ Get the text of the validation "not configured" message. """
@@ -390,6 +402,10 @@ class XBlockWrapper(PageObject):
     def preview_selector(self):
         return self._bounded_selector('.xblock-student_view,.xblock-author_view')
 
+    @property
+    def has_group_visibility_set(self):
+        return self.q(css=self._bounded_selector('.wrapper-xblock.has-group-visibility-set')).is_present()
+
     def go_to_container(self):
         """
         Open the container page linked to by this xblock, and return
@@ -401,7 +417,13 @@ class XBlockWrapper(PageObject):
         """
         Clicks the "edit" button for this xblock.
         """
-        return _click_edit(self, self._bounded_selector)
+        return _click_edit(self, '.edit-button', '.xblock-studio_view', self._bounded_selector)
+
+    def edit_visibility(self):
+        """
+        Clicks the edit visibility button for this xblock.
+        """
+        return _click_edit(self, '.visibility-button', '.xblock-visibility_view', self._bounded_selector)
 
     def open_advanced_tab(self):
         """
@@ -478,13 +500,13 @@ class XBlockWrapper(PageObject):
         return self.q(css=self._bounded_selector('span.message-text a')).first.text[0]
 
 
-def _click_edit(page_object, bounded_selector=lambda(x): x):
+def _click_edit(page_object, button_css, view_css, bounded_selector=lambda(x): x):
     """
-    Click on the first edit button found and wait for the Studio editor to be present.
+    Click on the first editing button found and wait for the Studio editor to be present.
     """
-    page_object.q(css=bounded_selector('.edit-button')).first.click()
+    page_object.q(css=bounded_selector(button_css)).first.click()
     EmptyPromise(
-        lambda: page_object.q(css='.xblock-studio_view').present,
+        lambda: page_object.q(css=view_css).present,
         'Wait for the Studio editor to be present'
     ).fulfill()
 

common/test/acceptance/tests/helpers.py

@@ -10,8 +10,8 @@ from path import path
 from bok_choy.javascript import js_defined
 from bok_choy.web_app_test import WebAppTest
 from opaque_keys.edx.locator import CourseLocator
-from xmodule.partitions.tests.test_partitions import MockUserPartitionScheme
 from xmodule.partitions.partitions import UserPartition
+from xmodule.partitions.tests.test_partitions import MockUserPartitionScheme
 
 
 def skip_if_browser(browser):

common/test/acceptance/tests/studio/test_studio_split_test.py

@@ -8,7 +8,7 @@ from unittest import skip
 from nose.plugins.attrib import attr
 from selenium.webdriver.support.ui import Select
 
-from xmodule.partitions.partitions import Group, UserPartition
+from xmodule.partitions.partitions import Group
 from bok_choy.promise import Promise, EmptyPromise
 
 from ...fixtures.course import XBlockFixtureDesc
@@ -217,37 +217,14 @@ class SettingsMenuTest(StudioCourseTest):
         )
         self.advanced_settings.visit()
 
-    def test_link_exist_if_split_test_enabled(self):
+    def test_link_exist(self):
         """
         Ensure that the link to the "Group Configurations" page is shown in the
         Settings menu.
         """
         link_css = 'li.nav-course-settings-group-configurations a'
-        self.assertFalse(self.advanced_settings.q(css=link_css).present)
-
-        self.advanced_settings.set('Advanced Module List', '["split_test"]')
-
-        self.browser.refresh()
-        self.advanced_settings.wait_for_page()
-
-        self.assertIn(
-            "split_test",
-            json.loads(self.advanced_settings.get('Advanced Module List')),
-        )
-
         self.assertTrue(self.advanced_settings.q(css=link_css).present)
 
-    def test_link_does_not_exist_if_split_test_disabled(self):
-        """
-        Ensure that the link to the "Group Configurations" page does not exist
-        in the Settings menu.
-        """
-        link_css = 'li.nav-course-settings-group-configurations a'
-        self.advanced_settings.set('Advanced Module List', '[]')
-        self.browser.refresh()
-        self.advanced_settings.wait_for_page()
-        self.assertFalse(self.advanced_settings.q(css=link_css).present)
-
 
 @attr('shard_1')
 class GroupConfigurationsTest(ContainerBase, SplitTestMixin):

lms/djangoapps/lms_xblock/mixin.py

@@ -69,7 +69,6 @@ class LmsBlockMixin(XBlockMixin):
         default=False,
         scope=Scope.settings,
     )
-
     group_access = GroupAccessDict(
         help=_(
             "A dictionary that maps which groups can be shown this block. The keys "
@@ -143,26 +142,32 @@ class LmsBlockMixin(XBlockMixin):
         """
         _ = self.runtime.service(self, "i18n").ugettext  # pylint: disable=redefined-outer-name
         validation = super(LmsBlockMixin, self).validate()
+        has_invalid_user_partitions = False
+        has_invalid_groups = False
         for user_partition_id, group_ids in self.group_access.iteritems():
             try:
                 user_partition = self._get_user_partition(user_partition_id)
             except NoSuchUserPartitionError:
-                validation.add(
-                    ValidationMessage(
-                        ValidationMessage.ERROR,
-                        _(u"This xblock refers to a deleted or invalid content group configuration.")
-                    )
-                )
+                has_invalid_user_partitions = True
             else:
                 for group_id in group_ids:
                     try:
                         user_partition.get_group(group_id)
                     except NoSuchUserPartitionGroupError:
-                        validation.add(
-                            ValidationMessage(
-                                ValidationMessage.ERROR,
-                                _(u"This xblock refers to a deleted or invalid content group.")
-                            )
-                        )
+                        has_invalid_groups = True
 
+        if has_invalid_user_partitions:
+            validation.add(
+                ValidationMessage(
+                    ValidationMessage.ERROR,
+                    _(u"This component refers to deleted or invalid content group configurations.")
+                )
+            )
+        if has_invalid_groups:
+            validation.add(
+                ValidationMessage(
+                    ValidationMessage.ERROR,
+                    _(u"This component refers to deleted or invalid content groups.")
+                )
+            )
         return validation

lms/lib/xblock/test/test_mixin.py

@@ -70,29 +70,51 @@ class XBlockValidationTest(LmsXBlockMixinTestCase):
         validation = self.video.validate()
         self.assertEqual(len(validation.messages), 0)
 
-    def test_validate_invalid_user_partition(self):
+    def test_validate_invalid_user_partitions(self):
         """
-        Test the validation messages produced for an xblock referring to a non-existent user partition.
+        Test the validation messages produced for an xblock referring to non-existent user partitions.
         """
         self.video.group_access[999] = [self.group1.id]
         validation = self.video.validate()
         self.assertEqual(len(validation.messages), 1)
         self.verify_validation_message(
             validation.messages[0],
-            u"This xblock refers to a deleted or invalid content group configuration.",
+            u"This component refers to deleted or invalid content group configurations.",
             ValidationMessage.ERROR,
         )
 
-    def test_validate_invalid_group(self):
+        # Now add a second invalid user partition and validate again.
+        # Note that even though there are two invalid configurations,
+        # only a single error message will be returned.
+        self.video.group_access[998] = [self.group2.id]
+        validation = self.video.validate()
+        self.assertEqual(len(validation.messages), 1)
+        self.verify_validation_message(
+            validation.messages[0],
+            u"This component refers to deleted or invalid content group configurations.",
+            ValidationMessage.ERROR,
+        )
+
+    def test_validate_invalid_groups(self):
         """
-        Test the validation messages produced for an xblock referring to a non-existent group.
+        Test the validation messages produced for an xblock referring to non-existent groups.
         """
         self.video.group_access[self.user_partition.id] = [self.group1.id, 999]    # pylint: disable=no-member
         validation = self.video.validate()
         self.assertEqual(len(validation.messages), 1)
         self.verify_validation_message(
             validation.messages[0],
-            u"This xblock refers to a deleted or invalid content group.",
+            u"This component refers to deleted or invalid content groups.",
+            ValidationMessage.ERROR,
+        )
+
+        # Now try again with two invalid group ids
+        self.video.group_access[self.user_partition.id] = [self.group1.id, 998, 999]    # pylint: disable=no-member
+        validation = self.video.validate()
+        self.assertEqual(len(validation.messages), 1)
+        self.verify_validation_message(
+            validation.messages[0],
+            u"This component refers to deleted or invalid content groups.",
             ValidationMessage.ERROR,
         )