Merge pull request #14830 from edx/christina/change-default-cohorting

Change default value for "always_cohort_inline_discussions" to False

common/lib/xmodule/xmodule/course_module.py

@@ -1157,16 +1157,19 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin):
     def always_cohort_inline_discussions(self):
         """
         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.
+        setting this to 'True', all inline discussions are cohorted. The default value is
+        now `False`, meaning that inline discussions are not cohorted unless their discussion IDs
+        are specifically listed as cohorted.
 
-        Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
+        Note: No longer used except to get the initial value when cohorts are first enabled on a course
+        (and for migrating old courses). See openedx.core.djangoapps.course_groups.models.CourseCohortSettings.
         """
         config = self.cohort_config
         if config is None:
-            return True
+            # This value sets the default for newly created courses.
+            return False
 
-        return bool(config.get("always_cohort_inline_discussions", True))
+        return bool(config.get("always_cohort_inline_discussions", False))
 
     @property
     def is_newish(self):

common/test/acceptance/pages/lms/instructor_dashboard.py

@@ -686,15 +686,15 @@ class CohortManagementSection(PageObject):
 
     def always_inline_discussion_selected(self):
         """
-        Returns the checked always_cohort_inline_discussions radio button.
+        Returns true if always_cohort_inline_discussions radio button is selected.
         """
-        return self.q(css=self._bounded_selector(".check-all-inline-discussions:checked"))
+        return len(self.q(css=self._bounded_selector(".check-all-inline-discussions:checked"))) > 0
 
     def cohort_some_inline_discussion_selected(self):
         """
-        Returns the checked some_cohort_inline_discussions radio button.
+        Returns true if some_cohort_inline_discussions radio button is selected.
         """
-        return self.q(css=self._bounded_selector(".check-cohort-inline-discussions:checked"))
+        return len(self.q(css=self._bounded_selector(".check-cohort-inline-discussions:checked"))) > 0
 
     def select_cohort_some_inline_discussion(self):
         """

common/test/acceptance/tests/discussion/test_cohort_management.py

@@ -824,36 +824,45 @@ class CohortDiscussionTopicsTest(UniqueCourseTest, CohortTestMixin):
         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.
+        And an 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
+        When I save the change
+        And I reload the page
+        Then I see the always_cohort_inline_topics option enabled
         """
         self.cohort_discussion_topics_are_visible()
 
-        # enable always inline discussion topics.
+        # enable always inline discussion topics and save the change
         self.cohort_management_page.select_always_inline_discussion()
-
+        self.assertFalse(self.cohort_management_page.is_save_button_disabled(self.inline_key))
         self.assertTrue(self.cohort_management_page.inline_discussion_topics_disabled())
+        self.cohort_management_page.save_discussion_topics(key=self.inline_key)
 
         self.reload_page()
-        self.assertIsNotNone(self.cohort_management_page.always_inline_discussion_selected())
+        self.assertTrue(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.
+        Given I have a course with a cohort defined and always_cohort_inline_topics set to True
+        And an 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
+        When I save the change
+        And I reload the page
+        Then I see the cohort_some_inline_topics option enabled
         """
         self.cohort_discussion_topics_are_visible()
+        # By default always inline discussion topics is False. Enable it (and reload the page).
+        self.assertFalse(self.cohort_management_page.always_inline_discussion_selected())
+        self.cohort_management_page.select_always_inline_discussion()
+        self.cohort_management_page.save_discussion_topics(key=self.inline_key)
+        self.reload_page()
+        self.assertFalse(self.cohort_management_page.cohort_some_inline_discussion_selected())
 
         # enable some inline discussion topic radio button.
         self.cohort_management_page.select_cohort_some_inline_discussion()
@@ -861,18 +870,17 @@ class CohortDiscussionTopicsTest(UniqueCourseTest, CohortTestMixin):
         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.cohort_management_page.save_discussion_topics(key=self.inline_key)
 
         self.reload_page()
-        self.assertIsNotNone(self.cohort_management_page.cohort_some_inline_discussion_selected())
+        self.assertTrue(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 a inline discussion topic with disabled Save button
         And When I click on inline discussion topic
         And I see enabled save button
         And When i click save button
@@ -882,9 +890,6 @@ class CohortDiscussionTopicsTest(UniqueCourseTest, CohortTestMixin):
         """
         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)
@@ -912,9 +917,6 @@ class CohortDiscussionTopicsTest(UniqueCourseTest, CohortTestMixin):
         """
         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())
 
@@ -936,9 +938,6 @@ class CohortDiscussionTopicsTest(UniqueCourseTest, CohortTestMixin):
         """
         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())
 
