Skip to content

E
edx-platform
Commit 13ba2c4a by jsa
Options

Revert "Merge pull request #6771 from edx/dan-f/forums-filter-discussion-modules" 

This reverts commit 086a2ba7, reversing
changes made to a44132ec.  Some minor
code quality issues were addressed as part of this change.

Conflicts:
	lms/djangoapps/django_comment_client/base/views.py
	lms/djangoapps/django_comment_client/utils.py
parent 5802e17c
Showing with 106 additions and 204 deletions
common/test/acceptance/pages/lms/discussion.py
......@@ -311,15 +311,13 @@ class DiscussionSortPreferencePage(CoursePage):
class DiscussionTabSingleThreadPage(CoursePage):
def __init__(self, browser, course_id, discussion_id, thread_id):
def __init__(self, browser, course_id, thread_id):
super(DiscussionTabSingleThreadPage, self).__init__(browser, course_id)
self.thread_page = DiscussionThreadPage(
browser,
"body.discussion .discussion-article[data-id='{thread_id}']".format(thread_id=thread_id)
)
self.url_path = "discussion/forum/{discussion_id}/threads/{thread_id}".format(
discussion_id=discussion_id, thread_id=thread_id
)
self.url_path = "discussion/forum/dummy/threads/" + thread_id
def is_browser_on_page(self):
return self.thread_page.is_browser_on_page()
......
common/test/acceptance/tests/discussion/helpers.py
......@@ -5,15 +5,12 @@ Helper functions and classes for discussion tests.
from uuid import uuid4
import json
from ...fixtures import LMS_BASE_URL
from ...fixtures.course import CourseFixture
from ...fixtures.discussion import (
SingleThreadViewFixture,
Thread,
Response,
)
from ...pages.lms.discussion import DiscussionTabSingleThreadPage
from ...tests.helpers import UniqueCourseTest
from ...fixtures import LMS_BASE_URL
class BaseDiscussionMixin(object):
......@@ -86,22 +83,3 @@ class CohortTestMixin(object):
data = {"users": username}
response = course_fixture.session.post(url, data=data, headers=course_fixture.headers)
self.assertTrue(response.ok, "Failed to add user to cohort")
class BaseDiscussionTestCase(UniqueCourseTest):
def setUp(self):
super(BaseDiscussionTestCase, self).setUp()
self.discussion_id = "test_discussion_{}".format(uuid4().hex)
self.course_fixture = CourseFixture(**self.course_info)
self.course_fixture.add_advanced_settings(
{'discussion_topics': {'value': {'Test Discussion Topic': {'id': self.discussion_id}}}}
)
self.course_fixture.install()
def create_single_thread_page(self, thread_id):
"""
Sets up a `DiscussionTabSingleThreadPage` for a given
`thread_id`.
"""
return DiscussionTabSingleThreadPage(self.browser, self.course_id, self.discussion_id, thread_id)
common/test/acceptance/tests/discussion/test_cohorts.py
......@@ -3,7 +3,7 @@ Tests related to the cohorting feature.
"""
from uuid import uuid4
from .helpers import BaseDiscussionMixin, BaseDiscussionTestCase
from .helpers import BaseDiscussionMixin
from .helpers import CohortTestMixin
from ..helpers import UniqueCourseTest
from ...pages.lms.auto_auth import AutoAuthPage
......@@ -57,17 +57,20 @@ class CohortedDiscussionTestMixin(BaseDiscussionMixin, CohortTestMixin):
self.assertEquals(self.thread_page.get_group_visibility_label(), "This post is visible to everyone.")
class DiscussionTabSingleThreadTest(BaseDiscussionTestCase):
class DiscussionTabSingleThreadTest(UniqueCourseTest):
"""
Tests for the discussion page displaying a single thread.
"""
def setUp(self):
super(DiscussionTabSingleThreadTest, self).setUp()
self.discussion_id = "test_discussion_{}".format(uuid4().hex)
# Create a course to register for
self.course_fixture = CourseFixture(**self.course_info).install()
self.setup_cohorts()
AutoAuthPage(self.browser, course_id=self.course_id).visit()
def setup_thread_page(self, thread_id):
self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, self.discussion_id, thread_id) # pylint: disable=attribute-defined-outside-init
self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, thread_id) # pylint: disable=attribute-defined-outside-init
self.thread_page.visit()
# pylint: disable=unused-argument
......
lms/djangoapps/django_comment_client/base/tests.py
......@@ -14,7 +14,7 @@ from lms.lib.comment_client import Thread
from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE
from django_comment_client.base import views
from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin, GroupIdAssertionMixin
from django_comment_client.tests.utils import CohortedTestCase
from django_comment_client.tests.utils import CohortedContentTestCase
from django_comment_client.tests.unicode import UnicodeTestMixin
from django_comment_common.models import Role
from django_comment_common.utils import seed_permissions_roles
......@@ -42,7 +42,7 @@ class MockRequestSetupMixin(object):
@patch('lms.lib.comment_client.utils.requests.request')
class CreateThreadGroupIdTestCase(
MockRequestSetupMixin,
CohortedTestCase,
CohortedContentTestCase,
CohortedTopicGroupIdTestMixin,
NonCohortedTopicGroupIdTestMixin
):
......@@ -77,7 +77,7 @@ class CreateThreadGroupIdTestCase(
@patch('lms.lib.comment_client.utils.requests.request')
class ThreadActionGroupIdTestCase(
MockRequestSetupMixin,
CohortedTestCase,
CohortedContentTestCase,
GroupIdAssertionMixin
):
def call_view(
......
lms/djangoapps/django_comment_client/base/views.py
......@@ -74,7 +74,7 @@ def track_forum_event(request, event_name, course, obj, data, id_map=None):
user = request.user
data['id'] = obj.id
if id_map is None:
id_map = get_discussion_id_map(course, user)
id_map = get_discussion_id_map(course)
commentable_id = data['commentable_id']
if commentable_id in id_map:
......@@ -173,9 +173,9 @@ def create_thread(request, course_id, commentable_id):
# Calls to id map are expensive, but we need this more than once.
# Prefetch it.
id_map = get_discussion_id_map(course, request.user)
id_map = get_discussion_id_map(course)
add_courseware_context([data], course, request.user, id_map=id_map)
add_courseware_context([data], course, id_map=id_map)
track_forum_event(request, 'edx.forum.thread.created',
course, thread, event_data, id_map=id_map)
......@@ -208,7 +208,7 @@ def update_thread(request, course_id, thread_id):
thread.thread_type = request.POST["thread_type"]
if "commentable_id" in request.POST:
course = get_course_with_access(request.user, 'load', course_key)
commentable_ids = get_discussion_categories_ids(course, request.user)
commentable_ids = get_discussion_categories_ids(course)
if request.POST.get("commentable_id") in commentable_ids:
thread.commentable_id = request.POST["commentable_id"]
else:
......
lms/djangoapps/django_comment_client/forum/views.py
......@@ -52,7 +52,7 @@ def _attr_safe_json(obj):
@newrelic.agent.function_trace()
def make_course_settings(course, user):
def make_course_settings(course):
"""
Generate a JSON-serializable model for course settings, which will be used to initialize a
DiscussionCourseSettings object on the client.
......@@ -63,14 +63,14 @@ def make_course_settings(course, user):
'allow_anonymous': course.allow_anonymous,
'allow_anonymous_to_peers': course.allow_anonymous_to_peers,
'cohorts': [{"id": str(g.id), "name": g.name} for g in get_course_cohorts(course)],
'category_map': utils.get_discussion_category_map(course, user)
'category_map': utils.get_discussion_category_map(course)
}
return obj
@newrelic.agent.function_trace()
def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PAGE):
"""
This may raise an appropriate subclass of cc.utils.CommentClientError
if something goes wrong, or ValueError if the group_id is invalid.
......@@ -81,19 +81,12 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
'sort_key': 'date',
'sort_order': 'desc',
'text': '',
'course_id': unicode(course.id),
'commentable_id': discussion_id,
'course_id': course_key.to_deprecated_string(),
'user_id': request.user.id,
'group_id': get_group_id_for_comments_service(request, course.id, discussion_id), # may raise ValueError
'group_id': get_group_id_for_comments_service(request, course_key, discussion_id), # may raise ValueError
}
if discussion_id is not None:
default_query_params['commentable_id'] = discussion_id
else:
default_query_params['commentable_ids'] = ','.join(
course.top_level_discussion_topic_ids +
utils.get_discussion_id_map(course, request.user).keys()
)
if not request.GET.get('sort_key'):
# If the user did not select a sort key, use their last used sort key
cc_user = cc.User.from_django_user(request.user)
......@@ -170,7 +163,7 @@ def inline_discussion(request, course_key, discussion_id):
user_info = cc_user.to_dict()
try:
threads, query_params = get_threads(request, course, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
threads, query_params = get_threads(request, course_key, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
except ValueError:
return HttpResponseBadRequest("Invalid group_id")
......@@ -179,7 +172,7 @@ def inline_discussion(request, course_key, discussion_id):
is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
threads = [utils.prepare_content(thread, course_key, is_staff) for thread in threads]
with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
add_courseware_context(threads, course, request.user)
add_courseware_context(threads, course)
return utils.JsonResponse({
'is_commentable_cohorted': is_commentable_cohorted(course_key, discussion_id),
'discussion_data': threads,
......@@ -188,7 +181,7 @@ def inline_discussion(request, course_key, discussion_id):
'page': query_params['page'],
'num_pages': query_params['num_pages'],
'roles': utils.get_role_ids(course_key),
'course_settings': make_course_settings(course, request.user)
'course_settings': make_course_settings(course)
})
......@@ -201,13 +194,13 @@ def forum_form_discussion(request, course_key):
nr_transaction = newrelic.agent.current_transaction()
course = get_course_with_access(request.user, 'load_forum', course_key, check_if_enrolled=True)
course_settings = make_course_settings(course, request.user)
course_settings = make_course_settings(course)
user = cc.User.from_django_user(request.user)
user_info = user.to_dict()
try:
unsafethreads, query_params = get_threads(request, course) # This might process a search query
unsafethreads, query_params = get_threads(request, course_key) # This might process a search query
is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
threads = [utils.prepare_content(thread, course_key, is_staff) for thread in unsafethreads]
except cc.utils.CommentClientMaintenanceError:
......@@ -220,7 +213,7 @@ def forum_form_discussion(request, course_key):
annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)
with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
add_courseware_context(threads, course, request.user)
add_courseware_context(threads, course)
if request.is_ajax():
return utils.JsonResponse({
......@@ -265,21 +258,15 @@ def single_thread(request, course_key, discussion_id, thread_id):
"""
Renders a response to display a single discussion thread.
"""
nr_transaction = newrelic.agent.current_transaction()
course = get_course_with_access(request.user, 'load_forum', course_key)
course_settings = make_course_settings(course, request.user)
course_settings = make_course_settings(course)
cc_user = cc.User.from_django_user(request.user)
user_info = cc_user.to_dict()
is_moderator = cached_has_permission(request.user, "see_all_cohorts", course_key)
# Verify that the student has access to this thread if belongs to a discussion module
accessible_discussion_ids = [
module.discussion_id for module in utils.get_accessible_discussion_modules(course, request.user)
]
if discussion_id not in set(course.top_level_discussion_topic_ids + accessible_discussion_ids):
raise Http404
# Currently, the front end always loads responses via AJAX, even for this
# page; it would be a nice optimization to avoid that extra round trip to
# the comments service.
......@@ -307,7 +294,7 @@ def single_thread(request, course_key, discussion_id, thread_id):
annotated_content_info = utils.get_annotated_content_infos(course_key, thread, request.user, user_info=user_info)
content = utils.prepare_content(thread.to_dict(), course_key, is_staff)
with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
add_courseware_context([content], course, request.user)
add_courseware_context([content], course)
return utils.JsonResponse({
'content': content,
'annotated_content_info': annotated_content_info,
......@@ -315,13 +302,13 @@ def single_thread(request, course_key, discussion_id, thread_id):
else:
try:
threads, query_params = get_threads(request, course)
threads, query_params = get_threads(request, course_key)
except ValueError:
return HttpResponseBadRequest("Invalid group_id")
threads.append(thread.to_dict())
with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
add_courseware_context(threads, course, request.user)
add_courseware_context(threads, course)
for thread in threads:
# patch for backward compatibility with comments service
......
lms/djangoapps/django_comment_client/tests/utils.py
"""
Utilities for tests within the django_comment_client module.
"""
from datetime import datetime
from django.test.utils import override_settings
from mock import patch
from pytz import UTC
from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup
from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
from openedx.core.djangoapps.course_groups.models import CourseUserGroup
from django_comment_common.models import Role
from django_comment_common.utils import seed_permissions_roles
from student.tests.factories import CourseEnrollmentFactory, UserFactory
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
from xmodule.modulestore.tests.factories import CourseFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.partitions.partitions import UserPartition, Group
class CohortedTestCase(ModuleStoreTestCase):
class CohortedContentTestCase(ModuleStoreTestCase):
"""
Sets up a course with a student, a moderator and their cohorts.
"""
@patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
def setUp(self):
super(CohortedTestCase, self).setUp()
super(CohortedContentTestCase, self).setUp()
self.course = CourseFactory.create(
discussion_topics={
......@@ -34,13 +26,15 @@ class CohortedTestCase(ModuleStoreTestCase):
"cohorted_discussions": ["cohorted_topic"]
}
)
self.student_cohort = CohortFactory.create(
self.student_cohort = CourseUserGroup.objects.create(
name="student_cohort",
course_id=self.course.id
course_id=self.course.id,
group_type=CourseUserGroup.COHORT
)
self.moderator_cohort = CohortFactory.create(
self.moderator_cohort = CourseUserGroup.objects.create(
name="moderator_cohort",
course_id=self.course.id
course_id=self.course.id,
group_type=CourseUserGroup.COHORT
)
seed_permissions_roles(self.course.id)
......@@ -51,82 +45,3 @@ class CohortedTestCase(ModuleStoreTestCase):
self.moderator.roles.add(Role.objects.get(name="Moderator", course_id=self.course.id))
self.student_cohort.users.add(self.student)
self.moderator_cohort.users.add(self.moderator)
class ContentGroupTestCase(ModuleStoreTestCase):
"""
Sets up discussion modules visible to content groups 'Alpha' and
'Beta', as well as a module visible to all students. Creates a
staff user, users with access to Alpha/Beta (by way of cohorts),
and a non-cohorted user with no special access.
"""
def setUp(self):
super(ContentGroupTestCase, self).setUp()
self.course = CourseFactory.create(
org='org', number='number', run='run',
# This test needs to use a course that has already started --
# discussion topics only show up if the course has already started,
# and the default start date for courses is Jan 1, 2030.
start=datetime(2012, 2, 3, tzinfo=UTC),
user_partitions=[
UserPartition(
0,
'Content Group Configuration',
'',
[Group(1, 'Alpha'), Group(2, 'Beta')],
scheme_id='cohort'
)
],
cohort_config={'cohorted': True},
discussion_topics={}
)
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]:
CourseEnrollmentFactory.create(user=user, course_id=self.course.id)
alpha_cohort = CohortFactory(
course_id=self.course.id,
name='Cohort Alpha',
users=[self.alpha_user]
)
beta_cohort = CohortFactory(
course_id=self.course.id,
name='Cohort Beta',
users=[self.beta_user]
)
CourseUserGroupPartitionGroup.objects.create(
course_user_group=alpha_cohort,
partition_id=self.course.user_partitions[0].id,
group_id=self.course.user_partitions[0].groups[0].id
)
CourseUserGroupPartitionGroup.objects.create(
course_user_group=beta_cohort,
partition_id=self.course.user_partitions[0].id,
group_id=self.course.user_partitions[0].groups[1].id
)
self.alpha_module = ItemFactory.create(
parent_location=self.course.location,
category='discussion',
discussion_id='alpha_group_discussion',
discussion_target='Visible to Alpha',
group_access={self.course.user_partitions[0].id: [self.course.user_partitions[0].groups[0].id]}
)
self.beta_module = ItemFactory.create(
parent_location=self.course.location,
category='discussion',
discussion_id='beta_group_discussion',
discussion_target='Visible to Beta',
group_access={self.course.user_partitions[0].id: [self.course.user_partitions[0].groups[1].id]}
)
self.global_module = ItemFactory.create(
parent_location=self.course.location,
category='discussion',
discussion_id='global_group_discussion',
discussion_target='Visible to Everyone'
)
self.course = self.store.get_item(self.course.location)
lms/djangoapps/django_comment_client/utils.py
......@@ -17,7 +17,7 @@ from xmodule.modulestore.django import modulestore
from django_comment_common.models import Role, FORUM_ROLE_STUDENT
from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
from edxmako import lookup_template
from courseware.access import has_access
from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted, \
is_course_cohorted
from openedx.core.djangoapps.course_groups.models import CourseUserGroup
......@@ -59,11 +59,9 @@ def has_forum_access(uname, course_id, rolename):
return role.users.filter(username=uname).exists()
# pylint: disable=invalid-name
def get_accessible_discussion_modules(course, user):
def _get_discussion_modules(course):
"""
Get all discussion modules within a course which are accessible to
the user.
Return a list of all valid discussion modules in this course.
"""
all_modules = modulestore().get_items(course.id, qualifiers={'category': 'discussion'})
......@@ -74,23 +72,20 @@ def get_accessible_discussion_modules(course, user):
return False
return True
return [
module for module in all_modules
if has_required_keys(module) and has_access(user, 'load', module, course.id)
]
return filter(has_required_keys, all_modules)
def get_discussion_id_map(course, user):
def get_discussion_id_map(course):
"""
Get metadata about discussion modules visible to the user in a course.
Transform the list of this course's discussion modules into a dictionary of metadata keyed by discussion_id.
"""
def get_entry(module):
def get_entry(module): # pylint: disable=missing-docstring
discussion_id = module.discussion_id
title = module.discussion_target
last_category = module.discussion_category.split("/")[-1].strip()
return (discussion_id, {"location": module.location, "title": last_category + " / " + title})
return dict(map(get_entry, get_accessible_discussion_modules(course, user)))
return dict(map(get_entry, _get_discussion_modules(course)))
def _filter_unstarted_categories(category_map):
......@@ -143,14 +138,14 @@ def _sort_map_entries(category_map, sort_alpha):
category_map["children"] = [x[0] for x in sorted(things, key=lambda x: x[1]["sort_key"])]
def get_discussion_category_map(course, user):
def get_discussion_category_map(course):
"""
Get a mapping of categories and subcategories that are visible to
the user within a course.
Transform the list of this course's discussion modules into a recursive dictionary structure. This is used
to render the discussion category map in the discussion tab sidebar.
"""
unexpanded_category_map = defaultdict(list)
modules = get_accessible_discussion_modules(course, user)
modules = _get_discussion_modules(course)
is_course_cohorted = course.is_cohorted
cohorted_discussion_ids = course.cohorted_discussions
......@@ -216,12 +211,12 @@ def get_discussion_category_map(course, user):
return _filter_unstarted_categories(category_map)
def get_discussion_categories_ids(course, user):
def get_discussion_categories_ids(course):
"""
Returns a list of available ids of categories for the course.
"""
ids = []
queue = [get_discussion_category_map(course, user)]
queue = [get_discussion_category_map(course)]
while queue:
category_map = queue.pop()
for child in category_map["children"]:
......@@ -380,12 +375,12 @@ def extend_content(content):
return merge_dict(content, content_info)
def add_courseware_context(content_list, course, user, id_map=None):
def add_courseware_context(content_list, course, id_map=None):
"""
Decorates `content_list` with courseware metadata.
"""
if id_map is None:
id_map = get_discussion_id_map(course, user)
id_map = get_discussion_id_map(course)
for content in content_list:
commentable_id = content['commentable_id']
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment