diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 23e455fa92..73bc5e656a 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/tests/test_course_module.py b/common/lib/xmodule/xmodule/tests/test_course_module.py index 3bbc1dfd9f..0c9091f1b4 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_module.py +++ b/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): diff --git a/openedx/core/lib/teams_config.py b/openedx/core/lib/teams_config.py new file mode 100644 index 0000000000..7f0b9a816f --- /dev/null +++ b/openedx/core/lib/teams_config.py @@ -0,0 +1,319 @@ +""" +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 diff --git a/openedx/core/lib/tests/test_teams_config.py b/openedx/core/lib/tests/test_teams_config.py new file mode 100644 index 0000000000..8d27f9b5e1 --- /dev/null +++ b/openedx/core/lib/tests/test_teams_config.py @@ -0,0 +1,204 @@ +""" +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