diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9de2890c37..fecccafdca 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,9 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Studio: Studio course authors (both instructors and staff) will be auto-enrolled +for their courses so that "View Live" works. + Common: Added ratelimiting to our authentication backend. Common: Add additional logging to cover login attempts and logouts. diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index d94b57ebd3..96ea9556b2 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -49,7 +49,7 @@ import datetime from pytz import UTC from uuid import uuid4 from pymongo import MongoClient - +from student.views import is_enrolled_in_course TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex @@ -1041,12 +1041,18 @@ class ContentStoreTest(ModuleStoreTestCase): data = parse_json(resp) self.assertNotIn('ErrMsg', data) self.assertEqual(data['id'], 'i4x://MITx/{0}/course/2013_Spring'.format(test_course_data['number'])) + # Verify that the creator is now registered in the course. + self.assertTrue(is_enrolled_in_course(self.user, self._get_course_id(test_course_data))) return test_course_data def test_create_course_check_forum_seeding(self): """Test new course creation and verify forum seeding """ test_course_data = self.assert_created_course(number_suffix=uuid4().hex) - self.assertTrue(are_permissions_roles_seeded('MITx/{0}/2013_Spring'.format(test_course_data['number']))) + self.assertTrue(are_permissions_roles_seeded(self._get_course_id(test_course_data))) + + def _get_course_id(self, test_course_data): + """Returns the course ID (org/number/run).""" + return "{org}/{number}/{run}".format(**test_course_data) def test_create_course_duplicate_course(self): """Test new course creation - error path""" @@ -1057,10 +1063,15 @@ class ContentStoreTest(ModuleStoreTestCase): """ Checks that the course did not get created """ + course_id = self._get_course_id(self.course_data) + initially_enrolled = is_enrolled_in_course(self.user, course_id) resp = self.client.post(reverse('create_new_course'), self.course_data) self.assertEqual(resp.status_code, 200) data = parse_json(resp) self.assertEqual(data['ErrMsg'], error_message) + # One test case involves trying to create the same course twice. Hence for that course, + # the user will be enrolled. In the other cases, initially_enrolled will be False. + self.assertEqual(initially_enrolled, is_enrolled_in_course(self.user, course_id)) def test_create_course_duplicate_number(self): """Test new course creation - error path""" diff --git a/cms/djangoapps/contentstore/tests/test_users.py b/cms/djangoapps/contentstore/tests/test_users.py index 4b9dcf487f..a9216da612 100644 --- a/cms/djangoapps/contentstore/tests/test_users.py +++ b/cms/djangoapps/contentstore/tests/test_users.py @@ -1,8 +1,12 @@ +""" +Tests for contentstore/views/user.py. +""" import json from .utils import CourseTestCase from django.contrib.auth.models import User, Group from django.core.urlresolvers import reverse from auth.authz import get_course_groupname_for_role +from student.views import is_enrolled_in_course class UsersTestCase(CourseTestCase): @@ -90,6 +94,7 @@ class UsersTestCase(CourseTestCase): # no content: should not be in any roles self.assertNotIn(self.staff_groupname, groups) self.assertNotIn(self.inst_groupname, groups) + self.assert_not_enrolled() def test_detail_post_staff(self): resp = self.client.post( @@ -104,6 +109,7 @@ class UsersTestCase(CourseTestCase): groups = [g.name for g in ext_user.groups.all()] self.assertIn(self.staff_groupname, groups) self.assertNotIn(self.inst_groupname, groups) + self.assert_enrolled() def test_detail_post_staff_other_inst(self): inst_group, _ = Group.objects.get_or_create(name=self.inst_groupname) @@ -122,6 +128,7 @@ class UsersTestCase(CourseTestCase): groups = [g.name for g in ext_user.groups.all()] self.assertIn(self.staff_groupname, groups) self.assertNotIn(self.inst_groupname, groups) + self.assert_enrolled() # check that other user is unchanged user = User.objects.get(email=self.user.email) groups = [g.name for g in user.groups.all()] @@ -141,6 +148,7 @@ class UsersTestCase(CourseTestCase): groups = [g.name for g in ext_user.groups.all()] self.assertNotIn(self.staff_groupname, groups) self.assertIn(self.inst_groupname, groups) + self.assert_enrolled() def test_detail_post_missing_role(self): resp = self.client.post( @@ -152,6 +160,7 @@ class UsersTestCase(CourseTestCase): self.assert4XX(resp.status_code) result = json.loads(resp.content) self.assertIn("error", result) + self.assert_not_enrolled() def test_detail_post_bad_json(self): resp = self.client.post( @@ -163,6 +172,7 @@ class UsersTestCase(CourseTestCase): self.assert4XX(resp.status_code) result = json.loads(resp.content) self.assertIn("error", result) + self.assert_not_enrolled() def test_detail_post_no_json(self): resp = self.client.post( @@ -176,6 +186,7 @@ class UsersTestCase(CourseTestCase): groups = [g.name for g in ext_user.groups.all()] self.assertIn(self.staff_groupname, groups) self.assertNotIn(self.inst_groupname, groups) + self.assert_enrolled() def test_detail_delete_staff(self): group, _ = Group.objects.get_or_create(name=self.staff_groupname) @@ -317,3 +328,57 @@ class UsersTestCase(CourseTestCase): ext_user = User.objects.get(email=self.ext_user.email) groups = [g.name for g in ext_user.groups.all()] self.assertIn(self.staff_groupname, groups) + + def test_user_not_initially_enrolled(self): + # Verify that ext_user is not enrolled in the new course before being added as a staff member. + self.assert_not_enrolled() + + def test_remove_staff_does_not_unenroll(self): + # Add user with staff permissions. + self.client.post( + self.detail_url, + data=json.dumps({"role": "staff"}), + content_type="application/json", + HTTP_ACCEPT="application/json", + ) + self.assert_enrolled() + # Remove user from staff on course. Will not un-enroll them from the course. + resp = self.client.delete( + self.detail_url, + HTTP_ACCEPT="application/json", + ) + self.assert2XX(resp.status_code) + self.assert_enrolled() + + def test_staff_to_instructor_still_enrolled(self): + # Add user with staff permission. + self.client.post( + self.detail_url, + data=json.dumps({"role": "staff"}), + content_type="application/json", + HTTP_ACCEPT="application/json", + ) + self.assert_enrolled() + # Now add with instructor permission. Verify still enrolled. + resp = self.client.post( + self.detail_url, + data=json.dumps({"role": "instructor"}), + content_type="application/json", + HTTP_ACCEPT="application/json", + ) + self.assert2XX(resp.status_code) + self.assert_enrolled() + + def assert_not_enrolled(self): + """ Asserts that self.ext_user is not enrolled in self.course. """ + self.assertFalse( + is_enrolled_in_course(self.ext_user, self.course.location.course_id), + 'Did not expect ext_user to be enrolled in course' + ) + + def assert_enrolled(self): + """ Asserts that self.ext_user is enrolled in self.course. """ + self.assertTrue( + is_enrolled_in_course(self.ext_user, self.course.location.course_id), + 'User ext_user should have been enrolled in the course' + ) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index e68210dea4..8ac1d223cb 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -44,6 +44,8 @@ from .component import ( from django_comment_common.utils import seed_permissions_roles +from student.views import enroll_in_course + from xmodule.html_module import AboutDescriptor __all__ = ['course_index', 'create_new_course', 'course_info', 'course_info_updates', 'get_course_settings', @@ -162,6 +164,9 @@ def create_new_course(request): # seed the forums seed_permissions_roles(new_course.location.course_id) + # auto-enroll the course creator in the course so that "View Live" will work. + enroll_in_course(request.user, new_course.location.course_id) + return JsonResponse({'id': new_course.location.url()}) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index 40076f825b..a5c495597c 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -23,6 +23,8 @@ from course_creators.views import ( from .access import has_access +from student.views import enroll_in_course + @login_required @ensure_csrf_cookie @@ -204,6 +206,8 @@ def course_team_user(request, org, course, name, email): return JsonResponse(msg, 400) user.groups.add(groups["instructor"]) user.save() + # auto-enroll the course creator in the course so that "View Live" will work. + enroll_in_course(user, location.course_id) elif role == "staff": # if we're trying to downgrade a user from "instructor" to "staff", # make sure we have at least one other instructor in the course team. @@ -217,6 +221,9 @@ def course_team_user(request, org, course, name, email): user.groups.remove(groups["instructor"]) user.groups.add(groups["staff"]) user.save() + # auto-enroll the course creator in the course so that "View Live" will work. + enroll_in_course(user, location.course_id) + return JsonResponse() diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 3e36016316..513216ba17 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -23,6 +23,7 @@ from textwrap import dedent from student.models import unique_id_for_user from student.views import process_survey_link, _cert_info, password_reset, password_reset_confirm_wrapper +from student.views import enroll_in_course, is_enrolled_in_course from student.tests.factories import UserFactory from student.tests.test_email import mock_render_to_string COURSE_1 = 'edX/toy/2012_Fall' @@ -205,3 +206,15 @@ class CourseEndingTest(TestCase): 'show_survey_button': False, 'grade': '67' }) + + +class EnrollInCourseTest(TestCase): + """ Tests the helper method for enrolling a user in a class """ + + def test_enroll_in_course(self): + user = User.objects.create_user("joe", "joe@joe.com", "password") + user.save() + course_id = "course_id" + self.assertFalse(is_enrolled_in_course(user, course_id)) + enroll_in_course(user, course_id) + self.assertTrue(is_enrolled_in_course(user, course_id)) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 223c124e46..bc6afdce0b 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -378,7 +378,7 @@ def change_enrollment(request): "run:{0}".format(run)]) try: - enrollment, _created = CourseEnrollment.objects.get_or_create(user=user, course_id=course.id) + enroll_in_course(user, course.id) except IntegrityError: # If we've already created this enrollment in a separate transaction, # then just continue @@ -403,6 +403,23 @@ def change_enrollment(request): return HttpResponseBadRequest(_("Enrollment action is invalid")) +def enroll_in_course(user, course_id): + """ + Helper method to enroll a user in a particular class. + + It is expected that this method is called from a method which has already + verified the user authentication and access. + """ + CourseEnrollment.objects.get_or_create(user=user, course_id=course_id) + + +def is_enrolled_in_course(user, course_id): + """ + Helper method that returns whether or not the user is enrolled in a particular course. + """ + return CourseEnrollment.objects.filter(user=user, course_id=course_id).count() > 0 + + @ensure_csrf_cookie def accounts_login(request, error=""):