Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BD-32] feat: integrate cohort assignment filter definition #30431

Merged
merged 1 commit into from
Jun 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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