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/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/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/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index e86aceb8f1..e43d58c001 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 @@ -28,6 +28,15 @@ class InstructorDashboardPage(CoursePage): membership_section.wait_for_page() return membership_section + def select_cohort_management(self): + """ + Selects the cohort management tab and returns the CohortManagementSection + """ + self.q(css='a[data-section=cohort_management]').first.click() + cohort_management_section = CohortManagementSection(self.browser) + cohort_management_section.wait_for_page() + return cohort_management_section + def select_data_download(self): """ Selects the data download tab and returns a DataDownloadPage. @@ -84,16 +93,10 @@ class MembershipPage(PageObject): """ return MembershipPageAutoEnrollSection(self.browser) - def select_cohort_management_section(self): - """ - Returns the MembershipPageCohortManagementSection page object. - """ - return MembershipPageCohortManagementSection(self.browser) - -class MembershipPageCohortManagementSection(PageObject): +class CohortManagementSection(PageObject): """ - The cohort management subsection of the Membership section of the Instructor dashboard. + The Cohort Management section of the Instructor dashboard. """ url = None csv_browse_button_selector_css = '.csv-upload #file-upload-form-file' @@ -101,21 +104,31 @@ 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' + 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.membership-section').present + return self.q(css='.cohort-management').present def _bounded_selector(self, selector): """ Return `selector`, but limited to the cohort management context. """ - return '.cohort-management.membership-section {}'.format(selector) + return '.cohort-management {}'.format(selector) def _get_cohort_options(self): """ 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 +142,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.visible: + return query.text[0] + + 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 +190,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 +206,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,20 +216,48 @@ 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. """ - create_buttons = self.q(css=self._bounded_selector(".action-create")) + add_cohort_selector = self._bounded_selector(".action-create") + + # We need to wait because sometime add cohort button is not in a state to be clickable. + self.wait_for_element_presence(add_cohort_selector, 'Add Cohort button is present.') + create_buttons = self.q(css=add_cohort_selector) # There are 2 create buttons on the page. The second one is only present when no cohort yet exists # (in which case the first is not visible). Click on the last present create button. 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 +268,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,22 +319,14 @@ 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): + def get_cohort_associated_assignment_type(self): """ - Waits for the expected content_group (or none) to show as selected for - cohort associated content group. + Returns the assignment type associated with the cohort currently being edited. """ - 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.') + self.select_cohort_settings() + css_selector = self._bounded_selector(self.assignment_type_buttons_css) + radio_button = self.q(css=css_selector).filter(lambda el: el.is_selected()).results[0] + return radio_button.get_attribute('value') def set_cohort_associated_content_group(self, content_group=None, select_settings=True): """ @@ -274,8 +340,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): """ @@ -390,6 +455,145 @@ class MembershipPageCohortManagementSection(PageObject): file_input.send_keys(path) self.q(css=self._bounded_selector(self.csv_upload_button_selector_css)).first.click() + @property + def is_cohorted(self): + """ + Returns the state of `Enable Cohorts` checkbox state. + """ + return self.q(css=self._bounded_selector('.cohorts-state')).selected + + @is_cohorted.setter + def is_cohorted(self, state): + """ + Check/Uncheck the `Enable Cohorts` checkbox state. + """ + 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). + """ + return (self.q(css=self._bounded_selector('.cohort-management-nav')).visible and + self.q(css=self._bounded_selector('.wrapper-cohort-supplemental')).visible) + class MembershipPageAutoEnrollSection(PageObject): """ 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/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py index f0722cd236..269148b20d 100644 --- a/common/test/acceptance/tests/discussion/helpers.py +++ b/common/test/acceptance/tests/discussion/helpers.py @@ -60,20 +60,17 @@ 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.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. """ 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..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 @@ -63,8 +63,7 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin # go to the membership page on the instructor dashboard self.instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id) self.instructor_dashboard_page.visit() - membership_page = self.instructor_dashboard_page.select_membership() - self.cohort_management_page = membership_page.select_cohort_management_section() + self.cohort_management_page = self.instructor_dashboard_page.select_cohort_management() def verify_cohort_description(self, cohort_name, expected_description): """ @@ -123,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. @@ -250,36 +233,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 +281,174 @@ 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 automatically be assigned." + self.assertEqual(message, self.cohort_management_page.assignment_settings_message) + + def test_cohort_enable_disable(self): + """ + Scenario: Cohort Enable/Disable checkbox related functionality is working as intended. + + Given I have a cohorted course with a user. + And I can see the `Enable Cohorts` checkbox is checked. + And cohort management controls are visible. + When I uncheck the `Enable Cohorts` checkbox. + Then cohort management controls are not visible. + And When I reload the page. + Then I can see the `Enable Cohorts` checkbox is unchecked. + And cohort management controls are not visible. + """ + self.assertTrue(self.cohort_management_page.is_cohorted) + self.assertTrue(self.cohort_management_page.cohort_management_controls_visible()) + self.cohort_management_page.is_cohorted = False + self.assertFalse(self.cohort_management_page.cohort_management_controls_visible()) + self.browser.refresh() + self.cohort_management_page.wait_for_page() + self.assertFalse(self.cohort_management_page.is_cohorted) + self.assertFalse(self.cohort_management_page.cohort_management_controls_visible()) + def test_link_to_data_download(self): """ Scenario: a link is present from the cohort configuration in @@ -462,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): """ @@ -504,8 +952,7 @@ class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): # go to the membership page on the instructor dashboard self.instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id) self.instructor_dashboard_page.visit() - membership_page = self.instructor_dashboard_page.select_membership() - self.cohort_management_page = membership_page.select_cohort_management_section() + self.cohort_management_page = self.instructor_dashboard_page.select_cohort_management() def test_no_content_group_linked(self): """ diff --git a/common/test/acceptance/tests/test_cohorted_courseware.py b/common/test/acceptance/tests/test_cohorted_courseware.py index 00f42cd14e..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): """ @@ -154,8 +139,7 @@ class EndToEndCohortedCoursewareTest(ContainerBase): """ instructor_dashboard_page = InstructorDashboardPage(self.browser, self.course_id) instructor_dashboard_page.visit() - membership_page = instructor_dashboard_page.select_membership() - cohort_management_page = membership_page.select_cohort_management_section() + cohort_management_page = instructor_dashboard_page.select_cohort_management() def add_cohort_with_student(cohort_name, content_group, student): cohort_management_page.add_cohort(cohort_name, content_group=content_group) @@ -220,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/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 7242c12879..bbeb7dba09 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: 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, 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), - (ModuleStoreEnum.Type.split, 50, 3, 3), + (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,6 +329,8 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): num_thread_responses, num_uncached_mongo_calls, num_cached_mongo_calls, + num_uncached_sql_queries, + num_cached_sql_queries, mock_request ): with modulestore().default_store(default_store): @@ -370,15 +372,16 @@ 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 check_mongo_calls(expected_calls): - call_single_thread() + 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 79d4cadf0e..81cff7d477 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -1,17 +1,24 @@ # -*- 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 +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 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 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -174,12 +181,13 @@ 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 = {} 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): @@ -193,6 +201,15 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase): **kwargs ) + 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, self.instructor, cohorted_if_in_list, exclude_unstarted), + expected + ) + def test_empty(self): self.assert_category_map_equals({"entries": {}, "subcategories": {}, "children": []}) @@ -218,20 +235,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): @@ -256,6 +288,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") @@ -352,11 +463,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): @@ -381,8 +492,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) @@ -420,6 +531,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 0a139850fa..d502a4c128 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 @@ -60,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. @@ -70,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)) ] @@ -145,24 +145,63 @@ 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) 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 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,29 +257,38 @@ 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": 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 # 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) - 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 @@ -402,7 +459,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, 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 @@ -411,6 +468,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', @@ -450,14 +513,18 @@ def prepare_content(content, course_key, is_staff=False): else: del endorsement["user_id"] + 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 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/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 512283db94..5b44f8ace2 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -65,9 +65,7 @@ def instructor_dashboard_2(request, course_id): 'finance_admin': CourseFinanceAdminRole(course_key).has_user(request.user), 'sales_admin': CourseSalesAdminRole(course_key).has_user(request.user), 'staff': has_access(request.user, 'staff', course), - 'forum_admin': has_forum_access( - request.user, course_key, FORUM_ROLE_ADMINISTRATOR - ), + 'forum_admin': has_forum_access(request.user, course_key, FORUM_ROLE_ADMINISTRATOR), } if not access['staff']: @@ -76,6 +74,7 @@ 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), @@ -330,9 +329,24 @@ def _section_membership(course, access): 'modify_access_url': reverse('modify_access', kwargs={'course_id': unicode(course_key)}), 'list_forum_members_url': reverse('list_forum_members', kwargs={'course_id': unicode(course_key)}), 'update_forum_role_membership_url': reverse('update_forum_role_membership', kwargs={'course_id': unicode(course_key)}), - 'cohorts_ajax_url': reverse('cohorts', kwargs={'course_key_string': unicode(course_key)}), - 'advanced_settings_url': get_studio_url(course, 'settings/advanced'), + } + return section_data + + +def _section_cohort_management(course, access): + """ Provide data for the corresponding cohort management section """ + course_key = course.id + section_data = { + 'section_key': 'cohort_management', + 'section_display_name': _('Cohorts'), + 'access': access, + 'course_cohort_settings_url': reverse( + 'course_cohort_settings', + kwargs={'course_key_string': unicode(course_key)} + ), + 'cohorts_url': reverse('cohorts', kwargs={'course_key_string': unicode(course_key)}), '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/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/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee b/lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee index fa7c0d353f..3334bd8fb8 100644 --- a/lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee +++ b/lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee @@ -176,6 +176,9 @@ setup_instructor_dashboard_sections = (idash_content) -> , constructor: window.InstructorDashboard.sections.Metrics $element: idash_content.find ".#{CSS_IDASH_SECTION}#metrics" + , + constructor: window.InstructorDashboard.sections.CohortManagement + $element: idash_content.find ".#{CSS_IDASH_SECTION}#cohort_management" ] sections_to_initialize.map ({constructor, $element}) -> diff --git a/lms/static/js/edxnotes/views/toggle_notes_factory.js b/lms/static/js/edxnotes/views/toggle_notes_factory.js index 7a37788dd2..8fdc5fc071 100644 --- a/lms/static/js/edxnotes/views/toggle_notes_factory.js +++ b/lms/static/js/edxnotes/views/toggle_notes_factory.js @@ -2,7 +2,7 @@ 'use strict'; define([ 'jquery', 'underscore', 'backbone', 'gettext', - 'annotator_1.2.9', 'js/edxnotes/views/visibility_decorator' + 'annotator_1.2.9', 'js/edxnotes/views/visibility_decorator', 'js/utils/animation' ], function($, _, Backbone, gettext, Annotator, EdxnotesVisibilityDecorator) { var ToggleNotesView = Backbone.View.extend({ events: { @@ -31,7 +31,7 @@ define([ toggleHandler: function (event) { event.preventDefault(); this.visibility = !this.visibility; - this.showActionMessage(); + AnimationUtil.triggerAnimation(this.actionToggleMessage); this.toggleNotes(this.visibility); }, @@ -51,13 +51,6 @@ define([ this.sendRequest(); }, - showActionMessage: function () { - // The following lines are necessary to re-trigger the CSS animation on span.action-toggle-message - this.actionToggleMessage.removeClass('is-fleeting'); - this.actionToggleMessage.offset().width = this.actionToggleMessage.offset().width; - this.actionToggleMessage.addClass('is-fleeting'); - }, - enableNotes: function () { _.each($('.edx-notes-wrapper'), EdxnotesVisibilityDecorator.enableNote); this.actionLink.addClass('is-active'); 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 new file mode 100644 index 0000000000..74be792614 --- /dev/null +++ b/lms/static/js/groups/models/course_cohort_settings.js @@ -0,0 +1,17 @@ +var edx = edx || {}; + +(function(Backbone) { + 'use strict'; + + edx.groups = edx.groups || {}; + + edx.groups.CourseCohortSettingsModel = Backbone.Model.extend({ + idAttribute: 'id', + defaults: { + is_cohorted: false, + cohorted_inline_discussions: [], + cohorted_course_wide_discussions:[], + always_cohort_inline_discussions: true + } + }); +}).call(this, Backbone); 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 630f8dec5f..8795c9baf5 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,12 @@ var edx = edx || {}; return this; }, + renderGroupHeader: function() { + this.$('.cohort-management-group-header').html(this.groupHeaderTemplate({ + cohort: this.model + })); + }, + selectTab: function(event) { var tabElement = $(event.currentTarget), tabName = tabElement.data('tab'); @@ -53,13 +62,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/groups/views/cohorts.js b/lms/static/js/groups/views/cohorts.js index 4815eaa567..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, - NotificationModel, NotificationView, FileUploaderView) { + CourseCohortSettingsNotificationView, NotificationModel, NotificationView, FileUploaderView, + InlineDiscussionsView, CourseWideDiscussionsView) { 'use strict'; var hiddenClass = 'is-hidden', @@ -12,11 +13,13 @@ var edx = edx || {}; edx.groups.CohortsView = Backbone.View.extend({ events : { 'change .cohort-select': 'onCohortSelected', + 'change .cohorts-state': 'onCohortsEnabledChanged', 'click .action-create': 'showAddCohortForm', '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) { @@ -26,19 +29,21 @@ var edx = edx || {}; this.selectorTemplate = _.template($('#cohort-selector-tpl').text()); this.context = options.context; this.contentGroups = options.contentGroups; + this.cohortSettings = options.cohortSettings; model.on('sync', this.onSync, this); - // Update cohort counts when the user clicks back on the membership tab + // Update cohort counts when the user clicks back on the cohort management tab // (for example, after uploading a csv file of cohort assignments and then // checking results on data download tab). - $(this.getSectionCss('membership')).click(function () { + $(this.getSectionCss('cohort_management')).click(function () { model.fetch(); }); }, render: function() { this.$el.html(this.template({ - cohorts: this.model.models + cohorts: this.model.models, + cohortsEnabled: this.cohortSettings.get('is_cohorted') })); this.onSync(); return this; @@ -51,6 +56,14 @@ var edx = edx || {}; })); }, + renderCourseCohortSettingsNotificationView: function() { + var cohortStateMessageNotificationView = new CourseCohortSettingsNotificationView({ + el: $('.cohort-state-message'), + cohortEnabled: this.getCohortsEnabled() + }); + cohortStateMessageNotificationView.render(); + }, + onSync: function(model, response, options) { var selectedCohort = this.lastSelectedCohortId && this.model.get(this.lastSelectedCohortId), hasCohorts = this.model.length > 0, @@ -98,6 +111,34 @@ var edx = edx || {}; this.showCohortEditor(selectedCohort); }, + onCohortsEnabledChanged: function(event) { + event.preventDefault(); + this.saveCohortSettings(); + }, + + saveCohortSettings: function() { + var self = this, + cohortSettings, + fieldData = {is_cohorted: this.getCohortsEnabled()}; + cohortSettings = this.cohortSettings; + cohortSettings.save( + fieldData, {patch: true, wait: true} + ).done(function() { + self.render(); + self.renderCourseCohortSettingsNotificationView(); + }).fail(function(result) { + self.showNotification({ + type: 'error', + title: gettext("We've encountered an error. Refresh your browser and then try again.")}, + self.$('.cohorts-state-section') + ); + }); + }, + + getCohortsEnabled: function() { + return this.$('.cohorts-state').prop('checked'); + }, + showCohortEditor: function(cohort) { this.removeNotification(); if (this.editor) { @@ -167,9 +208,11 @@ var edx = edx || {}; setCohortEditorVisibility: function(showEditor) { if (showEditor) { + this.$('.cohorts-state-section').removeClass(disabledClass).attr('aria-disabled', false); this.$('.cohort-management-group').removeClass(hiddenClass); this.$('.cohort-management-nav').removeClass(disabledClass).attr('aria-disabled', false); } else { + this.$('.cohorts-state-section').addClass(disabledClass).attr('aria-disabled', true); this.$('.cohort-management-group').addClass(hiddenClass); this.$('.cohort-management-nav').addClass(disabledClass).attr('aria-disabled', true); } @@ -236,10 +279,32 @@ 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 + "']"; } }); }).call(this, $, _, Backbone, gettext, interpolate_text, edx.groups.CohortModel, edx.groups.CohortEditorView, - edx.groups.CohortFormView, NotificationModel, NotificationView, FileUploaderView); + edx.groups.CohortFormView, edx.groups.CourseCohortSettingsNotificationView, NotificationModel, NotificationView, + FileUploaderView, edx.groups.InlineDiscussionsView, edx.groups.CourseWideDiscussionsView); diff --git a/lms/static/js/groups/views/cohorts_dashboard_factory.js b/lms/static/js/groups/views/cohorts_dashboard_factory.js new file mode 100644 index 0000000000..3ddf229220 --- /dev/null +++ b/lms/static/js/groups/views/cohorts_dashboard_factory.js @@ -0,0 +1,41 @@ +;(function (define, undefined) { + 'use strict'; + 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(), + 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'), + studioGroupConfigurationsUrl: studioGroupConfigurationsUrl + } + }); + + cohorts.fetch().done(function() { + courseCohortSettings.fetch().done(function() { + discussionTopicsSettings.fetch().done(function() { + cohortsView.render(); + }); + }); + }); + }; + }); +}).call(this, define || RequireJS.define); + diff --git a/lms/static/js/groups/views/course_cohort_settings_notification.js b/lms/static/js/groups/views/course_cohort_settings_notification.js new file mode 100644 index 0000000000..e675e3440c --- /dev/null +++ b/lms/static/js/groups/views/course_cohort_settings_notification.js @@ -0,0 +1,31 @@ +var edx = edx || {}; + +(function($, _, Backbone, gettext) { + 'use strict'; + + edx.groups = edx.groups || {}; + + edx.groups.CourseCohortSettingsNotificationView = Backbone.View.extend({ + initialize: function(options) { + this.template = _.template($('#cohort-state-tpl').text()); + this.cohortEnabled = options.cohortEnabled; + }, + + render: function() { + this.$el.html(this.template({})); + this.showCohortStateMessage(); + return this; + }, + + showCohortStateMessage: function () { + var actionToggleMessage = this.$('.action-toggle-message'); + + AnimationUtil.triggerAnimation(actionToggleMessage); + if (this.cohortEnabled) { + actionToggleMessage.text(gettext('Cohorts Enabled')); + } else { + actionToggleMessage.text(gettext('Cohorts Disabled')); + } + } + }); +}).call(this, $, _, Backbone, gettext); diff --git a/lms/static/js/instructor_dashboard/cohort_management.js b/lms/static/js/instructor_dashboard/cohort_management.js new file mode 100644 index 0000000000..abe725f21a --- /dev/null +++ b/lms/static/js/instructor_dashboard/cohort_management.js @@ -0,0 +1,19 @@ +(function() { + var CohortManagement; + + CohortManagement = (function() { + + function CohortManagement($section) { + this.$section = $section; + this.$section.data('wrapper', this); + } + + CohortManagement.prototype.onClickTitle = function() {}; + + return CohortManagement; + + })(); + + window.InstructorDashboard.sections.CohortManagement = CohortManagement; + +}).call(this); diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js index a2356b5f20..175b43f369 100644 --- a/lms/static/js/spec/groups/views/cohorts_spec.js +++ b/lms/static/js/spec/groups/views/cohorts_spec.js @@ -1,25 +1,45 @@ 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'], - function (Backbone, $, AjaxHelpers, TemplateHelpers, CohortsView, CohortCollection, ContentGroupModel) { + 'js/groups/views/cohorts', 'js/groups/collections/cohort', 'js/groups/models/content_group', + '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, Qubit, CourseCohortSettingsNotificationView, DiscussionTopicsSettingsModel, + CohortDiscussionsView, CohortCourseWideDiscussionsView, CohortInlineDiscussionsView) { 'use strict'; describe("Cohorts View", function () { var catLoversInitialCount = 123, dogLoversInitialCount = 456, unknownUserMessage, - createMockCohort, createMockCohorts, createMockContentGroups, createCohortsView, cohortsView, - 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; + createMockCohort, createMockCohorts, createMockContentGroups, createMockCohortSettingsJson, + createCohortsView, cohortsView, requests, respondToRefresh, verifyMessage, verifyNoMessage, + verifyDetailedMessage, verifyHeader, expectCohortAddRequest, getAddModal, selectContentGroup, + clearContentGroup, saveFormAndExpectErrors, createMockCohortSettings, MOCK_COHORTED_USER_PARTITION_ID, + MOCK_UPLOAD_COHORTS_CSV_URL, MOCK_STUDIO_ADVANCED_SETTINGS_URL, MOCK_STUDIO_GROUP_CONFIGURATIONS_URL, + MOCK_MANUAL_ASSIGNMENT, MOCK_RANDOM_ASSIGNMENT, 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'; 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 @@ -46,23 +66,95 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe ]; }; + createMockCohortSettingsJson = function (isCohorted, cohortedInlineDiscussions, cohortedCourseWideDiscussions, alwaysCohortInlineDiscussions) { + return { + id: 0, + is_cohorted: isCohorted || false, + cohorted_inline_discussions: cohortedInlineDiscussions || [], + cohorted_course_wide_discussions: cohortedCourseWideDiscussions || [], + always_cohort_inline_discussions: alwaysCohortInlineDiscussions || true + }; + }; + + createMockCohortSettings = function (isCohorted, cohortedInlineDiscussions, cohortedCourseWideDiscussions, alwaysCohortInlineDiscussions) { + return new CourseCohortSettingsModel( + 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; + var cohortsJson, cohorts, contentGroups, cohortSettings, cohortDiscussions; options = options || {}; cohortsJson = options.cohorts ? {cohorts: options.cohorts} : createMockCohorts(); cohorts = new CohortCollection(cohortsJson, {parse: true}); contentGroups = options.contentGroups || createMockContentGroups(); + 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(); @@ -73,13 +165,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 +222,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 +239,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) { @@ -164,17 +261,58 @@ 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; }; beforeEach(function () { - setFixtures('
'); + setFixtures('
'); TemplateHelpers.installTemplate('templates/instructor/instructor_dashboard_2/cohorts'); 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/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'); }); @@ -188,12 +326,14 @@ 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() { createCohortsView(this, {selectCohort: 1}); verifyHeader(1, 'Cat Lovers', catLoversInitialCount); - $(cohortsView.getSectionCss("membership")).click(); + $(cohortsView.getSectionCss("cohort_management")).click(); AjaxHelpers.expectRequest(requests, 'GET', '/mock_service/cohorts'); respondToRefresh(1001, 2); verifyHeader(1, 'Cat Lovers', 1001); @@ -227,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); @@ -246,6 +390,112 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); }); + describe("Course Cohort Settings", function () { + it('can enable and disable cohorting', function () { + createCohortsView(this, {cohortSettings: createMockCohortSettings(false)}); + + expect(cohortsView.$('.cohorts-state').prop('checked')).toBeFalsy(); + + cohortsView.$('.cohorts-state').prop('checked', true).change(); + AjaxHelpers.expectJsonRequest( + requests, 'PATCH', '/mock_service/cohorts/settings', + {is_cohorted: true} + ); + AjaxHelpers.respondWithJson( + requests, + {is_cohorted: true} + ); + expect(cohortsView.$('.cohorts-state').prop('checked')).toBeTruthy(); + + cohortsView.$('.cohorts-state').prop('checked', false).change(); + AjaxHelpers.expectJsonRequest( + requests, 'PATCH', '/mock_service/cohorts/settings', + {is_cohorted: false} + ); + AjaxHelpers.respondWithJson( + requests, + {is_cohorted: false} + ); + expect(cohortsView.$('.cohorts-state').prop('checked')).toBeFalsy(); + }); + + + it('shows an appropriate cohort status message', function () { + var createCourseCohortSettingsNotificationView = function (is_cohorted) { + var notificationView = new CourseCohortSettingsNotificationView({ + el: $('.cohort-state-message'), + cohortEnabled: is_cohorted}); + notificationView.render(); + return notificationView; + }; + + var notificationView = createCourseCohortSettingsNotificationView(true); + expect(notificationView.$('.action-toggle-message').text().trim()).toBe('Cohorts Enabled'); + + notificationView = createCourseCohortSettingsNotificationView(false); + expect(notificationView.$('.action-toggle-message').text().trim()).toBe('Cohorts Disabled'); + }); + + it('shows an appropriate error message for HTTP500', function () { + createCohortsView(this, {cohortSettings: createMockCohortSettings(false)}); + expect(cohortsView.$('.cohorts-state').prop('checked')).toBeFalsy(); + cohortsView.$('.cohorts-state').prop('checked', true).change(); + AjaxHelpers.respondWithError(requests, 500); + var expectedTitle = "We've encountered an error. Refresh your browser and then try again." + expect(cohortsView.$('.message-title').text().trim()).toBe(expectedTitle); + }); + }); + + describe("Cohort Group Header", function () { + 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 +517,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 +528,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 +837,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 +872,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 +887,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 +901,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 +925,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 +940,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,7 +979,471 @@ 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 automatically be 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'); + }); }); }); + + 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 caf1f418a8..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', @@ -67,6 +68,12 @@ 'js/views/notification': 'js/views/notification', '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', 'js/groups/views/cohort_form': 'js/groups/views/cohort_form', @@ -294,6 +301,30 @@ exports: 'edx.groups.ContentGroupModel', deps: ['backbone'] }, + 'js/groups/models/course_cohort_settings': { + 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'] + }, 'js/groups/collections/cohort': { exports: 'edx.groups.CohortCollection', deps: ['backbone', 'js/groups/models/cohort'] diff --git a/lms/static/js/utils/animation.js b/lms/static/js/utils/animation.js new file mode 100644 index 0000000000..dcc232e93d --- /dev/null +++ b/lms/static/js/utils/animation.js @@ -0,0 +1,15 @@ +(function() { + this.AnimationUtil = (function() { + function AnimationUtil() {} + AnimationUtil.triggerAnimation = function(messageElement) { + // The following lines are necessary to re-trigger the CSS animation on span.action-toggle-message + // To see how it works, please see `Another JavaScript Method to Restart a CSS Animation` + // at https://css-tricks.com/restart-css-animation/ + messageElement.removeClass('is-fleeting'); + messageElement.offset().width = messageElement.offset().width; + messageElement.addClass('is-fleeting'); + }; + return AnimationUtil; + }).call(this); +}).call(this); + 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 89368b2e28..f5ad96107a 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -435,300 +435,6 @@ } } - // cohort management - %cohort-management-form { - - .form-fields { - - .label, - .form-label, - .input, - .tip { - display: block; - } - - .label, - .form-label { - @extend %t-title7; - @extend %t-weight4; - margin-bottom: ($baseline/2); - } - - .tip { - @extend %t-copy-sub1; - margin-top: ($baseline/4); - color: $gray-l2; - } - - .field-text { - - // needed to reset poor input styling - input[type="text"] { - height: auto; - } - - .input { - width: 100%; - padding: ($baseline/2) ($baseline*0.75); - } - } - - .input-file { - margin-bottom: ($baseline/2); - } - } - - .form-submit, .form-cancel { - display: inline-block; - 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; - } - } - - .cohort-management-nav { - @include clearfix(); - - .cohort-management-nav-form { - width: 60%; - @include float(left); - } - - .cohort-select { - width: 100%; - margin-top: ($baseline/4); - } - - .action-create { - @include idashbutton($blue); - @extend %t-weight4; - @include float(right); - @include text-align(right); - text-shadow: none; - } - - // STATE: is disabled - &.is-disabled { - - - .cohort-select { - opacity: 0.25; - } - - .action-create { - opacity: 0.50; - } - } - } - - .cohort-management { - - // specific message actions - .message .action-create { - @include idashbutton($blue); - } - } - - // create or edit cohort - .cohort-management-settings, - .cohort-management-edit { - @extend %cohort-management-form; - margin-bottom: $baseline; - - .form-title { - @extend %t-title5; - @extend %t-weight4; - padding: $baseline; - background: $gray-l5; - border-bottom: ($baseline/10) solid $gray-l4; - } - - .form-field { - // padding: $baseline; - } - - .form-actions { - padding: $baseline 0; - - &.new-cohort-form { - padding: $baseline; - } - } - } - - // cohort - .cohort-management-group-header { - padding: $baseline; - border-bottom: ($baseline/10) solid $gray-l4; - background: $gray-l5; - - .group-header-title { - margin-bottom: ($baseline/2); - padding-bottom: ($baseline/2); - border-bottom: 1px solid $gray-l4; - - &:hover, &:active, &:focus { - - .action-edit-name { - opacity: 1.0; - pointer-events: auto; - } - } - } - - .title-value, .group-count, .action-edit { - display: inline-block; - vertical-align: middle; - } - - .title-value { - @extend %t-title5; - @extend %t-weight4; - @include margin-right($baseline/4); - } - - .group-count { - @extend %t-title7; - @extend %t-weight4; - } - - .action-edit-name { - @include idashbutton($gray-l3); - @include transition(opacity $tmg-f2 ease-in-out); - @include font-size(14); - @include line-height(14); - @include margin-left($baseline/2); - padding: ($baseline/4) ($baseline/2); - margin-bottom: 0; - opacity: 0.0; - pointer-events: none; - } - } - - .cohort-management-group-setup { - @include clearfix(); - @extend %t-copy-sub1; - color: $gray-l2; - - .setup-value { - @include float(left); - @include margin-right(5%); - width: 70%; - } - - .setup-actions { - @include float(right); - @include text-align(right); - width: 20%; - } - } - - .cohort-management-group-add { - @extend %cohort-management-form; - border: 1px solid $gray-l5; - - .message-title { - @extend %t-title7; - } - - .form-title { - @extend %t-title6; - @extend %t-weight4; - margin-bottom: ($baseline/4); - padding: 0; - border: none; - background: transparent; - } - - .form-introduction { - @extend %t-copy-sub1; - margin-bottom: $baseline; - - p { - color: $gray-l1; - } - } - - .form-fields { - padding: $baseline 0; - } - - .form-actions { - padding: 0 0 $baseline 0; - } - - .cohort-management-group-add-students { - min-height: ($baseline*10); - width: 100%; - padding: ($baseline/2) ($baseline*0.75); - } - } - - // CSV-based file upload for auto cohort assigning - .toggle-cohort-management-secondary { - @extend %t-copy-sub1; - } - - .cohort-management-file-upload { - - .message-title { - @extend %t-title7; - } - - .form-introduction { - @extend %t-copy-sub1; - margin-bottom: $baseline; - - p { - color: $gray-l1; - } - } - } - - .file-upload-form { - @extend %cohort-management-form; - - .form-fields { - margin-bottom: $baseline; - } - - .action-submit { - @include idashbutton($blue); - // needed to override very poor specificity and base rules for blue button - @include font-size(14); - margin-bottom: 0; - font-weight: 700; - text-shadow: none; - } - } - - .cohort-management-supplemental { - @extend %t-copy-sub1; - margin-top: $baseline; - padding: ($baseline/2) $baseline; - background: $gray-l5; - border-radius: ($baseline/10); - - - .icon { - @include margin-right($baseline/4); - color: $gray-l1; - } - } - - - .batch-enrollment, .batch-beta-testers { textarea { margin-top: 0.2em; @@ -844,7 +550,6 @@ display: block; } - label[for="email-students"]:hover + .email-students-hint { display: block; } @@ -965,6 +670,311 @@ } } } +} + + +// view - cohort management +// -------------------- +.instructor-dashboard-wrapper-2 section.idash-section#cohort_management { + + // cohort management + %cohort-management-form { + + .form-fields { + + .label, + .form-label, + .input, + .tip { + display: block; + } + + .label, + .form-label { + @extend %t-title7; + @extend %t-weight4; + margin-bottom: ($baseline/2); + } + + .tip { + @extend %t-copy-sub1; + margin-top: ($baseline/4); + color: $gray-l2; + } + + .field-text { + + // needed to reset poor input styling + input[type="text"] { + height: auto; + } + + .input { + width: 100%; + padding: ($baseline/2) ($baseline*0.75); + } + } + + .input-file { + margin-bottom: ($baseline/2); + } + } + + .form-submit, .form-cancel { + display: inline-block; + vertical-align: middle; + } + + .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(); + + .cohort-management-nav-form { + width: 60%; + @include float(left); + } + + .cohort-select { + width: 100%; + margin-top: ($baseline/4); + } + + .action-create { + @include idashbutton($blue); + @extend %t-weight4; + @include float(right); + @include text-align(right); + text-shadow: none; + } + + // STATE: is disabled + &.is-disabled { + + + .cohort-select { + opacity: 0.25; + } + + .action-create { + opacity: 0.50; + } + } + } + + .cohort-management { + + // specific message actions + .message .action-create { + @include idashbutton($blue); + } + } + + // create or edit cohort + .cohort-management-settings, + .cohort-management-edit { + @extend %cohort-management-form; + margin-bottom: $baseline; + + .form-title { + @extend %t-title5; + @extend %t-weight4; + padding: $baseline; + background: $gray-l5; + border-bottom: ($baseline/10) solid $gray-l4; + } + + .form-field { + // padding: $baseline; + } + + .form-actions { + padding: $baseline 0; + + &.new-cohort-form { + padding: $baseline; + } + } + } + + .cohort-management-assignment-type-settings, + .cohorts-state-section { + &.is-disabled { + opacity: 0.25; + } + } + + // cohort + .cohort-management-group-header { + padding: $baseline; + border-bottom: ($baseline/10) solid $gray-l4; + background: $gray-l5; + + .group-header-title { + margin-bottom: ($baseline/2); + padding-bottom: ($baseline/2); + border-bottom: 1px solid $gray-l4; + + &:hover, &:active, &:focus { + + .action-edit-name { + opacity: 1.0; + pointer-events: auto; + } + } + } + + .title-value, .group-count, .action-edit { + display: inline-block; + vertical-align: middle; + } + + .title-value { + @extend %t-title5; + @extend %t-weight4; + @include margin-right($baseline/4); + } + + .group-count { + @extend %t-title7; + @extend %t-weight4; + } + + .action-edit-name { + @include idashbutton($gray-l3); + @include transition(opacity $tmg-f2 ease-in-out); + @include font-size(14); + @include line-height(14); + @include margin-left($baseline/2); + padding: ($baseline/4) ($baseline/2); + margin-bottom: 0; + opacity: 0.0; + pointer-events: none; + } + } + + .cohort-management-group-setup { + @include clearfix(); + @extend %t-copy-sub1; + color: $gray-l2; + + .setup-value { + @include float(left); + @include margin-right(5%); + width: 70%; + } + + .setup-actions { + @include float(right); + @include text-align(right); + width: 20%; + } + } + + .cohort-management-group-add { + @extend %cohort-management-form; + border: 1px solid $gray-l5; + + .message-title { + @extend %t-title7; + } + + .form-title { + @extend %t-title6; + @extend %t-weight4; + margin-bottom: ($baseline/4); + padding: 0; + border: none; + background: transparent; + } + + .form-introduction { + @extend %t-copy-sub1; + margin-bottom: $baseline; + + p { + color: $gray-l1; + } + } + + .form-fields { + padding: $baseline 0; + } + + .form-actions { + padding: 0 0 $baseline 0; + } + + .cohort-management-group-add-students { + min-height: ($baseline*10); + width: 100%; + padding: ($baseline/2) ($baseline*0.75); + } + } + + // 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; + } + + .cohort-management-file-upload { + + .message-title { + @extend %t-title7; + } + + .form-introduction { + @extend %t-copy-sub1; + margin-bottom: $baseline; + + p { + color: $gray-l1; + } + } + } + + .file-upload-form { + @extend %cohort-management-form; + + .form-fields { + margin-bottom: $baseline; + } + + .action-submit { + @include idashbutton($blue); + // needed to override very poor specificity and base rules for blue button + @include font-size(14); + margin-bottom: 0; + font-weight: 700; + text-shadow: none; + } + } + + .cohort-management-supplemental { + @extend %t-copy-sub1; + margin-top: $baseline; + padding: ($baseline/2) $baseline; + background: $gray-l5; + border-radius: ($baseline/10); + + + .icon { + @include margin-right($baseline/4); + color: $gray-l1; + } + } .has-other-input-text { // Given to groups which have an 'other' input that appears when needed display: inline-block; @@ -1061,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; @@ -1112,6 +1165,7 @@ } } + // view - student admin // -------------------- .instructor-dashboard-wrapper-2 section.idash-section#student_admin > { 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/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 @@ +
  • +
    + +
    + + +
    • 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') %>

    +
    +
    +
    +
    +
    +

    <%- gettext('Course-Wide Discussion Topics') %>

    +

    <%- gettext('Select the course-wide discussion topics that you want to divide by cohort.') %>

    +
    +
      <%= courseWideTopics %>
    +
    +
    +
    +
    +
    + +
    +
    +
    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 @@ +
    + +
    +
    +
    +
    +
    +

    <%- gettext('Content-Specific Discussion Topics') %>

    +

    <%- gettext('Specify whether content-specific discussion topics are divided by cohort.') %>

    +
    + +
    +
    + +
    +
    +
    + <% if ( inlineDiscussionTopics ) { %> +
      <%= inlineDiscussionTopics %>
    + <% } else { %> + <%- gettext('No content-specific discussion topics exist.') %> + <% } %> +
    +
    +
    +
    +
    +
    + +
    +
    +
    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 @@ +
  • +
    + +
    +
    • 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 @@
    -
    -

    - <%- cohort.get('name') %> - <%- - interpolate( - ngettext('(contains %(student_count)s student)', '(contains %(student_count)s students)', cohort.get('user_count')), - { student_count: cohort.get('user_count') }, - true - ) - %> -

    -
    -
    - <% if (cohort.get('assignment_type') == "none") { %> - <%- gettext("Students are added to this cohort only when you provide their email addresses or usernames on this page.") %> - <%= gettext("What does this mean?") %> - <% } else { %> - <%- gettext("Students are added to this cohort automatically.") %> - <%- gettext("What does this mean?") %> - <% } %> -
    -
    - <% if (studioAdvancedSettingsUrl !== "None") { %> - <%- gettext("Edit settings in Studio") %> - <% } %> -
    -
    -
    +
    + diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index b53b47e987..d7ef87af5d 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -245,53 +245,3 @@ %endif - -% if course.is_cohorted: -
    -
    -
    - - <%block name="headextra"> - <% - cohorted_user_partition = get_cohorted_user_partition(course.id) - content_groups = cohorted_user_partition.groups if cohorted_user_partition else [] - %> - - - -% endif diff --git a/lms/urls.py b/lms/urls.py index b6de54a1cf..97a9a74e4e 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -379,6 +379,9 @@ if settings.COURSEWARE_ENABLED: 'open_ended_grading.views.take_action_on_flags', name='open_ended_flagged_problems_take_action'), # Cohorts management + url(r'^courses/{}/cohorts/settings$'.format(settings.COURSE_KEY_PATTERN), + 'openedx.core.djangoapps.course_groups.views.course_cohort_settings_handler', + name="course_cohort_settings"), url(r'^courses/{}/cohorts/(?P[0-9]+)?$'.format(settings.COURSE_KEY_PATTERN), 'openedx.core.djangoapps.course_groups.views.cohort_handler', name="cohorts"), url(r'^courses/{}/cohorts/(?P[0-9]+)$'.format(settings.COURSE_KEY_PATTERN), @@ -393,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/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 442ea506f4..863dcd0096 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -14,8 +14,10 @@ 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, CourseUserGroupPartitionGroup + +from .models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CourseUserGroupPartitionGroup log = logging.getLogger(__name__) @@ -71,42 +73,14 @@ 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" -class CohortAssignmentType(object): - """ - The various types of rule-based cohorts - """ - # No automatic rules are applied to this cohort; users must be manually added. - NONE = "none" - - # One of (possibly) multiple cohorts to which users are randomly assigned. - # Note: The 'default' cohort is included in this category iff it exists and - # there are no other random groups. (Also see Note 2 above.) - RANDOM = "random" - - @staticmethod - def get(cohort, course): - """ - Returns the assignment type of the given cohort for the given course - """ - if cohort.name in course.auto_cohort_groups: - return CohortAssignmentType.RANDOM - elif len(course.auto_cohort_groups) == 0 and cohort.name == DEFAULT_COHORT_NAME: - return CohortAssignmentType.RANDOM - else: - return CohortAssignmentType.NONE - - # tl;dr: global state is bad. capa reseeds random every time a problem is loaded. Even # if and when that's fixed, it's a good idea to have a local generator to avoid any other # code that messes with the global random module. @@ -135,15 +109,15 @@ 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): +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 @@ -160,18 +134,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 @@ -187,27 +162,32 @@ 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 @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. + 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 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 @@ -216,68 +196,104 @@ def get_cohort(user, course_key, assign=True): Raises: ValueError if the CourseKey doesn't exist. """ + 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) - try: - course = courses.get_course_by_id(course_key) - except Http404: - raise ValueError("Invalid course_key") - - if not course.is_cohorted: - return None + course_cohort_settings = get_course_cohort_settings(course_key) + if not course_cohort_settings.is_cohorted: + return request_cache.data.setdefault(cache_key, None) + # If course is cohorted, check if the user already has a cohort. try: - return CourseUserGroup.objects.get( + cohort = CourseUserGroup.objects.get( course_id=course_key, group_type=CourseUserGroup.COHORT, users__id=user.id, ) + 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 - choices = course.auto_cohort_groups - if len(choices) > 0: - # Randomly choose one of the auto_cohort_groups, creating it if needed. - group_name = local_random().choice(choices) + # 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) else: - # Use the "default cohort". - group_name = DEFAULT_COHORT_NAME + cohort = CourseCohort.create( + cohort_name=DEFAULT_COHORT_NAME, + course_id=course_key, + assignment_type=CourseCohort.RANDOM + ).course_user_group - group, __ = CourseUserGroup.objects.get_or_create( - course_id=course_key, - group_type=CourseUserGroup.COHORT, - name=group_name + user.course_groups.add(cohort) + + return request_cache.data.setdefault(cache_key, cohort) + + +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. + """ + cohort_settings, created = CourseCohortsSettings.objects.get_or_create( + course_id=course.id, + defaults={ + 'is_cohorted': course.is_cohorted, + 'cohorted_discussions': list(course.cohorted_discussions), + 'always_cohort_inline_discussions': course.always_cohort_inline_discussions + } ) - user.course_groups.add(group) - return group + + # Add the new and update the existing cohorts + if created: + # Update the manual cohorts already present in CourseUserGroup + manual_cohorts = CourseUserGroup.objects.filter( + 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) + + return cohort_settings -def get_course_cohorts(course): +def get_course_cohorts(course, assignment_type=None): """ Get a list of all the cohorts in the given course. This will include auto cohorts, regardless of whether or not the auto cohorts include any users. Arguments: course: the course for which cohorts should be returned + assignment_type: cohort assignment type Returns: - A list of CourseUserGroup objects. Empty if there are no cohorts. Does + A list of CourseUserGroup objects. Empty if there are no cohorts. Does not check whether the course is cohorted. """ - # Ensure all auto cohorts are created. - for group_name in course.auto_cohort_groups: - CourseUserGroup.objects.get_or_create( - course_id=course.location.course_key, - group_type=CourseUserGroup.COHORT, - name=group_name - ) + # Migrate cohort settings for this course + migrate_cohort_settings(course) - return list(CourseUserGroup.objects.filter( + query_set = CourseUserGroup.objects.filter( course_id=course.location.course_key, group_type=CourseUserGroup.COHORT - )) + ) + query_set = query_set.filter(cohort__assignment_type=assignment_type) if assignment_type else query_set + return list(query_set) ### Helpers for cohort management views @@ -297,7 +313,7 @@ def get_cohort_by_name(course_key, name): def get_cohort_by_id(course_key, cohort_id): """ Return the CourseUserGroup object for the given cohort. Raises DoesNotExist - it isn't present. Uses the course_key for extra validation... + it isn't present. Uses the course_key for extra validation. """ return CourseUserGroup.objects.get( course_id=course_key, @@ -306,15 +322,13 @@ def get_cohort_by_id(course_key, cohort_id): ) -def add_cohort(course_key, name): +def add_cohort(course_key, name, assignment_type): """ Add a cohort to a course. Raises ValueError if a cohort of the same name already exists. """ log.debug("Adding cohort %s to %s", name, course_key) - if CourseUserGroup.objects.filter(course_id=course_key, - group_type=CourseUserGroup.COHORT, - name=name).exists(): + if is_cohort_exists(course_key, name): raise ValueError(_("You cannot create two cohorts with the same name")) try: @@ -322,11 +336,12 @@ def add_cohort(course_key, name): except Http404: raise ValueError("Invalid course_key") - cohort = CourseUserGroup.objects.create( + cohort = CourseCohort.create( + cohort_name=name, course_id=course.id, - group_type=CourseUserGroup.COHORT, - name=name - ) + assignment_type=assignment_type + ).course_user_group + tracker.emit( "edx.cohort.creation_requested", {"cohort_name": cohort.name, "cohort_id": cohort.id} @@ -334,6 +349,13 @@ def add_cohort(course_key, name): return cohort +def is_cohort_exists(course_key, name): + """ + Check if a cohort already exists. + """ + return CourseUserGroup.objects.filter(course_id=course_key, group_type=CourseUserGroup.COHORT, name=name).exists() + + def add_user_to_cohort(cohort, username_or_email): """ Look up the given user, and if successful, add them to the specified cohort. @@ -384,15 +406,112 @@ 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). 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. """ - res = CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort) - if len(res): - return res[0].group_id, res[0].partition_id - return None, None + request_cache = RequestCache.get_request_cache() + cache_key = u"cohorts.get_group_info_for_cohort.{}".format(cohort.id) + + if use_cached and cache_key in request_cache.data: + return request_cache.data[cache_key] + + request_cache.data.pop(cache_key, 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): + """ + Set assignment type for cohort. + """ + course_cohort = user_group.cohort + + if is_default_cohort(user_group) and course_cohort.assignment_type != assignment_type: + raise ValueError(_("There must be one cohort to which students can automatically be assigned.")) + + course_cohort.assignment_type = assignment_type + course_cohort.save() + + +def get_assignment_type(user_group): + """ + Get assignment type for cohort. + """ + course_cohort = user_group.cohort + return course_cohort.assignment_type + + +def is_default_cohort(user_group): + """ + Check if a cohort is default. + """ + random_cohorts = CourseUserGroup.objects.filter( + course_id=user_group.course_id, + group_type=CourseUserGroup.COHORT, + cohort__assignment_type=CourseCohort.RANDOM + ) + + return len(random_cohorts) == 1 and random_cohorts[0].name == user_group.name + + +def set_course_cohort_settings(course_key, **kwargs): + """ + Set cohort settings for a course. + + Arguments: + course_key: CourseKey + is_cohorted (bool): If the course should be cohorted. + always_cohort_inline_discussions (bool): If inline discussions should always be cohorted. + cohorted_discussions (list): List of discussion ids. + + Returns: + A CourseCohortSettings object. + + Raises: + 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) + 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 + + +def get_course_cohort_settings(course_key): + """ + Return cohort settings for a course. + + Arguments: + course_key: CourseKey + + Returns: + A CourseCohortSettings object. + + Raises: + Http404 if course_key is invalid. + """ + try: + course_cohort_settings = CourseCohortsSettings.objects.get(course_id=course_key) + except CourseCohortsSettings.DoesNotExist: + 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/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/migrations/0003_auto__add_coursecohort__add_coursecohortssettings.py b/openedx/core/djangoapps/course_groups/migrations/0003_auto__add_coursecohort__add_coursecohortssettings.py new file mode 100644 index 0000000000..765f338d9f --- /dev/null +++ b/openedx/core/djangoapps/course_groups/migrations/0003_auto__add_coursecohort__add_coursecohortssettings.py @@ -0,0 +1,108 @@ +# -*- coding: utf-8 -*- +import datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'CourseCohort' + db.create_table('course_groups_coursecohort', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('course_user_group', self.gf('django.db.models.fields.related.OneToOneField')(related_name='cohort', unique=True, to=orm['course_groups.CourseUserGroup'])), + ('assignment_type', self.gf('django.db.models.fields.CharField')(default='manual', max_length=20)), + )) + db.send_create_signal('course_groups', ['CourseCohort']) + + # Adding model 'CourseCohortsSettings' + db.create_table('course_groups_coursecohortssettings', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('is_cohorted', self.gf('django.db.models.fields.BooleanField')(default=False)), + ('course_id', self.gf('xmodule_django.models.CourseKeyField')(unique=True, max_length=255, db_index=True)), + ('cohorted_discussions', self.gf('django.db.models.fields.TextField')(null=True, blank=True)), + ('always_cohort_inline_discussions', self.gf('django.db.models.fields.BooleanField')(default=True)), + )) + db.send_create_signal('course_groups', ['CourseCohortsSettings']) + + + def backwards(self, orm): + # Deleting model 'CourseCohort' + db.delete_table('course_groups_coursecohort') + + # Deleting model 'CourseCohortsSettings' + db.delete_table('course_groups_coursecohortssettings') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, + 'course_groups.coursecohort': { + 'Meta': {'object_name': 'CourseCohort'}, + 'assignment_type': ('django.db.models.fields.CharField', [], {'default': "'manual'", 'max_length': '20'}), + 'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'cohort'", 'unique': 'True', 'to': "orm['course_groups.CourseUserGroup']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'course_groups.coursecohortssettings': { + 'Meta': {'object_name': 'CourseCohortsSettings'}, + 'always_cohort_inline_discussions': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'cohorted_discussions': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'course_id': ('xmodule_django.models.CourseKeyField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_cohorted': ('django.db.models.fields.BooleanField', [], {'default': 'False'}) + }, + 'course_groups.courseusergroup': { + 'Meta': {'unique_together': "(('name', 'course_id'),)", 'object_name': 'CourseUserGroup'}, + 'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'group_type': ('django.db.models.fields.CharField', [], {'max_length': '20'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'users': ('django.db.models.fields.related.ManyToManyField', [], {'db_index': 'True', 'related_name': "'course_groups'", 'symmetrical': 'False', 'to': "orm['auth.User']"}) + }, + 'course_groups.courseusergrouppartitiongroup': { + 'Meta': {'object_name': 'CourseUserGroupPartitionGroup'}, + 'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['course_groups.CourseUserGroup']", 'unique': 'True'}), + 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'group_id': ('django.db.models.fields.IntegerField', [], {}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'partition_id': ('django.db.models.fields.IntegerField', [], {}), + 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}) + } + } + + complete_apps = ['course_groups'] \ No newline at end of file diff --git a/openedx/core/djangoapps/course_groups/migrations/0004_auto__del_field_coursecohortssettings_cohorted_discussions__add_field_.py b/openedx/core/djangoapps/course_groups/migrations/0004_auto__del_field_coursecohortssettings_cohorted_discussions__add_field_.py new file mode 100644 index 0000000000..3863c0943c --- /dev/null +++ b/openedx/core/djangoapps/course_groups/migrations/0004_auto__del_field_coursecohortssettings_cohorted_discussions__add_field_.py @@ -0,0 +1,91 @@ +# -*- coding: utf-8 -*- +import datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Changed 'CourseCohortsSettings.cohorted_discussions' to 'CourseCohortsSettings._cohorted_discussions' without + # changing db column name + pass + + def backwards(self, orm): + # Changed 'CourseCohortsSettings.cohorted_discussions' to 'CourseCohortsSettings._cohorted_discussions' without + # changing db column name + pass + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, + 'course_groups.coursecohort': { + 'Meta': {'object_name': 'CourseCohort'}, + 'assignment_type': ('django.db.models.fields.CharField', [], {'default': "'manual'", 'max_length': '20'}), + 'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'cohort'", 'unique': 'True', 'to': "orm['course_groups.CourseUserGroup']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'course_groups.coursecohortssettings': { + 'Meta': {'object_name': 'CourseCohortsSettings'}, + '_cohorted_discussions': ('django.db.models.fields.TextField', [], {'null': 'True', 'db_column': "'cohorted_discussions'", 'blank': 'True'}), + 'always_cohort_inline_discussions': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'course_id': ('xmodule_django.models.CourseKeyField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_cohorted': ('django.db.models.fields.BooleanField', [], {'default': 'False'}) + }, + 'course_groups.courseusergroup': { + 'Meta': {'unique_together': "(('name', 'course_id'),)", 'object_name': 'CourseUserGroup'}, + 'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'group_type': ('django.db.models.fields.CharField', [], {'max_length': '20'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'users': ('django.db.models.fields.related.ManyToManyField', [], {'db_index': 'True', 'related_name': "'course_groups'", 'symmetrical': 'False', 'to': "orm['auth.User']"}) + }, + 'course_groups.courseusergrouppartitiongroup': { + 'Meta': {'object_name': 'CourseUserGroupPartitionGroup'}, + 'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['course_groups.CourseUserGroup']", 'unique': 'True'}), + 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'group_id': ('django.db.models.fields.IntegerField', [], {}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'partition_id': ('django.db.models.fields.IntegerField', [], {}), + 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}) + } + } + + complete_apps = ['course_groups'] \ No newline at end of file diff --git a/openedx/core/djangoapps/course_groups/models.py b/openedx/core/djangoapps/course_groups/models.py index da17f4c20c..3fc6828bdc 100644 --- a/openedx/core/djangoapps/course_groups/models.py +++ b/openedx/core/djangoapps/course_groups/models.py @@ -1,3 +1,8 @@ +""" +Django models related to course groups functionality. +""" + +import json import logging from django.contrib.auth.models import User @@ -36,9 +41,26 @@ class CourseUserGroup(models.Model): GROUP_TYPE_CHOICES = ((COHORT, 'Cohort'),) group_type = models.CharField(max_length=20, choices=GROUP_TYPE_CHOICES) + @classmethod + def create(cls, name, course_id, group_type=COHORT): + """ + Create a new course user group. + + Args: + name: Name of group + course_id: course id + group_type: group type + """ + return cls.objects.get_or_create( + course_id=course_id, + group_type=group_type, + name=name + ) + class CourseUserGroupPartitionGroup(models.Model): """ + Create User Partition Info. """ course_user_group = models.OneToOneField(CourseUserGroup) partition_id = models.IntegerField( @@ -49,3 +71,65 @@ class CourseUserGroupPartitionGroup(models.Model): ) created_at = models.DateTimeField(auto_now_add=True) updated_at = models.DateTimeField(auto_now=True) + + +class CourseCohortsSettings(models.Model): + """ + This model represents cohort settings for courses. + """ + is_cohorted = models.BooleanField(default=False) + + course_id = CourseKeyField( + unique=True, + max_length=255, + db_index=True, + help_text="Which course are these settings associated with?", + ) + + _cohorted_discussions = models.TextField(db_column='cohorted_discussions', null=True, blank=True) # JSON list + + # pylint: disable=invalid-name + always_cohort_inline_discussions = models.BooleanField(default=True) + + @property + def cohorted_discussions(self): + """Jsonify the cohorted_discussions""" + return json.loads(self._cohorted_discussions) + + @cohorted_discussions.setter + def cohorted_discussions(self, value): + """Un-Jsonify the cohorted_discussions""" + self._cohorted_discussions = json.dumps(value) + + +class CourseCohort(models.Model): + """ + This model represents cohort related info. + """ + course_user_group = models.OneToOneField(CourseUserGroup, unique=True, related_name='cohort') + + RANDOM = 'random' + MANUAL = 'manual' + ASSIGNMENT_TYPE_CHOICES = ((RANDOM, 'Random'), (MANUAL, 'Manual'),) + assignment_type = models.CharField(max_length=20, choices=ASSIGNMENT_TYPE_CHOICES, default=MANUAL) + + @classmethod + def create(cls, cohort_name=None, course_id=None, course_user_group=None, assignment_type=MANUAL): + """ + Create a complete(CourseUserGroup + CourseCohort) object. + + Args: + cohort_name: Name of the cohort to be created + course_id: Course Id + course_user_group: CourseUserGroup + assignment_type: 'random' or 'manual' + """ + if course_user_group is None: + course_user_group, __ = CourseUserGroup.create(cohort_name, course_id) + + course_cohort, __ = cls.objects.get_or_create( + course_user_group=course_user_group, + defaults={'assignment_type': assignment_type} + ) + + return course_cohort 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 c0e1356265..eb5e312f5f 100644 --- a/openedx/core/djangoapps/course_groups/tests/helpers.py +++ b/openedx/core/djangoapps/course_groups/tests/helpers.py @@ -1,13 +1,17 @@ """ 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 +from ..cohorts import set_course_cohort_settings +from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings class CohortFactory(DjangoModelFactory): @@ -29,6 +33,29 @@ class CohortFactory(DjangoModelFactory): self.users.add(*extracted) +class CourseCohortFactory(DjangoModelFactory): + """ + Factory for constructing mock course cohort. + """ + FACTORY_FOR = CourseCohort + + course_user_group = factory.SubFactory(CohortFactory) + assignment_type = 'manual' + + +class CourseCohortSettingsFactory(DjangoModelFactory): + """ + Factory for constructing mock course cohort settings. + """ + FACTORY_FOR = CourseCohortsSettings + + is_cohorted = False + course_id = SlashSeparatedCourseKey("dummy", "dummy", "dummy") + cohorted_discussions = json.dumps([]) + # pylint: disable=invalid-name + always_cohort_inline_discussions = True + + def topic_name_to_id(course, name): """ Given a discussion topic name, return an id for that name (includes @@ -41,7 +68,7 @@ def topic_name_to_id(course, name): ) -def config_course_cohorts( +def config_course_cohorts_legacy( course, discussions, cohorted, @@ -51,7 +78,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 @@ -79,7 +110,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 @@ -93,3 +123,59 @@ def config_course_cohorts( modulestore().update_item(course, ModuleStoreEnum.UserID.test) except NotImplementedError: pass + + +# pylint: disable=dangerous-default-value +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): + """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 + ) + + 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 66a2b6322d..a2c98ad802 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -1,10 +1,15 @@ -from django.conf import settings +""" +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 @@ -12,10 +17,12 @@ from student.tests.factories import UserFactory from xmodule.modulestore.django import modulestore, clear_existing_modulestores from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_TOY_MODULESTORE, mixed_store_config, ModuleStoreTestCase -from ..models import CourseUserGroup, CourseUserGroupPartitionGroup +from ..models import CourseUserGroup, CourseCohort, CourseUserGroupPartitionGroup from .. import cohorts -from ..tests.helpers import topic_name_to_id, config_course_cohorts, CohortFactory - +from ..tests.helpers import ( + topic_name_to_id, config_course_cohorts, config_course_cohorts_legacy, + CohortFactory, CourseCohortFactory, CourseCohortSettingsFactory +) @patch("openedx.core.djangoapps.course_groups.cohorts.tracker") class TestCohortSignals(TestCase): @@ -115,6 +122,7 @@ class TestCohortSignals(TestCase): self.assertFalse(mock_tracker.emit.called) +@ddt.ddt class TestCohorts(ModuleStoreTestCase): """ Test the cohorts feature @@ -128,17 +136,23 @@ class TestCohorts(ModuleStoreTestCase): super(TestCohorts, self).setUp() self.toy_course_key = SlashSeparatedCourseKey("edX", "toy", "2012_Fall") + def _create_cohort(self, course_id, cohort_name, assignment_type): + """ + Create a cohort for testing. + """ + cohort = CohortFactory(course_id=course_id, name=cohort_name) + CourseCohortFactory(course_user_group=cohort, assignment_type=assignment_type) + return cohort + def test_is_course_cohorted(self): """ 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 @@ -151,28 +165,64 @@ 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) self.assertRaises( - ValueError, + Http404, lambda: cohorts.get_cohort_id(user, SlashSeparatedCourseKey("course", "does_not", "exist")) ) + def test_assignment_type(self): + """ + Make sure that cohorts.set_assignment_type() and cohorts.get_assignment_type() works correctly. + """ + course = modulestore().get_course(self.toy_course_key) + + # We are creating two random cohorts because we can't change assignment type of + # random cohort if it is the only random cohort present. + cohort1 = self._create_cohort(course.id, "TestCohort1", CourseCohort.RANDOM) + self._create_cohort(course.id, "TestCohort2", CourseCohort.RANDOM) + cohort3 = self._create_cohort(course.id, "TestCohort3", CourseCohort.MANUAL) + + self.assertEqual(cohorts.get_assignment_type(cohort1), CourseCohort.RANDOM) + + cohorts.set_assignment_type(cohort1, CourseCohort.MANUAL) + self.assertEqual(cohorts.get_assignment_type(cohort1), CourseCohort.MANUAL) + + cohorts.set_assignment_type(cohort3, CourseCohort.RANDOM) + self.assertEqual(cohorts.get_assignment_type(cohort3), CourseCohort.RANDOM) + + def test_cannot_set_assignment_type(self): + """ + Make sure that we can't change the assignment type of a random cohort if it is the only random cohort present. + """ + course = modulestore().get_course(self.toy_course_key) + + cohort = self._create_cohort(course.id, "TestCohort", CourseCohort.RANDOM) + + self.assertEqual(cohorts.get_assignment_type(cohort), CourseCohort.RANDOM) + + exception_msg = "There must be one cohort to which students can automatically be assigned." + with self.assertRaises(ValueError) as context_manager: + cohorts.set_assignment_type(cohort, CourseCohort.MANUAL) + + self.assertEqual(exception_msg, str(context_manager.exception)) + def test_get_cohort(self): """ Make sure cohorts.get_cohort() does the right thing when the course is cohorted """ 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") @@ -188,35 +238,55 @@ 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, 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 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 @@ -225,17 +295,16 @@ class TestCohorts(ModuleStoreTestCase): # get_cohort should return a group for user self.assertEquals(cohorts.get_cohort(user, course.id).name, "AutoGroup") - def test_auto_cohorting(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") - user3 = UserFactory(username="test3", email="a3@b.com") - user4 = UserFactory(username="test4", email="a4@b.com") cohort = CohortFactory(course_id=course.id, name="TestCohort") @@ -245,31 +314,36 @@ 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") self.assertEquals(cohorts.get_cohort(user2, course.id).name, "AutoGroup", "user2 should be auto-cohorted") - # Now make the auto_cohort_group list empty + def test_cohorting_with_migrations_done(self): + """ + 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) + + user1 = UserFactory(username="test", email="a@b.com") + user2 = UserFactory(username="test2", email="a2@b.com") + + # Add an auto_cohort_group to the course... config_course_cohorts( course, - discussions=[], - cohorted=True, - auto_cohort_groups=[] + is_cohorted=True, + auto_cohorts=["AutoGroup"] ) - self.assertEquals( - cohorts.get_cohort(user3, course.id).id, - cohorts.get_cohort_by_name(course.id, cohorts.DEFAULT_COHORT_NAME).id, - "No groups->default cohort" - ) + self.assertEquals(cohorts.get_cohort(user1, course.id).name, "AutoGroup", "user1 should be auto-cohorted") # Now set the auto_cohort_group to something different - config_course_cohorts( + # This will have no effect on lms side as we are already done with migrations + config_course_cohorts_legacy( course, discussions=[], cohorted=True, @@ -277,30 +351,71 @@ class TestCohorts(ModuleStoreTestCase): ) self.assertEquals( - cohorts.get_cohort(user4, course.id).name, "OtherGroup", "New list->new group" + cohorts.get_cohort(user2, course.id).name, "AutoGroup", "user2 should be assigned to AutoGroups" ) + self.assertEquals( - cohorts.get_cohort(user1, course.id).name, "TestCohort", "user1 should still be in originally placed cohort" + cohorts.get_cohort(user1, course.id).name, "AutoGroup", "user1 should still be in originally placed cohort" ) - self.assertEquals( - cohorts.get_cohort(user2, course.id).name, "AutoGroup", "user2 should still be in originally placed cohort" + + def test_cohorting_with_no_auto_cohorts(self): + """ + 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(cohorts.is_course_cohorted(course.id)) + + user1 = UserFactory(username="test", email="a@b.com") + user2 = UserFactory(username="test2", email="a2@b.com") + + # Make the auto_cohort_group list empty + config_course_cohorts( + course, + is_cohorted=True, + auto_cohorts=[] ) + self.assertEquals( - cohorts.get_cohort(user3, course.id).name, + cohorts.get_cohort(user1, course.id).id, + cohorts.get_cohort_by_name(course.id, cohorts.DEFAULT_COHORT_NAME).id, + "No groups->default cohort for user1" + ) + + # 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_legacy( + course, + discussions=[], + cohorted=True, + auto_cohort_groups=["AutoGroup"] + ) + + self.assertEquals( + cohorts.get_cohort(user1, course.id).name, cohorts.get_cohort_by_name(course.id, cohorts.DEFAULT_COHORT_NAME).name, - "user3 should still be in the default cohort" + "user1 should still be in the default cohort" ) + self.assertEquals( + cohorts.get_cohort(user2, course.id).id, + cohorts.get_cohort_by_name(course.id, cohorts.DEFAULT_COHORT_NAME).id, + "No groups->default cohort for user2" + ) + + def test_auto_cohorting_randomization(self): """ 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 @@ -326,7 +441,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): @@ -335,8 +450,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 @@ -348,7 +464,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) @@ -360,7 +476,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")), @@ -368,9 +484,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." @@ -378,12 +494,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." @@ -395,14 +512,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( @@ -413,8 +531,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 ) @@ -441,12 +560,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( @@ -455,8 +575,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( @@ -512,8 +633,9 @@ class TestCohorts(ModuleStoreTestCase): Make sure cohorts.add_cohort() properly adds a cohort to a course and handles errors. """ + assignment_type = CourseCohort.RANDOM course = modulestore().get_course(self.toy_course_key) - added_cohort = cohorts.add_cohort(course.id, "My Cohort") + added_cohort = cohorts.add_cohort(course.id, "My Cohort", assignment_type) mock_tracker.emit.assert_any_call( "edx.cohort.creation_requested", {"cohort_name": added_cohort.name, "cohort_id": added_cohort.id} @@ -522,11 +644,12 @@ class TestCohorts(ModuleStoreTestCase): self.assertEqual(added_cohort.name, "My Cohort") self.assertRaises( ValueError, - lambda: cohorts.add_cohort(course.id, "My Cohort") + lambda: cohorts.add_cohort(course.id, "My Cohort", assignment_type) ) + does_not_exist_course_key = SlashSeparatedCourseKey("course", "does_not", "exist") self.assertRaises( ValueError, - lambda: cohorts.add_cohort(SlashSeparatedCourseKey("course", "does_not", "exist"), "My Cohort") + lambda: cohorts.add_cohort(does_not_exist_course_key, "My Cohort", assignment_type) ) @patch("openedx.core.djangoapps.course_groups.cohorts.tracker") @@ -586,8 +709,66 @@ class TestCohorts(ModuleStoreTestCase): lambda: cohorts.add_user_to_cohort(first_cohort, "non_existent_username") ) + def test_get_course_cohort_settings(self): + """ + Test that cohorts.get_course_cohort_settings is working as expected. + """ + course = modulestore().get_course(self.toy_course_key) + course_cohort_settings = cohorts.get_course_cohort_settings(course.id) + self.assertFalse(course_cohort_settings.is_cohorted) + self.assertEqual(course_cohort_settings.cohorted_discussions, []) + self.assertTrue(course_cohort_settings.always_cohort_inline_discussions) + + def test_update_course_cohort_settings(self): + """ + Test that cohorts.set_course_cohort_settings is working as expected. + """ + course = modulestore().get_course(self.toy_course_key) + CourseCohortSettingsFactory(course_id=course.id) + + cohorts.set_course_cohort_settings( + course.id, + is_cohorted=False, + cohorted_discussions=['topic a id', 'topic b id'], + always_cohort_inline_discussions=False + ) + + course_cohort_settings = cohorts.get_course_cohort_settings(course.id) + + self.assertFalse(course_cohort_settings.is_cohorted) + 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__) + ) + + +@ddt.ddt class TestCohortsAndPartitionGroups(ModuleStoreTestCase): + """ + Test Cohorts and Partitions Groups. + """ MODULESTORE = TEST_DATA_MIXED_TOY_MODULESTORE def setUp(self): @@ -646,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 1bce300b06..57bb63b26e 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( @@ -63,6 +63,7 @@ class TestCohortPartitionScheme(ModuleStoreTestCase): self.course_key, self.student, partition or self.user_partition, + use_cached=False ), group ) @@ -76,6 +77,7 @@ class TestCohortPartitionScheme(ModuleStoreTestCase): first_cohort, second_cohort = [ CohortFactory(course_id=self.course_key) for _ in range(2) ] + # place student 0 into first cohort add_user_to_cohort(first_cohort, self.student.username) self.assert_student_in_group(None) diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py index 65dca811aa..886e9eb454 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_views.py +++ b/openedx/core/djangoapps/course_groups/tests/test_views.py @@ -1,30 +1,38 @@ """ Tests for course group views """ -from collections import namedtuple +# pylint: disable=attribute-defined-outside-init +# pylint: disable=no-member 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 - +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 xmodule.modulestore.django import modulestore from opaque_keys.edx.locations import SlashSeparatedCourseKey +from xmodule.modulestore.tests.factories import ItemFactory -from ..models import CourseUserGroup +from ..models import CourseUserGroup, CourseCohort from ..views import ( - cohort_handler, users_in_cohort, add_users_to_cohort, remove_user_from_cohort, link_cohort_to_partition_group + course_cohort_settings_handler, cohort_handler, users_in_cohort, add_users_to_cohort, remove_user_from_cohort, + link_cohort_to_partition_group, cohort_discussion_topics ) from ..cohorts import ( - get_cohort, CohortAssignmentType, get_cohort_by_name, get_cohort_by_id, + 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 +from .helpers import ( + config_course_cohorts, config_course_cohorts_legacy, CohortFactory, CourseCohortFactory, topic_name_to_id +) class CohortViewsTestCase(ModuleStoreTestCase): @@ -47,15 +55,22 @@ class CohortViewsTestCase(ModuleStoreTestCase): self.cohort1_users = [UserFactory() for _ in range(3)] self.cohort2_users = [UserFactory() for _ in range(2)] self.cohort3_users = [UserFactory() for _ in range(2)] + self.cohort4_users = [UserFactory() for _ in range(2)] self.cohortless_users = [UserFactory() for _ in range(3)] self.unenrolled_users = [UserFactory() for _ in range(3)] self._enroll_users( - self.cohort1_users + self.cohort2_users + self.cohort3_users + self.cohortless_users, + self.cohort1_users + self.cohort2_users + self.cohort3_users + self.cohortless_users + self.cohort4_users, self.course.id ) self.cohort1 = CohortFactory(course_id=self.course.id, users=self.cohort1_users) self.cohort2 = CohortFactory(course_id=self.course.id, users=self.cohort2_users) self.cohort3 = CohortFactory(course_id=self.course.id, users=self.cohort3_users) + self.cohort4 = CohortFactory(course_id=self.course.id, users=self.cohort4_users) + + CourseCohortFactory(course_user_group=self.cohort1) + CourseCohortFactory(course_user_group=self.cohort2) + CourseCohortFactory(course_user_group=self.cohort3) + CourseCohortFactory(course_user_group=self.cohort4, assignment_type=CourseCohort.RANDOM) def _user_in_cohort(self, username, cohort): """ @@ -81,49 +96,241 @@ 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 -class CohortHandlerTestCase(CohortViewsTestCase): - """ - Tests the `cohort_handler` view. - """ - def get_cohort_handler(self, course, cohort=None): + # 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 `cohort_handler` for a given `course` and return its response as a - dict. If `cohort` is specified, only information for that specific cohort is returned. + Call a GET on `handler` for a given `course` and return its response as a dict. + Raise an exception if response status code is not as expected. """ request = RequestFactory().get("dummy_url") request.user = self.staff_user if cohort: - response = cohort_handler(request, unicode(course.id), cohort.id) + response = handler(request, unicode(course.id), cohort.id) else: - response = cohort_handler(request, unicode(course.id)) - self.assertEqual(response.status_code, 200) + response = handler(request, unicode(course.id)) + self.assertEqual(response.status_code, expected_response_code) return json.loads(response.content) - def put_cohort_handler(self, course, cohort=None, data=None, expected_response_code=200): + def put_handler(self, course, cohort=None, data=None, expected_response_code=200, handler=cohort_handler): """ - Call a PUT on `cohort_handler` for a given `course` and return its response as a - dict. If `cohort` is not specified, a new cohort is created. If `cohort` is specified, - the existing cohort is updated. + Call a PUT 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().put(path="dummy path", data=data, content_type="application/json") request.user = self.staff_user if cohort: - response = cohort_handler(request, unicode(course.id), cohort.id) + response = handler(request, unicode(course.id), cohort.id) else: - response = cohort_handler(request, unicode(course.id)) + response = handler(request, unicode(course.id)) 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, "PATCH", [unicode(self.course.id)]) + + def test_get_settings(self): + """ + Verify that course_cohort_settings_handler is working for HTTP GET. + """ + cohorted_inline_discussions, cohorted_course_wide_discussions = self.create_cohorted_discussions() + + response = self.get_handler(self.course, handler=course_cohort_settings_handler) + expected_response = self.get_expected_response() + + 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_is_cohorted_settings(self): + """ + 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 = self.get_expected_response() + + self.assertEqual(response, expected_response) + + expected_response['is_cohorted'] = False + 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) + + def test_update_settings_with_missing_field(self): + """ + Verify that course_cohort_settings_handler return HTTP 400 if required data field is missing from post data. + """ + config_course_cohorts(self.course, is_cohorted=True) + + 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): + """ + Verify that course_cohort_settings_handler return HTTP 400 if field data type is incorrect. + """ + config_course_cohorts(self.course, is_cohorted=True) + + response = self.patch_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): + """ + Tests the `cohort_handler` view. + """ def verify_lists_expected_cohorts(self, expected_cohorts, response_dict=None): """ Verify that the server response contains the expected_cohorts. If response_dict is None, the list of cohorts is requested from the server. """ if response_dict is None: - response_dict = self.get_cohort_handler(self.course) + response_dict = self.get_handler(self.course) self.assertEqual( response_dict.get("cohorts"), @@ -171,9 +378,10 @@ class CohortHandlerTestCase(CohortViewsTestCase): """ self._create_cohorts() expected_cohorts = [ - CohortHandlerTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE), - CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE), - CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE), + 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), ] self.verify_lists_expected_cohorts(expected_cohorts) @@ -181,22 +389,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_cohort_handler(self.course) + 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(self.cohort1, 3, CohortAssignmentType.NONE), - CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE), - CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE), - CohortHandlerTestCase.create_expected_cohort(auto_cohort_1, 0, CohortAssignmentType.RANDOM), - CohortHandlerTestCase.create_expected_cohort(auto_cohort_2, 0, CohortAssignmentType.RANDOM), + 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), ] self.verify_lists_expected_cohorts(expected_cohorts, actual_cohorts) @@ -207,8 +414,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([]) @@ -218,42 +425,40 @@ class CohortHandlerTestCase(CohortViewsTestCase): self._enroll_users(users, self.course.id) # mimic users accessing the discussion forum + # Default Cohort will be created here for user in users: get_cohort(user, self.course.id) # verify the default cohort is automatically created default_cohort = get_cohort_by_name(self.course.id, DEFAULT_COHORT_NAME) - actual_cohorts = self.get_cohort_handler(self.course) + actual_cohorts = self.get_handler(self.course) self.verify_lists_expected_cohorts( - [CohortHandlerTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.RANDOM)], + [CohortHandlerTestCase.create_expected_cohort(default_cohort, len(users), CourseCohort.RANDOM)], actual_cohorts, ) - # set auto_cohort_groups and verify the default cohort is no longer listed as RANDOM - config_course_cohorts(self.course, [], cohorted=True, auto_cohort_groups=["AutoGroup"]) - actual_cohorts = self.get_cohort_handler(self.course) - auto_cohort = get_cohort_by_name(self.course.id, "AutoGroup") - self.verify_lists_expected_cohorts( - [ - CohortHandlerTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.NONE), - CohortHandlerTestCase.create_expected_cohort(auto_cohort, 0, CohortAssignmentType.RANDOM), - ], - actual_cohorts, - ) + # set auto_cohort_groups + # these cohort config will have not effect on lms side as we are already done with migrations + 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 + with self.assertRaises(CourseUserGroup.DoesNotExist): + get_cohort_by_name(self.course.id, "AutoGroup") def test_get_single_cohort(self): """ Tests that information for just a single cohort can be requested. """ self._create_cohorts() - response_dict = self.get_cohort_handler(self.course, self.cohort2) + response_dict = self.get_handler(self.course, self.cohort2) self.assertEqual( response_dict, { "name": self.cohort2.name, "id": self.cohort2.id, "user_count": 2, - "assignment_type": "none", + "assignment_type": CourseCohort.MANUAL, "user_partition_id": None, "group_id": None } @@ -262,7 +467,8 @@ class CohortHandlerTestCase(CohortViewsTestCase): ############### Tests of adding a new cohort ############### def verify_contains_added_cohort( - self, response_dict, cohort_name, expected_user_partition_id=None, expected_group_id=None + self, response_dict, cohort_name, assignment_type=CourseCohort.MANUAL, + expected_user_partition_id=None, expected_group_id=None ): """ Verifies that the cohort was created properly and the correct response was returned. @@ -275,7 +481,7 @@ class CohortHandlerTestCase(CohortViewsTestCase): "name": cohort_name, "id": created_cohort.id, "user_count": 0, - "assignment_type": CohortAssignmentType.NONE, + "assignment_type": assignment_type, "user_partition_id": expected_user_partition_id, "group_id": expected_group_id } @@ -287,29 +493,47 @@ class CohortHandlerTestCase(CohortViewsTestCase): Verify that a new cohort can be created, with and without user_partition_id/group_id information. """ new_cohort_name = "New cohort unassociated to content groups" - response_dict = self.put_cohort_handler(self.course, data={'name': new_cohort_name}) - self.verify_contains_added_cohort(response_dict, new_cohort_name) + request_data = {'name': new_cohort_name, 'assignment_type': CourseCohort.RANDOM} + response_dict = self.put_handler(self.course, data=request_data) + self.verify_contains_added_cohort(response_dict, new_cohort_name, assignment_type=CourseCohort.RANDOM) new_cohort_name = "New cohort linked to group" - response_dict = self.put_cohort_handler( - self.course, data={'name': new_cohort_name, 'user_partition_id': 1, 'group_id': 2} + data = { + 'name': new_cohort_name, + 'assignment_type': CourseCohort.MANUAL, + 'user_partition_id': 1, + 'group_id': 2 + } + response_dict = self.put_handler(self.course, data=data) + self.verify_contains_added_cohort( + response_dict, + new_cohort_name, + expected_user_partition_id=1, + expected_group_id=2 ) - self.verify_contains_added_cohort(response_dict, new_cohort_name, 1, 2) def test_create_new_cohort_missing_name(self): """ Verify that we cannot create a cohort without specifying a name. """ - response_dict = self.put_cohort_handler(self.course, expected_response_code=400) - self.assertEqual("In order to create a cohort, a name must be specified.", response_dict.get("error")) + response_dict = self.put_handler(self.course, expected_response_code=400) + self.assertEqual("Cohort name must be specified.", response_dict.get("error")) + + def test_create_new_cohort_missing_assignment_type(self): + """ + Verify that we cannot create a cohort without specifying an assignment type. + """ + response_dict = self.put_handler(self.course, data={'name': 'COHORT NAME'}, expected_response_code=400) + self.assertEqual("Assignment type must be specified.", response_dict.get("error")) def test_create_new_cohort_existing_name(self): """ Verify that we cannot add a cohort with the same name as an existing cohort. """ self._create_cohorts() - response_dict = self.put_cohort_handler( - self.course, data={'name': self.cohort1.name}, expected_response_code=400 + response_dict = self.put_handler( + self.course, data={'name': self.cohort1.name, 'assignment_type': CourseCohort.MANUAL}, + expected_response_code=400 ) self.assertEqual("You cannot create two cohorts with the same name", response_dict.get("error")) @@ -317,9 +541,8 @@ class CohortHandlerTestCase(CohortViewsTestCase): """ Verify that we cannot create a cohort with a group_id if the user_partition_id is not also specified. """ - response_dict = self.put_cohort_handler( - self.course, data={'name': "Cohort missing user_partition_id", 'group_id': 2}, expected_response_code=400 - ) + data = {'name': "Cohort missing user_partition_id", 'assignment_type': CourseCohort.MANUAL, 'group_id': 2} + response_dict = self.put_handler(self.course, data=data, expected_response_code=400) self.assertEqual( "If group_id is specified, user_partition_id must also be specified.", response_dict.get("error") ) @@ -332,32 +555,53 @@ class CohortHandlerTestCase(CohortViewsTestCase): """ self._create_cohorts() updated_name = self.cohort1.name + "_updated" - response_dict = self.put_cohort_handler(self.course, self.cohort1, {'name': updated_name}) + data = {'name': updated_name, 'assignment_type': CourseCohort.MANUAL} + response_dict = self.put_handler(self.course, self.cohort1, data=data) self.assertEqual(updated_name, get_cohort_by_id(self.course.id, self.cohort1.id).name) self.assertEqual(updated_name, response_dict.get("name")) - self.assertEqual(CohortAssignmentType.NONE, response_dict.get("assignment_type")) - self.assertEqual(CohortAssignmentType.NONE, CohortAssignmentType.get(self.cohort1, self.course)) + self.assertEqual(CourseCohort.MANUAL, response_dict.get("assignment_type")) - def test_update_random_cohort_name_not_supported(self): + def test_update_random_cohort_name(self): """ - Test that it is not possible to update the name of an existing random cohort. + Test that it is possible to update the name of an existing random cohort. """ - random_cohort = CohortFactory(course_id=self.course.id) - random_cohort_name = random_cohort.name + # Create a new cohort with random assignment + cohort_name = 'I AM A RANDOM COHORT' + data = {'name': cohort_name, 'assignment_type': CourseCohort.RANDOM} + response_dict = self.put_handler(self.course, data=data) - # Update course cohort_config so random_cohort is in the list of auto cohorts. - self.course.cohort_config["auto_cohort_groups"] = [random_cohort_name] - modulestore().update_item(self.course, self.staff_user.id) + self.assertEqual(cohort_name, response_dict.get("name")) + self.assertEqual(CourseCohort.RANDOM, response_dict.get("assignment_type")) - updated_name = random_cohort.name + "_updated" - response_dict = self.put_cohort_handler( - self.course, random_cohort, {'name': updated_name}, expected_response_code=400 - ) + # Update the newly created random cohort + newly_created_cohort = get_cohort_by_name(self.course.id, cohort_name) + cohort_name = 'I AM AN UPDATED RANDOM COHORT' + data = {'name': cohort_name, 'assignment_type': CourseCohort.RANDOM} + response_dict = self.put_handler(self.course, newly_created_cohort, data=data) + + self.assertEqual(cohort_name, get_cohort_by_id(self.course.id, newly_created_cohort.id).name) + self.assertEqual(cohort_name, response_dict.get("name")) + self.assertEqual(CourseCohort.RANDOM, response_dict.get("assignment_type")) + + def test_cannot_update_assignment_type_of_single_random_cohort(self): + """ + Test that it is not possible to update the assignment type of a single random cohort. + """ + # Create a new cohort with random assignment + cohort_name = 'I AM A RANDOM COHORT' + data = {'name': cohort_name, 'assignment_type': CourseCohort.RANDOM} + response_dict = self.put_handler(self.course, data=data) + + self.assertEqual(cohort_name, response_dict.get("name")) + self.assertEqual(CourseCohort.RANDOM, response_dict.get("assignment_type")) + + # Try to update the assignment type of newly created random cohort + cohort = get_cohort_by_name(self.course.id, cohort_name) + data = {'name': cohort_name, 'assignment_type': CourseCohort.MANUAL} + response_dict = self.put_handler(self.course, cohort, data=data, expected_response_code=400) self.assertEqual( - "Renaming of random cohorts is not supported at this time.", response_dict.get("error") + 'There must be one cohort to which students can automatically be assigned.', response_dict.get("error") ) - self.assertEqual(random_cohort_name, get_cohort_by_id(self.course.id, random_cohort.id).name) - self.assertEqual(CohortAssignmentType.RANDOM, CohortAssignmentType.get(random_cohort, self.course)) def test_update_cohort_group_id(self): """ @@ -365,9 +609,13 @@ class CohortHandlerTestCase(CohortViewsTestCase): """ self._create_cohorts() self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1)) - response_dict = self.put_cohort_handler( - self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2, 'user_partition_id': 3} - ) + data = { + 'name': self.cohort1.name, + 'assignment_type': CourseCohort.MANUAL, + 'group_id': 2, + 'user_partition_id': 3 + } + response_dict = self.put_handler(self.course, self.cohort1, data=data) self.assertEqual((2, 3), get_group_info_for_cohort(self.cohort1)) self.assertEqual(2, response_dict.get("group_id")) self.assertEqual(3, response_dict.get("user_partition_id")) @@ -381,9 +629,8 @@ class CohortHandlerTestCase(CohortViewsTestCase): self._create_cohorts() link_cohort_to_partition_group(self.cohort1, 5, 0) self.assertEqual((0, 5), get_group_info_for_cohort(self.cohort1)) - response_dict = self.put_cohort_handler( - self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': None} - ) + data = {'name': self.cohort1.name, 'assignment_type': CourseCohort.RANDOM, 'group_id': None} + response_dict = self.put_handler(self.course, self.cohort1, data=data) self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1)) self.assertIsNone(response_dict.get("group_id")) self.assertIsNone(response_dict.get("user_partition_id")) @@ -394,24 +641,32 @@ class CohortHandlerTestCase(CohortViewsTestCase): different group_id. """ self._create_cohorts() - self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1)) - self.put_cohort_handler( - self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2, 'user_partition_id': 3} - ) - self.assertEqual((2, 3), get_group_info_for_cohort(self.cohort1)) - self.put_cohort_handler( - self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 1, 'user_partition_id': 3} - ) - self.assertEqual((1, 3), get_group_info_for_cohort(self.cohort1)) + self.assertEqual((None, None), get_group_info_for_cohort(self.cohort4)) + data = { + 'name': self.cohort4.name, + 'assignment_type': CourseCohort.RANDOM, + 'group_id': 2, + 'user_partition_id': 3 + } + self.put_handler(self.course, self.cohort4, data=data) + self.assertEqual((2, 3), get_group_info_for_cohort(self.cohort4)) + + data = { + 'name': self.cohort4.name, + 'assignment_type': CourseCohort.RANDOM, + 'group_id': 1, + 'user_partition_id': 3 + } + self.put_handler(self.course, self.cohort4, data=data) + self.assertEqual((1, 3), get_group_info_for_cohort(self.cohort4)) def test_update_cohort_missing_user_partition_id(self): """ Verify that we cannot update a cohort with a group_id if the user_partition_id is not also specified. """ self._create_cohorts() - response_dict = self.put_cohort_handler( - self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2}, expected_response_code=400 - ) + data = {'name': self.cohort1.name, 'assignment_type': CourseCohort.RANDOM, 'group_id': 2} + response_dict = self.put_handler(self.course, self.cohort1, data=data, expected_response_code=400) self.assertEqual( "If group_id is specified, user_partition_id must also be specified.", response_dict.get("error") ) @@ -939,3 +1194,59 @@ 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) + + +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS') +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 8248576642..699002942f 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/openedx/core/djangoapps/course_groups/views.py @@ -1,3 +1,7 @@ +""" +Views related to course groups functionality. +""" + from django_future.csrf import ensure_csrf_cookie from django.views.decorators.http import require_POST from django.contrib.auth.models import User @@ -7,15 +11,18 @@ from django.http import Http404, HttpResponse, HttpResponseBadRequest from django.views.decorators.http import require_http_methods from util.json_request import expect_json, JsonResponse from django.contrib.auth.decorators import login_required -import json +from django.utils.translation import ugettext + import logging import re +from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey 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__) @@ -54,21 +61,114 @@ def unlink_cohort_partition_group(cohort): CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete() +# pylint: disable=invalid-name +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_inline_discussions': cohorted_inline_discussions, + 'cohorted_course_wide_discussions': cohorted_course_wide_discussions, + 'always_cohort_inline_discussions': course_cohort_settings.always_cohort_inline_discussions, + } + + def _get_cohort_representation(cohort, course): """ Returns a JSON representation of a cohort. """ group_id, partition_id = cohorts.get_group_info_for_cohort(cohort) + assignment_type = cohorts.get_assignment_type(cohort) return { 'name': cohort.name, 'id': cohort.id, 'user_count': cohort.users.count(), - 'assignment_type': cohorts.CohortAssignmentType.get(cohort, course), + 'assignment_type': assignment_type, 'user_partition_id': partition_id, - 'group_id': group_id + 'group_id': group_id, } +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, None, 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 +def course_cohort_settings_handler(request, course_key_string): + """ + The restful handler for cohort setting requests. Requires JSON. + This will raise 404 if user is not staff. + GET + Returns the JSON representation of cohort settings for the course. + PATCH + Updates the cohort settings for the course. Returns the JSON representation of updated settings. + """ + course_key = CourseKey.from_string(course_key_string) + 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, **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(course, cohort_settings)) + + @require_http_methods(("GET", "PUT", "POST", "PATCH")) @ensure_csrf_cookie @expect_json @@ -102,25 +202,30 @@ def cohort_handler(request, course_key_string, cohort_id=None): cohort = cohorts.get_cohort_by_id(course_key, cohort_id) return JsonResponse(_get_cohort_representation(cohort, course)) else: + name = request.json.get('name') + assignment_type = request.json.get('assignment_type') + if not name: + # Note: error message not translated because it is not exposed to the user (UI prevents this state). + return JsonResponse({"error": "Cohort name must be specified."}, 400) + if not assignment_type: + # Note: error message not translated because it is not exposed to the user (UI prevents this state). + return JsonResponse({"error": "Assignment type must be specified."}, 400) # If cohort_id is specified, update the existing cohort. Otherwise, create a new cohort. if cohort_id: cohort = cohorts.get_cohort_by_id(course_key, cohort_id) - name = request.json.get('name') if name != cohort.name: - if cohorts.CohortAssignmentType.get(cohort, course) == cohorts.CohortAssignmentType.RANDOM: - return JsonResponse( - # Note: error message not translated because it is not exposed to the user (UI prevents). - {"error": "Renaming of random cohorts is not supported at this time."}, 400 - ) + if cohorts.is_cohort_exists(course_key, name): + err_msg = ugettext("A cohort with the same name already exists.") + return JsonResponse({"error": unicode(err_msg)}, 400) cohort.name = name cohort.save() - else: - name = request.json.get('name') - if not name: - # Note: error message not translated because it is not exposed to the user (UI prevents this state). - return JsonResponse({"error": "In order to create a cohort, a name must be specified."}, 400) try: - cohort = cohorts.add_cohort(course_key, name) + cohorts.set_assignment_type(cohort, assignment_type) + except ValueError as err: + return JsonResponse({"error": unicode(err)}, 400) + else: + try: + cohort = cohorts.add_cohort(course_key, name, assignment_type) except ValueError as err: return JsonResponse({"error": unicode(err)}, 400) @@ -299,8 +404,86 @@ def debug_cohort_mgmt(request, course_key_string): # add staff check to make sure it's safe if it's accidentally deployed. get_course_with_access(request.user, 'staff', course_key) - context = {'cohorts_ajax_url': reverse( + context = {'cohorts_url': reverse( 'cohorts', 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, 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') + + 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)