diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index cf5fa7bac0..2952615b4e 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -879,6 +879,33 @@ class CourseEnrollmentManager(models.Manager): return enrollment_number + def num_enrolled_in_exclude_admins(self, course_id): + """ + Returns the count of active enrollments in a course excluding instructors, staff and CCX coaches. + + Arguments: + course_id (CourseLocator): course_id to return enrollments (count). + + Returns: + int: Count of enrollments excluding staff, instructors and CCX coaches. + + """ + # To avoid circular imports. + from student.roles import CourseCcxCoachRole, CourseInstructorRole, CourseStaffRole + course_locator = course_id + + if getattr(course_id, 'ccx', None): + course_locator = course_id.to_course_locator() + + staff = CourseStaffRole(course_locator).users_with_role() + admins = CourseInstructorRole(course_locator).users_with_role() + coaches = CourseCcxCoachRole(course_locator).users_with_role() + + return super(CourseEnrollmentManager, self).get_queryset().filter( + course_id=course_id, + is_active=1, + ).exclude(user__in=staff).exclude(user__in=admins).exclude(user__in=coaches).count() + def is_course_full(self, course): """ Returns a boolean value regarding whether a course has already reached it's max enrollment @@ -886,7 +913,8 @@ class CourseEnrollmentManager(models.Manager): """ is_course_full = False if course.max_student_enrollments_allowed is not None: - is_course_full = self.num_enrolled_in(course.id) >= course.max_student_enrollments_allowed + is_course_full = self.num_enrolled_in_exclude_admins(course.id) >= course.max_student_enrollments_allowed + return is_course_full def users_enrolled_in(self, course_id): diff --git a/common/djangoapps/student/tests/test_enrollment.py b/common/djangoapps/student/tests/test_enrollment.py index 23279d370b..74d377c681 100644 --- a/common/djangoapps/student/tests/test_enrollment.py +++ b/common/djangoapps/student/tests/test_enrollment.py @@ -13,7 +13,11 @@ from xmodule.modulestore.tests.factories import CourseFactory from util.testing import UrlResetMixin from embargo.test_utils import restrict_course from student.tests.factories import UserFactory, CourseModeFactory -from student.models import CourseEnrollment +from student.models import CourseEnrollment, CourseFullError +from student.roles import ( + CourseInstructorRole, + CourseStaffRole, +) @ddt.ddt @@ -31,6 +35,7 @@ class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase): def setUpClass(cls): super(EnrollmentTest, cls).setUpClass() cls.course = CourseFactory.create() + cls.course_limited = CourseFactory.create() @patch.dict(settings.FEATURES, {'EMBARGO': True}) def setUp(self): @@ -38,7 +43,8 @@ class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase): super(EnrollmentTest, self).setUp('embargo') self.user = UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) self.client.login(username=self.USERNAME, password=self.PASSWORD) - + self.course_limited.max_student_enrollments_allowed = 1 + self.store.update_item(self.course_limited, self.user.id) self.urls = [ reverse('course_modes_choose', kwargs={'course_id': unicode(self.course.id)}) ] @@ -202,6 +208,48 @@ class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase): resp = self._change_enrollment('unenroll', course_id="edx/") self.assertEqual(resp.status_code, 400) + def test_enrollment_limit(self): + """ + Assert that in a course with max student limit set to 1, we can enroll staff and instructor along with + student. To make sure course full check excludes staff and instructors. + """ + self.assertEqual(self.course_limited.max_student_enrollments_allowed, 1) + user1 = UserFactory.create(username="tester1", email="tester1@e.com", password="test") + user2 = UserFactory.create(username="tester2", email="tester2@e.com", password="test") + + # create staff on course. + staff = UserFactory.create(username="staff", email="staff@e.com", password="test") + role = CourseStaffRole(self.course_limited.id) + role.add_users(staff) + + # create instructor on course. + instructor = UserFactory.create(username="instructor", email="instructor@e.com", password="test") + role = CourseInstructorRole(self.course_limited.id) + role.add_users(instructor) + + CourseEnrollment.enroll(staff, self.course_limited.id, check_access=True) + CourseEnrollment.enroll(instructor, self.course_limited.id, check_access=True) + + self.assertTrue( + CourseEnrollment.objects.filter(course_id=self.course_limited.id, user=staff).exists() + ) + + self.assertTrue( + CourseEnrollment.objects.filter(course_id=self.course_limited.id, user=instructor).exists() + ) + + CourseEnrollment.enroll(user1, self.course_limited.id, check_access=True) + self.assertTrue( + CourseEnrollment.objects.filter(course_id=self.course_limited.id, user=user1).exists() + ) + + with self.assertRaises(CourseFullError): + CourseEnrollment.enroll(user2, self.course_limited.id, check_access=True) + + self.assertFalse( + CourseEnrollment.objects.filter(course_id=self.course_limited.id, user=user2).exists() + ) + def _change_enrollment(self, action, course_id=None, email_opt_in=None): """Change the student's enrollment status in a course. diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 5dbb02736f..ecf7419311 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -46,7 +46,6 @@ from student.tests.factories import ( from xmodule.x_module import XModuleMixin from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, SharedModuleStoreTestCase, @@ -134,26 +133,6 @@ class TestAdminAccessCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - def make_staff(self): - """ - create staff user - """ - staff = AdminFactory.create(password="test") - role = CourseStaffRole(self.course.id) - role.add_users(staff) - - return staff - - def make_instructor(self): - """ - create staff instructor - """ - instructor = AdminFactory.create(password="test") - role = CourseInstructorRole(self.course.id) - role.add_users(instructor) - - return instructor - def test_staff_access_coach_dashboard(self): """ User is staff, should access coach dashboard. @@ -607,10 +586,14 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): # create ccx and limit the maximum amount of students that can be enrolled to 2 ccx = self.make_ccx(max_students_allowed=2) ccx_course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) + staff = self.make_staff() + instructor = self.make_instructor() + # create some users - students = [ + students = [instructor, staff, self.coach] + [ UserFactory.create(is_staff=False) for _ in range(3) ] + url = reverse( 'ccx_invite', kwargs={'course_id': ccx_course_key} @@ -621,15 +604,26 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): } response = self.client.post(url, data=data, follow=True) self.assertEqual(response.status_code, 200) - # a CcxMembership exists for the first two students but not the third + # even if course is coach can enroll staff and admins of master course into ccx self.assertTrue( - CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[0]).exists() + CourseEnrollment.objects.filter(course_id=ccx_course_key, user=instructor).exists() ) self.assertTrue( - CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[1]).exists() + CourseEnrollment.objects.filter(course_id=ccx_course_key, user=staff).exists() + ) + self.assertTrue( + CourseEnrollment.objects.filter(course_id=ccx_course_key, user=self.coach).exists() + ) + + # a CcxMembership exists for the first five students but not the sixth + self.assertTrue( + CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[3]).exists() + ) + self.assertTrue( + CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[4]).exists() ) self.assertFalse( - CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[2]).exists() + CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[5]).exists() ) def test_manage_student_enrollment_limit(self): @@ -641,11 +635,13 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): """ students_limit = 1 self.make_coach() + staff = self.make_staff() ccx = self.make_ccx(max_students_allowed=students_limit) ccx_course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) students = [ UserFactory.create(is_staff=False) for _ in range(2) ] + url = reverse( 'ccx_manage_student', kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} @@ -653,7 +649,7 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): # enroll the first student data = { 'student-action': 'add', - 'student-id': u','.join([students[0].email, ]), + 'student-id': students[0].email, } response = self.client.post(url, data=data, follow=True) self.assertEqual(response.status_code, 200) @@ -661,11 +657,12 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): self.assertTrue( CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[0]).exists() ) + # try to enroll the second student without success # enroll the first student data = { 'student-action': 'add', - 'student-id': u','.join([students[1].email, ]), + 'student-id': students[1].email, } response = self.client.post(url, data=data, follow=True) self.assertEqual(response.status_code, 200) @@ -678,6 +675,27 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): ) self.assertContains(response, error_message, status_code=200) + # try to enroll the 3rd student which is staff + data = { + 'student-action': 'add', + 'student-id': staff.email, + } + response = self.client.post(url, data=data, follow=True) + self.assertEqual(response.status_code, 200) + # staff gets enroll + self.assertTrue( + CourseEnrollment.objects.filter(course_id=ccx_course_key, user=staff).exists() + ) + + self.assertEqual(CourseEnrollment.objects.num_enrolled_in_exclude_admins(ccx_course_key), 1) + + # asert that number of enroll is still 0 because staff and instructor do not count. + CourseEnrollment.enroll(staff, self.course.id) + self.assertEqual(CourseEnrollment.objects.num_enrolled_in_exclude_admins(self.course.id), 0) + # assert that handles wrong ccx id code + ccx_course_key_fake = CCXLocator.from_course_locator(self.course.id, 55) + self.assertEqual(CourseEnrollment.objects.num_enrolled_in_exclude_admins(ccx_course_key_fake), 0) + @ddt.data( ('ccx_invite', True, 1, 'student-ids', ('enrollment-button', 'Unenroll')), ('ccx_invite', False, 0, 'student-ids', ('enrollment-button', 'Unenroll')), diff --git a/lms/djangoapps/ccx/tests/utils.py b/lms/djangoapps/ccx/tests/utils.py index 9962bddba6..1198ccf22d 100644 --- a/lms/djangoapps/ccx/tests/utils.py +++ b/lms/djangoapps/ccx/tests/utils.py @@ -6,15 +6,13 @@ import pytz from django.conf import settings -from lms.djangoapps.ccx.overrides import override_field_for_ccx -from lms.djangoapps.ccx.tests.factories import CcxFactory from student.roles import ( CourseCcxCoachRole, CourseInstructorRole, CourseStaffRole ) from student.tests.factories import ( - UserFactory, + UserFactory ) from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ( @@ -26,6 +24,9 @@ from xmodule.modulestore.tests.factories import ( ItemFactory, ) +from lms.djangoapps.ccx.overrides import override_field_for_ccx +from lms.djangoapps.ccx.tests.factories import CcxFactory + class CcxTestCase(SharedModuleStoreTestCase): """ @@ -78,7 +79,6 @@ class CcxTestCase(SharedModuleStoreTestCase): Set up tests """ super(CcxTestCase, self).setUp() - # Create instructor account self.coach = UserFactory.create() # create an instance of modulestore diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 8f79b99fef..ade9d41fe3 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -32,7 +32,11 @@ from instructor.views.tools import get_student_from_identifier from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.course_structures.models import CourseStructure from student.models import CourseEnrollment, CourseEnrollmentException -from student.roles import CourseCcxCoachRole +from student.roles import ( + CourseCcxCoachRole, + CourseInstructorRole, + CourseStaffRole +) from lms.djangoapps.ccx.overrides import get_override_for_ccx from lms.djangoapps.ccx.custom_exception import CCXUserValidationException @@ -181,7 +185,7 @@ def get_ccx_by_ccx_id(course, coach, ccx_id): return ccx -def get_valid_student_email(identifier): +def get_valid_student_with_email(identifier): """ Helper function to get an user email from an identifier and validate it. @@ -195,7 +199,9 @@ def get_valid_student_email(identifier): identifier (str): Username or email of the user to enroll Returns: - str: A validated email for the user to enroll + (tuple): tuple containing: + email (str): A validated email for the user to enroll. + user (User): A valid User object or None. Raises: CCXUserValidationException: if the username is not found or the email @@ -212,10 +218,10 @@ def get_valid_student_email(identifier): validate_email(email) except ValidationError: raise CCXUserValidationException('Could not find a user with name or email "{0}" '.format(identifier)) - return email + return email, user -def ccx_students_enrolling_center(action, identifiers, email_students, course_key, email_params): +def ccx_students_enrolling_center(action, identifiers, email_students, course_key, email_params, coach): """ Function to enroll/add or unenroll/revoke students. @@ -231,6 +237,7 @@ def ccx_students_enrolling_center(action, identifiers, email_students, course_ke email_students (bool): Flag to send an email to students course_key (CCXLocator): a CCX course key email_params (dict): dictionary of settings for the email to be sent + coach (User): ccx coach Returns: list: list of error @@ -239,24 +246,32 @@ def ccx_students_enrolling_center(action, identifiers, email_students, course_ke if action == 'Enroll' or action == 'add': ccx_course_overview = CourseOverview.get_from_id(course_key) + course_locator = course_key.to_course_locator() + staff = CourseStaffRole(course_locator).users_with_role() + admins = CourseInstructorRole(course_locator).users_with_role() + for identifier in identifiers: - if CourseEnrollment.objects.is_course_full(ccx_course_overview): + must_enroll = False + try: + email, student = get_valid_student_with_email(identifier) + if student: + must_enroll = student in staff or student in admins or student == coach + except CCXUserValidationException as exp: + log.info("%s", exp) + errors.append("{0}".format(exp)) + continue + + if CourseEnrollment.objects.is_course_full(ccx_course_overview) and not must_enroll: error = _('The course is full: the limit is {max_student_enrollments_allowed}').format( max_student_enrollments_allowed=ccx_course_overview.max_student_enrollments_allowed) log.info("%s", error) errors.append(error) break - try: - email = get_valid_student_email(identifier) - except CCXUserValidationException as exp: - log.info("%s", exp) - errors.append("{0}".format(exp)) - continue enroll_email(course_key, email, auto_enroll=True, email_students=email_students, email_params=email_params) elif action == 'Unenroll' or action == 'revoke': for identifier in identifiers: try: - email = get_valid_student_email(identifier) + email, __ = get_valid_student_with_email(identifier) except CCXUserValidationException as exp: log.info("%s", exp) errors.append("{0}".format(exp)) diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 44ad0ed05b..86705f40e5 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -447,7 +447,7 @@ def ccx_invite(request, course, ccx=None): course_key = CCXLocator.from_course_locator(course.id, ccx.id) email_params = get_email_params(course, auto_enroll=True, course_key=course_key, display_name=ccx.display_name) - ccx_students_enrolling_center(action, identifiers, email_students, course_key, email_params) + ccx_students_enrolling_center(action, identifiers, email_students, course_key, email_params, ccx.coach) url = reverse('ccx_coach_dashboard', kwargs={'course_id': course_key}) return redirect(url) @@ -470,7 +470,7 @@ def ccx_student_management(request, course, ccx=None): course_key = CCXLocator.from_course_locator(course.id, ccx.id) email_params = get_email_params(course, auto_enroll=True, course_key=course_key, display_name=ccx.display_name) - errors = ccx_students_enrolling_center(action, identifiers, email_students, course_key, email_params) + errors = ccx_students_enrolling_center(action, identifiers, email_students, course_key, email_params, ccx.coach) for error_message in errors: messages.error(request, error_message)