Merge pull request #9142 from edx/diana/context-aware-comment-client

Make context for threads more implicit.

common/djangoapps/django_comment_common/utils.py

 from django_comment_common.models import Role
 
+
+class ThreadContext(object):
+    """ An enumeration that represents the context of a thread. Used primarily by the comments service. """
+    STANDALONE = 'standalone'
+    COURSE = 'course'
+
+
 _STUDENT_ROLE_PERMISSIONS = ["vote", "update_thread", "follow_thread", "unfollow_thread",
                              "update_comment", "create_sub_comment", "unvote", "create_thread",
                              "follow_commentable", "unfollow_commentable", "create_comment", ]

lms/djangoapps/django_comment_client/base/tests.py

@@ -19,7 +19,7 @@ from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin,
 from django_comment_client.tests.utils import CohortedTestCase
 from django_comment_client.tests.unicode import UnicodeTestMixin
 from django_comment_common.models import Role
-from django_comment_common.utils import seed_permissions_roles
+from django_comment_common.utils import seed_permissions_roles, ThreadContext
 from student.tests.factories import CourseEnrollmentFactory, UserFactory, CourseAccessRoleFactory
 from util.testing import UrlResetMixin
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
@@ -240,7 +240,7 @@ class ViewsTestCaseMixin(object):
             data["depth"] = 0
         self._set_mock_request_data(mock_request, data)
 
-    def create_thread_helper(self, mock_request, extra_data=None):
+    def create_thread_helper(self, mock_request, extra_request_data=None, extra_response_data=None):
         """
         Issues a request to create a thread and verifies the result.
         """
@@ -283,8 +283,8 @@ class ViewsTestCaseMixin(object):
             "anonymous": ["false"],
             "title": ["Hello"],
         }
-        if extra_data:
-            thread.update(extra_data)
+        if extra_request_data:
+            thread.update(extra_request_data)
         url = reverse('create_thread', kwargs={'commentable_id': 'i4x-MITx-999-course-Robot_Super_Course',
                                                'course_id': self.course_id.to_deprecated_string()})
         response = self.client.post(url, data=thread)
@@ -292,14 +292,15 @@ class ViewsTestCaseMixin(object):
         expected_data = {
             'thread_type': 'discussion',
             'body': u'this is a post',
+            'context': ThreadContext.COURSE,
             'anonymous_to_peers': False, 'user_id': 1,
             'title': u'Hello',
             'commentable_id': u'i4x-MITx-999-course-Robot_Super_Course',
             'anonymous': False,
             'course_id': unicode(self.course_id),
         }
