Merge pull request #4786 from aboudreault/inline-discusssion-cohorting

Add always_cohort_inline_discussions in course settings

common/lib/xmodule/xmodule/course_module.py

@@ -918,6 +918,19 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
         return set(config.get("cohorted_discussions", []))
 
     @property
+    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.
+        """
+        config = self.cohort_config
+        if config is None:
+            return True
+
+        return bool(config.get("always_cohort_inline_discussions", True))
+
+    @property
     def is_newish(self):
         """
         Returns if the course has been flagged as new. If

common/lib/xmodule/xmodule/discussion_module.py

@@ -11,7 +11,11 @@ _ = lambda text: text
 
 
 class DiscussionFields(object):
-    discussion_id = String(scope=Scope.settings, default="$$GUID$$")
+    discussion_id = String(
+        display_name=_("Discussion Id"),
+        help=_("The id is a unique identifier for the discussion. It is non editable."),
+        scope=Scope.settings,
+        default="$$GUID$$")
     display_name = String(
         display_name=_("Display Name"),
         help=_("Display name for this module"),

common/static/coffee/spec/discussion/discussion_spec_helper.coffee

@@ -321,7 +321,7 @@ browser and pasting the output.  When that file changes, this one should be rege
             <label class="field-label">
                 <span class="field-label-text">
                     Visible To:
-                </span><select class="field-input js-group-select" name="group_id">
+                </span><select class="field-input js-group-select" name="group_id" <% if (!is_commentable_cohorted) { %>disabled<% } %>>
                     <option value="">All Groups</option>
                     <% _.each(cohort_options, function(opt) { %>
                     <option value="<%= opt.value %>" <% if (opt.selected) { %>selected<% } %>><%- opt.text %></option>

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

@@ -10,6 +10,15 @@ describe "NewPostView", ->
         )
         @discussion = new Discussion([], {pages: 1})
 
+    checkVisibility = (view, expectedVisible, expectedDisabled) =>
+      view.render()
+      expect(view.$(".js-group-select").is(":visible")).toEqual(expectedVisible)
+      disabled = view.$(".js-group-select").prop("disabled")
+      if expectedVisible and ! expectedDisabled
+        expect(disabled).toEqual(false)
+      else if expectedDisabled
+        expect(disabled).toEqual(true)
+
     describe "cohort selector", ->
       beforeEach ->
         @course_settings = new DiscussionCourseSettings({
@@ -32,12 +41,6 @@ describe "NewPostView", ->
           mode: "tab"
         )
 
-      checkVisibility = (view, expectedVisible) =>
-        view.render()
-        expect(view.$(".js-group-select").is(":visible")).toEqual(expectedVisible)
-        if expectedVisible
-          expect(view.$(".js-group-select").prop("disabled")).toEqual(false)
-
       it "is not visible to students", ->
         checkVisibility(@view, false)
 
@@ -69,6 +72,46 @@ describe "NewPostView", ->
             $.ajax.reset()
         )
 
+    describe "always cohort inline discussions ", ->
+      beforeEach ->
+        @course_settings = new DiscussionCourseSettings({
+          "category_map": {
+            "children": [],
+            "entries": {}
+          },
+          "allow_anonymous": false,
+          "allow_anonymous_to_peers": false,
+          "is_cohorted": true,
+          "cohorts": [
+            {"id": 1, "name": "Cohort1"},
+            {"id": 2, "name": "Cohort2"}
+          ]
+        })
+        @view = new NewPostView(
+          el: $("#fixture-element"),
+          collection: @discussion,
+          course_settings: @course_settings,
+          mode: "tab"
+        )
+
+      it "disables the cohort menu if it is set false", ->
+        DiscussionSpecHelper.makeModerator()
+        @view.is_commentable_cohorted = false
+        checkVisibility(@view, true, true)
+
+      it "enables the cohort menu if it is set true", ->
+        DiscussionSpecHelper.makeModerator()
+        @view.is_commentable_cohorted = true
+        checkVisibility(@view, true)
+
+      it "is not visible to students when set false", ->
+        @view.is_commentable_cohorted = false
+        checkVisibility(@view, false)
+
+      it "is not visible to students when set true", ->
+        @view.is_commentable_cohorted = true
+        checkVisibility(@view, false)
+
     describe "cancel post resets form ", ->
       beforeEach ->
         @course_settings = new DiscussionCourseSettings({

common/static/coffee/src/discussion/discussion_module_view.coffee

@@ -116,7 +116,8 @@ if Backbone?
         el: @newPostForm,
         collection: @discussion,
         course_settings: @course_settings,
-        topicId: discussionId
+        topicId: discussionId,
+        is_commentable_cohorted: response.is_commentable_cohorted
       )
       @newPostView.render()
       @listenTo( @newPostView, 'newPost:cancel', @hideNewPost )

common/static/coffee/src/discussion/views/new_post_view.coffee

@@ -6,12 +6,14 @@ if Backbone?
           if @mode not in ["tab", "inline"]
               throw new Error("invalid mode: " + @mode)
           @course_settings = options.course_settings
+          @is_commentable_cohorted = options.is_commentable_cohorted
           @topicId = options.topicId
 
       render: () ->
           context = _.clone(@course_settings.attributes)
           _.extend(context, {
               cohort_options: @getCohortOptions(),
+              is_commentable_cohorted: @is_commentable_cohorted,
               mode: @mode,
               form_id: @mode + (if @topicId then "-" + @topicId else "")
           })

lms/djangoapps/django_comment_client/forum/views.py

@@ -154,6 +154,7 @@ def inline_discussion(request, course_id, discussion_id):
     with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
         add_courseware_context(threads, course)
     return utils.JsonResponse({
+        'is_commentable_cohorted': is_commentable_cohorted(course_key, discussion_id),
         'discussion_data': threads,
         'user_info': user_info,
         'annotated_content_info': annotated_content_info,

lms/templates/discussion/_discussion_module_studio.html

@@ -4,7 +4,8 @@
     <p>
         <span class="discussion-preview">
             <span class="icon icon-comment"/>
-            ${_("To view live discussions, click Preview or View Live in Unit Settings.")}
+            ${_("To view live discussions, click Preview or View Live in Unit Settings.")}<br />
+            ${_("Discussion ID: {discussion_id}").format(discussion_id=discussion_id)}
         </span>
     </p>
 </div>

lms/templates/discussion/_underscore_templates.html

@@ -390,7 +390,7 @@
                 <span class="field-label-text">
                     ## Translators: This labels the selector for which group of students can view a post
                     ${_("Visible To:")}
-                </span><select class="field-input js-group-select" name="group_id">
+                </span><select class="field-input js-group-select" name="group_id" ${'<% if (!is_commentable_cohorted) { %>'}disabled${'<% } %>'}>
                     <option value="">${_("All Groups")}</option>
                     ${'<% _.each(cohort_options, function(opt) { %>'}
                     <option value="${'<%= opt.value %>'}" ${'<% if (opt.selected) { %>selected<% } %>'}>${'<%- opt.text %>'}</option>

openedx/core/djangoapps/course_groups/cohorts.py

@@ -162,9 +162,13 @@ def is_commentable_cohorted(course_key, commentable_id):
     if not course.is_cohorted:
         # this is the easy case :)
         ans = False
-    elif commentable_id in course.top_level_discussion_topic_ids:
+    elif (
+            commentable_id in course.top_level_discussion_topic_ids or
+            course.always_cohort_inline_discussions is False
+    ):
         # top level discussions have to be manually configured as cohorted
-        # (default is not)
+        # (default is not).
+        # Same thing for inline discussions if the default is explicitly set to False in settings
         ans = commentable_id in course.cohorted_discussions
     else:
         # inline discussions are cohorted by default

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

@@ -46,7 +46,8 @@ def config_course_cohorts(
         discussions,
         cohorted,
         cohorted_discussions=None,
-        auto_cohort_groups=None
+        auto_cohort_groups=None,
+        always_cohort_inline_discussions=None  # pylint: disable=invalid-name
 ):
     """
     Given a course with no discussion set up, add the discussions and set
@@ -74,15 +75,18 @@ def config_course_cohorts(
 
     course.discussion_topics = topics
 
-    d = {"cohorted": cohorted}
+    config = {"cohorted": cohorted}
     if cohorted_discussions is not None:
-        d["cohorted_discussions"] = [to_id(name)
-                                     for name in cohorted_discussions]
+        config["cohorted_discussions"] = [to_id(name)
+                                          for name in cohorted_discussions]
 
     if auto_cohort_groups is not None:
-        d["auto_cohort_groups"] = auto_cohort_groups
+        config["auto_cohort_groups"] = auto_cohort_groups
 
-    course.cohort_config = d
+    if always_cohort_inline_discussions is not None:
+        config["always_cohort_inline_discussions"] = always_cohort_inline_discussions
+
+    course.cohort_config = config
 
     try:
         # Not implemented for XMLModulestore, which is used by test_cohorts.

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

@@ -357,10 +357,6 @@ class TestCohorts(TestCase):
             cohorts.is_commentable_cohorted(course.id, to_id("General")),
             "Course is cohorted, but 'General' isn't."
         )
-        self.assertTrue(
-            cohorts.is_commentable_cohorted(course.id, to_id("random")),
-            "Non-top-level discussion is always cohorted in cohorted courses."
-        )
 
         # cohorted, including "Feedback" top-level topics aren't
         config_course_cohorts(
@@ -379,6 +375,45 @@ class TestCohorts(TestCase):
             "Feedback was listed as cohorted.  Should be."
         )
 
+    def test_is_commentable_cohorted_inline_discussion(self):
+        course = modulestore().get_course(self.toy_course_key)
+        self.assertFalse(course.is_cohorted)
+
+        def to_id(name):  # pylint: disable=missing-docstring
+            return topic_name_to_id(course, name)
+
+        config_course_cohorts(
+            course, ["General", "Feedback"],
+            cohorted=True,
+            cohorted_discussions=["Feedback", "random_inline"]
+        )
+        self.assertTrue(
+            cohorts.is_commentable_cohorted(course.id, to_id("random")),
+            "By default, Non-top-level discussion is always cohorted in cohorted courses."
+        )
+
+        # if always_cohort_inline_discussions is set to False, non-top-level discussion are always
+        # non cohorted unless they are explicitly set in cohorted_discussions
+        config_course_cohorts(
+            course, ["General", "Feedback"],
+            cohorted=True,
+            cohorted_discussions=["Feedback", "random_inline"],
+            always_cohort_inline_discussions=False
+        )
+        self.assertFalse(
+            cohorts.is_commentable_cohorted(course.id, to_id("random")),
+            "Non-top-level discussion is not cohorted if always_cohort_inline_discussions is False."
+        )
+        self.assertTrue(
+            cohorts.is_commentable_cohorted(course.id, to_id("random_inline")),
+            "If always_cohort_inline_discussions set to False, Non-top-level discussion is "
+            "cohorted if explicitly set in cohorted_discussions."
+        )
+        self.assertTrue(
+            cohorts.is_commentable_cohorted(course.id, to_id("Feedback")),
+            "If always_cohort_inline_discussions set to False, top-level discussion are not affected."
+        )
+
     def test_get_cohorted_commentables(self):
         """
         Make sure cohorts.get_cohorted_commentables() correctly returns a list of strings representing cohorted