From 0ebd8c29b00e210a3b07d91af43c92278150db00 Mon Sep 17 00:00:00 2001
From: muhammad-ammar
Date: Wed, 11 Feb 2015 14:07:43 +0500
Subject: [PATCH 01/13] Allow Instructor To Rename Cohorts And Set Cohort
Assignment Method
TNL-181
---
.../pages/lms/instructor_dashboard.py | 107 ++++++---
.../acceptance/tests/discussion/helpers.py | 2 +-
.../discussion/test_cohort_management.py | 182 ++++++++++++++--
lms/static/js/groups/views/cohort_editor.js | 21 +-
lms/static/js/groups/views/cohort_form.js | 25 ++-
.../js/spec/groups/views/cohorts_spec.js | 205 ++++++++++++++++--
.../sass/course/instructor/_instructor_2.scss | 6 +
.../cohort-editor.underscore | 29 +--
.../cohort-form.underscore | 58 +++--
.../cohort-group-header.underscore | 26 +++
.../instructor_dashboard_2.html | 2 +-
.../core/djangoapps/course_groups/cohorts.py | 160 +++++++++-----
...coursecohort__add_coursecohortssettings.py | 108 +++++++++
.../core/djangoapps/course_groups/models.py | 69 ++++++
.../djangoapps/course_groups/tests/helpers.py | 13 +-
.../course_groups/tests/test_cohorts.py | 146 +++++++++++--
.../tests/test_partition_scheme.py | 1 +
.../course_groups/tests/test_views.py | 201 +++++++++++------
.../core/djangoapps/course_groups/views.py | 37 ++--
19 files changed, 1114 insertions(+), 284 deletions(-)
create mode 100644 lms/templates/instructor/instructor_dashboard_2/cohort-group-header.underscore
create mode 100644 openedx/core/djangoapps/course_groups/migrations/0003_auto__add_coursecohort__add_coursecohortssettings.py
diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py
index e86aceb8f1..368ba13e2f 100644
--- a/common/test/acceptance/pages/lms/instructor_dashboard.py
+++ b/common/test/acceptance/pages/lms/instructor_dashboard.py
@@ -6,7 +6,7 @@ Instructor (2) dashboard page.
from bok_choy.page_object import PageObject
from .course_page import CoursePage
import os
-from bok_choy.promise import EmptyPromise
+from bok_choy.promise import EmptyPromise, Promise
from ...tests.helpers import select_option_by_text, get_selected_option_text, get_options
@@ -101,6 +101,7 @@ class MembershipPageCohortManagementSection(PageObject):
content_group_selector_css = 'select.input-cohort-group-association'
no_content_group_button_css = '.cohort-management-details-association-course input.radio-no'
select_content_group_button_css = '.cohort-management-details-association-course input.radio-yes'
+ assignment_type_buttons_css = '.cohort-management-assignment-type-settings input'
def is_browser_on_page(self):
return self.q(css='.cohort-management.membership-section').present
@@ -115,7 +116,12 @@ class MembershipPageCohortManagementSection(PageObject):
"""
Returns the available options in the cohort dropdown, including the initial "Select a cohort".
"""
- return self.q(css=self._bounded_selector("#cohort-select option"))
+ def check_func():
+ """Promise Check Function"""
+ query = self.q(css=self._bounded_selector("#cohort-select option"))
+ return len(query) > 0, query
+
+ return Promise(check_func, "Waiting for cohort selector to populate").fulfill()
def _cohort_name(self, label):
"""
@@ -129,6 +135,41 @@ class MembershipPageCohortManagementSection(PageObject):
"""
return int(label.split(' (')[1].split(')')[0])
+ def save_cohort_settings(self):
+ """
+ Click on Save button shown after click on Settings tab or when we add a new cohort.
+ """
+ self.q(css=self._bounded_selector("div.form-actions .action-save")).first.click()
+
+ @property
+ def is_assignment_settings_disabled(self):
+ """
+ Check if assignment settings are disabled.
+ """
+ attributes = self.q(css=self._bounded_selector('.cohort-management-assignment-type-settings')).attrs('class')
+ if 'is-disabled' in attributes[0].split():
+ return True
+
+ return False
+
+ @property
+ def assignment_settings_message(self):
+ """
+ Return assignment settings disabled message in case of default cohort.
+ """
+ query = self.q(css=self._bounded_selector('.copy-error'))
+ if query.present:
+ return query.text[0]
+ else:
+ return ''
+
+ @property
+ def cohort_name_in_header(self):
+ """
+ Return cohort name as shown in cohort header.
+ """
+ return self._cohort_name(self.q(css=self._bounded_selector(".group-header-title .title-value")).text[0])
+
def get_cohorts(self):
"""
Returns, as a list, the names of the available cohorts in the drop-down, filtering out "Select a cohort".
@@ -142,10 +183,6 @@ class MembershipPageCohortManagementSection(PageObject):
"""
Returns the name of the selected cohort.
"""
- EmptyPromise(
- lambda: len(self._get_cohort_options().results) > 0,
- "Waiting for cohort selector to populate"
- ).fulfill()
return self._cohort_name(
self._get_cohort_options().filter(lambda el: el.is_selected()).first.text[0]
)
@@ -162,10 +199,6 @@ class MembershipPageCohortManagementSection(PageObject):
"""
Selects the given cohort in the drop-down.
"""
- EmptyPromise(
- lambda: cohort_name in self.get_cohorts(),
- "Waiting for cohort selector to populate"
- ).fulfill()
# Note: can't use Select to select by text because the count is also included in the displayed text.
self._get_cohort_options().filter(
lambda el: self._cohort_name(el.text) == cohort_name
@@ -176,7 +209,25 @@ class MembershipPageCohortManagementSection(PageObject):
"Waiting to confirm cohort has been selected"
).fulfill()
- def add_cohort(self, cohort_name, content_group=None):
+ def set_cohort_name(self, cohort_name):
+ """
+ Set Cohort Name.
+ """
+ textinput = self.q(css=self._bounded_selector("#cohort-name")).results[0]
+ textinput.clear()
+ textinput.send_keys(cohort_name)
+
+ def set_assignment_type(self, assignment_type):
+ """
+ Set assignment type for selected cohort.
+
+ Arguments:
+ assignment_type (str): Should be 'random' or 'manual'
+ """
+ css = self._bounded_selector(self.assignment_type_buttons_css)
+ self.q(css=css).filter(lambda el: el.get_attribute('value') == assignment_type).first.click()
+
+ def add_cohort(self, cohort_name, content_group=None, assignment_type=None):
"""
Adds a new manual cohort with the specified name.
If a content group should also be associated, the name of the content group should be specified.
@@ -187,9 +238,15 @@ class MembershipPageCohortManagementSection(PageObject):
create_buttons.results[len(create_buttons.results) - 1].click()
textinput = self.q(css=self._bounded_selector("#cohort-name")).results[0]
textinput.send_keys(cohort_name)
+
+ # Manual assignment type will be selected by default for a new cohort
+ # if we are not setting the assignment type explicitly
+ if assignment_type:
+ self.set_assignment_type(assignment_type)
+
if content_group:
self._select_associated_content_group(content_group)
- self.q(css=self._bounded_selector("div.form-actions .action-save")).first.click()
+ self.save_cohort_settings()
def get_cohort_group_setup(self):
"""
@@ -200,6 +257,12 @@ class MembershipPageCohortManagementSection(PageObject):
def select_edit_settings(self):
self.q(css=self._bounded_selector(".action-edit")).first.click()
+ def select_manage_settings(self):
+ """
+ Click on Manage Students Tab under cohort management section.
+ """
+ self.q(css=self._bounded_selector(".tab-manage_students")).first.click()
+
def add_students_to_selected_cohort(self, users):
"""
Adds a list of users (either usernames or email addresses) to the currently selected cohort.
@@ -245,23 +308,6 @@ class MembershipPageCohortManagementSection(PageObject):
return None
return get_selected_option_text(self.q(css=self._bounded_selector(self.content_group_selector_css)))
- def verify_cohort_content_group_selected(self, content_group=None):
- """
- Waits for the expected content_group (or none) to show as selected for
- cohort associated content group.
- """
- if content_group:
- self.wait_for(
- lambda: unicode(
- self.q(css='select.input-cohort-group-association option:checked').text[0]
- ) == content_group,
- "Cohort group has been selected."
- )
- else:
- self.wait_for_element_visibility(
- '.cohort-management-details-association-course input.radio-no:checked',
- 'Radio button "No content group" has been selected.')
-
def set_cohort_associated_content_group(self, content_group=None, select_settings=True):
"""
Sets the content group associated with the cohort currently being edited.
@@ -274,8 +320,7 @@ class MembershipPageCohortManagementSection(PageObject):
self.q(css=self._bounded_selector(self.no_content_group_button_css)).first.click()
else:
self._select_associated_content_group(content_group)
- self.q(css=self._bounded_selector("div.form-actions .action-save")).first.click()
- self.verify_cohort_content_group_selected(content_group)
+ self.save_cohort_settings()
def _select_associated_content_group(self, content_group):
"""
diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py
index f0722cd236..3f4c04e6bc 100644
--- a/common/test/acceptance/tests/discussion/helpers.py
+++ b/common/test/acceptance/tests/discussion/helpers.py
@@ -73,7 +73,7 @@ class CohortTestMixin(object):
Adds a cohort by name, returning its ID.
"""
url = LMS_BASE_URL + "/courses/" + course_fixture._course_key + '/cohorts/'
- data = json.dumps({"name": cohort_name})
+ data = json.dumps({"name": cohort_name, 'assignment_type': 'manual'})
response = course_fixture.session.post(url, data=data, headers=course_fixture.headers)
self.assertTrue(response.ok, "Failed to create cohort")
return response.json()['id']
diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py
index 686e1d8c17..31e4db463c 100644
--- a/common/test/acceptance/tests/discussion/test_cohort_management.py
+++ b/common/test/acceptance/tests/discussion/test_cohort_management.py
@@ -250,36 +250,47 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin
self.cohort_management_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.
+ def _verify_cohort_settings(
+ self,
+ cohort_name,
+ assignment_type=None,
+ new_cohort_name=None,
+ new_assignment_type=None,
+ verify_updated=False
+ ):
- 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
- And appropriate events have been emitted
+ """
+ Create a new cohort and verify the new and existing settings.
"""
start_time = datetime.now(UTC)
- new_cohort = str(uuid.uuid4().get_hex()[0:20])
- self.assertFalse(new_cohort in self.cohort_management_page.get_cohorts())
- self.cohort_management_page.add_cohort(new_cohort)
+ self.assertFalse(cohort_name in self.cohort_management_page.get_cohorts())
+ self.cohort_management_page.add_cohort(cohort_name, assignment_type=assignment_type)
# After adding the cohort, it should automatically be selected
EmptyPromise(
- lambda: new_cohort == self.cohort_management_page.get_selected_cohort(), "Waiting for new cohort to appear"
+ lambda: cohort_name == self.cohort_management_page.get_selected_cohort(), "Waiting for new cohort to appear"
).fulfill()
self.assertEqual(0, self.cohort_management_page.get_selected_cohort_count())
+ # After adding the cohort, it should automatically be selected and its
+ # assignment_type should be "manual" as this is the default assignment type
+ _assignment_type = assignment_type or 'manual'
+ msg = "Waiting for currently selected cohort assignment type"
+ EmptyPromise(
+ lambda: _assignment_type == self.cohort_management_page.get_cohort_associated_assignment_type(), msg
+ ).fulfill()
+ # Go back to Manage Students Tab
+ self.cohort_management_page.select_manage_settings()
self.cohort_management_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.cohort_management_page.get_selected_cohort_count(), 'Waiting for student to be added'
).fulfill()
+ self.assertFalse(self.cohort_management_page.is_assignment_settings_disabled)
+ self.assertEqual('', self.cohort_management_page.assignment_settings_message)
self.assertEqual(
self.event_collection.find({
"name": "edx.cohort.created",
"time": {"$gt": start_time},
- "event.cohort_name": new_cohort,
+ "event.cohort_name": cohort_name,
}).count(),
1
)
@@ -287,11 +298,152 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin
self.event_collection.find({
"name": "edx.cohort.creation_requested",
"time": {"$gt": start_time},
- "event.cohort_name": new_cohort,
+ "event.cohort_name": cohort_name,
}).count(),
1
)
+ if verify_updated:
+ self.cohort_management_page.select_cohort(cohort_name)
+ self.cohort_management_page.select_cohort_settings()
+ self.cohort_management_page.set_cohort_name(new_cohort_name)
+ self.cohort_management_page.set_assignment_type(new_assignment_type)
+ self.cohort_management_page.save_cohort_settings()
+
+ # If cohort name is empty, then we should get/see an error message.
+ if not new_cohort_name:
+ confirmation_messages = self.cohort_management_page.get_cohort_settings_messages(type='error')
+ self.assertEqual(
+ ["The cohort cannot be saved", "You must specify a name for the cohort"],
+ confirmation_messages
+ )
+ else:
+ confirmation_messages = self.cohort_management_page.get_cohort_settings_messages()
+ self.assertEqual(["Saved cohort"], confirmation_messages)
+ self.assertEqual(new_cohort_name, self.cohort_management_page.cohort_name_in_header)
+ self.assertTrue(new_cohort_name in self.cohort_management_page.get_cohorts())
+ self.assertEqual(1, self.cohort_management_page.get_selected_cohort_count())
+ self.assertEqual(
+ new_assignment_type,
+ self.cohort_management_page.get_cohort_associated_assignment_type()
+ )
+
+ 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 assignment type of displayed cohort to "manual" because this is the default
+ And when I add the user to the new cohort
+ Then the cohort has 1 user
+ And appropriate events have been emitted
+ """
+ cohort_name = str(uuid.uuid4().get_hex()[0:20])
+ self._verify_cohort_settings(cohort_name=cohort_name, assignment_type=None)
+
+ def test_add_new_cohort_with_manual_assignment_type(self):
+ """
+ Scenario: A new cohort with manual assignment type 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 with manual assignment type to the course via the LMS instructor dashboard
+ Then the new cohort is displayed and has no users in it
+ And assignment type of displayed cohort is "manual"
+ And when I add the user to the new cohort
+ Then the cohort has 1 user
+ And appropriate events have been emitted
+ """
+ cohort_name = str(uuid.uuid4().get_hex()[0:20])
+ self._verify_cohort_settings(cohort_name=cohort_name, assignment_type='manual')
+
+ def test_add_new_cohort_with_random_assignment_type(self):
+ """
+ Scenario: A new cohort with random assignment type 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 with random assignment type to the course via the LMS instructor dashboard
+ Then the new cohort is displayed and has no users in it
+ And assignment type of displayed cohort is "random"
+ And when I add the user to the new cohort
+ Then the cohort has 1 user
+ And appropriate events have been emitted
+ """
+ cohort_name = str(uuid.uuid4().get_hex()[0:20])
+ self._verify_cohort_settings(cohort_name=cohort_name, assignment_type='random')
+
+ def test_update_existing_cohort_settings(self):
+ """
+ Scenario: Update existing cohort settings(cohort name, assignment type)
+
+ Given I have a course with a user in the course
+ When I add a new cohort with random assignment type to the course via the LMS instructor dashboard
+ Then the new cohort is displayed and has no users in it
+ And assignment type of displayed cohort is "random"
+ And when I add the user to the new cohort
+ Then the cohort has 1 user
+ And appropriate events have been emitted
+ Then I select the cohort (that you just created) from existing cohorts
+ Then I change its name and assignment type set to "manual"
+ Then I Save the settings
+ And cohort with new name is present in cohorts dropdown list
+ And cohort assignment type should be "manual"
+ """
+ cohort_name = str(uuid.uuid4().get_hex()[0:20])
+ new_cohort_name = '{old}__NEW'.format(old=cohort_name)
+ self._verify_cohort_settings(
+ cohort_name=cohort_name,
+ assignment_type='random',
+ new_cohort_name=new_cohort_name,
+ new_assignment_type='manual',
+ verify_updated=True
+ )
+
+ def test_update_existing_cohort_settings_with_empty_cohort_name(self):
+ """
+ Scenario: Update existing cohort settings(cohort name, assignment type).
+
+ Given I have a course with a user in the course
+ When I add a new cohort with random assignment type to the course via the LMS instructor dashboard
+ Then the new cohort is displayed and has no users in it
+ And assignment type of displayed cohort is "random"
+ And when I add the user to the new cohort
+ Then the cohort has 1 user
+ And appropriate events have been emitted
+ Then I select a cohort from existing cohorts
+ Then I set its name as empty string and assignment type set to "manual"
+ And I click on Save button
+ Then I should see an error message
+ """
+ cohort_name = str(uuid.uuid4().get_hex()[0:20])
+ new_cohort_name = ''
+ self._verify_cohort_settings(
+ cohort_name=cohort_name,
+ assignment_type='random',
+ new_cohort_name=new_cohort_name,
+ new_assignment_type='manual',
+ verify_updated=True
+ )
+
+ def test_default_cohort_assignment_settings(self):
+ """
+ Scenario: Cohort assignment settings are disabled for default cohort.
+
+ Given I have a course with a user in the course
+ And I have added a manual cohort
+ And I have added a random cohort
+ When I select the random cohort
+ Then cohort assignment settings are disabled
+ """
+ self.cohort_management_page.select_cohort("AutoCohort1")
+ self.cohort_management_page.select_cohort_settings()
+
+ self.assertTrue(self.cohort_management_page.is_assignment_settings_disabled)
+
+ message = "There must be one cohort to which students can be randomly assigned."
+ self.assertEqual(message, self.cohort_management_page.assignment_settings_message)
+
def test_link_to_data_download(self):
"""
Scenario: a link is present from the cohort configuration in
diff --git a/lms/static/js/groups/views/cohort_editor.js b/lms/static/js/groups/views/cohort_editor.js
index 630f8dec5f..448b398546 100644
--- a/lms/static/js/groups/views/cohort_editor.js
+++ b/lms/static/js/groups/views/cohort_editor.js
@@ -6,14 +6,17 @@ var edx = edx || {};
edx.groups = edx.groups || {};
edx.groups.CohortEditorView = Backbone.View.extend({
+
events : {
'click .wrapper-tabs .tab': 'selectTab',
'click .tab-content-settings .action-save': 'saveSettings',
+ 'click .tab-content-settings .action-cancel': 'cancelSettings',
'submit .cohort-management-group-add-form': 'addStudents'
},
initialize: function(options) {
this.template = _.template($('#cohort-editor-tpl').text());
+ this.groupHeaderTemplate = _.template($('#cohort-group-header-tpl').text());
this.cohorts = options.cohorts;
this.contentGroups = options.contentGroups;
this.context = options.context;
@@ -26,9 +29,9 @@ var edx = edx || {};
render: function() {
this.$el.html(this.template({
- cohort: this.model,
- studioAdvancedSettingsUrl: this.context.studioAdvancedSettingsUrl
+ cohort: this.model
}));
+ this.renderGroupHeader();
this.cohortFormView = new CohortFormView({
model: this.model,
contentGroups: this.contentGroups,
@@ -39,6 +42,13 @@ var edx = edx || {};
return this;
},
+ renderGroupHeader: function() {
+ this.$('.cohort-management-group-header').html(this.groupHeaderTemplate({
+ cohort: this.model,
+ studioAdvancedSettingsUrl: this.context.studioAdvancedSettingsUrl
+ }));
+ },
+
selectTab: function(event) {
var tabElement = $(event.currentTarget),
tabName = tabElement.data('tab');
@@ -53,13 +63,20 @@ var edx = edx || {};
saveSettings: function(event) {
var cohortFormView = this.cohortFormView;
+ var self = this;
event.preventDefault();
cohortFormView.saveForm()
.done(function() {
+ self.renderGroupHeader();
cohortFormView.showMessage(gettext('Saved cohort'));
});
},
+ cancelSettings: function(event) {
+ event.preventDefault();
+ this.render();
+ },
+
setCohort: function(cohort) {
this.model = cohort;
this.render();
diff --git a/lms/static/js/groups/views/cohort_form.js b/lms/static/js/groups/views/cohort_form.js
index 01cbf7c85b..2eba90da49 100644
--- a/lms/static/js/groups/views/cohort_form.js
+++ b/lms/static/js/groups/views/cohort_form.js
@@ -35,12 +35,22 @@ var edx = edx || {};
render: function() {
this.$el.html(this.template({
cohort: this.model,
+ isDefaultCohort: this.isDefault(this.model.get('name')),
contentGroups: this.contentGroups,
studioGroupConfigurationsUrl: this.context.studioGroupConfigurationsUrl
}));
return this;
},
+ isDefault: function(name) {
+ var cohorts = this.model.collection;
+ if (_.isUndefined(cohorts)) {
+ return false;
+ }
+ var randomModels = cohorts.where({assignment_type:'random'});
+ return (randomModels.length === 1) && (randomModels[0].get('name') === name);
+ },
+
onRadioButtonChange: function(event) {
var target = $(event.currentTarget),
groupsEnabled = target.val() === 'yes';
@@ -77,7 +87,11 @@ var edx = edx || {};
getUpdatedCohortName: function() {
var cohortName = this.$('.cohort-name').val();
- return cohortName ? cohortName.trim() : this.model.get('name');
+ return cohortName ? cohortName.trim() : '';
+ },
+
+ getAssignmentType: function() {
+ return this.$('input[name="cohort-assignment-type"]:checked').val();
},
showMessage: function(message, type, details) {
@@ -109,18 +123,21 @@ var edx = edx || {};
cohort = this.model,
saveOperation = $.Deferred(),
isUpdate = !_.isUndefined(this.model.id),
- fieldData, selectedContentGroup, errorMessages, showErrorMessage;
+ fieldData, selectedContentGroup, selectedAssignmentType, errorMessages, showErrorMessage;
showErrorMessage = function(message, details) {
self.showMessage(message, 'error', details);
};
this.removeNotification();
selectedContentGroup = this.getSelectedContentGroup();
+ selectedAssignmentType = this.getAssignmentType();
fieldData = {
name: this.getUpdatedCohortName(),
group_id: selectedContentGroup ? selectedContentGroup.id : null,
- user_partition_id: selectedContentGroup ? selectedContentGroup.get('user_partition_id') : null
+ user_partition_id: selectedContentGroup ? selectedContentGroup.get('user_partition_id') : null,
+ assignment_type: selectedAssignmentType
};
errorMessages = this.validate(fieldData);
+
if (errorMessages.length > 0) {
showErrorMessage(
isUpdate ? gettext("The cohort cannot be saved") : gettext("The cohort cannot be added"),
@@ -129,7 +146,7 @@ var edx = edx || {};
saveOperation.reject();
} else {
cohort.save(
- fieldData, {patch: isUpdate}
+ fieldData, {patch: isUpdate, wait: true}
).done(function(result) {
cohort.id = result.id;
self.render(); // re-render to remove any now invalid error messages
diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js
index a2356b5f20..4aacf9caf0 100644
--- a/lms/static/js/spec/groups/views/cohorts_spec.js
+++ b/lms/static/js/spec/groups/views/cohorts_spec.js
@@ -9,17 +9,20 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
requests, respondToRefresh, verifyMessage, verifyNoMessage, verifyDetailedMessage, verifyHeader,
expectCohortAddRequest, getAddModal, selectContentGroup, clearContentGroup, saveFormAndExpectErrors,
MOCK_COHORTED_USER_PARTITION_ID, MOCK_UPLOAD_COHORTS_CSV_URL, MOCK_STUDIO_ADVANCED_SETTINGS_URL,
- MOCK_STUDIO_GROUP_CONFIGURATIONS_URL;
+ MOCK_STUDIO_GROUP_CONFIGURATIONS_URL, MOCK_MANUAL_ASSIGNMENT, MOCK_RANDOM_ASSIGNMENT;
+ MOCK_MANUAL_ASSIGNMENT = 'manual';
+ MOCK_RANDOM_ASSIGNMENT = 'random';
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) {
+ createMockCohort = function (name, id, userCount, groupId, userPartitionId, assignmentType) {
return {
id: id !== undefined ? id : 1,
name: name,
+ assignment_type: assignmentType || MOCK_MANUAL_ASSIGNMENT,
user_count: userCount !== undefined ? userCount : 0,
group_id: groupId,
user_partition_id: userPartitionId
@@ -73,13 +76,13 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
AjaxHelpers.respondWithJson(requests, createMockCohorts(catCount, dogCount));
};
- expectCohortAddRequest = function(name, groupId, userPartitionId) {
+ expectCohortAddRequest = function(name, groupId, userPartitionId, assignmentType) {
AjaxHelpers.expectJsonRequest(
requests, 'POST', '/mock_service/cohorts',
{
name: name,
user_count: 0,
- assignment_type: '',
+ assignment_type: assignmentType,
group_id: groupId,
user_partition_id: userPartitionId
}
@@ -130,7 +133,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
});
};
- verifyHeader = function(expectedCohortId, expectedTitle, expectedCount) {
+ verifyHeader = function(expectedCohortId, expectedTitle, expectedCount, assignmentType) {
var header = cohortsView.$('.cohort-management-group-header');
expect(cohortsView.$('.cohort-select').val()).toBe(expectedCohortId.toString());
expect(cohortsView.$('.cohort-select option:selected').text()).toBe(
@@ -147,6 +150,11 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
{count: expectedCount}
)
);
+ assignmentType = assignmentType || MOCK_MANUAL_ASSIGNMENT;
+ var manualMessage = "Students are added to this cohort only when you provide their email addresses or usernames on this page.";
+ var randomMessage = "Students are added to this cohort automatically.";
+ var message = (assignmentType == MOCK_MANUAL_ASSIGNMENT) ? manualMessage : randomMessage;
+ expect(header.find('.cohort-management-group-setup .setup-value').text().trim().split('\n')[0]).toBe(message);
};
saveFormAndExpectErrors = function(action, errors) {
@@ -174,6 +182,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/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/cohort-group-header');
TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/notification');
TemplateHelpers.installTemplate('templates/file-upload');
});
@@ -246,6 +255,56 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
});
});
+ describe("Cohort Group Header", function () {
+ it("renders header correctly", function () {
+ var cohortName = 'Transformers',
+ newCohortName = 'X Men';
+ var expectedRequest = function(assignment_type) {
+ return {
+ name: newCohortName,
+ assignment_type: assignment_type,
+ group_id: null,
+ user_partition_id: null
+ }
+ };
+ createCohortsView(this, {
+ cohorts: [
+ {
+ id: 1,
+ name: cohortName,
+ assignment_type: MOCK_RANDOM_ASSIGNMENT,
+ group_id: 111,
+ user_partition_id: MOCK_COHORTED_USER_PARTITION_ID
+ }
+ ],
+ selectCohort: 1
+ });
+
+ // Select settings tab
+ cohortsView.$('.tab-settings a').click();
+
+ verifyHeader(1, cohortName, 0, MOCK_RANDOM_ASSIGNMENT);
+
+ // Update existing cohort values
+ cohortsView.$('.cohort-name').val(newCohortName);
+ cohortsView.$('.type-manual').prop('checked', true).change();
+ clearContentGroup();
+
+ // Save the updated settings
+ cohortsView.$('.action-save').click();
+ AjaxHelpers.expectJsonRequest(
+ requests, 'PATCH', '/mock_service/cohorts/1',
+ expectedRequest(MOCK_MANUAL_ASSIGNMENT)
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ createMockCohort(newCohortName, 1, 0, null, null)
+ );
+
+ verifyHeader(1, newCohortName, 0, MOCK_MANUAL_ASSIGNMENT);
+ });
+ });
+
describe("Cohort Editor Tab Panel", function () {
it("initially selects the Manage Students tab", function () {
createCohortsView(this, {selectCohort: 1});
@@ -267,6 +326,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
describe("Add Cohorts Form", function () {
var defaultCohortName = 'New Cohort';
+ var assignmentType = 'random';
it("can add a cohort", function() {
var contentGroupId = 0,
@@ -277,39 +337,65 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled');
expect(cohortsView.$('.cohort-management-group')).toHaveClass('is-hidden');
cohortsView.$('.cohort-name').val(defaultCohortName);
+ cohortsView.$('.type-random').prop('checked', true).change();
selectContentGroup(contentGroupId, MOCK_COHORTED_USER_PARTITION_ID);
cohortsView.$('.action-save').click();
- expectCohortAddRequest(defaultCohortName, contentGroupId, MOCK_COHORTED_USER_PARTITION_ID);
+ expectCohortAddRequest(defaultCohortName, contentGroupId, MOCK_COHORTED_USER_PARTITION_ID, assignmentType);
AjaxHelpers.respondWithJson(
requests,
{
id: 1,
name: defaultCohortName,
+ assignment_type: assignmentType,
group_id: contentGroupId,
user_partition_id: MOCK_COHORTED_USER_PARTITION_ID
}
);
AjaxHelpers.respondWithJson(
requests,
- { cohorts: createMockCohort(defaultCohortName) }
+ { cohorts: createMockCohort(defaultCohortName, 1, 0, null, null, assignmentType) }
);
verifyMessage(
'The ' + defaultCohortName + ' cohort has been created.' +
' You can manually add students to this cohort below.',
'confirmation'
);
- verifyHeader(1, defaultCohortName, 0);
+ verifyHeader(1, defaultCohortName, 0, MOCK_RANDOM_ASSIGNMENT);
expect(cohortsView.$('.cohort-management-nav')).not.toHaveClass('is-disabled');
expect(cohortsView.$('.cohort-management-group')).not.toHaveClass('is-hidden');
expect(getAddModal().find('.cohort-management-settings-form').length).toBe(0);
});
+ it("has default assignment type set to manual", function() {
+ var cohortName = "iCohort";
+ createCohortsView(this, {cohorts: []});
+ cohortsView.$('.action-create').click();
+ cohortsView.$('.cohort-name').val(cohortName);
+ cohortsView.$('.action-save').click();
+ expectCohortAddRequest(cohortName, null, null, MOCK_MANUAL_ASSIGNMENT);
+ AjaxHelpers.respondWithJson(
+ requests,
+ {
+ id: 1,
+ name: cohortName,
+ assignment_type: MOCK_MANUAL_ASSIGNMENT,
+ group_id: null,
+ user_partition_id: null
+ }
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ { cohorts: createMockCohort(cohortName, 1, 0, null, null, MOCK_MANUAL_ASSIGNMENT) }
+ );
+ verifyHeader(1, cohortName, 0, MOCK_MANUAL_ASSIGNMENT);
+ });
+
it("trims off whitespace before adding a cohort", function() {
createCohortsView(this);
cohortsView.$('.action-create').click();
cohortsView.$('.cohort-name').val(' New Cohort ');
cohortsView.$('.action-save').click();
- expectCohortAddRequest('New Cohort', null, null);
+ expectCohortAddRequest('New Cohort', null, null, MOCK_MANUAL_ASSIGNMENT);
});
it("does not allow a blank cohort name to be submitted", function() {
@@ -560,6 +646,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
{
id: 1,
name: 'Cat Lovers',
+ assignment_type: MOCK_RANDOM_ASSIGNMENT,
group_id: 999,
user_partition_id: MOCK_COHORTED_USER_PARTITION_ID
}
@@ -594,6 +681,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
requests, 'PATCH', '/mock_service/cohorts/1',
{
name: 'Cat Lovers',
+ assignment_type: MOCK_MANUAL_ASSIGNMENT,
group_id: 0,
user_partition_id: MOCK_COHORTED_USER_PARTITION_ID
}
@@ -608,7 +696,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
it("can clear selected content group", function () {
createCohortsView(this, {
cohorts: [
- {id: 1, name: 'Cat Lovers', group_id: 0}
+ {id: 1, name: 'Cat Lovers', group_id: 0, 'assignment_type': MOCK_MANUAL_ASSIGNMENT}
],
selectCohort: 1
});
@@ -622,6 +710,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
requests, 'PATCH', '/mock_service/cohorts/1',
{
name: 'Cat Lovers',
+ 'assignment_type': MOCK_MANUAL_ASSIGNMENT,
group_id: null,
user_partition_id: null
}
@@ -645,7 +734,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
createCohortsViewWithDeletedContentGroup(this);
cohortsView.$('.tab-settings a').click();
expect(cohortsView.$('option.option-unavailable').text().trim()).toBe('Deleted Content Group');
- expect(cohortsView.$('.copy-error').text().trim()).toBe(
+ expect(cohortsView.$('.cohort-management-details-association-course .copy-error').text().trim()).toBe(
'Warning: The previously selected content group was deleted. Select another content group.'
);
});
@@ -660,13 +749,13 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
cohortsView.$('.action-save').click();
AjaxHelpers.respondWithJson(
requests,
- createMockCohort('Cat Lovers', 1, catLoversInitialCount, 0, 0)
+ createMockCohort('Cat Lovers', 1, catLoversInitialCount, 0, 0, MOCK_RANDOM_ASSIGNMENT)
);
verifyMessage('Saved cohort', '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('');
+ expect(cohortsView.$('.cohort-management-details-association-course .copy-error').text().trim()).toBe('');
});
it("shows an error when saving with a deleted content group", function () {
@@ -699,6 +788,96 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
MOCK_STUDIO_GROUP_CONFIGURATIONS_URL
);
});
+
+ it("can update existing cohort settings", function () {
+ var cohortName = 'Transformers',
+ newCohortName = 'X Men';
+ var expectedRequest = function(assignment_type) {
+ return {
+ name: newCohortName,
+ assignment_type: assignment_type,
+ group_id: null,
+ user_partition_id: null
+ }
+ };
+ createCohortsView(this, {
+ cohorts: [
+ {
+ id: 1,
+ name: cohortName,
+ assignment_type: MOCK_RANDOM_ASSIGNMENT,
+ group_id: 111,
+ user_partition_id: MOCK_COHORTED_USER_PARTITION_ID
+ }
+ ],
+ selectCohort: 1
+ });
+
+ // Select settings tab
+ cohortsView.$('.tab-settings a').click();
+
+ // Verify the existing cohort values
+ expect(cohortsView.$('.cohort-name').val()).toBe(cohortName);
+ expect(cohortsView.$('input[name="cohort-assignment-type"]:checked').val()).toBe(MOCK_RANDOM_ASSIGNMENT);
+ expect(cohortsView.$('.radio-yes').prop('checked')).toBeTruthy();
+
+ // Update existing cohort values
+ cohortsView.$('.cohort-name').val(newCohortName);
+ cohortsView.$('.type-manual').prop('checked', true).change();
+ clearContentGroup();
+
+ // Save the updated settings
+ cohortsView.$('.action-save').click();
+ AjaxHelpers.expectJsonRequest(
+ requests, 'PATCH', '/mock_service/cohorts/1',
+ expectedRequest(MOCK_MANUAL_ASSIGNMENT)
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ createMockCohort(newCohortName, 1, 0, null, null)
+ );
+
+ // Verify the new/updated cohort values
+ expect(cohortsView.$('.cohort-name').val()).toBe(newCohortName);
+ expect(cohortsView.$('input[name="cohort-assignment-type"]:checked').val()).toBe(MOCK_MANUAL_ASSIGNMENT);
+ expect(cohortsView.$('.radio-no').prop('checked')).toBeTruthy();
+
+ verifyMessage('Saved cohort', 'confirmation');
+
+ // Now try to update existing cohort name with an empty name
+ // We can't save a cohort with empty name, so we should see an error message
+ cohortsView.$('.cohort-name').val('');
+
+ saveFormAndExpectErrors('update', ['You must specify a name for the cohort']);
+ });
+
+ it("assignment settings are disabled for default cohort", function() {
+ createCohortsView(this, {
+ cohorts: [
+ {
+ id: 1,
+ name: 'Cohort.me',
+ assignment_type: MOCK_RANDOM_ASSIGNMENT,
+ group_id: 111,
+ user_partition_id: MOCK_COHORTED_USER_PARTITION_ID
+ }
+ ],
+ selectCohort: 1
+ });
+
+ // We have a single random cohort so we should not be allowed to change it assignment type
+ expect(cohortsView.$('.cohort-management-assignment-type-settings')).toHaveClass('is-disabled');
+ expect(cohortsView.$('.copy-error').text()).toContain("There must be one cohort to which students can be randomly assigned.");
+ });
+
+ it("cancel settings works", function() {
+ createCohortsView(this, {selectCohort: 1, contentGroups: []});
+ cohortsView.$('.tab-settings a').click();
+ cohortsView.$('.cohort-name').val('One Two Three');
+ cohortsView.$('.action-cancel').click();
+ expect(cohortsView.$('.tab-manage_students')).toHaveClass('is-selected');
+ expect(cohortsView.$('.tab-settings')).not.toHaveClass('is-selected');
+ });
});
});
});
diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss
index 89368b2e28..98cf31d895 100644
--- a/lms/static/sass/course/instructor/_instructor_2.scss
+++ b/lms/static/sass/course/instructor/_instructor_2.scss
@@ -567,6 +567,12 @@
}
}
+ .cohort-management-assignment-type-settings {
+ &.is-disabled {
+ opacity: 0.50;
+ }
+ }
+
// cohort
.cohort-management-group-header {
padding: $baseline;
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore
index a96327e247..d10f2f46f0 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore
@@ -1,32 +1,5 @@
-
+
<%- gettext('Selected tab') %> <%- gettext("Manage Students") %>
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
index e787c19cc7..20049ecde5 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
@@ -6,23 +6,6 @@
<% } %>
-
\ No newline at end of file
+
+
diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py
index 83092a4914..982b359fd0 100644
--- a/openedx/core/djangoapps/course_groups/cohorts.py
+++ b/openedx/core/djangoapps/course_groups/cohorts.py
@@ -456,9 +456,12 @@ def set_course_cohort_settings(course_key, **kwargs):
Raises:
ValueError if course_key is invalid.
"""
+ fields = {'is_cohorted': bool, 'always_cohort_inline_discussions': bool, 'cohorted_discussions': list}
course_cohort_settings = get_course_cohort_settings(course_key)
- for field in ('is_cohorted', 'always_cohort_inline_discussions', 'cohorted_discussions'):
+ for field, field_type in fields.items():
if field in kwargs:
+ if not isinstance(kwargs[field], field_type):
+ raise ValueError("Incorrect field type for `{}`. Type must be `{}`".format(field, field_type.__name__))
setattr(course_cohort_settings, field, kwargs[field])
course_cohort_settings.save()
return course_cohort_settings
diff --git a/openedx/core/djangoapps/course_groups/models.py b/openedx/core/djangoapps/course_groups/models.py
index 3c04648dbf..3fc6828bdc 100644
--- a/openedx/core/djangoapps/course_groups/models.py
+++ b/openedx/core/djangoapps/course_groups/models.py
@@ -93,12 +93,12 @@ class CourseCohortsSettings(models.Model):
@property
def cohorted_discussions(self):
- """Jsonfiy the cohorted_discussions"""
+ """Jsonify the cohorted_discussions"""
return json.loads(self._cohorted_discussions)
@cohorted_discussions.setter
def cohorted_discussions(self, value):
- """UnJsonfiy the cohorted_discussions"""
+ """Un-Jsonify the cohorted_discussions"""
self._cohorted_discussions = json.dumps(value)
diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
index 9e9039808c..0b704aa7ef 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
@@ -716,6 +716,29 @@ class TestCohorts(ModuleStoreTestCase):
self.assertEqual(course_cohort_settings.cohorted_discussions, ['topic a id', 'topic b id'])
self.assertFalse(course_cohort_settings.always_cohort_inline_discussions)
+ def test_update_course_cohort_settings_with_invalid_data_type(self):
+ """
+ Test that cohorts.set_course_cohort_settings raises exception if fields have incorrect data type.
+ """
+ course = modulestore().get_course(self.toy_course_key)
+ CourseCohortSettingsFactory(course_id=course.id)
+
+ exception_msg_tpl = "Incorrect field type for `{}`. Type must be `{}`"
+ fields = [
+ {'name': 'is_cohorted', 'type': bool},
+ {'name': 'always_cohort_inline_discussions', 'type': bool},
+ {'name': 'cohorted_discussions', 'type': list}
+ ]
+
+ for field in fields:
+ with self.assertRaises(ValueError) as value_error:
+ cohorts.set_course_cohort_settings(course.id, **{field['name']: ''})
+
+ self.assertEqual(
+ value_error.exception.message,
+ exception_msg_tpl.format(field['name'], field['type'].__name__)
+ )
+
class TestCohortsAndPartitionGroups(ModuleStoreTestCase):
"""
diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py
index 1355f6691a..f3ee3d21ae 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_views.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_views.py
@@ -3,7 +3,6 @@ Tests for course group views
"""
# pylint: disable=attribute-defined-outside-init
# pylint: disable=no-member
-from collections import namedtuple
import json
from collections import namedtuple
@@ -15,7 +14,6 @@ from student.models import CourseEnrollment
from student.tests.factories import UserFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
-from xmodule.modulestore.django import modulestore
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from ..models import CourseUserGroup, CourseCohort
@@ -180,13 +178,26 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
config_course_cohorts(self.course, [], cohorted=True)
- # Get the cohorts from the course. This will run the migrations.
- # And due to migrations CourseCohortsSettings object will be created.
- self.get_handler(self.course)
-
response = self.put_handler(self.course, expected_response_code=400, handler=course_cohort_settings_handler)
self.assertEqual("Bad Request", response.get("error"))
+ def test_update_settings_with_invalid_field_data_type(self):
+ """
+ Verify that course_cohort_settings_handler return HTTP 400 if field data type is incorrect.
+ """
+ config_course_cohorts(self.course, [], cohorted=True)
+
+ response = self.put_handler(
+ self.course,
+ data={'is_cohorted': ''},
+ expected_response_code=400,
+ handler=course_cohort_settings_handler
+ )
+ self.assertEqual(
+ "Incorrect field type for `{}`. Type must be `{}`".format('is_cohorted', bool.__name__),
+ response.get("error")
+ )
+
class CohortHandlerTestCase(CohortViewsTestCase):
"""
diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py
index 9d9356794f..e3c89f5345 100644
--- a/openedx/core/djangoapps/course_groups/views.py
+++ b/openedx/core/djangoapps/course_groups/views.py
@@ -112,7 +112,13 @@ def course_cohort_settings_handler(request, course_key_string):
if is_cohorted is None:
# Note: error message not translated because it is not exposed to the user (UI prevents this state).
return JsonResponse({"error": "Bad Request"}, 400)
- cohort_settings = cohorts.set_course_cohort_settings(course_key, is_cohorted=is_cohorted)
+
+ try:
+ cohort_settings = cohorts.set_course_cohort_settings(course_key, is_cohorted=is_cohorted)
+ except ValueError as err:
+ # Note: error message not translated because it is not exposed to the user (UI prevents this state).
+ return JsonResponse({"error": unicode(err)}, 400)
+
return JsonResponse(_get_course_cohort_settings_representation(cohort_settings))
From 9ce66c78fdea06560051af46f552859eca6bd14c Mon Sep 17 00:00:00 2001
From: muhammad-ammar
Date: Tue, 3 Mar 2015 13:17:01 +0500
Subject: [PATCH 05/13] Rename `Cohort Management` to `Cohorts` Move `Cohorts`
section after `Membership` section
---
lms/djangoapps/instructor/views/instructor_dashboard.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py
index ed6b7aefea..a7ed90df09 100644
--- a/lms/djangoapps/instructor/views/instructor_dashboard.py
+++ b/lms/djangoapps/instructor/views/instructor_dashboard.py
@@ -74,10 +74,10 @@ def instructor_dashboard_2(request, course_id):
sections = [
_section_course_info(course, access),
_section_membership(course, access),
+ _section_cohort_management(course, access),
_section_student_admin(course, access),
_section_data_download(course, access),
_section_analytics(course, access),
- _section_cohort_management(course, access),
]
#check if there is corresponding entry in the CourseMode Table related to the Instructor Dashboard course
@@ -338,7 +338,7 @@ def _section_cohort_management(course, access):
course_key = course.id
section_data = {
'section_key': 'cohort_management',
- 'section_display_name': _('Cohort Management'),
+ 'section_display_name': _('Cohorts'),
'access': access,
'course_cohort_settings_url': reverse(
'course_cohort_settings',
From b56e123350deb871cf9b1af3cba4f3e479c781de Mon Sep 17 00:00:00 2001
From: muhammad-ammar
Date: Thu, 5 Mar 2015 23:46:32 +0500
Subject: [PATCH 06/13] Change labels
---
.../instructor_dashboard_2/cohort-form.underscore | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
index b3cd072ad5..600156ac7e 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore
@@ -35,13 +35,13 @@
<% } %>
- /> <%- gettext("Automatically Assigned") %>
+ /> <%- gettext("Automatic") %>
- /> <%- gettext("Manually Assigned") %>
+ /> <%- gettext("Manual") %>
<% if (isDefaultCohort) { %>
From d59be9949eba1c6d0490f40f76c66f5d4b4f822f Mon Sep 17 00:00:00 2001
From: Usman Khalid <2200617@gmail.com>
Date: Sat, 21 Feb 2015 20:06:59 +0500
Subject: [PATCH 07/13] Use cohort settings from CourseCohortSettings.
TNL-1258
---
common/lib/xmodule/xmodule/course_module.py | 10 ++
.../lib/xmodule/xmodule/tests/test_import.py | 3 +
.../acceptance/tests/discussion/helpers.py | 12 +-
.../django_comment_client/forum/tests.py | 6 +-
.../django_comment_client/tests/test_utils.py | 37 ++++--
lms/djangoapps/django_comment_client/utils.py | 21 ++--
lms/djangoapps/instructor/tests/test_api.py | 5 +-
lms/djangoapps/instructor/views/api.py | 3 +-
lms/djangoapps/instructor/views/legacy.py | 3 +
.../instructor_task/tasks_helper.py | 7 +-
.../legacy_instructor_dashboard.html | 2 +-
.../core/djangoapps/course_groups/cohorts.py | 45 ++++----
.../djangoapps/course_groups/tests/helpers.py | 69 +++++++++++-
.../course_groups/tests/test_cohorts.py | 105 +++++++++---------
.../tests/test_partition_scheme.py | 2 +-
.../course_groups/tests/test_views.py | 28 ++---
16 files changed, 227 insertions(+), 131 deletions(-)
diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py
index 085aca92dc..9d1e3ee99b 100644
--- a/common/lib/xmodule/xmodule/course_module.py
+++ b/common/lib/xmodule/xmodule/course_module.py
@@ -1046,6 +1046,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
def is_cohorted(self):
"""
Return whether the course is cohorted.
+
+ Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
"""
config = self.cohort_config
if config is None:
@@ -1057,6 +1059,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
def auto_cohort(self):
"""
Return whether the course is auto-cohorted.
+
+ Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
"""
if not self.is_cohorted:
return False
@@ -1070,6 +1074,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
Return the list of groups to put students into. Returns [] if not
specified. Returns specified list even if is_cohorted and/or auto_cohort are
false.
+
+ Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
"""
if self.cohort_config is None:
return []
@@ -1090,6 +1096,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
Return the set of discussions that is explicitly cohorted. It may be
the empty set. Note that all inline discussions are automatically
cohorted based on the course's is_cohorted setting.
+
+ Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
"""
config = self.cohort_config
if config is None:
@@ -1103,6 +1111,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
This allow to change the default behavior of inline discussions cohorting. By
setting this to False, all inline discussions are non-cohorted unless their
ids are specified in cohorted_discussions.
+
+ Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
"""
config = self.cohort_config
if config is None:
diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py
index c70e1f2742..f185186459 100644
--- a/common/lib/xmodule/xmodule/tests/test_import.py
+++ b/common/lib/xmodule/xmodule/tests/test_import.py
@@ -643,6 +643,9 @@ class ImportTestCase(BaseCourseTestCase):
def test_cohort_config(self):
"""
Check that cohort config parsing works right.
+
+ Note: The cohort config on the CourseModule is no longer used.
+ See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
"""
modulestore = XMLModuleStore(DATA_DIR, source_dirs=['toy'])
diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py
index 3f4c04e6bc..a5fba485b0 100644
--- a/common/test/acceptance/tests/discussion/helpers.py
+++ b/common/test/acceptance/tests/discussion/helpers.py
@@ -60,13 +60,11 @@ class CohortTestMixin(object):
"""
Disables cohorting for the current course fixture.
"""
- course_fixture._update_xblock(course_fixture._course_location, {
- "metadata": {
- u"cohort_config": {
- "cohorted": False
- },
- },
- })
+ url = LMS_BASE_URL + "/courses/" + course_fixture._course_key + '/cohorts/settings' # pylint: disable=protected-access
+ data = json.dumps({'is_cohorted': False})
+ response = course_fixture.session.post(url, data=data, headers=course_fixture.headers)
+ self.assertTrue(response.ok, "Failed to disable cohorts")
+
def add_manual_cohort(self, course_fixture, cohort_name):
"""
diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py
index 7242c12879..d99657d009 100644
--- a/lms/djangoapps/django_comment_client/forum/tests.py
+++ b/lms/djangoapps/django_comment_client/forum/tests.py
@@ -315,9 +315,9 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
MODULESTORE = TEST_DATA_MONGO_MODULESTORE
@ddt.data(
- # old mongo with cache: number of responses plus 17. TODO: O(n)!
- (ModuleStoreEnum.Type.mongo, 1, 23, 18),
- (ModuleStoreEnum.Type.mongo, 50, 366, 67),
+ # old mongo with cache: 15
+ (ModuleStoreEnum.Type.mongo, 1, 21, 15),
+ (ModuleStoreEnum.Type.mongo, 50, 315, 15),
# split mongo: 3 queries, regardless of thread response size.
(ModuleStoreEnum.Type.split, 1, 3, 3),
(ModuleStoreEnum.Type.split, 50, 3, 3),
diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py
index 79d4cadf0e..1f99a364ff 100644
--- a/lms/djangoapps/django_comment_client/tests/test_utils.py
+++ b/lms/djangoapps/django_comment_client/tests/test_utils.py
@@ -1,18 +1,24 @@
# -*- coding: utf-8 -*-
from datetime import datetime
import json
+import mock
from pytz import UTC
+from django_comment_client.tests.factories import RoleFactory
+from django_comment_client.tests.unicode import UnicodeTestMixin
+import django_comment_client.utils as utils
from django.core.urlresolvers import reverse
from django.test import TestCase
from edxmako import add_lookup
-import mock
from django_comment_client.tests.factories import RoleFactory
from django_comment_client.tests.unicode import UnicodeTestMixin
from django_comment_client.tests.utils import ContentGroupTestCase
import django_comment_client.utils as utils
+
+from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings
from student.tests.factories import UserFactory, CourseEnrollmentFactory
+from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
@@ -218,20 +224,35 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
check_cohorted_topics([]) # default (empty) cohort config
- self.course.cohort_config = {"cohorted": False, "cohorted_discussions": []}
+ set_course_cohort_settings(course_key=self.course.id, is_cohorted=False, cohorted_discussions=[])
check_cohorted_topics([])
- self.course.cohort_config = {"cohorted": True, "cohorted_discussions": []}
+ set_course_cohort_settings(course_key=self.course.id, is_cohorted=True, cohorted_discussions=[])
check_cohorted_topics([])
- self.course.cohort_config = {"cohorted": True, "cohorted_discussions": ["Topic_B", "Topic_C"]}
+ set_course_cohort_settings(
+ course_key=self.course.id,
+ is_cohorted=True,
+ cohorted_discussions=["Topic_B", "Topic_C"],
+ always_cohort_inline_discussions=False,
+ )
check_cohorted_topics(["Topic_B", "Topic_C"])
- self.course.cohort_config = {"cohorted": True, "cohorted_discussions": ["Topic_A", "Some_Other_Topic"]}
+ set_course_cohort_settings(
+ course_key=self.course.id,
+ is_cohorted=True,
+ cohorted_discussions=["Topic_A", "Some_Other_Topic"],
+ always_cohort_inline_discussions=False,
+ )
check_cohorted_topics(["Topic_A"])
# unlikely case, but make sure it works.
- self.course.cohort_config = {"cohorted": False, "cohorted_discussions": ["Topic_A"]}
+ set_course_cohort_settings(
+ course_key=self.course.id,
+ is_cohorted=False,
+ cohorted_discussions=["Topic_A"],
+ always_cohort_inline_discussions=False,
+ )
check_cohorted_topics([])
def test_single_inline(self):
@@ -352,11 +373,11 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
check_cohorted(False)
# explicitly disabled cohorting
- self.course.cohort_config = {"cohorted": False}
+ set_course_cohort_settings(course_key=self.course.id, is_cohorted=False)
check_cohorted(False)
# explicitly enabled cohorting
- self.course.cohort_config = {"cohorted": True}
+ set_course_cohort_settings(course_key=self.course.id, is_cohorted=True)
check_cohorted(True)
def test_tree_with_duplicate_targets(self):
diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py
index 0a139850fa..3a91daef15 100644
--- a/lms/djangoapps/django_comment_client/utils.py
+++ b/lms/djangoapps/django_comment_client/utils.py
@@ -19,8 +19,9 @@ from django_comment_client.permissions import check_permissions_by_view, cached_
from edxmako import lookup_template
from courseware.access import has_access
-from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted, \
- is_course_cohorted
+from openedx.core.djangoapps.course_groups.cohorts import (
+ get_course_cohort_settings, get_cohort_by_id, get_cohort_id, is_commentable_cohorted, is_course_cohorted
+)
from openedx.core.djangoapps.course_groups.models import CourseUserGroup
@@ -154,8 +155,7 @@ def get_discussion_category_map(course, user):
modules = get_accessible_discussion_modules(course, user)
- is_course_cohorted = course.is_cohorted
- cohorted_discussion_ids = course.cohorted_discussions
+ course_cohort_settings = get_course_cohort_settings(course.id)
for module in modules:
id = module.discussion_id
@@ -209,16 +209,19 @@ def get_discussion_category_map(course, user):
node[level]["entries"][title] = {"id": entry["id"],
"sort_key": entry["sort_key"],
"start_date": entry["start_date"],
- "is_cohorted": is_course_cohorted}
+ "is_cohorted": course_cohort_settings.is_cohorted}
# TODO. BUG! : course location is not unique across multiple course runs!
# (I think Kevin already noticed this) Need to send course_id with requests, store it
# in the backend.
for topic, entry in course.discussion_topics.items():
- category_map['entries'][topic] = {"id": entry["id"],
- "sort_key": entry.get("sort_key", topic),
- "start_date": datetime.now(UTC()),
- "is_cohorted": is_course_cohorted and entry["id"] in cohorted_discussion_ids}
+ category_map['entries'][topic] = {
+ "id": entry["id"],
+ "sort_key": entry.get("sort_key", topic),
+ "start_date": datetime.now(UTC()),
+ "is_cohorted": (course_cohort_settings.is_cohorted and
+ entry["id"] in course_cohort_settings.cohorted_discussions)
+ }
_sort_map_entries(category_map, course.discussion_sort_alpha)
diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py
index 24eeac62c1..77624cfdd8 100644
--- a/lms/djangoapps/instructor/tests/test_api.py
+++ b/lms/djangoapps/instructor/tests/test_api.py
@@ -58,6 +58,8 @@ from instructor.views.api import generate_unique_password
from instructor.views.api import _split_input_list, common_exceptions_400
from instructor_task.api_helper import AlreadyRunningError
+from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings
+
from .test_tools import msk_from_problem_urlname
from ..views.tools import get_extended_due
@@ -2002,8 +2004,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa
cohorted, and does not when the course is not cohorted.
"""
url = reverse('get_students_features', kwargs={'course_id': unicode(self.course.id)})
- self.course.cohort_config = {'cohorted': is_cohorted}
- self.store.update_item(self.course, self.instructor.id)
+ set_course_cohort_settings(self.course.id, is_cohorted=is_cohorted)
response = self.client.get(url, {})
res_json = json.loads(response.content)
diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py
index b33abce283..8d5247c1c3 100644
--- a/lms/djangoapps/instructor/views/api.py
+++ b/lms/djangoapps/instructor/views/api.py
@@ -104,6 +104,7 @@ from .tools import (
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from opaque_keys import InvalidKeyError
+from openedx.core.djangoapps.course_groups.cohorts import is_course_cohorted
log = logging.getLogger(__name__)
@@ -1002,7 +1003,7 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=red
'goals': _('Goals'),
}
- if course.is_cohorted:
+ if is_course_cohorted(course.id):
# Translators: 'Cohort' refers to a group of students within a course.
query_features.append('cohort')
query_features_names['cohort'] = _('Cohort')
diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py
index 09ecfe05d3..4f208cb343 100644
--- a/lms/djangoapps/instructor/views/legacy.py
+++ b/lms/djangoapps/instructor/views/legacy.py
@@ -56,6 +56,8 @@ from django.utils.translation import ugettext as _
from microsite_configuration import microsite
from opaque_keys.edx.locations import i4xEncoder
+from openedx.core.djangoapps.course_groups.cohorts import is_course_cohorted
+
log = logging.getLogger(__name__)
@@ -451,6 +453,7 @@ def instructor_dashboard(request, course_id):
context = {
'course': course,
+ 'course_is_cohorted': is_course_cohorted(course.id),
'staff_access': True,
'admin_access': request.user.is_staff,
'instructor_access': instructor_access,
diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py
index de89f61330..92087df572 100644
--- a/lms/djangoapps/instructor_task/tasks_helper.py
+++ b/lms/djangoapps/instructor_task/tasks_helper.py
@@ -34,7 +34,7 @@ from lms.djangoapps.lms_xblock.runtime import LmsPartitionService
from openedx.core.djangoapps.course_groups.cohorts import get_cohort
from openedx.core.djangoapps.course_groups.models import CourseUserGroup
from opaque_keys.edx.keys import UsageKey
-from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort
+from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted
from student.models import CourseEnrollment
@@ -578,7 +578,8 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input,
TASK_LOG.info(u'%s, Task type: %s, Starting task execution', task_info_string, action_name)
course = get_course_by_id(course_id)
- cohorts_header = ['Cohort Name'] if course.is_cohorted else []
+ course_is_cohorted = is_course_cohorted(course.id)
+ cohorts_header = ['Cohort Name'] if course_is_cohorted else []
experiment_partitions = get_split_user_partitions(course.user_partitions)
group_configs_header = [u'Experiment Group ({})'.format(partition.name) for partition in experiment_partitions]
@@ -632,7 +633,7 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input,
}
cohorts_group_name = []
- if course.is_cohorted:
+ if course_is_cohorted:
group = get_cohort(student, course_id, assign=False)
cohorts_group_name.append(group.name if group else '')
diff --git a/lms/templates/courseware/legacy_instructor_dashboard.html b/lms/templates/courseware/legacy_instructor_dashboard.html
index 9fbe0b3fff..e80a7ee2df 100644
--- a/lms/templates/courseware/legacy_instructor_dashboard.html
+++ b/lms/templates/courseware/legacy_instructor_dashboard.html
@@ -372,7 +372,7 @@ function goto( mode)
%if modeflag.get('Manage Groups'):
%if instructor_access:
- %if course.is_cohorted:
+ %if course_is_cohorted:
${_("To manage beta tester roles and cohorts, visit the Membership section of the Instructor Dashboard.")}
%else:
${_("To manage beta tester roles, visit the Membership section of the Instructor Dashboard.")}
diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py
index 982b359fd0..ffc520825b 100644
--- a/openedx/core/djangoapps/course_groups/cohorts.py
+++ b/openedx/core/djangoapps/course_groups/cohorts.py
@@ -71,12 +71,9 @@ def _cohort_membership_changed(sender, **kwargs):
tracker.emit(event_name, event)
-# A 'default cohort' is an auto-cohort that is automatically created for a course if no auto_cohort_groups have been
-# specified. It is intended to be used in a cohorted-course for users who have yet to be assigned to a cohort.
-# Note 1: If an administrator chooses to configure a cohort with the same name, the said cohort will be used as
-# the "default cohort".
-# Note 2: If auto_cohort_groups are configured after the 'default cohort' has been created and populated, the
-# stagnant 'default cohort' will still remain (now as a manual cohort) with its previously assigned students.
+# A 'default cohort' is an auto-cohort that is automatically created for a course if no cohort with automatic
+# assignment have been specified. It is intended to be used in a cohorted-course for users who have yet to be assigned
+# to a cohort.
# Translation Note: We are NOT translating this string since it is the constant identifier for the "default group"
# and needed across product boundaries.
DEFAULT_COHORT_NAME = "Default Group"
@@ -110,7 +107,7 @@ def is_course_cohorted(course_key):
Raises:
Http404 if the course doesn't exist.
"""
- return courses.get_course_by_id(course_key).is_cohorted
+ return get_course_cohort_settings(course_key).is_cohorted
def get_cohort_id(user, course_key):
@@ -135,18 +132,19 @@ def is_commentable_cohorted(course_key, commentable_id):
Http404 if the course doesn't exist.
"""
course = courses.get_course_by_id(course_key)
+ course_cohort_settings = get_course_cohort_settings(course_key)
- if not course.is_cohorted:
+ if not course_cohort_settings.is_cohorted:
# this is the easy case :)
ans = False
elif (
commentable_id in course.top_level_discussion_topic_ids or
- course.always_cohort_inline_discussions is False
+ course_cohort_settings.always_cohort_inline_discussions is False
):
# top level discussions have to be manually configured as cohorted
# (default is not).
# Same thing for inline discussions if the default is explicitly set to False in settings
- ans = commentable_id in course.cohorted_discussions
+ ans = commentable_id in course_cohort_settings.cohorted_discussions
else:
# inline discussions are cohorted by default
ans = True
@@ -162,13 +160,13 @@ def get_cohorted_commentables(course_key):
Given a course_key return a set of strings representing cohorted commentables.
"""
- course = courses.get_course_by_id(course_key)
+ course_cohort_settings = get_course_cohort_settings(course_key)
- if not course.is_cohorted:
+ if not course_cohort_settings.is_cohorted:
# this is the easy case :)
ans = set()
else:
- ans = course.cohorted_discussions
+ ans = set(course_cohort_settings.cohorted_discussions)
return ans
@@ -193,12 +191,10 @@ def get_cohort(user, course_key, assign=True):
"""
# First check whether the course is cohorted (users shouldn't be in a cohort
# in non-cohorted courses, but settings can change after course starts)
- try:
- course = courses.get_course_by_id(course_key)
- except Http404:
- raise ValueError("Invalid course_key")
+ course = courses.get_course(course_key)
+ course_cohort_settings = get_course_cohort_settings(course.id)
- if not course.is_cohorted:
+ if not course_cohort_settings.is_cohorted:
return None
try:
@@ -232,9 +228,8 @@ def migrate_cohort_settings(course):
Migrate all the cohort settings associated with this course from modulestore to mysql.
After that we will never touch modulestore for any cohort related settings.
"""
- course_id = course.location.course_key
cohort_settings, created = CourseCohortsSettings.objects.get_or_create(
- course_id=course_id,
+ course_id=course.id,
defaults={
'is_cohorted': course.is_cohorted,
'cohorted_discussions': list(course.cohorted_discussions),
@@ -246,14 +241,14 @@ def migrate_cohort_settings(course):
if created:
# Update the manual cohorts already present in CourseUserGroup
manual_cohorts = CourseUserGroup.objects.filter(
- course_id=course_id,
+ course_id=course.id,
group_type=CourseUserGroup.COHORT
).exclude(name__in=course.auto_cohort_groups)
for cohort in manual_cohorts:
CourseCohort.create(course_user_group=cohort)
for group_name in course.auto_cohort_groups:
- CourseCohort.create(cohort_name=group_name, course_id=course_id, assignment_type=CourseCohort.RANDOM)
+ CourseCohort.create(cohort_name=group_name, course_id=course.id, assignment_type=CourseCohort.RANDOM)
return cohort_settings
@@ -454,7 +449,7 @@ def set_course_cohort_settings(course_key, **kwargs):
A CourseCohortSettings object.
Raises:
- ValueError if course_key is invalid.
+ Http404 if course_key is invalid.
"""
fields = {'is_cohorted': bool, 'always_cohort_inline_discussions': bool, 'cohorted_discussions': list}
course_cohort_settings = get_course_cohort_settings(course_key)
@@ -478,11 +473,11 @@ def get_course_cohort_settings(course_key):
A CourseCohortSettings object.
Raises:
- ValueError if course_key is invalid.
+ Http404 if course_key is invalid.
"""
try:
course_cohort_settings = CourseCohortsSettings.objects.get(course_id=course_key)
except CourseCohortsSettings.DoesNotExist:
- course = courses.get_course(course_key)
+ course = courses.get_course_by_id(course_key)
course_cohort_settings = migrate_cohort_settings(course)
return course_cohort_settings
diff --git a/openedx/core/djangoapps/course_groups/tests/helpers.py b/openedx/core/djangoapps/course_groups/tests/helpers.py
index 4964adada9..f14e250aac 100644
--- a/openedx/core/djangoapps/course_groups/tests/helpers.py
+++ b/openedx/core/djangoapps/course_groups/tests/helpers.py
@@ -4,13 +4,15 @@ Helper methods for testing cohorts.
import factory
from factory import post_generation, Sequence
from factory.django import DjangoModelFactory
+import json
+
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from xmodule.modulestore.django import modulestore
from xmodule.modulestore import ModuleStoreEnum
-from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings
-
import json
+from ..cohorts import get_course_cohort_settings, set_course_cohort_settings
+from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings
class CohortFactory(DjangoModelFactory):
@@ -67,7 +69,7 @@ def topic_name_to_id(course, name):
)
-def config_course_cohorts(
+def config_course_cohorts_legacy(
course,
discussions,
cohorted,
@@ -77,7 +79,11 @@ def config_course_cohorts(
):
"""
Given a course with no discussion set up, add the discussions and set
- the cohort config appropriately.
+ the cohort config on the course descriptor.
+
+ Since cohort settings are now stored in models.CourseCohortSettings,
+ this is only used for testing data migration from the CourseDescriptor
+ to the table.
Arguments:
course: CourseDescriptor
@@ -105,7 +111,6 @@ def config_course_cohorts(
if cohorted_discussions is not None:
config["cohorted_discussions"] = [to_id(name)
for name in cohorted_discussions]
-
if auto_cohort_groups is not None:
config["auto_cohort_groups"] = auto_cohort_groups
@@ -119,3 +124,57 @@ def config_course_cohorts(
modulestore().update_item(course, ModuleStoreEnum.UserID.test)
except NotImplementedError:
pass
+
+
+def config_course_cohorts(
+ course,
+ is_cohorted,
+ auto_cohorts=[],
+ manual_cohorts=[],
+ discussion_topics=[],
+ cohorted_discussions=[],
+ always_cohort_inline_discussions=True # pylint: disable=invalid-name
+):
+ """
+ Set discussions and configure cohorts for a course.
+
+ Arguments:
+ course: CourseDescriptor
+ is_cohorted (bool): Is the course cohorted?
+ auto_cohorts (list): Names of auto cohorts to create.
+ manual_cohorts (list): Names of manual cohorts to create.
+ discussion_topics (list): Discussion topic names. Picks ids and
+ sort_keys automatically.
+ cohorted_discussions: Discussion topics to cohort. Converts the
+ list to use the same ids as discussion topic names.
+ always_cohort_inline_discussions (bool): Whether inline discussions
+ should be cohorted by default.
+
+ Returns:
+ Nothing -- modifies course in place.
+ """
+ def to_id(name):
+ return topic_name_to_id(course, name)
+
+ set_course_cohort_settings(
+ course.id,
+ is_cohorted = is_cohorted,
+ cohorted_discussions = [to_id(name) for name in cohorted_discussions],
+ always_cohort_inline_discussions = always_cohort_inline_discussions
+ )
+
+ for cohort_name in auto_cohorts:
+ cohort = CohortFactory(course_id=course.id, name=cohort_name)
+ CourseCohortFactory(course_user_group=cohort, assignment_type=CourseCohort.RANDOM)
+
+ for cohort_name in manual_cohorts:
+ cohort = CohortFactory(course_id=course.id, name=cohort_name)
+ CourseCohortFactory(course_user_group=cohort, assignment_type=CourseCohort.MANUAL)
+
+ course.discussion_topics = dict((name, {"sort_key": "A", "id": to_id(name)})
+ for name in discussion_topics)
+ try:
+ # Not implemented for XMLModulestore, which is used by test_cohorts.
+ modulestore().update_item(course, ModuleStoreEnum.UserID.test)
+ except NotImplementedError:
+ pass
diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
index 0b704aa7ef..9985fd5964 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
@@ -19,7 +19,8 @@ from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_TOY_MODULESTO
from ..models import CourseUserGroup, CourseCohort, CourseUserGroupPartitionGroup
from .. import cohorts
from ..tests.helpers import (
- topic_name_to_id, config_course_cohorts, CohortFactory, CourseCohortFactory, CourseCohortSettingsFactory
+ topic_name_to_id, config_course_cohorts, config_course_cohorts_legacy,
+ CohortFactory, CourseCohortFactory, CourseCohortSettingsFactory
)
@patch("openedx.core.djangoapps.course_groups.cohorts.tracker")
@@ -146,12 +147,10 @@ class TestCohorts(ModuleStoreTestCase):
Make sure cohorts.is_course_cohorted() correctly reports if a course is cohorted or not.
"""
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
self.assertFalse(cohorts.is_course_cohorted(course.id))
- config_course_cohorts(course, [], cohorted=True)
+ config_course_cohorts(course, is_cohorted=True)
- self.assertTrue(course.is_cohorted)
self.assertTrue(cohorts.is_course_cohorted(course.id))
# Make sure we get a Http404 if there's no course
@@ -164,12 +163,12 @@ class TestCohorts(ModuleStoreTestCase):
invalid course key.
"""
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
user = UserFactory(username="test", email="a@b.com")
self.assertIsNone(cohorts.get_cohort_id(user, course.id))
- config_course_cohorts(course, discussions=[], cohorted=True)
+ config_course_cohorts(course, is_cohorted=True)
cohort = CohortFactory(course_id=course.id, name="TestCohort")
cohort.users.add(user)
self.assertEqual(cohorts.get_cohort_id(user, course.id), cohort.id)
@@ -221,7 +220,7 @@ class TestCohorts(ModuleStoreTestCase):
"""
course = modulestore().get_course(self.toy_course_key)
self.assertEqual(course.id, self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
user = UserFactory(username="test", email="a@b.com")
other_user = UserFactory(username="test2", email="a2@b.com")
@@ -237,7 +236,7 @@ class TestCohorts(ModuleStoreTestCase):
)
# Make the course cohorted...
- config_course_cohorts(course, discussions=[], cohorted=True)
+ config_course_cohorts(course, is_cohorted=True)
self.assertEquals(
cohorts.get_cohort(user, course.id).id,
@@ -256,16 +255,15 @@ class TestCohorts(ModuleStoreTestCase):
assigned to a user instead of assigning/creating a group automatically
"""
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
user = UserFactory(username="test", email="a@b.com")
# Add an auto_cohort_group to the course...
config_course_cohorts(
course,
- discussions=[],
- cohorted=True,
- auto_cohort_groups=["AutoGroup"]
+ is_cohorted=True,
+ auto_cohorts=["AutoGroup"]
)
# get_cohort should return None as no group is assigned to user
@@ -274,13 +272,13 @@ class TestCohorts(ModuleStoreTestCase):
# get_cohort should return a group for user
self.assertEquals(cohorts.get_cohort(user, course.id).name, "AutoGroup")
- def test_cohorting_with_auto_cohort_groups(self):
+ def test_cohorting_with_auto_cohorts(self):
"""
- Make sure cohorts.get_cohort() does the right thing with auto_cohort_groups.
+ Make sure cohorts.get_cohort() does the right thing.
If there are auto cohort groups then a user should be assigned one.
"""
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
user1 = UserFactory(username="test", email="a@b.com")
user2 = UserFactory(username="test2", email="a2@b.com")
@@ -293,9 +291,8 @@ class TestCohorts(ModuleStoreTestCase):
# Add an auto_cohort_group to the course...
config_course_cohorts(
course,
- discussions=[],
- cohorted=True,
- auto_cohort_groups=["AutoGroup"]
+ is_cohorted=True,
+ auto_cohorts=["AutoGroup"]
)
self.assertEquals(cohorts.get_cohort(user1, course.id).id, cohort.id, "user1 should stay put")
@@ -315,16 +312,15 @@ class TestCohorts(ModuleStoreTestCase):
# Add an auto_cohort_group to the course...
config_course_cohorts(
course,
- discussions=[],
- cohorted=True,
- auto_cohort_groups=["AutoGroup"]
+ is_cohorted=True,
+ auto_cohorts=["AutoGroup"]
)
self.assertEquals(cohorts.get_cohort(user1, course.id).name, "AutoGroup", "user1 should be auto-cohorted")
# Now set the auto_cohort_group to something different
# This will have no effect on lms side as we are already done with migrations
- config_course_cohorts(
+ config_course_cohorts_legacy(
course,
discussions=[],
cohorted=True,
@@ -339,15 +335,15 @@ class TestCohorts(ModuleStoreTestCase):
cohorts.get_cohort(user1, course.id).name, "AutoGroup", "user1 should still be in originally placed cohort"
)
- def test_cohorting_with_no_auto_cohort_groups(self):
+ def test_cohorting_with_no_auto_cohorts(self):
"""
- Make sure cohorts.get_cohort() does the right thing with auto_cohort_groups.
- If there are not auto cohort groups then a user should be assigned to Default Cohort Group.
+ Make sure cohorts.get_cohort() does the right thing.
+ If there are not auto cohorts then a user should be assigned to Default Cohort Group.
Also verifies that cohort config changes on studio/moduletore side will
not be reflected on lms after the migrations are done.
"""
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
user1 = UserFactory(username="test", email="a@b.com")
user2 = UserFactory(username="test2", email="a2@b.com")
@@ -355,9 +351,8 @@ class TestCohorts(ModuleStoreTestCase):
# Make the auto_cohort_group list empty
config_course_cohorts(
course,
- discussions=[],
- cohorted=True,
- auto_cohort_groups=[]
+ is_cohorted=True,
+ auto_cohorts=[]
)
self.assertEquals(
@@ -368,7 +363,7 @@ class TestCohorts(ModuleStoreTestCase):
# Add an auto_cohort_group to the course
# This will have no effect on lms side as we are already done with migrations
- config_course_cohorts(
+ config_course_cohorts_legacy(
course,
discussions=[],
cohorted=True,
@@ -393,11 +388,11 @@ class TestCohorts(ModuleStoreTestCase):
Make sure cohorts.get_cohort() randomizes properly.
"""
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
groups = ["group_{0}".format(n) for n in range(5)]
config_course_cohorts(
- course, discussions=[], cohorted=True, auto_cohort_groups=groups
+ course, is_cohorted=True, auto_cohorts=groups
)
# Assign 100 users to cohorts
@@ -423,7 +418,7 @@ class TestCohorts(ModuleStoreTestCase):
Tests get_course_cohorts returns an empty list when no cohorts exist.
"""
course = modulestore().get_course(self.toy_course_key)
- config_course_cohorts(course, [], cohorted=True)
+ config_course_cohorts(course, is_cohorted=True)
self.assertEqual([], cohorts.get_course_cohorts(course))
def test_get_course_cohorts(self):
@@ -432,8 +427,9 @@ class TestCohorts(ModuleStoreTestCase):
"""
course = modulestore().get_course(self.toy_course_key)
config_course_cohorts(
- course, [], cohorted=True,
- auto_cohort_groups=["AutoGroup1", "AutoGroup2"]
+ course,
+ is_cohorted=True,
+ auto_cohorts=["AutoGroup1", "AutoGroup2"]
)
# add manual cohorts to course 1
@@ -445,7 +441,7 @@ class TestCohorts(ModuleStoreTestCase):
def test_is_commentable_cohorted(self):
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
def to_id(name):
return topic_name_to_id(course, name)
@@ -457,7 +453,7 @@ class TestCohorts(ModuleStoreTestCase):
)
# not cohorted
- config_course_cohorts(course, ["General", "Feedback"], cohorted=False)
+ config_course_cohorts(course, is_cohorted=False, discussion_topics=["General", "Feedback"])
self.assertFalse(
cohorts.is_commentable_cohorted(course.id, to_id("General")),
@@ -465,9 +461,9 @@ class TestCohorts(ModuleStoreTestCase):
)
# cohorted, but top level topics aren't
- config_course_cohorts(course, ["General", "Feedback"], cohorted=True)
+ config_course_cohorts(course, is_cohorted=True, discussion_topics=["General", "Feedback"])
- self.assertTrue(course.is_cohorted)
+ self.assertTrue(cohorts.is_course_cohorted(course.id))
self.assertFalse(
cohorts.is_commentable_cohorted(course.id, to_id("General")),
"Course is cohorted, but 'General' isn't."
@@ -475,12 +471,13 @@ class TestCohorts(ModuleStoreTestCase):
# cohorted, including "Feedback" top-level topics aren't
config_course_cohorts(
- course, ["General", "Feedback"],
- cohorted=True,
+ course,
+ is_cohorted=True,
+ discussion_topics= ["General", "Feedback"],
cohorted_discussions=["Feedback"]
)
- self.assertTrue(course.is_cohorted)
+ self.assertTrue(cohorts.is_course_cohorted(course.id))
self.assertFalse(
cohorts.is_commentable_cohorted(course.id, to_id("General")),
"Course is cohorted, but 'General' isn't."
@@ -492,14 +489,15 @@ class TestCohorts(ModuleStoreTestCase):
def test_is_commentable_cohorted_inline_discussion(self):
course = modulestore().get_course(self.toy_course_key)
- self.assertFalse(course.is_cohorted)
+ self.assertFalse(cohorts.is_course_cohorted(course.id))
def to_id(name): # pylint: disable=missing-docstring
return topic_name_to_id(course, name)
config_course_cohorts(
- course, ["General", "Feedback"],
- cohorted=True,
+ course,
+ is_cohorted=True,
+ discussion_topics =["General", "Feedback"],
cohorted_discussions=["Feedback", "random_inline"]
)
self.assertTrue(
@@ -510,8 +508,9 @@ class TestCohorts(ModuleStoreTestCase):
# if always_cohort_inline_discussions is set to False, non-top-level discussion are always
# non cohorted unless they are explicitly set in cohorted_discussions
config_course_cohorts(
- course, ["General", "Feedback"],
- cohorted=True,
+ course,
+ is_cohorted=True,
+ discussion_topics=["General", "Feedback"],
cohorted_discussions=["Feedback", "random_inline"],
always_cohort_inline_discussions=False
)
@@ -538,12 +537,13 @@ class TestCohorts(ModuleStoreTestCase):
self.assertEqual(cohorts.get_cohorted_commentables(course.id), set())
- config_course_cohorts(course, [], cohorted=True)
+ config_course_cohorts(course, is_cohorted=True)
self.assertEqual(cohorts.get_cohorted_commentables(course.id), set())
config_course_cohorts(
- course, ["General", "Feedback"],
- cohorted=True,
+ course,
+ is_cohorted=True,
+ discussion_topics=["General", "Feedback"],
cohorted_discussions=["Feedback"]
)
self.assertItemsEqual(
@@ -552,8 +552,9 @@ class TestCohorts(ModuleStoreTestCase):
)
config_course_cohorts(
- course, ["General", "Feedback"],
- cohorted=True,
+ course,
+ is_cohorted=True,
+ discussion_topics=["General", "Feedback"],
cohorted_discussions=["General", "Feedback"]
)
self.assertItemsEqual(
diff --git a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py
index 5a33de1e1b..599e31fabe 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py
@@ -41,7 +41,7 @@ class TestCohortPartitionScheme(ModuleStoreTestCase):
self.course_key = SlashSeparatedCourseKey("edX", "toy", "2012_Fall")
self.course = modulestore().get_course(self.course_key)
- config_course_cohorts(self.course, [], cohorted=True)
+ config_course_cohorts(self.course, is_cohorted=True)
self.groups = [Group(10, 'Group 10'), Group(20, 'Group 20')]
self.user_partition = UserPartition(
diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py
index f3ee3d21ae..95bec032c0 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_views.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_views.py
@@ -25,7 +25,9 @@ from ..cohorts import (
get_cohort, get_cohort_by_name, get_cohort_by_id,
DEFAULT_COHORT_NAME, get_group_info_for_cohort
)
-from .helpers import config_course_cohorts, CohortFactory, CourseCohortFactory, topic_name_to_id
+from .helpers import (
+ config_course_cohorts, config_course_cohorts_legacy, CohortFactory, CourseCohortFactory, topic_name_to_id
+)
class CohortViewsTestCase(ModuleStoreTestCase):
@@ -136,7 +138,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
Verify that course_cohort_settings_handler is working for HTTP GET.
"""
cohorted_discussions = ['Topic A', 'Topic B']
- config_course_cohorts(self.course, [], cohorted=True, cohorted_discussions=cohorted_discussions)
+ config_course_cohorts(self.course, is_cohorted=True, cohorted_discussions=cohorted_discussions)
response = self.get_handler(self.course, handler=course_cohort_settings_handler)
response['cohorted_discussions'].sort()
@@ -155,7 +157,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
Verify that course_cohort_settings_handler is working for HTTP POST.
"""
- config_course_cohorts(self.course, [], cohorted=True)
+ config_course_cohorts(self.course, is_cohorted=True)
response = self.get_handler(self.course, handler=course_cohort_settings_handler)
@@ -176,7 +178,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
Verify that course_cohort_settings_handler return HTTP 400 if required data field is missing from post data.
"""
- config_course_cohorts(self.course, [], cohorted=True)
+ config_course_cohorts(self.course, is_cohorted=True)
response = self.put_handler(self.course, expected_response_code=400, handler=course_cohort_settings_handler)
self.assertEqual("Bad Request", response.get("error"))
@@ -185,7 +187,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
Verify that course_cohort_settings_handler return HTTP 400 if field data type is incorrect.
"""
- config_course_cohorts(self.course, [], cohorted=True)
+ config_course_cohorts(self.course, is_cohorted=True)
response = self.put_handler(
self.course,
@@ -268,23 +270,21 @@ class CohortHandlerTestCase(CohortViewsTestCase):
"""
Verify that auto cohorts are included in the response.
"""
- config_course_cohorts(self.course, [], cohorted=True,
- auto_cohort_groups=["AutoGroup1", "AutoGroup2"])
+ config_course_cohorts(self.course, is_cohorted=True, auto_cohorts=["AutoGroup1", "AutoGroup2"])
- # Will create cohort1, cohort2, and cohort3. Auto cohorts remain uncreated.
+ # Will create manual cohorts cohort1, cohort2, and cohort3.
self._create_cohorts()
- # Get the cohorts from the course, which will cause auto cohorts to be created.
actual_cohorts = self.get_handler(self.course)
# Get references to the created auto cohorts.
auto_cohort_1 = get_cohort_by_name(self.course.id, "AutoGroup1")
auto_cohort_2 = get_cohort_by_name(self.course.id, "AutoGroup2")
expected_cohorts = [
+ CohortHandlerTestCase.create_expected_cohort(auto_cohort_1, 0, CourseCohort.RANDOM),
+ CohortHandlerTestCase.create_expected_cohort(auto_cohort_2, 0, CourseCohort.RANDOM),
CohortHandlerTestCase.create_expected_cohort(self.cohort1, 3, CourseCohort.MANUAL),
CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CourseCohort.MANUAL),
CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CourseCohort.MANUAL),
CohortHandlerTestCase.create_expected_cohort(self.cohort4, 2, CourseCohort.RANDOM),
- CohortHandlerTestCase.create_expected_cohort(auto_cohort_1, 0, CourseCohort.RANDOM),
- CohortHandlerTestCase.create_expected_cohort(auto_cohort_2, 0, CourseCohort.RANDOM),
]
self.verify_lists_expected_cohorts(expected_cohorts, actual_cohorts)
@@ -295,8 +295,8 @@ class CohortHandlerTestCase(CohortViewsTestCase):
# verify the default cohort is not created when the course is not cohorted
self.verify_lists_expected_cohorts([])
- # create a cohorted course without any auto_cohort_groups
- config_course_cohorts(self.course, [], cohorted=True)
+ # create a cohorted course without any auto_cohorts
+ config_course_cohorts(self.course, is_cohorted=True)
# verify the default cohort is not yet created until a user is assigned
self.verify_lists_expected_cohorts([])
@@ -320,7 +320,7 @@ class CohortHandlerTestCase(CohortViewsTestCase):
# set auto_cohort_groups
# these cohort config will have not effect on lms side as we are already done with migrations
- config_course_cohorts(self.course, [], cohorted=True, auto_cohort_groups=["AutoGroup"])
+ config_course_cohorts_legacy(self.course, [], cohorted=True, auto_cohort_groups=["AutoGroup"])
# We should expect the DoesNotExist exception because above cohort config have
# no effect on lms side so as a result there will be no AutoGroup cohort present
From a9e0082c7c90c45797db634f3b4be9bf9d2df71c Mon Sep 17 00:00:00 2001
From: Usman Khalid <2200617@gmail.com>
Date: Thu, 26 Feb 2015 00:14:18 +0500
Subject: [PATCH 08/13] Cache cohort info during requests to reduce SQL
queries.
TNL-1258
---
.../core/djangoapps/course_groups/cohorts.py | 44 +++++++++++++----
.../course_groups/partition_scheme.py | 6 +--
.../djangoapps/course_groups/tests/helpers.py | 11 +++--
.../course_groups/tests/test_cohorts.py | 49 +++++++++++++++++--
.../tests/test_partition_scheme.py | 1 +
5 files changed, 90 insertions(+), 21 deletions(-)
diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py
index ffc520825b..2221afe574 100644
--- a/openedx/core/djangoapps/course_groups/cohorts.py
+++ b/openedx/core/djangoapps/course_groups/cohorts.py
@@ -110,12 +110,12 @@ def is_course_cohorted(course_key):
return get_course_cohort_settings(course_key).is_cohorted
-def get_cohort_id(user, course_key):
+def get_cohort_id(user, course_key, use_cached=False):
"""
Given a course key and a user, return the id of the cohort that user is
assigned to in that course. If they don't have a cohort, return None.
"""
- cohort = get_cohort(user, course_key)
+ cohort = get_cohort(user, course_key, use_cached=use_cached)
return None if cohort is None else cohort.id
@@ -172,7 +172,7 @@ def get_cohorted_commentables(course_key):
@transaction.commit_on_success
-def get_cohort(user, course_key, assign=True):
+def get_cohort(user, course_key, assign=True, use_cached=False):
"""
Given a Django user and a CourseKey, return the user's cohort in that
cohort.
@@ -181,6 +181,7 @@ def get_cohort(user, course_key, assign=True):
user: a Django User object.
course_key: CourseKey
assign (bool): if False then we don't assign a group to user
+ use_cached (bool): Whether to use the cached value or fetch from database.
Returns:
A CourseUserGroup object if the course is cohorted and the User has a
@@ -189,23 +190,33 @@ def get_cohort(user, course_key, assign=True):
Raises:
ValueError if the CourseKey doesn't exist.
"""
+ # pylint: disable=protected-access
+ # We cache the cohort on the user object so that we do not have to repeatedly
+ # query the database during a request. If the cached value exists, just return it.
+ if use_cached and hasattr(user, '_cohort'):
+ return user._cohort
+
# First check whether the course is cohorted (users shouldn't be in a cohort
# in non-cohorted courses, but settings can change after course starts)
course = courses.get_course(course_key)
course_cohort_settings = get_course_cohort_settings(course.id)
if not course_cohort_settings.is_cohorted:
- return None
+ user._cohort = None
+ return user._cohort
try:
- return CourseUserGroup.objects.get(
+ user._cohort = CourseUserGroup.objects.get(
course_id=course_key,
group_type=CourseUserGroup.COHORT,
users__id=user.id,
)
+ return user._cohort
except CourseUserGroup.DoesNotExist:
# Didn't find the group. We'll go on to create one if needed.
if not assign:
+ # Do not cache the cohort here, because in the next call assign
+ # may be True, and we will have to assign the user a cohort.
return None
cohorts = get_course_cohorts(course, assignment_type=CourseCohort.RANDOM)
@@ -220,7 +231,8 @@ def get_cohort(user, course_key, assign=True):
user.course_groups.add(cohort)
- return cohort
+ user._cohort = cohort
+ return user._cohort
def migrate_cohort_settings(course):
@@ -387,7 +399,7 @@ def add_user_to_cohort(cohort, username_or_email):
return (user, previous_cohort_name)
-def get_group_info_for_cohort(cohort):
+def get_group_info_for_cohort(cohort, use_cached=False):
"""
Get the ids of the group and partition to which this cohort has been linked
as a tuple of (int, int).
@@ -395,9 +407,21 @@ def get_group_info_for_cohort(cohort):
If the cohort has not been linked to any group/partition, both values in the
tuple will be None.
"""
- res = CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort)
- if len(res):
- return res[0].group_id, res[0].partition_id
+ # pylint: disable=protected-access
+ # We cache the partition group on the cohort object so that we do not have to repeatedly
+ # query the database during a request.
+ if not use_cached and hasattr(cohort, '_partition_group'):
+ delattr(cohort, '_partition_group')
+
+ if not hasattr(cohort, '_partition_group'):
+ try:
+ cohort._partition_group = CourseUserGroupPartitionGroup.objects.get(course_user_group=cohort)
+ except CourseUserGroupPartitionGroup.DoesNotExist:
+ cohort._partition_group = None
+
+ if cohort._partition_group:
+ return cohort._partition_group.group_id, cohort._partition_group.partition_id
+
return None, None
diff --git a/openedx/core/djangoapps/course_groups/partition_scheme.py b/openedx/core/djangoapps/course_groups/partition_scheme.py
index 7249699a3a..cb352ba993 100644
--- a/openedx/core/djangoapps/course_groups/partition_scheme.py
+++ b/openedx/core/djangoapps/course_groups/partition_scheme.py
@@ -22,7 +22,7 @@ class CohortPartitionScheme(object):
# pylint: disable=unused-argument
@classmethod
- def get_group_for_user(cls, course_key, user, user_partition, track_function=None):
+ def get_group_for_user(cls, course_key, user, user_partition, track_function=None, use_cached=True):
"""
Returns the Group from the specified user partition to which the user
is assigned, via their cohort membership and any mappings from cohorts
@@ -48,12 +48,12 @@ class CohortPartitionScheme(object):
return None
return None
- cohort = get_cohort(user, course_key)
+ cohort = get_cohort(user, course_key, use_cached=use_cached)
if cohort is None:
# student doesn't have a cohort
return None
- group_id, partition_id = get_group_info_for_cohort(cohort)
+ group_id, partition_id = get_group_info_for_cohort(cohort, use_cached=use_cached)
if partition_id is None:
# cohort isn't mapped to any partition group.
return None
diff --git a/openedx/core/djangoapps/course_groups/tests/helpers.py b/openedx/core/djangoapps/course_groups/tests/helpers.py
index f14e250aac..eb5e312f5f 100644
--- a/openedx/core/djangoapps/course_groups/tests/helpers.py
+++ b/openedx/core/djangoapps/course_groups/tests/helpers.py
@@ -10,8 +10,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
from xmodule.modulestore.django import modulestore
from xmodule.modulestore import ModuleStoreEnum
-import json
-from ..cohorts import get_course_cohort_settings, set_course_cohort_settings
+from ..cohorts import set_course_cohort_settings
from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings
@@ -126,6 +125,7 @@ def config_course_cohorts_legacy(
pass
+# pylint: disable=dangerous-default-value
def config_course_cohorts(
course,
is_cohorted,
@@ -154,13 +154,14 @@ def config_course_cohorts(
Nothing -- modifies course in place.
"""
def to_id(name):
+ """Convert name to id."""
return topic_name_to_id(course, name)
set_course_cohort_settings(
course.id,
- is_cohorted = is_cohorted,
- cohorted_discussions = [to_id(name) for name in cohorted_discussions],
- always_cohort_inline_discussions = always_cohort_inline_discussions
+ is_cohorted=is_cohorted,
+ cohorted_discussions=[to_id(name) for name in cohorted_discussions],
+ always_cohort_inline_discussions=always_cohort_inline_discussions
)
for cohort_name in auto_cohorts:
diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
index 9985fd5964..d5c80ab64e 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
@@ -2,13 +2,14 @@
Tests for cohorts
"""
# pylint: disable=no-member
+import ddt
+from mock import call, patch
from django.contrib.auth.models import User
from django.db import IntegrityError
from django.http import Http404
from django.test import TestCase
from django.test.utils import override_settings
-from mock import call, patch
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from student.models import CourseEnrollment
@@ -121,6 +122,7 @@ class TestCohortSignals(TestCase):
self.assertFalse(mock_tracker.emit.called)
+@ddt.ddt
class TestCohorts(ModuleStoreTestCase):
"""
Test the cohorts feature
@@ -243,12 +245,33 @@ class TestCohorts(ModuleStoreTestCase):
cohort.id,
"user should be assigned to the correct cohort"
)
+
self.assertEquals(
cohorts.get_cohort(other_user, course.id).id,
cohorts.get_cohort_by_name(course.id, cohorts.DEFAULT_COHORT_NAME).id,
"other_user should be assigned to the default cohort"
)
+ @ddt.data(
+ (True, 2),
+ (False, 6),
+ )
+ @ddt.unpack
+ def test_get_cohort_sql_queries(self, use_cached, num_sql_queries):
+ """
+ Test number of queries by cohorts.get_cohort() with and without caching.
+ """
+ course = modulestore().get_course(self.toy_course_key)
+ config_course_cohorts(course, is_cohorted=True)
+ cohort = CohortFactory(course_id=course.id, name="TestCohort")
+
+ user = UserFactory(username="test", email="a@b.com")
+ cohort.users.add(user)
+
+ with self.assertNumQueries(num_sql_queries):
+ for __ in range(3):
+ cohorts.get_cohort(user, course.id, use_cached=use_cached)
+
def test_get_cohort_with_assign(self):
"""
Make sure cohorts.get_cohort() returns None if no group is already
@@ -473,7 +496,7 @@ class TestCohorts(ModuleStoreTestCase):
config_course_cohorts(
course,
is_cohorted=True,
- discussion_topics= ["General", "Feedback"],
+ discussion_topics=["General", "Feedback"],
cohorted_discussions=["Feedback"]
)
@@ -497,7 +520,7 @@ class TestCohorts(ModuleStoreTestCase):
config_course_cohorts(
course,
is_cohorted=True,
- discussion_topics =["General", "Feedback"],
+ discussion_topics=["General", "Feedback"],
cohorted_discussions=["Feedback", "random_inline"]
)
self.assertTrue(
@@ -741,6 +764,7 @@ class TestCohorts(ModuleStoreTestCase):
)
+@ddt.ddt
class TestCohortsAndPartitionGroups(ModuleStoreTestCase):
"""
Test Cohorts and Partitions Groups.
@@ -803,6 +827,25 @@ class TestCohortsAndPartitionGroups(ModuleStoreTestCase):
(None, None),
)
+ @ddt.data(
+ (True, 1),
+ (False, 3),
+ )
+ @ddt.unpack
+ def test_get_group_info_for_cohort_queries(self, use_cached, num_sql_queries):
+ """
+ Basic test of the partition_group_info accessor function
+ """
+ # create a link for the cohort in the db
+ self._link_cohort_partition_group(
+ self.first_cohort,
+ self.partition_id,
+ self.group1_id
+ )
+ with self.assertNumQueries(num_sql_queries):
+ for __ in range(3):
+ self.assertIsNotNone(cohorts.get_group_info_for_cohort(self.first_cohort, use_cached=use_cached))
+
def test_multiple_cohorts(self):
"""
Test that multiple cohorts can be linked to the same partition group
diff --git a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py
index 599e31fabe..57bb63b26e 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py
@@ -63,6 +63,7 @@ class TestCohortPartitionScheme(ModuleStoreTestCase):
self.course_key,
self.student,
partition or self.user_partition,
+ use_cached=False
),
group
)
From d382f569c8b4afb4f0d8bab5e4f8ea966abb117a Mon Sep 17 00:00:00 2001
From: Usman Khalid <2200617@gmail.com>
Date: Fri, 27 Feb 2015 15:47:19 +0500
Subject: [PATCH 09/13] Refactor django_comment_client.utils.prepare_content()
to query course cohort settings only once.
TNL-1258
---
.../django_comment_client/forum/tests.py | 18 ++++++++++--------
.../django_comment_client/tests/test_utils.py | 1 -
lms/djangoapps/django_comment_client/utils.py | 17 ++++++++++++++---
3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py
index d99657d009..ecd30c2145 100644
--- a/lms/djangoapps/django_comment_client/forum/tests.py
+++ b/lms/djangoapps/django_comment_client/forum/tests.py
@@ -309,18 +309,18 @@ class SingleThreadTestCase(ModuleStoreTestCase):
@patch('requests.request')
class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
"""
- Ensures the number of modulestore queries is deterministic based on the
- number of responses retrieved for a given discussion thread.
+ Ensures the number of modulestore queries and number of sql queries are
+ independent of the number of responses retrieved for a given discussion thread.
"""
MODULESTORE = TEST_DATA_MONGO_MODULESTORE
@ddt.data(
# old mongo with cache: 15
- (ModuleStoreEnum.Type.mongo, 1, 21, 15),
- (ModuleStoreEnum.Type.mongo, 50, 315, 15),
+ (ModuleStoreEnum.Type.mongo, 1, 21, 15, 30),
+ (ModuleStoreEnum.Type.mongo, 50, 315, 15, 30),
# split mongo: 3 queries, regardless of thread response size.
- (ModuleStoreEnum.Type.split, 1, 3, 3),
- (ModuleStoreEnum.Type.split, 50, 3, 3),
+ (ModuleStoreEnum.Type.split, 1, 3, 3, 30),
+ (ModuleStoreEnum.Type.split, 50, 3, 3, 30),
)
@ddt.unpack
def test_number_of_mongo_queries(
@@ -329,6 +329,7 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
num_thread_responses,
num_uncached_mongo_calls,
num_cached_mongo_calls,
+ num_sql_queries,
mock_request
):
with modulestore().default_store(default_store):
@@ -377,8 +378,9 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
for single_thread_cache, expected_calls in cached_calls.items():
single_thread_cache.clear()
with patch("django_comment_client.permissions.CACHE", single_thread_cache):
- with check_mongo_calls(expected_calls):
- call_single_thread()
+ with self.assertNumQueries(num_sql_queries):
+ with check_mongo_calls(expected_calls):
+ call_single_thread()
single_thread_cache.clear()
diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py
index 1f99a364ff..ea6da4523d 100644
--- a/lms/djangoapps/django_comment_client/tests/test_utils.py
+++ b/lms/djangoapps/django_comment_client/tests/test_utils.py
@@ -18,7 +18,6 @@ import django_comment_client.utils as utils
from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings
from student.tests.factories import UserFactory, CourseEnrollmentFactory
-from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py
index 3a91daef15..4c649f8a6a 100644
--- a/lms/djangoapps/django_comment_client/utils.py
+++ b/lms/djangoapps/django_comment_client/utils.py
@@ -405,7 +405,7 @@ def add_courseware_context(content_list, course, user, id_map=None):
content.update({"courseware_url": url, "courseware_title": title})
-def prepare_content(content, course_key, is_staff=False):
+def prepare_content(content, course_key, is_staff=False, **kwargs):
"""
This function is used to pre-process thread and comment models in various
ways before adding them to the HTTP response. This includes fixing empty
@@ -414,6 +414,12 @@ def prepare_content(content, course_key, is_staff=False):
@TODO: not all response pre-processing steps are currently integrated into
this function.
+
+ Arguments:
+ content (dict): A thread or comment.
+ course_key (CourseKey): The course key of the course.
+ is_staff (bool): Whether the user is a staff member.
+ course_is_cohorted (bool): Whether the course is cohorted.
"""
fields = [
'id', 'title', 'body', 'course_id', 'anonymous', 'anonymous_to_peers',
@@ -453,14 +459,19 @@ def prepare_content(content, course_key, is_staff=False):
else:
del endorsement["user_id"]
+ course_is_cohorted = kwargs.get('course_is_cohorted')
+ if course_is_cohorted is None:
+ course_is_cohorted = is_course_cohorted(course_key)
+
for child_content_key in ["children", "endorsed_responses", "non_endorsed_responses"]:
if child_content_key in content:
children = [
- prepare_content(child, course_key, is_staff) for child in content[child_content_key]
+ prepare_content(child, course_key, is_staff, course_is_cohorted=course_is_cohorted)
+ for child in content[child_content_key]
]
content[child_content_key] = children
- if is_course_cohorted(course_key):
+ if course_is_cohorted:
# Augment the specified thread info to include the group name if a group id is present.
if content.get('group_id') is not None:
content['group_name'] = get_cohort_by_id(course_key, content.get('group_id')).name
From eee6250fb24471738292a145f0d8b331bd5990b5 Mon Sep 17 00:00:00 2001
From: Usman Khalid <2200617@gmail.com>
Date: Sun, 1 Mar 2015 21:40:54 +0500
Subject: [PATCH 10/13] Use RequestCache to cache cohorts information.
TNL-1258
---
.../core/djangoapps/course_groups/cohorts.py | 66 +++++++++++--------
.../course_groups/tests/test_cohorts.py | 2 +-
2 files changed, 39 insertions(+), 29 deletions(-)
diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py
index 2221afe574..863dcd0096 100644
--- a/openedx/core/djangoapps/course_groups/cohorts.py
+++ b/openedx/core/djangoapps/course_groups/cohorts.py
@@ -14,7 +14,9 @@ from django.utils.translation import ugettext as _
from courseware import courses
from eventtracking import tracker
+from request_cache.middleware import RequestCache
from student.models import get_user_by_username_or_email
+
from .models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CourseUserGroupPartitionGroup
@@ -177,6 +179,10 @@ def get_cohort(user, course_key, assign=True, use_cached=False):
Given a Django user and a CourseKey, return the user's cohort in that
cohort.
+ The cohort for the user is cached for the duration of a request. Pass
+ use_cached=True to use the cached value instead of fetching from the
+ database.
+
Arguments:
user: a Django User object.
course_key: CourseKey
@@ -190,35 +196,37 @@ def get_cohort(user, course_key, assign=True, use_cached=False):
Raises:
ValueError if the CourseKey doesn't exist.
"""
- # pylint: disable=protected-access
- # We cache the cohort on the user object so that we do not have to repeatedly
- # query the database during a request. If the cached value exists, just return it.
- if use_cached and hasattr(user, '_cohort'):
- return user._cohort
+ request_cache = RequestCache.get_request_cache()
+ cache_key = u"cohorts.get_cohort.{}.{}".format(user.id, course_key)
+
+ if use_cached and cache_key in request_cache.data:
+ return request_cache.data[cache_key]
+
+ request_cache.data.pop(cache_key, None)
# First check whether the course is cohorted (users shouldn't be in a cohort
# in non-cohorted courses, but settings can change after course starts)
- course = courses.get_course(course_key)
- course_cohort_settings = get_course_cohort_settings(course.id)
-
+ course_cohort_settings = get_course_cohort_settings(course_key)
if not course_cohort_settings.is_cohorted:
- user._cohort = None
- return user._cohort
+ return request_cache.data.setdefault(cache_key, None)
+ # If course is cohorted, check if the user already has a cohort.
try:
- user._cohort = CourseUserGroup.objects.get(
+ cohort = CourseUserGroup.objects.get(
course_id=course_key,
group_type=CourseUserGroup.COHORT,
users__id=user.id,
)
- return user._cohort
+ return request_cache.data.setdefault(cache_key, cohort)
except CourseUserGroup.DoesNotExist:
- # Didn't find the group. We'll go on to create one if needed.
+ # Didn't find the group. If we do not want to assign, return here.
if not assign:
# Do not cache the cohort here, because in the next call assign
# may be True, and we will have to assign the user a cohort.
return None
+ # Otherwise assign the user a cohort.
+ course = courses.get_course(course_key)
cohorts = get_course_cohorts(course, assignment_type=CourseCohort.RANDOM)
if cohorts:
cohort = local_random().choice(cohorts)
@@ -231,8 +239,7 @@ def get_cohort(user, course_key, assign=True, use_cached=False):
user.course_groups.add(cohort)
- user._cohort = cohort
- return user._cohort
+ return request_cache.data.setdefault(cache_key, cohort)
def migrate_cohort_settings(course):
@@ -406,23 +413,26 @@ def get_group_info_for_cohort(cohort, use_cached=False):
If the cohort has not been linked to any group/partition, both values in the
tuple will be None.
+
+ The partition group info is cached for the duration of a request. Pass
+ use_cached=True to use the cached value instead of fetching from the
+ database.
"""
- # pylint: disable=protected-access
- # We cache the partition group on the cohort object so that we do not have to repeatedly
- # query the database during a request.
- if not use_cached and hasattr(cohort, '_partition_group'):
- delattr(cohort, '_partition_group')
+ request_cache = RequestCache.get_request_cache()
+ cache_key = u"cohorts.get_group_info_for_cohort.{}".format(cohort.id)
- if not hasattr(cohort, '_partition_group'):
- try:
- cohort._partition_group = CourseUserGroupPartitionGroup.objects.get(course_user_group=cohort)
- except CourseUserGroupPartitionGroup.DoesNotExist:
- cohort._partition_group = None
+ if use_cached and cache_key in request_cache.data:
+ return request_cache.data[cache_key]
- if cohort._partition_group:
- return cohort._partition_group.group_id, cohort._partition_group.partition_id
+ request_cache.data.pop(cache_key, None)
- return None, None
+ try:
+ partition_group = CourseUserGroupPartitionGroup.objects.get(course_user_group=cohort)
+ return request_cache.data.setdefault(cache_key, (partition_group.group_id, partition_group.partition_id))
+ except CourseUserGroupPartitionGroup.DoesNotExist:
+ pass
+
+ return request_cache.data.setdefault(cache_key, (None, None))
def set_assignment_type(user_group, assignment_type):
diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
index d5c80ab64e..a2c98ad802 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py
@@ -176,7 +176,7 @@ class TestCohorts(ModuleStoreTestCase):
self.assertEqual(cohorts.get_cohort_id(user, course.id), cohort.id)
self.assertRaises(
- ValueError,
+ Http404,
lambda: cohorts.get_cohort_id(user, SlashSeparatedCourseKey("course", "does_not", "exist"))
)
From e07f45b5dd050c201cadba336f9710bf6eb1530c Mon Sep 17 00:00:00 2001
From: Usman Khalid <2200617@gmail.com>
Date: Mon, 2 Mar 2015 16:32:28 +0500
Subject: [PATCH 11/13] Removed cohort_config from advanced settings page.
TNL-1258
---
cms/djangoapps/models/settings/course_metadata.py | 1 +
common/test/acceptance/pages/studio/settings_advanced.py | 1 -
lms/djangoapps/django_comment_client/utils.py | 3 +--
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py
index 87d941632e..6f81d9de77 100644
--- a/cms/djangoapps/models/settings/course_metadata.py
+++ b/cms/djangoapps/models/settings/course_metadata.py
@@ -18,6 +18,7 @@ class CourseMetadata(object):
# Should not be used directly. Instead the filtered_list method should
# be used if the field needs to be filtered depending on the feature flag.
FILTERED_LIST = [
+ 'cohort_config',
'xml_attributes',
'start',
'end',
diff --git a/common/test/acceptance/pages/studio/settings_advanced.py b/common/test/acceptance/pages/studio/settings_advanced.py
index 126971bcd7..d556e4b72c 100644
--- a/common/test/acceptance/pages/studio/settings_advanced.py
+++ b/common/test/acceptance/pages/studio/settings_advanced.py
@@ -154,7 +154,6 @@ class AdvancedSettingsPage(CoursePage):
'cert_name_long',
'cert_name_short',
'certificates_display_behavior',
- 'cohort_config',
'course_image',
'cosmetic_display_price',
'advertised_start',
diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py
index 4c649f8a6a..3aa99ecc15 100644
--- a/lms/djangoapps/django_comment_client/utils.py
+++ b/lms/djangoapps/django_comment_client/utils.py
@@ -405,7 +405,7 @@ def add_courseware_context(content_list, course, user, id_map=None):
content.update({"courseware_url": url, "courseware_title": title})
-def prepare_content(content, course_key, is_staff=False, **kwargs):
+def prepare_content(content, course_key, is_staff=False, course_is_cohorted=None):
"""
This function is used to pre-process thread and comment models in various
ways before adding them to the HTTP response. This includes fixing empty
@@ -459,7 +459,6 @@ def prepare_content(content, course_key, is_staff=False, **kwargs):
else:
del endorsement["user_id"]
- course_is_cohorted = kwargs.get('course_is_cohorted')
if course_is_cohorted is None:
course_is_cohorted = is_course_cohorted(course_key)
From e6c75529c09f932abddfa843c49fb7e90fdbaa5f Mon Sep 17 00:00:00 2001
From: muzaffaryousaf
Date: Tue, 17 Feb 2015 16:51:27 +0500
Subject: [PATCH 12/13] Cohort discussion topics via UI in instructor
dashboard.
TNL-1256
---
common/lib/django_test_client_utils.py | 49 ++
.../pages/lms/instructor_dashboard.py | 121 ++++
.../acceptance/tests/discussion/helpers.py | 3 +-
.../discussion/test_cohort_management.py | 309 ++++++++++-
.../tests/test_cohorted_courseware.py | 33 +-
.../django_comment_client/tests/test_utils.py | 98 +++-
lms/djangoapps/django_comment_client/utils.py | 74 ++-
.../instructor/views/instructor_dashboard.py | 2 +-
.../js/groups/models/cohort_discussions.js | 14 +
.../groups/models/course_cohort_settings.js | 3 +-
.../js/groups/views/cohort_discussions.js | 99 ++++
.../views/cohort_discussions_course_wide.js | 83 +++
.../groups/views/cohort_discussions_inline.js | 145 +++++
lms/static/js/groups/views/cohort_editor.js | 3 +-
lms/static/js/groups/views/cohorts.js | 31 +-
.../views/cohorts_dashboard_factory.js} | 18 +-
.../js/spec/groups/views/cohorts_spec.js | 515 +++++++++++++++++-
lms/static/js/spec/main.js | 21 +
lms/static/js/vendor/jquery.qubit.js | 97 ++++
.../sass/course/instructor/_instructor_2.scss | 65 ++-
.../cohort-discussions-category.underscore | 10 +
.../cohort-discussions-course-wide.underscore | 19 +
.../cohort-discussions-inline.underscore | 38 ++
.../cohort-discussions-subcategory.underscore | 9 +
.../cohort-group-header.underscore | 7 +-
.../cohort_management.html | 4 +-
.../instructor_dashboard_2/cohorts.underscore | 10 +
.../instructor_dashboard_2.html | 8 +-
lms/urls.py | 3 +
.../course_groups/tests/test_views.py | 219 +++++++-
.../core/djangoapps/course_groups/views.py | 153 +++++-
31 files changed, 2118 insertions(+), 145 deletions(-)
create mode 100644 common/lib/django_test_client_utils.py
create mode 100644 lms/static/js/groups/models/cohort_discussions.js
create mode 100644 lms/static/js/groups/views/cohort_discussions.js
create mode 100644 lms/static/js/groups/views/cohort_discussions_course_wide.js
create mode 100644 lms/static/js/groups/views/cohort_discussions_inline.js
rename lms/static/js/{factories/cohorts_factory.js => groups/views/cohorts_dashboard_factory.js} (68%)
create mode 100755 lms/static/js/vendor/jquery.qubit.js
create mode 100644 lms/templates/instructor/instructor_dashboard_2/cohort-discussions-category.underscore
create mode 100644 lms/templates/instructor/instructor_dashboard_2/cohort-discussions-course-wide.underscore
create mode 100644 lms/templates/instructor/instructor_dashboard_2/cohort-discussions-inline.underscore
create mode 100644 lms/templates/instructor/instructor_dashboard_2/cohort-discussions-subcategory.underscore
diff --git a/common/lib/django_test_client_utils.py b/common/lib/django_test_client_utils.py
new file mode 100644
index 0000000000..47d57dec65
--- /dev/null
+++ b/common/lib/django_test_client_utils.py
@@ -0,0 +1,49 @@
+"""
+This file includes the monkey-patch for requests' PATCH method, as we are using
+older version of django that does not contains the PATCH method in its test client.
+"""
+from __future__ import unicode_literals
+
+from urlparse import urlparse
+
+from django.test.client import RequestFactory, Client, FakePayload
+
+
+BOUNDARY = 'BoUnDaRyStRiNg'
+MULTIPART_CONTENT = 'multipart/form-data; boundary=%s' % BOUNDARY
+
+
+def request_factory_patch(self, path, data={}, content_type=MULTIPART_CONTENT, **extra):
+ """
+ Construct a PATCH request.
+ """
+ patch_data = self._encode_data(data, content_type)
+
+ parsed = urlparse(path)
+ r = {
+ 'CONTENT_LENGTH': len(patch_data),
+ 'CONTENT_TYPE': content_type,
+ 'PATH_INFO': self._get_path(parsed),
+ 'QUERY_STRING': parsed[4],
+ 'REQUEST_METHOD': 'PATCH',
+ 'wsgi.input': FakePayload(patch_data),
+ }
+ r.update(extra)
+ return self.request(**r)
+
+
+def client_patch(self, path, data={}, content_type=MULTIPART_CONTENT, follow=False, **extra):
+ """
+ Send a resource to the server using PATCH.
+ """
+ response = super(Client, self).patch(path, data=data, content_type=content_type, **extra)
+ if follow:
+ response = self._handle_redirects(response, **extra)
+ return response
+
+
+if not hasattr(RequestFactory, 'patch'):
+ setattr(RequestFactory, 'patch', request_factory_patch)
+
+if not hasattr(Client, 'patch'):
+ setattr(Client, 'patch', client_patch)
diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py
index 9f4b1d3527..e43d58c001 100644
--- a/common/test/acceptance/pages/lms/instructor_dashboard.py
+++ b/common/test/acceptance/pages/lms/instructor_dashboard.py
@@ -105,6 +105,10 @@ class CohortManagementSection(PageObject):
no_content_group_button_css = '.cohort-management-details-association-course input.radio-no'
select_content_group_button_css = '.cohort-management-details-association-course input.radio-yes'
assignment_type_buttons_css = '.cohort-management-assignment-type-settings input'
+ discussion_form_selectors = {
+ 'course-wide': '.cohort-course-wide-discussions-form',
+ 'inline': '.cohort-inline-discussions-form'
+ }
def is_browser_on_page(self):
return self.q(css='.cohort-management').present
@@ -466,6 +470,123 @@ class CohortManagementSection(PageObject):
if state != self.is_cohorted:
self.q(css=self._bounded_selector('.cohorts-state')).first.click()
+ def toggles_showing_of_discussion_topics(self):
+ """
+ Shows the discussion topics.
+ """
+ EmptyPromise(
+ lambda: self.q(css=self._bounded_selector('.toggle-cohort-management-discussions')).results != 0,
+ "Waiting for discussion section to show"
+ ).fulfill()
+
+ # If the discussion topic section has not yet been toggled on, click on the toggle link.
+ self.q(css=self._bounded_selector(".toggle-cohort-management-discussions")).click()
+
+ def discussion_topics_visible(self):
+ """
+ Returns the visibility status of cohort discussion controls.
+ """
+ EmptyPromise(
+ lambda: self.q(css=self._bounded_selector('.cohort-discussions-nav')).results != 0,
+ "Waiting for discussion section to show"
+ ).fulfill()
+
+ return (self.q(css=self._bounded_selector('.cohort-course-wide-discussions-nav')).visible and
+ self.q(css=self._bounded_selector('.cohort-inline-discussions-nav')).visible)
+
+ def select_discussion_topic(self, key):
+ """
+ Selects discussion topic checkbox by clicking on it.
+ """
+ self.q(css=self._bounded_selector(".check-discussion-subcategory-%s" % key)).first.click()
+
+ def select_always_inline_discussion(self):
+ """
+ Selects the always_cohort_inline_discussions radio button.
+ """
+ self.q(css=self._bounded_selector(".check-all-inline-discussions")).first.click()
+
+ def always_inline_discussion_selected(self):
+ """
+ Returns the checked always_cohort_inline_discussions radio button.
+ """
+ return self.q(css=self._bounded_selector(".check-all-inline-discussions:checked"))
+
+ def cohort_some_inline_discussion_selected(self):
+ """
+ Returns the checked some_cohort_inline_discussions radio button.
+ """
+ return self.q(css=self._bounded_selector(".check-cohort-inline-discussions:checked"))
+
+ def select_cohort_some_inline_discussion(self):
+ """
+ Selects the cohort_some_inline_discussions radio button.
+ """
+ self.q(css=self._bounded_selector(".check-cohort-inline-discussions")).first.click()
+
+ def inline_discussion_topics_disabled(self):
+ """
+ Returns the status of inline discussion topics, enabled or disabled.
+ """
+ inline_topics = self.q(css=self._bounded_selector('.check-discussion-subcategory-inline'))
+ return all(topic.get_attribute('disabled') == 'true' for topic in inline_topics)
+
+ def is_save_button_disabled(self, key):
+ """
+ Returns the status for form's save button, enabled or disabled.
+ """
+ save_button_css = '%s %s' % (self.discussion_form_selectors[key], '.action-save')
+ disabled = self.q(css=self._bounded_selector(save_button_css)).attrs('disabled')
+ return disabled[0] == 'true'
+
+ def is_category_selected(self):
+ """
+ Returns the status for category checkboxes.
+ """
+ return self.q(css=self._bounded_selector('.check-discussion-category:checked')).is_present()
+
+ def get_cohorted_topics_count(self, key):
+ """
+ Returns the count for cohorted topics.
+ """
+ cohorted_topics = self.q(css=self._bounded_selector('.check-discussion-subcategory-%s:checked' % key))
+ return len(cohorted_topics.results)
+
+ def save_discussion_topics(self, key):
+ """
+ Saves the discussion topics.
+ """
+ save_button_css = '%s %s' % (self.discussion_form_selectors[key], '.action-save')
+ self.q(css=self._bounded_selector(save_button_css)).first.click()
+
+ def get_cohort_discussions_message(self, key, msg_type="confirmation"):
+ """
+ Returns the message related to modifying discussion topics.
+ """
+ title_css = "%s .message-%s .message-title" % (self.discussion_form_selectors[key], msg_type)
+
+ EmptyPromise(
+ lambda: self.q(css=self._bounded_selector(title_css)),
+ "Waiting for message to appear"
+ ).fulfill()
+
+ message_title = self.q(css=self._bounded_selector(title_css))
+
+ if len(message_title.results) == 0:
+ return ''
+ return message_title.first.text[0]
+
+ def cohort_discussion_heading_is_visible(self, key):
+ """
+ Returns the visibility of discussion topic headings.
+ """
+ form_heading_css = '%s %s' % (self.discussion_form_selectors[key], '.subsection-title')
+ discussion_heading = self.q(css=self._bounded_selector(form_heading_css))
+
+ if len(discussion_heading) == 0:
+ return False
+ return discussion_heading.first.text[0]
+
def cohort_management_controls_visible(self):
"""
Return the visibility status of cohort management controls(cohort selector section etc).
diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py
index a5fba485b0..269148b20d 100644
--- a/common/test/acceptance/tests/discussion/helpers.py
+++ b/common/test/acceptance/tests/discussion/helpers.py
@@ -62,10 +62,9 @@ class CohortTestMixin(object):
"""
url = LMS_BASE_URL + "/courses/" + course_fixture._course_key + '/cohorts/settings' # pylint: disable=protected-access
data = json.dumps({'is_cohorted': False})
- response = course_fixture.session.post(url, data=data, headers=course_fixture.headers)
+ response = course_fixture.session.patch(url, data=data, headers=course_fixture.headers)
self.assertTrue(response.ok, "Failed to disable cohorts")
-
def add_manual_cohort(self, course_fixture, cohort_name):
"""
Adds a cohort by name, returning its ID.
diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py
index 1b247c823a..48f452f0fa 100644
--- a/common/test/acceptance/tests/discussion/test_cohort_management.py
+++ b/common/test/acceptance/tests/discussion/test_cohort_management.py
@@ -11,7 +11,7 @@ from nose.plugins.attrib import attr
from .helpers import CohortTestMixin
from ..helpers import UniqueCourseTest, EventsTestMixin, create_user_partition_json
from xmodule.partitions.partitions import Group
-from ...fixtures.course import CourseFixture
+from ...fixtures.course import CourseFixture, XBlockFixtureDesc
from ...pages.lms.auto_auth import AutoAuthPage
from ...pages.lms.instructor_dashboard import InstructorDashboardPage, DataDownloadPage
from ...pages.studio.settings_advanced import AdvancedSettingsPage
@@ -122,22 +122,6 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin
)
group_settings_page.wait_for_page()
- def test_link_to_studio(self):
- """
- Scenario: a link is present from the cohort configuration in the instructor dashboard
- to the Studio Advanced Settings.
-
- Given I have a course with a cohort defined
- When I view the cohort in the LMS instructor dashboard
- There is a link to take me to the Studio Advanced Settings for the course
- """
- self.cohort_management_page.select_cohort(self.manual_cohort_name)
- self.cohort_management_page.select_edit_settings()
- advanced_settings_page = AdvancedSettingsPage(
- self.browser, self.course_info['org'], self.course_info['number'], self.course_info['run']
- )
- advanced_settings_page.wait_for_page()
-
def test_add_students_to_cohort_success(self):
"""
Scenario: When students are added to a cohort, the appropriate notification is shown.
@@ -635,6 +619,297 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin
self.assertEquals(expected_message, messages[0])
+@attr('shard_3')
+class CohortDiscussionTopicsTest(UniqueCourseTest, CohortTestMixin):
+ """
+ Tests for cohorting the inline and course-wide discussion topics.
+ """
+ def setUp(self):
+ """
+ Set up a discussion topics
+ """
+ super(CohortDiscussionTopicsTest, self).setUp()
+
+ self.discussion_id = "test_discussion_{}".format(uuid.uuid4().hex)
+ self.course_fixture = CourseFixture(**self.course_info).add_children(
+ XBlockFixtureDesc("chapter", "Test Section").add_children(
+ XBlockFixtureDesc("sequential", "Test Subsection").add_children(
+ XBlockFixtureDesc("vertical", "Test Unit").add_children(
+ XBlockFixtureDesc(
+ "discussion",
+ "Test Discussion",
+ metadata={"discussion_id": self.discussion_id}
+ )
+ )
+ )
+ )
+ ).install()
+
+ # create course with single cohort and two content groups (user_partition of type "cohort")
+ self.cohort_name = "OnlyCohort"
+ self.setup_cohort_config(self.course_fixture)
+ self.cohort_id = self.add_manual_cohort(self.course_fixture, self.cohort_name)
+
+ # login as an instructor
+ self.instructor_name = "instructor_user"
+ self.instructor_id = AutoAuthPage(
+ self.browser, username=self.instructor_name, email="instructor_user@example.com",
+ course_id=self.course_id, staff=True
+ ).visit().get_user_id()
+
+ # go to the membership page on the instructor dashboard
+ self.instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id)
+ self.instructor_dashboard_page.visit()
+ self.cohort_management_page = self.instructor_dashboard_page.select_cohort_management()
+ self.cohort_management_page.wait_for_page()
+
+ self.course_wide_key = 'course-wide'
+ self.inline_key = 'inline'
+
+ def cohort_discussion_topics_are_visible(self):
+ """
+ Assert that discussion topics are visible with appropriate content.
+ """
+ self.cohort_management_page.toggles_showing_of_discussion_topics()
+ self.assertTrue(self.cohort_management_page.discussion_topics_visible())
+
+ self.assertEqual(
+ "Course-Wide Discussion Topics",
+ self.cohort_management_page.cohort_discussion_heading_is_visible(self.course_wide_key)
+ )
+ self.assertTrue(self.cohort_management_page.is_save_button_disabled(self.course_wide_key))
+
+ self.assertEqual(
+ "Content-Specific Discussion Topics",
+ self.cohort_management_page.cohort_discussion_heading_is_visible(self.inline_key)
+ )
+ self.assertTrue(self.cohort_management_page.is_save_button_disabled(self.inline_key))
+
+ def save_and_verify_discussion_topics(self, key):
+ """
+ Saves the discussion topics and the verify the changes.
+ """
+ # click on the inline save button.
+ self.cohort_management_page.save_discussion_topics(key)
+
+ # verifies that changes saved successfully.
+ confirmation_message = self.cohort_management_page.get_cohort_discussions_message(key=key)
+ self.assertEqual("Your changes have been saved.", confirmation_message)
+
+ # save button disabled again.
+ self.assertTrue(self.cohort_management_page.is_save_button_disabled(key))
+
+ def reload_page(self):
+ """
+ Refresh the page.
+ """
+ self.browser.refresh()
+ self.cohort_management_page.wait_for_page()
+
+ self.instructor_dashboard_page.select_cohort_management()
+ self.cohort_management_page.wait_for_page()
+
+ self.cohort_discussion_topics_are_visible()
+
+ def verify_discussion_topics_after_reload(self, key, cohorted_topics):
+ """
+ Verifies the changed topics.
+ """
+ self.reload_page()
+ self.assertEqual(self.cohort_management_page.get_cohorted_topics_count(key), cohorted_topics)
+
+ def test_cohort_course_wide_discussion_topic(self):
+ """
+ Scenario: cohort a course-wide discussion topic.
+
+ Given I have a course with a cohort defined,
+ And a course-wide discussion with disabled Save button.
+ When I click on the course-wide discussion topic
+ Then I see the enabled save button
+ When I click on save button
+ Then I see success message
+ When I reload the page
+ Then I see the discussion topic selected
+ """
+ self.cohort_discussion_topics_are_visible()
+
+ cohorted_topics_before = self.cohort_management_page.get_cohorted_topics_count(self.course_wide_key)
+ self.cohort_management_page.select_discussion_topic(self.course_wide_key)
+
+ self.assertFalse(self.cohort_management_page.is_save_button_disabled(self.course_wide_key))
+
+ self.save_and_verify_discussion_topics(key=self.course_wide_key)
+ cohorted_topics_after = self.cohort_management_page.get_cohorted_topics_count(self.course_wide_key)
+
+ self.assertNotEqual(cohorted_topics_before, cohorted_topics_after)
+
+ self.verify_discussion_topics_after_reload(self.course_wide_key, cohorted_topics_after)
+
+ def test_always_cohort_inline_topic_enabled(self):
+ """
+ Scenario: Select the always_cohort_inline_topics radio button
+
+ Given I have a course with a cohort defined,
+ And a inline discussion topic with disabled Save button.
+ When I click on always_cohort_inline_topics
+ Then I see enabled save button
+ And I see disabled inline discussion topics
+ When I reload the page
+ Then I see the option enabled
+ """
+ self.cohort_discussion_topics_are_visible()
+
+ # enable always inline discussion topics.
+ self.cohort_management_page.select_always_inline_discussion()
+
+ self.assertTrue(self.cohort_management_page.inline_discussion_topics_disabled())
+
+ self.reload_page()
+ self.assertIsNotNone(self.cohort_management_page.always_inline_discussion_selected())
+
+ def test_cohort_some_inline_topics_enabled(self):
+ """
+ Scenario: Select the cohort_some_inline_topics radio button
+
+ Given I have a course with a cohort defined,
+ And a inline discussion topic with disabled Save button.
+ When I click on cohort_some_inline_topics
+ Then I see enabled save button
+ And I see enabled inline discussion topics
+ When I reload the page
+ Then I see the option enabled
+ """
+ self.cohort_discussion_topics_are_visible()
+
+ # enable some inline discussion topic radio button.
+ self.cohort_management_page.select_cohort_some_inline_discussion()
+ # I see that save button is enabled
+ self.assertFalse(self.cohort_management_page.is_save_button_disabled(self.inline_key))
+ # I see that inline discussion topics are enabled
+ self.assertFalse(self.cohort_management_page.inline_discussion_topics_disabled())
+
+ self.reload_page()
+ self.assertIsNotNone(self.cohort_management_page.cohort_some_inline_discussion_selected())
+
+ def test_cohort_inline_discussion_topic(self):
+ """
+ Scenario: cohort inline discussion topic.
+
+ Given I have a course with a cohort defined,
+ And a inline discussion topic with disabled Save button.
+ When I click on cohort_some_inline_discussion_topics
+ Then I see enabled saved button
+ And When I click on inline discussion topic
+ And I see enabled save button
+ And When i click save button
+ Then I see success message
+ When I reload the page
+ Then I see the discussion topic selected
+ """
+ self.cohort_discussion_topics_are_visible()
+
+ # select some inline discussion topics radio button.
+ self.cohort_management_page.select_cohort_some_inline_discussion()
+
+ cohorted_topics_before = self.cohort_management_page.get_cohorted_topics_count(self.inline_key)
+ # check the discussion topic.
+ self.cohort_management_page.select_discussion_topic(self.inline_key)
+
+ # Save button enabled.
+ self.assertFalse(self.cohort_management_page.is_save_button_disabled(self.inline_key))
+
+ # verifies that changes saved successfully.
+ self.save_and_verify_discussion_topics(key=self.inline_key)
+
+ cohorted_topics_after = self.cohort_management_page.get_cohorted_topics_count(self.inline_key)
+ self.assertNotEqual(cohorted_topics_before, cohorted_topics_after)
+
+ self.verify_discussion_topics_after_reload(self.inline_key, cohorted_topics_after)
+
+ def test_verify_that_selecting_the_final_child_selects_category(self):
+ """
+ Scenario: Category should be selected on selecting final child.
+
+ Given I have a course with a cohort defined,
+ And a inline discussion with disabled Save button.
+ When I click on child topics
+ Then I see enabled saved button
+ Then I see parent category to be checked.
+ """
+ self.cohort_discussion_topics_are_visible()
+
+ # enable some inline discussion topics.
+ self.cohort_management_page.select_cohort_some_inline_discussion()
+
+ # category should not be selected.
+ self.assertFalse(self.cohort_management_page.is_category_selected())
+
+ # check the discussion topic.
+ self.cohort_management_page.select_discussion_topic(self.inline_key)
+
+ # verify that category is selected.
+ self.assertTrue(self.cohort_management_page.is_category_selected())
+
+ def test_verify_that_deselecting_the_final_child_deselects_category(self):
+ """
+ Scenario: Category should be deselected on deselecting final child.
+
+ Given I have a course with a cohort defined,
+ And a inline discussion with disabled Save button.
+ When I click on final child topics
+ Then I see enabled saved button
+ Then I see parent category to be deselected.
+ """
+ self.cohort_discussion_topics_are_visible()
+
+ # enable some inline discussion topics.
+ self.cohort_management_page.select_cohort_some_inline_discussion()
+
+ # category should not be selected.
+ self.assertFalse(self.cohort_management_page.is_category_selected())
+
+ # check the discussion topic.
+ self.cohort_management_page.select_discussion_topic(self.inline_key)
+
+ # verify that category is selected.
+ self.assertTrue(self.cohort_management_page.is_category_selected())
+
+ # un-check the discussion topic.
+ self.cohort_management_page.select_discussion_topic(self.inline_key)
+
+ # category should not be selected.
+ self.assertFalse(self.cohort_management_page.is_category_selected())
+
+ def test_verify_that_correct_subset_of_category_being_selected_after_save(self):
+ """
+ Scenario: Category should be selected on selecting final child.
+
+ Given I have a course with a cohort defined,
+ And a inline discussion with disabled Save button.
+ When I click on child topics
+ Then I see enabled saved button
+ When I select subset of category
+ And I click on save button
+ Then I see success message with
+ same sub-category being selected
+ """
+ self.cohort_discussion_topics_are_visible()
+
+ # enable some inline discussion topics.
+ self.cohort_management_page.select_cohort_some_inline_discussion()
+
+ # category should not be selected.
+ self.assertFalse(self.cohort_management_page.is_category_selected())
+
+ cohorted_topics_after = self.cohort_management_page.get_cohorted_topics_count(self.inline_key)
+
+ # verifies that changes saved successfully.
+ self.save_and_verify_discussion_topics(key=self.inline_key)
+
+ # verify changes after reload.
+ self.verify_discussion_topics_after_reload(self.inline_key, cohorted_topics_after)
+
+
@attr('shard_3')
class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin):
"""
diff --git a/common/test/acceptance/tests/test_cohorted_courseware.py b/common/test/acceptance/tests/test_cohorted_courseware.py
index cd1df402b1..11097684d5 100644
--- a/common/test/acceptance/tests/test_cohorted_courseware.py
+++ b/common/test/acceptance/tests/test_cohorted_courseware.py
@@ -2,18 +2,17 @@
End-to-end test for cohorted courseware. This uses both Studio and LMS.
"""
-from nose.plugins.attrib import attr
import json
+from nose.plugins.attrib import attr
from studio.base_studio_test import ContainerBase
from ..pages.studio.settings_group_configurations import GroupConfigurationsPage
-from ..pages.studio.settings_advanced import AdvancedSettingsPage
from ..pages.studio.auto_auth import AutoAuthPage as StudioAutoAuthPage
from ..fixtures.course import XBlockFixtureDesc
+from ..fixtures import LMS_BASE_URL
from ..pages.studio.component_editor import ComponentVisibilityEditorView
from ..pages.lms.instructor_dashboard import InstructorDashboardPage
-from ..pages.lms.course_nav import CourseNavPage
from ..pages.lms.courseware import CoursewarePage
from ..pages.lms.auto_auth import AutoAuthPage as LmsAutoAuthPage
from ..tests.lms.test_lms_user_preview import verify_expected_problem_visibility
@@ -80,28 +79,14 @@ class EndToEndCohortedCoursewareTest(ContainerBase):
)
)
- def enable_cohorts_in_course(self):
+ def enable_cohorting(self, course_fixture):
"""
- This turns on cohorts for the course. Currently this is still done through Advanced
- Settings. Eventually it will be done in the LMS Instructor Dashboard.
+ Enables cohorting for the current course.
"""
- advanced_settings = AdvancedSettingsPage(
- self.browser,
- self.course_info['org'],
- self.course_info['number'],
- self.course_info['run']
- )
-
- advanced_settings.visit()
- cohort_config = '{"cohorted": true}'
- advanced_settings.set('Cohort Configuration', cohort_config)
- advanced_settings.refresh_and_wait_for_load()
-
- self.assertEquals(
- json.loads(cohort_config),
- json.loads(advanced_settings.get('Cohort Configuration')),
- 'Wrong input for Cohort Configuration'
- )
+ url = LMS_BASE_URL + "/courses/" + course_fixture._course_key + '/cohorts/settings' # pylint: disable=protected-access
+ data = json.dumps({'is_cohorted': True})
+ response = course_fixture.session.patch(url, data=data, headers=course_fixture.headers)
+ self.assertTrue(response.ok, "Failed to enable cohorts")
def create_content_groups(self):
"""
@@ -219,7 +204,7 @@ class EndToEndCohortedCoursewareTest(ContainerBase):
And the student in Cohort B can see all the problems except the one linked to Content Group A
And the student in the default cohort can ony see the problem that is unlinked to any Content Group
"""
- self.enable_cohorts_in_course()
+ self.enable_cohorting(self.course_fixture)
self.create_content_groups()
self.link_problems_to_content_groups_and_publish()
self.create_cohorts_and_assign_students()
diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py
index ea6da4523d..b67a0ad2d7 100644
--- a/lms/djangoapps/django_comment_client/tests/test_utils.py
+++ b/lms/djangoapps/django_comment_client/tests/test_utils.py
@@ -1,8 +1,9 @@
# -*- coding: utf-8 -*-
-from datetime import datetime
+import datetime
import json
import mock
from pytz import UTC
+from django.utils.timezone import UTC as django_utc
from django_comment_client.tests.factories import RoleFactory
from django_comment_client.tests.unicode import UnicodeTestMixin
@@ -179,7 +180,7 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
# This test needs to use a course that has already started --
# discussion topics only show up if the course has already started,
# and the default start date for courses is Jan 1, 2030.
- start=datetime(2012, 2, 3, tzinfo=UTC)
+ start=datetime.datetime(2012, 2, 3, tzinfo=UTC)
)
# Courses get a default discussion topic on creation, so remove it
self.course.discussion_topics = {}
@@ -198,6 +199,15 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
**kwargs
)
+ def assert_category_map_equals(self, expected, cohorted_if_in_list=False, exclude_unstarted=True):
+ """
+ Asserts the expected map with the map returned by get_discussion_category_map method.
+ """
+ self.assertEqual(
+ utils.get_discussion_category_map(self.course, cohorted_if_in_list, exclude_unstarted),
+ expected
+ )
+
def test_empty(self):
self.assert_category_map_equals({"entries": {}, "subcategories": {}, "children": []})
@@ -276,6 +286,85 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
}
)
+ def test_inline_with_always_cohort_inline_discussion_flag(self):
+ self.create_discussion("Chapter", "Discussion")
+ set_course_cohort_settings(course_key=self.course.id, is_cohorted=True)
+
+ self.assert_category_map_equals(
+ {
+ "entries": {},
+ "subcategories": {
+ "Chapter": {
+ "entries": {
+ "Discussion": {
+ "id": "discussion1",
+ "sort_key": None,
+ "is_cohorted": True,
+ }
+ },
+ "subcategories": {},
+ "children": ["Discussion"]
+ }
+ },
+ "children": ["Chapter"]
+ }
+ )
+
+ def test_inline_without_always_cohort_inline_discussion_flag(self):
+ self.create_discussion("Chapter", "Discussion")
+ set_course_cohort_settings(course_key=self.course.id, is_cohorted=True, always_cohort_inline_discussions=False)
+
+ self.assert_category_map_equals(
+ {
+ "entries": {},
+ "subcategories": {
+ "Chapter": {
+ "entries": {
+ "Discussion": {
+ "id": "discussion1",
+ "sort_key": None,
+ "is_cohorted": False,
+ }
+ },
+ "subcategories": {},
+ "children": ["Discussion"]
+ }
+ },
+ "children": ["Chapter"]
+ },
+ cohorted_if_in_list=True
+ )
+
+ def test_get_unstarted_discussion_modules(self):
+ later = datetime.datetime(datetime.MAXYEAR, 1, 1, tzinfo=django_utc())
+
+ self.create_discussion("Chapter 1", "Discussion 1", start=later)
+
+ self.assert_category_map_equals(
+ {
+ "entries": {},
+ "subcategories": {
+ "Chapter 1": {
+ "entries": {
+ "Discussion 1": {
+ "id": "discussion1",
+ "sort_key": None,
+ "is_cohorted": False,
+ "start_date": later
+ }
+ },
+ "subcategories": {},
+ "children": ["Discussion 1"],
+ "start_date": later,
+ "sort_key": "Chapter 1"
+ }
+ },
+ "children": ["Chapter 1"]
+ },
+ cohorted_if_in_list=True,
+ exclude_unstarted=False
+ )
+
def test_tree(self):
self.create_discussion("Chapter 1", "Discussion 1")
self.create_discussion("Chapter 1", "Discussion 2")
@@ -401,8 +490,8 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
self.assertEqual(set(subsection1["entries"].keys()), subsection1_discussions)
def test_start_date_filter(self):
- now = datetime.now()
- later = datetime.max
+ now = datetime.datetime.now()
+ later = datetime.datetime.max
self.create_discussion("Chapter 1", "Discussion 1", start=now)
self.create_discussion("Chapter 1", "Discussion 2 обсуждение", start=later)
self.create_discussion("Chapter 2", "Discussion", start=now)
@@ -440,6 +529,7 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
"children": ["Chapter 1", "Chapter 2"]
}
)
+ self.maxDiff = None
def test_sort_inline_explicit(self):
self.create_discussion("Chapter", "Discussion 1", sort_key="D")
diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py
index 3aa99ecc15..d502a4c128 100644
--- a/lms/djangoapps/django_comment_client/utils.py
+++ b/lms/djangoapps/django_comment_client/utils.py
@@ -61,8 +61,7 @@ def has_forum_access(uname, course_id, rolename):
return role.users.filter(username=uname).exists()
-# pylint: disable=invalid-name
-def get_accessible_discussion_modules(course, user):
+def get_accessible_discussion_modules(course, user, include_all=False): # pylint: disable=invalid-name
"""
Return a list of all valid discussion modules in this course that
are accessible to the given user.
@@ -71,14 +70,14 @@ def get_accessible_discussion_modules(course, user):
def has_required_keys(module):
for key in ('discussion_id', 'discussion_category', 'discussion_target'):
- if getattr(module, key) is None:
+ if getattr(module, key, None) is None:
log.warning("Required key '%s' not in discussion %s, leaving out of category map" % (key, module.location))
return False
return True
return [
module for module in all_modules
- if has_required_keys(module) and has_access(user, 'load', module, course.id)
+ if has_required_keys(module) and (include_all or has_access(user, 'load', module, course.id))
]
@@ -146,10 +145,50 @@ def _sort_map_entries(category_map, sort_alpha):
category_map["children"] = [x[0] for x in sorted(things, key=lambda x: x[1]["sort_key"])]
-def get_discussion_category_map(course, user):
+def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude_unstarted=True):
"""
Transform the list of this course's discussion modules into a recursive dictionary structure. This is used
to render the discussion category map in the discussion tab sidebar for a given user.
+
+ Args:
+ course: Course for which to get the ids.
+ user: User to check for access.
+ cohorted_if_in_list (bool): If True, inline topics are marked is_cohorted only if they are
+ in course_cohort_settings.discussion_topics.
+
+ Example:
+ >>> example = {
+ >>> "entries": {
+ >>> "General": {
+ >>> "sort_key": "General",
+ >>> "is_cohorted": True,
+ >>> "id": "i4x-edx-eiorguegnru-course-foobarbaz"
+ >>> }
+ >>> },
+ >>> "children": ["General", "Getting Started"],
+ >>> "subcategories": {
+ >>> "Getting Started": {
+ >>> "subcategories": {},
+ >>> "children": [
+ >>> "Working with Videos",
+ >>> "Videos on edX"
+ >>> ],
+ >>> "entries": {
+ >>> "Working with Videos": {
+ >>> "sort_key": None,
+ >>> "is_cohorted": False,
+ >>> "id": "d9f970a42067413cbb633f81cfb12604"
+ >>> },
+ >>> "Videos on edX": {
+ >>> "sort_key": None,
+ >>> "is_cohorted": False,
+ >>> "id": "98d8feb5971041a085512ae22b398613"
+ >>> }
+ >>> }
+ >>> }
+ >>> }
+ >>> }
+
"""
unexpanded_category_map = defaultdict(list)
@@ -162,7 +201,7 @@ def get_discussion_category_map(course, user):
title = module.discussion_target
sort_key = module.sort_key
category = " / ".join([x.strip() for x in module.discussion_category.split("/")])
- #Handle case where module.start is None
+ # Handle case where module.start is None
entry_start_date = module.start if module.start else datetime.max.replace(tzinfo=pytz.UTC)
unexpanded_category_map[category].append({"title": title, "id": id, "sort_key": sort_key, "start_date": entry_start_date})
@@ -198,8 +237,17 @@ def get_discussion_category_map(course, user):
if node[level]["start_date"] > category_start_date:
node[level]["start_date"] = category_start_date
+ always_cohort_inline_discussions = ( # pylint: disable=invalid-name
+ not cohorted_if_in_list and course_cohort_settings.always_cohort_inline_discussions
+ )
dupe_counters = defaultdict(lambda: 0) # counts the number of times we see each title
for entry in entries:
+ is_entry_cohorted = (
+ course_cohort_settings.is_cohorted and (
+ always_cohort_inline_discussions or entry["id"] in course_cohort_settings.cohorted_discussions
+ )
+ )
+
title = entry["title"]
if node[level]["entries"][title]:
# If we've already seen this title, append an incrementing number to disambiguate
@@ -209,7 +257,7 @@ def get_discussion_category_map(course, user):
node[level]["entries"][title] = {"id": entry["id"],
"sort_key": entry["sort_key"],
"start_date": entry["start_date"],
- "is_cohorted": course_cohort_settings.is_cohorted}
+ "is_cohorted": is_entry_cohorted}
# TODO. BUG! : course location is not unique across multiple course runs!
# (I think Kevin already noticed this) Need to send course_id with requests, store it
@@ -225,16 +273,22 @@ def get_discussion_category_map(course, user):
_sort_map_entries(category_map, course.discussion_sort_alpha)
- return _filter_unstarted_categories(category_map)
+ return _filter_unstarted_categories(category_map) if exclude_unstarted else category_map
-def get_discussion_categories_ids(course, user):
+def get_discussion_categories_ids(course, user, include_all=False):
"""
Returns a list of available ids of categories for the course that
are accessible to the given user.
+
+ Args:
+ course: Course for which to get the ids.
+ user: User to check for access.
+ include_all (bool): If True, return all ids. Used by configuration views.
+
"""
accessible_discussion_ids = [
- module.discussion_id for module in get_accessible_discussion_modules(course, user)
+ module.discussion_id for module in get_accessible_discussion_modules(course, user, include_all=include_all)
]
return course.top_level_discussion_topic_ids + accessible_discussion_ids
diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py
index a7ed90df09..5b44f8ace2 100644
--- a/lms/djangoapps/instructor/views/instructor_dashboard.py
+++ b/lms/djangoapps/instructor/views/instructor_dashboard.py
@@ -345,8 +345,8 @@ def _section_cohort_management(course, access):
kwargs={'course_key_string': unicode(course_key)}
),
'cohorts_url': reverse('cohorts', kwargs={'course_key_string': unicode(course_key)}),
- 'advanced_settings_url': get_studio_url(course, 'settings/advanced'),
'upload_cohorts_csv_url': reverse('add_users_to_cohorts', kwargs={'course_id': unicode(course_key)}),
+ 'discussion_topics_url': reverse('cohort_discussion_topics', kwargs={'course_key_string': unicode(course_key)}),
}
return section_data
diff --git a/lms/static/js/groups/models/cohort_discussions.js b/lms/static/js/groups/models/cohort_discussions.js
new file mode 100644
index 0000000000..90af5a9207
--- /dev/null
+++ b/lms/static/js/groups/models/cohort_discussions.js
@@ -0,0 +1,14 @@
+var edx = edx || {};
+
+(function(Backbone) {
+ 'use strict';
+
+ edx.groups = edx.groups || {};
+
+ edx.groups.DiscussionTopicsSettingsModel = Backbone.Model.extend({
+ defaults: {
+ course_wide_discussions: {},
+ inline_discussions: {}
+ }
+ });
+}).call(this, Backbone);
diff --git a/lms/static/js/groups/models/course_cohort_settings.js b/lms/static/js/groups/models/course_cohort_settings.js
index a87eeceec7..74be792614 100644
--- a/lms/static/js/groups/models/course_cohort_settings.js
+++ b/lms/static/js/groups/models/course_cohort_settings.js
@@ -9,7 +9,8 @@ var edx = edx || {};
idAttribute: 'id',
defaults: {
is_cohorted: false,
- cohorted_discussions: [],
+ cohorted_inline_discussions: [],
+ cohorted_course_wide_discussions:[],
always_cohort_inline_discussions: true
}
});
diff --git a/lms/static/js/groups/views/cohort_discussions.js b/lms/static/js/groups/views/cohort_discussions.js
new file mode 100644
index 0000000000..b20c37d1f8
--- /dev/null
+++ b/lms/static/js/groups/views/cohort_discussions.js
@@ -0,0 +1,99 @@
+var edx = edx || {};
+
+(function ($, _, Backbone, gettext, interpolate_text, NotificationModel, NotificationView) {
+ 'use strict';
+
+ edx.groups = edx.groups || {};
+
+ edx.groups.CohortDiscussionConfigurationView = Backbone.View.extend({
+
+ /**
+ * Add/Remove the disabled attribute on given element.
+ * @param {object} $element - The element to disable/enable.
+ * @param {bool} disable - The flag to add/remove 'disabled' attribute.
+ */
+ setDisabled: function($element, disable) {
+ $element.prop('disabled', disable ? 'disabled' : false);
+ },
+
+ /**
+ * Returns the cohorted discussions list.
+ * @param {string} selector - To select the discussion elements whose ids to return.
+ * @returns {Array} - Cohorted discussions.
+ */
+ getCohortedDiscussions: function(selector) {
+ var self=this,
+ cohortedDiscussions = [];
+
+ _.each(self.$(selector), function (topic) {
+ cohortedDiscussions.push($(topic).data('id'))
+ });
+ return cohortedDiscussions;
+ },
+
+ /**
+ * Save the cohortSettings' changed attributes to the server via PATCH method.
+ * It shows the error message(s) if any.
+ * @param {object} $element - Messages would be shown before this element.
+ * @param {object} fieldData - Data to update on the server.
+ */
+ saveForm: function ($element, fieldData) {
+ var self = this,
+ cohortSettingsModel = this.cohortSettings,
+ saveOperation = $.Deferred(),
+ showErrorMessage;
+
+ showErrorMessage = function (message, $element) {
+ self.showMessage(message, $element, 'error');
+ };
+ this.removeNotification();
+
+ cohortSettingsModel.save(
+ fieldData, {patch: true, wait: true}
+ ).done(function () {
+ saveOperation.resolve();
+ }).fail(function (result) {
+ var errorMessage = null;
+ try {
+ var jsonResponse = JSON.parse(result.responseText);
+ errorMessage = jsonResponse.error;
+ } catch (e) {
+ // Ignore the exception and show the default error message instead.
+ }
+ if (!errorMessage) {
+ errorMessage = gettext("We've encountered an error. Refresh your browser and then try again.");
+ }
+ showErrorMessage(errorMessage, $element);
+ saveOperation.reject();
+ });
+ return saveOperation.promise();
+ },
+
+ /**
+ * Shows the notification messages before given element using the NotificationModel.
+ * @param {string} message - Text message to show.
+ * @param {object} $element - Message would be shown before this element.
+ * @param {string} type - Type of message to show e.g. confirmation or error.
+ */
+ showMessage: function (message, $element, type) {
+ var model = new NotificationModel({type: type || 'confirmation', title: message});
+ this.removeNotification();
+ this.notification = new NotificationView({
+ model: model
+ });
+ $element.before(this.notification.$el);
+ this.notification.render();
+ },
+
+ /**
+ *Removes the notification messages.
+ */
+ removeNotification: function () {
+ if (this.notification) {
+ this.notification.remove();
+ }
+ }
+
+ });
+}).call(this, $, _, Backbone, gettext, interpolate_text, NotificationModel, NotificationView
+);
diff --git a/lms/static/js/groups/views/cohort_discussions_course_wide.js b/lms/static/js/groups/views/cohort_discussions_course_wide.js
new file mode 100644
index 0000000000..97a06fea84
--- /dev/null
+++ b/lms/static/js/groups/views/cohort_discussions_course_wide.js
@@ -0,0 +1,83 @@
+var edx = edx || {};
+
+(function ($, _, Backbone, gettext, interpolate_text, CohortDiscussionConfigurationView) {
+ 'use strict';
+
+ edx.groups = edx.groups || {};
+
+ edx.groups.CourseWideDiscussionsView = CohortDiscussionConfigurationView.extend({
+ events: {
+ 'change .check-discussion-subcategory-course-wide': 'discussionCategoryStateChanged',
+ 'click .cohort-course-wide-discussions-form .action-save': 'saveCourseWideDiscussionsForm'
+ },
+
+ initialize: function (options) {
+ this.template = _.template($('#cohort-discussions-course-wide-tpl').text());
+ this.cohortSettings = options.cohortSettings;
+ },
+
+ render: function () {
+ this.$('.cohort-course-wide-discussions-nav').html(this.template({
+ courseWideTopics: this.getCourseWideDiscussionsHtml(
+ this.model.get('course_wide_discussions')
+ )
+ }));
+ this.setDisabled(this.$('.cohort-course-wide-discussions-form .action-save'), true);
+ },
+
+ /**
+ * Returns the html list for course-wide discussion topics.
+ * @param {object} courseWideDiscussions - course-wide discussions object from server.
+ * @returns {Array} - HTML list for course-wide discussion topics.
+ */
+ getCourseWideDiscussionsHtml: function (courseWideDiscussions) {
+ var subCategoryTemplate = _.template($('#cohort-discussions-subcategory-tpl').html()),
+ entries = courseWideDiscussions.entries,
+ children = courseWideDiscussions.children;
+
+ return _.map(children, function (name) {
+ var entry = entries[name];
+ return subCategoryTemplate({
+ name: name,
+ id: entry.id,
+ is_cohorted: entry.is_cohorted,
+ type: 'course-wide'
+ });
+ }).join('');
+ },
+
+ /**
+ * Enables the save button for course-wide discussions.
+ */
+ discussionCategoryStateChanged: function(event) {
+ event.preventDefault();
+ this.setDisabled(this.$('.cohort-course-wide-discussions-form .action-save'), false);
+ },
+
+ /**
+ * Sends the cohorted_course_wide_discussions to the server and renders the view.
+ */
+ saveCourseWideDiscussionsForm: function (event) {
+ event.preventDefault();
+
+ var self = this,
+ courseWideCohortedDiscussions = self.getCohortedDiscussions(
+ '.check-discussion-subcategory-course-wide:checked'
+ ),
+ fieldData = { cohorted_course_wide_discussions: courseWideCohortedDiscussions };
+
+ self.saveForm(self.$('.course-wide-discussion-topics'),fieldData)
+ .done(function () {
+ self.model.fetch()
+ .done(function () {
+ self.render();
+ self.showMessage(gettext('Your changes have been saved.'), self.$('.course-wide-discussion-topics'));
+ }).fail(function() {
+ var errorMessage = gettext("We've encountered an error. Refresh your browser and then try again.");
+ self.showMessage(errorMessage, self.$('.course-wide-discussion-topics'), 'error')
+ });
+ });
+ }
+
+ });
+}).call(this, $, _, Backbone, gettext, interpolate_text, edx.groups.CohortDiscussionConfigurationView);
diff --git a/lms/static/js/groups/views/cohort_discussions_inline.js b/lms/static/js/groups/views/cohort_discussions_inline.js
new file mode 100644
index 0000000000..036b2dafc6
--- /dev/null
+++ b/lms/static/js/groups/views/cohort_discussions_inline.js
@@ -0,0 +1,145 @@
+var edx = edx || {};
+
+(function ($, _, Backbone, gettext, interpolate_text, CohortDiscussionConfigurationView) {
+ 'use strict';
+
+ edx.groups = edx.groups || {};
+
+ edx.groups.InlineDiscussionsView = CohortDiscussionConfigurationView.extend({
+ events: {
+ 'change .check-discussion-category': 'setSaveButton',
+ 'change .check-discussion-subcategory-inline': 'setSaveButton',
+ 'click .cohort-inline-discussions-form .action-save': 'saveInlineDiscussionsForm',
+ 'change .check-all-inline-discussions': 'setAllInlineDiscussions',
+ 'change .check-cohort-inline-discussions': 'setSomeInlineDiscussions'
+ },
+
+ initialize: function (options) {
+ this.template = _.template($('#cohort-discussions-inline-tpl').text());
+ this.cohortSettings = options.cohortSettings;
+ },
+
+ render: function () {
+ var alwaysCohortInlineDiscussions = this.cohortSettings.get('always_cohort_inline_discussions');
+
+ this.$('.cohort-inline-discussions-nav').html(this.template({
+ inlineDiscussionTopics: this.getInlineDiscussionsHtml(this.model.get('inline_discussions')),
+ alwaysCohortInlineDiscussions:alwaysCohortInlineDiscussions
+ }));
+
+ // Provides the semantics for a nested list of tri-state checkboxes.
+ // When attached to a jQuery element it listens for change events to
+ // input[type=checkbox] elements, and updates the checked and indeterminate
+ // based on the checked values of any checkboxes in child elements of the DOM.
+ this.$('ul.inline-topics').qubit();
+
+ this.setElementsEnabled(alwaysCohortInlineDiscussions, true);
+ },
+
+ /**
+ * Generate html list for inline discussion topics.
+ * @params {object} inlineDiscussions - inline discussions object from server.
+ * @returns {Array} - HTML for inline discussion topics.
+ */
+ getInlineDiscussionsHtml: function (inlineDiscussions) {
+ var categoryTemplate = _.template($('#cohort-discussions-category-tpl').html()),
+ entryTemplate = _.template($('#cohort-discussions-subcategory-tpl').html()),
+ isCategoryCohorted = false,
+ children = inlineDiscussions.children,
+ entries = inlineDiscussions.entries,
+ subcategories = inlineDiscussions.subcategories;
+
+ return _.map(children, function (name) {
+ var html = '', entry;
+ if (entries && _.has(entries, name)) {
+ entry = entries[name];
+ html = entryTemplate({
+ name: name,
+ id: entry.id,
+ is_cohorted: entry.is_cohorted,
+ type: 'inline'
+ });
+ } else { // subcategory
+ html = categoryTemplate({
+ name: name,
+ entries: this.getInlineDiscussionsHtml(subcategories[name]),
+ isCategoryCohorted: isCategoryCohorted
+ });
+ }
+ return html;
+ }, this).join('');
+ },
+
+ /**
+ * Enable/Disable the inline discussion elements.
+ *
+ * Disables the category and sub-category checkboxes.
+ * Enables the save button.
+ */
+ setAllInlineDiscussions: function(event) {
+ event.preventDefault();
+ this.setElementsEnabled(($(event.currentTarget).prop('checked')), false);
+ },
+
+ /**
+ * Enables the inline discussion elements.
+ *
+ * Enables the category and sub-category checkboxes.
+ * Enables the save button.
+ */
+ setSomeInlineDiscussions: function(event) {
+ event.preventDefault();
+ this.setElementsEnabled(!($(event.currentTarget).prop('checked')), false);
+ },
+
+ /**
+ * Enable/Disable the inline discussion elements.
+ *
+ * Enable/Disable the category and sub-category checkboxes.
+ * Enable/Disable the save button.
+ * @param {bool} enable_checkboxes - The flag to enable/disable the checkboxes.
+ * @param {bool} enable_save_button - The flag to enable/disable the save button.
+ */
+ setElementsEnabled: function(enable_checkboxes, enable_save_button) {
+ this.setDisabled(this.$('.check-discussion-category'), enable_checkboxes);
+ this.setDisabled(this.$('.check-discussion-subcategory-inline'), enable_checkboxes);
+ this.setDisabled(this.$('.cohort-inline-discussions-form .action-save'), enable_save_button);
+ },
+
+ /**
+ * Enables the save button for inline discussions.
+ */
+ setSaveButton: function(event) {
+ this.setDisabled(this.$('.cohort-inline-discussions-form .action-save'), false);
+ },
+
+ /**
+ * Sends the cohorted_inline_discussions to the server and renders the view.
+ */
+ saveInlineDiscussionsForm: function (event) {
+ event.preventDefault();
+
+ var self = this,
+ cohortedInlineDiscussions = self.getCohortedDiscussions(
+ '.check-discussion-subcategory-inline:checked'
+ ),
+ fieldData= {
+ cohorted_inline_discussions: cohortedInlineDiscussions,
+ always_cohort_inline_discussions: self.$('.check-all-inline-discussions').prop('checked')
+ };
+
+ self.saveForm(self.$('.inline-discussion-topics'), fieldData)
+ .done(function () {
+ self.model.fetch()
+ .done(function () {
+ self.render();
+ self.showMessage(gettext('Your changes have been saved.'), self.$('.inline-discussion-topics'));
+ }).fail(function() {
+ var errorMessage = gettext("We've encountered an error. Refresh your browser and then try again.");
+ self.showMessage(errorMessage, self.$('.inline-discussion-topics'), 'error')
+ });
+ });
+ }
+
+ });
+}).call(this, $, _, Backbone, gettext, interpolate_text, edx.groups.CohortDiscussionConfigurationView);
diff --git a/lms/static/js/groups/views/cohort_editor.js b/lms/static/js/groups/views/cohort_editor.js
index 448b398546..8795c9baf5 100644
--- a/lms/static/js/groups/views/cohort_editor.js
+++ b/lms/static/js/groups/views/cohort_editor.js
@@ -44,8 +44,7 @@ var edx = edx || {};
renderGroupHeader: function() {
this.$('.cohort-management-group-header').html(this.groupHeaderTemplate({
- cohort: this.model,
- studioAdvancedSettingsUrl: this.context.studioAdvancedSettingsUrl
+ cohort: this.model
}));
},
diff --git a/lms/static/js/groups/views/cohorts.js b/lms/static/js/groups/views/cohorts.js
index a8184bece6..0d5714d901 100644
--- a/lms/static/js/groups/views/cohorts.js
+++ b/lms/static/js/groups/views/cohorts.js
@@ -1,7 +1,8 @@
var edx = edx || {};
(function($, _, Backbone, gettext, interpolate_text, CohortModel, CohortEditorView, CohortFormView,
- CourseCohortSettingsNotificationView, NotificationModel, NotificationView, FileUploaderView) {
+ CourseCohortSettingsNotificationView, NotificationModel, NotificationView, FileUploaderView,
+ InlineDiscussionsView, CourseWideDiscussionsView) {
'use strict';
var hiddenClass = 'is-hidden',
@@ -17,7 +18,8 @@ var edx = edx || {};
'click .cohort-management-add-form .action-save': 'saveAddCohortForm',
'click .cohort-management-add-form .action-cancel': 'cancelAddCohortForm',
'click .link-cross-reference': 'showSection',
- 'click .toggle-cohort-management-secondary': 'showCsvUpload'
+ 'click .toggle-cohort-management-secondary': 'showCsvUpload',
+ 'click .toggle-cohort-management-discussions': 'showDiscussionTopics'
},
initialize: function(options) {
@@ -120,7 +122,7 @@ var edx = edx || {};
fieldData = {is_cohorted: this.getCohortsEnabled()};
cohortSettings = this.cohortSettings;
cohortSettings.save(
- fieldData, {wait: true}
+ fieldData, {patch: true, wait: true}
).done(function() {
self.render();
self.renderCourseCohortSettingsNotificationView();
@@ -277,6 +279,27 @@ var edx = edx || {};
this.$('#file-upload-form-file').focus();
}
},
+ showDiscussionTopics: function(event) {
+ event.preventDefault();
+
+ $(event.currentTarget).addClass(hiddenClass);
+ var cohortDiscussionsElement = this.$('.cohort-discussions-nav').removeClass(hiddenClass);
+
+ if (!this.CourseWideDiscussionsView) {
+ this.CourseWideDiscussionsView = new CourseWideDiscussionsView({
+ el: cohortDiscussionsElement,
+ model: this.context.discussionTopicsSettingsModel,
+ cohortSettings: this.cohortSettings
+ }).render();
+ }
+ if(!this.InlineDiscussionsView) {
+ this.InlineDiscussionsView = new InlineDiscussionsView({
+ el: cohortDiscussionsElement,
+ model: this.context.discussionTopicsSettingsModel,
+ cohortSettings: this.cohortSettings
+ }).render();
+ }
+ },
getSectionCss: function (section) {
return ".instructor-nav .nav-item a[data-section='" + section + "']";
@@ -284,4 +307,4 @@ var edx = edx || {};
});
}).call(this, $, _, Backbone, gettext, interpolate_text, edx.groups.CohortModel, edx.groups.CohortEditorView,
edx.groups.CohortFormView, edx.groups.CourseCohortSettingsNotificationView, NotificationModel, NotificationView,
- FileUploaderView);
+ FileUploaderView, edx.groups.InlineDiscussionsView, edx.groups.CourseWideDiscussionsView);
diff --git a/lms/static/js/factories/cohorts_factory.js b/lms/static/js/groups/views/cohorts_dashboard_factory.js
similarity index 68%
rename from lms/static/js/factories/cohorts_factory.js
rename to lms/static/js/groups/views/cohorts_dashboard_factory.js
index 3d04e1fd91..3ddf229220 100644
--- a/lms/static/js/factories/cohorts_factory.js
+++ b/lms/static/js/groups/views/cohorts_dashboard_factory.js
@@ -1,33 +1,39 @@
;(function (define, undefined) {
'use strict';
- define(['jquery', 'js/groups/views/cohorts', 'js/groups/collections/cohort', 'js/groups/models/course_cohort_settings'],
+ define(['jquery', 'js/groups/views/cohorts', 'js/groups/collections/cohort', 'js/groups/models/course_cohort_settings',
+ 'js/groups/models/cohort_discussions'],
function($) {
return function(contentGroups, studioGroupConfigurationsUrl) {
var cohorts = new edx.groups.CohortCollection(),
- courseCohortSettings = new edx.groups.CourseCohortSettingsModel();
+ courseCohortSettings = new edx.groups.CourseCohortSettingsModel(),
+ discussionTopicsSettings = new edx.groups.DiscussionTopicsSettingsModel();
var cohortManagementElement = $('.cohort-management');
cohorts.url = cohortManagementElement.data('cohorts_url');
courseCohortSettings.url = cohortManagementElement.data('course_cohort_settings_url');
-
+ discussionTopicsSettings.url = cohortManagementElement.data('discussion-topics-url');
+
var cohortsView = new edx.groups.CohortsView({
el: cohortManagementElement,
model: cohorts,
contentGroups: contentGroups,
cohortSettings: courseCohortSettings,
context: {
+ discussionTopicsSettingsModel: discussionTopicsSettings,
uploadCohortsCsvUrl: cohortManagementElement.data('upload_cohorts_csv_url'),
- studioAdvancedSettingsUrl: cohortManagementElement.data('advanced-settings-url'),
studioGroupConfigurationsUrl: studioGroupConfigurationsUrl
}
});
+
cohorts.fetch().done(function() {
courseCohortSettings.fetch().done(function() {
- cohortsView.render();
- })
+ discussionTopicsSettings.fetch().done(function() {
+ cohortsView.render();
+ });
+ });
});
};
});
diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js
index 6106c16f2c..175b43f369 100644
--- a/lms/static/js/spec/groups/views/cohorts_spec.js
+++ b/lms/static/js/spec/groups/views/cohorts_spec.js
@@ -1,10 +1,13 @@
define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpers/template_helpers',
'js/groups/views/cohorts', 'js/groups/collections/cohort', 'js/groups/models/content_group',
- 'js/groups/models/course_cohort_settings', 'js/utils/animation',
- 'js/groups/views/course_cohort_settings_notification'
+ 'js/groups/models/course_cohort_settings', 'js/utils/animation', 'js/vendor/jquery.qubit',
+ 'js/groups/views/course_cohort_settings_notification', 'js/groups/models/cohort_discussions',
+ 'js/groups/views/cohort_discussions', 'js/groups/views/cohort_discussions_course_wide',
+ 'js/groups/views/cohort_discussions_inline'
],
function (Backbone, $, AjaxHelpers, TemplateHelpers, CohortsView, CohortCollection, ContentGroupModel,
- CourseCohortSettingsModel, AnimationUtil, CourseCohortSettingsNotificationView) {
+ CourseCohortSettingsModel, AnimationUtil, Qubit, CourseCohortSettingsNotificationView, DiscussionTopicsSettingsModel,
+ CohortDiscussionsView, CohortCourseWideDiscussionsView, CohortInlineDiscussionsView) {
'use strict';
describe("Cohorts View", function () {
@@ -14,7 +17,16 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
verifyDetailedMessage, verifyHeader, expectCohortAddRequest, getAddModal, selectContentGroup,
clearContentGroup, saveFormAndExpectErrors, createMockCohortSettings, MOCK_COHORTED_USER_PARTITION_ID,
MOCK_UPLOAD_COHORTS_CSV_URL, MOCK_STUDIO_ADVANCED_SETTINGS_URL, MOCK_STUDIO_GROUP_CONFIGURATIONS_URL,
- MOCK_MANUAL_ASSIGNMENT, MOCK_RANDOM_ASSIGNMENT;
+ MOCK_MANUAL_ASSIGNMENT, MOCK_RANDOM_ASSIGNMENT, createMockCohortDiscussionsJson,
+ createMockCohortDiscussions, showAndAssertDiscussionTopics;
+
+ // Selectors
+ var discussionsToggle ='.toggle-cohort-management-discussions',
+ inlineDiscussionsFormCss = '.cohort-inline-discussions-form',
+ courseWideDiscussionsFormCss = '.cohort-course-wide-discussions-form',
+ courseWideDiscussionsSaveButtonCss = '.cohort-course-wide-discussions-form .action-save',
+ inlineDiscussionsSaveButtonCss = '.cohort-inline-discussions-form .action-save',
+ inlineDiscussionsForm, courseWideDiscussionsForm;
MOCK_MANUAL_ASSIGNMENT = 'manual';
MOCK_RANDOM_ASSIGNMENT = 'random';
@@ -54,23 +66,71 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
];
};
- createMockCohortSettingsJson = function (isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions) {
+ createMockCohortSettingsJson = function (isCohorted, cohortedInlineDiscussions, cohortedCourseWideDiscussions, alwaysCohortInlineDiscussions) {
return {
id: 0,
is_cohorted: isCohorted || false,
- cohorted_discussions: cohortedDiscussions || [],
+ cohorted_inline_discussions: cohortedInlineDiscussions || [],
+ cohorted_course_wide_discussions: cohortedCourseWideDiscussions || [],
always_cohort_inline_discussions: alwaysCohortInlineDiscussions || true
};
};
- createMockCohortSettings = function (isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions) {
+ createMockCohortSettings = function (isCohorted, cohortedInlineDiscussions, cohortedCourseWideDiscussions, alwaysCohortInlineDiscussions) {
return new CourseCohortSettingsModel(
- createMockCohortSettingsJson(isCohorted, cohortedDiscussions, alwaysCohortInlineDiscussions)
+ createMockCohortSettingsJson(isCohorted, cohortedInlineDiscussions, cohortedCourseWideDiscussions, alwaysCohortInlineDiscussions)
+ );
+ };
+
+ createMockCohortDiscussionsJson = function (allCohorted) {
+ return {
+ course_wide_discussions: {
+ children: ['Topic_C_1', 'Topic_C_2'],
+ entries: {
+ Topic_C_1: {
+ sort_key: null,
+ is_cohorted: true,
+ id: 'Topic_C_1'
+ },
+ Topic_C_2: {
+ sort_key: null,
+ is_cohorted: false,
+ id: 'Topic_C_2'
+ }
+ }
+ },
+ inline_discussions: {
+ subcategories: {
+ Topic_I_1: {
+ subcategories: {},
+ children: ['Inline_Discussion_1', 'Inline_Discussion_2'],
+ entries: {
+ Inline_Discussion_1: {
+ sort_key: null,
+ is_cohorted: true,
+ id: 'Inline_Discussion_1'
+ },
+ Inline_Discussion_2: {
+ sort_key: null,
+ is_cohorted: allCohorted || false,
+ id: 'Inline_Discussion_2'
+ }
+ }
+ }
+ },
+ children: ['Topic_I_1']
+ }
+ };
+ };
+
+ createMockCohortDiscussions = function (allCohorted) {
+ return new DiscussionTopicsSettingsModel(
+ createMockCohortDiscussionsJson(allCohorted)
);
};
createCohortsView = function (test, options) {
- var cohortsJson, cohorts, contentGroups, cohortSettings;
+ var cohortsJson, cohorts, contentGroups, cohortSettings, cohortDiscussions;
options = options || {};
cohortsJson = options.cohorts ? {cohorts: options.cohorts} : createMockCohorts();
cohorts = new CohortCollection(cohortsJson, {parse: true});
@@ -78,17 +138,23 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
cohortSettings = options.cohortSettings || createMockCohortSettings(true);
cohortSettings.url = '/mock_service/cohorts/settings';
cohorts.url = '/mock_service/cohorts';
+
+ cohortDiscussions = options.cohortDiscussions || createMockCohortDiscussions();
+ cohortDiscussions.url = '/mock_service/cohorts/discussion/topics';
+
requests = AjaxHelpers.requests(test);
cohortsView = new CohortsView({
model: cohorts,
contentGroups: contentGroups,
cohortSettings: cohortSettings,
context: {
+ discussionTopicsSettingsModel: cohortDiscussions,
uploadCohortsCsvUrl: MOCK_UPLOAD_COHORTS_CSV_URL,
studioAdvancedSettingsUrl: MOCK_STUDIO_ADVANCED_SETTINGS_URL,
studioGroupConfigurationsUrl: MOCK_STUDIO_GROUP_CONFIGURATIONS_URL
}
});
+
cohortsView.render();
if (options && options.selectCohort) {
cohortsView.$('.cohort-select').val(options.selectCohort.toString()).change();
@@ -195,6 +261,41 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
verifyDetailedMessage(expectedTitle, 'error', errors);
};
+ showAndAssertDiscussionTopics = function(that) {
+
+ createCohortsView(that);
+
+ // Should see the control to toggle cohort discussions.
+ expect(cohortsView.$(discussionsToggle)).not.toHaveClass('is-hidden');
+ // But discussions form should not be visible until toggle is clicked.
+ expect(cohortsView.$(inlineDiscussionsFormCss).length).toBe(0);
+ expect(cohortsView.$(courseWideDiscussionsFormCss).length).toBe(0);
+
+ expect(cohortsView.$(discussionsToggle).text()).
+ toContain('Specify whether discussion topics are divided by cohort');
+
+ cohortsView.$(discussionsToggle).click();
+ // After toggle is clicked, it should be hidden.
+ expect(cohortsView.$(discussionsToggle)).toHaveClass('is-hidden');
+
+ // Should see the course wide discussions form and its content
+ courseWideDiscussionsForm = cohortsView.$(courseWideDiscussionsFormCss);
+ expect(courseWideDiscussionsForm.length).toBe(1);
+
+ expect(courseWideDiscussionsForm.text()).
+ toContain('Course-Wide Discussion Topics');
+ expect(courseWideDiscussionsForm.text()).
+ toContain('Select the course-wide discussion topics that you want to divide by cohort.');
+
+ // Should see the inline discussions form and its content
+ inlineDiscussionsForm = cohortsView.$(inlineDiscussionsFormCss);
+ expect(inlineDiscussionsForm.length).toBe(1);
+ expect(inlineDiscussionsForm.text()).
+ toContain('Content-Specific Discussion Topics');
+ expect(inlineDiscussionsForm.text()).
+ toContain('Specify whether content-specific discussion topics are divided by cohort.');
+ };
+
unknownUserMessage = function (name) {
return "Unknown user: " + name;
};
@@ -208,6 +309,10 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-group-header');
TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/notification');
TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-state');
+ TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-discussions-category');
+ TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-discussions-subcategory');
+ TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-discussions-course-wide');
+ TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohort-discussions-inline');
TemplateHelpers.installTemplate('templates/file-upload');
});
@@ -221,6 +326,8 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
// If no cohorts have been created, can't upload a CSV file.
expect(cohortsView.$('.wrapper-cohort-supplemental')).toHaveClass('is-hidden');
+ // if no cohorts have been created, can't show the link to discussion topics.
+ expect(cohortsView.$('.cohort-discussions-nav')).toHaveClass('is-hidden');
});
it("syncs data when membership tab is clicked", function() {
@@ -260,6 +367,10 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
.toBe("Your file 'upload_file.txt' has been uploaded. Allow a few minutes for processing.");
});
+ it('can show discussion topics if cohort exists', function () {
+ showAndAssertDiscussionTopics(this);
+ });
+
describe("Cohort Selector", function () {
it('has no initial selection', function () {
createCohortsView(this);
@@ -287,23 +398,23 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
cohortsView.$('.cohorts-state').prop('checked', true).change();
AjaxHelpers.expectJsonRequest(
- requests, 'PUT', '/mock_service/cohorts/settings',
- createMockCohortSettingsJson(true, [], true)
+ requests, 'PATCH', '/mock_service/cohorts/settings',
+ {is_cohorted: true}
);
AjaxHelpers.respondWithJson(
requests,
- createMockCohortSettingsJson(true)
+ {is_cohorted: true}
);
expect(cohortsView.$('.cohorts-state').prop('checked')).toBeTruthy();
cohortsView.$('.cohorts-state').prop('checked', false).change();
AjaxHelpers.expectJsonRequest(
- requests, 'PUT', '/mock_service/cohorts/settings',
- createMockCohortSettingsJson(false, [], true)
+ requests, 'PATCH', '/mock_service/cohorts/settings',
+ {is_cohorted: false}
);
AjaxHelpers.respondWithJson(
requests,
- createMockCohortSettingsJson(false)
+ {is_cohorted: false}
);
expect(cohortsView.$('.cohorts-state').prop('checked')).toBeFalsy();
});
@@ -960,5 +1071,379 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe
});
});
});
+
+ describe("Discussion Topics", function() {
+ var createCourseWideView, createInlineView,
+ inlineView, courseWideView, assertCohortedTopics;
+
+ createCourseWideView = function(that) {
+ createCohortsView(that);
+
+ courseWideView = new CohortCourseWideDiscussionsView({
+ el: cohortsView.$('.cohort-discussions-nav').removeClass('is-hidden'),
+ model: cohortsView.context.discussionTopicsSettingsModel,
+ cohortSettings: cohortsView.cohortSettings
+ });
+ courseWideView.render();
+ };
+
+ createInlineView = function(that, discussionTopicsSettingsModel) {
+ createCohortsView(that);
+
+ inlineView = new CohortInlineDiscussionsView({
+ el: cohortsView.$('.cohort-discussions-nav').removeClass('is-hidden'),
+ model: discussionTopicsSettingsModel || cohortsView.context.discussionTopicsSettingsModel,
+ cohortSettings: cohortsView.cohortSettings
+ });
+ inlineView.render();
+ };
+
+ assertCohortedTopics = function(view, type) {
+ expect(view.$('.check-discussion-subcategory-' + type).length).toBe(2);
+ expect(view.$('.check-discussion-subcategory-' + type + ':checked').length).toBe(1);
+ };
+
+ it('renders the view properly', function() {
+ showAndAssertDiscussionTopics(this);
+ });
+
+ describe("Course Wide", function() {
+
+ it('shows the "Save" button as disabled initially', function() {
+ createCourseWideView(this);
+ expect(courseWideView.$(courseWideDiscussionsSaveButtonCss).prop('disabled')).toBeTruthy();
+ });
+
+ it('has one cohorted and one non-cohorted topic', function() {
+ createCourseWideView(this);
+
+ assertCohortedTopics(courseWideView, 'course-wide');
+
+ expect(courseWideView.$('.cohorted-text').length).toBe(2);
+ expect(courseWideView.$('.cohorted-text.hidden').length).toBe(1);
+ });
+
+ it('enables the "Save" button after changing checkbox', function() {
+ createCourseWideView(this);
+
+ // save button is disabled.
+ expect(courseWideView.$(courseWideDiscussionsSaveButtonCss).prop('disabled')).toBeTruthy();
+
+ $(courseWideView.$('.check-discussion-subcategory-course-wide')[0]).prop('checked', false).change();
+
+ // save button is enabled.
+ expect(courseWideView.$(courseWideDiscussionsSaveButtonCss).prop('disabled')).toBeFalsy();
+ });
+
+ it('saves the topic successfully', function() {
+ createCourseWideView(this);
+
+ $(courseWideView.$('.check-discussion-subcategory-course-wide')[1]).prop('checked', 'checked').change();
+ expect(courseWideView.$(courseWideDiscussionsSaveButtonCss).prop('disabled')).toBeFalsy();
+
+ // Save the updated settings
+ courseWideView.$('.action-save').click();
+
+ // fake requests for cohort settings with PATCH method.
+ AjaxHelpers.expectJsonRequest(
+ requests, 'PATCH', '/mock_service/cohorts/settings',
+ {cohorted_course_wide_discussions: ['Topic_C_1', 'Topic_C_2']}
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ {cohorted_course_wide_discussions: ['Topic_C_1', 'Topic_C_2']}
+ );
+
+ // fake request for discussion/topics with GET method.
+ AjaxHelpers.expectJsonRequest(
+ requests, 'GET', '/mock_service/cohorts/discussion/topics'
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ createMockCohortDiscussions()
+ );
+
+ // verify the success message.
+ expect(courseWideView.$(courseWideDiscussionsSaveButtonCss).prop('disabled')).toBeTruthy();
+ verifyMessage('Your changes have been saved.', 'confirmation');
+ });
+
+ it('shows an appropriate message when subsequent "GET" returns HTTP500', function() {
+ createCourseWideView(this);
+
+ $(courseWideView.$('.check-discussion-subcategory-course-wide')[1]).prop('checked', 'checked').change();
+ expect(courseWideView.$(courseWideDiscussionsSaveButtonCss).prop('disabled')).toBeFalsy();
+
+ // Save the updated settings
+ courseWideView.$('.action-save').click();
+
+ // fake requests for cohort settings with PATCH method.
+ AjaxHelpers.expectJsonRequest(
+ requests, 'PATCH', '/mock_service/cohorts/settings',
+ {cohorted_course_wide_discussions: ['Topic_C_1', 'Topic_C_2']}
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ {cohorted_course_wide_discussions: ['Topic_C_1', 'Topic_C_2']}
+ );
+
+ // fake request for discussion/topics with GET method.
+ AjaxHelpers.expectJsonRequest(
+ requests, 'GET', '/mock_service/cohorts/discussion/topics'
+ );
+ AjaxHelpers.respondWithError(requests, 500);
+
+ var expectedTitle = "We've encountered an error. Refresh your browser and then try again.";
+ expect(courseWideView.$('.message-title').text().trim()).toBe(expectedTitle);
+ });
+
+ it('shows an appropriate error message for HTTP500', function () {
+ createCourseWideView(this);
+
+ $(courseWideView.$('.check-discussion-subcategory-course-wide')[1]).prop('checked', 'checked').change();
+ courseWideView.$('.action-save').click();
+
+ AjaxHelpers.respondWithError(requests, 500);
+ var expectedTitle = "We've encountered an error. Refresh your browser and then try again.";
+ expect(courseWideView.$('.message-title').text().trim()).toBe(expectedTitle);
+ });
+ });
+
+ describe("Inline", function() {
+ var enableSaveButton, mockGetRequest, verifySuccess, mockPatchRequest;
+
+ enableSaveButton = function() {
+ // enable the inline discussion topics.
+ inlineView.$('.check-cohort-inline-discussions').prop('checked', 'checked').change();
+
+ $(inlineView.$('.check-discussion-subcategory-inline')[0]).prop('checked', 'checked').change();
+
+ expect(inlineView.$(inlineDiscussionsSaveButtonCss).prop('disabled')).toBeFalsy();
+ };
+
+ verifySuccess = function() {
+ // verify the success message.
+ expect(inlineView.$(inlineDiscussionsSaveButtonCss).prop('disabled')).toBeTruthy();
+ verifyMessage('Your changes have been saved.', 'confirmation');
+ };
+
+ mockPatchRequest = function(cohortedInlineDiscussions) {
+ AjaxHelpers.expectJsonRequest(
+ requests, 'PATCH', '/mock_service/cohorts/settings',
+ {
+ cohorted_inline_discussions: cohortedInlineDiscussions,
+ always_cohort_inline_discussions: false
+ }
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ {
+ cohorted_inline_discussions: cohortedInlineDiscussions,
+ always_cohort_inline_discussions: false
+ }
+ );
+ };
+
+ mockGetRequest = function(allCohorted) {
+ // fake request for discussion/topics with GET method.
+ AjaxHelpers.expectJsonRequest(
+ requests, 'GET', '/mock_service/cohorts/discussion/topics'
+ );
+ AjaxHelpers.respondWithJson(
+ requests,
+ createMockCohortDiscussions(allCohorted)
+ );
+ };
+
+ it('shows the "Save" button as disabled initially', function() {
+ createInlineView(this);
+ expect(inlineView.$(inlineDiscussionsSaveButtonCss).prop('disabled')).toBeTruthy();
+ });
+
+ it('shows always cohort radio button as selected', function() {
+ createInlineView(this);
+ inlineView.$('.check-all-inline-discussions').prop('checked', 'checked').change();
+
+ // verify always cohort inline discussions is being selected.
+ expect(inlineView.$('.check-all-inline-discussions').prop('checked')).toBeTruthy();
+
+ // verify that inline topics are disabled
+ expect(inlineView.$('.check-discussion-subcategory-inline').prop('disabled')).toBeTruthy();
+ expect(inlineView.$('.check-discussion-category').prop('disabled')).toBeTruthy();
+
+ // verify that cohort some topics are not being selected.
+ expect(inlineView.$('.check-cohort-inline-discussions').prop('checked')).toBeFalsy();
+ });
+
+ it('shows cohort some topics radio button as selected', function() {
+ createInlineView(this);
+ inlineView.$('.check-cohort-inline-discussions').prop('checked', 'checked').change();
+
+ // verify some cohort inline discussions radio is being selected.
+ expect(inlineView.$('.check-cohort-inline-discussions').prop('checked')).toBeTruthy();
+
+ // verify always cohort radio is not selected.
+ expect(inlineView.$('.check-all-inline-discussions').prop('checked')).toBeFalsy();
+
+ // verify that inline topics are enabled
+ expect(inlineView.$('.check-discussion-subcategory-inline').prop('disabled')).toBeFalsy();
+ expect(inlineView.$('.check-discussion-category').prop('disabled')).toBeFalsy();
+ });
+
+ it('has cohorted and non-cohorted topics', function() {
+ createInlineView(this);
+ enableSaveButton();
+ assertCohortedTopics(inlineView, 'inline');
+ });
+
+ it('enables "Save" button after changing from always inline option', function() {
+ createInlineView(this);
+ enableSaveButton();
+ });
+
+ it('saves the topic', function() {
+ createInlineView(this);
+ enableSaveButton();
+
+ // Save the updated settings
+ inlineView.$('.action-save').click();
+
+ mockPatchRequest(['Inline_Discussion_1']);
+ mockGetRequest();
+
+ verifySuccess();
+ });
+
+ it('selects the parent category when all children are selected', function() {
+ createInlineView(this);
+ enableSaveButton();
+
+ // parent category should be indeterminate.
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(0);
+ expect(inlineView.$('.check-discussion-category:indeterminate').length).toBe(1);
+
+ inlineView.$('.check-discussion-subcategory-inline').prop('checked', 'checked').change();
+ // parent should be checked as we checked all children
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(1);
+ });
+
+ it('selects/deselects all children when a parent category is selected/deselected', function() {
+ createInlineView(this);
+ enableSaveButton();
+
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(0);
+
+ inlineView.$('.check-discussion-category').prop('checked', 'checked').change();
+
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(1);
+ expect(inlineView.$('.check-discussion-subcategory-inline:checked').length).toBe(2);
+
+ // un-check the parent, all children should be unchecd.
+ inlineView.$('.check-discussion-category').prop('checked', false).change();
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(0);
+ expect(inlineView.$('.check-discussion-subcategory-inline:checked').length).toBe(0);
+ });
+
+ it('saves correctly when a subset of topics are selected within a category', function() {
+ createInlineView(this);
+ enableSaveButton();
+
+ // parent category should be indeterminate.
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(0);
+ expect(inlineView.$('.check-discussion-category:indeterminate').length).toBe(1);
+
+ // Save the updated settings
+ inlineView.$('.action-save').click();
+
+ mockPatchRequest(['Inline_Discussion_1']);
+ mockGetRequest();
+
+ verifySuccess();
+ // parent category should be indeterminate.
+ expect(inlineView.$('.check-discussion-category:indeterminate').length).toBe(1);
+ });
+
+ it('saves correctly when all child topics are selected within a category', function() {
+ createInlineView(this);
+ enableSaveButton();
+
+ // parent category should be indeterminate.
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(0);
+ expect(inlineView.$('.check-discussion-category:indeterminate').length).toBe(1);
+
+ inlineView.$('.check-discussion-subcategory-inline').prop('checked', 'checked').change();
+ // Save the updated settings
+ inlineView.$('.action-save').click();
+
+ mockPatchRequest(['Inline_Discussion_1', 'Inline_Discussion_2']);
+ mockGetRequest(true);
+
+ verifySuccess();
+ // parent category should be checked.
+ expect(inlineView.$('.check-discussion-category:checked').length).toBe(1);
+ });
+
+ it('shows an appropriate message when no inline topics exist', function() {
+
+ var topicsJson, discussionTopicsSettingsModel;
+
+ topicsJson = {
+ course_wide_discussions: {
+ children: ['Topic_C_1'],
+ entries: {
+ Topic_C_1: {
+ sort_key: null,
+ is_cohorted: true,
+ id: 'Topic_C_1'
+ }
+ }
+ },
+ inline_discussions: {
+ subcategories: {},
+ children: []
+ }
+ };
+ discussionTopicsSettingsModel = new DiscussionTopicsSettingsModel(topicsJson);
+
+ createInlineView(this, discussionTopicsSettingsModel);
+
+ var expectedTitle = "No content-specific discussion topics exist.";
+ expect(inlineView.$('.no-topics').text().trim()).toBe(expectedTitle);
+ });
+
+ it('shows an appropriate message when subsequent "GET" returns HTTP500', function() {
+ createInlineView(this);
+ enableSaveButton();
+
+ // Save the updated settings
+ inlineView.$('.action-save').click();
+
+ mockPatchRequest(['Inline_Discussion_1']);
+
+ // fake request for discussion/topics with GET method.
+ AjaxHelpers.expectJsonRequest(
+ requests, 'GET', '/mock_service/cohorts/discussion/topics'
+ );
+ AjaxHelpers.respondWithError(requests, 500);
+
+ var expectedTitle = "We've encountered an error. Refresh your browser and then try again.";
+ expect(inlineView.$('.message-title').text().trim()).toBe(expectedTitle);
+ });
+
+ it('shows an appropriate error message for HTTP500', function () {
+ createInlineView(this);
+ enableSaveButton();
+
+ $(inlineView.$('.check-discussion-subcategory-inline')[1]).prop('checked', 'checked').change();
+ inlineView.$('.action-save').click();
+
+ AjaxHelpers.respondWithError(requests, 500);
+ var expectedTitle = "We've encountered an error. Refresh your browser and then try again.";
+ expect(inlineView.$('.message-title').text().trim()).toBe(expectedTitle);
+ });
+
+ });
+
+ });
});
});
diff --git a/lms/static/js/spec/main.js b/lms/static/js/spec/main.js
index 0c2b9fcbca..d3932fc64b 100644
--- a/lms/static/js/spec/main.js
+++ b/lms/static/js/spec/main.js
@@ -60,6 +60,7 @@
'history': 'js/vendor/history',
'js/verify_student/photocapture': 'js/verify_student/photocapture',
'js/staff_debug_actions': 'js/staff_debug_actions',
+ 'js/vendor/jquery.qubit': 'js/vendor/jquery.qubit',
// Backbone classes loaded explicitly until they are converted to use RequireJS
'js/models/notification': 'js/models/notification',
@@ -68,6 +69,10 @@
'js/groups/models/cohort': 'js/groups/models/cohort',
'js/groups/models/content_group': 'js/groups/models/content_group',
'js/groups/models/course_cohort_settings': 'js/groups/models/course_cohort_settings',
+ 'js/groups/models/cohort_discussions': 'js/groups/models/cohort_discussions',
+ 'js/groups/views/cohort_discussions': 'js/groups/views/cohort_discussions',
+ 'js/groups/views/cohort_discussions_course_wide': 'js/groups/views/cohort_discussions_course_wide',
+ 'js/groups/views/cohort_discussions_inline': 'js/groups/views/cohort_discussions_inline',
'js/groups/views/course_cohort_settings_notification': 'js/groups/views/course_cohort_settings_notification',
'js/groups/collections/cohort': 'js/groups/collections/cohort',
'js/groups/views/cohort_editor': 'js/groups/views/cohort_editor',
@@ -300,6 +305,22 @@
exports: 'edx.groups.CourseCohortSettingsModel',
deps: ['backbone']
},
+ 'js/groups/models/cohort_discussions': {
+ exports: 'edx.groups.DiscussionTopicsSettingsModel',
+ deps: ['backbone']
+ },
+ 'js/groups/views/cohort_discussions': {
+ exports: 'edx.groups.CohortDiscussionConfigurationView',
+ deps: ['backbone']
+ },
+ 'js/groups/views/cohort_discussions_course_wide': {
+ exports: 'edx.groups.CourseWideDiscussionsView',
+ deps: ['backbone', 'js/groups/views/cohort_discussions']
+ },
+ 'js/groups/views/cohort_discussions_inline': {
+ exports: 'edx.groups.InlineDiscussionsView',
+ deps: ['backbone', 'js/groups/views/cohort_discussions', 'js/vendor/jquery.qubit']
+ },
'js/groups/views/course_cohort_settings_notification': {
exports: 'edx.groups.CourseCohortSettingsNotificationView',
deps: ['backbone']
diff --git a/lms/static/js/vendor/jquery.qubit.js b/lms/static/js/vendor/jquery.qubit.js
new file mode 100755
index 0000000000..7f85712708
--- /dev/null
+++ b/lms/static/js/vendor/jquery.qubit.js
@@ -0,0 +1,97 @@
+/*
+** Checkboxes TreeView- jQuery
+** https://github.com/aexmachina/jquery-qubit
+**
+** Copyright (c) 2014 Simon Wade
+** The MIT License (MIT)
+** https://github.com/aexmachina/jquery-qubit/blob/master/LICENSE.txt
+**
+*/
+
+(function($) {
+ $.fn.qubit = function(options) {
+ return this.each(function() {
+ var qubit = new Qubit(this, options);
+ });
+ };
+ var Qubit = function(el) {
+ var self = this;
+ this.scope = $(el);
+ this.scope.on('change', 'input[type=checkbox]', function(e) {
+ if (!self.suspendListeners) {
+ self.process(e.target);
+ }
+ });
+ this.scope.find('input[type=checkbox]:checked').each(function() {
+ self.process(this);
+ });
+ };
+ Qubit.prototype = {
+ itemSelector: 'li',
+ process: function(checkbox) {
+ var checkbox = $(checkbox),
+ parentItems = checkbox.parentsUntil(this.scope, this.itemSelector);
+ try {
+ this.suspendListeners = true;
+ // all children inherit my state
+ parentItems.eq(0).find('input[type=checkbox]')
+ .filter(checkbox.prop('checked') ? ':not(:checked)' : ':checked')
+ .each(function() {
+ if (!$(this).parent().hasClass('hidden')) {
+ $(this).prop('checked', checkbox.prop('checked'));
+ }
+ })
+ .trigger('change');
+ this.processParents(checkbox);
+ } finally {
+ this.suspendListeners = false;
+ }
+ },
+ processParents: function() {
+ var self = this, changed = false;
+ this.scope.find('input[type=checkbox]').each(function() {
+ var $this = $(this),
+ parent = $this.closest(self.itemSelector),
+ children = parent.find('input[type=checkbox]').not($this),
+ numChecked = children.filter(function() {
+ return $(this).prop('checked') || $(this).prop('indeterminate');
+ }).length;
+
+ if (children.length) {
+ if (numChecked == 0) {
+ if (self.setChecked($this, false)) changed = true;
+ } else if (numChecked == children.length) {
+ if (self.setChecked($this, true)) changed = true;
+ } else {
+ if (self.setIndeterminate($this, true)) changed = true;
+ }
+ }
+ else {
+ if (self.setIndeterminate($this, false)) changed = true;
+ }
+ });
+ if (changed) this.processParents();
+ },
+ setChecked: function(checkbox, value, event) {
+ var changed = false;
+ if (checkbox.prop('indeterminate')) {
+ checkbox.prop('indeterminate', false);
+ changed = true;
+ }
+ if (checkbox.prop('checked') != value) {
+ checkbox.prop('checked', value).trigger('change');
+ changed = true;
+ }
+ return changed;
+ },
+ setIndeterminate: function(checkbox, value) {
+ if (value) {
+ checkbox.prop('checked', false);
+ }
+ if (checkbox.prop('indeterminate') != value) {
+ checkbox.prop('indeterminate', value);
+ return true;
+ }
+ }
+ };
+}(jQuery));
diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss
index bb9c40cad2..f5ad96107a 100644
--- a/lms/static/sass/course/instructor/_instructor_2.scss
+++ b/lms/static/sass/course/instructor/_instructor_2.scss
@@ -725,19 +725,18 @@
vertical-align: middle;
}
- .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 {
@extend %t-copy-sub1;
}
}
+ .form-submit {
+ @include idashbutton($blue);
+ @include font-size(14);
+ @include line-height(14);
+ margin-right: ($baseline/2);
+ margin-bottom: 0;
+ text-shadow: none;
+ }
.cohort-management-nav {
@include clearfix();
@@ -924,8 +923,9 @@
}
}
- // CSV-based file upload for auto cohort assigning
- .toggle-cohort-management-secondary {
+ // CSV-based file upload for auto cohort assigning and
+ // cohort the discussion topics.
+ .toggle-cohort-management-secondary, .toggle-cohort-management-discussions {
@extend %t-copy-sub1;
}
@@ -1071,6 +1071,49 @@
}
}
+ // cohort discussions interface.
+ .cohort-discussions-nav {
+
+ .cohort-course-wide-discussions-form {
+
+ .form-actions {
+ padding-top: ($baseline/2);
+ }
+ }
+
+ .category-title,
+ .topic-name,
+ .all-inline-discussions,
+ .always_cohort_inline_discussions,
+ .cohort_inline_discussions {
+ padding-left: ($baseline/2);
+ }
+
+ .always_cohort_inline_discussions,
+ .cohort_inline_discussions {
+ padding-top: ($baseline/2);
+ }
+
+ .category-item,
+ .subcategory-item {
+ padding-top: ($baseline/2);
+ }
+
+ .cohorted-text {
+ color: $link-color;
+ }
+
+ .discussions-wrapper {
+ @extend %ui-no-list;
+ padding: 0 ($baseline/2);
+
+ .subcategories {
+ padding: 0 ($baseline*1.5);
+ }
+ }
+ }
+
+
.wrapper-tabs { // This applies to the tab-like interface that toggles between the student management and the group settings
@extend %ui-no-list;
@extend %ui-depth1;
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-category.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-category.underscore
new file mode 100644
index 0000000000..735f4f0ad6
--- /dev/null
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-category.underscore
@@ -0,0 +1,10 @@
+
+
+
+ />
+ <%- name %>
+
+
+
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-course-wide.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-course-wide.underscore
new file mode 100644
index 0000000000..7cc647389f
--- /dev/null
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-course-wide.underscore
@@ -0,0 +1,19 @@
+<%- gettext('Specify whether discussion topics are divided by cohort') %>
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-inline.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-inline.underscore
new file mode 100644
index 0000000000..f70225d3d7
--- /dev/null
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-inline.underscore
@@ -0,0 +1,38 @@
+
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-subcategory.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-subcategory.underscore
new file mode 100644
index 0000000000..28b2dffb86
--- /dev/null
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-discussions-subcategory.underscore
@@ -0,0 +1,9 @@
+
+
+
+ />
+ <%- name %>
+ - <%- gettext('Cohorted') %>
+
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-group-header.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-group-header.underscore
index e9cac34abf..b5e441859e 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohort-group-header.underscore
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort-group-header.underscore
@@ -18,9 +18,4 @@
<%- gettext("What does this mean?") %>
<% } %>
-
-
\ No newline at end of file
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort_management.html b/lms/templates/instructor/instructor_dashboard_2/cohort_management.html
index 715d16e3d1..0b3ca814cd 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohort_management.html
+++ b/lms/templates/instructor/instructor_dashboard_2/cohort_management.html
@@ -7,9 +7,9 @@
@@ -34,7 +34,7 @@
];
(function (require) {
- require(['js/factories/cohorts_factory'], function (CohortsFactory) {
+ require(['js/groups/views/cohorts_dashboard_factory'], function (CohortsFactory) {
CohortsFactory(contentGroups, '${get_studio_url(course, 'group_configurations') | h}');
});
}).call(this, require || RequireJS.require);
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore
index 3c5e28d5fe..26925bb7d7 100644
--- a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore
+++ b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore
@@ -47,5 +47,15 @@
) %>
+
+
+
+
+ <%- gettext('Specify whether discussion topics are divided by cohort') %>
+
+
<% } %>
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 9cac21bb1d..6148f285d3 100644
--- a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
+++ b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
@@ -48,6 +48,8 @@
+
+
<%static:js group='module-descriptor-js'/>
<%static:js group='instructor_dash'/>
<%static:js group='application'/>
@@ -63,6 +65,10 @@
+
+
+
+
@@ -71,7 +77,7 @@
## Include Underscore templates
<%block name="header_extras">
-% for template_name in ["cohorts", "cohort-editor", "cohort-group-header", "cohort-selector", "cohort-form", "notification", "cohort-state"]:
+% for template_name in ["cohorts", "cohort-editor", "cohort-group-header", "cohort-selector", "cohort-form", "notification", "cohort-state", "cohort-discussions-inline", "cohort-discussions-course-wide", "cohort-discussions-category","cohort-discussions-subcategory"]:
diff --git a/lms/urls.py b/lms/urls.py
index 445aecfdcd..97a9a74e4e 100644
--- a/lms/urls.py
+++ b/lms/urls.py
@@ -396,6 +396,9 @@ if settings.COURSEWARE_ENABLED:
url(r'^courses/{}/cohorts/debug$'.format(settings.COURSE_KEY_PATTERN),
'openedx.core.djangoapps.course_groups.views.debug_cohort_mgmt',
name="debug_cohort_mgmt"),
+ url(r'^courses/{}/cohorts/topics$'.format(settings.COURSE_KEY_PATTERN),
+ 'openedx.core.djangoapps.course_groups.views.cohort_discussion_topics',
+ name='cohort_discussion_topics'),
# Open Ended Notifications
url(r'^courses/{}/open_ended_notifications$'.format(settings.COURSE_ID_PATTERN),
diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py
index 95bec032c0..900ce98889 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_views.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_views.py
@@ -6,20 +6,22 @@ Tests for course group views
import json
from collections import namedtuple
+from datetime import datetime
from django.contrib.auth.models import User
from django.http import Http404
from django.test.client import RequestFactory
-
+import django_test_client_utils # monkey-patch for PATCH request method.
from student.models import CourseEnrollment
from student.tests.factories import UserFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from xmodule.modulestore.tests.factories import ItemFactory
from ..models import CourseUserGroup, CourseCohort
from ..views import (
course_cohort_settings_handler, cohort_handler, users_in_cohort, add_users_to_cohort, remove_user_from_cohort,
- link_cohort_to_partition_group
+ link_cohort_to_partition_group, cohort_discussion_topics
)
from ..cohorts import (
get_cohort, get_cohort_by_name, get_cohort_by_id,
@@ -91,6 +93,33 @@ class CohortViewsTestCase(ModuleStoreTestCase):
view_args.insert(0, request)
self.assertRaises(Http404, view, *view_args)
+ def create_cohorted_discussions(self):
+ cohorted_inline_discussions = ['Topic A']
+ cohorted_course_wide_discussions = ["Topic B"]
+ cohorted_discussions = cohorted_inline_discussions + cohorted_course_wide_discussions
+
+ # inline discussion
+ ItemFactory.create(
+ parent_location=self.course.location,
+ category="discussion",
+ discussion_id=topic_name_to_id(self.course, "Topic A"),
+ discussion_category="Chapter",
+ discussion_target="Discussion",
+ start=datetime.now()
+ )
+ # course-wide discussion
+ discussion_topics = {
+ "Topic B": {"id": "Topic B"},
+ }
+
+ config_course_cohorts(
+ self.course,
+ is_cohorted=True,
+ discussion_topics=discussion_topics,
+ cohorted_discussions=cohorted_discussions
+ )
+ return cohorted_inline_discussions, cohorted_course_wide_discussions
+
def get_handler(self, course, cohort=None, expected_response_code=200, handler=cohort_handler):
"""
Call a GET on `handler` for a given `course` and return its response as a dict.
@@ -121,56 +150,143 @@ class CohortViewsTestCase(ModuleStoreTestCase):
self.assertEqual(response.status_code, expected_response_code)
return json.loads(response.content)
+ def patch_handler(self, course, cohort=None, data=None, expected_response_code=200, handler=cohort_handler):
+ """
+ Call a PATCH on `handler` for a given `course` and return its response as a dict.
+ Raise an exception if response status code is not as expected.
+ """
+ if not isinstance(data, basestring):
+ data = json.dumps(data or {})
+
+ request = RequestFactory().patch(path="dummy path", data=data, content_type="application/json")
+ request.user = self.staff_user
+ if cohort:
+ response = handler(request, unicode(course.id), cohort.id)
+ else:
+ response = handler(request, unicode(course.id))
+ self.assertEqual(response.status_code, expected_response_code)
+ return json.loads(response.content)
+
class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
Tests the `course_cohort_settings_handler` view.
"""
+
+ def get_expected_response(self):
+ """
+ Returns the static response dict.
+ """
+ return {
+ 'is_cohorted': True,
+ 'always_cohort_inline_discussions': True,
+ 'cohorted_inline_discussions': [],
+ 'cohorted_course_wide_discussions': [],
+ 'id': 1
+ }
+
def test_non_staff(self):
"""
Verify that we cannot access course_cohort_settings_handler if we're a non-staff user.
"""
self._verify_non_staff_cannot_access(course_cohort_settings_handler, "GET", [unicode(self.course.id)])
- self._verify_non_staff_cannot_access(course_cohort_settings_handler, "PUT", [unicode(self.course.id)])
+ self._verify_non_staff_cannot_access(course_cohort_settings_handler, "PATCH", [unicode(self.course.id)])
def test_get_settings(self):
"""
Verify that course_cohort_settings_handler is working for HTTP GET.
"""
- cohorted_discussions = ['Topic A', 'Topic B']
- config_course_cohorts(self.course, is_cohorted=True, cohorted_discussions=cohorted_discussions)
+ cohorted_inline_discussions, cohorted_course_wide_discussions = self.create_cohorted_discussions()
response = self.get_handler(self.course, handler=course_cohort_settings_handler)
- response['cohorted_discussions'].sort()
+ expected_response = self.get_expected_response()
- expected_response = {
- 'is_cohorted': True,
- 'always_cohort_inline_discussions': True,
- 'cohorted_discussions': [topic_name_to_id(self.course, name) for name in cohorted_discussions],
- 'id': 1
- }
- expected_response['cohorted_discussions'].sort()
+ expected_response['cohorted_inline_discussions'] = [topic_name_to_id(self.course, name)
+ for name in cohorted_inline_discussions]
+ expected_response['cohorted_course_wide_discussions'] = [topic_name_to_id(self.course, name)
+ for name in cohorted_course_wide_discussions]
self.assertEqual(response, expected_response)
- def test_update_settings(self):
+ def test_update_is_cohorted_settings(self):
"""
- Verify that course_cohort_settings_handler is working for HTTP POST.
+ Verify that course_cohort_settings_handler is working for is_cohorted via HTTP PATCH.
"""
config_course_cohorts(self.course, is_cohorted=True)
response = self.get_handler(self.course, handler=course_cohort_settings_handler)
- expected_response = {
- 'is_cohorted': True,
- 'always_cohort_inline_discussions': True,
- 'cohorted_discussions': [],
- 'id': 1
- }
+ expected_response = self.get_expected_response()
+
self.assertEqual(response, expected_response)
expected_response['is_cohorted'] = False
- response = self.put_handler(self.course, data=expected_response, handler=course_cohort_settings_handler)
+ response = self.patch_handler(self.course, data=expected_response, handler=course_cohort_settings_handler)
+
+ self.assertEqual(response, expected_response)
+
+ def test_update_always_cohort_inline_discussion_settings(self):
+ """
+ Verify that course_cohort_settings_handler is working for always_cohort_inline_discussions via HTTP PATCH.
+ """
+ config_course_cohorts(self.course, is_cohorted=True)
+
+ response = self.get_handler(self.course, handler=course_cohort_settings_handler)
+
+ expected_response = self.get_expected_response()
+
+ self.assertEqual(response, expected_response)
+
+ expected_response['always_cohort_inline_discussions'] = False
+ response = self.patch_handler(self.course, data=expected_response, handler=course_cohort_settings_handler)
+
+ self.assertEqual(response, expected_response)
+
+ def test_update_course_wide_discussion_settings(self):
+ """
+ Verify that course_cohort_settings_handler is working for cohorted_course_wide_discussions via HTTP PATCH.
+ """
+ # course-wide discussion
+ discussion_topics = {
+ "Topic B": {"id": "Topic B"},
+ }
+
+ config_course_cohorts(self.course, is_cohorted=True, discussion_topics=discussion_topics)
+
+ response = self.get_handler(self.course, handler=course_cohort_settings_handler)
+
+ expected_response = self.get_expected_response()
+ self.assertEqual(response, expected_response)
+
+ expected_response['cohorted_course_wide_discussions'] = [topic_name_to_id(self.course, "Topic B")]
+ response = self.patch_handler(self.course, data=expected_response, handler=course_cohort_settings_handler)
+
+ self.assertEqual(response, expected_response)
+
+ def test_update_inline_discussion_settings(self):
+ """
+ Verify that course_cohort_settings_handler is working for cohorted_inline_discussions via HTTP PATCH.
+ """
+ config_course_cohorts(self.course, is_cohorted=True)
+
+ response = self.get_handler(self.course, handler=course_cohort_settings_handler)
+
+ expected_response = self.get_expected_response()
+ self.assertEqual(response, expected_response)
+
+ now = datetime.now()
+ # inline discussion
+ ItemFactory.create(
+ parent_location=self.course.location,
+ category="discussion",
+ discussion_id="Topic_A",
+ discussion_category="Chapter",
+ discussion_target="Discussion",
+ start=now
+ )
+
+ expected_response['cohorted_inline_discussions'] = ["Topic_A"]
+ response = self.patch_handler(self.course, data=expected_response, handler=course_cohort_settings_handler)
self.assertEqual(response, expected_response)
@@ -180,7 +296,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
config_course_cohorts(self.course, is_cohorted=True)
- response = self.put_handler(self.course, expected_response_code=400, handler=course_cohort_settings_handler)
+ response = self.patch_handler(self.course, expected_response_code=400, handler=course_cohort_settings_handler)
self.assertEqual("Bad Request", response.get("error"))
def test_update_settings_with_invalid_field_data_type(self):
@@ -189,7 +305,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
"""
config_course_cohorts(self.course, is_cohorted=True)
- response = self.put_handler(
+ response = self.patch_handler(
self.course,
data={'is_cohorted': ''},
expected_response_code=400,
@@ -1075,3 +1191,58 @@ class RemoveUserFromCohortTestCase(CohortViewsTestCase):
cohort = CohortFactory(course_id=self.course.id, users=[user])
response_dict = self.request_remove_user_from_cohort(user.username, cohort)
self.verify_removed_user_from_cohort(user.username, response_dict, cohort)
+
+
+class CourseCohortDiscussionTopicsTestCase(CohortViewsTestCase):
+ """
+ Tests the `cohort_discussion_topics` view.
+ """
+
+ def test_non_staff(self):
+ """
+ Verify that we cannot access cohort_discussion_topics if we're a non-staff user.
+ """
+ self._verify_non_staff_cannot_access(cohort_discussion_topics, "GET", [unicode(self.course.id)])
+
+ def test_get_discussion_topics(self):
+ """
+ Verify that course_cohort_settings_handler is working for HTTP GET.
+ """
+ # create inline & course-wide discussion to verify the different map.
+ self.create_cohorted_discussions()
+
+ response = self.get_handler(self.course, handler=cohort_discussion_topics)
+ start_date = response['inline_discussions']['subcategories']['Chapter']['start_date']
+ expected_response = {
+ "course_wide_discussions": {
+ 'children': ['Topic B'],
+ 'entries': {
+ 'Topic B': {
+ 'sort_key': 'A',
+ 'is_cohorted': True,
+ 'id': topic_name_to_id(self.course, "Topic B"),
+ 'start_date': response['course_wide_discussions']['entries']['Topic B']['start_date']
+ }
+ }
+ },
+ "inline_discussions": {
+ 'subcategories': {
+ 'Chapter': {
+ 'subcategories': {},
+ 'children': ['Discussion'],
+ 'entries': {
+ 'Discussion': {
+ 'sort_key': None,
+ 'is_cohorted': True,
+ 'id': topic_name_to_id(self.course, "Topic A"),
+ 'start_date': start_date
+ }
+ },
+ 'sort_key': 'Chapter',
+ 'start_date': start_date
+ }
+ },
+ 'children': ['Chapter']
+ }
+ }
+ self.assertEqual(response, expected_response)
diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py
index e3c89f5345..f49e278e4e 100644
--- a/openedx/core/djangoapps/course_groups/views.py
+++ b/openedx/core/djangoapps/course_groups/views.py
@@ -22,6 +22,7 @@ from courseware.courses import get_course_with_access
from edxmako.shortcuts import render_to_response
from . import cohorts
+from lms.djangoapps.django_comment_client.utils import get_discussion_category_map, get_discussion_categories_ids
from .models import CourseUserGroup, CourseUserGroupPartitionGroup
log = logging.getLogger(__name__)
@@ -61,14 +62,19 @@ def unlink_cohort_partition_group(cohort):
# pylint: disable=invalid-name
-def _get_course_cohort_settings_representation(course_cohort_settings):
+def _get_course_cohort_settings_representation(course, course_cohort_settings):
"""
Returns a JSON representation of a course cohort settings.
"""
+ cohorted_course_wide_discussions, cohorted_inline_discussions = get_cohorted_discussions(
+ course, course_cohort_settings
+ )
+
return {
'id': course_cohort_settings.id,
'is_cohorted': course_cohort_settings.is_cohorted,
- 'cohorted_discussions': course_cohort_settings.cohorted_discussions,
+ 'cohorted_inline_discussions': cohorted_inline_discussions,
+ 'cohorted_course_wide_discussions': cohorted_course_wide_discussions,
'always_cohort_inline_discussions': course_cohort_settings.always_cohort_inline_discussions,
}
@@ -89,7 +95,26 @@ def _get_cohort_representation(cohort, course):
}
-@require_http_methods(("GET", "PUT", "POST"))
+def get_cohorted_discussions(course, course_settings):
+ """
+ Returns the course-wide and inline cohorted discussion ids separately.
+ """
+ cohorted_course_wide_discussions = []
+ cohorted_inline_discussions = []
+
+ course_wide_discussions = [topic['id'] for __, topic in course.discussion_topics.items()]
+ all_discussions = get_discussion_categories_ids(course, include_all=True)
+
+ for cohorted_discussion_id in course_settings.cohorted_discussions:
+ if cohorted_discussion_id in course_wide_discussions:
+ cohorted_course_wide_discussions.append(cohorted_discussion_id)
+ elif cohorted_discussion_id in all_discussions:
+ cohorted_inline_discussions.append(cohorted_discussion_id)
+
+ return cohorted_course_wide_discussions, cohorted_inline_discussions
+
+
+@require_http_methods(("GET", "PATCH"))
@ensure_csrf_cookie
@expect_json
@login_required
@@ -99,27 +124,49 @@ def course_cohort_settings_handler(request, course_key_string):
This will raise 404 if user is not staff.
GET
Returns the JSON representation of cohort settings for the course.
- PUT or POST
+ PATCH
Updates the cohort settings for the course. Returns the JSON representation of updated settings.
"""
course_key = CourseKey.from_string(course_key_string)
- get_course_with_access(request.user, 'staff', course_key)
- if request.method == 'GET':
- cohort_settings = cohorts.get_course_cohort_settings(course_key)
- return JsonResponse(_get_course_cohort_settings_representation(cohort_settings))
- else:
- is_cohorted = request.json.get('is_cohorted')
- if is_cohorted is None:
- # Note: error message not translated because it is not exposed to the user (UI prevents this state).
- return JsonResponse({"error": "Bad Request"}, 400)
+ course = get_course_with_access(request.user, 'staff', course_key)
+ cohort_settings = cohorts.get_course_cohort_settings(course_key)
+
+ if request.method == 'PATCH':
+ cohorted_course_wide_discussions, cohorted_inline_discussions = get_cohorted_discussions(
+ course, cohort_settings
+ )
+
+ settings_to_change = {}
+
+ if 'is_cohorted' in request.json:
+ settings_to_change['is_cohorted'] = request.json.get('is_cohorted')
+
+ if 'cohorted_course_wide_discussions' in request.json or 'cohorted_inline_discussions' in request.json:
+ cohorted_course_wide_discussions = request.json.get(
+ 'cohorted_course_wide_discussions', cohorted_course_wide_discussions
+ )
+ cohorted_inline_discussions = request.json.get(
+ 'cohorted_inline_discussions', cohorted_inline_discussions
+ )
+ settings_to_change['cohorted_discussions'] = cohorted_course_wide_discussions + cohorted_inline_discussions
+
+ if 'always_cohort_inline_discussions' in request.json:
+ settings_to_change['always_cohort_inline_discussions'] = request.json.get(
+ 'always_cohort_inline_discussions'
+ )
+
+ if not settings_to_change:
+ return JsonResponse({"error": unicode("Bad Request")}, 400)
try:
- cohort_settings = cohorts.set_course_cohort_settings(course_key, is_cohorted=is_cohorted)
+ cohort_settings = cohorts.set_course_cohort_settings(
+ course_key, **settings_to_change
+ )
except ValueError as err:
# Note: error message not translated because it is not exposed to the user (UI prevents this state).
return JsonResponse({"error": unicode(err)}, 400)
- return JsonResponse(_get_course_cohort_settings_representation(cohort_settings))
+ return JsonResponse(_get_course_cohort_settings_representation(course, cohort_settings))
@require_http_methods(("GET", "PUT", "POST", "PATCH"))
@@ -362,3 +409,79 @@ def debug_cohort_mgmt(request, course_key_string):
kwargs={'course_key': course_key.to_deprecated_string()}
)}
return render_to_response('/course_groups/debug.html', context)
+
+
+@expect_json
+@login_required
+def cohort_discussion_topics(request, course_key_string):
+ """
+ The handler for cohort discussion categories requests.
+ This will raise 404 if user is not staff.
+
+ Returns the JSON representation of discussion topics w.r.t categories for the course.
+
+ Example:
+ >>> example = {
+ >>> "course_wide_discussions": {
+ >>> "entries": {
+ >>> "General": {
+ >>> "sort_key": "General",
+ >>> "is_cohorted": True,
+ >>> "id": "i4x-edx-eiorguegnru-course-foobarbaz"
+ >>> }
+ >>> }
+ >>> "children": ["General"]
+ >>> },
+ >>> "inline_discussions" : {
+ >>> "subcategories": {
+ >>> "Getting Started": {
+ >>> "subcategories": {},
+ >>> "children": [
+ >>> "Working with Videos",
+ >>> "Videos on edX"
+ >>> ],
+ >>> "entries": {
+ >>> "Working with Videos": {
+ >>> "sort_key": None,
+ >>> "is_cohorted": False,
+ >>> "id": "d9f970a42067413cbb633f81cfb12604"
+ >>> },
+ >>> "Videos on edX": {
+ >>> "sort_key": None,
+ >>> "is_cohorted": False,
+ >>> "id": "98d8feb5971041a085512ae22b398613"
+ >>> }
+ >>> }
+ >>> },
+ >>> "children": ["Getting Started"]
+ >>> },
+ >>> }
+ >>> }
+ """
+ course_key = CourseKey.from_string(course_key_string)
+ course = get_course_with_access(request.user, 'staff', course_key)
+
+ discussion_topics = {}
+ discussion_category_map = get_discussion_category_map(course, cohorted_if_in_list=True, exclude_unstarted=False)
+
+ # We extract the data for the course wide discussions from the category map.
+ course_wide_entries = discussion_category_map.pop('entries')
+
+ course_wide_children = []
+ inline_children = []
+
+ for name in discussion_category_map['children']:
+ if name in course_wide_entries:
+ course_wide_children.append(name)
+ else:
+ inline_children.append(name)
+
+ discussion_topics['course_wide_discussions'] = {
+ 'entries': course_wide_entries,
+ 'children': course_wide_children
+ }
+
+ discussion_category_map['children'] = inline_children
+ discussion_topics['inline_discussions'] = discussion_category_map
+
+ return JsonResponse(discussion_topics)
From c50f8281875cd1bdce0f2737e026f8f5ba8da607 Mon Sep 17 00:00:00 2001
From: Usman Khalid <2200617@gmail.com>
Date: Mon, 23 Mar 2015 16:36:48 +0500
Subject: [PATCH 13/13] Fixes after rebase.
---
.../django_comment_client/forum/tests.py | 25 ++++++++++---------
.../django_comment_client/tests/test_utils.py | 6 +++--
...test_remove_users_from_multiple_cohorts.py | 4 +--
.../course_groups/tests/test_views.py | 4 +++
.../core/djangoapps/course_groups/views.py | 6 +++--
5 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py
index ecd30c2145..bbeb7dba09 100644
--- a/lms/djangoapps/django_comment_client/forum/tests.py
+++ b/lms/djangoapps/django_comment_client/forum/tests.py
@@ -316,11 +316,11 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
@ddt.data(
# old mongo with cache: 15
- (ModuleStoreEnum.Type.mongo, 1, 21, 15, 30),
- (ModuleStoreEnum.Type.mongo, 50, 315, 15, 30),
+ (ModuleStoreEnum.Type.mongo, 1, 21, 15, 40, 27),
+ (ModuleStoreEnum.Type.mongo, 50, 315, 15, 628, 27),
# split mongo: 3 queries, regardless of thread response size.
- (ModuleStoreEnum.Type.split, 1, 3, 3, 30),
- (ModuleStoreEnum.Type.split, 50, 3, 3, 30),
+ (ModuleStoreEnum.Type.split, 1, 3, 3, 40, 27),
+ (ModuleStoreEnum.Type.split, 50, 3, 3, 628, 27),
)
@ddt.unpack
def test_number_of_mongo_queries(
@@ -329,7 +329,8 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
num_thread_responses,
num_uncached_mongo_calls,
num_cached_mongo_calls,
- num_sql_queries,
+ num_uncached_sql_queries,
+ num_cached_sql_queries,
mock_request
):
with modulestore().default_store(default_store):
@@ -371,15 +372,15 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
backend='django.core.cache.backends.dummy.DummyCache',
LOCATION='single_thread_local_cache'
)
- cached_calls = {
- single_thread_dummy_cache: num_uncached_mongo_calls,
- single_thread_local_cache: num_cached_mongo_calls
- }
- for single_thread_cache, expected_calls in cached_calls.items():
+ cached_calls = [
+ [single_thread_dummy_cache, num_uncached_mongo_calls, num_uncached_sql_queries],
+ [single_thread_local_cache, num_cached_mongo_calls, num_cached_sql_queries]
+ ]
+ for single_thread_cache, expected_mongo_calls, expected_sql_queries in cached_calls:
single_thread_cache.clear()
with patch("django_comment_client.permissions.CACHE", single_thread_cache):
- with self.assertNumQueries(num_sql_queries):
- with check_mongo_calls(expected_calls):
+ with self.assertNumQueries(expected_sql_queries):
+ with check_mongo_calls(expected_mongo_calls):
call_single_thread()
single_thread_cache.clear()
diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py
index b67a0ad2d7..81cff7d477 100644
--- a/lms/djangoapps/django_comment_client/tests/test_utils.py
+++ b/lms/djangoapps/django_comment_client/tests/test_utils.py
@@ -17,6 +17,7 @@ from django_comment_client.tests.unicode import UnicodeTestMixin
from django_comment_client.tests.utils import ContentGroupTestCase
import django_comment_client.utils as utils
+from courseware.tests.factories import InstructorFactory
from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings
from student.tests.factories import UserFactory, CourseEnrollmentFactory
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
@@ -186,6 +187,7 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
self.course.discussion_topics = {}
self.course.save()
self.discussion_num = 0
+ self.instructor = InstructorFactory(course_key=self.course.id)
self.maxDiff = None # pylint: disable=invalid-name
def create_discussion(self, discussion_category, discussion_target, **kwargs):
@@ -199,12 +201,12 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
**kwargs
)
- def assert_category_map_equals(self, expected, cohorted_if_in_list=False, exclude_unstarted=True):
+ def assert_category_map_equals(self, expected, cohorted_if_in_list=False, exclude_unstarted=True): # pylint: disable=arguments-differ
"""
Asserts the expected map with the map returned by get_discussion_category_map method.
"""
self.assertEqual(
- utils.get_discussion_category_map(self.course, cohorted_if_in_list, exclude_unstarted),
+ utils.get_discussion_category_map(self.course, self.instructor, cohorted_if_in_list, exclude_unstarted),
expected
)
diff --git a/openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py b/openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py
index afd233b046..1546dee667 100644
--- a/openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py
+++ b/openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py
@@ -36,10 +36,10 @@ class TestMultipleCohortUsers(ModuleStoreTestCase):
"""
# set two auto_cohort_groups for both courses
config_course_cohorts(
- self.course1, [], cohorted=True, auto_cohort_groups=["Course1AutoGroup1", "Course1AutoGroup2"]
+ self.course1, is_cohorted=True, auto_cohorts=["Course1AutoGroup1", "Course1AutoGroup2"]
)
config_course_cohorts(
- self.course2, [], cohorted=True, auto_cohort_groups=["Course2AutoGroup1", "Course2AutoGroup2"]
+ self.course2, is_cohorted=True, auto_cohorts=["Course2AutoGroup1", "Course2AutoGroup2"]
)
# get the cohorts from the courses, which will cause auto cohorts to be created
diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py
index 900ce98889..886e9eb454 100644
--- a/openedx/core/djangoapps/course_groups/tests/test_views.py
+++ b/openedx/core/djangoapps/course_groups/tests/test_views.py
@@ -7,6 +7,9 @@ import json
from collections import namedtuple
from datetime import datetime
+from unittest import skipUnless
+
+from django.conf import settings
from django.contrib.auth.models import User
from django.http import Http404
from django.test.client import RequestFactory
@@ -1193,6 +1196,7 @@ class RemoveUserFromCohortTestCase(CohortViewsTestCase):
self.verify_removed_user_from_cohort(user.username, response_dict, cohort)
+@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS')
class CourseCohortDiscussionTopicsTestCase(CohortViewsTestCase):
"""
Tests the `cohort_discussion_topics` view.
diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py
index f49e278e4e..699002942f 100644
--- a/openedx/core/djangoapps/course_groups/views.py
+++ b/openedx/core/djangoapps/course_groups/views.py
@@ -103,7 +103,7 @@ def get_cohorted_discussions(course, course_settings):
cohorted_inline_discussions = []
course_wide_discussions = [topic['id'] for __, topic in course.discussion_topics.items()]
- all_discussions = get_discussion_categories_ids(course, include_all=True)
+ all_discussions = get_discussion_categories_ids(course, None, include_all=True)
for cohorted_discussion_id in course_settings.cohorted_discussions:
if cohorted_discussion_id in course_wide_discussions:
@@ -462,7 +462,9 @@ def cohort_discussion_topics(request, course_key_string):
course = get_course_with_access(request.user, 'staff', course_key)
discussion_topics = {}
- discussion_category_map = get_discussion_category_map(course, cohorted_if_in_list=True, exclude_unstarted=False)
+ discussion_category_map = get_discussion_category_map(
+ course, request.user, cohorted_if_in_list=True, exclude_unstarted=False
+ )
# We extract the data for the course wide discussions from the category map.
course_wide_entries = discussion_category_map.pop('entries')