Skip to content

Commit

Permalink
feat: integrate cohort assignment filter definition to cohort model
Browse files Browse the repository at this point in the history
  • Loading branch information
mariajgrimaldi committed Jun 13, 2022
1 parent 116431c commit ec69659
Show file tree
Hide file tree
Showing 5 changed files with 145 additions and 11 deletions.
15 changes: 10 additions & 5 deletions lms/static/js/groups/views/cohort_editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,16 +260,17 @@

// Show error messages.
this.undelegateViewEvents(this.errorNotifications);
numErrors = modifiedUsers.unknown.length + modifiedUsers.invalid.length;
numErrors = modifiedUsers.unknown.length + modifiedUsers.invalid.length + modifiedUsers.not_allowed.length;
if (numErrors > 0) {
createErrorDetails = function(unknownUsers, invalidEmails, showAllErrors) {
createErrorDetails = function(unknownUsers, invalidEmails, notAllowed, showAllErrors) {
var unknownErrorsShown = showAllErrors ? unknownUsers.length :
Math.min(errorLimit, unknownUsers.length);
var invalidErrorsShown = showAllErrors ? invalidEmails.length :
Math.min(errorLimit - unknownUsers.length, invalidEmails.length);
var notAllowedErrorsShown = showAllErrors ? notAllowed.length :
Math.min(errorLimit - notAllowed.length, notAllowed.length);
details = [];


for (i = 0; i < unknownErrorsShown; i++) {
details.push(interpolate_text(gettext('Unknown username: {user}'),
{user: unknownUsers[i]}));
Expand All @@ -278,6 +279,10 @@
details.push(interpolate_text(gettext('Invalid email address: {email}'),
{email: invalidEmails[i]}));
}
for (i = 0; i < notAllowedErrorsShown; i++) {
details.push(interpolate_text(gettext('Cohort assignment not allowed: {email_or_username}'),
{email_or_username: notAllowed[i]}));
}
return details;
};

Expand All @@ -286,12 +291,12 @@
'{numErrors} learners could not be added to this cohort:', numErrors),
{numErrors: numErrors}
);
details = createErrorDetails(modifiedUsers.unknown, modifiedUsers.invalid, false);
details = createErrorDetails(modifiedUsers.unknown, modifiedUsers.invalid, modifiedUsers.not_allowed, false);

errorActionCallback = function(view) {
view.model.set('actionText', null);
view.model.set('details',
createErrorDetails(modifiedUsers.unknown, modifiedUsers.invalid, true));
createErrorDetails(modifiedUsers.unknown, modifiedUsers.invalid, modifiedUsers.not_allowed, true));
view.render();
};