-        if extra_data:
-            expected_data.update(extra_data)
+        if extra_response_data:
+            expected_data.update(extra_response_data)
         mock_request.assert_called_with(
             'post',
             '{prefix}/i4x-MITx-999-course-Robot_Super_Course/threads'.format(prefix=CS_PREFIX),
@@ -387,9 +388,19 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V
     def test_create_thread(self, mock_request):
         self.create_thread_helper(mock_request)
 
-    def test_create_thread_with_context(self, mock_request):
+    def test_create_thread_standalone(self, mock_request):
+        team = CourseTeamFactory.create(
+            name="A Team",
+            course_id=self.course_id,
+            topic_id='topic_id',
+            discussion_topic_id="i4x-MITx-999-course-Robot_Super_Course"
+        )
+
+        # Add the student to the team so they can post to the commentable.
+        team.add_user(self.student)
+
         # create_thread_helper verifies that extra data are passed through to the comments service
-        self.create_thread_helper(mock_request, extra_data={'context': 'standalone'})
+        self.create_thread_helper(mock_request, extra_response_data={'context': ThreadContext.STANDALONE})
 
     def test_delete_comment(self, mock_request):
         self._set_mock_request_data(mock_request, {

lms/djangoapps/django_comment_client/base/views.py

@@ -18,6 +18,7 @@ from courseware.access import has_access
 from util.file import store_uploaded_file
 from courseware.courses import get_course_with_access, get_course_by_id
 import django_comment_client.settings as cc_settings
+from django_comment_common.utils import ThreadContext
 from django_comment_client.utils import (
     add_courseware_context,
     get_annotated_content_info,
@@ -30,7 +31,7 @@ from django_comment_client.utils import (
     discussion_category_id_access,
     get_cached_discussion_id_map,
 )
-from django_comment_client.permissions import check_permissions_by_view, has_permission
+from django_comment_client.permissions import check_permissions_by_view, has_permission, get_team
 from eventtracking import tracker
 import lms.lib.comment_client as cc
 
@@ -187,8 +188,11 @@ def create_thread(request, course_id, commentable_id):
         'title': post["title"],
     }
 
-    if 'context' in post:
-        params['context'] = post['context']
+    # Check for whether this commentable belongs to a team, and add the right context
+    if get_team(commentable_id) is not None:
+        params['context'] = ThreadContext.STANDALONE
+    else:
+        params['context'] = ThreadContext.COURSE
 
     thread = cc.Thread(**params)
 

lms/djangoapps/django_comment_client/forum/tests.py

@@ -8,7 +8,9 @@ from django.test.client import Client, RequestFactory
 from django.test.utils import override_settings
 from edxmako.tests import mako_middleware_process_request
 
+from django_comment_common.utils import ThreadContext
 from django_comment_client.forum import views
+from django_comment_client.permissions import get_team
 from django_comment_client.tests.group_id import (
     CohortedTopicGroupIdTestMixin,
     NonCohortedTopicGroupIdTestMixin
@@ -33,6 +35,8 @@ from mock import patch, Mock, ANY, call
 
 from openedx.core.djangoapps.course_groups.models import CourseUserGroup
 
+from teams.tests.factories import CourseTeamFactory
+
 log = logging.getLogger(__name__)
 
 # pylint: disable=missing-docstring
@@ -112,21 +116,23 @@ def make_mock_thread_data(
         group_id=None,
         group_name=None,
         commentable_id=None,
-        context=None
 ):
+    data_commentable_id = (
+        commentable_id or course.discussion_topics.get('General', {}).get('id') or "dummy_commentable_id"
+    )
     thread_data = {
         "id": thread_id,
         "type": "thread",
         "title": text,
         "body": text,
-        "commentable_id": (
-            commentable_id or course.discussion_topics.get('General', {}).get('id') or "dummy_commentable_id"
-        ),
+        "commentable_id": data_commentable_id,
         "resp_total": 42,
         "resp_skip": 25,
         "resp_limit": 5,
         "group_id": group_id,
-        "context": context if context else "course"
+        "context": (
+            ThreadContext.COURSE if get_team(data_commentable_id) is None else ThreadContext.STANDALONE
+        )
     }
     if group_id is not None:
         thread_data['group_name'] = group_name
@@ -146,7 +152,6 @@ def make_mock_request_impl(
         group_id=None,
         commentable_id=None,
         num_thread_responses=1,
-        context=None
 ):
     def mock_request_impl(*args, **kwargs):
         url = args[1]
@@ -161,7 +166,6 @@ def make_mock_request_impl(
                         num_children=None,
                         group_id=group_id,
                         commentable_id=commentable_id,
-                        context=context
                     )
                 ]
             }
@@ -172,7 +176,6 @@ def make_mock_request_impl(
                 thread_id=thread_id,
                 num_children=num_thread_responses,
                 group_id=group_id,
-                context=context
             )
         elif "/users/" in url:
             data = {
@@ -672,12 +675,20 @@ class InlineDiscussionContextTestCase(ModuleStoreTestCase):
         super(InlineDiscussionContextTestCase, self).setUp()
         self.course = CourseFactory.create()
         CourseEnrollmentFactory(user=self.user, course_id=self.course.id)
+        self.discussion_topic_id = "dummy_topic"
+        self.team = CourseTeamFactory(
+            name="A team",
+            course_id=self.course.id,
+            topic_id='topic_id',
+            discussion_topic_id=self.discussion_topic_id
+        )
+        self.team.add_user(self.user)  # pylint: disable=no-member
 
     def test_context_can_be_standalone(self, mock_request):
         mock_request.side_effect = make_mock_request_impl(
             course=self.course,
             text="dummy text",
-            context="standalone"
+            commentable_id=self.discussion_topic_id
         )
 
         request = RequestFactory().get("dummy_url")
@@ -686,11 +697,11 @@ class InlineDiscussionContextTestCase(ModuleStoreTestCase):
         response = views.inline_discussion(
             request,
             unicode(self.course.id),
-            "dummy_topic",
+            self.discussion_topic_id,
         )
 
         json_response = json.loads(response.content)
-        self.assertEqual(json_response['discussion_data'][0]['context'], 'standalone')
+        self.assertEqual(json_response['discussion_data'][0]['context'], ThreadContext.STANDALONE)
 
 
 @patch('lms.lib.comment_client.utils.requests.request')
@@ -1041,8 +1052,15 @@ class InlineDiscussionTestCase(ModuleStoreTestCase):
         self.verify_response(self.send_request(mock_request))
 
     def test_context(self, mock_request):
-        response = self.send_request(mock_request, {'context': 'standalone'})
-        self.assertEqual(mock_request.call_args[1]['params']['context'], 'standalone')
+        team = CourseTeamFactory(
+            name='Team Name',
+            topic_id='A topic',
+            course_id=self.course.id,
+            discussion_topic_id=self.discussion1.discussion_id
+        )
+        team.add_user(self.student)  # pylint: disable=no-member
+        response = self.send_request(mock_request)
+        self.assertEqual(mock_request.call_args[1]['params']['context'], ThreadContext.STANDALONE)
         self.verify_response(response)
 
 

lms/djangoapps/django_comment_client/forum/views.py

@@ -30,7 +30,8 @@ from courseware.access import has_access
 from xmodule.modulestore.django import modulestore
 from ccx.overrides import get_current_ccx
 
-from django_comment_client.permissions import has_permission
+from django_comment_common.utils import ThreadContext
+from django_comment_client.permissions import has_permission, get_team
 from django_comment_client.utils import (
     merge_dict,
     extract,
@@ -111,6 +112,7 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
         'text': '',
         'course_id': unicode(course.id),
         'user_id': request.user.id,
+        'context': ThreadContext.COURSE,
         'group_id': get_group_id_for_comments_service(request, course.id, discussion_id),  # may raise ValueError
     }
 
@@ -118,6 +120,9 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
     # comments_service.
     if discussion_id is not None:
         default_query_params['commentable_id'] = discussion_id
+        # Use the discussion id/commentable id to determine the context we are going to pass through to the backend.
+        if get_team(discussion_id) is not None:
+            default_query_params['context'] = ThreadContext.STANDALONE
 
     if not request.GET.get('sort_key'):
         # If the user did not select a sort key, use their last used sort key
@@ -149,7 +154,6 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
                     'flagged',
                     'unread',
                     'unanswered',
-                    'context',
                 ]
             )
         )

