Short-circuit group_access check if only user_partitions are split_test partitions.

common/lib/xmodule/xmodule/partitions/tests/test_partitions.py

@@ -115,13 +115,14 @@ class PartitionTestCase(TestCase):
 
     def setUp(self):
         # Set up two user partition schemes: mock and random
+        self.non_random_scheme = MockUserPartitionScheme(self.TEST_SCHEME_NAME)
+        self.random_scheme = MockUserPartitionScheme("random")
         extensions = [
             Extension(
-                self.TEST_SCHEME_NAME, USER_PARTITION_SCHEME_NAMESPACE,
-                MockUserPartitionScheme(self.TEST_SCHEME_NAME), None
+                self.non_random_scheme.name, USER_PARTITION_SCHEME_NAMESPACE, self.non_random_scheme, None
             ),
             Extension(
-                "random", USER_PARTITION_SCHEME_NAMESPACE, MockUserPartitionScheme("random"), None
+                self.random_scheme.name, USER_PARTITION_SCHEME_NAMESPACE, self.random_scheme, None
             ),
         ]
         UserPartition.scheme_extensions = ExtensionManager.make_test_instance(
@@ -137,6 +138,10 @@ class PartitionTestCase(TestCase):
             extensions[0].plugin
         )
 
+        # Make sure the names are set on the schemes (which happens normally in code, but may not happen in tests).
+        self.user_partition.get_scheme(self.non_random_scheme.name)
+        self.user_partition.get_scheme(self.random_scheme.name)
+
 
 class TestUserPartition(PartitionTestCase):
     """Test constructing UserPartitions"""

common/lib/xmodule/xmodule/split_test_module.py

@@ -49,8 +49,10 @@ class SplitTestFields(object):
         # Add "No selection" value if there is not a valid selected user partition.
         if not selected_user_partition:
             SplitTestFields.user_partition_values.append(SplitTestFields.no_partition_selected)
-        for user_partition in all_user_partitions:
-            SplitTestFields.user_partition_values.append({"display_name": user_partition.name, "value": user_partition.id})
+        for user_partition in get_split_user_partitions(all_user_partitions):
+            SplitTestFields.user_partition_values.append(
+                {"display_name": user_partition.name, "value": user_partition.id}
+            )
         return SplitTestFields.user_partition_values
 
     display_name = String(
@@ -86,6 +88,14 @@ class SplitTestFields(object):
     )
 
 
+def get_split_user_partitions(user_partitions):
+    """
+    Helper method that filters a list of user_partitions and returns just the
+    ones that are suitable for the split_test module.
+    """
+    return [user_partition for user_partition in user_partitions if user_partition.scheme.name == "random"]
+
+
 @XBlock.needs('user_tags')  # pylint: disable=abstract-method
 @XBlock.wants('partitions')
 class SplitTestModule(SplitTestFields, XModule, StudioEditableModule):
@@ -566,23 +576,35 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes
                     )
                 )
             else:
-                [active_children, inactive_children] = self.active_and_inactive_children()
-                if len(active_children) < len(user_partition.groups):
+                # If the user_partition selected is not valid for the split_test module, error.
+                # This can only happen via XML and import/export.
+                if not get_split_user_partitions([user_partition]):
                     split_validation.add(
                         StudioValidationMessage(
                             StudioValidationMessage.ERROR,
-                            _(u"The experiment does not contain all of the groups in the configuration."),
-                            action_runtime_event='add-missing-groups',
-                            action_label=_(u"Add Missing Groups")
+                            _(u"The experiment uses a group configuration that is not supported for experiments. "
+                              u"Select a valid group configuration or delete this experiment.")
                         )
                     )