@@ -954,35 +953,6 @@ class CohortDiscussionTopicsTest(UniqueCourseTest, CohortTestMixin):
         # 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=6)
 class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin):

lms/djangoapps/discussion_api/tests/test_api.py

@@ -84,6 +84,24 @@ def _discussion_disabled_course_for(user):
     return course_with_disabled_forums
 
 
+def _create_course_and_cohort_with_user_role(course_is_cohorted, user, role_name):
+    """
+    Creates a course with the value of `course_is_cohorted`, plus `always_cohort_inline_discussions`
+    set to True (which is no longer the default value). Then 1) enrolls the user in that course,
+    2) creates a cohort that the user is placed in, and 3) adds the user to the given role.
+
+    Returns: a tuple of the created course and the created cohort
+    """
+    cohort_course = CourseFactory.create(
+        cohort_config={"cohorted": course_is_cohorted, "always_cohort_inline_discussions": True}
+    )
+    CourseEnrollmentFactory.create(user=user, course_id=cohort_course.id)
+    cohort = CohortFactory.create(course_id=cohort_course.id, users=[user])
+    role = Role.objects.create(name=role_name, course_id=cohort_course.id)
+    role.users = [user]
+    return [cohort_course, cohort]
+
+
 @attr(shard=2)
 @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
 class GetCourseTest(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase):
@@ -1857,11 +1875,7 @@ class CreateCommentTest(
     )
     @ddt.unpack
     def test_group_access(self, role_name, course_is_cohorted, thread_group_state):