lms/djangoapps/django_comment_client/permissions.py

@@ -31,20 +31,40 @@ def has_permission(user, permission, course_id=None):
 CONDITIONS = ['is_open', 'is_author', 'is_question_author', 'is_team_member_if_applicable']
 
 
+def get_team(commentable_id):
+    """ Returns the team that the commentable_id belongs to if it exists. Returns None otherwise. """
+    request_cache_dict = RequestCache.get_request_cache().data
+    cache_key = "django_comment_client.team_commentable.{}".format(commentable_id)
+    if cache_key in request_cache_dict:
+        return request_cache_dict[cache_key]
+
+    try:
+        team = CourseTeam.objects.get(discussion_topic_id=commentable_id)
+    except CourseTeam.DoesNotExist:
+        team = None
+
+    request_cache_dict[cache_key] = team
+    return team
+
+
 def _check_condition(user, condition, content):
-    def check_open(user, content):
+    """ Check whether or not the given condition applies for the given user and content. """
+    def check_open(_user, content):
+        """ Check whether the content is open. """
         try:
             return content and not content['closed']
         except KeyError:
             return False
 
     def check_author(user, content):
+        """ Check if the given user is the author of the content. """
         try:
             return content and content['user_id'] == str(user.id)
         except KeyError:
             return False
 
     def check_question_author(user, content):
+        """ Check if the given user is the author of the original question for both threads and comments. """
         if not content:
             return False
         try:
@@ -69,17 +89,16 @@ def _check_condition(user, condition, content):
             cache_key = "django_comment_client.check_team_member.{}.{}".format(user.id, commentable_id)
             if cache_key in request_cache_dict:
                 return request_cache_dict[cache_key]
-            team = CourseTeam.objects.get(discussion_topic_id=commentable_id)
-            passes_condition = team.users.filter(id=user.id).count() > 0
+            team = get_team(commentable_id)
+            if team is None:
+                passes_condition = True
+            else:
+                passes_condition = team.users.filter(id=user.id).exists()
             request_cache_dict[cache_key] = passes_condition
         except KeyError:
             # We do not expect KeyError in production-- it usually indicates an improper test mock.
             logging.warning("Did not find key commentable_id in content.")
             passes_condition = False
-        except CourseTeam.DoesNotExist:
-            passes_condition = True
-            request_cache_dict[cache_key] = passes_condition
-
         return passes_condition
 
     handlers = {