From 1f349245735910247c3f272147f1a84a66f1b333 Mon Sep 17 00:00:00 2001 From: rabiaiftikhar Date: Thu, 26 Jul 2018 22:26:30 +0500 Subject: [PATCH] EDUCATOR-2120 allow Community TA to see all discussion posts regardless of cohort and enrollment track --- lms/djangoapps/discussion/tests/test_views.py | 135 +++++++++++++++--- lms/djangoapps/discussion/views.py | 5 +- lms/djangoapps/django_comment_client/utils.py | 22 ++- .../tests/test_tasks_helper.py | 7 +- openedx/core/djangoapps/util/testing.py | 16 ++- 5 files changed, 157 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 6375a448f2..2030c75d15 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -177,6 +177,33 @@ def make_mock_thread_data( return thread_data +def make_mock_collection_data( + course, + text, + thread_id, + num_children=None, + group_id=None, + commentable_id=None, + thread_list=None +): + if thread_list: + return [ + make_mock_thread_data(course=course, text=text, num_children=num_children, **thread) + for thread in thread_list + ] + else: + return [ + make_mock_thread_data( + course=course, + text=text, + thread_id=thread_id, + num_children=num_children, + group_id=group_id, + commentable_id=commentable_id, + ) + ] + + def make_mock_perform_request_impl( course, text, @@ -184,21 +211,15 @@ def make_mock_perform_request_impl( group_id=None, commentable_id=None, num_thread_responses=1, + thread_list=None ): def mock_perform_request_impl(*args, **kwargs): url = args[1] if url.endswith("threads") or url.endswith("user_profile"): return { - "collection": [ - make_mock_thread_data( - course=course, - text=text, - thread_id=thread_id, - num_children=None, - group_id=group_id, - commentable_id=commentable_id, - ) - ] + "collection": make_mock_collection_data( + course, text, thread_id, None, group_id, commentable_id, thread_list + ) } elif thread_id and url.endswith(thread_id): return make_mock_thread_data( @@ -235,6 +256,7 @@ def make_mock_request_impl( group_id=None, commentable_id=None, num_thread_responses=1, + thread_list=None, ): impl = make_mock_perform_request_impl( course, @@ -242,7 +264,8 @@ def make_mock_request_impl( thread_id=thread_id, group_id=group_id, commentable_id=commentable_id, - num_thread_responses=num_thread_responses + num_thread_responses=num_thread_responses, + thread_list=thread_list ) def mock_request_impl(*args, **kwargs): @@ -407,18 +430,18 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): # course is outside the context manager that is verifying the number of queries, # and with split mongo, that method ends up querying disabled_xblocks (which is then # cached and hence not queried as part of call_single_thread). - (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 16, 4), - (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 16, 4), + (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 17, 5), + (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 17, 5), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 16, 4), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 16, 4), + (ModuleStoreEnum.Type.split, False, 1, 3, 3, 17, 5), + (ModuleStoreEnum.Type.split, False, 50, 3, 3, 17, 5), # Enabling Enterprise integration should have no effect on the number of mongo queries made. - (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 16, 4), - (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 16, 4), + (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 17, 5), + (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 17, 5), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 16, 4), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 16, 4), + (ModuleStoreEnum.Type.split, True, 1, 3, 3, 17, 5), + (ModuleStoreEnum.Type.split, True, 50, 3, 3, 17, 5), ) @ddt.unpack def test_number_of_mongo_queries( @@ -675,6 +698,80 @@ class SingleThreadGroupIdTestCase(CohortedTestCase, GroupIdAssertionMixin): ) +@patch('requests.request', autospec=True) +class ForumFormDiscussionContentGroupTestCase(ForumsEnableMixin, ContentGroupTestCase): + """ + Tests `forum_form_discussion api` works with different content groups. + Discussion modules are setup in ContentGroupTestCase class i.e + alpha_module => alpha_group_discussion => alpha_cohort => alpha_user/community_ta + beta_module => beta_group_discussion => beta_cohort => beta_user + """ + shard = 4 + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(ForumFormDiscussionContentGroupTestCase, self).setUp() + self.thread_list = [ + {"thread_id": "test_general_thread_id"}, + {"thread_id": "test_global_group_thread_id", "commentable_id": self.global_module.discussion_id}, + {"thread_id": "test_alpha_group_thread_id", "group_id": self.alpha_module.group_access[0][0], "commentable_id": self.alpha_module.discussion_id}, # pylint: disable=line-too-long + {"thread_id": "test_beta_group_thread_id", "group_id": self.beta_module.group_access[0][0], "commentable_id": self.beta_module.discussion_id} # pylint: disable=line-too-long + ] + + def assert_has_access(self, response, expected_discussion_threads): + """ + Verify that a users have access to the threads in their assigned + cohorts and non-cohorted modules. + """ + discussion_data = json.loads(response.content)['discussion_data'] + self.assertEqual(len(discussion_data), expected_discussion_threads) + + def call_view(self, mock_request, user): + mock_request.side_effect = make_mock_request_impl( + course=self.course, + text="dummy content", + thread_list=self.thread_list + ) + self.client.login(username=user.username, password='test') + return self.client.get( + reverse("forum_form_discussion", args=[unicode(self.course.id)]), + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + + def test_community_ta_user(self, mock_request): + """ + Verify that community_ta user has access to all threads regardless + of cohort. + """ + response = self.call_view( + mock_request, + self.community_ta + ) + self.assert_has_access(response, 4) + + def test_alpha_cohort_user(self, mock_request): + """ + Verify that alpha_user has access to alpha_cohort and non-cohorted + threads. + """ + response = self.call_view( + mock_request, + self.alpha_user + ) + self.assert_has_access(response, 3) + + def test_beta_cohort_user(self, mock_request): + """ + Verify that beta_user has access to beta_cohort and non-cohorted + threads. + """ + response = self.call_view( + mock_request, + self.beta_user + ) + self.assert_has_access(response, 3) + + @patch('requests.request', autospec=True) class SingleThreadContentGroupTestCase(ForumsEnableMixin, UrlResetMixin, ContentGroupTestCase): shard = 4 diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index eedbc34156..064db20609 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -149,7 +149,6 @@ def get_threads(request, course, user_info, discussion_id=None, per_page=THREADS ) ) ) - paginated_results = cc.Thread.search(query_params) threads = paginated_results.collection @@ -248,6 +247,7 @@ def forum_form_discussion(request, course_key): Renders the main Discussion page, potentially filtered by a search query """ course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) + request.user.is_community_ta = utils.is_user_community_ta(request.user, course.id) if request.is_ajax(): user = cc.User.from_django_user(request.user) user_info = user.to_dict() @@ -292,7 +292,7 @@ def single_thread(request, course_key, discussion_id, thread_id): Depending on the HTTP headers, we'll adjust our response accordingly. """ course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) - + request.user.is_community_ta = utils.is_user_community_ta(request.user, course.id) if request.is_ajax(): cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() @@ -350,7 +350,6 @@ def _find_thread(request, course, discussion_id, thread_id): ) except cc.utils.CommentClientRequestError: return None - # Verify that the student has access to this thread if belongs to a course discussion module thread_context = getattr(thread, "context", "course") if thread_context == "course" and not utils.discussion_category_id_access(course, request.user, discussion_id): diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 3f61c40f61..4cabdadc7a 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -18,7 +18,13 @@ from courseware.access import has_access from django_comment_client.constants import TYPE_ENTRY, TYPE_SUBCATEGORY from django_comment_client.permissions import check_permissions_by_view, get_team, has_permission from django_comment_client.settings import MAX_COMMENT_DEPTH -from django_comment_common.models import FORUM_ROLE_STUDENT, CourseDiscussionSettings, DiscussionsIdMapping, Role +from django_comment_common.models import ( + FORUM_ROLE_STUDENT, + FORUM_ROLE_COMMUNITY_TA, + CourseDiscussionSettings, + DiscussionsIdMapping, + Role +) from django_comment_common.utils import get_course_discussion_settings from openedx.core.djangoapps.course_groups.cohorts import get_cohort_id, get_cohort_names, is_course_cohorted from openedx.core.djangoapps.request_cache.middleware import request_cached @@ -99,6 +105,13 @@ def has_forum_access(uname, course_id, rolename): return role.users.filter(username=uname).exists() +def is_user_community_ta(user, course_id): + """ + Boolean operation to check whether a user's role is Community TA or not + """ + return has_forum_access(user, course_id, FORUM_ROLE_COMMUNITY_TA) + + def has_required_keys(xblock): """ Returns True iff xblock has the proper attributes for generating metadata @@ -120,6 +133,7 @@ def get_accessible_discussion_xblocks(course, user, include_all=False): # pylin Return a list of all valid discussion xblocks in this course that are accessible to the given user. """ + include_all = getattr(user, 'is_community_ta', False) return get_accessible_discussion_xblocks_by_course_id(course.id, user, include_all=include_all) @@ -189,6 +203,7 @@ def get_cached_discussion_id_map_by_course_id(course_id, discussion_ids, user): Returns a dict mapping discussion_ids to respective discussion xblock metadata if it is cached and visible to the user. If not, returns the result of get_discussion_id_map """ + include_all = getattr(user, 'is_community_ta', False) try: entries = [] for discussion_id in discussion_ids: @@ -196,7 +211,7 @@ def get_cached_discussion_id_map_by_course_id(course_id, discussion_ids, user): if not key: continue xblock = _get_item_from_modulestore(key) - if not (has_required_keys(xblock) and has_access(user, 'load', xblock, course_id)): + if not (has_required_keys(xblock) and (include_all or has_access(user, 'load', xblock, course_id))): continue entries.append(get_discussion_id_map_entry(xblock)) return dict(entries) @@ -429,6 +444,7 @@ def discussion_category_id_access(course, user, discussion_id, xblock=None): Uses the discussion id cache if available, falling back to get_discussion_categories_ids if there is no cache. """ + include_all = getattr(user, 'is_community_ta', False) if discussion_id in course.top_level_discussion_topic_ids: return True try: @@ -437,7 +453,7 @@ def discussion_category_id_access(course, user, discussion_id, xblock=None): if not key: return False xblock = _get_item_from_modulestore(key) - return has_required_keys(xblock) and has_access(user, 'load', xblock, course.id) + return has_required_keys(xblock) and (include_all or has_access(user, 'load', xblock, course.id)) except DiscussionIdMapIsNotCached: return discussion_id in get_discussion_categories_ids(course, user) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index cb43c584c6..ec29a9bd9b 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1191,7 +1191,7 @@ class TestProblemReportCohortedContent(TestReportMixin, ContentGroupTestCase, In with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): result = ProblemGradeReport.generate(None, None, self.course.id, None, 'graded') self.assertDictContainsSubset( - {'action_name': 'graded', 'attempted': 4, 'succeeded': 4, 'failed': 0}, result + {'action_name': 'graded', 'attempted': 5, 'succeeded': 5, 'failed': 0}, result ) problem_names = [u'Homework 1: Subsection - Problem0', u'Homework 1: Subsection - Problem1'] header_row = [u'Student ID', u'Email', u'Username', u'Enrollment Status', u'Grade'] @@ -1219,6 +1219,11 @@ class TestProblemReportCohortedContent(TestReportMixin, ContentGroupTestCase, In 'enrollment_status': ENROLLED_IN_COURSE, 'grade': [u'0.0', u'Not Available', u'Not Available', u'Not Available', u'Not Available'], }, + { + 'user': self.community_ta, + 'enrollment_status': ENROLLED_IN_COURSE, + 'grade': [u'0.0', u'Not Attempted', u'2.0', u'Not Available', u'Not Available'], + }, ] # Verify generated grades and expected grades match diff --git a/openedx/core/djangoapps/util/testing.py b/openedx/core/djangoapps/util/testing.py index 380c8cdfcf..1558640f3d 100644 --- a/openedx/core/djangoapps/util/testing.py +++ b/openedx/core/djangoapps/util/testing.py @@ -3,6 +3,8 @@ from datetime import datetime from pytz import UTC +from django_comment_common.models import Role +from django_comment_common.utils import seed_permissions_roles from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from openedx.core.djangoapps.user_api.tests.factories import UserCourseTagFactory @@ -50,17 +52,27 @@ class ContentGroupTestCase(ModuleStoreTestCase): discussion_topics={} ) + seed_permissions_roles(self.course.id) + self.staff_user = UserFactory.create(is_staff=True) self.alpha_user = UserFactory.create() self.beta_user = UserFactory.create() self.non_cohorted_user = UserFactory.create() - for user in [self.staff_user, self.alpha_user, self.beta_user, self.non_cohorted_user]: + self.community_ta = UserFactory.create(username="community_ta") + self.community_ta.roles.add(Role.objects.get(name="Community TA", course_id=self.course.id)) + for user in [ + self.staff_user, + self.alpha_user, + self.beta_user, + self.non_cohorted_user, + self.community_ta + ]: CourseEnrollmentFactory.create(user=user, course_id=self.course.id) alpha_cohort = CohortFactory( course_id=self.course.id, name='Cohort Alpha', - users=[self.alpha_user] + users=[self.alpha_user, self.community_ta] ) beta_cohort = CohortFactory( course_id=self.course.id,