diff --git a/lms/djangoapps/teams/api_urls.py b/lms/djangoapps/teams/api_urls.py index bf4f0da8f7..39d279bcd8 100644 --- a/lms/djangoapps/teams/api_urls.py +++ b/lms/djangoapps/teams/api_urls.py @@ -7,7 +7,9 @@ from .views import ( TeamsListView, TeamsDetailView, TopicDetailView, - TopicListView + TopicListView, + MembershipListView, + MembershipDetailView ) TEAM_ID_PATTERN = r'(?P[a-z\d_-]+)' @@ -35,5 +37,15 @@ urlpatterns = patterns( r'^v0/topics/' + TOPIC_ID_PATTERN + ',' + settings.COURSE_ID_PATTERN + '$', TopicDetailView.as_view(), name="topics_detail" + ), + url( + r'^v0/team_membership$', + MembershipListView.as_view(), + name="team_membership_list" + ), + url( + r'^v0/team_membership/' + TEAM_ID_PATTERN + ',' + USERNAME_PATTERN + '$', + MembershipDetailView.as_view(), + name="team_membership_detail" ) ) diff --git a/lms/djangoapps/teams/errors.py b/lms/djangoapps/teams/errors.py new file mode 100644 index 0000000000..36b8ca54a0 --- /dev/null +++ b/lms/djangoapps/teams/errors.py @@ -0,0 +1,16 @@ +"""Errors thrown in the Team API""" + + +class TeamAPIRequestError(Exception): + """There was a problem with a request to the Team API.""" + pass + + +class NotEnrolledInCourseForTeam(TeamAPIRequestError): + """User is not enrolled in the course for the team they are trying to join.""" + pass + + +class AlreadyOnTeamInCourse(TeamAPIRequestError): + """User is already a member of another team in the same course.""" + pass diff --git a/lms/djangoapps/teams/models.py b/lms/djangoapps/teams/models.py index 15874a69b2..51f4fe9721 100644 --- a/lms/djangoapps/teams/models.py +++ b/lms/djangoapps/teams/models.py @@ -7,7 +7,8 @@ from django_countries.fields import CountryField from xmodule_django.models import CourseKeyField from util.model_utils import generate_unique_readable_id -from student.models import LanguageField +from student.models import LanguageField, CourseEnrollment +from .errors import AlreadyOnTeamInCourse, NotEnrolledInCourseForTeam class CourseTeam(models.Model): @@ -62,7 +63,11 @@ class CourseTeam(models.Model): def add_user(self, user): """Adds the given user to the CourseTeam.""" - CourseTeamMembership.objects.get_or_create( + if not CourseEnrollment.is_enrolled(user, self.course_id): + raise NotEnrolledInCourseForTeam + if CourseTeamMembership.objects.filter(user=user, team__course_id=self.course_id).exists(): + raise AlreadyOnTeamInCourse + return CourseTeamMembership.objects.create( user=user, team=self ) diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 49ba25c67f..9babb1f83e 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -121,6 +121,12 @@ class TeamAPITestCase(APITestCase, ModuleStoreTestCase): self.users = { 'student_unenrolled': UserFactory.create(password=self.test_password), 'student_enrolled': UserFactory.create(password=self.test_password), + 'student_enrolled_not_on_team': UserFactory.create(password=self.test_password), + + # This student is enrolled in both test courses and is a member of a team in each course, but is not on the + # same team as student_enrolled. + 'student_enrolled_both_courses_other_team': UserFactory.create(password=self.test_password), + 'staff': AdminFactory.create(password=self.test_password), 'course_staff': StaffFactory.create(course_key=self.test_course_1.id, password=self.test_password) } @@ -135,11 +141,19 @@ class TeamAPITestCase(APITestCase, ModuleStoreTestCase): self.test_team_4 = CourseTeamFactory.create(name='Coal Team', course_id=self.test_course_1.id, is_active=False) self.test_team_4 = CourseTeamFactory.create(name='Another Team', course_id=self.test_course_2.id) - self.test_team_1.add_user(self.users['student_enrolled']) + for user, course in [ + ('student_enrolled', self.test_course_1), + ('student_enrolled_not_on_team', self.test_course_1), + ('student_enrolled_both_courses_other_team', self.test_course_1), + ('student_enrolled_both_courses_other_team', self.test_course_2) + ]: + CourseEnrollment.enroll( + self.users[user], course.id, check_access=True + ) - CourseEnrollment.enroll( - self.users['student_enrolled'], self.test_course_1.id, check_access=True - ) + self.test_team_1.add_user(self.users['student_enrolled']) + self.test_team_3.add_user(self.users['student_enrolled_both_courses_other_team']) + self.test_team_4.add_user(self.users['student_enrolled_both_courses_other_team']) def login(self, user): """Given a user string, logs the given user in. @@ -197,9 +211,9 @@ class TeamAPITestCase(APITestCase, ModuleStoreTestCase): """Posts data to the team creation endpoint. Verifies expected_status.""" return self.make_call(reverse('teams_list'), expected_status, 'post', data, **kwargs) - def get_team_detail(self, team_id, expected_status=200, **kwargs): + def get_team_detail(self, team_id, expected_status=200, data=None, **kwargs): """Gets detailed team information for team_id. Verifies expected_status.""" - return self.make_call(reverse('teams_detail', args=[team_id]), expected_status, 'get', **kwargs) + return self.make_call(reverse('teams_detail', args=[team_id]), expected_status, 'get', data, **kwargs) def patch_team_detail(self, team_id, expected_status, data=None, **kwargs): """Patches the team with team_id using data. Verifies expected_status.""" @@ -226,14 +240,51 @@ class TeamAPITestCase(APITestCase, ModuleStoreTestCase): **kwargs ) + def get_membership_list(self, expected_status=200, data=None, **kwargs): + """Gets the membership list, passing data as query params. Verifies expected_status.""" + return self.make_call(reverse('team_membership_list'), expected_status, 'get', data, **kwargs) + + def post_create_membership(self, expected_status=200, data=None, **kwargs): + """Posts data to the membership creation endpoint. Verifies expected_status.""" + return self.make_call(reverse('team_membership_list'), expected_status, 'post', data, **kwargs) + + def get_membership_detail(self, team_id, username, expected_status=200, data=None, **kwargs): + """Gets an individual membership record, passing data as query params. Verifies expected_status.""" + return self.make_call( + reverse('team_membership_detail', args=[team_id, username]), + expected_status, + 'get', + data, + **kwargs + ) + + def delete_membership(self, team_id, username, expected_status=200, **kwargs): + """Deletes an individual membership record. Verifies expected_status.""" + return self.make_call( + reverse('team_membership_detail', args=[team_id, username]), + expected_status, + 'delete', + **kwargs + ) + + def verify_expanded_user(self, user): + """Verifies that fields exist on the returned user json indicating that it is expanded.""" + for field in ['id', 'url', 'email', 'name', 'username', 'preferences']: + self.assertIn(field, user) + + def verify_expanded_team(self, team): + """Verifies that fields exist on the returned team json indicating that it is expanded.""" + for field in ['id', 'name', 'is_active', 'course_id', 'topic_id', 'date_created', 'description']: + self.assertIn(field, team) + @ddt.ddt class TestListTeamsAPI(TeamAPITestCase): """Test cases for the team listing API endpoint.""" @ddt.data( - (None, 403), - ('student_inactive', 403), + (None, 401), + ('student_inactive', 401), ('student_unenrolled', 403), ('student_enrolled', 200), ('staff', 200), @@ -255,7 +306,7 @@ class TestListTeamsAPI(TeamAPITestCase): self.assertEqual(names, [team['name'] for team in teams['results']]) def test_filter_invalid_course_id(self): - self.verify_names({'course_id': 'foobar'}, 400) + self.verify_names({'course_id': 'no_such_course'}, 400) def test_filter_course_id(self): self.verify_names({'course_id': self.test_course_2.id}, 200, ['Another Team'], user='staff') @@ -274,7 +325,7 @@ class TestListTeamsAPI(TeamAPITestCase): @ddt.data( (None, 200, ['Nuclear Team', u'sólar team', 'Wind Team']), ('name', 200, ['Nuclear Team', u'sólar team', 'Wind Team']), - ('open_slots', 200, ['Wind Team', 'Nuclear Team', u'sólar team']), + ('open_slots', 200, ['Wind Team', u'sólar team', 'Nuclear Team']), ('last_activity', 400, []), ) @ddt.unpack @@ -282,7 +333,7 @@ class TestListTeamsAPI(TeamAPITestCase): data = {'order_by': field} if field else {} self.verify_names(data, status, names) - @ddt.data({'course_id': 'foobar/foobar/foobar'}, {'topic_id': 'foobar'}) + @ddt.data({'course_id': 'no/such/course'}, {'topic_id': 'no_such_topic'}) def test_no_results(self, data): self.get_teams_list(404, data) @@ -296,14 +347,18 @@ class TestListTeamsAPI(TeamAPITestCase): self.assertIsNone(result['next']) self.assertIsNotNone(result['previous']) + def test_expand_user(self): + result = self.get_teams_list(200, {'expand': 'user', 'topic_id': 'renewable'}) + self.verify_expanded_user(result['results'][0]['membership'][0]['user']) + @ddt.ddt class TestCreateTeamAPI(TeamAPITestCase): """Test cases for the team creation endpoint.""" @ddt.data( - (None, 403), - ('student_inactive', 403), + (None, 401), + ('student_inactive', 401), ('student_unenrolled', 403), ('student_enrolled', 200), ('staff', 200), @@ -329,11 +384,11 @@ class TestCreateTeamAPI(TeamAPITestCase): @ddt.data((400, { 'name': 'Bad Course Id', - 'course_id': 'foobar', + 'course_id': 'no_such_course', 'description': "Filler Description" }), (404, { 'name': "Non-existent course id", - 'course_id': 'foobar/foobar/foobar', + 'course_id': 'no/such/course', 'description': "Filler Description" })) @ddt.unpack @@ -380,8 +435,8 @@ class TestDetailTeamAPI(TeamAPITestCase): """Test cases for the team detail endpoint.""" @ddt.data( - (None, 403), - ('student_inactive', 403), + (None, 401), + ('student_inactive', 401), ('student_unenrolled', 403), ('student_enrolled', 200), ('staff', 200), @@ -394,7 +449,11 @@ class TestDetailTeamAPI(TeamAPITestCase): self.assertEquals(team['description'], self.test_team_1.description) def test_does_not_exist(self): - self.get_team_detail('foobar', 404) + self.get_team_detail('no_such_team', 404) + + def test_expand_user(self): + result = self.get_team_detail(self.test_team_1.team_id, 200, {'expand': 'user'}) + self.verify_expanded_user(result['membership'][0]['user']) @ddt.ddt @@ -402,8 +461,8 @@ class TestUpdateTeamAPI(TeamAPITestCase): """Test cases for the team update endpoint.""" @ddt.data( - (None, 403), - ('student_inactive', 403), + (None, 401), + ('student_inactive', 401), ('student_unenrolled', 403), ('student_enrolled', 403), ('staff', 200), @@ -416,8 +475,8 @@ class TestUpdateTeamAPI(TeamAPITestCase): self.assertEquals(team['name'], 'foo') @ddt.data( - (None, 403), - ('student_inactive', 403), + (None, 401), + ('student_inactive', 401), ('student_unenrolled', 404), ('student_enrolled', 404), ('staff', 404), @@ -425,13 +484,13 @@ class TestUpdateTeamAPI(TeamAPITestCase): ) @ddt.unpack def test_access_bad_id(self, user, status): - self.patch_team_detail("foobar", status, {'name': 'foo'}, user=user) + self.patch_team_detail("no_such_team", status, {'name': 'foo'}, user=user) @ddt.data( ('id', 'foobar'), ('description', ''), - ('country', 'foobar'), - ('language', 'foobar') + ('country', 'no_such_country'), + ('language', 'no_such_language') ) @ddt.unpack def test_bad_requests(self, key, value): @@ -443,7 +502,7 @@ class TestUpdateTeamAPI(TeamAPITestCase): self.patch_team_detail(self.test_team_1.team_id, 200, {key: value}, user='staff') def test_does_not_exist(self): - self.patch_team_detail('foobar', 404, user='staff') + self.patch_team_detail('no_such_team', 404, user='staff') @ddt.ddt @@ -451,8 +510,8 @@ class TestListTopicsAPI(TeamAPITestCase): """Test cases for the topic listing endpoint.""" @ddt.data( - (None, 403), - ('student_inactive', 403), + (None, 401), + ('student_inactive', 401), ('student_unenrolled', 403), ('student_enrolled', 200), ('staff', 200), @@ -474,7 +533,7 @@ class TestListTopicsAPI(TeamAPITestCase): @ddt.data( (None, 200, ['Coal Power', 'Nuclear Power', u'sólar power', 'Wind Power']), ('name', 200, ['Coal Power', 'Nuclear Power', u'sólar power', 'Wind Power']), - ('foobar', 400, []), + ('no_such_field', 400, []), ) @ddt.unpack def test_order_by(self, field, status, names): @@ -503,8 +562,8 @@ class TestDetailTopicAPI(TeamAPITestCase): """Test cases for the topic detail endpoint.""" @ddt.data( - (None, 403), - ('student_inactive', 403), + (None, 401), + ('student_inactive', 401), ('student_unenrolled', 403), ('student_enrolled', 200), ('staff', 200), @@ -522,4 +581,227 @@ class TestDetailTopicAPI(TeamAPITestCase): self.get_topic_detail('topic_0', course_id, 404) def test_invalid_topic_id(self): - self.get_topic_detail('foobar', self.test_course_1.id, 404) + self.get_topic_detail('no_such_topic', self.test_course_1.id, 404) + + +@ddt.ddt +class TestListMembershipAPI(TeamAPITestCase): + """Test cases for the membership list endpoint.""" + + @ddt.data( + (None, 401), + ('student_inactive', 401), + ('student_unenrolled', 404), + ('student_enrolled', 200), + ('student_enrolled_both_courses_other_team', 200), + ('staff', 200), + ('course_staff', 200), + ) + @ddt.unpack + def test_access(self, user, status): + membership = self.get_membership_list(status, {'team_id': self.test_team_1.team_id}, user=user) + if status == 200: + self.assertEqual(membership['count'], 1) + self.assertEqual(membership['results'][0]['user']['id'], self.users['student_enrolled'].username) + + @ddt.data( + (None, 401, False), + ('student_inactive', 401, False), + ('student_unenrolled', 200, False), + ('student_enrolled', 200, True), + ('student_enrolled_both_courses_other_team', 200, True), + ('staff', 200, True), + ('course_staff', 200, True), + ) + @ddt.unpack + def test_access_by_username(self, user, status, has_content): + membership = self.get_membership_list(status, {'username': self.users['student_enrolled'].username}, user=user) + if status == 200: + if has_content: + self.assertEqual(membership['count'], 1) + self.assertEqual(membership['results'][0]['team']['id'], self.test_team_1.team_id) + else: + self.assertEqual(membership['count'], 0) + + def test_no_username_or_team_id(self): + self.get_membership_list(400, {}) + + def test_bad_team_id(self): + self.get_membership_list(404, {'team_id': 'no_such_team'}) + + def test_expand_user(self): + result = self.get_membership_list(200, {'team_id': self.test_team_1.team_id, 'expand': 'user'}) + self.verify_expanded_user(result['results'][0]['user']) + + def test_expand_team(self): + result = self.get_membership_list(200, {'team_id': self.test_team_1.team_id, 'expand': 'team'}) + self.verify_expanded_team(result['results'][0]['team']) + + +@ddt.ddt +class TestCreateMembershipAPI(TeamAPITestCase): + """Test cases for the membership creation endpoint.""" + + def build_membership_data_raw(self, username, team): + """Assembles a membership creation payload based on the raw values provided.""" + return {'username': username, 'team_id': team} + + def build_membership_data(self, username, team): + """Assembles a membership creation payload based on the username and team model provided.""" + return self.build_membership_data_raw(self.users[username].username, team.team_id) + + @ddt.data( + (None, 401), + ('student_inactive', 401), + ('student_unenrolled', 404), + ('student_enrolled_not_on_team', 200), + ('student_enrolled', 404), + ('student_enrolled_both_courses_other_team', 404), + ('staff', 200), + ('course_staff', 200), + ) + @ddt.unpack + def test_access(self, user, status): + membership = self.post_create_membership( + status, + self.build_membership_data('student_enrolled_not_on_team', self.test_team_1), + user=user + ) + if status == 200: + self.assertEqual(membership['user']['id'], self.users['student_enrolled_not_on_team'].username) + self.assertEqual(membership['team']['id'], self.test_team_1.team_id) + memberships = self.get_membership_list(200, {'team_id': self.test_team_1.team_id}) + self.assertEqual(memberships['count'], 2) + + def test_no_username(self): + response = self.post_create_membership(400, {'team_id': self.test_team_1.team_id}) + self.assertIn('username', json.loads(response.content)['field_errors']) + + def test_no_team(self): + response = self.post_create_membership(400, {'username': self.users['student_enrolled_not_on_team'].username}) + self.assertIn('team_id', json.loads(response.content)['field_errors']) + + def test_bad_team(self): + self.post_create_membership( + 404, + self.build_membership_data_raw(self.users['student_enrolled'].username, 'no_such_team') + ) + + def test_bad_username(self): + self.post_create_membership( + 404, + self.build_membership_data_raw('no_such_user', self.test_team_1.team_id), + user='staff' + ) + + @ddt.data('student_enrolled', 'staff', 'course_staff') + def test_join_twice(self, user): + response = self.post_create_membership( + 400, + self.build_membership_data('student_enrolled', self.test_team_1), + user=user + ) + self.assertIn('already a member', json.loads(response.content)['developer_message']) + + def test_join_second_team_in_course(self): + response = self.post_create_membership( + 400, + self.build_membership_data('student_enrolled_both_courses_other_team', self.test_team_1), + user='student_enrolled_both_courses_other_team' + ) + self.assertIn('already a member', json.loads(response.content)['developer_message']) + + @ddt.data('staff', 'course_staff') + def test_not_enrolled_in_team_course(self, user): + response = self.post_create_membership( + 400, + self.build_membership_data('student_unenrolled', self.test_team_1), + user=user + ) + self.assertIn('not enrolled', json.loads(response.content)['developer_message']) + + +@ddt.ddt +class TestDetailMembershipAPI(TeamAPITestCase): + """Test cases for the membership detail endpoint.""" + + @ddt.data( + (None, 401), + ('student_inactive', 401), + ('student_unenrolled', 404), + ('student_enrolled_not_on_team', 200), + ('student_enrolled', 200), + ('staff', 200), + ('course_staff', 200), + ) + @ddt.unpack + def test_access(self, user, status): + self.get_membership_detail( + self.test_team_1.team_id, + self.users['student_enrolled'].username, + status, + user=user + ) + + def test_bad_team(self): + self.get_membership_detail('no_such_team', self.users['student_enrolled'].username, 404) + + def test_bad_username(self): + self.get_membership_detail(self.test_team_1.team_id, 'no_such_user', 404) + + def test_no_membership(self): + self.get_membership_detail( + self.test_team_1.team_id, + self.users['student_enrolled_not_on_team'].username, + 404 + ) + + def test_expand_user(self): + result = self.get_membership_detail( + self.test_team_1.team_id, + self.users['student_enrolled'].username, + 200, + {'expand': 'user'} + ) + self.verify_expanded_user(result['user']) + + def test_expand_team(self): + result = self.get_membership_detail( + self.test_team_1.team_id, + self.users['student_enrolled'].username, + 200, + {'expand': 'team'} + ) + self.verify_expanded_team(result['team']) + + +@ddt.ddt +class TestDeleteMembershipAPI(TeamAPITestCase): + """Test cases for the membership deletion endpoint.""" + + @ddt.data( + (None, 401), + ('student_inactive', 401), + ('student_unenrolled', 404), + ('student_enrolled_not_on_team', 404), + ('student_enrolled', 204), + ('staff', 204), + ('course_staff', 204), + ) + @ddt.unpack + def test_access(self, user, status): + self.delete_membership( + self.test_team_1.team_id, + self.users['student_enrolled'].username, + status, + user=user + ) + + def test_bad_team(self): + self.delete_membership('no_such_team', self.users['student_enrolled'].username, 404) + + def test_bad_username(self): + self.delete_membership(self.test_team_1.team_id, 'no_such_user', 404) + + def test_missing_membership(self): + self.delete_membership(self.test_team_2.team_id, self.users['student_enrolled'].username, 404) diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index 759567d5b4..9d678d9fce 100644 --- a/lms/djangoapps/teams/views.py +++ b/lms/djangoapps/teams/views.py @@ -18,15 +18,21 @@ from rest_framework import status from rest_framework import permissions from django.db.models import Count +from django.contrib.auth.models import User from django.utils.translation import ugettext as _ from django.utils.translation import ugettext_noop -from student.models import CourseEnrollment +from student.models import CourseEnrollment, CourseAccessRole from student.roles import CourseStaffRole from openedx.core.lib.api.parsers import MergePatchParser from openedx.core.lib.api.permissions import IsStaffOrReadOnly -from openedx.core.lib.api.view_utils import RetrievePatchAPIView, add_serializer_errors +from openedx.core.lib.api.view_utils import ( + RetrievePatchAPIView, + add_serializer_errors, + build_api_error, + ExpandableFieldViewMixin +) from openedx.core.lib.api.serializers import PaginationSerializer from xmodule.modulestore.django import modulestore @@ -34,8 +40,9 @@ from xmodule.modulestore.django import modulestore from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from .models import CourseTeam -from .serializers import CourseTeamSerializer, CourseTeamCreationSerializer, TopicSerializer +from .models import CourseTeam, CourseTeamMembership +from .serializers import CourseTeamSerializer, CourseTeamCreationSerializer, TopicSerializer, MembershipSerializer +from .errors import AlreadyOnTeamInCourse, NotEnrolledInCourseForTeam class TeamsDashboardView(View): @@ -71,7 +78,7 @@ def is_feature_enabled(course): return settings.FEATURES.get('ENABLE_TEAMS', False) and course.teams_enabled -def has_team_api_access(user, course_key): +def has_team_api_access(user, course_key, access_username=None): """Returns True if the user has access to the Team API for the course given by `course_key`. The user must either be enrolled in the course, be course staff, or be global staff. @@ -79,16 +86,21 @@ def has_team_api_access(user, course_key): Args: user (User): The user to check access for. course_key (CourseKey): The key to the course which we are checking access to. + access_username (string): If provided, access_username must match user.username for non staff access. Returns: bool: True if the user has access, False otherwise. """ - return (CourseEnrollment.is_enrolled(user, course_key) or - CourseStaffRole(course_key).has_user(user) or - user.is_staff) + if user.is_staff: + return True + if CourseStaffRole(course_key).has_user(user): + return True + if not access_username or access_username == user.username: + return CourseEnrollment.is_enrolled(user, course_key) + return False -class TeamsListView(GenericAPIView): +class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): """ **Use Cases** @@ -125,6 +137,9 @@ class TeamsListView(GenericAPIView): * include_inactive: If true, inactive teams will be returned. The default is to not include inactive teams. + * expand: Comma separated list of types for which to return + expanded representations. Supports "user" and "team". + **Response Values for GET** If the user is logged in and enrolled, the response contains: @@ -172,8 +187,10 @@ class TeamsListView(GenericAPIView): stored exactly as specified. The intention is that plain text is supported, not HTML. - If the user is not logged in and enrolled in the course specified by - course_id or is not course or global staff, a 403 error is returned. + If the user is not logged in, a 401 error is returned. + + If the user is not enrolled in the course specified by course_id or + is not course or global staff, a 403 error is returned. If the specified course_id is not valid or the user attempts to use an unsupported query parameter, a 400 error is returned. @@ -189,8 +206,10 @@ class TeamsListView(GenericAPIView): but does not include the id, is_active, date_created, or membership fields. id is automatically computed based on name. - If the user is not logged in, is not enrolled in the course, or is - not course or global staff, a 403 error is returned. + If the user is not logged in, a 401 error is returned. + + If the user is not enrolled in the course, or is not course or + global staff, a 403 error is returned. If the course_id is not valid or extra fields are included in the request, a 400 error is returned. @@ -198,8 +217,8 @@ class TeamsListView(GenericAPIView): If the specified course does not exist, a 404 error is returned. """ - # SessionAuthentication must come first to return a 403 for unauthenticated users - authentication_classes = (SessionAuthentication, OAuth2Authentication) + # OAuth2Authentication must come first to return a 401 for unauthenticated users + authentication_classes = (OAuth2Authentication, SessionAuthentication) permission_classes = (permissions.IsAuthenticated,) paginate_by = 10 @@ -207,12 +226,6 @@ class TeamsListView(GenericAPIView): pagination_serializer_class = PaginationSerializer serializer_class = CourseTeamSerializer - def get_serializer_context(self): - """Adds expand information from query parameters to the serializer context to support expandable fields.""" - result = super(TeamsListView, self).get_serializer_context() - result['expand'] = [x for x in self.request.QUERY_PARAMS.get('expand', '').split(',') if x] - return result - def get(self, request): """GET /api/team/v0/teams/""" result_filter = { @@ -228,33 +241,29 @@ class TeamsListView(GenericAPIView): return Response(status=status.HTTP_404_NOT_FOUND) result_filter.update({'course_id': course_key}) except InvalidKeyError: - error_message = ugettext_noop("The supplied course id {course_id} is not valid.").format( - course_id=course_id_string + error = build_api_error( + ugettext_noop("The supplied course id {course_id} is not valid."), + course_id=course_id_string, ) - return Response({ - 'developer_message': error_message, - 'user_message': _(error_message) # pylint: disable=translation-of-non-string - }, status=status.HTTP_400_BAD_REQUEST) + return Response(error, status=status.HTTP_400_BAD_REQUEST) if not has_team_api_access(request.user, course_key): return Response(status=status.HTTP_403_FORBIDDEN) else: - error_message = ugettext_noop('course_id must be provided') - return Response({ - 'developer_message': error_message, - 'user_message': _(error_message), # pylint: disable=translation-of-non-string - }, status=status.HTTP_400_BAD_REQUEST) + return Response( + build_api_error(ugettext_noop("course_id must be provided")), + status=status.HTTP_400_BAD_REQUEST + ) if 'topic_id' in request.QUERY_PARAMS: result_filter.update({'topic_id': request.QUERY_PARAMS['topic_id']}) if 'include_inactive' in request.QUERY_PARAMS and request.QUERY_PARAMS['include_inactive'].lower() == 'true': del result_filter['is_active'] if 'text_search' in request.QUERY_PARAMS: - error_message = ugettext_noop('text_search is not yet supported') - return Response({ - 'developer_message': error_message, - 'user_message': _(error_message), # pylint: disable=translation-of-non-string - }, status=status.HTTP_400_BAD_REQUEST) + return Response( + build_api_error(ugettext_noop("text_search is not yet supported.")), + status=status.HTTP_400_BAD_REQUEST + ) queryset = CourseTeam.objects.filter(**result_filter) @@ -266,10 +275,10 @@ class TeamsListView(GenericAPIView): queryset = queryset.annotate(team_size=Count('users')) order_by_field = 'team_size' elif order_by_input == 'last_activity': - return Response({ - 'developer_message': "last_activity is not yet supported", - 'user_message': _("The last_activity parameter is not yet supported."), - }, status=status.HTTP_400_BAD_REQUEST) + return Response( + build_api_error(ugettext_noop("last_activity is not yet supported")), + status=status.HTTP_400_BAD_REQUEST + ) queryset = queryset.order_by(order_by_field) @@ -292,10 +301,10 @@ class TeamsListView(GenericAPIView): if not modulestore().has_course(course_key): return Response(status=status.HTTP_404_NOT_FOUND) except InvalidKeyError: - field_errors['course_id'] = { - 'developer_message': "course_id {} is not valid.".format(course_id), - 'user_message': _("The supplied course_id {} is not valid.").format(course_id), - } + field_errors['course_id'] = build_api_error( + ugettext_noop('The supplied course_id {course_id} is not valid.'), + course_id=course_id + ) if course_key and not has_team_api_access(request.user, course_key): return Response(status=status.HTTP_403_FORBIDDEN) @@ -315,7 +324,7 @@ class TeamsListView(GenericAPIView): return Response(CourseTeamSerializer(team).data) -class TeamsDetailView(RetrievePatchAPIView): +class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView): """ **Use Cases** @@ -328,6 +337,11 @@ class TeamsDetailView(RetrievePatchAPIView): PATCH /api/team/v0/teams/{team_id} "application/merge-patch+json" + **Query Parameters for GET** + + * expand: Comma separated list of types for which to return + expanded representations. Supports "user" and "team". + **Response Values for GET** If the user is logged in, the response contains the following fields: @@ -363,8 +377,9 @@ class TeamsDetailView(RetrievePatchAPIView): stored exactly as specified. The intention is that plain text is supported, not HTML. - If the user is not logged in or is not course or global staff, a 403 - error is returned. + If the user is not logged in, a 401 error is returned. + + If the user is not course or global staff, a 403 error is returned. If the specified team does not exist, a 404 error is returned. @@ -372,7 +387,8 @@ class TeamsDetailView(RetrievePatchAPIView): Only staff can patch teams. - If the user is anonymous or inactive, a 403 is returned. + If the user is anonymous or inactive, a 401 is returned. + If the user is logged in and the team does not exist, a 404 is returned. If the user is not course or global staff and the team does exist, a 403 is returned. @@ -392,7 +408,7 @@ class TeamsDetailView(RetrievePatchAPIView): """Returns true if the user is enrolled or is staff.""" return has_team_api_access(request.user, obj.course_id) - authentication_classes = (SessionAuthentication, OAuth2Authentication) + authentication_classes = (OAuth2Authentication, SessionAuthentication) permission_classes = (permissions.IsAuthenticated, IsStaffOrReadOnly, IsEnrolledOrIsStaff,) lookup_field = 'team_id' serializer_class = CourseTeamSerializer @@ -427,6 +443,8 @@ class TopicListView(GenericAPIView): **Response Values for GET** + If the user is not logged in, a 401 error is returned. + If the course_id is not given or an unsupported value is passed for order_by, returns a 400 error. @@ -458,7 +476,7 @@ class TopicListView(GenericAPIView): """ - authentication_classes = (SessionAuthentication, OAuth2Authentication) + authentication_classes = (OAuth2Authentication, SessionAuthentication) permission_classes = (permissions.IsAuthenticated,) paginate_by = 10 @@ -472,10 +490,10 @@ class TopicListView(GenericAPIView): if course_id_string is None: return Response({ 'field_errors': { - 'course_id': { - 'developer_message': "course_id {} is not valid.".format(course_id_string), - 'user_message': _('The supplied course_id {} is not valid.').format(course_id_string) - } + 'course_id': build_api_error( + ugettext_noop("The supplied course id {course_id} is not valid."), + course_id=course_id_string + ) } }, status=status.HTTP_400_BAD_REQUEST) @@ -527,11 +545,13 @@ class TopicDetailView(APIView): **Response Values for GET** + If the user is not logged in, a 401 error is returned. + If the topic_id course_id are not given or an unsupported value is passed for order_by, returns a 400 error. - If the user is not logged in, is not enrolled in the course, or is - not course or global staff, returns a 403 error. + If the user is not enrolled in the course, or is not course or + global staff, returns a 403 error. If the course does not exist, returns a 404 error. @@ -545,7 +565,7 @@ class TopicDetailView(APIView): """ - authentication_classes = (SessionAuthentication, OAuth2Authentication) + authentication_classes = (OAuth2Authentication, SessionAuthentication) permission_classes = (permissions.IsAuthenticated,) def get(self, request, topic_id, course_id): @@ -570,3 +590,322 @@ class TopicDetailView(APIView): serializer = TopicSerializer(topics[0]) return Response(serializer.data) + + +class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): + """ + **Use Cases** + + List course team memberships or add a user to a course team. + + **Example Requests**: + + GET /api/team/v0/team_membership + + POST /api/team/v0/team_membership + + **Query Parameters for GET** + + At least one of username and team_id must be provided. + + * username: Returns membership records only for the specified user. + If the requesting user is not staff then only memberships for + teams associated with courses in which the requesting user is + enrolled are returned. + + * team_id: Returns only membership records associated with the + specified team. The requesting user must be staff or enrolled in + the course associated with the team. + + * page_size: Number of results to return per page. + + * page: Page number to retrieve. + + * expand: Comma separated list of types for which to return + expanded representations. Supports "user" and "team". + + **Response Values for GET** + + If the user is logged in and enrolled, the response contains: + + * count: The total number of memberships matching the request. + + * next: The URL to the next page of results, or null if this is the + last page. + + * previous: The URL to the previous page of results, or null if this + is the first page. + + * num_pages: The total number of pages in the result. + + * results: A list of the memberships matching the request. + + * user: The user associated with the membership. This field may + contain an expanded or collapsed representation. + + * team: The team associated with the membership. This field may + contain an expanded or collapsed representation. + + * date_joined: The date and time the membership was created. + + For all text fields, clients rendering the values should take care + to HTML escape them to avoid script injections, as the data is + stored exactly as specified. The intention is that plain text is + supported, not HTML. + + If the user is not logged in and active, a 401 error is returned. + + If neither team_id nor username are provided, a 400 error is + returned. + + If team_id is provided but the team does not exist, a 404 error is + returned. + + This endpoint uses 404 error codes to avoid leaking information + about team or user existence. Specifically, a 404 error will be + returned if a logged in user specifies a team_id for a course + they are not enrolled in. + + Additionally, when username is specified the list of returned + memberships will be filtered to memberships in teams associated + with courses that the requesting user is enrolled in. + + **Response Values for POST** + + Any logged in user enrolled in a course can enroll themselves in a + team in the course. Course and global staff can enroll any user in + a team, with a few exceptions noted below. + + If the user is not logged in and active, a 401 error is returned. + + If username and team are not provided in the posted JSON, a 400 + error is returned describing the missing fields. + + If the specified team does not exist, a 404 error is returned. + + If the user is not staff and is not enrolled in the course + associated with the team they are trying to join, or if they are + trying to add a user other than themselves to a team, a 404 error + is returned. This is to prevent leaking information about the + existence of teams and users. + + If the specified user does not exist, a 404 error is returned. + + If the user is already a member of a team in the course associated + with the team they are trying to join, a 400 error is returned. + This applies to both staff and students. + + If the user is not enrolled in the course associated with the team + they are trying to join, a 400 error is returned. This can occur + when a staff user posts a request adding another user to a team. + """ + + authentication_classes = (OAuth2Authentication, SessionAuthentication) + permission_classes = (permissions.IsAuthenticated,) + + serializer_class = MembershipSerializer + + paginate_by = 10 + paginate_by_param = 'page_size' + pagination_serializer_class = PaginationSerializer + + def get(self, request): + """GET /api/team/v0/team_membership""" + queryset = CourseTeamMembership.objects.all() + + specified_username_or_team = False + + if 'team_id' in request.QUERY_PARAMS: + specified_username_or_team = True + team_id = request.QUERY_PARAMS['team_id'] + try: + team = CourseTeam.objects.get(team_id=team_id) + except CourseTeam.DoesNotExist: + return Response(status=status.HTTP_404_NOT_FOUND) + if not has_team_api_access(request.user, team.course_id): + return Response(status=status.HTTP_404_NOT_FOUND) + queryset = queryset.filter(team__team_id=team_id) + + if 'username' in request.QUERY_PARAMS: + specified_username_or_team = True + if not request.user.is_staff: + enrolled_courses = ( + CourseEnrollment.enrollments_for_user(request.user).values_list('course_id', flat=True) + ) + staff_courses = ( + CourseAccessRole.objects.filter(user=request.user, role='staff').values_list('course_id', flat=True) + ) + valid_courses = [ + CourseKey.from_string(course_key_string) + for course_list in [enrolled_courses, staff_courses] + for course_key_string in course_list + ] + queryset = queryset.filter(team__course_id__in=valid_courses) + queryset = queryset.filter(user__username=request.QUERY_PARAMS['username']) + + if not specified_username_or_team: + return Response( + build_api_error(ugettext_noop("username or team_id must be specified.")), + status=status.HTTP_400_BAD_REQUEST + ) + + page = self.paginate_queryset(queryset) + serializer = self.get_pagination_serializer(page) + return Response(serializer.data) # pylint: disable=maybe-no-member + + def post(self, request): + """POST /api/team/v0/team_membership""" + field_errors = {} + + if 'username' not in request.DATA: + field_errors['username'] = build_api_error(ugettext_noop("Username is required.")) + + if 'team_id' not in request.DATA: + field_errors['team_id'] = build_api_error(ugettext_noop("Team id is required.")) + + if field_errors: + return Response({ + 'field_errors': field_errors, + }, status=status.HTTP_400_BAD_REQUEST) + + try: + team = CourseTeam.objects.get(team_id=request.DATA['team_id']) + except CourseTeam.DoesNotExist: + return Response(status=status.HTTP_404_NOT_FOUND) + + username = request.DATA['username'] + if not has_team_api_access(request.user, team.course_id, access_username=username): + return Response(status=status.HTTP_404_NOT_FOUND) + + try: + user = User.objects.get(username=username) + except User.DoesNotExist: + return Response(status=status.HTTP_404_NOT_FOUND) + + try: + membership = team.add_user(user) + except AlreadyOnTeamInCourse: + return Response( + build_api_error( + ugettext_noop("The user {username} is already a member of a team in this course."), + username=username + ), + status=status.HTTP_400_BAD_REQUEST + ) + except NotEnrolledInCourseForTeam: + return Response( + build_api_error( + ugettext_noop("The user {username} is not enrolled in the course associated with this team."), + username=username + ), + status=status.HTTP_400_BAD_REQUEST + ) + + serializer = self.get_serializer(instance=membership) + return Response(serializer.data) + + +class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView): + """ + **Use Cases** + + Gets individual course team memberships or removes a user from a course team. + + **Example Requests**: + + GET /api/team/v0/team_membership/{team_id},{username} + + DELETE /api/team/v0/team_membership/{team_id},{username} + + **Query Parameters for GET** + + * expand: Comma separated list of types for which to return + expanded representations. Supports "user" and "team". + + **Response Values for GET** + + If the user is logged in and enrolled, or is course or global staff + the response contains: + + * user: The user associated with the membership. This field may + contain an expanded or collapsed representation. + + * team: The team associated with the membership. This field may + contain an expanded or collapsed representation. + + * date_joined: The date and time the membership was created. + + For all text fields, clients rendering the values should take care + to HTML escape them to avoid script injections, as the data is + stored exactly as specified. The intention is that plain text is + supported, not HTML. + + If the user is not logged in and active, a 401 error is returned. + + If specified team does not exist, a 404 error is returned. + + If the user is logged in but is not enrolled in the course + associated with the specified team, or is not staff, a 404 error is + returned. This avoids leaking information about course or team + existence. + + If the membership does not exist, a 404 error is returned. + + **Response Values for DELETE** + + Any logged in user enrolled in a course can remove themselves from + a team in the course. Course and global staff can remove any user + from a team. Successfully deleting a membership will return a 204 + response with no content. + + If the user is not logged in and active, a 401 error is returned. + + If the specified team or username does not exist, a 404 error is + returned. + + If the user is not staff and is attempting to remove another user + from a team, a 404 error is returned. This prevents leaking + information about team and user existence. + + If the membership does not exist, a 404 error is returned. + """ + + authentication_classes = (OAuth2Authentication, SessionAuthentication) + permission_classes = (permissions.IsAuthenticated,) + + serializer_class = MembershipSerializer + + def get_team(self, team_id): + """Returns the team with team_id, or throws Http404 if it does not exist.""" + try: + return CourseTeam.objects.get(team_id=team_id) + except CourseTeam.DoesNotExist: + raise Http404 + + def get_membership(self, username, team): + """Returns the membership for the given user and team, or throws Http404 if it does not exist.""" + try: + return CourseTeamMembership.objects.get(user__username=username, team=team) + except CourseTeamMembership.DoesNotExist: + raise Http404 + + def get(self, request, team_id, username): + """GET /api/team/v0/team_membership/{team_id},{username}""" + team = self.get_team(team_id) + if not has_team_api_access(request.user, team.course_id): + return Response(status=status.HTTP_404_NOT_FOUND) + + membership = self.get_membership(username, team) + + serializer = self.get_serializer(instance=membership) + return Response(serializer.data) + + def delete(self, request, team_id, username): + """DELETE /api/team/v0/team_membership/{team_id},{username}""" + team = self.get_team(team_id) + if has_team_api_access(request.user, team.course_id, access_username=username): + membership = self.get_membership(username, team) + membership.delete() + return Response(status=status.HTTP_204_NO_CONTENT) + else: + return Response(status=status.HTTP_404_NOT_FOUND) diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index c14838c1cd..ca1c85740f 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/openedx/core/lib/api/view_utils.py @@ -70,6 +70,16 @@ class DeveloperErrorViewMixin(object): raise +class ExpandableFieldViewMixin(object): + """A view mixin to add expansion information to the serializer context for later use by an ExpandableField.""" + + def get_serializer_context(self): + """Adds expand information from query parameters to the serializer context to support expandable fields.""" + result = super(ExpandableFieldViewMixin, self).get_serializer_context() + result['expand'] = [x for x in self.request.QUERY_PARAMS.get('expand', '').split(',') if x] + return result + + def view_course_access(depth=0, access_action='load', check_for_milestones=False): """ Method decorator for an API endpoint that verifies the user has access to the course. @@ -142,6 +152,21 @@ def add_serializer_errors(serializer, data, field_errors): return field_errors +def build_api_error(message, **kwargs): + """Build an error dict corresponding to edX API conventions. + + Args: + message (string): The string to use for developer and user messages. + The user message will be translated, but for this to work message + must have already been scraped. ugettext_noop is useful for this. + **kwargs: format parameters for message + """ + return { + 'developer_message': message.format(**kwargs), + 'user_message': _(message).format(**kwargs), # pylint: disable=translation-of-non-string + } + + class RetrievePatchAPIView(RetrieveModelMixin, UpdateModelMixin, GenericAPIView): """Concrete view for retrieving and updating a model instance.