Refactor server-size code to enable enrollment tracks.

EDUCATOR-11

common/djangoapps/django_comment_common/migrations/0005_coursediscussionsettings.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import openedx.core.djangoapps.xmodule_django.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('django_comment_common', '0004_auto_20161117_1209'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='CourseDiscussionSettings',
+            fields=[
+                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
+                ('course_id', openedx.core.djangoapps.xmodule_django.models.CourseKeyField(help_text=b'Which course are these settings associated with?', unique=True, max_length=255, db_index=True)),
+                ('always_divide_inline_discussions', models.BooleanField(default=False)),
+                ('_divided_discussions', models.TextField(null=True, db_column=b'divided_discussions', blank=True)),
+                ('division_scheme', models.CharField(default=b'none', max_length=20, choices=[(b'none', b'None'), (b'cohort', b'Cohort'), (b'enrollment_track', b'Enrollment Track')])),
+            ],
+        ),
+    ]

common/djangoapps/django_comment_common/models.py

+import json
 import logging
 
 from config_models.models import ConfigurationModel
@@ -162,3 +163,30 @@ class ForumsConfig(ConfigurationModel):
     def __unicode__(self):
         """Simple representation so the admin screen looks less ugly."""
         return u"ForumsConfig: timeout={}".format(self.connection_timeout)
+
+
+class CourseDiscussionSettings(models.Model):
+    course_id = CourseKeyField(
+        unique=True,
+        max_length=255,
+        db_index=True,
+        help_text="Which course are these settings associated with?",
+    )
+    always_divide_inline_discussions = models.BooleanField(default=False)
+    _divided_discussions = models.TextField(db_column='divided_discussions', null=True, blank=True)  # JSON list
+
+    COHORT = 'cohort'
+    ENROLLMENT_TRACK = 'enrollment_track'
+    NONE = 'none'
+    ASSIGNMENT_TYPE_CHOICES = ((NONE, 'None'), (COHORT, 'Cohort'), (ENROLLMENT_TRACK, 'Enrollment Track'))
+    division_scheme = models.CharField(max_length=20, choices=ASSIGNMENT_TYPE_CHOICES, default=NONE)
+
+    @property
+    def divided_discussions(self):
+        """Jsonify the divided_discussions"""
+        return json.loads(self._divided_discussions)
+
+    @divided_discussions.setter
+    def divided_discussions(self, value):
+        """Un-Jsonify the divided_discussions"""
+        self._divided_discussions = json.dumps(value)

common/djangoapps/django_comment_common/tests.py

-from django.test import TestCase
-from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from nose.plugins.attrib import attr
 
+from django.test import TestCase
 from django_comment_common.models import Role
+from models import CourseDiscussionSettings
+from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from openedx.core.djangoapps.course_groups.cohorts import CourseCohortsSettings
 from student.models import CourseEnrollment, User
+from utils import get_course_discussion_settings, set_course_discussion_settings
+from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
 
 
+@attr(shard=1)
 class RoleAssignmentTest(TestCase):
     """
     Basic checks to make sure our Roles get assigned and unassigned as students
@@ -55,3 +64,73 @@ class RoleAssignmentTest(TestCase):
     #     )
     #     self.assertNotIn(student_role, self.student_user.roles.all())
     #     self.assertIn(student_role, another_student.roles.all())
+
+
+@attr(shard=1)
+class CourseDiscussionSettingsTest(ModuleStoreTestCase):
+
+    def setUp(self):
+        super(CourseDiscussionSettingsTest, self).setUp()
+        self.course = CourseFactory.create()
+
+    def test_get_course_discussion_settings(self):
+        discussion_settings = get_course_discussion_settings(self.course.id)
+        self.assertEqual(CourseDiscussionSettings.NONE, discussion_settings.division_scheme)
+        self.assertEqual([], discussion_settings.divided_discussions)
+        self.assertFalse(discussion_settings.always_divide_inline_discussions)
+
+    def test_get_course_discussion_settings_legacy_settings(self):
+        self.course.cohort_config = {
+            'cohorted': True,
+            'always_cohort_inline_discussions': True,
+            'cohorted_discussions': ['foo']
+        }
+        modulestore().update_item(self.course, ModuleStoreEnum.UserID.system)
+        discussion_settings = get_course_discussion_settings(self.course.id)
+        self.assertEqual(CourseDiscussionSettings.COHORT, discussion_settings.division_scheme)
+        self.assertEqual(['foo'], discussion_settings.divided_discussions)
+        self.assertTrue(discussion_settings.always_divide_inline_discussions)
+
+    def test_get_course_discussion_settings_cohort_settings(self):
+        CourseCohortsSettings.objects.get_or_create(
+            course_id=self.course.id,
+            defaults={
+                'is_cohorted': True,
+                'always_cohort_inline_discussions': True,
+                'cohorted_discussions': ['foo', 'bar']
+            }
+        )
+        discussion_settings = get_course_discussion_settings(self.course.id)
+        self.assertEqual(CourseDiscussionSettings.COHORT, discussion_settings.division_scheme)
+        self.assertEqual(['foo', 'bar'], discussion_settings.divided_discussions)
+        self.assertTrue(discussion_settings.always_divide_inline_discussions)
+
+    def test_set_course_discussion_settings(self):
+        set_course_discussion_settings(
+            course_key=self.course.id,
+            divided_discussions=['cohorted_topic'],
+            division_scheme=CourseDiscussionSettings.ENROLLMENT_TRACK,
+            always_divide_inline_discussions=True,
+        )
+        discussion_settings = get_course_discussion_settings(self.course.id)
+        self.assertEqual(CourseDiscussionSettings.ENROLLMENT_TRACK, discussion_settings.division_scheme)
+        self.assertEqual(['cohorted_topic'], discussion_settings.divided_discussions)
+        self.assertTrue(discussion_settings.always_divide_inline_discussions)
+
+    def test_invalid_data_types(self):
+        exception_msg_template = "Incorrect field type for `{}`. Type must be `{}`"
+        fields = [
+            {'name': 'division_scheme', 'type': str},
+            {'name': 'always_divide_inline_discussions', 'type': bool},
+            {'name': 'divided_discussions', 'type': list}
+        ]
+        invalid_value = 3.14
+
+        for field in fields:
+            with self.assertRaises(ValueError) as value_error:
+                set_course_discussion_settings(self.course.id, **{field['name']: invalid_value})
+
+            self.assertEqual(
+                value_error.exception.message,
+                exception_msg_template.format(field['name'], field['type'].__name__)
+            )

common/djangoapps/django_comment_common/utils.py

@@ -9,6 +9,10 @@ from django_comment_common.models import (
     FORUM_ROLE_STUDENT,
     Role
 )
+from openedx.core.djangoapps.course_groups.cohorts import get_legacy_discussion_settings
+from request_cache.middleware import request_cached
+
+from .models import CourseDiscussionSettings
 
 
 class ThreadContext(object):
@@ -91,3 +95,47 @@ def are_permissions_roles_seeded(course_id):
             return False
 
     return True
+
+
+@request_cached
+def get_course_discussion_settings(course_key):
+    try:
+        course_discussion_settings = CourseDiscussionSettings.objects.get(course_id=course_key)
+    except CourseDiscussionSettings.DoesNotExist:
+        legacy_discussion_settings = get_legacy_discussion_settings(course_key)
+        course_discussion_settings, _ = CourseDiscussionSettings.objects.get_or_create(
+            course_id=course_key,
+            defaults={
+                'always_divide_inline_discussions': legacy_discussion_settings['always_cohort_inline_discussions'],
+                'divided_discussions': legacy_discussion_settings['cohorted_discussions'],
+                'division_scheme': CourseDiscussionSettings.COHORT if legacy_discussion_settings['is_cohorted']
+                else CourseDiscussionSettings.NONE
+            }
+        )
+
+    return course_discussion_settings
+
+
+def set_course_discussion_settings(course_key, **kwargs):
+    """
+    Set discussion settings for a course.
+
+    Arguments:
+        course_key: CourseKey
+        always_divide_inline_discussions (bool): If inline discussions should always be divided.
+        divided_discussions (list): List of discussion ids.
+        division_scheme (str): `CourseDiscussionSettings.NONE`, `CourseDiscussionSettings.COHORT`,
+            or `CourseDiscussionSettings.ENROLLMENT_TRACK`
+
+    Returns:
+        A CourseDiscussionSettings object.
+    """
+    fields = {'division_scheme': str, 'always_divide_inline_discussions': bool, 'divided_discussions': list}
+    course_discussion_settings = get_course_discussion_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_discussion_settings, field, kwargs[field])
+        course_discussion_settings.save()
+    return course_discussion_settings

common/lib/xmodule/xmodule/partitions/partitions_service.py

@@ -133,7 +133,7 @@ class PartitionService(object):
         if self._cache and (cache_key in self._cache):
             return self._cache[cache_key]
 
-        user_partition = self._get_user_partition(user_partition_id)
+        user_partition = self.get_user_partition(user_partition_id)
         if user_partition is None:
             raise ValueError(
                 "Configuration problem!  No user_partition with id {0} "
@@ -148,7 +148,7 @@ class PartitionService(object):
 
         return group_id
 
-    def _get_user_partition(self, user_partition_id):
+    def get_user_partition(self, user_partition_id):
         """
         Look for a user partition with a matching id in the course's partitions.
         Note that this method can return an inactive user partition.

