Merge pull request #11947 from mitocw/fix/aq/max_enrollments_and_staff

Excluded admins, staff and coach of master course from max enrollment

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):

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.
 

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')),

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

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):
-                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
+            must_enroll = False
             try:
-                email = get_valid_student_email(identifier)
+                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
             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))

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)