Expand and refactor teams configuration on course. (#22168)

Wrap CourseModule.teams_configuration in TeamsConfig
class, centralizing parsing, validation, error handling,
etc. Wrapped object is exposed on 'teams_conf' field.

Old code still uses 'teams_configuration' dict;
we should change this in the future (MST-18).

MST-16

common/lib/xmodule/xmodule/course_module.py

@@ -22,6 +22,7 @@ from xblock.fields import Boolean, Dict, Float, Integer, List, Scope, String
 from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
 from openedx.core.djangoapps.video_pipeline.models import VideoUploadsEnabledByDefault
 from openedx.core.lib.license import LicenseMixin
+from openedx.core.lib.teams_config import TeamsConfig
 from xmodule import course_metadata_utils
 from xmodule.course_metadata_utils import DEFAULT_GRADING_POLICY, DEFAULT_START_DATE
 from xmodule.graders import grader_from_conf
@@ -1470,12 +1471,41 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin):
         """
         return course_metadata_utils.clean_course_key(self.location.course_key, padding_char)
 
+    @property
+    def teams_conf(self):
+        """
+        Returns a TeamsConfig object wrapping the dict `teams_configuration`.
+
+        TODO: In MST-18, `teams_configuration` will be a custom field that
+        parses its input into a TeamsConfig object.
+        All references to this property will become references to
+        `.teams_configuration`.
+        """
+        # If the `teams_configuration` dict hasn't changed, return a cached
+        # `TeamsConfig` to avoid re-parsing things.
+        # If it has changed, recompute the `TeamsConfig`.
+        try:
+            cached_teams_conf, cached_teams_conf_source = (
+                self._teams_conf, self._teams_conf_source
+            )
+        except AttributeError:
+            pass
+        else:
+            if cached_teams_conf_source == self.teams_configuration:
+                return cached_teams_conf
+        self._teams_conf_source = self.teams_configuration
+        self._teams_conf = TeamsConfig(self._teams_conf_source)
+        return self._teams_conf
+
     @property
     def teams_enabled(self):
         """
         Returns whether or not teams has been enabled for this course.
 
         Currently, teams are considered enabled when at least one topic has been configured for the course.
+
+        Deprecated; please use `self.teams_conf.is_enabled` instead.
+        Will be removed (TODO MST-18).
         """
         if self.teams_configuration:
             return len(self.teams_configuration.get('topics', [])) > 0
@@ -1485,6 +1515,9 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin):
     def teams_max_size(self):
         """
         Returns the max size for teams if teams has been configured, else None.
+
+        Deprecated; please use `self.teams_conf.calc_max_team_size(...)` instead.
+        Will be removed (TODO MST-18).
         """
         return self.teams_configuration.get('max_team_size', None)
 
@@ -1492,6 +1525,9 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin):
     def teams_topics(self):
         """
         Returns the topics that have been configured for teams for this course, else None.
+
+        Deprecated; please use `self.teams_conf.teamsets` instead.
+        Will be removed (TODO MST-18).
         """
         return self.teams_configuration.get('topics', None)
 

common/lib/xmodule/xmodule/tests/test_course_module.py

@@ -272,6 +272,10 @@ class DiscussionTopicsTestCase(unittest.TestCase):
 class TeamsConfigurationTestCase(unittest.TestCase):
     """
     Tests for the configuration of teams and the helper methods for accessing them.
