diff --git a/common/test/acceptance/pages/lms/ccx_dashboard_page.py b/common/test/acceptance/pages/lms/ccx_dashboard_page.py index 02a5fea010..f802bb5774 100644 --- a/common/test/acceptance/pages/lms/ccx_dashboard_page.py +++ b/common/test/acceptance/pages/lms/ccx_dashboard_page.py @@ -23,7 +23,7 @@ class CoachDashboardPage(CoursePage): """ check if enrollment page in ccx dashboard is open. """ - return self.q(css='div.batch-enrollment').present + return self.q(css='div.batch-enrollment-ccx').present def fill_ccx_name_text_box(self, ccx_name): """ diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 6d88d0d814..2dc0523a4c 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -594,10 +594,8 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): self.assertEqual(response.status_code, 200) @ddt.data( - ('ccx_invite', True, 1, 'student-ids', ('enrollment-button', 'Enroll')), - ('ccx_invite', False, 0, 'student-ids', ('enrollment-button', 'Enroll')), - ('ccx_manage_student', True, 1, 'student-id', ('student-action', 'add')), - ('ccx_manage_student', False, 0, 'student-id', ('student-action', 'add')), + ('ccx-manage-students', True, 1, 'student-ids', ('enrollment-button', 'Enroll')), + ('ccx-manage-students', False, 0, 'student-ids', ('enrollment-button', 'Enroll')), ) @ddt.unpack def test_enroll_member_student(self, view_name, send_email, outbox_count, student_form_input_name, button_tuple): @@ -658,7 +656,7 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): ] url = reverse( - 'ccx_invite', + 'ccx-manage-students', kwargs={'course_id': ccx_course_key} ) data = { @@ -689,81 +687,11 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[5]).exists() ) - def test_manage_student_enrollment_limit(self): - """ - Enroll students up to the enrollment limit. - - This test is specific to one of the enrollment views: the reason is because - the view used in this test cannot perform bulk enrollments. - """ - 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)} - ) - # enroll the first student - data = { - 'student-action': 'add', - 'student-id': students[0].email, - } - response = self.client.post(url, data=data, follow=True) - self.assertEqual(response.status_code, 200) - # a CcxMembership exists for this student - 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': students[1].email, - } - response = self.client.post(url, data=data, follow=True) - self.assertEqual(response.status_code, 200) - # a CcxMembership does not exist for this student - self.assertFalse( - CourseEnrollment.objects.filter(course_id=ccx_course_key, user=students[1]).exists() - ) - error_message = 'The course is full: the limit is {students_limit}'.format( - students_limit=students_limit - ) - 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')), - ('ccx_manage_student', True, 1, 'student-id', ('student-action', 'revoke')), - ('ccx_manage_student', False, 0, 'student-id', ('student-action', 'revoke')), + ('ccx-manage-students', True, 1, 'student-ids', ('enrollment-button', 'Unenroll')), + ('ccx-manage-students', False, 0, 'student-ids', ('enrollment-button', 'Unenroll')), + ('ccx-manage-students', True, 1, 'student-id', ('student-action', 'revoke')), + ('ccx-manage-students', False, 0, 'student-id', ('student-action', 'revoke')), ) @ddt.unpack def test_unenroll_member_student(self, view_name, send_email, outbox_count, student_form_input_name, button_tuple): @@ -805,14 +733,10 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): ) @ddt.data( - ('ccx_invite', True, 1, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody@nowhere.com'), - ('ccx_invite', False, 0, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody@nowhere.com'), - ('ccx_invite', True, 0, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody'), - ('ccx_invite', False, 0, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody'), - ('ccx_manage_student', True, 0, 'student-id', ('student-action', 'add'), 'dummy_student_id'), - ('ccx_manage_student', False, 0, 'student-id', ('student-action', 'add'), 'dummy_student_id'), - ('ccx_manage_student', True, 1, 'student-id', ('student-action', 'add'), 'xyz@gmail.com'), - ('ccx_manage_student', False, 0, 'student-id', ('student-action', 'add'), 'xyz@gmail.com'), + ('ccx-manage-students', True, 1, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody@nowhere.com'), + ('ccx-manage-students', False, 0, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody@nowhere.com'), + ('ccx-manage-students', True, 0, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody'), + ('ccx-manage-students', False, 0, 'student-ids', ('enrollment-button', 'Enroll'), 'nobody'), ) @ddt.unpack def test_enroll_non_user_student( @@ -862,10 +786,10 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): ) @ddt.data( - ('ccx_invite', True, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody@nowhere.com'), - ('ccx_invite', False, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody@nowhere.com'), - ('ccx_invite', True, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody'), - ('ccx_invite', False, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody'), + ('ccx-manage-students', True, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody@nowhere.com'), + ('ccx-manage-students', False, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody@nowhere.com'), + ('ccx-manage-students', True, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody'), + ('ccx-manage-students', False, 0, 'student-ids', ('enrollment-button', 'Unenroll'), 'nobody'), ) @ddt.unpack def test_unenroll_non_user_student( diff --git a/lms/djangoapps/ccx/urls.py b/lms/djangoapps/ccx/urls.py index 5a25eb29e6..85d5d1980a 100644 --- a/lms/djangoapps/ccx/urls.py +++ b/lms/djangoapps/ccx/urls.py @@ -9,9 +9,8 @@ urlpatterns = [ url(r'^ccx_coach$', ccx.views.dashboard, name='ccx_coach_dashboard'), url(r'^create_ccx$', ccx.views.create_ccx, name='create_ccx'), url(r'^save_ccx$', ccx.views.save_ccx, name='save_ccx'), - url(r'^ccx_invite$', ccx.views.ccx_invite, name='ccx_invite'), url(r'^ccx_schedule$', ccx.views.ccx_schedule, name='ccx_schedule'), - url(r'^ccx_manage_student$', ccx.views.ccx_student_management, name='ccx_manage_student'), + url(r'^ccx-manage-students$', ccx.views.ccx_students_management, name='ccx-manage-students'), # Grade book url(r'^ccx_gradebook$', ccx.views.ccx_gradebook, name='ccx_gradebook'), diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 9a34150e5c..f940ce7ebd 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -21,12 +21,14 @@ from lms.djangoapps.ccx.models import CustomCourseForEdX from lms.djangoapps.ccx.overrides import get_override_for_ccx from lms.djangoapps.instructor.access import allow_access, list_with_level, revoke_access from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params, unenroll_email +from lms.djangoapps.instructor.views.api import _split_input_list from lms.djangoapps.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, CourseInstructorRole, CourseStaffRole + log = logging.getLogger("edx.ccx") @@ -96,6 +98,27 @@ def get_date(ccx, node, date_type=None, parent_node=None): return date +def get_enrollment_action_and_identifiers(request): + """ + Extracts action type and student identifiers from the request + on Enrollment tab of CCX Dashboard. + """ + action, identifiers = None, None + student_action = request.POST.get('student-action', None) + batch_action = request.POST.get('enrollment-button', None) + + if student_action: + action = student_action + student_id = request.POST.get('student-id', '') + identifiers = [student_id] + elif batch_action: + action = batch_action + identifiers_raw = request.POST.get('student-ids') + identifiers = _split_input_list(identifiers_raw) + + return action, identifiers + + def validate_date(year, month, day, hour, minute): """ avoid corrupting db if bad dates come in @@ -208,16 +231,10 @@ def get_valid_student_with_email(identifier): def ccx_students_enrolling_center(action, identifiers, email_students, course_key, email_params, coach): """ - Function to enroll/add or unenroll/revoke students. - - This function exists for backwards compatibility: in CCX there are - two different views to manage students that used to implement - a different logic. Now the logic has been reconciled at the point that - this function can be used by both. - The two different views can be merged after some UI refactoring. + Function to enroll or unenroll/revoke students. Arguments: - action (str): type of action to perform (add, Enroll, revoke, Unenroll) + action (str): type of action to perform (Enroll, Unenroll/revoke) identifiers (list): list of students username/email email_students (bool): Flag to send an email to students course_key (CCXLocator): a CCX course key @@ -229,7 +246,7 @@ def ccx_students_enrolling_center(action, identifiers, email_students, course_ke """ errors = [] - if action == 'Enroll' or action == 'add': + if action == 'Enroll': ccx_course_overview = CourseOverview.get_from_id(course_key) course_locator = course_key.to_course_locator() staff = CourseStaffRole(course_locator).users_with_role() diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 1a43a560fe..81e32a34bd 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -45,11 +45,11 @@ from lms.djangoapps.ccx.utils import ( get_ccx_creation_dict, get_ccx_for_coach, get_date, + get_enrollment_action_and_identifiers, parse_date, ) from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params -from lms.djangoapps.instructor.views.api import _split_input_list from lms.djangoapps.instructor.views.gradebook_api import get_grade_book_page from student.models import CourseEnrollment from student.roles import CourseCcxCoachRole @@ -459,43 +459,18 @@ def ccx_schedule(request, course, ccx=None): # pylint: disable=unused-argument @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @coach_dashboard -def ccx_invite(request, course, ccx=None): +def ccx_students_management(request, course, ccx=None): """ - Invite users to new ccx + Manage the enrollment of the students in a CCX """ if not ccx: raise Http404 - action = request.POST.get('enrollment-button') - identifiers_raw = request.POST.get('student-ids') - identifiers = _split_input_list(identifiers_raw) + action, identifiers = get_enrollment_action_and_identifiers(request) email_students = 'email-students' in request.POST course_key = CCXLocator.from_course_locator(course.id, unicode(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.coach) - - url = reverse('ccx_coach_dashboard', kwargs={'course_id': course_key}) - return redirect(url) - - -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@coach_dashboard -def ccx_student_management(request, course, ccx=None): - """ - Manage the enrollment of individual students in a CCX - """ - if not ccx: - raise Http404 - - action = request.POST.get('student-action', None) - student_id = request.POST.get('student-id', '') - email_students = 'email-students' in request.POST - identifiers = [student_id] - course_key = CCXLocator.from_course_locator(course.id, unicode(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, ccx.coach) for error_message in errors: diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index d1f5ed96ca..c0bff7fb61 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -635,6 +635,16 @@ } } } + + .batch-enrollment-ccx{ + @extend .batch-enrollment; + float: left; + width: 100%; + + textarea { + width: 100%; + } + } // Auto Enroll Csv Section .auto_enroll_csv { .enrollment_signup_button { @@ -806,6 +816,12 @@ } } } + + .member-lists-management-ccx { + @extend .member-lists-management; + float: left; + width: 100%; + } } diff --git a/lms/templates/ccx/coach_dashboard.html b/lms/templates/ccx/coach_dashboard.html index a7078a1eb2..acea0a12f2 100644 --- a/lms/templates/ccx/coach_dashboard.html +++ b/lms/templates/ccx/coach_dashboard.html @@ -112,34 +112,12 @@ from openedx.core.djangolib.js_utils import ( function setup_management_form() { - $(".member-lists-management form").on("submit", function (event) { - var target, action; - target = $(event.target); - if (target.serialize().indexOf('student-action') < 0) { - action = $('', { - type: 'hidden', - name: 'student-action', - value: 'add' - }); - target.append(action); - } - }); - - $(".member-lists-management form .add, .member-lists-management form .revoke").on("click", function(event) { + $(".member-lists-management-ccx form .revoke").on("click", function(event) { var target, form, action, studentId, selectedStudent; event.preventDefault(); target = $(event.target); form = target.parents('form').first(); - if (target.hasClass('add')) { - // adding a new student, add the student-action input and submit - action = $('', { - type: 'hidden', - name: 'student-action', - // this is untenable, tied to a translated value. Fix it. - value: 'add' - }); - form.append(action).submit(); - } else if (target.hasClass('revoke')) { + if (target.hasClass('revoke')) { // revoking access for a student, get set form values and submit // get the email address of the student, since they might not be 'enrolled' yet. selectedStudent = target.parent('td').siblings().last().text(); diff --git a/lms/templates/ccx/enrollment.html b/lms/templates/ccx/enrollment.html index 07f5cb4d08..ca6bdeff7a 100644 --- a/lms/templates/ccx/enrollment.html +++ b/lms/templates/ccx/enrollment.html @@ -4,14 +4,14 @@ from django.utils.translation import ugettext as _ from openedx.core.djangolib.markup import HTML, Text %> -

