Skip to content

E
edx-platform
Commit bf577fff by Sofiya Semenova Committed by GitHub
Options

Merge pull request #15298 from edx/sofiya/preregister-cohorts 

Assign users to cohorts before registration
parents c22aecea 31d99e8d
Showing with 395 additions and 97 deletions
common/test/acceptance/tests/discussion/test_cohort_management.py
......@@ -149,8 +149,8 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin
confirmation_messages = self.cohort_management_page.get_cohort_confirmation_messages()
self.assertEqual(
[
"2 students have been added to this cohort",
"1 student was removed from " + self.manual_cohort_name
"2 learners have been added to this cohort.",
"1 learner was moved from " + self.manual_cohort_name
],
confirmation_messages
)
......@@ -217,16 +217,16 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin
self.assertEqual(
[
"0 students have been added to this cohort",
"1 student was already in the cohort"
"0 learners have been added to this cohort.",
"1 learner was already in the cohort"
],
self.cohort_management_page.get_cohort_confirmation_messages()
)
self.assertEqual(
[
"There was an error when trying to add students:",
"Unknown user: unknown_user"
"There was an error when trying to add learners:",
"Unknown username: unknown_user"
],
self.cohort_management_page.get_cohort_error_messages()
)
......
lms/djangoapps/instructor_task/tasks_helper/misc.py
......@@ -10,6 +10,7 @@ from time import time
import unicodecsv
from django.contrib.auth.models import User
from django.core.exceptions import ValidationError
from django.core.files.storage import DefaultStorage
from openassessment.data import OraAggregateData
from pytz import UTC
......@@ -137,9 +138,9 @@ def cohort_students_and_upload(_xmodule_instance_args, _entry_id, course_id, tas
# cohorts_status is a mapping from cohort_name to metadata about
# that cohort. The metadata will include information about users
# successfully added to the cohort, users not found, and a cached
# reference to the corresponding cohort object to prevent
# redundant cohort queries.
# successfully added to the cohort, users not found, Preassigned
# users, and a cached reference to the corresponding cohort object
# to prevent redundant cohort queries.
cohorts_status = {}
with DefaultStorage().open(task_input['file_name']) as f:
......@@ -152,8 +153,10 @@ def cohort_students_and_upload(_xmodule_instance_args, _entry_id, course_id, tas
if not cohorts_status.get(cohort_name):
cohorts_status[cohort_name] = {
'Cohort Name': cohort_name,
'Students Added': 0,
'Students Not Found': set()
'Learners Added': 0,
'Learners Not Found': set(),
'Invalid Email Addresses': set(),
'Preassigned Learners': set()
}
try:
cohorts_status[cohort_name]['cohort'] = CourseUserGroup.objects.get(
......@@ -170,11 +173,25 @@ def cohort_students_and_upload(_xmodule_instance_args, _entry_id, course_id, tas
continue
try:
add_user_to_cohort(cohorts_status[cohort_name]['cohort'], username_or_email)
cohorts_status[cohort_name]['Students Added'] += 1
task_progress.succeeded += 1
# If add_user_to_cohort successfully adds a user, a user object is returned.
# If a user is preassigned to a cohort, no user object is returned (we already have the email address).
(user, previous_cohort, preassigned) = add_user_to_cohort(cohorts_status[cohort_name]['cohort'], username_or_email)
if preassigned:
cohorts_status[cohort_name]['Preassigned Learners'].add(username_or_email)
task_progress.preassigned += 1
else:
cohorts_status[cohort_name]['Learners Added'] += 1
task_progress.succeeded += 1
except User.DoesNotExist:
cohorts_status[cohort_name]['Students Not Found'].add(username_or_email)
# Raised when a user with the username could not be found, and the email is not valid
cohorts_status[cohort_name]['Learners Not Found'].add(username_or_email)
task_progress.failed += 1
except ValidationError:
# Raised when a user with the username could not be found, and the email is not valid,
# but the entered string contains an "@"
# Since there is no way to know if the entered string is an invalid username or an invalid email,
# assume that a string with the "@" symbol in it is an attempt at entering an email
cohorts_status[cohort_name]['Invalid Email Addresses'].add(username_or_email)
task_progress.failed += 1
except ValueError:
# Raised when the user is already in the given cohort
......@@ -186,10 +203,12 @@ def cohort_students_and_upload(_xmodule_instance_args, _entry_id, course_id, tas
task_progress.update_task_state(extra_meta=current_step)
# Filter the output of `add_users_to_cohorts` in order to upload the result.
output_header = ['Cohort Name', 'Exists', 'Students Added', 'Students Not Found']
output_header = ['Cohort Name', 'Exists', 'Learners Added', 'Learners Not Found', 'Invalid Email Addresses', 'Preassigned Learners']
output_rows = [
[
','.join(status_dict.get(column_name, '')) if column_name == 'Students Not Found'
','.join(status_dict.get(column_name, '')) if (column_name == 'Learners Not Found'
or column_name == 'Invalid Email Addresses'
or column_name == 'Preassigned Learners')
else status_dict[column_name]
for column_name in output_header
]
......
lms/djangoapps/instructor_task/tasks_helper/runner.py
......@@ -26,6 +26,7 @@ class TaskProgress(object):
self.succeeded = 0
self.skipped = 0
self.failed = 0
self.preassigned = 0
def update_task_state(self, extra_meta=None):
"""
......@@ -47,6 +48,7 @@ class TaskProgress(object):
'skipped': self.skipped,
'failed': self.failed,
'total': self.total,
'preassigned': self.preassigned,
'duration_ms': int((time() - self.start_time) * 1000),
}
if extra_meta is not None:
......
lms/djangoapps/instructor_task/tests/test_tasks_helper.py
......@@ -1464,7 +1464,7 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.cohort_2 = CohortFactory(course_id=self.course.id, name='Cohort 2')
self.student_1 = self.create_student(username=u'student_1\xec', email='student_1@example.com')
self.student_2 = self.create_student(username='student_2', email='student_2@example.com')
self.csv_header_row = ['Cohort Name', 'Exists', 'Students Added', 'Students Not Found']
self.csv_header_row = ['Cohort Name', 'Exists', 'Learners Added', 'Learners Not Found', 'Invalid Email Addresses', 'Preassigned Learners']
def _cohort_students_and_upload(self, csv_data):
"""
......@@ -1485,8 +1485,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
......@@ -1500,8 +1500,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
......@@ -1515,8 +1515,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
......@@ -1536,8 +1536,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
......@@ -1546,13 +1546,11 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
result = self._cohort_students_and_upload(
'username,email,cohort\n'
'Invalid,,Cohort 1\n'
'student_2,also_fake@bad.com,Cohort 2'
)
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 0, 'failed': 2}, result)
self.assertDictContainsSubset({'total': 1, 'attempted': 1, 'succeeded': 0, 'failed': 1}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', 'Invalid'])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '0', 'also_fake@bad.com'])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', 'Invalid', '', ''])),
],
verify_order=False
)
......@@ -1566,8 +1564,35 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 1, 'failed': 1}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Does Not Exist', 'False', '0', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Does Not Exist', 'False', '0', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
def test_preassigned_user(self):
result = self._cohort_students_and_upload(
'username,email,cohort\n'
',example_email@example.com,Cohort 1'
)
self.assertDictContainsSubset({'total': 1, 'attempted': 1, 'succeeded': 0, 'failed': 0},
result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', '', '', 'example_email@example.com'])),
],
verify_order=False
)
def test_invalid_email(self):
result = self._cohort_students_and_upload(
'username,email,cohort\n'
',student_1@,Cohort 1\n'
)
self.assertDictContainsSubset({'total': 1, 'attempted': 1, 'succeeded': 0, 'failed': 1}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', '', 'student_1@', ''])),
],
verify_order=False
)
......@@ -1592,7 +1617,7 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 0, 'failed': 2}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['', 'False', '0', ''])),
dict(zip(self.csv_header_row, ['', 'False', '0', '', '', ''])),
],
verify_order=False
)
......@@ -1616,8 +1641,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
......@@ -1634,8 +1659,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
......@@ -1654,8 +1679,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '1', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '1', '', '', ''])),
],
verify_order=False
)
......@@ -1674,8 +1699,8 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
self.assertDictContainsSubset({'total': 2, 'attempted': 2, 'skipped': 2, 'failed': 0}, result)
self.verify_rows_in_csv(
[
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '0', ''])),
dict(zip(self.csv_header_row, ['Cohort 1', 'True', '0', '', '', ''])),
dict(zip(self.csv_header_row, ['Cohort 2', 'True', '0', '', '', ''])),
],
verify_order=False
)
......
lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore
......@@ -2,26 +2,27 @@
<header class="cohort-management-group-header"></header>
<ul class="wrapper-tabs">
<li class="tab tab-manage_students is-selected" data-tab="manage_students"><button type="button" class="toggle-button"><span class="sr"><%- gettext('Selected tab') %> </span><%- gettext("Manage Students") %></button></li>
<li class="tab tab-manage_students is-selected" data-tab="manage_students"><button type="button" class="toggle-button"><span class="sr"><%- gettext('Selected tab') %> </span><%- gettext("Manage Learners") %></button></li>
<li class="tab tab-settings" data-tab="settings"><button type="button" class="toggle-button"><%- gettext("Settings") %></button></li>
</ul>
<div class="cohort-management-group-add tab-content tab-content-manage_students" tabindex="-1">
<form action="" method="post" id="cohort-management-group-add-form" class="cohort-management-group-add-form">
<h4 class="hd hd-3 form-title"><%- gettext('Add students to this cohort') %></h4>
<h4 class="hd hd-3 form-title"><%- gettext('Add learners to this cohort') %></h4>
<div class="form-introduction">
<p><%- gettext('Note: Students can be in only one cohort. Adding students to this group overrides any previous group assignment.') %></p>
<p><%- gettext('Note: Learners can be in only one cohort. Adding learners to this group overrides any previous group assignment.') %></p>
</div>
<div class="cohort-confirmations" aria-live="polite" tabindex="-1"></div>
<div class="cohort-preassigned" aria-live="polite" tabindex="-1"></div>
<div class="cohort-errors" aria-live="polite" tabindex="-1"></div>
<div class="form-fields">
<div class="field field-textarea is-required">
<label for="cohort-management-group-add-students" class="label">
<%- gettext('Enter email addresses and/or usernames, separated by new lines or commas, for the students you want to add. *') %>
<%- gettext('Enter email addresses and/or usernames, separated by new lines or commas, for the learners you want to add. *') %>
<span class="sr"><%- gettext('(Required Field)') %></span>
</label>
<textarea name="cohort-management-group-add-students" id="cohort-management-group-add-students"
......@@ -35,7 +36,7 @@
<div class="form-actions">
<button class="form-submit button action-primary action-view">
<span class="button-icon icon fa fa-plus" aria-hidden="true"></span> <%- gettext('Add Students') %>
<span class="button-icon icon fa fa-plus" aria-hidden="true"></span> <%- gettext('Add Learners') %>
</button>
</div>
</form>
......
lms/templates/instructor/instructor_dashboard_2/cohorts.underscore
......@@ -34,14 +34,14 @@
<hr class="divider divider-lv1" />
<!-- Uploading a CSV file of cohort assignments. -->
<button class="toggle-cohort-management-secondary" data-href="#cohort-management-file-upload"><%- gettext('Assign students to cohorts by uploading a CSV file') %></button>
<button class="toggle-cohort-management-secondary" data-href="#cohort-management-file-upload"><%- gettext('Assign learners to cohorts by uploading a CSV file') %></button>
<div class="cohort-management-file-upload csv-upload hidden" id="cohort-management-file-upload" tabindex="-1"></div>
<div class="cohort-management-supplemental">
<p class="">
<span class="icon fa fa-info-circle" aria-hidden="true"></span>
<%= HtmlUtils.interpolateHtml(
gettext('To review student cohort assignments or see the results of uploading a CSV file, download course profile information or cohort results on the {link_start}Data Download{link_end} page.'),
gettext('To review learner cohort assignments or see the results of uploading a CSV file, download course profile information or cohort results on the {link_start}Data Download{link_end} page.'),
{
link_start: HtmlUtils.HTML('<button type="button" class="btn-link link-cross-reference" data-section="data_download">'),
link_end: HtmlUtils.HTML('</button>')
......
openedx/core/djangoapps/course_groups/cohorts.py
......@@ -6,14 +6,16 @@ forums, and to the cohort admin views.
import logging
import random
import request_cache
from courseware import courses
from django.contrib.auth.models import User
from django.core.exceptions import ValidationError
from django.core.validators import validate_email
from django.db import IntegrityError, transaction
from django.db.models.signals import m2m_changed, post_save
from django.dispatch import receiver
from django.http import Http404
from django.utils.translation import ugettext as _
import request_cache
from courseware import courses
from eventtracking import tracker
from request_cache.middleware import request_cached
from student.models import get_user_by_username_or_email
......@@ -23,7 +25,8 @@ from .models import (
CourseCohort,
CourseCohortsSettings,
CourseUserGroup,
CourseUserGroupPartitionGroup
CourseUserGroupPartitionGroup,
UnregisteredLearnerCohortAssignments
)
log = logging.getLogger(__name__)
......@@ -241,10 +244,22 @@ def get_cohort(user, course_key, assign=True, use_cached=False):
# Otherwise assign the user a cohort.
try:
with transaction.atomic():
# If learner has been pre-registered in a cohort, get that cohort. Otherwise assign to a random cohort.
course_user_group = None
for assignment in UnregisteredLearnerCohortAssignments.objects.filter(email=user.email, course_id=course_key):
course_user_group = assignment.course_user_group
unregistered_learner = assignment
if course_user_group:
unregistered_learner.delete()
else:
course_user_group = get_random_cohort(course_key)
membership = CohortMembership.objects.create(
user=user,
course_user_group=get_random_cohort(course_key)
course_user_group=course_user_group,
)
return cache.setdefault(cache_key, membership.course_user_group)
except IntegrityError as integrity_error:
# An IntegrityError is raised when multiple workers attempt to
......@@ -423,28 +438,65 @@ def add_user_to_cohort(cohort, username_or_email):
username_or_email: string. Treated as email if has '@'
Returns:
Tuple of User object and string (or None) indicating previous cohort
User object (or None if the email address is preassigned),
string (or None) indicating previous cohort,
and whether the user is a preassigned user or not
Raises:
User.DoesNotExist if can't find user.
User.DoesNotExist if can't find user. However, if a valid email is provided for the user, it is stored
in a database so that the user can be added to the cohort if they eventually enroll in the course.
ValueError if user already present in this cohort.
ValidationError if an invalid email address is entered.
User.DoesNotExist if a user could not be found.
"""
user = get_user_by_username_or_email(username_or_email)
try:
user = get_user_by_username_or_email(username_or_email)
membership = CohortMembership(course_user_group=cohort, user=user)
membership.save() # This will handle both cases, creation and updating, of a CohortMembership for this user.
membership = CohortMembership(course_user_group=cohort, user=user)
membership.save() # This will handle both cases, creation and updating, of a CohortMembership for this user.
tracker.emit(
"edx.cohort.user_add_requested",
{
"user_id": user.id,
"cohort_id": cohort.id,
"cohort_name": cohort.name,
"previous_cohort_id": membership.previous_cohort_id,
"previous_cohort_name": membership.previous_cohort_name,
}
)
return (user, membership.previous_cohort_name)
tracker.emit(
"edx.cohort.user_add_requested",
{
"user_id": user.id,
"cohort_id": cohort.id,
"cohort_name": cohort.name,
"previous_cohort_id": membership.previous_cohort_id,
"previous_cohort_name": membership.previous_cohort_name,
}
)
return (user, membership.previous_cohort_name, False)
except User.DoesNotExist as ex:
# If username_or_email is an email address, store in database.
try:
validate_email(username_or_email)
try:
assignment = UnregisteredLearnerCohortAssignments.objects.get(
email=username_or_email, course_id=cohort.course_id
)
assignment.course_user_group = cohort
assignment.save()
except UnregisteredLearnerCohortAssignments.DoesNotExist:
assignment = UnregisteredLearnerCohortAssignments.objects.create(
course_user_group=cohort, email=username_or_email, course_id=cohort.course_id
)
tracker.emit(
"edx.cohort.email_address_preassigned",
{
"user_email": assignment.email,
"cohort_id": cohort.id,
"cohort_name": cohort.name,
}
)
return (None, None, True)
except ValidationError as invalid:
if "@" in username_or_email:
raise invalid
else:
raise ex
def get_group_info_for_cohort(cohort, use_cached=False):
......
openedx/core/djangoapps/course_groups/migrations/0003_auto_20170609_1455.py 0 → 100644
# -*- coding: utf-8 -*-
from __future__ import unicode_literals
from django.db import migrations, models
import openedx.core.djangoapps.xmodule_django.models
class Migration(migrations.Migration):
dependencies = [
('course_groups', '0002_change_inline_default_cohort_value'),
]
operations = [
migrations.CreateModel(
name='UnregisteredLearnerCohortAssignments',
fields=[
('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
('email', models.CharField(db_index=True, max_length=255, blank=True)),
('course_id', openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255)),
('course_user_group', models.ForeignKey(to='course_groups.CourseUserGroup')),
],
),
migrations.AlterUniqueTogether(
name='unregisteredlearnercohortassignments',
unique_together=set([('course_id', 'email')]),
),
]
openedx/core/djangoapps/course_groups/models.py
......@@ -10,7 +10,6 @@ from django.core.exceptions import ValidationError
from django.db import models, transaction
from django.db.models.signals import pre_delete
from django.dispatch import receiver
from openedx.core.djangoapps.xmodule_django.models import CourseKeyField
from util.db import outer_atomic
......@@ -236,3 +235,13 @@ class CourseCohort(models.Model):
)
return course_cohort
class UnregisteredLearnerCohortAssignments(models.Model):
class Meta(object):
unique_together = (('course_id', 'email'), )
course_user_group = models.ForeignKey(CourseUserGroup)
email = models.CharField(blank=True, max_length=255, db_index=True)
course_id = CourseKeyField(max_length=255)
openedx/core/djangoapps/course_groups/tests/test_cohorts.py
......@@ -258,6 +258,64 @@ class TestCohorts(ModuleStoreTestCase):
"other_user should be assigned to the default cohort"
)
def test_get_cohort_preassigned_user(self):
"""
When an email address is added to a cohort and a user signs up for the course with that email address,
the user should automatically be added to that cohort and not a random cohort.
"""
course = modulestore().get_course(self.toy_course_key)
cohort = CohortFactory(course_id=course.id, name="TestCohort", users=[])
cohort2 = CohortFactory(course_id=course.id, name="RandomCohort", users=[])
config_course_cohorts(course, is_cohorted=True)
# Add email address to the cohort
(user, previous_cohort, prereg) = cohorts.add_user_to_cohort(cohort, "email@example.com")
self.assertEquals(
(user, previous_cohort, prereg),
(None, None, True)
)
# Create user with this email address
user = UserFactory(username="test", email="email@example.com")
self.assertEquals(
cohorts.get_cohort(user, course.id).id,
cohort.id,
"User should be assigned to the right cohort"
)
def test_get_cohort_multiple_preassignments(self):
"""
When an email address is added to multiple cohorts, the last cohort assignment should be respected.
Then, when a user signs up for the course with that email address,
the user should automatically be added to that cohort and not a random cohort.
"""
course = modulestore().get_course(self.toy_course_key)
cohort = CohortFactory(course_id=course.id, name="TestCohort", users=[])
cohort2 = CohortFactory(course_id=course.id, name="RandomCohort", users=[])
config_course_cohorts(course, is_cohorted=True)
# Add email address to the first cohort
(user, previous_cohort, prereg) = cohorts.add_user_to_cohort(cohort, "email@example.com")
self.assertEquals(
(user, previous_cohort, prereg),
(None, None, True)
)
# Add email address to the second cohort
(user, previous_cohort, prereg) = cohorts.add_user_to_cohort(cohort2, "email@example.com")
self.assertEquals(
(user, previous_cohort, prereg),
(None, None, True)
)
# Create user with this email address
user = UserFactory(username="test", email="email@example.com")
self.assertEquals(
cohorts.get_cohort(user, course.id).id,
cohort2.id,
"User should be assigned to the right cohort"
)
@ddt.data(
(True, 2),
(False, 6),
......@@ -549,7 +607,7 @@ class TestCohorts(ModuleStoreTestCase):
# We shouldn't get back a previous cohort, since the user wasn't in one
self.assertEqual(
cohorts.add_user_to_cohort(first_cohort, "Username"),
(course_user, None)
(course_user, None, False)
)
mock_tracker.emit.assert_any_call(
"edx.cohort.user_add_requested",
......@@ -565,7 +623,7 @@ class TestCohorts(ModuleStoreTestCase):
# another
self.assertEqual(
cohorts.add_user_to_cohort(second_cohort, "Username"),
(course_user, "FirstCohort")
(course_user, "FirstCohort", False)
)
mock_tracker.emit.assert_any_call(
"edx.cohort.user_add_requested",
......@@ -577,6 +635,21 @@ class TestCohorts(ModuleStoreTestCase):
"previous_cohort_name": first_cohort.name,
}
)
# Should preregister email address for a cohort if an email address
# not associated with a user is added
(user, previous_cohort, prereg) = cohorts.add_user_to_cohort(first_cohort, "new_email@example.com")
self.assertEqual(
(user, previous_cohort, prereg),
(None, None, True)
)
mock_tracker.emit.assert_any_call(
"edx.cohort.email_address_preassigned",
{
"user_email": "new_email@example.com",
"cohort_id": first_cohort.id,
"cohort_name": first_cohort.name,
}
)
# Error cases
# Should get ValueError if user already in cohort
self.assertRaises(
......
openedx/core/djangoapps/course_groups/tests/test_views.py
......@@ -747,7 +747,7 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
return json.loads(response.content)
def verify_added_users_to_cohort(self, response_dict, cohort, course, expected_added, expected_changed,
expected_present, expected_unknown):
expected_present, expected_unknown, expected_preassigned, expected_invalid):
"""
Check that add_users_to_cohort returned the expected response and has
the expected side effects.
......@@ -757,6 +757,8 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
`expected_present` is a list of (user, email/username) tuples where
email/username corresponds to the input
`expected_unknown` is a list of strings corresponding to the input
'expected_preassigned' is a list of email addresses
'expected_invalid' is a list of email addresses
"""
self.assertTrue(response_dict.get("success"))
self.assertEqual(
......@@ -782,6 +784,8 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
[username_or_email for (_, username_or_email) in expected_present]
)
self.assertEqual(response_dict.get("unknown"), expected_unknown)
self.assertEqual(response_dict.get("invalid"), expected_invalid)
self.assertEqual(response_dict.get("preassigned"), expected_preassigned)
for user in expected_added + [user for (user, _) in expected_changed + expected_present]:
self.assertEqual(
CourseUserGroup.objects.get(
......@@ -815,7 +819,9 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
expected_added=[],
expected_changed=[],
expected_present=[],
expected_unknown=[]
expected_preassigned=[],
expected_unknown=[],
expected_invalid=[]
)
def test_only_added(self):
......@@ -834,7 +840,9 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
expected_added=self.cohortless_users,
expected_changed=[],
expected_present=[],
expected_unknown=[]
expected_preassigned=[],
expected_unknown=[],
expected_invalid=[]
)
def test_only_changed(self):
......@@ -856,7 +864,9 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
[(user, self.cohort3.name) for user in self.cohort3_users]
),
expected_present=[],
expected_unknown=[]
expected_preassigned=[],
expected_unknown=[],
expected_invalid=[]
)
def test_only_present(self):
......@@ -876,7 +886,9 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
expected_added=[],
expected_changed=[],
expected_present=[(user, user.username) for user in self.cohort1_users],
expected_unknown=[]
expected_preassigned=[],
expected_unknown=[],
expected_invalid=[]
)
def test_only_unknown(self):
......@@ -896,7 +908,54 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
expected_added=[],
expected_changed=[],
expected_present=[],
expected_unknown=usernames
expected_preassigned=[],
expected_unknown=usernames,
expected_invalid=[]
)
def test_preassigned_users(self):
"""
Verify that email addresses can be preassigned for a cohort if the user associated with that email
address does not yet exist.
"""
email_addresses = ["email@example.com", "email2@example.com", "email3@example.com"]
response_dict = self.request_add_users_to_cohort(
",".join(email_addresses),
self.cohort1,
self.course
)
self.verify_added_users_to_cohort(
response_dict,
self.cohort1,
self.course,
expected_added=[],
expected_changed=[],
expected_present=[],
expected_preassigned=email_addresses,
expected_unknown=[],
expected_invalid=[]
)
def test_invalid_email_addresses(self):
"""
Verify that invalid email addresses return an error.
"""
email_addresses = ["email@", "@email", "invalid@email."]
response_dict = self.request_add_users_to_cohort(
",".join(email_addresses),
self.cohort1,
self.course
)
self.verify_added_users_to_cohort(
response_dict,
self.cohort1,
self.course,
expected_added=[],
expected_changed=[],
expected_present=[],
expected_preassigned=[],
expected_unknown=[],
expected_invalid=email_addresses
)
def check_user_count(self, expected_count, cohort):
......@@ -915,10 +974,12 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
Test all adding conditions together.
"""
unknowns = ["unknown_user{}".format(i) for i in range(3)]
valid_emails = ["email@example.com", "email2@example.com", "email3@example.com"]
new_users = self.cohortless_users + self.cohort1_users + self.cohort2_users + self.cohort3_users
response_dict = self.request_add_users_to_cohort(
",".join(
unknowns +
valid_emails +
[
user.username
for user in new_users
......@@ -940,20 +1001,26 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
[(user, self.cohort3.name) for user in self.cohort3_users]
),
expected_present=[(user, user.username) for user in self.cohort1_users],
expected_unknown=unknowns
expected_preassigned=valid_emails,
expected_unknown=unknowns,
expected_invalid=[]
)
def test_emails(self):
"""
Verify that we can use emails to identify users.
Expect unknown email address not associated with an account to be preassigned.
Expect unknown user (neither an email address nor a username) to not be added.
"""
unknown = "unknown_user@example.com"
valid_email_no_account = "unknown_user@example.com"
unknown_user = "unknown"
response_dict = self.request_add_users_to_cohort(
",".join([
self.cohort1_users[0].email,
self.cohort2_users[0].email,
self.cohortless_users[0].email,
unknown
valid_email_no_account,
unknown_user
]),
self.cohort1,
self.course
......@@ -965,7 +1032,9 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
expected_added=[self.cohortless_users[0]],
expected_changed=[(self.cohort2_users[0], self.cohort2.name)],
expected_present=[(self.cohort1_users[0], self.cohort1_users[0].email)],
expected_unknown=[unknown]
expected_preassigned=[valid_email_no_account],
expected_unknown=[unknown_user],
expected_invalid=[]
)
def test_delimiters(self):
......@@ -991,7 +1060,9 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
expected_added=[self.cohortless_users[0]],
expected_changed=[(self.cohort2_users[0], self.cohort2.name)],
expected_present=[(self.cohort1_users[0], self.cohort1_users[0].username)],
expected_unknown=[unknown]
expected_preassigned=[],
expected_unknown=[unknown],
expected_invalid=[]
)
def test_can_cohort_unenrolled_users(self):
......@@ -1018,7 +1089,9 @@ class AddUsersToCohortTestCase(CohortViewsTestCase):
expected_added=self.unenrolled_users,
expected_changed=[],
expected_present=[],
expected_unknown=[]
expected_preassigned=[],
expected_unknown=[],
expected_invalid=[]
)
def test_non_existent_cohort(self):
......
openedx/core/djangoapps/course_groups/views.py
......@@ -5,9 +5,9 @@ Views related to course groups functionality.
import logging
import re
from courseware.courses import get_course_with_access
from django.contrib.auth.decorators import login_required
from django.contrib.auth.models import User
from django.core.exceptions import ValidationError
from django.core.paginator import EmptyPage, Paginator
from django.core.urlresolvers import reverse
from django.db import transaction
......@@ -15,9 +15,13 @@ from django.http import Http404, HttpResponseBadRequest
from django.utils.translation import ugettext
from django.views.decorators.csrf import ensure_csrf_cookie
from django.views.decorators.http import require_http_methods, require_POST
from edxmako.shortcuts import render_to_response
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from courseware.courses import get_course_with_access
from edxmako.shortcuts import render_to_response
from lms.djangoapps.django_comment_client.constants import TYPE_ENTRY
from lms.djangoapps.django_comment_client.utils import get_discussion_categories_ids, get_discussion_category_map
from util.json_request import JsonResponse, expect_json
from . import cohorts
......@@ -267,7 +271,9 @@ def add_users_to_cohort(request, course_key_string, cohort_id):
'email': ...,
'previous_cohort': ...}, ...],
'present': [str1, str2, ...], # already there
'unknown': [str1, str2, ...]}
'unknown': [str1, str2, ...],
'preassigned': [str1, str2, ...],
'invalid': [str1, str2, ...]}
Raises Http404 if the cohort cannot be found for the given course.
"""
......@@ -288,31 +294,41 @@ def add_users_to_cohort(request, course_key_string, cohort_id):
changed = []
present = []
unknown = []
preassigned = []
invalid = []
for username_or_email in split_by_comma_and_whitespace(users):
if not username_or_email:
continue
try:
(user, previous_cohort) = cohorts.add_user_to_cohort(cohort, username_or_email)
info = {
'username': user.username,
'email': user.email,
}
if previous_cohort:
info['previous_cohort'] = previous_cohort
# A user object is only returned by add_user_to_cohort if the user already exists.
(user, previous_cohort, preassignedCohort) = cohorts.add_user_to_cohort(cohort, username_or_email)
if preassignedCohort:
preassigned.append(username_or_email)
elif previous_cohort:
info = {'email': user.email,
'previous_cohort': previous_cohort,
'username': user.username}
changed.append(info)
else:
info = {'username': user.username,
'email': user.email}
added.append(info)
except ValueError:
present.append(username_or_email)
except User.DoesNotExist:
unknown.append(username_or_email)
except ValidationError:
invalid.append(username_or_email)
except ValueError:
present.append(username_or_email)
return json_http_response({'success': True,
'added': added,
'changed': changed,
'present': present,
'unknown': unknown})
'unknown': unknown,
'preassigned': preassigned,
'invalid': invalid})
@ensure_csrf_cookie
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment