Merge pull request #9113 from edx/christina/team-discussion-permissions

Christina/team discussion permissions

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/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
 
@@ -51,6 +52,8 @@ def permitted(fn):
                 content = cc.Thread.find(kwargs["thread_id"]).to_dict()
             elif "comment_id" in kwargs:
                 content = cc.Comment.find(kwargs["comment_id"]).to_dict()
+            elif "commentable_id" in kwargs:
+                content = cc.Commentable.find(kwargs["commentable_id"]).to_dict()
             else:
                 content = None
             return content
@@ -185,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)
 
@@ -608,16 +614,6 @@ def follow_commentable(request, course_id, commentable_id):
 @require_POST
 @login_required
 @permitted
-def follow_user(request, course_id, followed_user_id):
-    user = cc.User.from_django_user(request.user)
-    followed_user = cc.User.find(followed_user_id)
-    user.follow(followed_user)
-    return JsonResponse({})
-
-
-@require_POST
-@login_required
-@permitted
 def unfollow_thread(request, course_id, thread_id):
     """
     given a course id and thread id, stop following this thread
@@ -645,20 +641,6 @@ def unfollow_commentable(request, course_id, commentable_id):
 
 @require_POST
 @login_required
-@permitted
-def unfollow_user(request, course_id, followed_user_id):
-    """
-    given a course id and user id, stop following this user
-    ajax only
-    """
-    user = cc.User.from_django_user(request.user)
-    followed_user = cc.User.find(followed_user_id)
-    user.unfollow(followed_user)
-    return JsonResponse({})
-
-
-@require_POST
-@login_required
 @csrf.csrf_exempt
 def upload(request, course_id):  # ajax upload file to a question or answer
     """view that handles file upload via Ajax

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 = {
@@ -333,11 +336,11 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
 
     @ddt.data(
         # old mongo with cache
-        (ModuleStoreEnum.Type.mongo, 1, 7, 5, 13, 8),
-        (ModuleStoreEnum.Type.mongo, 50, 7, 5, 13, 8),
+        (ModuleStoreEnum.Type.mongo, 1, 7, 5, 14, 8),
+        (ModuleStoreEnum.Type.mongo, 50, 7, 5, 14, 8),
         # split mongo: 3 queries, regardless of thread response size.
-        (ModuleStoreEnum.Type.split, 1, 3, 3, 13, 8),
-        (ModuleStoreEnum.Type.split, 50, 3, 3, 13, 8),
+        (ModuleStoreEnum.Type.split, 1, 3, 3, 14, 8),
+        (ModuleStoreEnum.Type.split, 50, 3, 3, 14, 8),
     )
     @ddt.unpack
     def test_number_of_mongo_queries(
@@ -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

@@ -29,7 +29,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

@@ -10,6 +10,7 @@ from lms.lib.comment_client import Thread
 from opaque_keys.edx.keys import CourseKey
 
 from django_comment_common.models import all_permissions_for_user_in_course
+from teams.models import CourseTeam
 
 
 def has_permission(user, permission, course_id=None):
@@ -27,23 +28,43 @@ def has_permission(user, permission, course_id=None):
     return permission in all_permissions
 
 
-CONDITIONS = ['is_open', 'is_author', 'is_question_author']
+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:
@@ -55,10 +76,36 @@ def _check_condition(user, condition, content):
         except KeyError:
             return False
 
+    def check_team_member(user, content):
+        """
+        If the content has a commentable_id, verifies that either it is not associated with a team,
+        or if it is, that the user is a member of that team.
+        """
+        if not content:
+            return False
+        try:
+            commentable_id = content['commentable_id']
+            request_cache_dict = RequestCache.get_request_cache().data
+            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 = 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
+        return passes_condition
+
     handlers = {
         'is_open': check_open,
         'is_author': check_author,
         'is_question_author': check_question_author,
+        'is_team_member_if_applicable': check_team_member
     }
 
     return handlers[condition](user, content)
@@ -86,32 +133,32 @@ def _check_conditions_permissions(user, permissions, course_id, content):
     return test(user, permissions, operator="or")
 
 
+# Note: 'edit_content' is being used as a generic way of telling if someone is a privileged user
+# (forum Moderator/Admin/TA), because there is a desire that team membership does not impact privileged users.
 VIEW_PERMISSIONS = {
     'update_thread': ['edit_content', ['update_thread', 'is_open', 'is_author']],
-    'create_comment': [["create_comment", "is_open"]],
+    'create_comment': ['edit_content', ["create_comment", "is_open", "is_team_member_if_applicable"]],
     'delete_thread': ['delete_thread', ['update_thread', 'is_author']],
     'update_comment': ['edit_content', ['update_comment', 'is_open', 'is_author']],
     'endorse_comment': ['endorse_comment', 'is_question_author'],
     'openclose_thread': ['openclose_thread'],
-    'create_sub_comment': [['create_sub_comment', 'is_open']],
+    'create_sub_comment': ['edit_content', ['create_sub_comment', 'is_open', 'is_team_member_if_applicable']],
     'delete_comment': ['delete_comment', ['update_comment', 'is_open', 'is_author']],
-    'vote_for_comment': [['vote', 'is_open']],
-    'undo_vote_for_comment': [['unvote', 'is_open']],
-    'vote_for_thread': [['vote', 'is_open']],
-    'flag_abuse_for_thread': ['vote'],
-    'un_flag_abuse_for_thread': ['vote'],
-    'flag_abuse_for_comment': ['vote'],
-    'un_flag_abuse_for_comment': ['vote'],
-    'undo_vote_for_thread': [['unvote', 'is_open']],
+    'vote_for_comment': ['edit_content', ['vote', 'is_open', 'is_team_member_if_applicable']],
+    'undo_vote_for_comment': ['edit_content', ['unvote', 'is_open', 'is_team_member_if_applicable']],
+    'vote_for_thread': ['edit_content', ['vote', 'is_open', 'is_team_member_if_applicable']],
+    'flag_abuse_for_thread': ['edit_content', ['vote', 'is_team_member_if_applicable']],
+    'un_flag_abuse_for_thread': ['edit_content', ['vote', 'is_team_member_if_applicable']],
+    'flag_abuse_for_comment': ['edit_content', ['vote', 'is_team_member_if_applicable']],
+    'un_flag_abuse_for_comment': ['edit_content', ['vote', 'is_team_member_if_applicable']],
+    'undo_vote_for_thread': ['edit_content', ['unvote', 'is_open', 'is_team_member_if_applicable']],
     'pin_thread': ['openclose_thread'],
     'un_pin_thread': ['openclose_thread'],
-    'follow_thread': ['follow_thread'],
-    'follow_commentable': ['follow_commentable'],
-    'follow_user': ['follow_user'],
-    'unfollow_thread': ['unfollow_thread'],
-    'unfollow_commentable': ['unfollow_commentable'],
-    'unfollow_user': ['unfollow_user'],
-    'create_thread': ['create_thread'],
+    'follow_thread': ['edit_content', ['follow_thread', 'is_team_member_if_applicable']],
+    'follow_commentable': ['edit_content', ['follow_commentable', 'is_team_member_if_applicable']],
+    'unfollow_thread': ['edit_content', ['unfollow_thread', 'is_team_member_if_applicable']],
+    'unfollow_commentable': ['edit_content', ['unfollow_commentable', 'is_team_member_if_applicable']],
+    'create_thread': ['edit_content', ['create_thread', 'is_team_member_if_applicable']],
 }
 
 

lms/lib/comment_client/commentable.py

@@ -5,5 +5,15 @@ from lms.lib.comment_client import settings
 
 class Commentable(models.Model):
 
+    accessible_fields = ['id', 'commentable_id']
+
     base_url = "{prefix}/commentables".format(prefix=settings.PREFIX)
     type = 'commentable'
+
+    def retrieve(self, *args, **kwargs):
+        """
+        Override default behavior because commentables don't actually exist in the comment service.
+        """
+        self.attributes["commentable_id"] = self.attributes["id"]
+        self.retrieved = True
+        return self