Skip to content

fix: resolve cross-team discussion visibility issue #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
naincy128 wants to merge 1 commit into
base: release-ulmo
Choose a base branch
from
Open

fix: resolve cross-team discussion visibility issue #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lms/djangoapps/discussion/rest_api/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
ENABLE_DISCUSSION_BAN,
ONLY_VERIFIED_USERS_CAN_POST,
)
from lms.djangoapps.discussion.views import is_privileged_user
from lms.djangoapps.discussion.views import is_privileged_user, _filter_team_discussions
from openedx.core.djangoapps.discussions.models import (
Comment on lines 41 to 45
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest_api/api.py imports _filter_team_discussions from discussion.views, even though the leading underscore indicates a private helper. To avoid a hard dependency on a private view-layer function (and make reuse intentional), consider moving this logic into a shared utility module (e.g., lms.djangoapps.discussion.utils or rest_api/utils.py) with a public name, and import it from there.

Copilot uses AI. Check for mistakes.
DiscussionsConfiguration,
DiscussionTopicLink,
Expand Down Expand Up @@ -1483,6 +1483,9 @@ def get_learner_active_thread_list(request, course_key, query_params):
if not show_deleted: # Fail safe: include thread for regular users
filtered_threads_with_deletion_status.append(thread)

# Apply team filtering - only include team discussions if user is a team member
filtered_threads = _filter_team_discussions(filtered_threads, course_key, request.user)

results = _serialize_discussion_entities(
request,
context,
Expand Down
35 changes: 29 additions & 6 deletions lms/djangoapps/discussion/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,25 @@ def make_course_settings(course, user, include_category_map=True):
return course_setting


def _filter_team_discussions(threads, course_key, user):
"""
Filter team discussions - only include team discussions if user is a team member.
Privileged users (staff, moderators, etc.) can see all threads.
"""
if is_privileged_user(course_key, user):
return threads

filtered_threads = []
for thread in threads:
thread_discussion_id = thread.get('commentable_id')
if thread_discussion_id:
team = team_api.get_team_by_discussion(thread_discussion_id)
if team and not team.users.filter(id=user.id).exists():
continue # Skip team threads where user is not a member
Comment on lines +112 to +118
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_filter_team_discussions does a DB lookup for every thread (get_team_by_discussion) and then another query to check membership (team.users.filter(...).exists()), which can become an N+1 (or 2N) query pattern on thread lists. Consider bulk-fetching teams for all commentable_ids in the page (e.g., CourseTeam.objects.filter(discussion_topic_id__in=...)) and checking membership via the membership table in one query, or caching results per request.

Suggested change
filtered_threads = []
for thread in threads:
thread_discussion_id = thread.get('commentable_id')
if thread_discussion_id:
team = team_api.get_team_by_discussion(thread_discussion_id)
if team and not team.users.filter(id=user.id).exists():
continue # Skip team threads where user is not a member
# Cache teams per discussion ID and membership per team to avoid N+1/2N queries.
discussion_ids = {
thread.get('commentable_id')
for thread in threads
if thread.get('commentable_id')
}
teams_by_discussion = {}
for discussion_id in discussion_ids:
teams_by_discussion[discussion_id] = team_api.get_team_by_discussion(discussion_id)
user_membership_by_team_id = {}
filtered_threads = []
for thread in threads:
thread_discussion_id = thread.get('commentable_id')
if thread_discussion_id:
team = teams_by_discussion.get(thread_discussion_id)
if team:
team_id = getattr(team, "id", None)
if team_id is not None:
if team_id not in user_membership_by_team_id:
user_membership_by_team_id[team_id] = team.users.filter(id=user.id).exists()
if not user_membership_by_team_id[team_id]:
# Skip team threads where user is not a member
continue

Copilot uses AI. Check for mistakes.
filtered_threads.append(thread)
return filtered_threads


def get_threads(request, course, user_info, discussion_id=None, per_page=THREADS_PER_PAGE):
"""
This may raise an appropriate subclass of cc.utils.CommentClientError
Expand Down Expand Up @@ -189,10 +208,7 @@ def get_threads(request, course, user_info, discussion_id=None, per_page=THREADS
thread['pinned'] = False

# Filter team discussions - only team members can see team posts
if discussion_id is not None and not is_privileged_user(course.id, request.user):
team = team_api.get_team_by_discussion(discussion_id)
if team and not team.users.filter(id=request.user.id).exists():
threads = []
threads = _filter_team_discussions(threads, course.id, request.user)

query_params['page'] = paginated_results.page
query_params['num_pages'] = paginated_results.num_pages
Expand Down Expand Up @@ -608,6 +624,9 @@ def create_user_profile_context(request, course_key, user_id):
user_info = cc.User.from_django_user(request.user).to_dict()
annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)

# Filter team discussions - only include team discussions if user is a team member
threads = _filter_team_discussions(threads, course_key, request.user)

Comment on lines 625 to +629
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes access control behavior for multiple thread-list endpoints (legacy views + REST API). There are existing discussion and REST API test suites in this repo; adding/adjusting tests to cover the new team-level filtering (including ensuring annotated_content_info does not include filtered-out threads) would help prevent regressions.

Suggested change
annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)
# Filter team discussions - only include team discussions if user is a team member
threads = _filter_team_discussions(threads, course_key, request.user)
# Filter team discussions - only include team discussions if user is a team member
threads = _filter_team_discussions(threads, course_key, request.user)
with function_trace("get_metadata_for_threads"):
annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)

Copilot uses AI. Check for mistakes.
is_staff = has_permission(request.user, 'openclose_thread', course.id)
is_community_ta = utils.is_user_community_ta(request.user, course.id)
threads = [utils.prepare_content(thread, course_key, is_staff, is_community_ta) for thread in threads]
Expand Down Expand Up @@ -729,6 +748,10 @@ def followed_threads(request, course_key, user_id):
paginated_results.collection,
request.user, user_info
)

# Filter team discussions - only include team discussions if user is a team member
threads = _filter_team_discussions(paginated_results.collection, course_key, request.user)

if request.headers.get('x-requested-with') == 'XMLHttpRequest':
is_staff = has_permission(request.user, 'openclose_thread', course.id)
is_community_ta = utils.is_user_community_ta(request.user, course.id)
Expand All @@ -737,7 +760,7 @@ def followed_threads(request, course_key, user_id):
'discussion_data': [
utils.prepare_content(
thread, course_key, is_staff, is_community_ta
) for thread in paginated_results.collection
) for thread in threads
],
'page': query_params['page'],
'num_pages': query_params['num_pages'],
Expand All @@ -749,7 +772,7 @@ def followed_threads(request, course_key, user_id):
'user': request.user,
'django_user': User.objects.get(id=user_id),
'profiled_user': profiled_user.to_dict(),
'threads': paginated_results.collection,
'threads': threads,
'user_info': user_info,
'annotated_content_info': annotated_content_info,
# 'content': content,
Expand Down
Loading