common/static/common/js/discussion/views/discussion_thread_list_view.js

@@ -32,8 +32,8 @@
                 this.retrieveFollowed = function() {
                     return DiscussionThreadListView.prototype.retrieveFollowed.apply(self, arguments);
                 };
-                this.chooseCohort = function() {
-                    return DiscussionThreadListView.prototype.chooseCohort.apply(self, arguments);
+                this.chooseGroup = function() {
+                    return DiscussionThreadListView.prototype.chooseGroup.apply(self, arguments);
                 };
                 this.chooseFilter = function() {
                     return DiscussionThreadListView.prototype.chooseFilter.apply(self, arguments);
@@ -85,7 +85,7 @@
                 'click .forum-nav-thread-link': 'threadSelected',
                 'click .forum-nav-load-more-link': 'loadMorePages',
                 'change .forum-nav-filter-main-control': 'chooseFilter',
-                'change .forum-nav-filter-cohort-control': 'chooseCohort'
+                'change .forum-nav-filter-cohort-control': 'chooseGroup'
             };
 
             DiscussionThreadListView.prototype.initialize = function(options) {
@@ -194,7 +194,7 @@
                 edx.HtmlUtils.append(
                     this.$el,
                     this.template({
-                        isCohorted: this.courseSettings.get('is_cohorted'),
+                        isDiscussionDivisionEnabled: this.courseSettings.get('is_discussion_division_enabled'),
                         isPrivilegedUser: DiscussionUtil.isPrivilegedUser()
                     })
                 );
@@ -404,7 +404,7 @@
                         return $(elem).data('discussion-id');
                     }).get();
                     this.retrieveDiscussions(discussionIds);
-                    return this.$('.forum-nav-filter-cohort').toggle($item.data('cohorted') === true);
+                    return this.$('.forum-nav-filter-cohort').toggle($item.data('divided') === true);
                 }
             };
 
@@ -413,7 +413,7 @@
                 return this.retrieveFirstPage();
             };
 
-            DiscussionThreadListView.prototype.chooseCohort = function() {
+            DiscussionThreadListView.prototype.chooseGroup = function() {
                 this.group_id = this.$('.forum-nav-filter-cohort-control :selected').val();
                 return this.retrieveFirstPage();
             };

common/static/common/js/discussion/views/discussion_topic_menu_view.js

@@ -35,7 +35,7 @@
                         '[data-discussion-id="' + this.getCurrentTopicId() + '"]'
                     ));
                 } else if ($general.length > 0) {
-                    this.setTopic($general);
+                    this.setTopic($general.first());
                 } else {
                     this.setTopic(this.$('.post-topic option').first());
                 }

common/static/common/js/discussion/views/new_post_view.js

