From fc5b94eeeb6453b788de9d923e307f27ebc6fa41 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Mon, 5 Jan 2015 10:38:52 -0500 Subject: [PATCH] Address most of Christina's code review comments --- .../pages/lms/instructor_dashboard.py | 4 +- .../discussion/test_cohort_management.py | 16 +-- lms/static/js/groups/models/content_group.js | 3 +- lms/static/js/groups/views/cohort_editor.js | 19 +-- lms/static/js/groups/views/cohort_form.js | 74 ++++++----- lms/static/js/groups/views/cohorts.js | 24 ++-- .../js/spec/groups/views/cohorts_spec.js | 122 +++++++++++++----- .../sass/course/instructor/_instructor_2.scss | 11 +- .../cohort-editor.underscore | 14 +- .../cohort-form.underscore | 34 ++--- .../instructor_dashboard_2/membership.html | 13 +- test_root/uploads/nonunique_store.txt | 1 - test_root/uploads/nonunique_store_keDQRg1.txt | 1 - 13 files changed, 206 insertions(+), 130 deletions(-) delete mode 100755 test_root/uploads/nonunique_store.txt delete mode 100755 test_root/uploads/nonunique_store_keDQRg1.txt diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index be4e4d9aa3..3d69333fe0 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -201,7 +201,7 @@ class MembershipPageCohortManagementSection(PageObject): select = self.q(css=self._bounded_selector(self.content_group_selector)) groups = [] for option in select: - if option.text != "Choose a content group to associate": + if option.text != "": groups.append(option.text) return groups @@ -213,7 +213,7 @@ class MembershipPageCohortManagementSection(PageObject): """ self.select_cohort_settings() radio_button = self.q(css=self._bounded_selector(self.no_content_group_button)).results[0] - if radio_button.get_attribute("checked") == "true": + if radio_button.is_selected(): return None option_selector = self.q(css=self._bounded_selector(self.content_group_selector)) return option_selector.filter(lambda el: el.is_selected())[0].text diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index b2ac786c18..5b649ab031 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -88,11 +88,12 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): """ self.verify_cohort_description( self.manual_cohort_name, - 'Students are added to this group only when you provide their email addresses or usernames on this page', + 'Students are added to this cohort group only when you provide ' + 'their email addresses or usernames on this page', ) self.verify_cohort_description( self.auto_cohort_name, - 'Students are added to this group automatically', + 'Students are added to this cohort group automatically', ) def test_no_content_groups(self): @@ -110,7 +111,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): self.cohort_management_page.select_cohort(self.manual_cohort_name) self.assertIsNone(self.cohort_management_page.get_cohort_associated_content_group()) self.assertEqual( - "You haven't configured any content groups yet. You need to create a content group before you can create assignments. Create a content group", + "No content groups exist. Create a content group to associate with cohort groups. Create a content group", self.cohort_management_page.get_cohort_related_content_group_message() ) # TODO: test can't select radio button @@ -346,7 +347,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): start_time = datetime.now(UTC) self.cohort_management_page.upload_cohort_file(filename) self._verify_cohort_by_csv_notification( - "Your file '{}' has been uploaded. Please allow a few minutes for processing.".format(filename) + "Your file '{}' has been uploaded. Allow a few minutes for processing.".format(filename) ) # student_user is moved from manual cohort group to auto cohort group @@ -601,7 +602,7 @@ class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): self.browser.refresh() self.cohort_management_page.wait_for_page() self.cohort_management_page.select_cohort(new_cohort) - self.assertEqual("Some content group that's been deleted", self.cohort_management_page.get_cohort_associated_content_group()) + self.assertEqual("Deleted Content Group", self.cohort_management_page.get_cohort_associated_content_group()) self.assertEquals(["Bananas", "Pears", "Some content group that's been deleted"], self.cohort_management_page.get_all_content_groups()) self.assertEqual( "The selected content group has been deleted, you may wish to reassign this cohort group.", @@ -610,9 +611,8 @@ class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): self.cohort_management_page.set_cohort_associated_content_group("Pears") confirmation_messages = self.cohort_management_page.get_cohort_settings_messages() self.assertEqual(["Saved cohort group."], confirmation_messages) - # TODO: uncomment - # self.assertIsNone(self.cohort_management_page.get_cohort_related_content_group_message()) - # self.assertEquals(["Bananas", "Pears"], self.cohort_management_page.get_all_content_groups()) + self.assertIsNone(self.cohort_management_page.get_cohort_related_content_group_message()) + self.assertEquals(["Bananas", "Pears"], self.cohort_management_page.get_all_content_groups()) def _create_new_cohort_linked_to_content_group(self, new_cohort, cohort_group): """ diff --git a/lms/static/js/groups/models/content_group.js b/lms/static/js/groups/models/content_group.js index d52abe4b8e..bb8077518e 100644 --- a/lms/static/js/groups/models/content_group.js +++ b/lms/static/js/groups/models/content_group.js @@ -8,7 +8,8 @@ var edx = edx || {}; edx.groups.ContentGroupModel = Backbone.Model.extend({ idAttribute: 'id', defaults: { - name: '' + name: '', + user_partition_id: null } }); }).call(this, Backbone); diff --git a/lms/static/js/groups/views/cohort_editor.js b/lms/static/js/groups/views/cohort_editor.js index 6b808a1745..da243bcc8d 100644 --- a/lms/static/js/groups/views/cohort_editor.js +++ b/lms/static/js/groups/views/cohort_editor.js @@ -15,9 +15,8 @@ var edx = edx || {}; initialize: function(options) { this.template = _.template($('#cohort-editor-tpl').text()); this.cohorts = options.cohorts; - this.cohortUserPartitionId = options.cohortUserPartitionId; this.contentGroups = options.contentGroups; - this.advanced_settings_url = options.advanced_settings_url; + this.context = options.context; }, // Any errors that are currently being displayed to the instructor (for example, unknown email addresses). @@ -28,14 +27,12 @@ var edx = edx || {}; render: function() { this.$el.html(this.template({ cohort: this.model, - cohortUserPartitionId: this.cohortUserPartitionId, - contentGroups: this.contentGroups, - advanced_settings_url: this.advanced_settings_url + studioAdvancedSettingsUrl: this.context.studioAdvancedSettingsUrl })); this.cohortFormView = new CohortFormView({ model: this.model, - cohortUserPartitionId: this.cohortUserPartitionId, - contentGroups: this.contentGroups + contentGroups: this.contentGroups, + context: this.context }); this.cohortFormView.render(); this.$('.tab-content-settings').append(this.cohortFormView.$el); @@ -53,8 +50,12 @@ var edx = edx || {}; }, saveSettings: function(event) { + var cohortFormView = this.cohortFormView; event.preventDefault(); - this.cohortFormView.saveForm(); + cohortFormView.saveForm() + .done(function() { + cohortFormView.showMessage(gettext('Saved cohort group.')); + }); }, setCohort: function(cohort) { @@ -94,7 +95,7 @@ var edx = edx || {}; self.showErrorMessage(gettext('Error adding students.'), true); }); } else { - self.showErrorMessage(gettext('Please enter a username or email.'), true); + self.showErrorMessage(gettext('Enter a username or email.'), true); input.val(''); } }, diff --git a/lms/static/js/groups/views/cohort_form.js b/lms/static/js/groups/views/cohort_form.js index be67affc53..dc0ad23513 100644 --- a/lms/static/js/groups/views/cohort_form.js +++ b/lms/static/js/groups/views/cohort_form.js @@ -8,15 +8,14 @@ var edx = edx || {}; edx.groups.CohortFormView = Backbone.View.extend({ events : { 'change .cohort-management-details-association-course input': 'onRadioButtonChange', - 'change .input-cohort-group-association': 'onGroupAssociationChange', 'click .tab-content-settings .action-save': 'saveSettings', 'submit .cohort-management-group-add-form': 'addStudents' }, initialize: function(options) { this.template = _.template($('#cohort-form-tpl').text()); - this.cohortUserPartitionId = options.cohortUserPartitionId; this.contentGroups = options.contentGroups; + this.context = options.context; }, showNotification: function(options, beforeElement) { @@ -26,9 +25,6 @@ var edx = edx || {}; model: model }); this.notification.render(); - if (!beforeElement) { - beforeElement = this.$('.cohort-management-group'); - } beforeElement.before(this.notification.$el); }, @@ -41,7 +37,8 @@ var edx = edx || {}; render: function() { this.$el.html(this.template({ cohort: this.model, - contentGroups: this.contentGroups + contentGroups: this.contentGroups, + studioGroupConfigurationsUrl: this.context.studioGroupConfigurationsUrl })); return this; }, @@ -51,22 +48,29 @@ var edx = edx || {}; groupsEnabled = target.val() === 'yes'; if (!groupsEnabled) { // If the user has chosen 'no', then clear the selection by setting - // it to the first option ('Choose a content group to associate'). + // it to the first option which represents no selection. this.$('.input-cohort-group-association').val('None'); } + // Enable the select if the user has chosen groups, else disable it + this.$('.input-cohort-group-association').prop('disabled', !groupsEnabled); }, - onGroupAssociationChange: function(event) { - // Since the user has chosen a content group, click the 'Yes' button too - this.$('.cohort-management-details-association-course .radio-yes').click(); - }, - - getSelectedGroupId: function() { - var selectValue = this.$('.input-cohort-group-association').val(); + getSelectedContentGroup: function() { + var selectValue = this.$('.input-cohort-group-association').val(), + ids, groupId, userPartitionId, i, contentGroup; if (!this.$('.radio-yes').prop('checked') || selectValue === 'None') { return null; } - return parseInt(selectValue); + ids = selectValue.split(':'); + groupId = parseInt(ids[0]); + userPartitionId = parseInt(ids[1]); + for (i=0; i < this.contentGroups.length; i++) { + contentGroup = this.contentGroups[i]; + if (contentGroup.get('id') === groupId && contentGroup.get('user_partition_id') === userPartitionId) { + return contentGroup; + } + } + return null; }, getUpdatedCohortName: function() { @@ -74,37 +78,43 @@ var edx = edx || {}; return cohortName ? cohortName.trim() : this.model.get('name'); }, + showMessage: function(message, type) { + this.showNotification( + {type: type || 'confirmation', title: message}, + this.$('.form-fields') + ); + }, + saveForm: function() { var self = this, cohort = this.model, saveOperation = $.Deferred(), - cohortName, groupId, showMessage, showAddError; + isUpdate = this.model.id !== null, + cohortName, selectedContentGroup, showErrorMessage; this.removeNotification(); - showMessage = function(message, type) { - self.showNotification( - {type: type || 'confirmation', title: message}, - self.$('.form-fields') - ); - }; - showAddError = function(message, type) { - showMessage(message, 'error'); + showErrorMessage = function(message) { + self.showMessage(message, 'error'); }; cohortName = this.getUpdatedCohortName(); if (cohortName.length === 0) { - showAddError(gettext('Please enter a name for your new cohort group.')); + showErrorMessage(gettext('Enter a name for your cohort group.')); saveOperation.reject(); } else { - groupId = this.getSelectedGroupId(); + selectedContentGroup = this.getSelectedContentGroup(); cohort.save( - {name: cohortName, user_partition_id: this.cohortUserPartitionId, group_id: groupId}, - {patch: true} + { + name: cohortName, + group_id: selectedContentGroup ? selectedContentGroup.id : null, + user_partition_id: selectedContentGroup ? selectedContentGroup.get('user_partition_id') : null + }, + {patch: isUpdate} ).done(function(result) { if (!result.error) { cohort.id = result.id; - showMessage(gettext('Saved cohort group.')); + self.render(); // re-render to remove any now invalid error messages saveOperation.resolve(); } else { - showAddError(result.error); + showErrorMessage(result.error); saveOperation.reject(); } }).fail(function(result) { @@ -116,9 +126,9 @@ var edx = edx || {}; // Ignore the exception and show the default error message instead. } if (!errorMessage) { - errorMessage = gettext("We've encountered an error. Please refresh your browser and then try again."); + errorMessage = gettext("We've encountered an error. Refresh your browser and then try again."); } - showAddError(errorMessage); + showErrorMessage(errorMessage); saveOperation.reject(); }); } diff --git a/lms/static/js/groups/views/cohorts.js b/lms/static/js/groups/views/cohorts.js index d5f02b2c6c..9efa8054de 100644 --- a/lms/static/js/groups/views/cohorts.js +++ b/lms/static/js/groups/views/cohorts.js @@ -24,9 +24,7 @@ var edx = edx || {}; this.template = _.template($('#cohorts-tpl').text()); this.selectorTemplate = _.template($('#cohort-selector-tpl').text()); - this.advanced_settings_url = options.advanced_settings_url; - this.upload_cohorts_csv_url = options.upload_cohorts_csv_url; - this.cohortUserPartitionId = options.cohortUserPartitionId; + this.context = options.context; this.contentGroups = options.contentGroups; model.on('sync', this.onSync, this); @@ -58,9 +56,14 @@ var edx = edx || {}; hasCohorts = this.model.length > 0, cohortNavElement = this.$('.cohort-management-nav'), additionalCohortControlElement = this.$('.wrapper-cohort-supplemental'), - isModelUpdate = options && options.patch && response.hasOwnProperty('user_partition_id'); + isModelUpdate; + isModelUpdate = function() { + // Distinguish whether this is a sync event for just one model, or if it is for + // an entire collection. + return options && options.patch && response.hasOwnProperty('user_partition_id'); + }; this.hideAddCohortForm(); - if (isModelUpdate) { + if (isModelUpdate()) { // Refresh the selector in case the model's name changed this.renderSelector(selectedCohort); } else if (hasCohorts) { @@ -104,9 +107,8 @@ var edx = edx || {}; el: this.$('.cohort-management-group'), model: cohort, cohorts: this.model, - cohortUserPartitionId: this.cohortUserPartitionId, contentGroups: this.contentGroups, - advanced_settings_url: this.advanced_settings_url + context: this.context }); this.editor.render(); } @@ -142,8 +144,8 @@ var edx = edx || {}; newCohort.url = this.model.url; this.cohortFormView = new CohortFormView({ model: newCohort, - cohortUserPartitionId: this.cohortUserPartitionId, - contentGroups: this.contentGroups + contentGroups: this.contentGroups, + context: this.context }); this.cohortFormView.render(); this.$('.cohort-management-add-modal').append(this.cohortFormView.$el); @@ -215,10 +217,10 @@ var edx = edx || {}; inputTip: gettext("Only properly formatted .csv files will be accepted."), submitButtonText: gettext("Upload File and Assign Students"), extensions: ".csv", - url: this.upload_cohorts_csv_url, + url: this.context.uploadCohortsCsvUrl, successNotification: function (file, event, data) { var message = interpolate_text(gettext( - "Your file '{file}' has been uploaded. Please allow a few minutes for processing." + "Your file '{file}' has been uploaded. Allow a few minutes for processing." ), {file: file}); return new NotificationModel({ type: "confirmation", diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js index cf9484599e..3331adf766 100644 --- a/lms/static/js/spec/groups/views/cohorts_spec.js +++ b/lms/static/js/spec/groups/views/cohorts_spec.js @@ -7,13 +7,20 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe var catLoversInitialCount = 123, dogLoversInitialCount = 456, unknownUserMessage, createMockCohort, createMockCohorts, createMockContentGroups, createCohortsView, cohortsView, requests, respondToRefresh, verifyMessage, verifyNoMessage, verifyDetailedMessage, verifyHeader, - expectCohortAddRequest, getAddModal, selectContentGroup, clearContentGroup; + expectCohortAddRequest, getAddModal, selectContentGroup, clearContentGroup, + MOCK_COHORTED_USER_PARTITION_ID, MOCK_UPLOAD_COHORTS_CSV_URL, MOCK_STUDIO_ADVANCED_SETTINGS_URL, + MOCK_STUDIO_GROUP_CONFIGURATIONS_URL; + + MOCK_COHORTED_USER_PARTITION_ID = 0; + MOCK_UPLOAD_COHORTS_CSV_URL = 'http://upload-csv-file-url/'; + MOCK_STUDIO_ADVANCED_SETTINGS_URL = 'http://studio/settings/advanced'; + MOCK_STUDIO_GROUP_CONFIGURATIONS_URL = 'http://studio/group_configurations'; createMockCohort = function (name, id, userCount, groupId, userPartitionId) { return { - id: id || 1, + id: id !== undefined ? id : 1, name: name, - user_count: userCount || 0, + user_count: userCount !== undefined ? userCount : 0, group_id: groupId, user_partition_id: userPartitionId }; @@ -30,8 +37,12 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe createMockContentGroups = function () { return [ - new ContentGroupModel({id: 0, name: 'Dog Content'}), - new ContentGroupModel({id: 1, name: 'Cat Content'}) + new ContentGroupModel({ + id: 0, name: 'Dog Content', user_partition_id: MOCK_COHORTED_USER_PARTITION_ID + }), + new ContentGroupModel({ + id: 1, name: 'Cat Content', user_partition_id: MOCK_COHORTED_USER_PARTITION_ID + }) ]; }; @@ -46,7 +57,11 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView = new CohortsView({ model: cohorts, contentGroups: contentGroups, - upload_cohorts_csv_url: "http://upload-csv-file-url/" + context: { + uploadCohortsCsvUrl: MOCK_UPLOAD_COHORTS_CSV_URL, + studioAdvancedSettingsUrl: MOCK_STUDIO_ADVANCED_SETTINGS_URL, + studioGroupConfigurationsUrl: MOCK_STUDIO_GROUP_CONFIGURATIONS_URL + } }); cohortsView.render(); if (options && options.selectCohort) { @@ -58,14 +73,15 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe AjaxHelpers.respondWithJson(requests, createMockCohorts(catCount, dogCount)); }; - expectCohortAddRequest = function(name, group_id) { + expectCohortAddRequest = function(name, groupId, userPartitionId) { AjaxHelpers.expectJsonRequest( requests, 'POST', '/mock_service/cohorts', { name: name, user_count: 0, assignment_type: '', - group_id: group_id + group_id: groupId, + user_partition_id: userPartitionId } ); }; @@ -74,14 +90,16 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe return cohortsView.$('.cohort-management-add-modal'); }; - selectContentGroup = function(values) { + selectContentGroup = function(groupId, userPartitionId) { + var ids = groupId + ':' + userPartitionId; cohortsView.$('.radio-yes').prop('checked', true).change(); - cohortsView.$('.input-cohort-group-association').val(values).change(); + cohortsView.$('.input-cohort-group-association').val(ids).change(); + expect(cohortsView.$('.input-cohort-group-association').prop('disabled')).toBeFalsy(); }; clearContentGroup = function() { cohortsView.$('.radio-no').prop('checked', true).change(); - expect(cohortsView.$('.radio-yes').prop('checked')).toBeFalsy(); + expect(cohortsView.$('.input-cohort-group-association').prop('disabled')).toBeTruthy(); expect(cohortsView.$('.input-cohort-group-association').val()).toBe('None'); }; @@ -145,7 +163,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe TemplateHelpers.installTemplate('templates/file-upload'); }); - it("Show an error if no cohorts are defined", function() { + it("shows an error if no cohorts are defined", function() { createCohortsView(this, {cohorts: []}); verifyMessage( 'You currently have no cohort groups configured', @@ -157,7 +175,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe expect(cohortsView.$('.wrapper-cohort-supplemental')).toHaveClass('is-hidden'); }); - it("Syncs data when membership tab is clicked", function() { + it("syncs data when membership tab is clicked", function() { createCohortsView(this, {selectCohort: 1}); verifyHeader(1, 'Cat Lovers', catLoversInitialCount); $(cohortsView.getSectionCss("membership")).click(); @@ -188,10 +206,10 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.submit-file-button').click(); // No file will actually be uploaded because "uploaded_file.txt" doesn't actually exist. - AjaxHelpers.expectRequest(requests, 'POST', "http://upload-csv-file-url/", new FormData()); + AjaxHelpers.expectRequest(requests, 'POST', MOCK_UPLOAD_COHORTS_CSV_URL, new FormData()); AjaxHelpers.respondWithJson(requests, {}); expect(cohortsView.$('.file-upload-form-result .message-confirmation .message-title').text().trim()) - .toBe("Your file 'upload_file.txt' has been uploaded. Please allow a few minutes for processing."); + .toBe("Your file 'upload_file.txt' has been uploaded. Allow a few minutes for processing."); }); describe("Cohort Selector", function () { @@ -208,7 +226,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe it('can switch cohort', function () { createCohortsView(this, {selectCohort: 1}); - cohortsView.$('.cohort-select').val("2").change(); + cohortsView.$('.cohort-select').val('2').change(); verifyHeader(2, 'Dog Lovers', dogLoversInitialCount); }); }); @@ -236,22 +254,24 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe var defaultCohortName = 'New Cohort'; it("can add a cohort", function() { - var contentGroupId = 0; + var contentGroupId = 0, + contentGroupUserPartitionId = 0; createCohortsView(this, {cohorts: []}); cohortsView.$('.action-create').click(); expect(cohortsView.$('.cohort-management-settings-form').length).toBe(1); expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled'); expect(cohortsView.$('.cohort-management-group')).toHaveClass('is-hidden'); cohortsView.$('.cohort-name').val(defaultCohortName); - selectContentGroup(contentGroupId); + selectContentGroup(contentGroupId, MOCK_COHORTED_USER_PARTITION_ID); cohortsView.$('.action-save').click(); - expectCohortAddRequest(defaultCohortName, contentGroupId); + expectCohortAddRequest(defaultCohortName, contentGroupId, MOCK_COHORTED_USER_PARTITION_ID); AjaxHelpers.respondWithJson( requests, { id: 1, name: defaultCohortName, - group_id: contentGroupId + group_id: contentGroupId, + user_partition_id: MOCK_COHORTED_USER_PARTITION_ID } ); AjaxHelpers.respondWithJson( @@ -274,7 +294,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.action-create').click(); cohortsView.$('.cohort-name').val(' New Cohort '); cohortsView.$('.action-save').click(); - expectCohortAddRequest('New Cohort', null); + expectCohortAddRequest('New Cohort', null, null); }); it("does not allow a blank cohort name to be submitted", function() { @@ -285,7 +305,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled'); getAddModal().find('.action-save').click(); expect(requests.length).toBe(0); - verifyMessage('Please enter a name for your new cohort group.', 'error'); + verifyMessage('Enter a name for your cohort group.', 'error'); }); it("shows a message when adding a cohort returns a server error", function() { @@ -336,10 +356,10 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.action-create').click(); cohortsView.$('.cohort-name').val(''); cohortsView.$('.action-save').click(); - verifyMessage('Please enter a name for your new cohort group.', 'error'); + verifyMessage('Enter a name for your cohort group.', 'error'); // Now switch to a different cohort - cohortsView.$('.cohort-select').val("2").change(); + cohortsView.$('.cohort-select').val('2').change(); verifyHeader(2, 'Dog Lovers', dogLoversInitialCount); verifyNoMessage(); }); @@ -351,7 +371,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.action-create').click(); cohortsView.$('.cohort-name').val(''); cohortsView.$('.action-save').click(); - verifyMessage('Please enter a name for your new cohort group.', 'error'); + verifyMessage('Enter a name for your cohort group.', 'error'); // Now cancel the form cohortsView.$('.action-cancel').click(); @@ -382,7 +402,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe createCohortsView(this, {selectCohort: 1}); addStudents(' '); expect(requests.length).toBe(0); - verifyMessage('Please enter a username or email.', 'error'); + verifyMessage('Enter a username or email.', 'error'); expect(getStudentInput().val()).toBe(''); }); @@ -508,9 +528,10 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe var options; createCohortsView(this, {selectCohort: 1}); cohortsView.$('.tab-settings a').click(); + expect(cohortsView.$('.input-cohort-group-association').prop('disabled')).toBeTruthy(); options = cohortsView.$('.input-cohort-group-association option'); expect(options.length).toBe(3); - expect($(options[0]).text().trim()).toBe('Choose a content group to associate'); + expect($(options[0]).text().trim()).toBe(''); expect($(options[1]).text().trim()).toBe('Cat Content'); expect($(options[2]).text().trim()).toBe('Dog Content'); }); @@ -520,7 +541,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.tab-settings a').click(); // Select the content group with id 1 and verify the radio button was switched to 'Yes' - selectContentGroup(0); + selectContentGroup(0, MOCK_COHORTED_USER_PARTITION_ID); expect(cohortsView.$('.radio-yes').prop('checked')).toBeTruthy(); // Click the save button and verify that the correct request is sent @@ -529,7 +550,8 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe requests, 'PATCH', '/mock_service/cohorts/1', { name: 'Cat Lovers', - group_id: 0 + group_id: 0, + user_partition_id: MOCK_COHORTED_USER_PARTITION_ID } ); AjaxHelpers.respondWithJson( @@ -556,7 +578,8 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe requests, 'PATCH', '/mock_service/cohorts/1', { name: 'Cat Lovers', - group_id: null + group_id: null, + user_partition_id: null } ); AjaxHelpers.respondWithJson( @@ -566,6 +589,30 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe verifyMessage('Saved cohort group.', 'confirmation'); }); + it("can clear a selected content group which had been deleted", function () { + createCohortsView(this, { + cohorts: [ + {id: 1, name: 'Cat Lovers', group_id: 999} + ], + selectCohort: 1 + }); + cohortsView.$('.tab-settings a').click(); + expect(cohortsView.$('.radio-yes').prop('checked')).toBeTruthy(); + clearContentGroup(); + + // Click the save button and verify that the correct request is sent + cohortsView.$('.action-save').click(); + AjaxHelpers.respondWithJson( + requests, + createMockCohort('Cat Lovers', 1, catLoversInitialCount, 0, 0) + ); + verifyMessage('Saved cohort group.', 'confirmation'); + + // Verify that the deleted content group and associated message have been removed + expect(cohortsView.$('option.option-unavailable').text().trim()).toBe(''); + expect(cohortsView.$('.copy-error').text().trim()).toBe(''); + }); + it("shows a message when the selected content group does not exist", function () { createCohortsView(this, { cohorts: [ @@ -574,8 +621,9 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe selectCohort: 1 }); cohortsView.$('.tab-settings a').click(); + expect(cohortsView.$('option.option-unavailable').text().trim()).toBe('Deleted Content Group'); expect(cohortsView.$('.copy-error').text().trim()).toBe( - 'The selected content group has been deleted, you may wish to reassign this cohort group.' + 'The previously selected content group was deleted. Select another content group.' ); }); @@ -585,7 +633,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.action-save').click(); AjaxHelpers.respondWithError(requests); verifyMessage( - 'We\'ve encountered an error. Please refresh your browser and then try again.', + 'We\'ve encountered an error. Refresh your browser and then try again.', 'error' ); }); @@ -593,10 +641,14 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe it("shows an error message when no content groups are specified", function () { createCohortsView(this, {selectCohort: 1, contentGroups: []}); cohortsView.$('.tab-settings a').click(); + expect(cohortsView.$('.radio-yes').prop('disabled')).toBeTruthy(); + expect(cohortsView.$('.msg-inline').text().trim()).toBe( + 'No content groups exist. Create a content group to associate with cohort groups. ' + + 'Create a content group' + ); expect( - cohortsView.$('.msg-inline').text().trim(), - 'You haven\'t configured any content groups yet. You need to create a content group ' + - 'before you can create assignments.' + cohortsView.$('.msg-inline a').attr('href'), + MOCK_STUDIO_GROUP_CONFIGURATIONS_URL ); }); }); diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index 9986772314..9b77a55ad9 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -57,10 +57,15 @@ // TYPE: inline .msg-inline { - @extend %t-copy-sub2; + // @extend %t-copy-sub1; display: inline-block; margin: 0 0 0 $baseline; padding: 0; + + .icon, + .fa { + margin-right: ($baseline/2); + } } // TYPE: warning @@ -1085,7 +1090,7 @@ a { display: inline-block; - padding: $baseline; + padding: ($baseline - 5); -webkit-transition: none; -moz-transition: none; -ms-transition: none; @@ -1096,7 +1101,7 @@ &.is-selected { // Active or selected tabs (
  • ) get this class. Also useful for aria stuff if ever implemented in the future. a { - padding-bottom: ($baseline+($baseline/5)); + padding-bottom: ($baseline - 5); border-style: solid; border-width: ($baseline/5) ($baseline/5) 0 ($baseline/5); border-color: $gray-l4; diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore index 60a39690af..65f2a68a66 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore @@ -13,16 +13,16 @@
    <% if (cohort.get('assignment_type') == "none") { %> - <%- gettext("Students are added to this group only when you provide their email addresses or usernames on this page.") %> + <%- gettext("Students are added to this cohort group only when you provide their email addresses or usernames on this page.") %> <%= gettext("What does this mean?") %> <% } else { %> - <%- gettext("Students are added to this group automatically.") %> + <%- gettext("Students are added to this cohort group automatically.") %> <%- gettext("What does this mean?") %> <% } %>
    - <% if (advanced_settings_url != "None") { %> - <%- gettext("Edit settings in Studio") %> + <% if (studioAdvancedSettingsUrl !== "None") { %> + <%- gettext("Edit settings in Studio") %> <% } %>
    @@ -39,7 +39,7 @@

    <%- gettext('Add students to this cohort group') %>

    -

    <%- gettext('Note: Students can only be in one cohort group. Adding students to this group overrides any previous group assignment.') %>

    +

    <%- gettext('Note: Students can be in only one cohort group. Adding students to this group overrides any previous group assignment.') %>

    @@ -48,14 +48,14 @@
    - <%- gettext('You will not get notification for emails that bounce, so please double-check spelling.') %> + <%- gettext('You will not receive notification for emails that bounce, so double-check your spelling.') %>
    diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore index 0311df107f..a1b5422590 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore @@ -14,12 +14,12 @@
    " required="required" /> + placeholder="<%- gettext("Enter the name of the cohort group") %>" required="required" />
    <% } %> @@ -28,20 +28,21 @@ var foundSelected = false; var selectedContentGroupId = cohort.get('group_id'); var hasSelectedContentGroup = selectedContentGroupId != null; + var hasContentGroups = contentGroups.length > 0; %>
    - +
    - - <% if (contentGroups.length > 0) { %> + + <% if (hasContentGroups) { %>
    - > + <% var orderedContentGroups = _.sortBy( @@ -49,33 +50,34 @@ function(group) { return group.get('name'); } ); for (var i=0; i < orderedContentGroups.length; i++) { - var contentGroup = orderedContentGroups[i]; - var contentGroupId = contentGroup.get('id'); - var isSelected = contentGroupId == selectedContentGroupId; + var contentGroup = orderedContentGroups[i], + contentGroupUserPartitionId = contentGroup.get('user_partition_id'), + contentGroupId = contentGroup.get('id'), + isSelected = contentGroupId == selectedContentGroupId; if (isSelected) { foundSelected = true; } %> - + <% } %> <% if (hasSelectedContentGroup && !foundSelected) { %> - + <% } %> <% if (hasSelectedContentGroup && !foundSelected) { %>
    -

    <%- gettext("The selected content group has been deleted, you may wish to reassign this cohort group.") %>

    +

    <%- gettext("The previously selected content group was deleted. Select another content group.") %>

    <% } %>
    <% } else { // no content groups available %> -
    +
    -

    You haven't configured any content groups yet. You need to create a content group before you can create assignments. Create a content group

    +

    <%- gettext("No content groups exist. Create a content group to associate with cohort groups.") %> <%- gettext("Create a content group") %>

    <% } %> diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 1e198a0ebc..b53b47e987 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -1,5 +1,6 @@ <%! from django.utils.translation import ugettext as _ %> <%page args="section_data"/> +<%! from courseware.courses import get_studio_url %> <%! from microsite_configuration import microsite %> <%! from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_user_partition %> @@ -264,11 +265,13 @@ var cohortManagementElement = $('.cohort-management'); if (cohortManagementElement.length > 0) { var cohorts = new edx.groups.CohortCollection(), + cohortUserPartitionId = ${cohorted_user_partition.id if cohorted_user_partition else 'null'}, contentGroups = [ % for content_group in content_groups: new edx.groups.ContentGroupModel({ id: ${content_group.id}, - name: "${content_group.name | h}" + name: "${content_group.name | h}", + user_partition_id: cohortUserPartitionId }), % endfor ]; @@ -276,10 +279,12 @@ var cohortsView = new edx.groups.CohortsView({ el: cohortManagementElement, model: cohorts, - cohortUserPartitionId: ${cohorted_user_partition.id if cohorted_user_partition else 'null'}, contentGroups: contentGroups, - advanced_settings_url: cohortManagementElement.data('advanced-settings-url'), - upload_cohorts_csv_url: cohortManagementElement.data('upload_cohorts_csv_url') + context: { + uploadCohortsCsvUrl: cohortManagementElement.data('upload_cohorts_csv_url'), + studioAdvancedSettingsUrl: cohortManagementElement.data('advanced-settings-url'), + studioGroupConfigurationsUrl: '${get_studio_url(course, 'group_configurations') | h}' + } }); cohorts.fetch().done(function() { cohortsView.render(); diff --git a/test_root/uploads/nonunique_store.txt b/test_root/uploads/nonunique_store.txt deleted file mode 100755 index 88740b2cb9..0000000000 --- a/test_root/uploads/nonunique_store.txt +++ /dev/null @@ -1 +0,0 @@ -copy \ No newline at end of file diff --git a/test_root/uploads/nonunique_store_keDQRg1.txt b/test_root/uploads/nonunique_store_keDQRg1.txt deleted file mode 100755 index 88740b2cb9..0000000000 --- a/test_root/uploads/nonunique_store_keDQRg1.txt +++ /dev/null @@ -1 +0,0 @@ -copy \ No newline at end of file