${_("Batch Enrollment")}

-
-
+

${_("Enrollment")}

+
+

- ${_("Enter email addresses and/or usernames separated by new lines or commas.")} - ${_("You will not get notification for emails that bounce, so please double-check spelling.")} + ${_("Enter one or more email addresses or usernames separated by new lines or commas.")} + ${_("Make sure you enter the information carefully. You will not receive notification for invalid usernames or email addresses.")}

@@ -61,8 +61,8 @@ from openedx.core.djangolib.markup import HTML, Text
-
-
+
+
@@ -79,9 +79,9 @@ from openedx.core.djangolib.markup import HTML, Text - - - + + + @@ -95,45 +95,6 @@ from openedx.core.djangolib.markup import HTML, Text
UsernameEmailRevoke access${_("Username")}${_("Email")}${_("Revoke access")}
-
- - - -
- - -
- -

- ${Text(_("If this option is {em_start}checked{em_end}, users who have not yet registered for {platform_name} will be automatically enrolled.")).format( - em_start=HTML(''), - em_end=HTML(''), - platform_name=settings.PLATFORM_NAME, - )} - ${Text(_("If this option is left {em_start}unchecked{em_end}, users who have not yet registered for {platform_name} will not be enrolled, but will be allowed to enroll once they make an account.")).format( - em_start=HTML(''), - em_end=HTML(''), - platform_name=settings.PLATFORM_NAME, - )} -

- ${_("Checking this box has no effect if 'Revoke' is clicked.")} -

-
-
-
- - - -
-