From b2f69fc803cf6ac814205f0e020cde7fd83aefa4 Mon Sep 17 00:00:00 2001 From: Jansen Kantor Date: Tue, 14 Jan 2020 16:06:27 -0500 Subject: [PATCH] EDUCATOR-4858: Update teams service to consider teamset/topic (#22834) * update teams service and api to consider topic --- lms/djangoapps/teams/api.py | 32 ++++++-- lms/djangoapps/teams/services.py | 4 +- lms/djangoapps/teams/tests/test_api.py | 84 +++++++++++++++------ lms/djangoapps/teams/tests/test_services.py | 15 +++- 4 files changed, 99 insertions(+), 36 deletions(-) diff --git a/lms/djangoapps/teams/api.py b/lms/djangoapps/teams/api.py index 109bb15d89..f40baeab23 100644 --- a/lms/djangoapps/teams/api.py +++ b/lms/djangoapps/teams/api.py @@ -245,11 +245,11 @@ def can_user_create_team_in_topic(user, course_id, topic_id): ) -def get_team_for_user_and_course(user, course_id): +def get_team_for_user_course_topic(user, course_id, topic_id): """ - Returns the team that the given user is on in the course, or None + Returns the matching CourseTeam for the given user, course, and topic - If course_id does not exist, a ValueError is raised + If course_id is invalid, a ValueError is raised """ try: course_key = CourseKey.from_string(course_id) @@ -257,8 +257,24 @@ def get_team_for_user_and_course(user, course_id): raise ValueError(u"The supplied course id {course_id} is not valid.".format( course_id=course_id )) - - return CourseTeam.objects.filter( - course_id=course_key, - membership__user__username=user.username, - ).first() + try: + return CourseTeam.objects.get( + course_id=course_key, + membership__user__username=user.username, + topic_id=topic_id, + ) + except CourseTeam.DoesNotExist: + return None + except CourseTeam.MultipleObjectsReturned: + # This shouldn't ever happen but it's here for safety's sake + msg = "user {username} is on multiple teams within course {course} topic {topic}" + logger.error(msg.format( + username=user.username, + course=course_id, + topic=topic_id, + )) + return CourseTeam.objects.filter( + course_id=course_key, + membership__user__username=user.username, + topic_id=topic_id, + ).first() diff --git a/lms/djangoapps/teams/services.py b/lms/djangoapps/teams/services.py index 900d4eb0b6..077c8e0d2b 100644 --- a/lms/djangoapps/teams/services.py +++ b/lms/djangoapps/teams/services.py @@ -6,9 +6,9 @@ from django.urls import reverse class TeamsService(object): """ Functions to provide teams functionality to XBlocks""" - def get_team(self, user, course_id): + def get_team(self, user, course_id, topic_id): from . import api - return api.get_team_for_user_and_course(user, course_id) + return api.get_team_for_user_course_topic(user, course_id, topic_id) def get_team_detail_url(self, team): """ Returns the url to the detail view for the given team """ diff --git a/lms/djangoapps/teams/tests/test_api.py b/lms/djangoapps/teams/tests/test_api.py index 9b09cc2378..4e29a683f9 100644 --- a/lms/djangoapps/teams/tests/test_api.py +++ b/lms/djangoapps/teams/tests/test_api.py @@ -3,15 +3,16 @@ Tests for Python APIs of the Teams app """ - import unittest from uuid import uuid4 import ddt +import mock from opaque_keys.edx.keys import CourseKey from course_modes.models import CourseMode from lms.djangoapps.teams import api as teams_api +from lms.djangoapps.teams.models import CourseTeam from lms.djangoapps.teams.tests.factories import CourseTeamFactory from student.models import CourseEnrollment from student.roles import CourseStaffRole @@ -20,10 +21,13 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase COURSE_KEY1 = CourseKey.from_string('edx/history/1') COURSE_KEY2 = CourseKey.from_string('edx/history/2') +TOPIC1 = 'topic-1' +TOPIC2 = 'topic-2' DISCUSSION_TOPIC_ID = uuid4().hex +@ddt.ddt class PythonAPITests(SharedModuleStoreTestCase): """ The set of tests for different API endpoints @@ -34,23 +38,39 @@ class PythonAPITests(SharedModuleStoreTestCase): cls.user1 = UserFactory.create(username='user1') cls.user2 = UserFactory.create(username='user2') cls.user3 = UserFactory.create(username='user3') + cls.user4 = UserFactory.create(username='user4') - for user in (cls.user1, cls.user2, cls.user3): + for user in (cls.user1, cls.user2, cls.user3, cls.user4): CourseEnrollmentFactory.create(user=user, course_id=COURSE_KEY1) - CourseEnrollmentFactory.create(user=cls.user3, course_id=COURSE_KEY2) + for user in (cls.user3, cls.user4): + CourseEnrollmentFactory.create(user=user, course_id=COURSE_KEY2) cls.team1 = CourseTeamFactory( course_id=COURSE_KEY1, discussion_topic_id=DISCUSSION_TOPIC_ID, - team_id='team1' + team_id='team1', + topic_id=TOPIC1, + ) + cls.team1a = CourseTeamFactory( # Same topic / team set as team1 + course_id=COURSE_KEY1, + team_id='team1a', + topic_id=TOPIC1, + ) + cls.team2 = CourseTeamFactory(course_id=COURSE_KEY2, team_id='team2', topic_id=TOPIC2) + cls.team2a = CourseTeamFactory( # Same topic / team set as team2 + course_id=COURSE_KEY2, + team_id='team2a', + topic_id=TOPIC2 ) - cls.team2 = CourseTeamFactory(course_id=COURSE_KEY2, team_id='team2') cls.team1.add_user(cls.user1) cls.team1.add_user(cls.user2) cls.team2.add_user(cls.user3) + cls.team1a.add_user(cls.user4) + cls.team2a.add_user(cls.user4) + def test_get_team_by_discussion_non_existence(self): self.assertIsNone(teams_api.get_team_by_discussion('DO_NOT_EXIST')) @@ -81,34 +101,48 @@ class PythonAPITests(SharedModuleStoreTestCase): self.assertTrue(teams_api.discussion_visible_by_user(self.team2.discussion_topic_id, self.user2)) self.assertTrue(teams_api.discussion_visible_by_user('DO_NOT_EXISTS', self.user3)) - def test_get_team(self): - # Course 1 - user1_team = teams_api.get_team_for_user_and_course(self.user1, str(COURSE_KEY1)) - user2_team = teams_api.get_team_for_user_and_course(self.user2, str(COURSE_KEY1)) - user3_team = teams_api.get_team_for_user_and_course(self.user3, str(COURSE_KEY1)) + @ddt.unpack + @ddt.data( + (COURSE_KEY1, TOPIC1, ['team1', 'team1', None, 'team1a']), + (COURSE_KEY1, TOPIC2, [None, None, None, None]), + (COURSE_KEY2, TOPIC1, [None, None, None, None]), + (COURSE_KEY2, TOPIC2, [None, None, 'team2', 'team2a']), + ) + def test_get_team_for_user_course_topic(self, course_key, topic_id, expected_team_ids): + user1_team = teams_api.get_team_for_user_course_topic(self.user1, str(course_key), topic_id) + user2_team = teams_api.get_team_for_user_course_topic(self.user2, str(course_key), topic_id) + user3_team = teams_api.get_team_for_user_course_topic(self.user3, str(course_key), topic_id) + user4_team = teams_api.get_team_for_user_course_topic(self.user4, str(course_key), topic_id) - self.assertEqual(user1_team, self.team1) - self.assertEqual(user2_team, self.team1) - self.assertEqual(user3_team, None) + self.assertEqual(user1_team.team_id if user1_team else None, expected_team_ids[0]) + self.assertEqual(user2_team.team_id if user2_team else None, expected_team_ids[1]) + self.assertEqual(user3_team.team_id if user3_team else None, expected_team_ids[2]) + self.assertEqual(user4_team.team_id if user4_team else None, expected_team_ids[3]) - # Course 2 - user1_team = teams_api.get_team_for_user_and_course(self.user1, str(COURSE_KEY2)) - user2_team = teams_api.get_team_for_user_and_course(self.user2, str(COURSE_KEY2)) - user3_team = teams_api.get_team_for_user_and_course(self.user3, str(COURSE_KEY2)) + @mock.patch('lms.djangoapps.teams.api.CourseTeam.objects') + def test_get_team_multiple_teams(self, mocked_manager): + """ + This is a test for a use case that is very unlikely to occur. + Currently users cannot be in multiple teams in a course, but even after we allow multiple + teams in a course then they should still be limited to one team per topic + """ + mocked_manager.get.side_effect = CourseTeam.MultipleObjectsReturned() + expected_result = "This is somehow the first team" + mock_qs = mock.MagicMock() + mock_qs.first.return_value = expected_result + mocked_manager.filter.return_value = mock_qs + result = teams_api.get_team_for_user_course_topic(self.user1, str(COURSE_KEY1), TOPIC1) + self.assertEqual(result, expected_result) - self.assertEqual(user1_team, None) - self.assertEqual(user2_team, None) - self.assertEqual(user3_team, self.team2) + def test_get_team_course_not_found(self): + team = teams_api.get_team_for_user_course_topic(self.user1, 'nonsense/garbage/nonexistant', 'topic') + self.assertIsNone(team) def test_get_team_invalid_course(self): invalid_course_id = 'lol!()#^$&course' message = 'The supplied course id lol!()#^$&course is not valid' with self.assertRaisesMessage(ValueError, message): - teams_api.get_team_for_user_and_course(self.user1, invalid_course_id) - - def test_get_team_course_not_found(self): - team = teams_api.get_team_for_user_and_course(self.user1, 'nonsense/garbage/nonexistant') - self.assertIsNone(team) + teams_api.get_team_for_user_course_topic(self.user1, invalid_course_id, 'who-cares') @ddt.ddt diff --git a/lms/djangoapps/teams/tests/test_services.py b/lms/djangoapps/teams/tests/test_services.py index acde4570e0..c5d5eef72d 100644 --- a/lms/djangoapps/teams/tests/test_services.py +++ b/lms/djangoapps/teams/tests/test_services.py @@ -6,6 +6,7 @@ Tests for any Teams app services from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from openedx.core.djangoapps.catalog.tests.factories import CourseRunFactory +from student.tests.factories import CourseEnrollmentFactory, UserFactory from lms.djangoapps.teams.services import TeamsService from lms.djangoapps.teams.tests.factories import CourseTeamFactory @@ -17,8 +18,20 @@ class TeamsServiceTests(ModuleStoreTestCase): def setUp(self): super(TeamsServiceTests, self).setUp() self.course_run = CourseRunFactory.create() - self.team = CourseTeamFactory.create(course_id=self.course_run['key']) + self.course_key = self.course_run['key'] + self.team = CourseTeamFactory.create(course_id=self.course_key) self.service = TeamsService() + self.user = UserFactory.create() + CourseEnrollmentFactory.create(user=self.user, course_id=self.course_key) + self.team.add_user(self.user) + + def test_get_team(self): + user_team = self.service.get_team(self.user, self.course_key, self.team.topic_id) + self.assertEqual(user_team, self.team) + + user2 = UserFactory.create() + user2_team = self.service.get_team(user2, self.course_key, self.team.topic_id) + self.assertIsNone(user2_team) def test_get_team_detail_url(self): # edx.org/courses/blah/teams/#teams/topic_id/team_id