Skip to content

E
edx-platform
Commit fc5b94ee by Andy Armstrong
Options

Address most of Christina's code review comments

parent 8e787b4f
Showing with 119 additions and 97 deletions
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
......
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):
"""
......
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);
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('');
}
},
......
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();
});
}
......
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",
......
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 (<li>) 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;
......
lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore
......@@ -13,16 +13,16 @@
<div class="cohort-management-group-setup">
<div class="setup-value">
<% 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.") %>
<a href="http://edx.readthedocs.org/projects/edx-partner-course-staff/en/latest/cohorts/cohort_config.html#assign-students-to-cohort-groups-manually" class="incontext-help action-secondary action-help"><%= gettext("What does this mean?") %></a>
<% } else { %>
<%- gettext("Students are added to this group automatically.") %>
<%- gettext("Students are added to this cohort group automatically.") %>
<a href="http://edx.readthedocs.org/projects/edx-partner-course-staff/en/latest/cohorts/cohorts_overview.html#all-automated-assignment" class="incontext-help action-secondary action-help"><%- gettext("What does this mean?") %></a>
<% } %>
</div>
<div class="setup-actions">
<% if (advanced_settings_url != "None") { %>
<a href="<%= advanced_settings_url %>" class="action-secondary action-edit"><%- gettext("Edit settings in Studio") %></a>
<% if (studioAdvancedSettingsUrl !== "None") { %>
<a href="<%= studioAdvancedSettingsUrl %>" class="action-secondary action-edit"><%- gettext("Edit settings in Studio") %></a>
<% } %>
</div>
</div>
......@@ -39,7 +39,7 @@
<h4 class="form-title"><%- gettext('Add students to this cohort group') %></h4>
<div class="form-introduction">
<p><%- gettext('Note: Students can only be in one cohort group. Adding students to this group overrides any previous group assignment.') %></p>
<p><%- gettext('Note: Students can be in only one cohort group. Adding students to this group overrides any previous group assignment.') %></p>
</div>
<div class="cohort-confirmations"></div>
......@@ -48,14 +48,14 @@
<div class="form-fields">
<div class="field field-textarea is-required">
<label for="cohort-management-group-add-students" class="label">
<%- gettext('Enter email addresses and/or usernames separated by new lines or commas for students to add. *') %>
<%- gettext('Enter email addresses and/or usernames, separated by new lines or commas, for the students you want to add. *') %>
<span class="sr"><%- gettext('(Required Field)') %></span>
</label>
<textarea name="cohort-management-group-add-students" id="cohort-management-group-add-students"
class="input cohort-management-group-add-students"
placeholder="<%- gettext('e.g. johndoe@example.com, JaneDoe, joeydoe@example.com') %>"></textarea>
<span class="tip"><%- gettext('You will not get notification for emails that bounce, so please double-check spelling.') %></span>
<span class="tip"><%- gettext('You will not receive notification for emails that bounce, so double-check your spelling.') %></span>
</div>
</div>
......
lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
......@@ -14,12 +14,12 @@
<div class="form-field">
<div class="cohort-management-settings-form-name field field-text">
<label for="cohort-name" class="label">
<%- gettext('Cohort Name') %> *
<%- gettext('Cohort Group Name') %> *
<span class="sr"><%- gettext('(Required Field)')%></span>
</label>
<input type="text" name="cohort-name" value="<%- cohort ? cohort.get('name') : '' %>" class="input cohort-name"
id="cohort-name"
placeholder="<%- gettext("Enter Your Cohort Group's Name") %>" required="required" />
placeholder="<%- gettext("Enter the name of the cohort group") %>" required="required" />
</div>
</div>
<% } %>
......@@ -28,20 +28,21 @@
var foundSelected = false;
var selectedContentGroupId = cohort.get('group_id');
var hasSelectedContentGroup = selectedContentGroupId != null;
var hasContentGroups = contentGroups.length > 0;
%>
<div class="form-field">
<div class="cohort-management-details-association-course field field-radio">
<label class="label">
<%- gettext('Is this cohort group associated with course-based content groups?') %>
<%- gettext('Associate this cohort group with a content group') %>
</label>
<label><input type="radio" class="radio-no" name="cohort-association-course" value="no" <%- !hasSelectedContentGroup ? 'checked="checked"' : '' %>/> <%- gettext("No") %></label>
<label><input type="radio" class="radio-no" name="cohort-association-course" value="no" <%- !hasSelectedContentGroup ? 'checked="checked"' : '' %>/> <%- gettext("No Content Group") %></label>
<div class="input-group has-other-input-text">
<label><input type="radio" class="radio-yes" name="cohort-association-course" value="yes" <%- hasSelectedContentGroup ? 'checked="checked"' : '' %> /> <%- gettext("Yes") %></label>
<% if (contentGroups.length > 0) { %>
<label><input type="radio" class="radio-yes" name="cohort-association-course" value="yes" <%- !hasContentGroups ? 'disabled="disabled"' : '' %> <%- hasSelectedContentGroup ? 'checked="checked"' : '' %> /> <%- gettext("Select a Content Group") %></label>
<% if (hasContentGroups) { %>
<div class="input-group-other">
<label class="sr" for="cohort-group-association"><%- gettext("Choose a content group to associate") %></label>
<select name="cohort-group-association" class="input input-lg has-option-unavailable input-cohort-group-association">
<option value="None"><%- gettext("Choose a content group to associate") %></option>
<select name="cohort-group-association" class="input input-lg has-option-unavailable input-cohort-group-association" <%- !hasSelectedContentGroup ? 'disabled="disabled"' : '' %>>
<option value="None"></option>
<%
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;
}
%>
<option value="<%- contentGroupId %>" <%- isSelected ? 'selected="selected"' : '' %>><%- contentGroup.get('name') %></option>
<option value="<%- contentGroupId %>:<%- contentGroupUserPartitionId %>" <%- isSelected ? 'selected="selected"' : '' %>><%- contentGroup.get('name') %></option>
<%
}
%>
<% if (hasSelectedContentGroup && !foundSelected) { %>
<option value="<%- contentGroupId %>" class="option-unavailable" selected="selected"><%- gettext("Some content group that's been deleted") %></option>
<option value="<%- contentGroupId %>:<%- contentGroupUserPartitionId %>" class="option-unavailable" selected="selected"><%- gettext("Deleted Content Group") %></option>
<% } %>
</select>
<% if (hasSelectedContentGroup && !foundSelected) { %>
<div class="msg-inline">
<p class="copy-error"><i class="icon icon-warning-sign"></i><%- gettext("The selected content group has been deleted, you may wish to reassign this cohort group.") %></p>
<p class="copy-error"><i class="icon icon-warning-sign"></i><%- gettext("The previously selected content group was deleted. Select another content group.") %></p>
</div>
<% } %>
</div>
<% } else { // no content groups available %>
<div class="input-group-other is-visible">
<div class="input-group-other">
<div class="msg-inline">
<p class="copy-error"><i class="icon icon-warning-sign"></i> You haven't configured any content groups yet. You need to create a content group before you can create assignments. <a href="#">Create a content group</a></p>
<p class="copy-error"><i class="icon icon-warning-sign"></i><%- gettext("No content groups exist. Create a content group to associate with cohort groups.") %> <a href="<%- studioGroupConfigurationsUrl %>"><%- gettext("Create a content group") %></a></p>
</div>
</div>
<% } %>
......
lms/templates/instructor/instructor_dashboard_2/membership.html
<%! 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();
......
test_root/uploads/nonunique_store.txt deleted 100755 → 0
copy
\ No newline at end of file
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment