From 2bb790680a34dc7d2e18d15d8cb87e89abae2b48 Mon Sep 17 00:00:00 2001
From: Ibrahim <ibrahimahmed443@gmail.com>
Date: Tue, 6 Mar 2018 16:35:42 +0500
Subject: [PATCH] add role and expose reason field to Instructor Dashboard
Manual Enrollment WL-1473
---
.../0015_manualenrollmentaudit_add_role.py | 19 +++++++
common/djangoapps/student/models.py | 6 ++-
lms/djangoapps/instructor/tests/test_api.py | 50 ++++++++-----------
.../tests/views/test_instructor_dashboard.py | 45 +++++++++++++++++
lms/djangoapps/instructor/views/api.py | 23 +++++----
.../instructor/views/instructor_dashboard.py | 9 ++--
lms/envs/common.py | 4 ++
.../js/instructor_dashboard/membership.js | 16 +++---
.../sass/course/instructor/_instructor_2.scss | 7 +++
.../instructor_dashboard_2/membership.html | 23 ++++++---
10 files changed, 144 insertions(+), 58 deletions(-)
create mode 100644 common/djangoapps/student/migrations/0015_manualenrollmentaudit_add_role.py
diff --git a/common/djangoapps/student/migrations/0015_manualenrollmentaudit_add_role.py b/common/djangoapps/student/migrations/0015_manualenrollmentaudit_add_role.py
new file mode 100644
index 00000000000..9178dd66dff
--- /dev/null
+++ b/common/djangoapps/student/migrations/0015_manualenrollmentaudit_add_role.py
@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('student', '0014_courseenrollmentallowed_user'),
+ ]
+
+ operations = [
+ migrations.AddField(
+ model_name='manualenrollmentaudit',
+ name='role',
+ field=models.CharField(max_length=64, null=True, blank=True),
+ ),
+ ]
diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py
index 4e333c8c985..3ea25776988 100644
--- a/common/djangoapps/student/models.py
+++ b/common/djangoapps/student/models.py
@@ -1973,9 +1973,10 @@ class ManualEnrollmentAudit(models.Model):
time_stamp = models.DateTimeField(auto_now_add=True, null=True)
state_transition = models.CharField(max_length=255, choices=TRANSITION_STATES)
reason = models.TextField(null=True)
+ role = models.CharField(blank=True, null=True, max_length=64)
@classmethod
- def create_manual_enrollment_audit(cls, user, email, state_transition, reason, enrollment=None):
+ def create_manual_enrollment_audit(cls, user, email, state_transition, reason, enrollment=None, role=None):
"""
saves the student manual enrollment information
"""
@@ -1984,7 +1985,8 @@ class ManualEnrollmentAudit(models.Model):
enrolled_email=email,
state_transition=state_transition,
reason=reason,
- enrollment=enrollment
+ enrollment=enrollment,
+ role=role,
)
@classmethod
diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py
index 1d2ebd5170a..1eb3abfe0e3 100644
--- a/lms/djangoapps/instructor/tests/test_api.py
+++ b/lms/djangoapps/instructor/tests/test_api.py
@@ -1585,32 +1585,6 @@ class TestInstructorAPIEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTest
CourseInstructorRole(paid_course.id).add_users(self.instructor)
return paid_course
- def test_reason_field_should_not_be_empty(self):
- """
- test to check that reason field should not be empty when
- manually enrolling the students for the paid courses.
- """
- paid_course = self.create_paid_course()
- url = reverse('students_update_enrollment', kwargs={'course_id': text_type(paid_course.id)})
- params = {'identifiers': self.notregistered_email, 'action': 'enroll', 'email_students': False,
- 'auto_enroll': False}
- response = self.client.post(url, params)
- manual_enrollments = ManualEnrollmentAudit.objects.all()
- self.assertEqual(manual_enrollments.count(), 0)
-
- # test the response data
- expected = {
- "action": "enroll",
- "auto_enroll": False,
- "results": [
- {
- "error": True
- }
- ]
- }
- res_json = json.loads(response.content)
- self.assertEqual(res_json, expected)
-
def test_unenrolled_allowed_to_enroll_user(self):
"""
test to unenroll allow to enroll user.
@@ -1618,7 +1592,7 @@ class TestInstructorAPIEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTest
paid_course = self.create_paid_course()
url = reverse('students_update_enrollment', kwargs={'course_id': text_type(paid_course.id)})
params = {'identifiers': self.notregistered_email, 'action': 'enroll', 'email_students': False,
- 'auto_enroll': False, 'reason': 'testing..'}
+ 'auto_enroll': False, 'reason': 'testing..', 'role': 'Learner'}
response = self.client.post(url, params)
manual_enrollments = ManualEnrollmentAudit.objects.all()
self.assertEqual(manual_enrollments.count(), 1)
@@ -1629,7 +1603,7 @@ class TestInstructorAPIEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTest
UserFactory(email=self.notregistered_email)
url = reverse('students_update_enrollment', kwargs={'course_id': text_type(paid_course.id)})
params = {'identifiers': self.notregistered_email, 'action': 'enroll', 'email_students': False,
- 'auto_enroll': False, 'reason': 'testing'}
+ 'auto_enroll': False, 'reason': 'testing', 'role': 'Learner'}
response = self.client.post(url, params)
manual_enrollments = ManualEnrollmentAudit.objects.all()
self.assertEqual(manual_enrollments.count(), 2)
@@ -1673,7 +1647,7 @@ class TestInstructorAPIEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTest
url = reverse('students_update_enrollment', kwargs={'course_id': text_type(paid_course.id)})
params = {'identifiers': self.notregistered_email, 'action': 'unenroll', 'email_students': False,
- 'auto_enroll': False, 'reason': 'testing'}
+ 'auto_enroll': False, 'reason': 'testing', 'role': 'Learner'}
response = self.client.post(url, params)
self.assertEqual(response.status_code, 200)
@@ -1730,6 +1704,21 @@ class TestInstructorAPIEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTest
)
self.assertEqual(course_enrollment.mode, CourseMode.DEFAULT_MODE_SLUG)
+ def test_role_and_reason_are_persisted(self):
+ """
+ test that role and reason fields are persisted in the database
+ """
+ paid_course = self.create_paid_course()
+ url = reverse('students_update_enrollment', kwargs={'course_id': text_type(paid_course.id)})
+ params = {'identifiers': self.notregistered_email, 'action': 'enroll', 'email_students': False,
+ 'auto_enroll': False, 'reason': 'testing', 'role': 'Learner'}
+ response = self.client.post(url, params)
+
+ manual_enrollment = ManualEnrollmentAudit.objects.first()
+ self.assertEqual(manual_enrollment.reason, 'testing')
+ self.assertEqual(manual_enrollment.role, 'Learner')
+ self.assertEqual(response.status_code, 200)
+
def _change_student_enrollment(self, user, course, action):
"""
Helper function that posts to 'students_update_enrollment' to change
@@ -1743,7 +1732,8 @@ class TestInstructorAPIEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTest
'identifiers': user.email,
'action': action,
'email_students': True,
- 'reason': 'change user enrollment'
+ 'reason': 'change user enrollment',
+ 'role': 'Learner'
}
response = self.client.post(url, params)
self.assertEqual(response.status_code, 200)
diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py
index 9d4c291ce10..102d6c4a207 100644
--- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py
+++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py
@@ -5,6 +5,7 @@ import datetime
import ddt
from django.conf import settings
+from django.contrib.sites.models import Site
from django.core.urlresolvers import reverse
from django.test.client import RequestFactory
from django.test.utils import override_settings
@@ -20,6 +21,7 @@ from courseware.tests.factories import StaffFactory, StudentModuleFactory, UserF
from courseware.tests.helpers import LoginEnrollmentTestCase
from edxmako.shortcuts import render_to_response
from lms.djangoapps.instructor.views.gradebook_api import calculate_page_info
+from openedx.core.djangoapps.site_configuration.models import SiteConfiguration
from pyquery import PyQuery as pq
from shoppingcart.models import CourseRegCodeItem, Order, PaidCourseRegistration
from student.models import CourseEnrollment
@@ -152,6 +154,49 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssT
content('#field-course-organization b').contents()[0].strip()
)
+ def test_membership_site_configuration_role(self):
+ """
+ Verify that the role choices set via site configuration are loaded in the membership tab
+ of the instructor dashboard
+ """
+
+ configuration_values = {
+ "MANUAL_ENROLLMENT_ROLE_CHOICES": [
+ "role1",
+ "role2",
+ ]
+ }
+ site = Site.objects.first()
+ SiteConfiguration.objects.create(site=site, values=configuration_values, enabled=True)
+ url = reverse(
+ 'instructor_dashboard',
+ kwargs={
+ 'course_id': unicode(self.course_info.id)
+ }
+ )
+
+ response = self.client.get(url)
+ self.assertIn('<option value="role1">role1</option>', response.content)
+ self.assertIn('<option value="role2">role2</option>', response.content)
+
+ def test_membership_default_role(self):
+ """
+ Verify that in the absence of site configuration role choices, default values of role choices are loaded
+ in the membership tab of the instructor dashboard
+ """
+
+ url = reverse(
+ 'instructor_dashboard',
+ kwargs={
+ 'course_id': unicode(self.course_info.id)
+ }
+ )
+
+ response = self.client.get(url)
+ self.assertIn('<option value="Learner">Learner</option>', response.content)
+ self.assertIn('<option value="Support">Support</option>', response.content)
+ self.assertIn('<option value="Partner">Partner</option>', response.content)
+
def test_student_admin_staff_instructor(self):
"""
Verify that staff users are not able to see course-wide options, while still
diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py
index 0fb7cd0461c..1d1d5fbbe1c 100644
--- a/lms/djangoapps/instructor/views/api.py
+++ b/lms/djangoapps/instructor/views/api.py
@@ -628,16 +628,19 @@ def students_update_enrollment(request, course_id):
identifiers = _split_input_list(identifiers_raw)
auto_enroll = _get_boolean_param(request, 'auto_enroll')
email_students = _get_boolean_param(request, 'email_students')
- is_white_label = CourseMode.is_white_label(course_id)
reason = request.POST.get('reason')
- if is_white_label:
- if not reason:
- return JsonResponse(
- {
- 'action': action,
- 'results': [{'error': True}],
- 'auto_enroll': auto_enroll,
- }, status=400)
+ role = request.POST.get('role')
+
+ allowed_role_choices = configuration_helpers.get_value('MANUAL_ENROLLMENT_ROLE_CHOICES',
+ settings.MANUAL_ENROLLMENT_ROLE_CHOICES)
+ if role and role not in allowed_role_choices:
+ return JsonResponse(
+ {
+ 'action': action,
+ 'results': [{'error': True, 'message': 'Not a valid role choice'}],
+ 'auto_enroll': auto_enroll,
+ }, status=400)
+
enrollment_obj = None
state_transition = DEFAULT_TRANSITION_STATE
@@ -729,7 +732,7 @@ def students_update_enrollment(request, course_id):
else:
ManualEnrollmentAudit.create_manual_enrollment_audit(
- request.user, email, state_transition, reason, enrollment_obj
+ request.user, email, state_transition, reason, enrollment_obj, role
)
results.append({
'identifier': identifier,
diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py
index c1cf33e58ff..97ac3eaec35 100644
--- a/lms/djangoapps/instructor/views/instructor_dashboard.py
+++ b/lms/djangoapps/instructor/views/instructor_dashboard.py
@@ -122,7 +122,7 @@ def instructor_dashboard_2(request, course_id):
sections = [
_section_course_info(course, access),
- _section_membership(course, access, is_white_label),
+ _section_membership(course, access),
_section_cohort_management(course, access),
_section_discussions_management(course, access),
_section_student_admin(course, access),
@@ -480,16 +480,18 @@ def _section_course_info(course, access):
return section_data
-def _section_membership(course, access, is_white_label):
+def _section_membership(course, access):
""" Provide data for the corresponding dashboard section """
course_key = course.id
ccx_enabled = settings.FEATURES.get('CUSTOM_COURSES_EDX', False) and course.enable_ccx
+ enrollment_role_choices = configuration_helpers.get_value('MANUAL_ENROLLMENT_ROLE_CHOICES',
+ settings.MANUAL_ENROLLMENT_ROLE_CHOICES)
+
section_data = {
'section_key': 'membership',
'section_display_name': _('Membership'),
'access': access,
'ccx_is_enabled': ccx_enabled,
- 'is_white_label': is_white_label,
'enroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': unicode(course_key)}),
'unenroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': unicode(course_key)}),
'upload_student_csv_button_url': reverse('register_and_enroll_students', kwargs={'course_id': unicode(course_key)}),
@@ -498,6 +500,7 @@ def _section_membership(course, access, is_white_label):
'modify_access_url': reverse('modify_access', kwargs={'course_id': unicode(course_key)}),
'list_forum_members_url': reverse('list_forum_members', kwargs={'course_id': unicode(course_key)}),
'update_forum_role_membership_url': reverse('update_forum_role_membership', kwargs={'course_id': unicode(course_key)}),
+ 'enrollment_role_choices': enrollment_role_choices
}
return section_data
diff --git a/lms/envs/common.py b/lms/envs/common.py
index 0bb43f21472..c125c526d5e 100644
--- a/lms/envs/common.py
+++ b/lms/envs/common.py
@@ -67,6 +67,10 @@ LMS_ROOT_URL = "http://localhost:8000"
LMS_INTERNAL_ROOT_URL = LMS_ROOT_URL
LMS_ENROLLMENT_API_PATH = "/api/enrollment/v1/"
+# Default choices for role dropdown in the membership tab of the instructor dashboard
+# This setting is used when a site does not define its own choices via site configuration
+MANUAL_ENROLLMENT_ROLE_CHOICES = ['Learner', 'Support', 'Partner']
+
# Features
FEATURES = {
'DISPLAY_DEBUG_INFO_TO_STAFF': True,
diff --git a/lms/static/js/instructor_dashboard/membership.js b/lms/static/js/instructor_dashboard/membership.js
index 5d40391bd1c..fd8a1b69d59 100644
--- a/lms/static/js/instructor_dashboard/membership.js
+++ b/lms/static/js/instructor_dashboard/membership.js
@@ -593,8 +593,8 @@ such that the value can be defined later than this assignment (file load order).
var batchEnroll = this;
this.$container = $container;
this.$identifier_input = this.$container.find("textarea[name='student-ids']");
+ this.$role = this.$container.find("select[name='role']");
this.$enrollment_button = this.$container.find('.enrollment-button');
- this.$is_course_white_label = this.$container.find('#is_course_white_label').val();
this.$reason_field = this.$container.find("textarea[name='reason-field']");
this.$checkbox_autoenroll = this.$container.find("input[name='auto-enroll']");
this.$checkbox_emailstudents = this.$container.find("input[name='email-students']");
@@ -603,16 +603,20 @@ such that the value can be defined later than this assignment (file load order).
this.$request_response_error = this.$container.find('.request-response-error');
this.$enrollment_button.click(function(event) {
var sendData;
- if (batchEnroll.$is_course_white_label === 'True') {
- if (!batchEnroll.$reason_field.val()) {
- batchEnroll.fail_with_error(gettext('Reason field should not be left blank.'));
- return false;
- }
+ if (!batchEnroll.$reason_field.val()) {
+ batchEnroll.fail_with_error(gettext('Reason field should not be left blank.'));
+ return false;
}
+ if (!batchEnroll.$role.val()) {
+ batchEnroll.fail_with_error(gettext('Role field should not be left unselected.'));
+ return false;
+ }
+
emailStudents = batchEnroll.$checkbox_emailstudents.is(':checked');
sendData = {
action: $(event.target).data('action'),
identifiers: batchEnroll.$identifier_input.val(),
+ role: batchEnroll.$role.val(),
auto_enroll: batchEnroll.$checkbox_autoenroll.is(':checked'),
email_students: emailStudents,
reason: batchEnroll.$reason_field.val()
diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss
index c0bff7fb618..2c3df43af4c 100644
--- a/lms/static/sass/course/instructor/_instructor_2.scss
+++ b/lms/static/sass/course/instructor/_instructor_2.scss
@@ -615,6 +615,13 @@
width: 90%;
}
+ .role {
+ margin: 20px 0;
+ select {
+ margin-left: 15px;
+ }
+ }
+
input {
margin-right: ($baseline/4);
}
diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html
index 102f9ae2f6e..1c1def0fbbe 100644
--- a/lms/templates/instructor/instructor_dashboard_2/membership.html
+++ b/lms/templates/instructor/instructor_dashboard_2/membership.html
@@ -12,15 +12,24 @@ from openedx.core.djangolib.markup import HTML, Text
${_("You will not get notification for emails that bounce, so please double-check spelling.")}
<textarea rows="6" name="student-ids" placeholder="${_("Email Addresses/Usernames")}" spellcheck="false"></textarea>
</label>
- <input type="hidden" id="is_course_white_label" value="${section_data['is_white_label']}">
- % if section_data['is_white_label']:
+
+ <div class="role">
<label>
- ${_("Enter the reason why the students are to be manually enrolled or unenrolled.")}
- ${_("This cannot be left blank and will be recorded and presented in Enrollment Reports.")}
- ${_("Therefore, please give enough detail to account for this action.")}
- <textarea rows="2" id="reason-field-id" name="reason-field" placeholder="${_('Reason')}" spellcheck="false"></textarea>
+ ${_("Role of the users being enrolled.")}
+ <select name="role">
+ % for role in section_data['enrollment_role_choices']:
+ <option value="${role}">${role}</option>
+ % endfor
+ </select>
</label>
- %endif
+ </div>
+
+ <label>
+ ${_("Enter the reason why the students are to be manually enrolled or unenrolled.")}
+ ${_("This cannot be left blank and will be recorded and presented in Enrollment Reports.")}
+ ${_("Therefore, please give enough detail to account for this action.")}
+ <textarea rows="2" id="reason-field-id" name="reason-field" placeholder="${_('Reason')}" spellcheck="false"></textarea>
+ </label>
<div class="enroll-option">
<label class="has-hint">
<input type="checkbox" name="auto-enroll" id="auto-enroll" value="Auto-Enroll" checked="yes" aria-describedby="heading-batch-enrollment">
--
GitLab