diff --git a/lms/djangoapps/teams/api.py b/lms/djangoapps/teams/api.py index 83e575b20e..4f0a3a5c1f 100644 --- a/lms/djangoapps/teams/api.py +++ b/lms/djangoapps/teams/api.py @@ -12,7 +12,7 @@ from opaque_keys.edx.keys import CourseKey from course_modes.models import CourseMode from lms.djangoapps.discussion.django_comment_client.utils import has_discussion_privileges -from lms.djangoapps.teams.models import CourseTeam +from lms.djangoapps.teams.models import CourseTeam, CourseTeamMembership from openedx.core.lib.teams_config import TeamsetType from student.models import CourseEnrollment, anonymous_id_for_user from student.roles import CourseInstructorRole, CourseStaffRole @@ -181,6 +181,34 @@ def user_organization_protection_status(user, course_key): def has_specific_team_access(user, team): + """ + To have access to a team a user must: + - Be course staff + OR + - be in the correct bubble + - be in the team if it is private + """ + return has_course_staff_privileges(user, team.course_id) or ( + user_protection_status_matches_team(user, team) and user_on_team_or_team_is_public(user, team) + ) + + +def user_on_team_or_team_is_public(user, team): + """ + The only users who should be able to see private_managed teams + or recieve any information about them at all from the API are: + - Course staff + - Users who are enrolled in a team in a private_managed teamset + * They should only be able to see their own team, no other teams. + """ + if CourseTeamMembership.is_user_on_team(user, team): + return True + course_module = modulestore().get_course(team.course_id) + teamset = course_module.teams_configuration.teamsets_by_id[team.topic_id] + return teamset.teamset_type != TeamsetType.private_managed + + +def user_protection_status_matches_team(user, team): """ Check whether the user have access to the specific team. The user can be of a different organization protection bubble with the team in question. diff --git a/lms/djangoapps/teams/models.py b/lms/djangoapps/teams/models.py index 65ab899f8e..f8e6b7642b 100644 --- a/lms/djangoapps/teams/models.py +++ b/lms/djangoapps/teams/models.py @@ -205,13 +205,13 @@ class CourseTeam(models.Model): def add_user(self, user): """Adds the given user to the CourseTeam.""" - from lms.djangoapps.teams.api import has_specific_team_access + from lms.djangoapps.teams.api import user_protection_status_matches_team if not CourseEnrollment.is_enrolled(user, self.course_id): raise NotEnrolledInCourseForTeam if CourseTeamMembership.user_in_team_for_course(user, self.course_id, self.topic_id): raise AlreadyOnTeamInCourse - if not has_specific_team_access(user, self): + if not user_protection_status_matches_team(user, self): raise AddToIncompatibleTeamError return CourseTeamMembership.objects.create( user=user, diff --git a/lms/djangoapps/teams/tests/test_api.py b/lms/djangoapps/teams/tests/test_api.py index 6fb7a3bf35..9baa4aaccf 100644 --- a/lms/djangoapps/teams/tests/test_api.py +++ b/lms/djangoapps/teams/tests/test_api.py @@ -235,13 +235,22 @@ class TeamAccessTests(SharedModuleStoreTestCase): 'user_unenrolled': cls.user_unenrolled, } + cls.topic_id = 'RANDOM TOPIC' + cls.course_1 = CourseFactory.create( + teams_configuration=TeamsConfig({ + 'team_sets': [{'id': cls.topic_id, 'name': cls.topic_id, 'description': cls.topic_id}] + }), + org=COURSE_KEY1.org, + course=COURSE_KEY1.course, + run=COURSE_KEY1.run + ) + for user in (cls.user_audit, cls.user_staff): CourseEnrollmentFactory.create(user=user, course_id=COURSE_KEY1) CourseEnrollmentFactory.create(user=cls.user_masters, course_id=COURSE_KEY1, mode=CourseMode.MASTERS) CourseStaffRole(COURSE_KEY1).add_users(cls.user_staff) - cls.topic_id = 'RANDOM TOPIC' cls.team_unprotected_1 = CourseTeamFactory( course_id=COURSE_KEY1, topic_id=cls.topic_id, diff --git a/lms/djangoapps/teams/tests/test_models.py b/lms/djangoapps/teams/tests/test_models.py index 8c31a8b624..f2774ce08d 100644 --- a/lms/djangoapps/teams/tests/test_models.py +++ b/lms/djangoapps/teams/tests/test_models.py @@ -30,14 +30,32 @@ from openedx.core.djangoapps.django_comment_common.signals import ( thread_edited, thread_voted ) +from openedx.core.lib.teams_config import TeamsConfig from student.tests.factories import CourseEnrollmentFactory, UserFactory from util.testing import EventTestMixin + from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory COURSE_KEY1 = CourseKey.from_string('edx/history/1') -COURSE_KEY2 = CourseKey.from_string('edx/history/2') +COURSE_KEY2 = CourseKey.from_string('edx/math/1') TEAMSET_1_ID = "the-teamset" TEAMSET_2_ID = "the-teamset-2" +TEAMS_CONFIG_1 = TeamsConfig({ + 'team_sets': [{'id': TEAMSET_1_ID, 'name': 'Teamset1Name', 'description': 'Teamset1Desc'}] +}) +TEAMS_CONFIG_2 = TeamsConfig({ + 'team_sets': [{'id': TEAMSET_2_ID, 'name': 'Teamset2Name', 'description': 'Teamset2Desc'}] +}) + + +def create_course(course_key, teams_config): + return CourseFactory.create( + teams_configuration=teams_config, + org=course_key.org, + course=course_key.course, + run=course_key.run + ) class TestModelStrings(SharedModuleStoreTestCase): @@ -47,10 +65,12 @@ class TestModelStrings(SharedModuleStoreTestCase): @classmethod def setUpClass(cls): super(TestModelStrings, cls).setUpClass() + cls.course_id = "edx/the-course/1" + cls.course1 = create_course(CourseKey.from_string(cls.course_id), TEAMS_CONFIG_1) cls.user = UserFactory.create(username="the-user") - CourseEnrollmentFactory.create(user=cls.user, course_id="edx/the-course/1") + CourseEnrollmentFactory.create(user=cls.user, course_id=cls.course_id) cls.team = CourseTeamFactory( - course_id="edx/the-course/1", + course_id=cls.course_id, team_id="the-team", topic_id=TEAMSET_1_ID, name="The Team" @@ -90,6 +110,8 @@ class CourseTeamTest(SharedModuleStoreTestCase): @classmethod def setUpClass(cls): super(CourseTeamTest, cls).setUpClass() + cls.course_id = "edx/the-course/1" + cls.course1 = create_course(CourseKey.from_string(cls.course_id), TEAMS_CONFIG_1) cls.audit_learner = UserFactory.create(username="audit") CourseEnrollmentFactory.create(user=cls.audit_learner, course_id="edx/the-course/1", mode=CourseMode.AUDIT) @@ -129,6 +151,12 @@ class CourseTeamTest(SharedModuleStoreTestCase): class TeamMembershipTest(SharedModuleStoreTestCase): """Tests for the TeamMembership model.""" + @classmethod + def setUpClass(cls): + super(TeamMembershipTest, cls).setUpClass() + create_course(COURSE_KEY1, TEAMS_CONFIG_1) + create_course(COURSE_KEY2, TEAMS_CONFIG_2) + def setUp(self): """ Set up tests. diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 5a4716034b..be330b9ba5 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -378,9 +378,21 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): course_id=cls.test_course_1.id, topic_id='topic_0' ) - cls.wind_team = CourseTeamFactory.create(name='Wind Team', course_id=cls.test_course_1.id) - cls.nuclear_team = CourseTeamFactory.create(name='Nuclear Team', course_id=cls.test_course_1.id) - cls.another_team = CourseTeamFactory.create(name='Another Team', course_id=cls.test_course_2.id) + cls.wind_team = CourseTeamFactory.create( + name='Wind Team', + course_id=cls.test_course_1.id, + topic_id='topic_1' + ) + cls.nuclear_team = CourseTeamFactory.create( + name='Nuclear Team', + course_id=cls.test_course_1.id, + topic_id='topic_2' + ) + cls.another_team = CourseTeamFactory.create( + name='Another Team', + course_id=cls.test_course_2.id, + topic_id='topic_5' + ) cls.public_profile_team = CourseTeamFactory.create( name='Public Profile Team', course_id=cls.test_course_2.id, @@ -1263,10 +1275,10 @@ class TestListTopicsAPI(TeamAPITestCase): u'Sólar power', 'Wind Power'], 'name'), ('name', 200, ['Coal Power', 'Nuclear Power', 'private_topic_1_name', 'private_topic_2_name', u'Sólar power', 'Wind Power'], 'name'), - # Note that "Nuclear Power" and "Solar power" both have 2 teams. "Coal Power" and "Window Power" - # both have 0 teams. The secondary sort is alphabetical by name. - ('team_count', 200, ['Nuclear Power', u'Sólar power', 'Coal Power', 'private_topic_1_name', - 'private_topic_2_name', 'Wind Power'], 'team_count'), + # Note that "Nuclear Power" will have 2 teams. "Coal Power" "Wind Power" and "Solar Power" + # all have 1 team. The secondary sort is alphabetical by name. + ('team_count', 200, ['Nuclear Power', 'Coal Power', u'Sólar power', 'Wind Power', + 'private_topic_1_name', 'private_topic_2_name'], 'team_count'), ('no_such_field', 400, [], None), ) @ddt.unpack @@ -1277,12 +1289,13 @@ class TestListTopicsAPI(TeamAPITestCase): sender=CourseTeam, dispatch_uid='teams.signals.course_team_post_save_callback' ): - # Add 2 teams to "Nuclear Power", which previously had no teams. + # Add a team to "Nuclear Power", so it has two teams CourseTeamFactory.create( name=u'Nuclear Team 1', course_id=self.test_course_1.id, topic_id='topic_2' ) + # Add a team to "Coal Power", so it has one team, same as "Wind" and "Solar" CourseTeamFactory.create( - name=u'Nuclear Team 2', course_id=self.test_course_1.id, topic_id='topic_2' + name=u'Coal Team 1', course_id=self.test_course_1.id, topic_id='topic_3' ) data = {'course_id': str(self.test_course_1.id)} if field: @@ -1297,20 +1310,29 @@ class TestListTopicsAPI(TeamAPITestCase): Ensure that the secondary sort (alphabetical) when primary sort is team_count works across pagination boundaries. """ + # All teams have one teamset, except for Coal Power, topic_3 with skip_signal( post_save, receiver=course_team_post_save_callback, sender=CourseTeam, dispatch_uid='teams.signals.course_team_post_save_callback' ): - # Add 2 teams to "Wind Power", which previously had no teams. + # Add two wind teams, a solar team and a coal team, to bring the totals to + # Wind: 3 Solar: 2 Coal: 1, Nuclear: 1 CourseTeamFactory.create( name=u'Wind Team 1', course_id=self.test_course_1.id, topic_id='topic_1' ) CourseTeamFactory.create( name=u'Wind Team 2', course_id=self.test_course_1.id, topic_id='topic_1' ) + CourseTeamFactory.create( + name=u'Solar Team 1', course_id=self.test_course_1.id, topic_id='topic_0' + ) + CourseTeamFactory.create( + name=u'Coal Team 1', course_id=self.test_course_1.id, topic_id='topic_3' + ) + # Wind power has the most teams, followed by Solar topics = self.get_topics_list(data={ 'course_id': str(self.test_course_1.id), 'page_size': 2, @@ -1319,6 +1341,7 @@ class TestListTopicsAPI(TeamAPITestCase): }) self.assertEqual(["Wind Power", u'Sólar power'], [topic['name'] for topic in topics['results']]) + # Coal and Nuclear are tied, so they are alphabetically sorted. topics = self.get_topics_list(data={ 'course_id': str(self.test_course_1.id), 'page_size': 2, @@ -1348,7 +1371,7 @@ class TestListTopicsAPI(TeamAPITestCase): response = self.get_topics_list(data={'course_id': str(self.test_course_1.id)}) for topic in response['results']: self.assertIn('team_count', topic) - if topic['id'] == u'topic_0': + if topic['id'] in ('topic_0', 'topic_1', 'topic_2'): self.assertEqual(topic['team_count'], 1) else: self.assertEqual(topic['team_count'], 0) @@ -1389,6 +1412,10 @@ class TestDetailTopicAPI(TeamAPITestCase): topic = self.get_topic_detail(topic_id='topic_0', course_id=self.test_course_1.id) self.assertEqual(topic['team_count'], 1) topic = self.get_topic_detail(topic_id='topic_1', course_id=self.test_course_1.id) + self.assertEqual(topic['team_count'], 1) + topic = self.get_topic_detail(topic_id='topic_2', course_id=self.test_course_1.id) + self.assertEqual(topic['team_count'], 1) + topic = self.get_topic_detail(topic_id='topic_3', course_id=self.test_course_1.id) self.assertEqual(topic['team_count'], 0)