diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 73bc5e656a..bf23e31bab 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -271,6 +271,33 @@ def get_available_providers(): return available_providers +class TeamsConfigField(Dict): + """ + XBlock field for teams configuration, including definitions for teamsets. + + Serializes to JSON dictionary. + """ + _default = TeamsConfig({}) + + def from_json(self, value): + """ + Return a TeamsConfig instance from a dict. + """ + return TeamsConfig(value) + + def to_json(self, value): + """ + Convert a TeamsConfig instance back to a dict. + + If we have the data that was used to build the TeamsConfig instance, + return that instead of `value.cleaned_data`, thus preserving the + data in the form that the user entered it. + """ + if value.source_data is not None: + return value.source_data + return value.cleaned_data + + class CourseFields(object): lti_passports = List( display_name=_("LTI Passports"), @@ -797,7 +824,7 @@ class CourseFields(object): scope=Scope.settings ) - teams_configuration = Dict( + teams_configuration = TeamsConfigField( display_name=_("Teams Configuration"), # Translators: please don't translate "id". help=_( @@ -808,6 +835,8 @@ class CourseFields(object): 'For example, to specify that teams should have a maximum of 5 participants and provide a list of ' '2 topics, enter the configuration in this format: {example_format}. ' 'In "id" values, the only supported special characters are underscore, hyphen, and period.' + # Note that we also support space (" "), which may have been an accident, but it's in + # our DB now. Let's not advertise the fact, though. ), help_format_args=dict( # Put the sample JSON into a format variable so that translators @@ -1471,65 +1500,32 @@ 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. + Alias to `self.teams_configuration.is_enabled`, for convenience. - 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). + Returns bool. """ - if self.teams_configuration: - return len(self.teams_configuration.get('topics', [])) > 0 - return False + return self.teams_configuration.is_enabled # pylint: disable=no-member @property - def teams_max_size(self): + def teamsets(self): """ - Returns the max size for teams if teams has been configured, else None. + Alias to `self.teams_configuration.teamsets`, for convenience. - Deprecated; please use `self.teams_conf.calc_max_team_size(...)` instead. - Will be removed (TODO MST-18). + Returns list[TeamsetConfig]. """ - return self.teams_configuration.get('max_team_size', None) + return self.teams_configuration.teamsets # pylint: disable=no-member @property - def teams_topics(self): + def teamsets_by_id(self): """ - Returns the topics that have been configured for teams for this course, else None. + Alias to `self.teams_configuration.teamsets_by_id`, for convenience. - Deprecated; please use `self.teams_conf.teamsets` instead. - Will be removed (TODO MST-18). + Returns dict[str: TeamsetConfig]. """ - return self.teams_configuration.get('topics', None) + return self.teams_configuration.teamsets_by_id def set_user_partitions_for_scheme(self, partitions, scheme): """ diff --git a/common/lib/xmodule/xmodule/tests/test_course_module.py b/common/lib/xmodule/xmodule/tests/test_course_module.py index 0c9091f1b4..76d63be5ea 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_module.py +++ b/common/lib/xmodule/xmodule/tests/test_course_module.py @@ -15,6 +15,7 @@ from opaque_keys.edx.keys import CourseKey from pytz import utc from xblock.runtime import DictKeyValueStore, KvsFieldData +from openedx.core.lib.teams_config import TeamsConfig import xmodule.course_module from xmodule.modulestore.xml import ImportSystem, XMLModuleStore @@ -272,25 +273,21 @@ 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): super(TeamsConfigurationTestCase, self).setUp() self.course = get_dummy_course('2012-12-02T12:00') - self.course.teams_configuration = dict() + self.course.teams_configuration = TeamsConfig(None) self.count = itertools.count() def add_team_configuration(self, max_team_size=3, topics=None): """ Add a team configuration to the course. """ - teams_configuration = {} - teams_configuration["topics"] = [] if topics is None else topics + teams_config_data = {} + teams_config_data["topics"] = [] if topics is None else topics if max_team_size is not None: - teams_configuration["max_team_size"] = max_team_size - self.course.teams_configuration = teams_configuration + teams_config_data["max_team_size"] = max_team_size + self.course.teams_configuration = TeamsConfig(teams_config_data) def make_topic(self): """ Make a sample topic dictionary. """ @@ -303,66 +300,56 @@ 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) + self.assertIsNone(self.course.teams_configuration.default_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) + self.assertEqual(size, self.course.teams_configuration.default_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_teamsets_no_config(self): + self.assertEqual(self.course.teamsets, []) - def test_teams_topics_no_topics(self): + def test_teamsets_empty(self): self.add_team_configuration(max_team_size=4) - self.assertEqual(self.course.teams_topics, []) - self.assertEqual(self.course.teams_conf.teamsets, []) + self.assertEqual(self.course.teamsets, []) - def test_teams_topics_with_topics(self): + def test_teamsets_present(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 + for teamset in self.course.teamsets ] self.assertEqual(expected_teamsets_data, topics) - def test_teams_conf_caching(self): + def test_teams_conf_cached_by_xblock_field(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 + cold_cache_conf = self.course.teams_configuration + warm_cache_conf = self.course.teams_configuration 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 + new_cold_cache_conf = self.course.teams_configuration + new_warm_cache_conf = self.course.teams_configuration 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) diff --git a/common/test/acceptance/tests/lms/test_teams.py b/common/test/acceptance/tests/lms/test_teams.py index 29c9546765..f9ab1cdff9 100644 --- a/common/test/acceptance/tests/lms/test_teams.py +++ b/common/test/acceptance/tests/lms/test_teams.py @@ -103,15 +103,15 @@ class TeamsTabBase(EventsTestMixin, ForumsConfigMixin, UniqueCourseTest): ) return json.loads(response.text) - def set_team_configuration(self, configuration, enroll_in_course=True, global_staff=False): + def set_team_configuration(self, configuration_data, enroll_in_course=True, global_staff=False): """ Sets team configuration on the course and calls auto-auth on the user. """ #pylint: disable=attribute-defined-outside-init self.course_fixture = CourseFixture(**self.course_info) - if configuration: + if configuration_data: self.course_fixture.add_advanced_settings( - {u"teams_configuration": {u"value": configuration}} + {u"teams_configuration": {u"value": configuration_data}} ) self.course_fixture.install() @@ -534,7 +534,12 @@ class BrowseTopicsTest(TeamsTabBase): """ initial_description = "A" + " really" * 50 + " long description" self.set_team_configuration( - {u"max_team_size": 1, u"topics": [{"name": "", "id": "", "description": initial_description}]} + { + u"max_team_size": 1, + u"topics": [ + {"id": "long-description-topic", "description": initial_description}, + ] + } ) self.topics_page.visit() truncated_description = self.topics_page.topic_descriptions[0] diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index a22bb2c73f..a4f6737cea 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -47,6 +47,7 @@ from openedx.core.djangoapps.django_comment_common.utils import ( set_course_discussion_settings ) from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES +from openedx.core.lib.teams_config import TeamsConfig from student.roles import CourseStaffRole, UserBasedRole from student.tests.factories import CourseAccessRoleFactory, CourseEnrollmentFactory, UserFactory from track.middleware import TrackMiddleware @@ -1440,10 +1441,10 @@ class TeamsPermissionsTestCase(ForumsEnableMixin, UrlResetMixin, SharedModuleSto def setUpClass(cls): # pylint: disable=super-method-not-called with super(TeamsPermissionsTestCase, cls).setUpClassAndTestData(): - teams_configuration = { + teams_config_data = { 'topics': [{'id': "topic_id", 'name': 'Solar Power', 'description': 'Solar power is hot'}] } - cls.course = CourseFactory.create(teams_configuration=teams_configuration) + cls.course = CourseFactory.create(teams_configuration=TeamsConfig(teams_config_data)) @classmethod def setUpTestData(cls): diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 81f575ae34..4b54ed2e24 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -67,6 +67,7 @@ from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_COMM from openedx.core.djangoapps.django_comment_common.utils import seed_permissions_roles from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin +from openedx.core.lib.teams_config import TeamsConfig from openedx.core.lib.xblock_utils import grade_histogram from shoppingcart.models import ( Coupon, @@ -3033,9 +3034,9 @@ class TestInstructorAPILevelsDataDump(SharedModuleStoreTestCase, LoginEnrollment has teams enabled, and does not when the course does not have teams enabled """ if has_teams: - self.course = CourseFactory.create(teams_configuration={ - 'max_size': 2, 'topics': [{'topic-id': 'topic', 'name': 'Topic', 'description': 'A Topic'}] - }) + self.course = CourseFactory.create(teams_configuration=TeamsConfig({ + 'max_size': 2, 'topics': [{'id': 'topic', 'name': 'Topic', 'description': 'A Topic'}] + })) course_instructor = InstructorFactory(course_key=self.course.id) self.client.login(username=course_instructor.username, password='test') diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index a5b2106dc1..ea8bc75638 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -75,6 +75,7 @@ from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from openedx.core.djangoapps.credit.tests.factories import CreditCourseFactory from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme from openedx.core.djangoapps.util.testing import ContentGroupTestCase, TestConditionalContent +from openedx.core.lib.teams_config import TeamsConfig from shoppingcart.models import ( Coupon, CourseRegistrationCode, @@ -96,6 +97,12 @@ from ..models import ReportStore from ..tasks_helper.utils import UPDATE_STATUS_FAILED, UPDATE_STATUS_SUCCEEDED +_TEAMS_CONFIG = TeamsConfig({ + 'max_size': 2, + 'topics': [{'id': 'topic', 'name': 'Topic', 'description': 'A Topic'}], +}) + + class InstructorGradeReportTestCase(TestReportMixin, InstructorTaskCourseTestCase): """ Base class for grade report tests. """ @@ -403,9 +410,7 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase): course = CourseFactory.create( cohort_config={'cohorted': True, 'auto_cohort': True, 'auto_cohort_groups': ['cohort 1', 'cohort 2']}, user_partitions=[experiment_partition], - teams_configuration={ - 'max_size': 2, 'topics': [{'topic-id': 'topic', 'name': 'Topic', 'description': 'A Topic'}] - }, + teams_configuration=_TEAMS_CONFIG, ) _ = CreditCourseFactory(course_key=course.id) @@ -451,9 +456,7 @@ class TestTeamGradeReport(InstructorGradeReportTestCase): def setUp(self): super(TestTeamGradeReport, self).setUp() - self.course = CourseFactory.create(teams_configuration={ - 'max_size': 2, 'topics': [{'topic-id': 'topic', 'name': 'Topic', 'description': 'A Topic'}] - }) + self.course = CourseFactory.create(teams_configuration=_TEAMS_CONFIG) self.student1 = UserFactory.create() CourseEnrollment.enroll(self.student1, self.course.id) self.student2 = UserFactory.create() @@ -1547,9 +1550,7 @@ class TestTeamStudentReport(TestReportMixin, InstructorTaskCourseTestCase): def setUp(self): super(TestTeamStudentReport, self).setUp() - self.course = CourseFactory.create(teams_configuration={ - 'max_size': 2, 'topics': [{'topic-id': 'topic', 'name': 'Topic', 'description': 'A Topic'}] - }) + self.course = CourseFactory.create(teams_configuration=_TEAMS_CONFIG) self.student1 = UserFactory.create() CourseEnrollment.enroll(self.student1, self.course.id) self.student2 = UserFactory.create() diff --git a/lms/djangoapps/teams/templates/teams/teams.html b/lms/djangoapps/teams/templates/teams/teams.html index 5b0a885a75..14278bc042 100644 --- a/lms/djangoapps/teams/templates/teams/teams.html +++ b/lms/djangoapps/teams/templates/teams/teams.html @@ -51,7 +51,7 @@ from openedx.core.djangolib.js_utils import ( teamMembershipsUrl: '${team_memberships_url | n, js_escaped_string}', teamMembershipDetailUrl: '${team_membership_detail_url | n, js_escaped_string}', myTeamsUrl: '${my_teams_url | n, js_escaped_string}', - maxTeamSize: ${course.teams_max_size | n, dump_js_escaped_json}, + maxTeamSize: ${course.teams_configuration.default_max_team_size | n, dump_js_escaped_json}, languages: ${languages | n, dump_js_escaped_json}, countries: ${countries | n, dump_js_escaped_json}, teamsBaseUrl: '${teams_base_url | n, js_escaped_string}' diff --git a/lms/djangoapps/teams/tests/test_serializers.py b/lms/djangoapps/teams/tests/test_serializers.py index 3a965bdf4d..27ee6445ef 100644 --- a/lms/djangoapps/teams/tests/test_serializers.py +++ b/lms/djangoapps/teams/tests/test_serializers.py @@ -10,6 +10,7 @@ from django.test.client import RequestFactory from lms.djangoapps.teams.serializers import BulkTeamCountTopicSerializer, MembershipSerializer, TopicSerializer from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory +from openedx.core.lib.teams_config import TeamsConfig from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -25,10 +26,10 @@ class SerializerTestCase(SharedModuleStoreTestCase): """ super(SerializerTestCase, self).setUp() self.course = CourseFactory.create( - teams_configuration={ + teams_configuration=TeamsConfig({ "max_team_size": 10, "topics": [{u'name': u'Tøpic', u'description': u'The bést topic!', u'id': u'0'}] - } + }), ) @@ -41,7 +42,7 @@ class MembershipSerializerTestCase(SerializerTestCase): super(MembershipSerializerTestCase, self).setUp() self.team = CourseTeamFactory.create( course_id=self.course.id, - topic_id=self.course.teams_topics[0]['id'] + topic_id=self.course.teamsets[0].teamset_id, ) self.user = UserFactory.create() CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) @@ -81,7 +82,10 @@ class TopicSerializerTestCase(SerializerTestCase): team count of 0, and that it only takes one SQL query. """ with self.assertNumQueries(1): - serializer = TopicSerializer(self.course.teams_topics[0], context={'course_id': self.course.id}) + serializer = TopicSerializer( + self.course.teamsets[0].cleaned_data_old_format, + context={'course_id': self.course.id}, + ) self.assertEqual( serializer.data, {u'name': u'Tøpic', u'description': u'The bést topic!', u'id': u'0', u'team_count': 0} @@ -92,9 +96,14 @@ class TopicSerializerTestCase(SerializerTestCase): Verifies that the `TopicSerializer` correctly displays a topic with a positive team count, and that it only takes one SQL query. """ - CourseTeamFactory.create(course_id=self.course.id, topic_id=self.course.teams_topics[0]['id']) + CourseTeamFactory.create( + course_id=self.course.id, topic_id=self.course.teamsets[0].teamset_id + ) with self.assertNumQueries(1): - serializer = TopicSerializer(self.course.teams_topics[0], context={'course_id': self.course.id}) + serializer = TopicSerializer( + self.course.teamsets[0].cleaned_data_old_format, + context={'course_id': self.course.id}, + ) self.assertEqual( serializer.data, {u'name': u'Tøpic', u'description': u'The bést topic!', u'id': u'0', u'team_count': 1} @@ -102,17 +111,20 @@ class TopicSerializerTestCase(SerializerTestCase): def test_scoped_within_course(self): """Verify that team count is scoped within a course.""" - duplicate_topic = self.course.teams_topics[0] + duplicate_topic = self.course.teamsets[0].cleaned_data_old_format second_course = CourseFactory.create( - teams_configuration={ + teams_configuration=TeamsConfig({ "max_team_size": 10, "topics": [duplicate_topic] - } + }), ) CourseTeamFactory.create(course_id=self.course.id, topic_id=duplicate_topic[u'id']) CourseTeamFactory.create(course_id=second_course.id, topic_id=duplicate_topic[u'id']) with self.assertNumQueries(1): - serializer = TopicSerializer(self.course.teams_topics[0], context={'course_id': self.course.id}) + serializer = TopicSerializer( + self.course.teamsets[0].cleaned_data_old_format, + context={'course_id': self.course.id}, + ) self.assertEqual( serializer.data, {u'name': u'Tøpic', u'description': u'The bést topic!', u'id': u'0', u'team_count': 1} @@ -140,16 +152,17 @@ class BaseTopicSerializerTestCase(SerializerTestCase): Helper method to set up topics on the course. Returns a list of created topics. """ - self.course.teams_configuration['topics'] = [] topics = [ {u'name': u'Tøpic {}'.format(i), u'description': u'The bést topic! {}'.format(i), u'id': six.text_type(i)} for i in six.moves.range(num_topics) ] - for i in six.moves.range(num_topics): - topic_id = six.text_type(i) - self.course.teams_configuration['topics'].append(topics[i]) + for topic in topics: for _ in six.moves.range(teams_per_topic): - CourseTeamFactory.create(course_id=self.course.id, topic_id=topic_id) + CourseTeamFactory.create(course_id=self.course.id, topic_id=topic['id']) + self.course.teams_configuration = TeamsConfig({ + 'max_team_size': self.course.teams_configuration.default_max_team_size, + 'topics': topics, + }) return topics def assert_serializer_output(self, topics, num_teams_per_topic, num_queries): @@ -157,7 +170,10 @@ class BaseTopicSerializerTestCase(SerializerTestCase): Verify that the serializer produced the expected topics. """ with self.assertNumQueries(num_queries): - page = Paginator(self.course.teams_topics, self.PAGE_SIZE).page(1) + page = Paginator( + self.course.teams_configuration.cleaned_data_old_format['topics'], + self.PAGE_SIZE, + ).page(1) # pylint: disable=not-callable serializer = self.serializer(instance=page, context={'course_id': self.course.id}) self.assertEqual( @@ -170,7 +186,7 @@ class BaseTopicSerializerTestCase(SerializerTestCase): Verify that we return no results and make no SQL queries for a page with no topics. """ - self.course.teams_configuration['topics'] = [] + self.course.teams_configuration = TeamsConfig({'topics': []}) self.assert_serializer_output([], num_teams_per_topic=0, num_queries=0) @@ -216,10 +232,10 @@ class BulkTeamCountTopicSerializerTestCase(BaseTopicSerializerTestCase): first_course_topics = self.setup_topics(num_topics=self.NUM_TOPICS, teams_per_topic=teams_per_topic) duplicate_topic = first_course_topics[0] second_course = CourseFactory.create( - teams_configuration={ + teams_configuration=TeamsConfig({ "max_team_size": 10, "topics": [duplicate_topic] - } + }), ) CourseTeamFactory.create(course_id=second_course.id, topic_id=duplicate_topic[u'id']) self.assert_serializer_output(first_course_topics, num_teams_per_topic=teams_per_topic, num_queries=1) @@ -230,23 +246,6 @@ class BulkTeamCountTopicSerializerTestCase(BaseTopicSerializerTestCase): result.update(second) return result - def setup_topics(self, num_topics=5, teams_per_topic=0): - """ - Helper method to set up topics on the course. Returns a list of - created topics. - """ - self.course.teams_configuration['topics'] = [] - topics = [ - {u'name': u'Tøpic {}'.format(i), u'description': u'The bést topic! {}'.format(i), u'id': six.text_type(i)} - for i in six.moves.range(num_topics) - ] - for i in six.moves.range(num_topics): - topic_id = six.text_type(i) - self.course.teams_configuration['topics'].append(topics[i]) - for _ in six.moves.range(teams_per_topic): - CourseTeamFactory.create(course_id=self.course.id, topic_id=topic_id) - return topics - def assert_serializer_output(self, topics, num_teams_per_topic, num_queries): """ Verify that the serializer produced the expected topics. @@ -263,5 +262,5 @@ class BulkTeamCountTopicSerializerTestCase(BaseTopicSerializerTestCase): Verify that we return no results and make no SQL queries for a page with no topics. """ - self.course.teams_configuration['topics'] = [] + self.course.teams_configuration = TeamsConfig({'topics': []}) self.assert_serializer_output([], num_teams_per_topic=0, num_queries=0) diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 4d4d938d7e..5d6ec0300f 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -25,6 +25,7 @@ from common.test.utils import skip_signal from lms.djangoapps.courseware.tests.factories import StaffFactory from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_COMMUNITY_TA, Role from openedx.core.djangoapps.django_comment_common.utils import seed_permissions_roles +from openedx.core.lib.teams_config import TeamsConfig from student.models import CourseEnrollment from student.tests.factories import AdminFactory, CourseEnrollmentFactory, UserFactory from util.testing import EventTestMixin @@ -46,7 +47,7 @@ class TestDashboard(SharedModuleStoreTestCase): def setUpClass(cls): super(TestDashboard, cls).setUpClass() cls.course = CourseFactory.create( - teams_configuration={ + teams_configuration=TeamsConfig({ "max_team_size": 10, "topics": [ { @@ -56,7 +57,7 @@ class TestDashboard(SharedModuleStoreTestCase): } for topic_id in range(cls.NUM_TOPICS) ] - } + }) ) def setUp(self): @@ -158,7 +159,7 @@ class TestDashboard(SharedModuleStoreTestCase): # Create a course two course_two = CourseFactory.create( - teams_configuration={ + teams_configuration=TeamsConfig({ "max_team_size": 1, "topics": [ { @@ -167,7 +168,7 @@ class TestDashboard(SharedModuleStoreTestCase): "description": "Description for test topic for course two." } ] - } + }) ) # Login and enroll user in both course course @@ -204,7 +205,7 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): def setUpClass(cls): # pylint: disable=super-method-not-called with super(TeamAPITestCase, cls).setUpClassAndTestData(): - teams_configuration_1 = { + teams_configuration_1 = TeamsConfig({ 'topics': [ { @@ -213,7 +214,7 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): 'description': u'Description for topic {}.'.format(i) } for i, name in enumerate([u'Sólar power', 'Wind Power', 'Nuclear Power', 'Coal Power']) ] - } + }) cls.test_course_1 = CourseFactory.create( org='TestX', course='TS101', @@ -221,7 +222,7 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): teams_configuration=teams_configuration_1 ) - teams_configuration_2 = { + teams_configuration_2 = TeamsConfig({ 'topics': [ { @@ -241,7 +242,7 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): }, ], 'max_team_size': 1 - } + }) cls.test_course_2 = CourseFactory.create( org='MIT', course='6.002x', diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index b6f270d825..4c329a5d05 100644 --- a/lms/djangoapps/teams/views.py +++ b/lms/djangoapps/teams/views.py @@ -404,7 +404,7 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): result_filter.update({'membership__user__username': username}) topic_id = request.query_params.get('topic_id', None) if topic_id is not None: - if topic_id not in [topic['id'] for topic in course_module.teams_configuration['topics']]: + if topic_id not in course_module.teamsets_by_id: error = build_api_error( ugettext_noop(u'The supplied topic id {topic_id} is not valid'), topic_id=topic_id @@ -824,7 +824,10 @@ def get_alphabetical_topics(course_module): Returns: list: a list of sorted team topics """ - return sorted(course_module.teams_topics, key=lambda t: t['name'].lower()) + return sorted( + course_module.teams_configuration.cleaned_data_old_format['topics'], + key=lambda t: t['name'].lower(), + ) class TopicDetailView(APIView): @@ -884,12 +887,14 @@ class TopicDetailView(APIView): if not has_team_api_access(request.user, course_id): return Response(status=status.HTTP_403_FORBIDDEN) - topics = [t for t in course_module.teams_topics if t['id'] == topic_id] - - if not topics: + try: + topic = course_module.teamsets_by_id[topic_id] + except KeyError: return Response(status=status.HTTP_404_NOT_FOUND) - serializer = TopicSerializer(topics[0], context={'course_id': course_id}) + serializer = TopicSerializer( + topic.cleaned_data_old_format, context={'course_id': course_id} + ) return Response(serializer.data) @@ -1112,7 +1117,9 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): return Response(status=status.HTTP_404_NOT_FOUND) course_module = modulestore().get_course(team.course_id) - if course_module.teams_max_size is not None and team.users.count() >= course_module.teams_max_size: + # This should use `calc_max_team_size` instead of `default_max_team_size` (TODO MST-32). + max_team_size = course_module.teams_configuration.default_max_team_size + if max_team_size is not None and team.users.count() >= max_team_size: return Response( build_api_error(ugettext_noop("This team is already full.")), status=status.HTTP_400_BAD_REQUEST diff --git a/openedx/core/lib/teams_config.py b/openedx/core/lib/teams_config.py index 7f0b9a816f..4257153701 100644 --- a/openedx/core/lib/teams_config.py +++ b/openedx/core/lib/teams_config.py @@ -41,12 +41,25 @@ class TeamsConfig(object): """ Return developer-helpful string. """ - return "<{} max_team_size={} teamsets=[{}]>".format( + return "<{} default_max_team_size={} teamsets=[{}]>".format( self.__class__.__name__, - self.max_team_size, + self.default_max_team_size, ", ".join(repr(teamset) for teamset in self.teamsets), ) + def __eq__(self, other): + """ + Define equality based on cleaned data. + """ + return isinstance(other, self.__class__) and self.cleaned_data == other.cleaned_data + + def __ne__(self, other): + """ + Overrides default inequality to be the inverse of our custom equality. + Safe to remove once we're in Python 3 -- Py3 does this for us. + """ + return not self.__eq__(other) + @property def source_data(self): """ @@ -60,7 +73,7 @@ class TeamsConfig(object): JSON-friendly dictionary containing cleaned data from this TeamsConfig. """ return { - 'max_team_size': self.max_team_size, + 'max_team_size': self.default_max_team_size, 'team_sets': [ teamset.cleaned_data for teamset in self.teamsets ] @@ -72,10 +85,10 @@ class TeamsConfig(object): JSON-friendly dictionary containing cleaned data from this TeamsConfig, excluding newly added fields. - Here for backwards compatibility; to be removed (TODO MST-18). + Here for backwards compatibility; to be removed (TODO MST-40). """ return { - 'max_team_size': self.max_team_size, + 'max_team_size': self.default_max_team_size, 'topics': [ teamset.cleaned_data_old_format for teamset in self.teamsets ] @@ -123,9 +136,11 @@ class TeamsConfig(object): return {teamset.teamset_id: teamset for teamset in self.teamsets} @cached_property - def max_team_size(self): + def default_max_team_size(self): """ The default maximum size for teams in this course. + + Can be overriden by individual team sets; see `calc_max_team_size`. """ return _clean_max_team_size(self._data.get('max_team_size')) @@ -147,7 +162,7 @@ class TeamsConfig(object): return None if teamset.max_team_size: return teamset.max_team_size - return self.max_team_size + return self.default_max_team_size class TeamsetConfig(object): @@ -157,7 +172,7 @@ class TeamsetConfig(object): 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_-]+$') + teamset_id_regex = re.compile(r'^[A-Za-z0-9_. -]+$') def __init__(self, data): """ @@ -192,6 +207,19 @@ class TeamsetConfig(object): ), ) + def __eq__(self, other): + """ + Define equality based on cleaned data. + """ + return isinstance(other, self.__class__) and self.cleaned_data == other.cleaned_data + + def __ne__(self, other): + """ + Overrides default inequality to be the inverse of our custom equality. + Safe to remove once we're in Python 3 -- Py3 does this for us. + """ + return not self.__eq__(other) + @property def source_data(self): """ @@ -218,7 +246,7 @@ class TeamsetConfig(object): JSON-friendly dictionary containing cleaned data from this TeamsConfig, excluding newly added fields. - Here for backwards compatibility; to be removed (TODO MST-18). + Here for backwards compatibility; to be removed (TODO MST-40). """ return { 'id': self.teamset_id, @@ -231,7 +259,8 @@ class TeamsetConfig(object): """ An identifier for this team-set. - Should be a URL-slug friendly string. + Should be a URL-slug friendly string, + but for a historical reasons may contain periods and spaces. """ teamset_id = _clean_string(self._data.get('id')) if not self.teamset_id_regex.match(teamset_id): @@ -301,11 +330,11 @@ class TeamsetType(Enum): def _clean_string(value): """ - Return `value` if it's a string, otherwise "". + Return `str(value)` if it's a string or int, otherwise "". """ - if not isinstance(value, six.string_types): - return "" - return value + if isinstance(value, six.integer_types + six.string_types): + return six.text_type(value) + return "" def _clean_max_team_size(value): diff --git a/openedx/core/lib/tests/test_teams_config.py b/openedx/core/lib/tests/test_teams_config.py index 8d27f9b5e1..70ce37b5c5 100644 --- a/openedx/core/lib/tests/test_teams_config.py +++ b/openedx/core/lib/tests/test_teams_config.py @@ -91,14 +91,14 @@ class TeamsConfigTests(TestCase): }, { # Team-set should be dropped due to invalid ID. - "id": "Not a slug.", - "name": "Assignment about slugs", + "id": "the character & cannot be in an ID", + "name": "Assignment about ampersands", }, { # All fields invalid except ID; # Team-set will exist but have all fallbacks. - "id": "horses", - "name": {"assignment", "about", "horses"}, + "id": "_1. How quickly daft-jumping zebras vex", + "name": {"assignment", "about", "zebras"}, "description": object(), "max_team_size": -1000, "type": "matrix", @@ -115,8 +115,8 @@ class TeamsConfigTests(TestCase): "max_team_size": None, "team_sets": [ { - "id": "horses", - "name": "horses", + "id": "_1. How quickly daft-jumping zebras vex", + "name": "_1. How quickly daft-jumping zebras vex", "description": "", "max_team_size": None, "type": "open", @@ -202,3 +202,11 @@ class TeamsConfigTests(TestCase): assert '987' in config_repr assert 'open' in config_repr assert 'hedgehogs' in config_repr + + def test_teamset_int_id(self): + """ + Assert integer teaset IDs are treated as strings, + for backwards compatibility. + """ + teamset = TeamsetConfig({"id": 5}) + assert teamset.teamset_id == "5"