Changes after Andy's Review

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.

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');

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);
             }

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 {

lms/static/js/instructor_dashboard/cohort_management.js

 (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);

lms/static/js/spec/groups/views/cohorts_spec.js

 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 () {

lms/static/js/utils/animation.js

+(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);
+

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;
       }
   }
 

lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore

@@ -35,7 +35,7 @@
                     <div class="cohort-management-assignment-type-settings field field-radio">
                 <% } %>
                     <h4 class="form-label">
-                        <%- gettext('Students in this cohort are:') %>
+                        <%- gettext('Students in this cohort are') %>
                     </h4>
                     <label>
                         <input type="radio" class="type-random" name="cohort-assignment-type" value="random"  <%- assignment_type == 'random' ? 'checked="checked"' : '' %>/> <%- gettext("Automatically Assigned") %>

lms/templates/instructor/instructor_dashboard_2/cohort-state.underscore

 <div class="nav-utilities">
     <span class="action-toggle-message" aria-live="polite"></span>
-</div>
+</div>
\ No newline at end of file
+

lms/templates/instructor/instructor_dashboard_2/cohorts.underscore

@@ -8,7 +8,7 @@
         <form action="" method="post" name="" id="cohort-management-nav-form" class="cohort-management-nav-form">
 
             <div class="cohort-management-nav-form-select field field-select">
-                <label for="cohort-select" class="label sr"><%- gettext("Select a cohort group to manage") %></label>
+                <label for="cohort-select" class="label sr"><%- gettext("Select a cohort to manage") %></label>
                 <select class="input cohort-select" name="cohort-select" id="cohort-select"></select>
             </div>
 
@@ -17,10 +17,10 @@
             </div>
         </form>
 
-        <a href="" class="action-primary action-create">
+        <button class="button action-primary action-create">
             <i class="icon fa fa-plus" aria-hidden="true"></i>
             <%- gettext('Add Cohort') %>
-        </a>
+        </button>
     </div>
 
     <!-- Add modal -->

lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html

@@ -66,6 +66,7 @@
   <script type="text/javascript" src="${static.url('js/groups/views/cohort_form.js')}"></script>
   <script type="text/javascript" src="${static.url('js/groups/views/cohort_editor.js')}"></script>
   <script type="text/javascript" src="${static.url('js/groups/views/cohorts.js')}"></script>
+  <script type="text/javascript" src="${static.url('js/utils/animation.js')}"></script>
 </%block>
 
 ## Include Underscore templates
@@ -129,4 +130,5 @@
       % endfor
     </section>
   </div>
-</section>
+</section>
\ No newline at end of file
+

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

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)
 
 

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):
     """

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):
     """

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))