Expand Down
20 changes: 19 additions & 1 deletion lms/static/js/spec/groups/views/cohorts_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ define(['backbone', 'jquery', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers
var catLoversInitialCount = 123,
dogLoversInitialCount = 456,
unknownUserMessage,
notAllowedUserMessage,
invalidEmailMessage, createMockCohort, createMockCohorts, createMockContentGroups,
createMockCohortSettingsJson,
createCohortsView, cohortsView, requests, respondToRefresh, verifyMessage, verifyNoMessage,
Expand Down Expand Up @@ -210,6 +211,10 @@ define(['backbone', 'jquery', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers
return 'Invalid email address: ' + name;
};

notAllowedUserMessage = function(email) {
return 'Cohort assignment not allowed: ' + email;
};

beforeEach(function() {
setFixtures('<ul class="instructor-nav">' +
'<li class="nav-item"><button type="button" data-section="cohort_management" ' +
Expand Down Expand Up @@ -602,7 +607,7 @@ define(['backbone', 'jquery', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers
respondToAdd = function(result) {
AjaxHelpers.respondWithJson(
requests,
_.extend({unknown: [], added: [], present: [], changed: [],
_.extend({unknown: [], added: [], present: [], changed: [], not_allowed: [],
success: true, preassigned: [], invalid: []}, result)
);
};
Expand Down Expand Up @@ -670,6 +675,19 @@ define(['backbone', 'jquery', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers
);
});

it('shows an error when user assignment not allowed', function() {
createCohortsView(this, {selectCohort: 1});
addStudents('not_allowed');
AjaxHelpers.expectRequest(
requests, 'POST', '/mock_service/cohorts/1/add', 'users=not_allowed'
);
respondToAdd({not_allowed: ['not_allowed']});
respondToRefresh(catLoversInitialCount, dogLoversInitialCount);
verifyHeader(1, 'Cat Lovers', catLoversInitialCount);
verifyDetailedMessage('There was an error when trying to add learners:', 'error',
[notAllowedUserMessage('not_allowed')]
);
});

it('shows a "view all" button when more than 5 students do not exist', function() {
var sixUsers = 'unknown1, unknown2, unknown3, unknown4, unknown5, unknown6';
Expand Down
13 changes: 12 additions & 1 deletion openedx/core/djangoapps/course_groups/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from django.dispatch import receiver

from opaque_keys.edx.django.models import CourseKeyField
from openedx_filters.learning.filters import CohortChangeRequested
from openedx_filters.learning.filters import CohortAssignmentRequested, CohortChangeRequested

from openedx.core.djangolib.model_mixins import DeletableByUserValue

Expand All @@ -31,6 +31,10 @@ class CohortChangeNotAllowed(CohortMembershipException):
pass


class CohortAssignmentNotAllowed(CohortMembershipException):
pass


class CourseUserGroup(models.Model):
"""
This model represents groups of users in a course. Groups may have different types,
Expand Down Expand Up @@ -113,6 +117,13 @@ def assign(cls, cohort, user):
cohort
Returns CohortMembership, previous_cohort (if any)
"""
try:
# .. filter_implemented_name: CohortAssignmentRequested
# .. filter_type: org.openedx.learning.cohort.assignment.requested.v1
user, cohort = CohortAssignmentRequested.run_filter(user=user, target_cohort=cohort)
except CohortAssignmentRequested.PreventCohortAssignment as exc:
raise CohortAssignmentNotAllowed(str(exc)) from exc

with transaction.atomic():
membership, created = cls.objects.select_for_update().get_or_create(
user__id=user.id,
Expand Down
96 changes: 94 additions & 2 deletions openedx/core/djangoapps/course_groups/tests/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@
"""
from django.test import override_settings
from openedx_filters import PipelineStep
from openedx_filters.learning.filters import CohortChangeRequested
from openedx_filters.learning.filters import CohortAssignmentRequested, CohortChangeRequested
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase

from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
from openedx.core.djangoapps.course_groups.models import CohortChangeNotAllowed, CohortMembership
from openedx.core.djangoapps.course_groups.models import (
CohortAssignmentNotAllowed,
CohortChangeNotAllowed,
CohortMembership,
)
from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
from openedx.core.djangolib.testing.utils import skip_unless_lms

Expand All @@ -31,6 +35,23 @@ def run_filter(self, current_membership, target_cohort): # pylint: disable=argu
return {}


class TestCohortAssignmentStep(PipelineStep):
"""
Utility function used when getting steps for pipeline.
"""

def run_filter(self, user, target_cohort): # pylint: disable=arguments-differ
"""Pipeline step that adds cohort info to the users profile."""
user.profile.set_meta(
{
"cohort_info":
f"User assigned to Cohort {str(target_cohort)}",
}
)
user.profile.save()
return {}


class TestStopCohortChangeStep(PipelineStep):
"""
Utility function used when getting steps for pipeline.
Expand All @@ -41,6 +62,16 @@ def run_filter(self, current_membership, target_cohort, *args, **kwargs): # pyl
raise CohortChangeRequested.PreventCohortChange("You can't change cohorts.")


class TestStopAssignmentChangeStep(PipelineStep):
"""
Utility function used when getting steps for pipeline.
"""

def run_filter(self, user, target_cohort, *args, **kwargs): # pylint: disable=arguments-differ
"""Pipeline step that stops the cohort change process."""
raise CohortAssignmentRequested.PreventCohortAssignment("You can't be assign to this cohort.")


@skip_unless_lms
class CohortFiltersTest(SharedModuleStoreTestCase):
"""
Expand Down Expand Up @@ -92,6 +123,33 @@ def test_cohort_change_filter_executed(self):
cohort_membership.user.profile.get_meta(),
)

@override_settings(
OPEN_EDX_FILTERS_CONFIG={
"org.openedx.learning.cohort.assignment.requested.v1": {
"pipeline": [
"openedx.core.djangoapps.course_groups.tests.test_filters.TestCohortAssignmentStep",
],
"fail_silently": False,
},
},
)
def test_cohort_assignment_filter_executed(self):
"""
Test whether the student cohort assignment filter is triggered before the user's
assignment.
Expected result:
- CohortAssignmentRequested is triggered and executes TestCohortAssignmentStep.
- The user's profile meta contains cohort_info.
"""

cohort_membership, _ = CohortMembership.assign(user=self.user, cohort=self.second_cohort, )

self.assertEqual(
{"cohort_info": "User assigned to Cohort SecondCohort"},
cohort_membership.user.profile.get_meta(),
)

@override_settings(
OPEN_EDX_FILTERS_CONFIG={
"org.openedx.learning.cohort.change.requested.v1": {
Expand All @@ -115,6 +173,27 @@ def test_cohort_change_filter_prevent_move(self):
with self.assertRaises(CohortChangeNotAllowed):
CohortMembership.assign(cohort=self.second_cohort, user=self.user)

@override_settings(
OPEN_EDX_FILTERS_CONFIG={
"org.openedx.learning.cohort.assignment.requested.v1": {
"pipeline": [
"openedx.core.djangoapps.course_groups.tests.test_filters.TestStopAssignmentChangeStep",
],
"fail_silently": False,
},
},
)
def test_cohort_assignment_filter_prevent_move(self):
"""
Test prevent the user's cohort assignment through a pipeline step.
Expected result:
- CohortAssignmentRequested is triggered and executes TestStopAssignmentChangeStep.
- The user can't be assigned to the cohort.
"""
with self.assertRaises(CohortAssignmentNotAllowed):
CohortMembership.assign(cohort=self.second_cohort, user=self.user)

@override_settings(OPEN_EDX_FILTERS_CONFIG={})
def test_cohort_change_without_filter_configuration(self):
"""
Expand All @@ -129,3 +208,16 @@ def test_cohort_change_without_filter_configuration(self):
cohort_membership, _ = CohortMembership.assign(cohort=self.second_cohort, user=self.user)

self.assertEqual({}, cohort_membership.user.profile.get_meta())

@override_settings(OPEN_EDX_FILTERS_CONFIG={})
def test_cohort_assignment_without_filter_configuration(self):
"""
Test usual cohort assignment process, without filter's intervention.
Expected result:
- CohortAssignmentRequested does not have any effect on the cohort change process.
- The cohort assignment process ends successfully.
"""
cohort_membership, _ = CohortMembership.assign(cohort=self.second_cohort, user=self.user)

self.assertEqual({}, cohort_membership.user.profile.get_meta())
12 changes: 10 additions & 2 deletions openedx/core/djangoapps/course_groups/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@

from lms.djangoapps.courseware.courses import get_course, get_course_with_access
from common.djangoapps.edxmako.shortcuts import render_to_response
from openedx.core.djangoapps.course_groups.models import CohortMembership
from openedx.core.djangoapps.course_groups.models import (
CohortAssignmentNotAllowed,
CohortChangeNotAllowed,
CohortMembership,
)
from openedx.core.djangoapps.course_groups.permissions import IsStaffOrAdmin
from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser
from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin
Expand Down Expand Up @@ -321,6 +325,7 @@ def add_users_to_cohort(request, course_key_string, cohort_id):
unknown = []
preassigned = []
invalid = []
not_allowed = []
for username_or_email in split_by_comma_and_whitespace(users):
if not username_or_email:
continue
Expand All @@ -346,14 +351,17 @@ def add_users_to_cohort(request, course_key_string, cohort_id):
invalid.append(username_or_email)
except ValueError:
present.append(username_or_email)
except (CohortAssignmentNotAllowed, CohortChangeNotAllowed):
not_allowed.append(username_or_email)

return json_http_response({'success': True,
'added': added,
'changed': changed,
'present': present,
'unknown': unknown,
'preassigned': preassigned,
'invalid': invalid})
'invalid': invalid,
'not_allowed': not_allowed})


@ensure_csrf_cookie
Expand Down

0 comments on commit ec69659

Please sign in to comment.