diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py
index 7ec69497b9..1b247c823a 100644
--- a/common/test/acceptance/tests/discussion/test_cohort_management.py
+++ b/common/test/acceptance/tests/discussion/test_cohort_management.py
@@ -451,7 +451,7 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin
And I can see the `Enable Cohorts` checkbox is checked.
And cohort management controls are visible.
When I uncheck the `Enable Cohorts` checkbox.
- Then I cohort management controls are not visible.
+ Then cohort management controls are not visible.
And When I reload the page.
Then I can see the `Enable Cohorts` checkbox is unchecked.
And cohort management controls are not visible.
diff --git a/lms/static/js/edxnotes/views/toggle_notes_factory.js b/lms/static/js/edxnotes/views/toggle_notes_factory.js
index 7a37788dd2..8fdc5fc071 100644
--- a/lms/static/js/edxnotes/views/toggle_notes_factory.js
+++ b/lms/static/js/edxnotes/views/toggle_notes_factory.js
@@ -2,7 +2,7 @@
'use strict';
define([
'jquery', 'underscore', 'backbone', 'gettext',
- 'annotator_1.2.9', 'js/edxnotes/views/visibility_decorator'
+ 'annotator_1.2.9', 'js/edxnotes/views/visibility_decorator', 'js/utils/animation'
], function($, _, Backbone, gettext, Annotator, EdxnotesVisibilityDecorator) {
var ToggleNotesView = Backbone.View.extend({
events: {
@@ -31,7 +31,7 @@ define([
toggleHandler: function (event) {
event.preventDefault();
this.visibility = !this.visibility;
- this.showActionMessage();
+ AnimationUtil.triggerAnimation(this.actionToggleMessage);
this.toggleNotes(this.visibility);
},
@@ -51,13 +51,6 @@ define([
this.sendRequest();
},
- showActionMessage: function () {
- // The following lines are necessary to re-trigger the CSS animation on span.action-toggle-message
- this.actionToggleMessage.removeClass('is-fleeting');
- this.actionToggleMessage.offset().width = this.actionToggleMessage.offset().width;
- this.actionToggleMessage.addClass('is-fleeting');
- },
-
enableNotes: function () {
_.each($('.edx-notes-wrapper'), EdxnotesVisibilityDecorator.enableNote);
this.actionLink.addClass('is-active');
diff --git a/lms/static/js/groups/views/cohorts.js b/lms/static/js/groups/views/cohorts.js
index ba175097a9..a8184bece6 100644
--- a/lms/static/js/groups/views/cohorts.js
+++ b/lms/static/js/groups/views/cohorts.js
@@ -57,7 +57,8 @@ var edx = edx || {};
renderCourseCohortSettingsNotificationView: function() {
var cohortStateMessageNotificationView = new CourseCohortSettingsNotificationView({
el: $('.cohort-state-message'),
- cohortEnabled: this.getCohortsEnabled()});
+ cohortEnabled: this.getCohortsEnabled()
+ });
cohortStateMessageNotificationView.render();
},
@@ -123,6 +124,12 @@ var edx = edx || {};
).done(function() {
self.render();
self.renderCourseCohortSettingsNotificationView();
+ }).fail(function(result) {
+ self.showNotification({
+ type: 'error',
+ title: gettext("We've encountered an error. Refresh your browser and then try again.")},
+ self.$('.cohorts-state-section')
+ );
});
},
@@ -199,9 +206,11 @@ var edx = edx || {};
setCohortEditorVisibility: function(showEditor) {
if (showEditor) {
+ this.$('.cohorts-state-section').removeClass(disabledClass).attr('aria-disabled', false);
this.$('.cohort-management-group').removeClass(hiddenClass);
this.$('.cohort-management-nav').removeClass(disabledClass).attr('aria-disabled', false);
} else {
+ this.$('.cohorts-state-section').addClass(disabledClass).attr('aria-disabled', true);
this.$('.cohort-management-group').addClass(hiddenClass);
this.$('.cohort-management-nav').addClass(disabledClass).attr('aria-disabled', true);
}
diff --git a/lms/static/js/groups/views/course_cohort_settings_notification.js b/lms/static/js/groups/views/course_cohort_settings_notification.js
index 1708b1e532..e675e3440c 100644
--- a/lms/static/js/groups/views/course_cohort_settings_notification.js
+++ b/lms/static/js/groups/views/course_cohort_settings_notification.js
@@ -20,11 +20,7 @@ var edx = edx || {};
showCohortStateMessage: function () {
var actionToggleMessage = this.$('.action-toggle-message');
- // The following lines are necessary to re-trigger the CSS animation on span.action-toggle-message
- actionToggleMessage.removeClass('is-fleeting');
- actionToggleMessage.offset().width = actionToggleMessage.offset().width;
- actionToggleMessage.addClass('is-fleeting');
-
+ AnimationUtil.triggerAnimation(actionToggleMessage);
if (this.cohortEnabled) {
actionToggleMessage.text(gettext('Cohorts Enabled'));
} else {
diff --git a/lms/static/js/instructor_dashboard/cohort_management.js b/lms/static/js/instructor_dashboard/cohort_management.js
index 1d820ba2cc..abe725f21a 100644
--- a/lms/static/js/instructor_dashboard/cohort_management.js
+++ b/lms/static/js/instructor_dashboard/cohort_management.js
@@ -1,7 +1,7 @@
(function() {
- var CohortManagement;
+ var CohortManagement;
- CohortManagement = (function() {
+ CohortManagement = (function() {
function CohortManagement($section) {
this.$section = $section;
@@ -12,18 +12,8 @@
return CohortManagement;
- })();
+ })();
- _.defaults(window, {
- InstructorDashboard: {}
- });
-
- _.defaults(window.InstructorDashboard, {
- sections: {}
- });
-
- _.defaults(window.InstructorDashboard.sections, {
- CohortManagement: CohortManagement
- });
+ window.InstructorDashboard.sections.CohortManagement = CohortManagement;
}).call(this);
diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js
index 3123bef275..6106c16f2c 100644
--- a/lms/static/js/spec/groups/views/cohorts_spec.js
+++ b/lms/static/js/spec/groups/views/cohorts_spec.js
@@ -1,16 +1,18 @@
define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpers/template_helpers',
'js/groups/views/cohorts', 'js/groups/collections/cohort', 'js/groups/models/content_group',
- 'js/groups/models/course_cohort_settings', 'js/groups/views/course_cohort_settings_notification'],
+ 'js/groups/models/course_cohort_settings', 'js/utils/animation',
+ 'js/groups/views/course_cohort_settings_notification'
+ ],
function (Backbone, $, AjaxHelpers, TemplateHelpers, CohortsView, CohortCollection, ContentGroupModel,
- CourseCohortSettingsModel, CourseCohortSettingsNotificationView) {
+ CourseCohortSettingsModel, AnimationUtil, CourseCohortSettingsNotificationView) {
'use strict';
describe("Cohorts View", function () {
var catLoversInitialCount = 123, dogLoversInitialCount = 456, unknownUserMessage,
- createMockCohort, createMockCohorts, createMockContentGroups, createCohortSettings, createCohortsView,
- cohortsView, requests, respondToRefresh, verifyMessage, verifyNoMessage, verifyDetailedMessage,
- verifyHeader, expectCohortAddRequest, getAddModal, selectContentGroup, clearContentGroup,
- saveFormAndExpectErrors, createMockCohortSettings, MOCK_COHORTED_USER_PARTITION_ID,
+ createMockCohort, createMockCohorts, createMockContentGroups, createMockCohortSettingsJson,
+ createCohortsView, cohortsView, requests, respondToRefresh, verifyMessage, verifyNoMessage,
+ verifyDetailedMessage, verifyHeader, expectCohortAddRequest, getAddModal, selectContentGroup,
+ clearContentGroup, saveFormAndExpectErrors, createMockCohortSettings, MOCK_COHORTED_USER_PARTITION_ID,
MOCK_UPLOAD_COHORTS_CSV_URL, MOCK_STUDIO_ADVANCED_SETTINGS_URL, MOCK_STUDIO_GROUP_CONFIGURATIONS_URL,
MOCK_MANUAL_ASSIGNMENT, MOCK_RANDOM_ASSIGNMENT;
@@ -52,7 +54,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
];
};
- createMockCohortSettings = function (isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions) {
+ createMockCohortSettingsJson = function (isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions) {
return {
id: 0,
is_cohorted: isCohorted || false,
@@ -61,9 +63,9 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
};
};
- createCohortSettings = function (isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions) {
+ createMockCohortSettings = function (isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions) {
return new CourseCohortSettingsModel(
- createMockCohortSettings(isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions)
+ createMockCohortSettingsJson(isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions)
);
};
@@ -73,7 +75,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
cohortsJson = options.cohorts ? {cohorts: options.cohorts} : createMockCohorts();
cohorts = new CohortCollection(cohortsJson, {parse: true});
contentGroups = options.contentGroups || createMockContentGroups();
- cohortSettings = options.cohortSettings || createCohortSettings(true);
+ cohortSettings = options.cohortSettings || createMockCohortSettings(true);
cohortSettings.url = '/mock_service/cohorts/settings';
cohorts.url = '/mock_service/cohorts';
requests = AjaxHelpers.requests(test);
@@ -278,36 +280,36 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
});
describe("Course Cohort Settings", function () {
- it('enable/disable working correctly', function () {
- createCohortsView(this, {cohortSettings: createCohortSettings(false)});
+ it('can enable and disable cohorting', function () {
+ createCohortsView(this, {cohortSettings: createMockCohortSettings(false)});
expect(cohortsView.$('.cohorts-state').prop('checked')).toBeFalsy();
cohortsView.$('.cohorts-state').prop('checked', true).change();
AjaxHelpers.expectJsonRequest(
requests, 'PUT', '/mock_service/cohorts/settings',
- createMockCohortSettings(true, [], true)
+ createMockCohortSettingsJson(true, [], true)
);
AjaxHelpers.respondWithJson(
requests,
- createMockCohortSettings(true)
+ createMockCohortSettingsJson(true)
);
expect(cohortsView.$('.cohorts-state').prop('checked')).toBeTruthy();
cohortsView.$('.cohorts-state').prop('checked', false).change();
AjaxHelpers.expectJsonRequest(
requests, 'PUT', '/mock_service/cohorts/settings',
- createMockCohortSettings(false, [], true)
+ createMockCohortSettingsJson(false, [], true)
);
AjaxHelpers.respondWithJson(
requests,
- createMockCohortSettings(false)
+ createMockCohortSettingsJson(false)
);
expect(cohortsView.$('.cohorts-state').prop('checked')).toBeFalsy();
});
- it('Course Cohort Settings Notification View renders correctly', function () {
+ it('shows an appropriate cohort status message', function () {
var createCourseCohortSettingsNotificationView = function (is_cohorted) {
var notificationView = new CourseCohortSettingsNotificationView({
el: $('.cohort-state-message'),
@@ -323,6 +325,14 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
expect(notificationView.$('.action-toggle-message').text().trim()).toBe('Cohorts Disabled');
});
+ it('shows an appropriate error message for HTTP500', function () {
+ createCohortsView(this, {cohortSettings: createMockCohortSettings(false)});
+ expect(cohortsView.$('.cohorts-state').prop('checked')).toBeFalsy();
+ cohortsView.$('.cohorts-state').prop('checked', true).change();
+ AjaxHelpers.respondWithError(requests, 500);
+ var expectedTitle = "We've encountered an error. Refresh your browser and then try again."
+ expect(cohortsView.$('.message-title').text().trim()).toBe(expectedTitle);
+ });
});
describe("Cohort Group Header", function () {
diff --git a/lms/static/js/utils/animation.js b/lms/static/js/utils/animation.js
new file mode 100644
index 0000000000..dcc232e93d
--- /dev/null
+++ b/lms/static/js/utils/animation.js
@@ -0,0 +1,15 @@
+(function() {
+ this.AnimationUtil = (function() {
+ function AnimationUtil() {}
+ AnimationUtil.triggerAnimation = function(messageElement) {
+ // The following lines are necessary to re-trigger the CSS animation on span.action-toggle-message
+ // To see how it works, please see `Another JavaScript Method to Restart a CSS Animation`
+ // at https://css-tricks.com/restart-css-animation/
+ messageElement.removeClass('is-fleeting');
+ messageElement.offset().width = messageElement.offset().width;
+ messageElement.addClass('is-fleeting');
+ };
+ return AnimationUtil;
+ }).call(this);
+}).call(this);
+
diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss
index 5ea18fe818..bb9c40cad2 100644
--- a/lms/static/sass/course/instructor/_instructor_2.scss
+++ b/lms/static/sass/course/instructor/_instructor_2.scss
@@ -809,9 +809,10 @@
}
}
- .cohort-management-assignment-type-settings {
+ .cohort-management-assignment-type-settings,
+ .cohorts-state-section {
&.is-disabled {
- opacity: 0.50;
+ opacity: 0.25;
}
}
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
index 8d7df60bf0..b3cd072ad5 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
@@ -35,7 +35,7 @@
<% } %>
- <%- gettext('Students in this cohort are:') %>
+ <%- gettext('Students in this cohort are') %>
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore
index f00eacc474..3c5e28d5fe 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore
+++ b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore
@@ -8,7 +8,7 @@
-
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
index d21300a103..9cac21bb1d 100644
--- a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
+++ b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
@@ -66,6 +66,7 @@
+
## Include Underscore templates
@@ -129,4 +130,5 @@
% endfor
-
\ No newline at end of file
+
+
diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py
index 83092a4914..982b359fd0 100644
--- a/openedx/core/djangoapps/course_groups/cohorts.py
+++ b/openedx/core/djangoapps/course_groups/cohorts.py
@@ -456,9 +456,12 @@ def set_course_cohort_settings(course_key, **kwargs):
Raises:
ValueError if course_key is invalid.
"""
+ fields = {'is_cohorted': bool, 'always_cohort_inline_discussions': bool, 'cohorted_discussions': list}
course_cohort_settings = get_course_cohort_settings(course_key)
- for field in ('is_cohorted', 'always_cohort_inline_discussions', 'cohorted_discussions'):
+ for field, field_type in fields.items():
if field in kwargs:
+ if not isinstance(kwargs[field], field_type):
+ raise ValueError("Incorrect field type for `{}`. Type must be `{}`".format(field, field_type.__name__))
setattr(course_cohort_settings, field, kwargs[field])
course_cohort_settings.save()
return course_cohort_settings
diff --git a/openedx/core/djangoapps/course_groups/models.py b/openedx/core/djangoapps/course_groups/models.py
index 3c04648dbf..3fc6828bdc 100644
--- a/openedx/core/djangoapps/course_groups/models.py
+++ b/openedx/core/djangoapps/course_groups/models.py
@@ -93,12 +93,12 @@ class CourseCohortsSettings(models.Model):
@property
def cohorted_discussions(self):
- """Jsonfiy the cohorted_discussions"""
+ """Jsonify the cohorted_discussions"""
return json.loads(self._cohorted_discussions)
@cohorted_discussions.setter
def cohorted_discussions(self, value):
- """UnJsonfiy the cohorted_discussions"""
+ """Un-Jsonify the cohorted_discussions"""
self._cohorted_discussions = json.dumps(value)
diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
index 9e9039808c..0b704aa7ef 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
@@ -716,6 +716,29 @@ class TestCohorts(ModuleStoreTestCase):
self.assertEqual(course_cohort_settings.cohorted_discussions, ['topic a id', 'topic b id'])
self.assertFalse(course_cohort_settings.always_cohort_inline_discussions)
+ def test_update_course_cohort_settings_with_invalid_data_type(self):
+ """
+ Test that cohorts.set_course_cohort_settings raises exception if fields have incorrect data type.
+ """
+ course = modulestore().get_course(self.toy_course_key)
+ CourseCohortSettingsFactory(course_id=course.id)
+
+ exception_msg_tpl = "Incorrect field type for `{}`. Type must be `{}`"
+ fields = [
+ {'name': 'is_cohorted', 'type': bool},
+ {'name': 'always_cohort_inline_discussions', 'type': bool},
+ {'name': 'cohorted_discussions', 'type': list}
+ ]
+
+ for field in fields:
+ with self.assertRaises(ValueError) as value_error:
+ cohorts.set_course_cohort_settings(course.id, **{field['name']: ''})
+
+ self.assertEqual(
+ value_error.exception.message,
+ exception_msg_tpl.format(field['name'], field['type'].__name__)
+ )
+
class TestCohortsAndPartitionGroups(ModuleStoreTestCase):
"""
diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py
index 1355f6691a..f3ee3d21ae 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_views.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_views.py
@@ -3,7 +3,6 @@ Tests for course group views
"""
# pylint: disable=attribute-defined-outside-init
# pylint: disable=no-member
-from collections import namedtuple
import json
from collections import namedtuple
@@ -15,7 +14,6 @@ from student.models import CourseEnrollment
from student.tests.factories import UserFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
-from xmodule.modulestore.django import modulestore
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from ..models import CourseUserGroup, CourseCohort
@@ -180,13 +178,26 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
config_course_cohorts(self.course, [], cohorted=True)
- # Get the cohorts from the course. This will run the migrations.
- # And due to migrations CourseCohortsSettings object will be created.
- self.get_handler(self.course)
-
response = self.put_handler(self.course, expected_response_code=400, handler=course_cohort_settings_handler)
self.assertEqual("Bad Request", response.get("error"))
+ def test_update_settings_with_invalid_field_data_type(self):
+ """
+ Verify that course_cohort_settings_handler return HTTP 400 if field data type is incorrect.
+ """
+ config_course_cohorts(self.course, [], cohorted=True)
+
+ response = self.put_handler(
+ self.course,
+ data={'is_cohorted': ''},
+ expected_response_code=400,
+ handler=course_cohort_settings_handler
+ )
+ self.assertEqual(
+ "Incorrect field type for `{}`. Type must be `{}`".format('is_cohorted', bool.__name__),
+ response.get("error")
+ )
+
class CohortHandlerTestCase(CohortViewsTestCase):
"""
diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py
index 9d9356794f..e3c89f5345 100644
--- a/openedx/core/djangoapps/course_groups/views.py
+++ b/openedx/core/djangoapps/course_groups/views.py
@@ -112,7 +112,13 @@ def course_cohort_settings_handler(request, course_key_string):
if is_cohorted is None:
# Note: error message not translated because it is not exposed to the user (UI prevents this state).
return JsonResponse({"error": "Bad Request"}, 400)
- cohort_settings = cohorts.set_course_cohort_settings(course_key, is_cohorted=is_cohorted)
+
+ try:
+ cohort_settings = cohorts.set_course_cohort_settings(course_key, is_cohorted=is_cohorted)
+ except ValueError as err:
+ # Note: error message not translated because it is not exposed to the user (UI prevents this state).
+ return JsonResponse({"error": unicode(err)}, 400)
+
return JsonResponse(_get_course_cohort_settings_representation(cohort_settings))