-        cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted})
-        CourseEnrollmentFactory.create(user=self.user, course_id=cohort_course.id)
-        cohort = CohortFactory.create(course_id=cohort_course.id, users=[self.user])
-        role = Role.objects.create(name=role_name, course_id=cohort_course.id)
-        role.users = [self.user]
+        cohort_course, cohort = _create_course_and_cohort_with_user_role(course_is_cohorted, self.user, role_name)
         self.register_get_thread_response(make_minimal_cs_thread({
             "id": "cohort_thread",
             "course_id": unicode(cohort_course.id),
@@ -2017,11 +2031,7 @@ class UpdateThreadTest(
     )
     @ddt.unpack
     def test_group_access(self, role_name, course_is_cohorted, thread_group_state):
-        cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted})
-        CourseEnrollmentFactory.create(user=self.user, course_id=cohort_course.id)
-        cohort = CohortFactory.create(course_id=cohort_course.id, users=[self.user])
-        role = Role.objects.create(name=role_name, course_id=cohort_course.id)
-        role.users = [self.user]
+        cohort_course, cohort = _create_course_and_cohort_with_user_role(course_is_cohorted, self.user, role_name)
         self.register_thread({
             "course_id": unicode(cohort_course.id),
             "group_id": (
@@ -2419,11 +2429,7 @@ class UpdateCommentTest(
     )
     @ddt.unpack
     def test_group_access(self, role_name, course_is_cohorted, thread_group_state):
-        cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted})
-        CourseEnrollmentFactory.create(user=self.user, course_id=cohort_course.id)
-        cohort = CohortFactory.create(course_id=cohort_course.id, users=[self.user])
-        role = Role.objects.create(name=role_name, course_id=cohort_course.id)
-        role.users = [self.user]
+        cohort_course, cohort = _create_course_and_cohort_with_user_role(course_is_cohorted, self.user, role_name)
         self.register_get_thread_response(make_minimal_cs_thread())
         self.register_comment(
             {"thread_id": "test_thread"},
@@ -2798,11 +2804,7 @@ class DeleteThreadTest(
         the student role is the author and the thread is not in a cohort,
         the student role is the author and the thread is in the author's cohort.
         """
-        cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted})
-        CourseEnrollmentFactory.create(user=self.user, course_id=cohort_course.id)
-        cohort = CohortFactory.create(course_id=cohort_course.id, users=[self.user])
-        role = Role.objects.create(name=role_name, course_id=cohort_course.id)
-        role.users = [self.user]
+        cohort_course, cohort = _create_course_and_cohort_with_user_role(course_is_cohorted, self.user, role_name)
         self.register_thread({
             "course_id": unicode(cohort_course.id),
             "group_id": (
@@ -2955,11 +2957,7 @@ class DeleteCommentTest(
         the student role is the author and the comment is not in a cohort,
         the student role is the author and the comment is in the author's cohort.
         """
-        cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted})
-        CourseEnrollmentFactory.create(user=self.user, course_id=cohort_course.id)
-        cohort = CohortFactory.create(course_id=cohort_course.id, users=[self.user])
-        role = Role.objects.create(name=role_name, course_id=cohort_course.id)
-        role.users = [self.user]
+        cohort_course, cohort = _create_course_and_cohort_with_user_role(course_is_cohorted, self.user, role_name)
         self.register_comment_and_thread(
             overrides={"thread_id": "test_thread"},
             thread_overrides={
@@ -3084,11 +3082,7 @@ class RetrieveThreadTest(
         the student role is the author and the thread is not in a cohort,
         the student role is the author and the thread is in the author's cohort.
         """
-        cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted})
-        CourseEnrollmentFactory.create(user=self.user, course_id=cohort_course.id)
-        cohort = CohortFactory.create(course_id=cohort_course.id, users=[self.user])
-        role = Role.objects.create(name=role_name, course_id=cohort_course.id)
-        role.users = [self.user]
+        cohort_course, cohort = _create_course_and_cohort_with_user_role(course_is_cohorted, self.user, role_name)
         self.register_thread({
             "course_id": unicode(cohort_course.id),
             "group_id": (

lms/djangoapps/django_comment_client/tests/test_utils.py

@@ -479,7 +479,7 @@ 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)
+        set_course_cohort_settings(course_key=self.course.id, is_cohorted=True, always_cohort_inline_discussions=True)
 
         self.assert_category_map_equals(
             {
@@ -503,7 +503,7 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
 
     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)
+        set_course_cohort_settings(course_key=self.course.id, is_cohorted=True)
 
         self.assert_category_map_equals(
             {
@@ -656,8 +656,8 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
         set_course_cohort_settings(course_key=self.course.id, is_cohorted=False)
         check_cohorted(False)
 
-        # explicitly enabled cohorting
-        set_course_cohort_settings(course_key=self.course.id, is_cohorted=True)
+        # explicitly enabled cohorting with inline discusssions also cohorted.
+        set_course_cohort_settings(course_key=self.course.id, is_cohorted=True, always_cohort_inline_discussions=True)
         check_cohorted(True)
 
     def test_tree_with_duplicate_targets(self):
@@ -1074,17 +1074,17 @@ class ContentGroupCategoryMapTestCase(CategoryMapTestMixin, ContentGroupTestCase
                         'entries': {
                             'Visible to Alpha': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'alpha_group_discussion'
                             },
                             'Visible to Beta': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'beta_group_discussion'
                             },
                             'Visible to Everyone': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'global_group_discussion'
                             }
                         }
@@ -1119,12 +1119,12 @@ class ContentGroupCategoryMapTestCase(CategoryMapTestMixin, ContentGroupTestCase
                         'entries': {
                             'Visible to Alpha': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'alpha_group_discussion'
                             },
                             'Visible to Everyone': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'global_group_discussion'
                             }
                         }
@@ -1159,12 +1159,12 @@ class ContentGroupCategoryMapTestCase(CategoryMapTestMixin, ContentGroupTestCase
                         'entries': {
                             'Visible to Beta': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'beta_group_discussion'
                             },
                             'Visible to Everyone': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'global_group_discussion'
                             }
                         }
@@ -1198,7 +1198,7 @@ class ContentGroupCategoryMapTestCase(CategoryMapTestMixin, ContentGroupTestCase
                         'entries': {
                             'Visible to Everyone': {
                                 'sort_key': None,
-                                'is_cohorted': True,
+                                'is_cohorted': False,
                                 'id': 'global_group_discussion'
                             }
                         }
@@ -1349,9 +1349,9 @@ class IsCommentableCohortedTestCase(ModuleStoreTestCase):
             discussion_topics=["General", "Feedback"],
             cohorted_discussions=["Feedback", "random_inline"]
         )
-        self.assertTrue(
+        self.assertFalse(
             utils.is_commentable_cohorted(course.id, to_id("random")),
-            "By default, Non-top-level discussion is always cohorted in cohorted courses."
+            "By default, Non-top-level discussions are not cohorted in a cohorted courses."
         )
 
         # if always_cohort_inline_discussions is set to False, non-top-level discussion are always
@@ -1381,10 +1381,11 @@ class IsCommentableCohortedTestCase(ModuleStoreTestCase):
         course = modulestore().get_course(self.toy_course_key)
         self.assertFalse(cohorts.is_course_cohorted(course.id))
 
-        config_course_cohorts(course, is_cohorted=True)
+        config_course_cohorts(course, is_cohorted=True, always_cohort_inline_discussions=True)
         team = CourseTeamFactory(course_id=course.id)
 
         # Verify that team discussions are not cohorted, but other discussions are
+        # if "always cohort inline discussions" is set to true.
         self.assertFalse(utils.is_commentable_cohorted(course.id, team.discussion_topic_id))
         self.assertTrue(utils.is_commentable_cohorted(course.id, "random"))
 

lms/static/js/groups/models/course_cohort_settings.js

@@ -7,7 +7,7 @@
                 is_cohorted: false,
                 cohorted_inline_discussions: [],
                 cohorted_course_wide_discussions: [],
-                always_cohort_inline_discussions: true
+                always_cohort_inline_discussions: false
             }
         });
         return CourseCohortSettingsModel;

lms/static/js/spec/groups/views/cohorts_spec.js

@@ -76,7 +76,7 @@ define(['backbone', 'jquery', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers
                     is_cohorted: isCohorted || false,
                     cohorted_inline_discussions: cohortedInlineDiscussions || [],
                     cohorted_course_wide_discussions: cohortedCourseWideDiscussions || [],
-                    always_cohort_inline_discussions: alwaysCohortInlineDiscussions || true
+                    always_cohort_inline_discussions: alwaysCohortInlineDiscussions || false
                 };
             };
 

openedx/core/djangoapps/course_groups/migrations/0002_change_inline_default_cohort_value.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('course_groups', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='coursecohortssettings',
+            name='always_cohort_inline_discussions',
+            field=models.BooleanField(default=False),
+        ),
+    ]

openedx/core/djangoapps/course_groups/models.py

@@ -176,8 +176,11 @@ class CourseCohortsSettings(models.Model):
 
     _cohorted_discussions = models.TextField(db_column='cohorted_discussions', null=True, blank=True)  # JSON list
 
+    # Note that although a default value is specified here for always_cohort_inline_discussions (False),
+    # in reality the default value at the time that cohorting is enabled for a course comes from
+    # course_module.always_cohort_inline_discussions (via `migrate_cohort_settings`).
     # pylint: disable=invalid-name
-    always_cohort_inline_discussions = models.BooleanField(default=True)
+    always_cohort_inline_discussions = models.BooleanField(default=False)
 
     @property
     def cohorted_discussions(self):

openedx/core/djangoapps/course_groups/tests/helpers.py

@@ -58,7 +58,7 @@ class CourseCohortSettingsFactory(DjangoModelFactory):
     course_id = SlashSeparatedCourseKey("dummy", "dummy", "dummy")
     cohorted_discussions = json.dumps([])
     # pylint: disable=invalid-name
-    always_cohort_inline_discussions = True
+    always_cohort_inline_discussions = False
 
 
 def topic_name_to_id(course, name):
@@ -141,7 +141,7 @@ def config_course_cohorts(
         manual_cohorts=[],
         discussion_topics=[],
         cohorted_discussions=[],
-        always_cohort_inline_discussions=True
+        always_cohort_inline_discussions=False
 ):
     """
     Set discussions and configure cohorts for a course.

openedx/core/djangoapps/course_groups/tests/test_cohorts.py

@@ -681,7 +681,7 @@ class TestCohorts(ModuleStoreTestCase):
 
         self.assertFalse(course_cohort_settings.is_cohorted)
         self.assertEqual(course_cohort_settings.cohorted_discussions, [])
-        self.assertTrue(course_cohort_settings.always_cohort_inline_discussions)
+        self.assertFalse(course_cohort_settings.always_cohort_inline_discussions)
 
     def test_update_course_cohort_settings(self):
         """
@@ -694,14 +694,14 @@ class TestCohorts(ModuleStoreTestCase):
             course.id,
             is_cohorted=False,
             cohorted_discussions=['topic a id', 'topic b id'],
-            always_cohort_inline_discussions=False
+            always_cohort_inline_discussions=True
         )
 
         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)
+        self.assertTrue(course_cohort_settings.always_cohort_inline_discussions)
 
     def test_update_course_cohort_settings_with_invalid_data_type(self):
         """

openedx/core/djangoapps/course_groups/tests/test_views.py

@@ -189,7 +189,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
         """
         return {
             'is_cohorted': True,
-            'always_cohort_inline_discussions': True,
+            'always_cohort_inline_discussions': False,
             'cohorted_inline_discussions': [],
             'cohorted_course_wide_discussions': [],
             'id': 1
@@ -247,7 +247,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
 
         self.assertEqual(response, expected_response)
 
-        expected_response['always_cohort_inline_discussions'] = False
+        expected_response['always_cohort_inline_discussions'] = True
         response = self.patch_handler(self.course, data=expected_response, handler=course_cohort_settings_handler)
 
         self.assertEqual(response, expected_response)