diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a68ad7530f..b6390d58de 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,12 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Support adding cohorts from the instructor dashboard. TNL-162 + +LMS: Support adding students to a cohort via the instructor dashboard. TNL-163 + +LMS: Show cohorts on the new instructor dashboard. TNL-161 + LMS: Mobile API available for courses that opt in using the Course Advanced Setting "Mobile Course Available" (only used in limited closed beta). diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index a21d99d31a..9a6340c2bf 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/common/djangoapps/course_groups/cohorts.py @@ -3,11 +3,12 @@ This file contains the logic for cohort groups, as exposed internally to the forums, and to the cohort admin views. """ -from django.http import Http404 - import logging import random +from django.http import Http404 +from django.utils.translation import ugettext as _ + from courseware import courses from student.models import get_user_by_username_or_email from .models import CourseUserGroup @@ -140,7 +141,7 @@ def get_cohorted_commentables(course_key): def get_cohort(user, course_key): """ - Given a django User and a CourseKey, return the user's cohort in that + Given a Django user and a CourseKey, return the user's cohort in that cohort. Arguments: @@ -180,7 +181,7 @@ def get_cohort(user, course_key): # Use the "default cohort". group_name = DEFAULT_COHORT_NAME - group, _ = CourseUserGroup.objects.get_or_create( + group, __ = CourseUserGroup.objects.get_or_create( course_id=course_key, group_type=CourseUserGroup.COHORT, name=group_name @@ -250,7 +251,7 @@ def add_cohort(course_key, name): if CourseUserGroup.objects.filter(course_id=course_key, group_type=CourseUserGroup.COHORT, name=name).exists(): - raise ValueError("Can't create two cohorts with the same name") + raise ValueError(_("You cannot create two cohorts with the same name")) try: course = courses.get_course_by_id(course_key) @@ -296,9 +297,10 @@ def add_user_to_cohort(cohort, username_or_email): ) if course_cohorts.exists(): if course_cohorts[0] == cohort: - raise ValueError("User {0} already present in cohort {1}".format( - user.username, - cohort.name)) + raise ValueError("User {user_name} already present in cohort {cohort_name}".format( + user_name=user.username, + cohort_name=cohort.name + )) else: previous_cohort = course_cohorts[0].name course_cohorts[0].users.remove(user) @@ -313,8 +315,9 @@ def delete_empty_cohort(course_key, name): """ cohort = get_cohort_by_name(course_key, name) if cohort.users.exists(): - raise ValueError( - "Can't delete non-empty cohort {0} in course {1}".format( - name, course_key)) + raise ValueError(_("You cannot delete non-empty cohort {cohort_name} in course {course_key}").format( + cohort_name=name, + course_key=course_key + )) cohort.delete() diff --git a/common/djangoapps/course_groups/tests/helpers.py b/common/djangoapps/course_groups/tests/helpers.py index da367f0eae..8b19efa379 100644 --- a/common/djangoapps/course_groups/tests/helpers.py +++ b/common/djangoapps/course_groups/tests/helpers.py @@ -9,6 +9,9 @@ from xmodule.modulestore import ModuleStoreEnum class CohortFactory(DjangoModelFactory): + """ + Factory for constructing mock cohorts. + """ FACTORY_FOR = CourseUserGroup name = Sequence("cohort{}".format) @@ -17,6 +20,9 @@ class CohortFactory(DjangoModelFactory): @post_generation def users(self, create, extracted, **kwargs): # pylint: disable=W0613 + """ + Returns the users associated with the cohort. + """ if extracted: self.users.add(*extracted) diff --git a/common/djangoapps/course_groups/tests/test_views.py b/common/djangoapps/course_groups/tests/test_views.py index 4de119b046..819190237f 100644 --- a/common/djangoapps/course_groups/tests/test_views.py +++ b/common/djangoapps/course_groups/tests/test_views.py @@ -277,7 +277,7 @@ class AddCohortTestCase(CohortViewsTestCase): self.verify_contains_added_cohort( self.request_add_cohort(cohort_name, self.course), cohort_name, - expected_error_msg="Can't create two cohorts with the same name" + expected_error_msg="You cannot create two cohorts with the same name" ) diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index 28ea76c8dc..1ed9b1d8ce 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -37,7 +37,7 @@ class MembershipPage(PageObject): def _get_cohort_options(self): """ - Returns the available options in the cohort dropdown, including the initial "Select a cohort". + Returns the available options in the cohort dropdown, including the initial "Select a cohort group". """ return self.q(css=".cohort-management #cohort-select option") @@ -55,7 +55,7 @@ class MembershipPage(PageObject): def get_cohorts(self): """ - Returns, as a list, the names of the available cohorts in the drop-down, filtering out "Select a cohort". + Returns, as a list, the names of the available cohorts in the drop-down, filtering out "Select a cohort group". """ return [ self._cohort_name(opt.text) @@ -86,6 +86,15 @@ class MembershipPage(PageObject): lambda el: self._cohort_name(el.text) == cohort_name ).first.click() + def add_cohort(self, cohort_name): + """ + Adds a new manual cohort with the specified name. + """ + self.q(css="div.cohort-management-nav .action-create").first.click() + textinput = self.q(css="#cohort-create-name").results[0] + textinput.send_keys(cohort_name) + self.q(css="div.form-actions .action-save").first.click() + def get_cohort_group_setup(self): """ Returns the description of the current cohort diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 740adc9d0a..f67af694dd 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -11,6 +11,8 @@ from ...pages.lms.auto_auth import AutoAuthPage from ...pages.lms.instructor_dashboard import InstructorDashboardPage from ...pages.studio.settings_advanced import AdvancedSettingsPage +import uuid + class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): """ @@ -149,6 +151,30 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): self.assertEqual("There was an error when trying to add students:", error_messages[0]) self.assertEqual("Unknown user: unknown_user", error_messages[1]) self.assertEqual( - self.student_name+",unknown_user,", + self.student_name + ",unknown_user,", self.membership_page.get_cohort_student_input_field_value() ) + + def test_add_new_cohort(self): + """ + Scenario: A new manual cohort can be created, and a student assigned to it. + + Given I have a course with a user in the course + When I add a new manual cohort to the course via the LMS instructor dashboard + Then the new cohort is displayed and has no users in it + And when I add the user to the new cohort + Then the cohort has 1 user + """ + new_cohort = str(uuid.uuid4().get_hex()[0:20]) + self.assertFalse(new_cohort in self.membership_page.get_cohorts()) + self.membership_page.add_cohort(new_cohort) + # After adding the cohort, it should automatically be selected + EmptyPromise( + lambda: new_cohort == self.membership_page.get_selected_cohort(), "Waiting for new cohort to appear" + ).fulfill() + self.assertEqual(0, self.membership_page.get_selected_cohort_count()) + self.membership_page.add_students_to_selected_cohort([self.instructor_name]) + # Wait for the number of users in the cohort to change, indicating that the add operation is complete. + EmptyPromise( + lambda: 1 == self.membership_page.get_selected_cohort_count(), 'Waiting for student to be added' + ).fulfill() diff --git a/lms/envs/common.py b/lms/envs/common.py index b91dedd894..294dd4765d 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -992,6 +992,7 @@ courseware_js = ( base_vendor_js = [ 'js/vendor/jquery.min.js', 'js/vendor/jquery.cookie.js', + 'js/vendor/underscore-min.js' ] main_vendor_js = base_vendor_js + [ @@ -999,7 +1000,6 @@ main_vendor_js = base_vendor_js + [ 'js/RequireJS-namespace-undefine.js', 'js/vendor/json2.js', 'js/vendor/jquery-ui.min.js', - 'js/vendor/jquery.cookie.js', 'js/vendor/jquery.qtip.min.js', 'js/vendor/swfobject/swfobject.js', 'js/vendor/jquery.ba-bbq.min.js', diff --git a/lms/static/js/models/notification.js b/lms/static/js/models/notification.js index 83dd011a7a..d256ef1803 100644 --- a/lms/static/js/models/notification.js +++ b/lms/static/js/models/notification.js @@ -1,11 +1,42 @@ (function(Backbone) { var NotificationModel = Backbone.Model.extend({ defaults: { - // Supported types are "confirmation" and "error". + /** + * The type of notification to be shown. + * Supported types are "confirmation", "warning" and "error". + */ type: "confirmation", + /** + * The title to be shown for the notification. This string should be short so + * that it can be shown on a single line. + */ title: "", + /** + * An optional message giving more details for the notification. This string can be as long + * as needed and will wrap. + */ + message: "", + /** + * An optional array of detail messages to be shown beneath the title and message. This is + * typically used to enumerate a set of warning or error conditions that occurred. + */ details: [], - actionText: "", + /** + * The text label to be shown for an action button, or null if there is no associated action. + */ + actionText: null, + /** + * The class to be added to the action button. This allows selectors to be written that can + * target the action button directly. + */ + actionClass: "", + /** + * An optional icon class to be shown before the text on the action button. + */ + actionIconClass: "", + /** + * An optional callback that will be invoked when the user clicks on the action button. + */ actionCallback: null } }); diff --git a/lms/static/js/spec/views/cohorts_spec.js b/lms/static/js/spec/views/cohorts_spec.js index 7074ab44d7..0bf1a1a5d6 100644 --- a/lms/static/js/spec/views/cohorts_spec.js +++ b/lms/static/js/spec/views/cohorts_spec.js @@ -3,27 +3,28 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe function (Backbone, $, AjaxHelpers, TemplateHelpers, CohortsView, CohortCollection) { describe("Cohorts View", function () { var catLoversInitialCount = 123, dogLoversInitialCount = 456, unknownUserMessage, - createMockCohorts, createCohortsView, cohortsView, requests, verifyMessage, verifyHeader; + createMockCohort, createMockCohorts, createCohortsView, cohortsView, requests, respondToRefresh, + verifyMessage, verifyNoMessage, verifyDetailedMessage, verifyHeader; + + createMockCohort = function (name, id, user_count) { + return { + id: id || 1, + name: name, + user_count: user_count || 0 + }; + }; createMockCohorts = function (catCount, dogCount) { return { cohorts: [ - { - id: 1, - name: 'Cat Lovers', - user_count: catCount || catLoversInitialCount - }, - { - id: 2, - name: 'Dog Lovers', - user_count: dogCount || dogLoversInitialCount - } + createMockCohort('Cat Lovers', 1, catCount || catLoversInitialCount), + createMockCohort('Dog Lovers', 2, dogCount || dogLoversInitialCount) ] }; }; - createCohortsView = function (test, initialCohortID) { - var cohorts = new CohortCollection(createMockCohorts(), {parse: true}); + createCohortsView = function (test, initialCohortID, initialCohorts) { + var cohorts = new CohortCollection(initialCohorts || createMockCohorts(), {parse: true}); cohorts.url = '/mock_service'; requests = AjaxHelpers.requests(test); cohortsView = new CohortsView({ @@ -31,32 +32,41 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); cohortsView.render(); if (initialCohortID) { - cohortsView.$('.cohort-select').val(initialCohortID).change(); + cohortsView.$('.cohort-select').val(initialCohortID.toString()).change(); } }; - verifyMessage = function(expectedTitle, messageType, expectedDetails, expectedAction) { - var numDetails = cohortsView.$('.summary-items').children().length; + respondToRefresh = function(catCount, dogCount) { + AjaxHelpers.respondWithJson(requests, createMockCohorts(catCount, dogCount)); + }; + verifyMessage = function(expectedTitle, expectedMessageType, expectedAction, hasDetails) { expect(cohortsView.$('.message-title').text().trim()).toBe(expectedTitle); - expect(cohortsView.$('div.message').hasClass('message-' + messageType)).toBe(true); + expect(cohortsView.$('div.message')).toHaveClass('message-' + expectedMessageType); if (expectedAction) { - expect(cohortsView.$('.message-actions .action-primary').text()).toBe(expectedAction); + expect(cohortsView.$('.message-actions .action-primary').text().trim()).toBe(expectedAction); } else { expect(cohortsView.$('.message-actions .action-primary').length).toBe(0); } - if (expectedDetails) { - expect(numDetails).toBe(expectedDetails.length); - cohortsView.$('.summary-item').each(function (index) { - expect($(this).text().trim()).toBe(expectedDetails[index]); - }); - } - else { - expect(numDetails).toBe(0); + if (!hasDetails) { + expect(cohortsView.$('.summary-items').length).toBe(0); } }; + verifyNoMessage = function() { + expect(cohortsView.$('.message').length).toBe(0); + }; + + verifyDetailedMessage = function(expectedTitle, expectedMessageType, expectedDetails, expectedAction) { + var numDetails = cohortsView.$('.summary-items').children().length; + verifyMessage(expectedTitle, expectedMessageType, expectedAction, true); + expect(numDetails).toBe(expectedDetails.length); + cohortsView.$('.summary-item').each(function (index) { + expect($(this).text().trim()).toBe(expectedDetails[index]); + }); + }; + verifyHeader = function(expectedCohortId, expectedTitle, expectedCount) { var header = cohortsView.$('.cohort-management-group-header'); expect(cohortsView.$('.cohort-select').val()).toBe(expectedCohortId.toString()); @@ -83,11 +93,21 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe beforeEach(function () { setFixtures("
"); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohorts'); + TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/add-cohort-form'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-selector'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-editor'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/notification'); }); + it("Show an error if no cohorts are defined", function() { + createCohortsView(this, null, { cohorts: [] }); + verifyMessage( + 'You currently have no cohort groups configured', + 'warning', + 'Add Cohort Group' + ); + }); + describe("Cohort Selector", function () { it('has no initial selection', function () { createCohortsView(this); @@ -96,19 +116,158 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); it('can select a cohort', function () { - createCohortsView(this, "1"); + createCohortsView(this, 1); verifyHeader(1, 'Cat Lovers', catLoversInitialCount); }); it('can switch cohort', function () { - createCohortsView(this, "1"); + createCohortsView(this, 1); cohortsView.$('.cohort-select').val("2").change(); verifyHeader(2, 'Dog Lovers', dogLoversInitialCount); }); }); + describe("Add Cohorts Form", function () { + var defaultCohortName = 'New Cohort'; + + it("can add a cohort", function() { + createCohortsView(this, null, { cohorts: [] }); + cohortsView.$('.action-create').click(); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(1); + expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled'); + expect(cohortsView.$('.cohort-management-group')).toHaveClass('is-hidden'); + cohortsView.$('.cohort-create-name').val(defaultCohortName); + cohortsView.$('.action-save').click(); + AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/add', 'name=New+Cohort'); + AjaxHelpers.respondWithJson( + requests, + { + success: true, + cohort: { id: 1, name: defaultCohortName } + } + ); + AjaxHelpers.respondWithJson( + requests, + { cohorts: createMockCohort(defaultCohortName) } + ); + verifyMessage( + 'The ' + defaultCohortName + ' cohort group has been created.' + + ' You can manually add students to this group below.', + 'confirmation' + ); + verifyHeader(1, defaultCohortName, 0); + expect(cohortsView.$('.cohort-management-nav')).not.toHaveClass('is-disabled'); + expect(cohortsView.$('.cohort-management-group')).not.toHaveClass('is-hidden'); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(0); + }); + + it("trims off whitespace before adding a cohort", function() { + createCohortsView(this); + cohortsView.$('.action-create').click(); + cohortsView.$('.cohort-create-name').val(' New Cohort '); + cohortsView.$('.action-save').click(); + AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/add', 'name=New+Cohort'); + }); + + it("does not allow a blank cohort name to be submitted", function() { + createCohortsView(this, 1); + cohortsView.$('.action-create').click(); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(1); + cohortsView.$('.cohort-create-name').val(''); + expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled'); + cohortsView.$('.action-save').click(); + expect(requests.length).toBe(0); + verifyMessage('Please enter a name for your new cohort group.', 'error'); + }); + + it("shows a message when adding a cohort throws a server error", function() { + createCohortsView(this, 1); + cohortsView.$('.action-create').click(); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(1); + cohortsView.$('.cohort-create-name').val(defaultCohortName); + cohortsView.$('.action-save').click(); + AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/add', 'name=New+Cohort'); + AjaxHelpers.respondWithError(requests); + verifyHeader(1, 'Cat Lovers', catLoversInitialCount); + verifyMessage( + "We've encountered an error. Please refresh your browser and then try again.", + 'error' + ); + }); + + it("shows a server message if adding a cohort fails", function() { + createCohortsView(this, 1); + cohortsView.$('.action-create').click(); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(1); + cohortsView.$('.cohort-create-name').val('Cat Lovers'); + cohortsView.$('.action-save').click(); + AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/add', 'name=Cat+Lovers'); + AjaxHelpers.respondWithJson( + requests, + { + success: false, + msg: 'You cannot create two cohorts with the same name' + } + ); + verifyHeader(1, 'Cat Lovers', catLoversInitialCount); + verifyMessage('You cannot create two cohorts with the same name', 'error'); + }); + + it("is removed when 'Cancel' is clicked", function() { + createCohortsView(this, 1); + cohortsView.$('.action-create').click(); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(1); + expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled'); + cohortsView.$('.action-cancel').click(); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(0); + expect(cohortsView.$('.cohort-management-nav')).not.toHaveClass('is-disabled'); + }); + + it("shows an error if canceled when no cohorts are defined", function() { + createCohortsView(this, null, { cohorts: [] }); + cohortsView.$('.action-create').click(); + expect(cohortsView.$('.cohort-management-create-form').length).toBe(1); + expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled'); + cohortsView.$('.action-cancel').click(); + verifyMessage( + 'You currently have no cohort groups configured', + 'warning', + 'Add Cohort Group' + ); + }); + + it("hides any error message when switching to show a cohort", function() { + createCohortsView(this, 1); + + // First try to save a blank name to create a message + cohortsView.$('.action-create').click(); + cohortsView.$('.cohort-create-name').val(''); + cohortsView.$('.action-save').click(); + verifyMessage('Please enter a name for your new cohort group.', 'error'); + + // Now switch to a different cohort + cohortsView.$('.cohort-select').val("2").change(); + verifyHeader(2, 'Dog Lovers', dogLoversInitialCount); + verifyNoMessage(); + }); + + it("hides any error message when canceling the form", function() { + createCohortsView(this, 1); + + // First try to save a blank name to create a message + cohortsView.$('.action-create').click(); + cohortsView.$('.cohort-create-name').val(''); + cohortsView.$('.action-save').click(); + verifyMessage('Please enter a name for your new cohort group.', 'error'); + + // Now cancel the form + cohortsView.$('.action-cancel').click(); + verifyNoMessage(); + }); + }); + describe("Add Students Button", function () { - var getStudentInput, addStudents, respondToAdd, respondToRefresh; + var getStudentInput, addStudents, respondToAdd; getStudentInput = function() { return cohortsView.$('.cohort-management-group-add-students'); @@ -126,12 +285,8 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe ); }; - respondToRefresh = function(catCount, dogCount) { - AjaxHelpers.respondWithJson(requests, createMockCohorts(catCount, dogCount)); - }; - it('shows an error when adding with no students specified', function() { - createCohortsView(this, "1"); + createCohortsView(this, 1); addStudents(' '); expect(requests.length).toBe(0); verifyMessage('Please enter a username or email.', 'error'); @@ -140,7 +295,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe it('can add a single student', function() { var catLoversUpdatedCount = catLoversInitialCount + 1; - createCohortsView(this, "1"); + createCohortsView(this, 1); addStudents('student@sample.com'); AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/1/add', 'users=student%40sample.com'); respondToAdd({ added: ['student@sample.com'] }); @@ -151,13 +306,13 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); it('shows an error when adding a student that does not exist', function() { - createCohortsView(this, "1"); + createCohortsView(this, 1); addStudents('unknown@sample.com'); AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/1/add', 'users=unknown%40sample.com'); respondToAdd({ unknown: ['unknown@sample.com'] }); respondToRefresh(catLoversInitialCount, dogLoversInitialCount); verifyHeader(1, 'Cat Lovers', catLoversInitialCount); - verifyMessage('There was an error when trying to add students:', 'error', + verifyDetailedMessage('There was an error when trying to add students:', 'error', [unknownUserMessage('unknown@sample.com')] ); expect(getStudentInput().val()).toBe('unknown@sample.com'); @@ -165,11 +320,11 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe it('shows a "view all" button when more than 5 students do not exist', function() { var sixUsers = 'unknown1@sample.com, unknown2@sample.com, unknown3@sample.com, unknown4@sample.com, unknown5@sample.com, unknown6@sample.com'; - createCohortsView(this, "1"); + createCohortsView(this, 1); addStudents(sixUsers); AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/1/add', - 'users=' + sixUsers.replace(/@/g, "%40").replace(/, /g, "%2C+") + 'users=' + sixUsers.replace(/@/g, "%40").replace(/, /g, "%2C+") ); respondToAdd({ unknown: [ 'unknown1@sample.com', @@ -180,7 +335,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe 'unknown6@sample.com'] }); respondToRefresh(catLoversInitialCount + 6, dogLoversInitialCount); - verifyMessage('There were 6 errors when trying to add students:', 'error', + verifyDetailedMessage('There were 6 errors when trying to add students:', 'error', [ unknownUserMessage('unknown1@sample.com'), unknownUserMessage('unknown2@sample.com'), unknownUserMessage('unknown3@sample.com'), unknownUserMessage('unknown4@sample.com'), @@ -190,8 +345,8 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe ); expect(getStudentInput().val()).toBe(sixUsers); // Click "View all" - cohortsView.$('a.action-primary').click(); - verifyMessage('There were 6 errors when trying to add students:', 'error', + cohortsView.$('.action-expand').click(); + verifyDetailedMessage('There were 6 errors when trying to add students:', 'error', [ unknownUserMessage('unknown1@sample.com'), unknownUserMessage('unknown2@sample.com'), unknownUserMessage('unknown3@sample.com'), unknownUserMessage('unknown4@sample.com'), @@ -202,11 +357,11 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe it('shows students moved from one cohort to another', function() { var sixUsers = 'moved1@sample.com, moved2@sample.com, moved3@sample.com, alreadypresent@sample.com'; - createCohortsView(this, "1"); + createCohortsView(this, 1); addStudents(sixUsers); AjaxHelpers.expectRequest(requests, 'POST', '/mock_service/1/add', - 'users=' + sixUsers.replace(/@/g, "%40").replace(/, /g, "%2C+") + 'users=' + sixUsers.replace(/@/g, "%40").replace(/, /g, "%2C+") ); respondToAdd({ changed: [ @@ -218,7 +373,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); respondToRefresh(); - verifyMessage('3 students have been added to this cohort group', 'confirmation', + verifyDetailedMessage('3 students have been added to this cohort group', 'confirmation', [ "2 students were removed from group 2", "1 student was removed from group 3", @@ -229,7 +384,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); it('shows a message when the add fails', function() { - createCohortsView(this, "1"); + createCohortsView(this, 1); addStudents('student@sample.com'); AjaxHelpers.respondWithError(requests); verifyMessage('Error adding students.', 'error'); @@ -237,7 +392,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); it('clears an error message on subsequent add', function() { - createCohortsView(this, "1"); + createCohortsView(this, 1); // First verify that an error is shown addStudents('student@sample.com'); diff --git a/lms/static/js/spec/views/notification_spec.js b/lms/static/js/spec/views/notification_spec.js index 4ee1d928de..66747b37b4 100644 --- a/lms/static/js/spec/views/notification_spec.js +++ b/lms/static/js/spec/views/notification_spec.js @@ -1,7 +1,7 @@ define(['backbone', 'jquery', 'js/models/notification', 'js/views/notification', 'js/common_helpers/template_helpers'], function (Backbone, $, NotificationModel, NotificationView, TemplateHelpers) { describe("NotificationView", function () { - var createNotification, verifyTitle, verifyDetails, verifyAction, notificationView; + var createNotification, verifyTitle, verifyMessage, verifyDetails, verifyAction, notificationView; createNotification = function (modelVals) { var notificationModel = new NotificationModel(modelVals); @@ -16,6 +16,10 @@ define(['backbone', 'jquery', 'js/models/notification', 'js/views/notification', expect(notificationView.$('.message-title').text().trim()).toBe(expectedTitle); }; + verifyMessage = function (expectedMessage) { + expect(notificationView.$('.message-copy').text().trim()).toBe(expectedMessage); + }; + verifyDetails = function (expectedDetails) { var details = notificationView.$('.summary-item'); expect(details.length).toBe(expectedDetails.length); @@ -41,7 +45,7 @@ define(['backbone', 'jquery', 'js/models/notification', 'js/views/notification', it('has default values', function () { createNotification({}); - expect(notificationView.$('div.message').hasClass('message-confirmation')).toBe(true); + expect(notificationView.$('div.message')).toHaveClass('message-confirmation'); verifyTitle(''); verifyDetails([]); verifyAction(null); @@ -49,8 +53,8 @@ define(['backbone', 'jquery', 'js/models/notification', 'js/views/notification', it('can use an error type', function () { createNotification({type: 'error'}); - expect(notificationView.$('div.message').hasClass('message-error')).toBe(true); - expect(notificationView.$('div.message').hasClass('message-confirmation')).toBe(false); + expect(notificationView.$('div.message')).toHaveClass('message-error'); + expect(notificationView.$('div.message')).not.toHaveClass('message-confirmation'); }); it('can specify a title', function () { @@ -58,6 +62,11 @@ define(['backbone', 'jquery', 'js/models/notification', 'js/views/notification', verifyTitle('notification title'); }); + it('can specify a message', function () { + createNotification({message: 'This is a dummy message'}); + verifyMessage('This is a dummy message'); + }); + it('can specify details', function () { var expectedDetails = ['detail 1', 'detail 2']; createNotification({details: expectedDetails}); @@ -69,9 +78,9 @@ define(['backbone', 'jquery', 'js/models/notification', 'js/views/notification', verifyAction('action text'); }); - it ('does not show an action button if callback is not provided', function () { + it ('shows an action button if only text is provided', function () { createNotification({actionText: 'action text'}); - verifyAction(null); + verifyAction('action text'); }); it ('does not show an action button if text is not provided', function () { @@ -82,7 +91,7 @@ define(['backbone', 'jquery', 'js/models/notification', 'js/views/notification', it ('triggers the callback when the action button is clicked', function () { var actionCallback = jasmine.createSpy('Spy on callback'); var view = createNotification({actionText: 'action text', actionCallback: actionCallback}); - notificationView.$('a.action-primary').click(); + notificationView.$('button.action-primary').click(); expect(actionCallback).toHaveBeenCalledWith(view); }); }); diff --git a/lms/static/js/views/cohort_editor.js b/lms/static/js/views/cohort_editor.js index 52183ec247..bb9294689c 100644 --- a/lms/static/js/views/cohort_editor.js +++ b/lms/static/js/views/cohort_editor.js @@ -101,7 +101,8 @@ addNotifications: function(modifiedUsers) { var oldCohort, title, details, numPresent, numUsersAdded, numErrors, - createErrorDetails, errorActionCallback, errorModel; + createErrorDetails, errorActionCallback, errorModel, + errorLimit = 5; // Show confirmation messages. this.undelegateViewEvents(this.confirmationNotifications); @@ -165,7 +166,7 @@ numErrors = modifiedUsers.unknown.length; if (numErrors > 0) { createErrorDetails = function (unknownUsers, showAllErrors) { - var numErrors = unknownUsers.length, details = [], errorLimit = 5; + var numErrors = unknownUsers.length, details = []; for (var i = 0; i < (showAllErrors ? numErrors : Math.min(errorLimit, numErrors)); i++) { details.push(interpolate_text(gettext("Unknown user: {user}"), {user: unknownUsers[i]})); @@ -181,15 +182,16 @@ details = createErrorDetails(modifiedUsers.unknown, false); errorActionCallback = function (view) { - view.model.set("actionCallback", null); + view.model.set("actionText", null); view.model.set("details", createErrorDetails(modifiedUsers.unknown, true)); view.render(); }; errorModel = new NotificationModel({ details: details, - actionText: gettext("View all errors"), - actionCallback: numErrors > 5 ? errorActionCallback : null + actionText: numErrors > errorLimit ? gettext("View all errors") : null, + actionCallback: errorActionCallback, + actionClass: 'action-expand' }); this.showErrorMessage(title, false, errorModel); diff --git a/lms/static/js/views/cohorts.js b/lms/static/js/views/cohorts.js index cce1035162..c7cf01aeba 100644 --- a/lms/static/js/views/cohorts.js +++ b/lms/static/js/views/cohorts.js @@ -1,12 +1,19 @@ -(function(Backbone, CohortEditorView) { - var CohortsView = Backbone.View.extend({ +(function($, _, Backbone, gettext, interpolate_text, CohortEditorView, NotificationModel, NotificationView) { + var hiddenClass = 'is-hidden', + disabledClass = 'is-disabled'; + + this.CohortsView = Backbone.View.extend({ events : { - "change .cohort-select": "showCohortEditor" + 'change .cohort-select': 'onCohortSelected', + 'click .action-create': 'showAddCohortForm', + 'click .action-cancel': 'cancelAddCohortForm', + 'click .action-save': 'saveAddCohortForm' }, initialize: function(options) { this.template = _.template($('#cohorts-tpl').text()); this.selectorTemplate = _.template($('#cohort-selector-tpl').text()); + this.addCohortFormTemplate = _.template($('#add-cohort-form-tpl').text()); this.advanced_settings_url = options.advanced_settings_url; this.model.on('sync', this.onSync, this); }, @@ -15,7 +22,7 @@ this.$el.html(this.template({ cohorts: this.model.models })); - this.renderSelector(); + this.onSync(); return this; }, @@ -27,7 +34,25 @@ }, onSync: function() { - this.renderSelector(this.getSelectedCohort()); + var selectedCohort = this.lastSelectedCohortId && this.model.get(this.lastSelectedCohortId), + hasCohorts = this.model.length > 0; + this.hideAddCohortForm(); + if (hasCohorts) { + this.$('.cohort-management-nav').removeClass(hiddenClass); + this.renderSelector(selectedCohort); + if (selectedCohort) { + this.showCohortEditor(selectedCohort); + } + } else { + this.$('.cohort-management-nav').addClass(hiddenClass); + this.showNotification({ + type: 'warning', + title: gettext('You currently have no cohort groups configured'), + actionText: gettext('Add Cohort Group'), + actionClass: 'action-create', + actionIconClass: 'icon-plus' + }); + } }, getSelectedCohort: function() { @@ -35,22 +60,116 @@ return id && this.model.get(parseInt(id)); }, - showCohortEditor: function(event) { + onCohortSelected: function(event) { event.preventDefault(); var selectedCohort = this.getSelectedCohort(); + this.lastSelectedCohortId = selectedCohort.get('id'); + this.showCohortEditor(selectedCohort); + }, + + showCohortEditor: function(cohort) { + this.removeNotification(); if (this.editor) { - this.editor.setCohort(selectedCohort); + this.editor.setCohort(cohort); } else { this.editor = new CohortEditorView({ el: this.$('.cohort-management-group'), - model: selectedCohort, + model: cohort, cohorts: this.model, advanced_settings_url: this.advanced_settings_url }); this.editor.render(); } + }, + + showNotification: function(options, beforeElement) { + var model = new NotificationModel(options); + this.removeNotification(); + this.notification = new NotificationView({ + model: model + }); + this.notification.render(); + if (!beforeElement) { + beforeElement = this.$('.cohort-management-group'); + } + beforeElement.before(this.notification.$el); + }, + + removeNotification: function() { + if (this.notification) { + this.notification.remove(); + } + }, + + showAddCohortForm: function(event) { + event.preventDefault(); + this.removeNotification(); + this.addCohortForm = $(this.addCohortFormTemplate({})); + this.addCohortForm.insertAfter(this.$('.cohort-management-nav')); + this.setCohortEditorVisibility(false); + }, + + hideAddCohortForm: function() { + this.setCohortEditorVisibility(true); + if (this.addCohortForm) { + this.addCohortForm.remove(); + this.addCohortForm = null; + } + }, + + setCohortEditorVisibility: function(showEditor) { + if (showEditor) { + this.$('.cohort-management-group').removeClass(hiddenClass); + this.$('.cohort-management-nav').removeClass(disabledClass); + } else { + this.$('.cohort-management-group').addClass(hiddenClass); + this.$('.cohort-management-nav').addClass(disabledClass); + } + }, + + saveAddCohortForm: function(event) { + event.preventDefault(); + var self = this, + showAddError, + cohortName = this.$('.cohort-create-name').val().trim(); + showAddError = function(message) { + self.showNotification( + {type: 'error', title: message}, + self.$('.cohort-management-create-form-name label') + ); + }; + this.removeNotification(); + if (cohortName.length > 0) { + $.post( + this.model.url + '/add', + {name: cohortName} + ).done(function(result) { + if (result.success) { + self.lastSelectedCohortId = result.cohort.id; + self.model.fetch().done(function() { + self.showNotification({ + type: 'confirmation', + title: interpolate_text( + gettext('The {cohortGroupName} cohort group has been created. You can manually add students to this group below.'), + {cohortGroupName: cohortName} + ) + }); + }); + } else { + showAddError(result.msg); + } + }).fail(function() { + showAddError(gettext("We've encountered an error. Please refresh your browser and then try again.")); + }); + } else { + showAddError(gettext('Please enter a name for your new cohort group.')); + } + }, + + cancelAddCohortForm: function(event) { + event.preventDefault(); + this.removeNotification(); + this.onSync(); } }); - - this.CohortsView = CohortsView; -}).call(this, Backbone, CohortEditorView); +}).call(this, $, _, Backbone, gettext, interpolate_text, CohortEditorView, NotificationModel, NotificationView); diff --git a/lms/static/js/views/notification.js b/lms/static/js/views/notification.js index 48c9eeefeb..3b94384029 100644 --- a/lms/static/js/views/notification.js +++ b/lms/static/js/views/notification.js @@ -9,17 +9,14 @@ }, render: function() { - var actionText = null; - // If no actionText is passed to the template, the action button - // will not be shown. - if (this.model.get("actionText") && this.model.get("actionCallback")) { - actionText = this.model.get("actionText"); - } this.$el.html(this.template({ type: this.model.get("type"), title: this.model.get("title"), + message: this.model.get("message"), details: this.model.get("details"), - actionText: actionText + actionText: this.model.get("actionText"), + actionClass: this.model.get("actionClass"), + actionIconClass: this.model.get("actionIconClass") })); return this; }, diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index d220271f4b..d530877f3f 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -454,8 +454,11 @@ section.instructor-dashboard-content-2 { .form-submit { @include idashbutton($blue); + @include font-size(14); + @include line-height(14); margin-right: ($baseline/2); margin-bottom: 0; + text-shadow: none; } .form-cancel { @@ -481,6 +484,21 @@ section.instructor-dashboard-content-2 { @include idashbutton($blue); float: right; text-align: right; + text-shadow: none; + font-weight: 600; + } + + // STATE: is disabled + &.is-disabled { + + + .cohort-select { + opacity: 0.25; + } + + .action-create { + opacity: 0.50; + } } } diff --git a/lms/templates/discussion/_js_head_dependencies.html b/lms/templates/discussion/_js_head_dependencies.html index dd437c33b3..4bf92ae79c 100644 --- a/lms/templates/discussion/_js_head_dependencies.html +++ b/lms/templates/discussion/_js_head_dependencies.html @@ -11,7 +11,6 @@ - diff --git a/lms/templates/instructor/instructor_dashboard_2/add-cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/add-cohort-form.underscore new file mode 100644 index 0000000000..8770977fe7 --- /dev/null +++ b/lms/templates/instructor/instructor_dashboard_2/add-cohort-form.underscore @@ -0,0 +1,26 @@ +
+
+ +

<%- gettext('Add a New Cohort Group') %>

+ +
+
+ + " required="required" /> +
+
+ +
+ + <%- gettext('Cancel') %> +
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore index 7ac3f498e3..36ee294685 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore @@ -1,4 +1,6 @@ - +<% if (!selectedCohort) { %> + +<% } %> <% _.each(cohorts, function(cohort) { %> <% var label = interpolate( diff --git a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore index d9714149d6..2cd0f98bc0 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore @@ -4,19 +4,24 @@ -<% if (cohorts.length > 0) { %> -
-
-
- - -
+
+ +
+ + +
- - -
+
+ +
+ - -
-<% } %> + + + <%- gettext('Add Cohort Group') %> + +
+ + +
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 865266b5c8..931f424b80 100644 --- a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html +++ b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html @@ -32,7 +32,6 @@ window.Range.prototype = { }; } - @@ -63,7 +62,7 @@ ## Include Underscore templates <%block name="header_extras"> -% for template_name in ["cohorts", "cohort-editor", "cohort-selector", "notification"]: +% for template_name in ["cohorts", "cohort-editor", "cohort-selector", "add-cohort-form", "notification"]: diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 5b17ba6aef..6a91820f3e 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -220,31 +220,30 @@ %if course.is_cohorted: -
-
-
-% endif +
+
+
-<%block name="headextra"> - - + <%block name="headextra"> + + +% endif diff --git a/lms/templates/instructor/instructor_dashboard_2/notification.underscore b/lms/templates/instructor/instructor_dashboard_2/notification.underscore index 39298c94b5..48b9b59be4 100644 --- a/lms/templates/instructor/instructor_dashboard_2/notification.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/notification.underscore @@ -3,22 +3,29 @@ <%- title %> - <% if (details.length > 0) { %> + <% if (details.length > 0 || message) { %>
- + <% if (message) { %> +

<%- message %>

+ <% } %> + <% if (details.length > 0) { %> + + <% } %>
<% } %> <% if (actionText) { %>
- <%- actionText %> +
<% } %> - diff --git a/lms/templates/instructor/staff_grading.html b/lms/templates/instructor/staff_grading.html index dfec9af5f8..dbf8536c18 100644 --- a/lms/templates/instructor/staff_grading.html +++ b/lms/templates/instructor/staff_grading.html @@ -6,7 +6,6 @@ <%block name="headextra"> <%static:css group='style-course-vendor'/> <%static:css group='style-course'/> - <%block name="pagetitle">${_("{course_number} Staff Grading").format(course_number=course.display_number_with_default) | h} diff --git a/lms/templates/ux/reference/instructor_dashboard/membership.html b/lms/templates/ux/reference/instructor_dashboard/membership.html index d95050cf7d..9a528c1b72 100644 --- a/lms/templates/ux/reference/instructor_dashboard/membership.html +++ b/lms/templates/ux/reference/instructor_dashboard/membership.html @@ -19,7 +19,6 @@ window.Range.prototype = { }; } - @@ -244,9 +243,10 @@
- + - No special characters or symbols are allowed in group names
@@ -337,7 +336,6 @@ (Required Field) - No special characters or symbols are allowed in group names @@ -356,10 +354,6 @@

New Cohort Name has been created. You can manually add students to this group below.

-
-

Special characters are not allowed in cohort group names

-
-