@@ -51,7 +51,7 @@
                     threadTypeTemplate;
                 context = _.clone(this.course_settings.attributes);
                 _.extend(context, {
-                    cohort_options: this.getCohortOptions(),
+                    group_options: this.getGroupOptions(),
                     is_commentable_divided: this.is_commentable_divided,
                     mode: this.mode,
                     startHeader: this.startHeader,
@@ -84,15 +84,15 @@
                 return this.mode === 'tab';
             };
 
-            NewPostView.prototype.getCohortOptions = function() {
+            NewPostView.prototype.getGroupOptions = function() {
                 var userGroupId;
-                if (this.course_settings.get('is_cohorted') && DiscussionUtil.isPrivilegedUser()) {
+                if (this.course_settings.get('is_discussion_division_enabled') && DiscussionUtil.isPrivilegedUser()) {
                     userGroupId = $('#discussion-container').data('user-group-id');
-                    return _.map(this.course_settings.get('cohorts'), function(cohort) {
+                    return _.map(this.course_settings.get('groups'), function(group) {
                         return {
-                            value: cohort.id,
-                            text: cohort.name,
-                            selected: cohort.id === userGroupId
+                            value: group.id,
+                            text: group.name,
+                            selected: group.id === userGroupId
                         };
                     });
                 } else {
@@ -112,7 +112,7 @@
             };
 
             NewPostView.prototype.toggleGroupDropdown = function($target) {
-                if ($target.data('cohorted')) {
+                if ($target.data('divided')) {
                     $('.js-group-select').prop('disabled', false);
                     return $('.group-selector-wrapper').removeClass('disabled');
                 } else {

common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js

@@ -62,7 +62,7 @@
                 '                            <li' +
                 '                                class="forum-nav-browse-menu-item"' +
                 '                                data-discussion-id="child"' +
-                '                                data-cohorted="false"' +
+                '                                data-divided="false"' +
                 '                            >' +
                 '                                <a href="#" class="forum-nav-browse-title">Child</a>' +
                 '                            </li>' +
@@ -70,7 +70,7 @@
                 '                    <li' +
                 '                        class="forum-nav-browse-menu-item"' +
                 '                        data-discussion-id="sibling"' +
-                '                        data-cohorted="false"' +
+                '                        data-divided="false"' +
                 '                    >' +
                 '                        <a href="#" class="forum-nav-browse-title">Sibling</a>' +
                 '                    </li>' +
@@ -79,7 +79,7 @@
                 '            <li' +
                 '                class="forum-nav-browse-menu-item"' +
                 '                data-discussion-id="other"' +
-                '                data-cohorted="true"' +
+                '                data-divided="true"' +
                 '            >' +
                 '                <a href="#" class="forum-nav-browse-title">Other Category</a>' +
                 '            </li>' +
@@ -95,11 +95,11 @@
                 '                    <option value="flagged">Flagged</option>' +
                 '                </select>' +
                 '            </label>' +
-                '            <% if (isCohorted && isPrivilegedUser) { %>' +
+                '            <% if (isDiscussionDivisionEnabled && isPrivilegedUser) { %>' +
                 '            <label class="forum-nav-filter-cohort">' +
-                '                <span class="sr">Cohort:</span>' +
+                '                <span class="sr">Group:</span>' +
                 '                <select class="forum-nav-filter-cohort-control">' +
-                '                    <option value="">in all cohorts</option>' +
+                '                    <option value="">in all groups</option>' +
                 '                    <option value="1">Cohort1</option>' +
                 '                    <option value="2">Cohort2</option>' +
                 '                </select>' +
@@ -164,7 +164,7 @@
                 collection: this.discussion,
                 el: $('#fixture-element'),
                 courseSettings: new DiscussionCourseSettings({
-                    is_cohorted: true
+                    is_discussion_division_enabled: true
                 })
             });
             return this.view.render();
@@ -199,7 +199,7 @@
                         collection: discussion,
                         showThreadPreview: true,
                         courseSettings: new DiscussionCourseSettings({
-                            is_cohorted: true
+                            is_discussion_division_enabled: true
                         })
                     },
                     props
@@ -233,7 +233,7 @@
             });
         });
 
-        describe('cohort selector', function() {
+        describe('group selector', function() {
             it('should not be visible to students', function() {
                 return expect(this.view.$('.forum-nav-filter-cohort-control:visible')).not.toExist();
             });

common/static/common/js/spec/discussion/view/new_post_view_spec.js

@@ -31,7 +31,7 @@
                 return expect(group_disabled).toEqual(true);
             }
         };
-        describe('cohort selector', function() {
+        describe('group selector', function() {
             beforeEach(function() {
                 this.course_settings = new DiscussionCourseSettings({
                     category_map: {
@@ -53,8 +53,8 @@
                     },
                     allow_anonymous: false,
                     allow_anonymous_to_peers: false,
-                    is_cohorted: true,
-                    cohorts: [
+                    is_discussion_division_enabled: true,
+                    groups: [
                         {
                             id: 1,
                             name: 'Cohort1'
@@ -75,15 +75,15 @@
             it('is not visible to students', function() {
                 return checkVisibility(this.view, false, false, true);
             });
-            it('allows TAs to see the cohort selector when the topic is cohorted', function() {
+            it('allows TAs to see the group selector when the topic is divided', function() {
                 DiscussionSpecHelper.makeTA();
                 return checkVisibility(this.view, true, false, true);
             });
-            it('allows moderators to see the cohort selector when the topic is cohorted', function() {
+            it('allows moderators to see the group selector when the topic is divided', function() {
                 DiscussionSpecHelper.makeModerator();
                 return checkVisibility(this.view, true, false, true);
             });
-            it('only enables the cohort selector when applicable', function() {
+            it('only enables the group selector when applicable', function() {
                 DiscussionSpecHelper.makeModerator();
                 checkVisibility(this.view, true, false, true);
 
@@ -95,7 +95,7 @@
                 $('.post-topic').trigger('change');
                 return checkVisibility(this.view, true, false, false);
             });
-            it('allows the user to make a cohort selection', function() {
+            it('allows the user to make a group selection', function() {
                 var expectedGroupId,
                     self = this;
                 DiscussionSpecHelper.makeModerator();
@@ -116,23 +116,23 @@
                 });
             });
         });
-        describe('always cohort inline discussions ', function() {
+        describe('always divide inline discussions ', function() {
             beforeEach(function() {
                 this.course_settings = new DiscussionCourseSettings({
-                    'category_map': {
-                        'children': [],
-                        'entries': {}
+                    category_map: {
+                        children: [],
+                        entries: {}
                     },
-                    'allow_anonymous': false,
-                    'allow_anonymous_to_peers': false,
-                    'is_cohorted': true,
-                    'cohorts': [
+                    allow_anonymous: false,
+                    allow_anonymous_to_peers: false,
+                    is_discussion_division_enabled: true,
+                    groups: [
                         {
-                            'id': 1,
-                            'name': 'Cohort1'
+                            id: 1,
+                            name: 'Cohort1'
                         }, {
-                            'id': 2,
-                            'name': 'Cohort2'
+                            id: 2,
+                            name: 'Cohort2'
                         }
                     ]
                 });
@@ -143,12 +143,12 @@
                     mode: 'tab'
                 });
             });
-            it('disables the cohort menu if it is set false', function() {
+            it('disables the group menu if it is set false', function() {
                 DiscussionSpecHelper.makeModerator();
                 this.view.is_commentable_divided = false;
                 return checkVisibility(this.view, true, true, true);
             });
-            it('enables the cohort menu if it is set true', function() {
+            it('enables the group menu if it is set true', function() {
                 DiscussionSpecHelper.makeModerator();
                 this.view.is_commentable_divided = true;
                 return checkVisibility(this.view, true, false, true);

common/static/common/js/spec_helpers/discussion_spec_helper.js

@@ -67,7 +67,7 @@
                             }
                         }
                     },
-                    is_cohorted: true,
+                    is_discussion_division_enabled: true,
                     allow_anonymous: false,
                     allow_anonymous_to_peers: false
                 },
@@ -170,7 +170,7 @@
                 '                            <li' +
                 '                                class="forum-nav-browse-menu-item"' +
                 '                                data-discussion-id="child"' +
-                '                                data-cohorted="false"' +
+                '                                data-divided="false"' +
                 '                            >' +
                 '                                <a href="#" class="forum-nav-browse-title">Child</a>' +
                 '                            </li>' +
@@ -178,7 +178,7 @@
                 '                    <li' +
                 '                        class="forum-nav-browse-menu-item"' +
                 '                        data-discussion-id="sibling"' +
-                '                        data-cohorted="false"' +
+                '                        data-divided="false"' +
                 '                    >' +
                 '                        <a href="#" class="forum-nav-browse-title">Sibling</a>' +
                 '                    </li>' +
@@ -187,7 +187,7 @@
                 '            <li' +
                 '                class="forum-nav-browse-menu-item"' +
                 '                data-discussion-id="other"' +
-                '                data-cohorted="true"' +
+                '                data-divided="true"' +
                 '            >' +
                 '                <a href="#" class="forum-nav-browse-title">Other Category</a>' +
                 '            </li>' +
@@ -203,11 +203,11 @@
                 '                    <option value="flagged">Flagged</option>' +
                 '                </select>' +
                 '            </label>' +
-                '            <% if (isCohorted && isPrivilegedUser) { %>' +
+                '            <% if (isDiscussionDivisionEnabled && isPrivilegedUser) { %>' +
                 '            <label class="forum-nav-filter-cohort">' +
-                '                <span class="sr">Cohort:</span>' +
+                '                <span class="sr">Group:</span>' +
                 '                <select class="forum-nav-filter-cohort-control">' +
-                '                    <option value="">in all cohorts</option>' +
+                '                    <option value="">in all groups</option>' +
                 '                    <option value="1">Cohort1</option>' +
                 '                    <option value="2">Cohort2</option>' +
                 '                </select>' +

common/static/common/templates/discussion/new-post-menu-entry.underscore

-<option class="topic-title" data-discussion-id="<%- id %>" data-cohorted="<%- is_divided %>"><%- text %></option>
+<option class="topic-title" data-discussion-id="<%- id %>" data-divided="<%- is_divided %>"><%- text %></option>

common/static/common/templates/discussion/new-post.underscore

@@ -9,7 +9,7 @@
     <% } %>
     <ul class="post-errors" style="display: none"></ul>
     <div class="forum-new-post-form-wrapper"></div>
-    <% if (cohort_options) { %>
+    <% if (group_options) { %>
     <div class="post-field group-selector-wrapper <% if (!is_commentable_divided) { print('disabled'); } %>">
         <label class="field-label">
             <span class="field-label-text">
@@ -17,12 +17,12 @@
                 <%- gettext("Visible to") %>
             </span>
             <div class="field-help" id="field_help_visible_to">
-                <%- gettext("Discussion admins, moderators, and TAs can make their posts visible to all students or specify a single cohort.") %>
+                <%- gettext("Discussion admins, moderators, and TAs can make their posts visible to all students or specify a single group.") %>
             </div>
             <div class="field-input">
                 <select aria-describedby="field_help_visible_to" class="post-topic field-input js-group-select" name="group_id" <% if (!is_commentable_divided) { print("disabled"); } %>>
                     <option value=""><%- gettext("All Groups") %></option>
-                    <% _.each(cohort_options, function(opt) { %>
+                    <% _.each(group_options, function(opt) { %>
                     <option value="<%- opt.value %>" <% if (opt.selected) { print("selected"); } %>><%- opt.text %></option>
                     <% }); %>
                 </select>

lms/djangoapps/discussion/static/discussion/js/spec/discussion_board_factory_spec.js

@@ -41,10 +41,10 @@ define(
                     thread_pages: [],
                     contentInfo: null,
                     courseSettings: {
-                        is_cohorted: false,
+                        is_discussion_division_enabled: false,
                         allow_anonymous: false,
                         allow_anonymous_to_peers: false,
-                        cohorts: [],
+                        groups: [],
                         category_map: {}
                     }
                 });

lms/djangoapps/discussion/tests/test_views.py

@@ -125,6 +125,7 @@ def make_mock_thread_data(
         group_id=None,
         group_name=None,
         commentable_id=None,
+        is_commentable_divided=None,
 ):
     data_commentable_id = (
         commentable_id or course.discussion_topics.get('General', {}).get('id') or "dummy_commentable_id"
@@ -145,6 +146,8 @@ def make_mock_thread_data(
     }
     if group_id is not None:
         thread_data['group_name'] = group_name
+    if is_commentable_divided is not None:
+        thread_data['is_commentable_divided'] = is_commentable_divided
     if num_children is not None:
         thread_data["children"] = [{
             "id": "dummy_comment_id_{}".format(i),
@@ -429,7 +432,10 @@ class SingleCohortedThreadTestCase(CohortedTestCase):
         self.mock_text = "dummy content"
         self.mock_thread_id = "test_thread_id"
         mock_request.side_effect = make_mock_request_impl(
-            course=self.course, text=self.mock_text, thread_id=self.mock_thread_id, group_id=self.student_cohort.id
+            course=self.course, text=self.mock_text,
+            thread_id=self.mock_thread_id,
+            group_id=self.student_cohort.id,
+            commentable_id="cohorted_topic",
         )
 
     def test_ajax(self, mock_request):
@@ -453,11 +459,13 @@ class SingleCohortedThreadTestCase(CohortedTestCase):
             response_data["content"],
             make_mock_thread_data(
                 course=self.course,
+                commentable_id="cohorted_topic",
                 text=self.mock_text,
                 thread_id=self.mock_thread_id,
                 num_children=1,
                 group_id=self.student_cohort.id,
-                group_name=self.student_cohort.name
+                group_name=self.student_cohort.name,
+                is_commentable_divided=True,
             )
         )
 

lms/djangoapps/discussion/views.py

@@ -30,17 +30,14 @@ from web_fragments.fragment import Fragment
 
 from courseware.courses import get_course_with_access
 from courseware.views.views import CourseTabView
-from openedx.core.djangoapps.course_groups.cohorts import (
-    is_course_cohorted,
-    get_course_cohorts,
-)
 from openedx.core.djangoapps.plugin_api.views import EdxFragmentView
 
 from courseware.access import has_access
 from student.models import CourseEnrollment
 from xmodule.modulestore.django import modulestore
 
-from django_comment_common.utils import ThreadContext
+from django_comment_common.utils import ThreadContext, get_course_discussion_settings
+
 from django_comment_client.permissions import has_permission, get_team
 from django_comment_client.utils import (
     merge_dict,
@@ -48,6 +45,8 @@ from django_comment_client.utils import (
     strip_none,
     add_courseware_context,
     get_group_id_for_comments_service,
+    course_discussion_division_enabled,
+    get_group_names_by_id,
     is_commentable_divided,
     get_group_id_for_user,
 )
@@ -83,11 +82,15 @@ def make_course_settings(course, user):
     Generate a JSON-serializable model for course settings, which will be used to initialize a
     DiscussionCourseSettings object on the client.
     """
+    course_discussion_settings = get_course_discussion_settings(course.id)
+    group_names_by_id = get_group_names_by_id(course_discussion_settings)
     return {
-        'is_cohorted': is_course_cohorted(course.id),
+        'is_discussion_division_enabled': course_discussion_division_enabled(course_discussion_settings),
         'allow_anonymous': course.allow_anonymous,
         'allow_anonymous_to_peers': course.allow_anonymous_to_peers,
-        'cohorts': [{"id": str(g.id), "name": g.name} for g in get_course_cohorts(course)],
+        'groups': [
+            {"id": str(group_id), "name": group_name} for group_id, group_name in group_names_by_id.iteritems()
+        ],
         'category_map': utils.get_discussion_category_map(course, user)
     }
 
@@ -346,10 +349,11 @@ def _find_thread(request, course, discussion_id, thread_id):
     if thread_context == "course" and not utils.discussion_category_id_access(course, request.user, discussion_id):
         return None
 
-    # verify that the thread belongs to the requesting student's cohort
+    # verify that the thread belongs to the requesting student's group
     is_moderator = has_permission(request.user, "see_all_cohorts", course.id)
-    if is_commentable_divided(course.id, discussion_id) and not is_moderator:
-        user_group_id = get_group_id_for_user(request.user, course.id)
+    course_discussion_settings = get_course_discussion_settings(course.id)
+    if is_commentable_divided(course.id, discussion_id, course_discussion_settings) and not is_moderator:
+        user_group_id = get_group_id_for_user(request.user, course_discussion_settings)
         if getattr(thread, "group_id", None) is not None and user_group_id != thread.group_id:
             return None
 
@@ -424,7 +428,8 @@ def _create_discussion_board_context(request, course_key, discussion_id=None, th
         add_courseware_context(threads, course, user)
 
     with newrelic_function_trace("get_cohort_info"):
-        user_group_id = get_group_id_for_user(user, course_key)
+        course_discussion_settings = get_course_discussion_settings(course_key)
+        user_group_id = get_group_id_for_user(user, course_discussion_settings)
 
     context.update({
         'root_url': root_url,
@@ -434,12 +439,12 @@ def _create_discussion_board_context(request, course_key, discussion_id=None, th
         'thread_pages': thread_pages,
         'annotated_content_info': annotated_content_info,
         'is_moderator': has_permission(user, "see_all_cohorts", course_key),
-        'cohorts': course_settings["cohorts"],  # still needed to render _thread_list_template
+        'groups': course_settings["groups"],  # still needed to render _thread_list_template
         'user_group_id': user_group_id,  # read from container in NewPostView
         'sort_preference': cc_user.default_sort_key,
         'category_map': course_settings["category_map"],
         'course_settings': course_settings,
-        'is_commentable_divided': is_commentable_divided(course_key, discussion_id)
+        'is_commentable_divided': is_commentable_divided(course_key, discussion_id, course_discussion_settings)
     })
     return context
 
@@ -501,7 +506,8 @@ def user_profile(request, course_key, user_id):
             ).order_by("name").values_list("name", flat=True).distinct()
 
             with newrelic_function_trace("get_cohort_info"):
-                user_group_id = get_group_id_for_user(request.user, course_key)
+                course_discussion_settings = get_course_discussion_settings(course_key)
+                user_group_id = get_group_id_for_user(request.user, course_discussion_settings)
 
             context = _create_base_discussion_view_context(request, course_key)
             context.update({

lms/djangoapps/discussion_api/api.py

@@ -43,6 +43,8 @@ from django_comment_common.signals import (
     comment_voted,
     comment_deleted,
 )
+from django_comment_common.utils import get_course_discussion_settings
+
 from django_comment_client.utils import get_accessible_discussion_xblocks, is_commentable_divided, get_group_id_for_user
 from lms.djangoapps.discussion_api.pagination import DiscussionAPIPagination
 from lms.lib.comment_client.comment import Comment
@@ -109,12 +111,13 @@ def _get_thread_and_context(request, thread_id, retrieve_kwargs=None):
         course_key = CourseKey.from_string(cc_thread["course_id"])
         course = _get_course(course_key, request.user)
         context = get_context(course, request, cc_thread)
+        course_discussion_settings = get_course_discussion_settings(course_key)
         if (
                 not context["is_requester_privileged"] and
                 cc_thread["group_id"] and
-                is_commentable_divided(course.id, cc_thread["commentable_id"])
+                is_commentable_divided(course.id, cc_thread["commentable_id"], course_discussion_settings)
         ):
-            requester_group_id = get_group_id_for_user(request.user, course.id)
+            requester_group_id = get_group_id_for_user(request.user, course_discussion_settings)
             if requester_group_id is not None and cc_thread["group_id"] != requester_group_id:
                 raise ThreadNotFoundError("Thread not found.")
         return cc_thread, context
@@ -546,7 +549,7 @@ def get_thread_list(
         "user_id": unicode(request.user.id),
         "group_id": (
             None if context["is_requester_privileged"] else
-            get_group_id_for_user(request.user, course.id)
+            get_group_id_for_user(request.user, get_course_discussion_settings(course.id))
         ),
         "page": page,
         "per_page": page_size,
@@ -828,12 +831,13 @@ def create_thread(request, thread_data):
 
     context = get_context(course, request)
     _check_initializable_thread_fields(thread_data, context)
+    discussion_settings = get_course_discussion_settings(course_key)
     if (
             "group_id" not in thread_data and
-            is_commentable_divided(course_key, thread_data.get("topic_id"))
+            is_commentable_divided(course_key, thread_data.get("topic_id"), discussion_settings)
     ):
         thread_data = thread_data.copy()
-        thread_data["group_id"] = get_group_id_for_user(user, course_key)
+        thread_data["group_id"] = get_group_id_for_user(user, discussion_settings)
     serializer = ThreadSerializer(data=thread_data, context=context)
     actions_form = ThreadActionsForm(thread_data)
     if not (serializer.is_valid() and actions_form.is_valid()):

lms/djangoapps/discussion_api/permissions.py

@@ -77,7 +77,7 @@ def get_editable_fields(cc_content, context):
         ret |= {"following", "read"}
         if _is_author_or_privileged(cc_content, context):
             ret |= {"topic_id", "type", "title"}
-        if context["is_requester_privileged"] and context["course"].is_cohorted:
+        if context["is_requester_privileged"] and context["discussion_division_enabled"]:
             ret |= {"group_id"}
 
     # Comment fields

lms/djangoapps/discussion_api/serializers.py

@@ -17,6 +17,7 @@ from discussion_api.permissions import (
 )
 from discussion_api.render import render_body
 from django_comment_client.utils import is_comment_too_deep
+from django_comment_common.utils import get_course_discussion_settings
 from django_comment_common.models import (
     FORUM_ROLE_ADMINISTRATOR,
     FORUM_ROLE_COMMUNITY_TA,
@@ -27,7 +28,7 @@ from lms.lib.comment_client.comment import Comment
 from lms.lib.comment_client.thread import Thread
 from lms.lib.comment_client.user import User as CommentClientUser
 from lms.lib.comment_client.utils import CommentClientRequestError
-from openedx.core.djangoapps.course_groups.cohorts import get_cohort_names
+from lms.djangoapps.django_comment_client.utils import course_discussion_division_enabled, get_group_names_by_id
 
 
 def get_context(course, request, thread=None):
@@ -52,12 +53,13 @@ def get_context(course, request, thread=None):
     requester = request.user
     cc_requester = CommentClientUser.from_django_user(requester).retrieve()
     cc_requester["course_id"] = course.id
+    course_discussion_settings = get_course_discussion_settings(course.id)
     return {
         "course": course,
         "request": request,
         "thread": thread,
-        # For now, the only groups are cohorts
-        "group_ids_to_names": get_cohort_names(course),
+        "discussion_division_enabled": course_discussion_division_enabled(course_discussion_settings),
+        "group_ids_to_names": get_group_names_by_id(course_discussion_settings),
         "is_requester_privileged": requester.id in staff_user_ids or requester.id in ta_user_ids,
         "staff_user_ids": staff_user_ids,
         "ta_user_ids": ta_user_ids,

lms/djangoapps/discussion_api/tests/test_api.py

@@ -55,6 +55,7 @@ from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
 from openedx.core.lib.exceptions import CourseNotFoundError, PageNotFoundError
 from student.tests.factories import CourseEnrollmentFactory, UserFactory
 from util.testing import UrlResetMixin
+from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
@@ -591,6 +592,8 @@ class GetThreadListTest(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMix
         self.request.user = self.user
         CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id)
         self.author = UserFactory.create()
+        self.course.cohort_config = {"cohorted": False}
+        modulestore().update_item(self.course, ModuleStoreEnum.UserID.test)
         self.cohort = CohortFactory.create(course_id=self.course.id)
 
     def get_thread_list(
@@ -662,6 +665,8 @@ class GetThreadListTest(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMix
         })
 
     def test_thread_content(self):
+        self.course.cohort_config = {"cohorted": True}
+        modulestore().update_item(self.course, ModuleStoreEnum.UserID.test)
         source_threads = [
             make_minimal_cs_thread({
                 "id": "test_thread_id_0",

lms/djangoapps/discussion_api/tests/test_permissions.py

@@ -25,6 +25,7 @@ def _get_context(requester_id, is_requester_privileged, is_cohorted=False, threa
         "cc_requester": User(id=requester_id),
         "is_requester_privileged": is_requester_privileged,
         "course": CourseFactory(cohort_config={"cohorted": is_cohorted}),
+        "discussion_division_enabled": is_cohorted,
         "thread": thread,
     }
 

lms/djangoapps/discussion_api/tests/test_serializers.py

@@ -28,6 +28,8 @@ from lms.lib.comment_client.comment import Comment
 from lms.lib.comment_client.thread import Thread
 from student.tests.factories import UserFactory
 from util.testing import UrlResetMixin
+from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
@@ -209,6 +211,8 @@ class ThreadSerializerSerializationTest(SerializerTestMixin, SharedModuleStoreTe
         self.assertEqual(serialized["pinned"], False)
 
     def test_group(self):
+        self.course.cohort_config = {"cohorted": True}
+        modulestore().update_item(self.course, ModuleStoreEnum.UserID.test)
         cohort = CohortFactory.create(course_id=self.course.id)
         serialized = self.serialize(self.make_cs_content({"group_id": cohort.id}))
         self.assertEqual(serialized["group_id"], cohort.id)

lms/djangoapps/django_comment_client/tests/group_id.py

 import json
 import re
+
+from course_modes.models import CourseMode
+from course_modes.tests.factories import CourseModeFactory
+from django_comment_common.models import CourseDiscussionSettings
+from django_comment_common.utils import set_course_discussion_settings
 from lms.djangoapps.teams.tests.factories import CourseTeamFactory
 
 
@@ -94,11 +99,22 @@ class CohortedTopicGroupIdTestMixin(GroupIdAssertionMixin):
 
     def test_cohorted_topic_moderator_with_invalid_group_id(self, mock_request):
         invalid_id = self.student_cohort.id + self.moderator_cohort.id
-        try:
-            response = self.call_view(mock_request, "cohorted_topic", self.moderator, invalid_id)
-            self.assertEqual(response.status_code, 500)
-        except ValueError:
-            pass  # In mock request mode, server errors are not captured
+        response = self.call_view(mock_request, "cohorted_topic", self.moderator, invalid_id)
+        self.assertEqual(response.status_code, 500)
+
+    def test_cohorted_topic_enrollment_track_invalid_group_id(self, mock_request):
+        CourseModeFactory.create(course_id=self.course.id, mode_slug=CourseMode.AUDIT)
+        CourseModeFactory.create(course_id=self.course.id, mode_slug=CourseMode.VERIFIED)
+        set_course_discussion_settings(
+            course_key=self.course.id,
+            divided_discussions=['cohorted_topic'],
+            division_scheme=CourseDiscussionSettings.ENROLLMENT_TRACK,
+            always_divide_inline_discussions=True,
+        )
+
+        invalid_id = -1000
+        response = self.call_view(mock_request, "cohorted_topic", self.moderator, invalid_id)
+        self.assertEqual(response.status_code, 500)
 
 
 class NonCohortedTopicGroupIdTestMixin(GroupIdAssertionMixin):

lms/djangoapps/instructor/tests/test_api.py

@@ -67,7 +67,7 @@ from lms.djangoapps.instructor_task.api_helper import AlreadyRunningError
 from certificates.tests.factories import GeneratedCertificateFactory
 from certificates.models import CertificateStatuses
 
-from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings
+from openedx.core.djangoapps.course_groups.cohorts import set_course_cohorted
 from openedx.core.lib.xblock_utils import grade_histogram
 from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
 from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin
@@ -2701,7 +2701,7 @@ class TestInstructorAPILevelsDataDump(SharedModuleStoreTestCase, LoginEnrollment
         cohorted, and does not when the course is not cohorted.
         """
         url = reverse('get_students_features', kwargs={'course_id': unicode(self.course.id)})
-        set_course_cohort_settings(self.course.id, is_cohorted=is_cohorted)
+        set_course_cohorted(self.course.id, is_cohorted)
 
         response = self.client.post(url, {})
         res_json = json.loads(response.content)

lms/templates/discussion/_filter_dropdown.html

@@ -20,7 +20,7 @@ from openedx.core.djangolib.markup import HTML
         class="forum-nav-browse-menu-item"
         data-discussion-id='${entries[entry]["id"]}'
         id='${entries[entry]["id"]}'
-        data-cohorted="${str(entries[entry]['is_divided']).lower()}"
+        data-divided="${str(entries[entry]['is_divided']).lower()}"
         role="option"
     >
          % if entry:

lms/templates/discussion/_thread_list_template.html

@@ -21,14 +21,14 @@
                     %endif
                 </select>
             ## safe-lint: disable=python-parse-error,python-wrap-html
-            </label>${"<% if (isCohorted && isPrivilegedUser) { %>" | n, decode.utf8}<label class="forum-nav-filter-cohort">
-                ## Translators: This labels a cohort menu in forum navigation
-                <span class="sr">${_("Cohort:")}</span>
+            </label>${"<% if (isDiscussionDivisionEnabled && isPrivilegedUser) { %>" | n, decode.utf8}<label class="forum-nav-filter-cohort">
+                ## Translators: This labels a group menu in forum navigation
+                <span class="sr">${_("Group:")}</span>
                 <select class="forum-nav-filter-cohort-control">
-                    <option value="">${_("in all cohorts")}</option>
-                        ## cohorts is not iterable sometimes because inline discussions xmodule doesn't pass it
-                        %for c in (cohorts or []):
-                            <option value="${c['id']}">${c['name']}</option>
+                    <option value="">${_("in all groups")}</option>
+                        ## groups is not iterable sometimes because inline discussions xmodule doesn't pass it
+                        %for group in (groups or []):
+                            <option value="${group['id']}">${group['name']}</option>
                         %endfor
                 </select>
             ## safe-lint: disable=python-parse-error,python-wrap-html

openedx/core/djangoapps/course_groups/cohorts.py

@@ -118,7 +118,21 @@ def is_course_cohorted(course_key):
     Raises:
        Http404 if the course doesn't exist.
     """
-    return get_course_cohort_settings(course_key).is_cohorted
+    return _get_course_cohort_settings(course_key).is_cohorted
+
+
+def set_course_cohorted(course_key, cohorted):
+    """
+    Given a course course and a boolean, sets whether or not the course is cohorted.
+
+    Raises:
+        Value error if `cohorted` is not a boolean
+    """
+    if not isinstance(cohorted, bool):
+        raise ValueError("Cohorted must be a boolean")
+    course_cohort_settings = _get_course_cohort_settings(course_key)
+    course_cohort_settings.is_cohorted = cohorted
+    course_cohort_settings.save()
 
 
 def get_cohort_id(user, course_key, use_cached=False):
@@ -130,22 +144,6 @@ def get_cohort_id(user, course_key, use_cached=False):
     return None if cohort is None else cohort.id
 
 
-def get_cohorted_commentables(course_key):
-    """
-    Given a course_key return a set of strings representing cohorted commentables.
-    """
-
-    course_cohort_settings = get_course_cohort_settings(course_key)
-
-    if not course_cohort_settings.is_cohorted:
-        # this is the easy case :)
-        ans = set()
-    else:
-        ans = set(course_cohort_settings.cohorted_discussions)
-
-    return ans
-
-
 COHORT_CACHE_NAMESPACE = u"cohorts.get_cohort"
 
 
@@ -213,8 +211,7 @@ def get_cohort(user, course_key, assign=True, use_cached=False):
 
     # First check whether the course is cohorted (users shouldn't be in a cohort
     # in non-cohorted courses, but settings can change after course starts)
-    course_cohort_settings = get_course_cohort_settings(course_key)
-    if not course_cohort_settings.is_cohorted:
+    if not is_course_cohorted(course_key):
         return cache.setdefault(cache_key, None)
 
     # If course is cohorted, check if the user already has a cohort.
@@ -277,11 +274,7 @@ def migrate_cohort_settings(course):
     """
     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
-        }
+        defaults=_get_cohort_settings_from_modulestore(course)
     )
 
     # Add the new and update the existing cohorts
@@ -507,50 +500,49 @@ def is_last_random_cohort(user_group):
     return len(random_cohorts) == 1 and random_cohorts[0].name == user_group.name
 
 
-def set_course_cohort_settings(course_key, **kwargs):
+@request_cached
+def _get_course_cohort_settings(course_key):
     """
-    Set cohort settings for a course.
+    Return cohort settings for a course. NOTE that the only non-deprecated fields in
+    CourseCohortSettings are `course_id` and  `is_cohorted`. Other fields should only be used for
+    migration purposes.
 
     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.
+        A CourseCohortSettings object. NOTE that the only non-deprecated field in
+        CourseCohortSettings are `course_id` and  `is_cohorted`. Other fields should only be used
+        for migration purposes.
 
     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()
+    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
 
 
-@request_cached
-def get_course_cohort_settings(course_key):
-    """
-    Return cohort settings for a course.
-
-    Arguments:
-        course_key: CourseKey
+def get_legacy_discussion_settings(course_key):
 
-    Returns:
-        A CourseCohortSettings object.
-
-    Raises:
-        Http404 if course_key is invalid.
-    """
     try:
         course_cohort_settings = CourseCohortsSettings.objects.get(course_id=course_key)
+        return {
+            'is_cohorted': course_cohort_settings.is_cohorted,
+            'cohorted_discussions': course_cohort_settings.cohorted_discussions,
+            'always_cohort_inline_discussions': course_cohort_settings.always_cohort_inline_discussions
+        }
     except CourseCohortsSettings.DoesNotExist:
         course = courses.get_course_by_id(course_key)
-        course_cohort_settings = migrate_cohort_settings(course)
-    return course_cohort_settings
+        return _get_cohort_settings_from_modulestore(course)
+
+
+def _get_cohort_settings_from_modulestore(course):
+    return {
+        'is_cohorted': course.is_cohorted,
+        'cohorted_discussions': list(course.cohorted_discussions),
+        'always_cohort_inline_discussions': course.always_cohort_inline_discussions
+    }

openedx/core/djangoapps/course_groups/models.py

@@ -168,6 +168,7 @@ class CourseUserGroupPartitionGroup(models.Model):
 class CourseCohortsSettings(models.Model):
     """
     This model represents cohort settings for courses.
+    The only non-deprecated fields are `is_cohorted` and `course_id`.
     """
     is_cohorted = models.BooleanField(default=False)
 
@@ -184,16 +185,23 @@ class CourseCohortsSettings(models.Model):
     # 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
+    # DEPRECATED-- DO NOT USE: Instead use `CourseDiscussionSettings.always_divide_inline_discussions`
+    # via `get_course_discussion_settings` or `set_course_discussion_settings`.
     always_cohort_inline_discussions = models.BooleanField(default=False)
 
     @property
     def cohorted_discussions(self):
-        """Jsonify the cohorted_discussions"""
+        """
+        DEPRECATED-- DO NOT USE. Instead use `CourseDiscussionSettings.divided_discussions`
+        via `get_course_discussion_settings`.
+        """
         return json.loads(self._cohorted_discussions)
 
     @cohorted_discussions.setter
     def cohorted_discussions(self, value):
-        """Un-Jsonify the cohorted_discussions"""
+        """
+        DEPRECATED-- DO NOT USE. Instead use `CourseDiscussionSettings` via `set_course_discussion_settings`.
+        """
         self._cohorted_discussions = json.dumps(value)
 
 

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

@@ -10,7 +10,9 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore import ModuleStoreEnum
 
-from ..cohorts import set_course_cohort_settings
+from ..cohorts import set_course_cohorted
+from django_comment_common.models import CourseDiscussionSettings
+from django_comment_common.utils import set_course_discussion_settings
 from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CohortMembership
 
 
@@ -140,8 +142,8 @@ def config_course_cohorts(
         auto_cohorts=[],
         manual_cohorts=[],
         discussion_topics=[],
-        cohorted_discussions=[],
-        always_cohort_inline_discussions=False
+        divided_discussions=[],
+        always_divide_inline_discussions=False
 ):
     """
     Set discussions and configure cohorts for a course.
@@ -153,10 +155,10 @@ def config_course_cohorts(
         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
+        divided_discussions: Discussion topics to divide. 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.
+        always_divide_inline_discussions (bool): Whether inline discussions
+            should be divided by default.
 
     Returns:
         Nothing -- modifies course in place.
@@ -165,11 +167,12 @@ def config_course_cohorts(
         """Convert name to id."""
         return topic_name_to_id(course, name)
 
-    set_course_cohort_settings(
+    set_course_cohorted(course.id, is_cohorted)
+    set_course_discussion_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
+        divided_discussions=[to_id(name) for name in divided_discussions],
+        always_divide_inline_discussions=always_divide_inline_discussions,
+        division_scheme=CourseDiscussionSettings.COHORT,
     )
 
     for cohort_name in auto_cohorts:

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

@@ -22,8 +22,8 @@ from xmodule.modulestore.tests.factories import ToyCourseFactory
 from ..models import CourseUserGroup, CourseCohort, CourseUserGroupPartitionGroup
 from .. import cohorts
 from ..tests.helpers import (
-    topic_name_to_id, config_course_cohorts, config_course_cohorts_legacy,
-    CohortFactory, CourseCohortFactory, CourseCohortSettingsFactory
+    config_course_cohorts, config_course_cohorts_legacy,
+    CohortFactory, CourseCohortFactory
 )
 
 
@@ -475,44 +475,6 @@ class TestCohorts(ModuleStoreTestCase):
             {cohort1.id: cohort1.name, cohort2.id: cohort2.name}
         )
 
-    def test_get_cohorted_commentables(self):
-        """
-        Make sure cohorts.get_cohorted_commentables() correctly returns a list of strings representing cohorted
-        commentables.  Also verify that we can't get the cohorted commentables from a course which does not exist.
-        """
-        course = modulestore().get_course(self.toy_course_key)
-
-        self.assertEqual(cohorts.get_cohorted_commentables(course.id), set())
-
-        config_course_cohorts(course, is_cohorted=True)
-        self.assertEqual(cohorts.get_cohorted_commentables(course.id), set())
-
-        config_course_cohorts(
-            course,
-            is_cohorted=True,
-            discussion_topics=["General", "Feedback"],
-            cohorted_discussions=["Feedback"]
-        )
-        self.assertItemsEqual(
-            cohorts.get_cohorted_commentables(course.id),
-            set([topic_name_to_id(course, "Feedback")])
-        )
-
-        config_course_cohorts(
-            course,
-            is_cohorted=True,
-            discussion_topics=["General", "Feedback"],
-            cohorted_discussions=["General", "Feedback"]
-        )
-        self.assertItemsEqual(
-            cohorts.get_cohorted_commentables(course.id),
-            set([topic_name_to_id(course, "General"), topic_name_to_id(course, "Feedback")])
-        )
-        self.assertRaises(
-            Http404,
-            lambda: cohorts.get_cohorted_commentables(SlashSeparatedCourseKey("course", "does_not", "exist"))
-        )
-
     def test_get_cohort_by_name(self):
         """
         Make sure cohorts.get_cohort_by_name() properly finds a cohort by name for a given course.  Also verify that it
@@ -672,59 +634,16 @@ class TestCohorts(ModuleStoreTestCase):
         # Note that the following get() will fail with MultipleObjectsReturned if race condition is not handled.
         self.assertEqual(first_cohort.users.get(), course_user)
 
-    def test_get_course_cohort_settings(self):
+    def test_set_cohorted_with_invalid_data_type(self):
         """
-        Test that cohorts.get_course_cohort_settings is working as expected.
+        Test that cohorts.set_course_cohorted raises exception if argument is not a boolean.
         """
         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.assertFalse(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)
+        with self.assertRaises(ValueError) as value_error:
+            cohorts.set_course_cohorted(course.id, 'not a boolean')
 
-        cohorts.set_course_cohort_settings(
-            course.id,
-            is_cohorted=False,
-            cohorted_discussions=['topic a id', 'topic b id'],
-            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.assertTrue(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__)
-            )
+        self.assertEqual("Cohorted must be a boolean", value_error.exception.message)
 
 
 @attr(shard=2)

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

@@ -125,7 +125,7 @@ class CohortViewsTestCase(ModuleStoreTestCase):
             self.course,
             is_cohorted=True,
             discussion_topics=discussion_topics,
-            cohorted_discussions=cohorted_discussions
+            divided_discussions=cohorted_discussions
         )
         return cohorted_inline_discussions, cohorted_course_wide_discussions
 
@@ -317,12 +317,23 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase):
 
         response = self.patch_handler(
             self.course,
+            data={'always_cohort_inline_discussions': ''},
+            expected_response_code=400,
+            handler=course_cohort_settings_handler
+        )
+        self.assertEqual(
+            "Incorrect field type for `{}`. Type must be `{}`".format('always_divide_inline_discussions', bool.__name__),
+            response.get("error")
+        )
+
+        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__),
+            "Cohorted must be a boolean",
             response.get("error")
         )
 

openedx/core/djangoapps/course_groups/views.py

@@ -5,6 +5,7 @@ Views related to course groups functionality.
 import logging
 import re
 
+from courseware.courses import get_course_with_access
 from django.contrib.auth.decorators import login_required
 from django.contrib.auth.models import User
 from django.core.paginator import EmptyPage, Paginator
@@ -14,13 +15,13 @@ from django.http import Http404, HttpResponseBadRequest
 from django.utils.translation import ugettext
 from django.views.decorators.csrf import ensure_csrf_cookie
 from django.views.decorators.http import require_http_methods, require_POST
-from opaque_keys.edx.keys import CourseKey
-from opaque_keys.edx.locations import SlashSeparatedCourseKey
-
-from courseware.courses import get_course_with_access
+from django_comment_common.models import CourseDiscussionSettings
+from django_comment_common.utils import get_course_discussion_settings, set_course_discussion_settings
 from edxmako.shortcuts import render_to_response
 from lms.djangoapps.django_comment_client.constants import TYPE_ENTRY
 from lms.djangoapps.django_comment_client.utils import get_discussion_categories_ids, get_discussion_category_map
+from opaque_keys.edx.keys import CourseKey
+from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from util.json_request import JsonResponse, expect_json
 
 from . import cohorts
@@ -63,20 +64,20 @@ def unlink_cohort_partition_group(cohort):
 
 
 # pylint: disable=invalid-name
-def _get_course_cohort_settings_representation(course, course_cohort_settings):
+def _get_course_cohort_settings_representation(course, is_cohorted, course_discussion_settings):
     """
     Returns a JSON representation of a course cohort settings.
     """
-    cohorted_course_wide_discussions, cohorted_inline_discussions = get_cohorted_discussions(
-        course, course_cohort_settings
+    divided_course_wide_discussions, divided_inline_discussions = get_divided_discussions(
+        course, course_discussion_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,
+        'id': course_discussion_settings.id,
+        'is_cohorted': is_cohorted,
+        'cohorted_inline_discussions': divided_inline_discussions,
+        'cohorted_course_wide_discussions': divided_course_wide_discussions,
+        'always_cohort_inline_discussions': course_discussion_settings.always_divide_inline_discussions,
     }
 
 
@@ -97,23 +98,23 @@ def _get_cohort_representation(cohort, course):
     }
 
 
-def get_cohorted_discussions(course, course_settings):
+def get_divided_discussions(course, discussion_settings):
     """
-    Returns the course-wide and inline cohorted discussion ids separately.
+    Returns the course-wide and inline divided discussion ids separately.
     """
-    cohorted_course_wide_discussions = []
-    cohorted_inline_discussions = []
+    divided_course_wide_discussions = []
+    divided_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)
+    for divided_discussion_id in discussion_settings.divided_discussions:
+        if divided_discussion_id in course_wide_discussions:
+            divided_course_wide_discussions.append(divided_discussion_id)
+        elif divided_discussion_id in all_discussions:
+            divided_inline_discussions.append(divided_discussion_id)
 
-    return cohorted_course_wide_discussions, cohorted_inline_discussions
+    return divided_course_wide_discussions, divided_inline_discussions
 
 
 @require_http_methods(("GET", "PATCH"))
@@ -131,11 +132,12 @@ def course_cohort_settings_handler(request, course_key_string):
     """
     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)
+    is_cohorted = cohorts.is_course_cohorted(course_key)
+    discussion_settings = get_course_discussion_settings(course_key)
 
     if request.method == 'PATCH':
-        cohorted_course_wide_discussions, cohorted_inline_discussions = get_cohorted_discussions(
-            course, cohort_settings
+        divided_course_wide_discussions, divided_inline_discussions = get_divided_discussions(
+            course, discussion_settings
         )
 
         settings_to_change = {}
@@ -144,16 +146,16 @@ def course_cohort_settings_handler(request, course_key_string):
             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
+            divided_course_wide_discussions = request.json.get(
+                'cohorted_course_wide_discussions', divided_course_wide_discussions
             )
-            cohorted_inline_discussions = request.json.get(
-                'cohorted_inline_discussions', cohorted_inline_discussions
+            divided_inline_discussions = request.json.get(
+                'cohorted_inline_discussions', divided_inline_discussions
             )
-            settings_to_change['cohorted_discussions'] = cohorted_course_wide_discussions + cohorted_inline_discussions
+            settings_to_change['divided_discussions'] = divided_course_wide_discussions + divided_inline_discussions
 
         if 'always_cohort_inline_discussions' in request.json:
-            settings_to_change['always_cohort_inline_discussions'] = request.json.get(
+            settings_to_change['always_divide_inline_discussions'] = request.json.get(
                 'always_cohort_inline_discussions'
             )
 
@@ -161,14 +163,19 @@ def course_cohort_settings_handler(request, course_key_string):
             return JsonResponse({"error": unicode("Bad Request")}, 400)
 
         try:
-            cohort_settings = cohorts.set_course_cohort_settings(
-                course_key, **settings_to_change
-            )
+            if 'is_cohorted' in settings_to_change:
+                is_cohorted = settings_to_change['is_cohorted']
+                cohorts.set_course_cohorted(course_key, is_cohorted)
+                del settings_to_change['is_cohorted']
+                settings_to_change['division_scheme'] = CourseDiscussionSettings.COHORT if is_cohorted \
+                    else CourseDiscussionSettings.NONE
+            if settings_to_change:
+                discussion_settings = set_course_discussion_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))
+    return JsonResponse(_get_course_cohort_settings_representation(course, is_cohorted, discussion_settings))
 
 
 @require_http_methods(("GET", "PUT", "POST", "PATCH"))

openedx/core/djangoapps/verified_track_content/partition_scheme.py

@@ -12,7 +12,7 @@ from course_modes.models import CourseMode
 from student.models import CourseEnrollment
 from opaque_keys.edx.keys import CourseKey
 from openedx.core.djangoapps.verified_track_content.models import VerifiedTrackCohortedCourse
-from xmodule.partitions.partitions import NoSuchUserPartitionGroupError, Group, UserPartition
+from xmodule.partitions.partitions import Group, UserPartition
 
 
 # These IDs must be less than 100 so that they do not overlap with Groups in

openedx/core/djangoapps/verified_track_content/tests/test_models.py

@@ -18,7 +18,7 @@ from xmodule.modulestore.tests.factories import CourseFactory
 from ..models import VerifiedTrackCohortedCourse, DEFAULT_VERIFIED_COHORT_NAME
 from ..tasks import sync_cohort_with_mode
 from openedx.core.djangoapps.course_groups.cohorts import (
-    set_course_cohort_settings, add_cohort, CourseCohort, DEFAULT_COHORT_NAME
+    set_course_cohorted, add_cohort, CourseCohort, DEFAULT_COHORT_NAME
 )
 from openedx.core.djangolib.testing.utils import skip_unless_lms
 
@@ -88,7 +88,7 @@ class TestMoveToVerified(SharedModuleStoreTestCase):
 
     def _enable_cohorting(self):
         """ Turn on cohorting in the course. """
-        set_course_cohort_settings(self.course.id, is_cohorted=True)
+        set_course_cohorted(self.course.id, True)
 
     def _create_verified_cohort(self, name=DEFAULT_VERIFIED_COHORT_NAME):
         """ Create a verified cohort. """