-                if len(inactive_children) > 0:
-                    split_validation.add(
-                        StudioValidationMessage(
-                            StudioValidationMessage.WARNING,
-                            _(u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.")
+                else:
+                    [active_children, inactive_children] = self.active_and_inactive_children()
+                    if len(active_children) < len(user_partition.groups):
+                        split_validation.add(
+                            StudioValidationMessage(
+                                StudioValidationMessage.ERROR,
+                                _(u"The experiment does not contain all of the groups in the configuration."),
+                                action_runtime_event='add-missing-groups',
+                                action_label=_(u"Add Missing Groups")
+                            )
+                        )
+                    if len(inactive_children) > 0:
+                        split_validation.add(
+                            StudioValidationMessage(
+                                StudioValidationMessage.WARNING,
+                                _(u"The experiment has an inactive group. "
+                                  u"Move content into active groups, then delete the inactive group.")
+                            )
                         )
-                    )
         return split_validation
 
     def general_validation_message(self, validation=None):

common/lib/xmodule/xmodule/tests/test_split_test_module.py

@@ -12,7 +12,7 @@ from xmodule.tests.xml import XModuleXmlImportTest
 from xmodule.tests import get_test_system
 from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW
 from xmodule.validation import StudioValidationMessage
-from xmodule.split_test_module import SplitTestDescriptor, SplitTestFields
+from xmodule.split_test_module import SplitTestDescriptor, SplitTestFields, get_split_user_partitions
 from xmodule.partitions.partitions import Group, UserPartition
 
 
@@ -23,6 +23,37 @@ class SplitTestModuleFactory(xml.XmlImportFactory):
     tag = 'split_test'
 
 
+class SplitTestUtilitiesTest(PartitionTestCase):
+    """
+    Tests for utility methods related to split_test module.
+    """
+    def test_split_user_partitions(self):
+        """
+        Tests the get_split_user_partitions helper method.
+        """
+        first_random_partition = UserPartition(
+            0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta')],
+            self.random_scheme
+        )
+        second_random_partition = UserPartition(
+            0, 'second_partition', 'Second Partition', [Group("4", 'zeta'), Group("5", 'omega')],
+            self.random_scheme
+        )
+        all_partitions = [
+            first_random_partition,
+            # Only UserPartitions with scheme "random" will be returned as available options.
+            UserPartition(
+                1, 'non_random_partition', 'Will Not Be Returned', [Group("1", 'apple'), Group("2", 'banana')],
+                self.non_random_scheme
+            ),
+            second_random_partition
+        ]
+        self.assertEqual(
+            [first_random_partition, second_random_partition],
+            get_split_user_partitions(all_partitions)
+        )
+
+
 class SplitTestModuleTest(XModuleXmlImportTest, PartitionTestCase):
     """
     Base class for all split_module tests.
@@ -221,7 +252,15 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
 
         # Populate user_partitions and call editable_metadata_fields again
         self.split_test_module.user_partitions = [
-            UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta')])
+            UserPartition(
+                0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta')],
+                self.random_scheme
+            ),
+            # Only UserPartitions with scheme "random" will be returned as available options.
+            UserPartition(
+                1, 'non_random_partition', 'Will Not Be Returned', [Group("1", 'apple'), Group("2", 'banana')],
+                self.non_random_scheme
+            )
         ]
         self.split_test_module.editable_metadata_fields  # pylint: disable=pointless-statement
         partitions = SplitTestDescriptor.user_partition_id.values
@@ -423,3 +462,25 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
             u"This content experiment has issues that affect content visibility.",
             StudioValidationMessage.ERROR
         )
+
+        # Verify the message for a split test referring to a non-random user partition
+        split_test_module.user_partitions = [
+            UserPartition(
+                10, 'incorrect_partition', 'Non Random Partition', [Group("0", 'alpha'), Group("2", 'gamma')],
+                scheme=self.non_random_scheme
+            )
+        ]
+        split_test_module.user_partition_id = 10
+        validation = split_test_module.validate()
+        self.assertEqual(len(validation.messages), 1)
+        verify_validation_message(
+            validation.messages[0],
+            u"The experiment uses a group configuration that is not supported for experiments. "
+            u"Select a valid group configuration or delete this experiment.",
+            StudioValidationMessage.ERROR
+        )
+        verify_summary_message(
+            validation.summary,
+            u"This content experiment has issues that affect content visibility.",
+            StudioValidationMessage.ERROR
+        )

lms/djangoapps/courseware/access.py

@@ -13,6 +13,7 @@ from xmodule.course_module import (
     CATALOG_VISIBILITY_ABOUT)
 from xmodule.error_module import ErrorDescriptor
 from xmodule.x_module import XModule
+from xmodule.split_test_module import get_split_user_partitions
 
 from xblock.core import XBlock
 from xmodule.partitions.partitions import NoSuchUserPartitionError, NoSuchUserPartitionGroupError
@@ -307,8 +308,10 @@ def _has_group_access(descriptor, user, course_key):
     This function returns a boolean indicating whether or not `user` has
     sufficient group memberships to "load" a block (the `descriptor`)
     """
-    if len(descriptor.user_partitions) == 0:
-        # short circuit the process, since there are no defined user partitions
+    if len(descriptor.user_partitions) == len(get_split_user_partitions(descriptor.user_partitions)):
+        # Short-circuit the process, since there are no defined user partitions that are not
+        # user_partitions used by the split_test module. The split_test module handles its own access
+        # via updating the children of the split_test module.
         return True
 
     # use merged_group_access which takes group access on the block's

lms/djangoapps/courseware/tests/test_group_access.py

@@ -81,6 +81,12 @@ class GroupAccessTestCase(ModuleStoreTestCase):
                     MemoryUserPartitionScheme(),
                     None
                 ),
+                Extension(
+                    "random",
+                    USER_PARTITION_SCHEME_NAMESPACE,
+                    MemoryUserPartitionScheme(),
+                    None
+                )
             ],
             namespace=USER_PARTITION_SCHEME_NAMESPACE
         )
@@ -382,3 +388,31 @@ class GroupAccessTestCase(ModuleStoreTestCase):
         self.check_access(self.blue_dog, block_accessed, False)
         self.check_access(self.gray_worm, block_accessed, False)
         self.ensure_staff_access(block_accessed)
+
+    def test_group_access_short_circuits(self):
+        """
+        Test that the group_access check short-circuits if there are no user_partitions defined
+        except user_partitions in use by the split_test module.
+        """
+        # Initially, "red_cat" user can't view the vertical.
+        self.set_group_access(self.chapter, {self.animal_partition.id: [self.dog_group.id]})
+        self.check_access(self.red_cat, self.vertical, False)
+
+        # Change the vertical's user_partitions value to the empty list. Now red_cat can view the vertical.
+        self.vertical.user_partitions = []
+        self.check_access(self.red_cat, self.vertical, True)
+
+        # Change the vertical's user_partitions value to include only "split_test" partitions.
+        split_test_partition = UserPartition(
+            199,
+            'split_test partition',
+            'nothing to look at here',
+            [Group(2, 'random group')],
+            scheme=UserPartition.get_scheme("random"),
+        )
+        self.vertical.user_partitions = [split_test_partition]
+        self.check_access(self.red_cat, self.vertical, True)
+
+        # Finally, add back in a cohort user_partition
+        self.vertical.user_partitions = [split_test_partition, self.animal_partition]
+        self.check_access(self.red_cat, self.vertical, False)