diff --git a/common/djangoapps/django_comment_common/utils.py b/common/djangoapps/django_comment_common/utils.py index 2bc8aec..bccc127 100644 --- a/common/djangoapps/django_comment_common/utils.py +++ b/common/djangoapps/django_comment_common/utils.py @@ -1,5 +1,12 @@ 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", ] diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index dbcab2b..87fd8d5 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/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, { diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 0111927..119bd59 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/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) diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 4d076a5..2dc5420 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/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) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 1521b0d..853e30e 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/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', ] ) ) diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index f0b72e2..e2534a2 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/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 = {