+
+    Also tests new configuration wrapper, `.teams_conf`.
+    Eventually the tests for the old dict-based `.teams_configuration` field
+    wil be removed (TODO MST-18).
     """
 
     def setUp(self):
@@ -299,44 +303,69 @@ class TeamsConfigurationTestCase(unittest.TestCase):
     def test_teams_enabled_new_course(self):
         # Make sure we can detect when no teams exist.
         self.assertFalse(self.course.teams_enabled)
+        self.assertFalse(self.course.teams_conf.is_enabled)
 
         # add topics
         self.add_team_configuration(max_team_size=4, topics=[self.make_topic()])
         self.assertTrue(self.course.teams_enabled)
+        self.assertTrue(self.course.teams_conf.is_enabled)
 
         # remove them again
         self.add_team_configuration(max_team_size=4, topics=[])
         self.assertFalse(self.course.teams_enabled)
+        self.assertFalse(self.course.teams_conf.is_enabled)
 
     def test_teams_enabled_max_size_only(self):
         self.add_team_configuration(max_team_size=4)
         self.assertFalse(self.course.teams_enabled)
+        self.assertFalse(self.course.teams_conf.is_enabled)
 
     def test_teams_enabled_no_max_size(self):
         self.add_team_configuration(max_team_size=None, topics=[self.make_topic()])
         self.assertTrue(self.course.teams_enabled)
+        self.assertTrue(self.course.teams_conf.is_enabled)
 
     def test_teams_max_size_no_teams_configuration(self):
         self.assertIsNone(self.course.teams_max_size)
+        self.assertIsNone(self.course.teams_conf.max_team_size)
 
     def test_teams_max_size_with_teams_configured(self):
         size = 4
         self.add_team_configuration(max_team_size=size, topics=[self.make_topic(), self.make_topic()])
         self.assertTrue(self.course.teams_enabled)
         self.assertEqual(size, self.course.teams_max_size)
+        self.assertEqual(size, self.course.teams_conf.max_team_size)
 
     def test_teams_topics_no_teams(self):
         self.assertIsNone(self.course.teams_topics)
+        self.assertEqual(self.course.teams_conf.teamsets, [])
 
     def test_teams_topics_no_topics(self):
         self.add_team_configuration(max_team_size=4)
         self.assertEqual(self.course.teams_topics, [])
+        self.assertEqual(self.course.teams_conf.teamsets, [])
 
     def test_teams_topics_with_topics(self):
         topics = [self.make_topic(), self.make_topic()]
         self.add_team_configuration(max_team_size=4, topics=topics)
         self.assertTrue(self.course.teams_enabled)
         self.assertEqual(self.course.teams_topics, topics)
+        expected_teamsets_data = [
+            teamset.cleaned_data_old_format
+            for teamset in self.course.teams_conf.teamsets
+        ]
+        self.assertEqual(expected_teamsets_data, topics)
+
+    def test_teams_conf_caching(self):
+        self.add_team_configuration(max_team_size=5, topics=[self.make_topic()])
+        cold_cache_conf = self.course.teams_conf
+        warm_cache_conf = self.course.teams_conf
+        self.add_team_configuration(max_team_size=5, topics=[self.make_topic(), self.make_topic()])
+        new_cold_cache_conf = self.course.teams_conf
+        new_warm_cache_conf = self.course.teams_conf
+        self.assertIs(cold_cache_conf, warm_cache_conf)
+        self.assertIs(new_cold_cache_conf, new_warm_cache_conf)
+        self.assertIsNot(cold_cache_conf, new_cold_cache_conf)
 
 
 class SelfPacedTestCase(unittest.TestCase):

openedx/core/lib/teams_config.py

+"""
+Safe configuration wrapper for Course Teams feature.
+"""
+from __future__ import absolute_import, unicode_literals
+
+import re
+from enum import Enum
+
+import six
+from django.utils.functional import cached_property
+
+
+class TeamsConfig(object):
+    """
+    Configuration for the Course Teams feature on a course run.
+
+    Takes in a configuration from a JSON-friendly dictionary,
+    and exposes cleaned data from it.
+    """
+    def __init__(self, data):
+        """
+        Initialize a TeamsConfig object with a dictionary.
+        """
+        self._data = data if isinstance(data, dict) else {}
+
+    def __unicode__(self):
+        """
+        Return user-friendly string.
+
+        TODO move this code to __str__ after Py3 upgrade.
+        """
+        return "Teams configuration for {} team-sets".format(len(self.teamsets))
+
+    def __str__(self):
+        """
+        Return user-friendly string.
+        """
+        return str(self.__unicode__())
+
+    def __repr__(self):
+        """
+        Return developer-helpful string.
+        """
+        return "<{} max_team_size={} teamsets=[{}]>".format(
+            self.__class__.__name__,
+            self.max_team_size,
+            ", ".join(repr(teamset) for teamset in self.teamsets),
+        )
+
+    @property
+    def source_data(self):
+        """
+        Dictionary containing the data from which this TeamsConfig was built.
+        """
+        return self._data
+
+    @cached_property
+    def cleaned_data(self):
+        """
+        JSON-friendly dictionary containing cleaned data from this TeamsConfig.
+        """
+        return {
+            'max_team_size': self.max_team_size,
+            'team_sets': [
+                teamset.cleaned_data for teamset in self.teamsets
+            ]
+        }
+
+    @cached_property
+    def cleaned_data_old_format(self):
+        """
+        JSON-friendly dictionary containing cleaned data from this TeamsConfig,
+        excluding newly added fields.
+
+        Here for backwards compatibility; to be removed (TODO MST-18).
+        """
+        return {
+            'max_team_size': self.max_team_size,
+            'topics': [
+                teamset.cleaned_data_old_format for teamset in self.teamsets
+            ]
+        }
+
+    @property
+    def is_enabled(self):
+        """
+        Whether the Course Teams feature is enabled for this course run.
+        """
+        return bool(self.teamsets)
+
+    @cached_property
+    def teamsets(self):
+        """
+        List of configurations for team-sets.
+
+        A team-set is a logical collection of teams, generally centered around a
+        discussion topic or assignment.
+
+        A learner should be able to join one team per team-set
+        (TODO MST-12... currently, a learner may join one team per course).
+        """
+        all_teamsets_data = self._data.get(
+            'team_sets',
+            # For backwards compatibility, also check "topics" key.
+            self._data.get('topics', [])
+        )
+        if not isinstance(all_teamsets_data, list):
+            return []
+        all_teamsets = [
+            TeamsetConfig(teamset_data)
+            for teamset_data in all_teamsets_data
+        ]
+        good_teamsets = []
+        seen_ids = set()
+        for teamset in all_teamsets:
+            if teamset.teamset_id and teamset.teamset_id not in seen_ids:
+                good_teamsets.append(teamset)
+                seen_ids.add(teamset.teamset_id)
+        return good_teamsets
+
+    @cached_property
+    def teamsets_by_id(self):
+        return {teamset.teamset_id: teamset for teamset in self.teamsets}
+
+    @cached_property
+    def max_team_size(self):
+        """
+        The default maximum size for teams in this course.
+        """
+        return _clean_max_team_size(self._data.get('max_team_size'))
+
+    def calc_max_team_size(self, teamset_id):
+        """
+        Given a team-set's ID, return the maximum allowed size of teams within it.
+
+        For 'open' team-sets, first regards the team-set's `max_team_size`,
+        then falls back to the course's `max_team_size`.
+        For managed team-stes, `max_team_size` is ignored.
+
+        Return value of None should be regarded as "no maximum size" (TODO MST-33).
+        """
+        try:
+            teamset = self.teamsets_by_id[teamset_id]
+        except KeyError:
+            raise ValueError("Team-set {!r} does not exist.".format(teamset_id))
+        if teamset.teamset_type != TeamsetType.open:
+            return None
+        if teamset.max_team_size:
+            return teamset.max_team_size
+        return self.max_team_size
+
+
+class TeamsetConfig(object):
+    """
+    Configuration for a team-set within a course run.
+
+    Takes in a configuration from a JSON-friendly dictionary,
+    and exposes cleaned data from it.
+    """
+    teamset_id_regex = re.compile(r'^[A-Za-z0-9_-]+$')
+
+    def __init__(self, data):
+        """
+        Initialize a TeamsConfig object with a dictionary.
+        """
+        self._data = data if isinstance(data, dict) else {}
+
+    def __unicode__(self):
+        """
+        Return user-friendly string.
+
+        TODO move this code to __str__ after Py3 upgrade.
+        """
+        return self.name
+
+    def __str__(self):
+        """
+        Return user-friendly string.
+        """
+        return str(self.__unicode__())
+
+    def __repr__(self):
+        """
+        Return developer-helpful string.
+        """
+        attrs = ['teamset_id', 'name', 'description', 'max_team_size', 'teamset_type']
+        return "<{} {}>".format(
+            self.__class__.__name__,
+            " ".join(
+                attr + "=" + repr(getattr(self, attr))
+                for attr in attrs if hasattr(self, attr)
+            ),
+        )
+
+    @property
+    def source_data(self):
+        """
+        Dictionary containing the data from which this TeamsConfig was built.
+        """
+        return self._data
+
+    @cached_property
+    def cleaned_data(self):
+        """
+        JSON-friendly dictionary containing cleaned data from this TeamsConfig.
+        """
+        return {
+            'id': self.teamset_id,
+            'name': self.name,
+            'description': self.description,
+            'max_team_size': self.max_team_size,
+            'type': self.teamset_type.value,
+        }
+
+    @cached_property
+    def cleaned_data_old_format(self):
+        """
+        JSON-friendly dictionary containing cleaned data from this TeamsConfig,
+        excluding newly added fields.
+
+        Here for backwards compatibility; to be removed (TODO MST-18).
+        """
+        return {
+            'id': self.teamset_id,
+            'name': self.name,
+            'description': self.description,
+        }
+
+    @cached_property
+    def teamset_id(self):
+        """
+        An identifier for this team-set.
+
+        Should be a URL-slug friendly string.
+        """
+        teamset_id = _clean_string(self._data.get('id'))
+        if not self.teamset_id_regex.match(teamset_id):
+            return ""
+        return teamset_id
+
+    @cached_property
+    def name(self):
+        """
+        A human-friendly name of the team-set,
+        falling back to `teamset_id`.
+        """
+        return _clean_string(self._data.get('name')) or self.teamset_id
+
+    @cached_property
+    def description(self):
+        """
+        A brief description of the team-set,
+        falling back to empty string.
+        """
+        return _clean_string(self._data.get('description'))
+
+    @cached_property
+    def max_team_size(self):
+        """
+        Configured maximum team size override for this team-set,
+        falling back to None.
+        """
+        return _clean_max_team_size(self._data.get('max_team_size'))
+
+    @cached_property
+    def teamset_type(self):
+        """
+        Configured TeamsetType,
+        falling back to default TeamsetType.
+        """
+        try:
+            return TeamsetType(self._data['type'])
+        except (KeyError, ValueError):
+            return TeamsetType.get_default()
+
+
+class TeamsetType(Enum):
+    """
+    Management and privacy scheme for teams within a team-set.
+
+    "open" team-sets allow learners to freely join, leave, and create teams.
+
+    "public_managed" team-sets forbid learners from modifying teams' membership.
+    Instead, instructors manage membership (TODO MST-9).
+
+    "private_managed" is like public_managed, except for that team names,
+    team memberships, and team discussions are all private to the members
+    of the teams (TODO MST-10).
+    """
+    open = "open"
+    public_managed = "public_managed"
+    private_managed = "private_managed"
+
+    @classmethod
+    def get_default(cls):
+        """
+        Return default TeamsetType.
+        """
+        return cls.open
+
+
+def _clean_string(value):
+    """
+    Return `value` if it's a string, otherwise "".
+    """
+    if not isinstance(value, six.string_types):
+        return ""
+    return value
+
+
+def _clean_max_team_size(value):
+    """
+    Return `value` if it's a positive int, otherwise None.
+    """
+    if not isinstance(value, six.integer_types):
+        return None
+    if value < 0:
+        return None
+    return value

openedx/core/lib/tests/test_teams_config.py

+"""
+Tests for Course Teams configuration.
+"""
+from __future__ import absolute_import, unicode_literals
+
+import ddt
+import six
+from django.test import TestCase
+
+from ..teams_config import TeamsConfig, TeamsetConfig
+
+
+@ddt.ddt
+class TeamsConfigTests(TestCase):
+    """
+    Test cases for `TeamsConfig` functions.
+    """
+    @ddt.data(
+        None,
+        "not-a-dict",
+        {},
+        {"max_team_size": 5},
+        {"team_sets": []},
+        {"team_sets": "not-a-list"},
+        {"team_sets": ["not-a-dict"]},
+        {"topics": None, "random_key": 88},
+    )
+    def test_disabled_team_configs(self, data):
+        """
+        Test that configuration that doesn't specify any valid team-sets
+        is considered disabled.
+        """
+        teams_config = TeamsConfig(data)
+        assert not teams_config.is_enabled
+
+    INPUT_DATA_1 = {
+        "max_team_size": 5,
+        "topics": [
+            {
+                "id": "bananas",
+                "max_team_size": 10,
+                "type": "private_managed",
+            },
+            {
+                "id": "bokonism",
+                "name": "BOKONISM",
+                "description": "Busy busy busy",
+                "type": "open",
+                "max_team_size": 2,
+            },
+            {
+                # Clusters with duplicate IDs should be dropped.
+                "id": "bananas",
+                "name": "All about Bananas",
+                "description": "Not to be confused with bandanas",
+            },
+
+        ],
+    }
+
+    OUTPUT_DATA_1 = {
+        "max_team_size": 5,
+        "team_sets": [
+            {
+                "id": "bananas",
+                "name": "bananas",
+                "description": "",
+                "max_team_size": 10,
+                "type": "private_managed",
+            },
+            {
+                "id": "bokonism",
+                "name": "BOKONISM",
+                "description": "Busy busy busy",
+                "max_team_size": 2,
+                "type": "open",
+            },
+        ]
+    }
+
+    INPUT_DATA_2 = {
+        "team_sets": [
+            {
+                # Team-set should be dropped due to lack of ID.
+                "name": "Assignment about existence",
+            },
+            {
+                # Team-set should be dropped due to invalid ID.
+                "id": ["not", "a", "string"],
+                "name": "Assignment about strings",
+            },
+            {
+                # Team-set should be dropped due to invalid ID.
+                "id": "Not a slug.",
+                "name": "Assignment about slugs",
+            },
+            {
+                # All fields invalid except ID;
+                # Team-set will exist but have all fallbacks.
+                "id": "horses",
+                "name": {"assignment", "about", "horses"},
+                "description": object(),
+                "max_team_size": -1000,
+                "type": "matrix",
+                "extra_key": "Should be ignored",
+            },
+            [
+                # Team-set should be dropped because it's not a dict.
+                "this", "isn't", "a", "valid", "team-set"
+            ],
+        ],
+    }
+
+    OUTPUT_DATA_2 = {
+        "max_team_size": None,
+        "team_sets": [
+            {
+                "id": "horses",
+                "name": "horses",
+                "description": "",
+                "max_team_size": None,
+                "type": "open",
+            },
+        ],
+    }
+
+    @ddt.data(
+        (INPUT_DATA_1, OUTPUT_DATA_1),
+        (INPUT_DATA_2, OUTPUT_DATA_2),
+    )
+    @ddt.unpack
+    def test_teams_config_round_trip(self, input_data, expected_output_data):
+        """
+        Test that when we load some config data,
+        it is cleaned in the way we expect it to be.
+        """
+        teams_config = TeamsConfig(input_data)
+        actual_output_data = teams_config.cleaned_data
+        self.assertDictEqual(actual_output_data, expected_output_data)
+
+    @ddt.data(
+        (None, None, "open", None),
+        (None, None, "public_managed", None),
+        (None, 6666, "open", 6666),
+        (None, 6666, "public_managed", None),
+        (1812, None, "open", 1812),
+        (1812, None, "public_managed", None),
+        (1812, 6666, "open", 6666),
+        (1812, 6666, "public_managed", None),
+    )
+    @ddt.unpack
+    def test_calc_max_team_size(
+            self,
+            course_run_max_team_size,
+            teamset_max_team_size,
+            teamset_type,
+            expected_max_team_size,
+    ):
+        """
+        Test that a team set's max team size is calculated as expected.
+        """
+        teamset_data = {"id": "teamset-1", "name": "Team size testing team-set"}
+        teamset_data["max_team_size"] = teamset_max_team_size
+        teamset_data["type"] = teamset_type
+        config_data = {
+            "max_team_size": course_run_max_team_size,
+            "team_sets": [teamset_data],
+        }
+        config = TeamsConfig(config_data)
+        assert config.calc_max_team_size("teamset-1") == expected_max_team_size
+
+    def test_teams_config_string(self):
+        """
+        Assert that teams configs can be reasonably stringified.
+        """
+        config = TeamsConfig({})
+        assert six.text_type(config) == "Teams configuration for 0 team-sets"
+
+    def test_teamset_config_string(self):
+        """
+        Assert that team-set configs can be reasonably stringified.
+        """
+        config = TeamsetConfig({"id": "omlette-du-fromage"})
+        assert six.text_type(config) == "omlette-du-fromage"
+
+    def test_teams_config_repr(self):
+        """
+        Assert that the developer-friendly repr isn't broken.
+        """
+        config = TeamsConfig({"team_sets": [{"id": "hedgehogs"}], "max_team_size": 987})
+        config_repr = repr(config)
+        assert isinstance(config_repr, six.string_types)
+
+        # When repr() fails, it doesn't always throw an exception.
+        # Instead, it puts error messages in the repr.
+        assert 'Error' not in config_repr
+
+        # Instead of checking the specific string,
+        # just make sure important info is there.
+        assert 'TeamsetConfig' in config_repr
+        assert 'TeamsConfig' in config_repr
+        assert '987' in config_repr
+        assert 'open' in config_repr
+        assert 'hedgehogs' in config_repr