From c0960cd1c4825b84ac10a56588e0d25fbe211cab Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 19 Oct 2022 12:15:07 +0000 Subject: [PATCH 01/16] docs: Document the discussions app (#31045) This change documents some of the workings of the discussions app for reference. --- .../core/djangoapps/discussions/README.rst | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 openedx/core/djangoapps/discussions/README.rst diff --git a/openedx/core/djangoapps/discussions/README.rst b/openedx/core/djangoapps/discussions/README.rst new file mode 100644 index 000000000000..00bb8a2e78de --- /dev/null +++ b/openedx/core/djangoapps/discussions/README.rst @@ -0,0 +1,66 @@ +Discussions +=========== + +This Discussions app is responsible for providing support for configuring +discussion tools in the Open edX platform. This includes the in-built forum +tool that uses the `cs_comments_service`, but also other LTI-based tools. + + +Technical Overview +------------------ + +This app adds support for alternative Discussion providers, where a provider is +initially an LTI app that can be embedded in the discussion tab. + +A list of supported providers is included configured in this app, with the +features each app supports, its support level, and other details that are +returned by the API and drive the UI for selecting and comparing providers in +the course authoring MFE. + +Each context (course) has an associated `DiscussionConfiguration` object that +specifies the provider to use for that course, and includes other configuration +information for the course. For LTI-based providers, it's also linked to an +`LtiConfiguration` entry. + +The `plugin_configuration` field on this model is to store +provider/plugin-specific configuration that won't make sense for other plugins. + +Different parts of the course can be linked to different discussion topics. The +standard use case here to have a topic for each Unit in the course. The +`DiscussionTopicLink` model handles this association, and links a particular +usage key in a particular course to a topic. + +The new Discussion API is driven entirely by these `DiscussionTopicLinks` in +the database, so it's possible to have a third-party plugin that changes how +and where these topic links are created. + +When a course is published, the `listen_for_course_publish` signal handler is +called, and this signal in turn calls the +update_discussions_settings_from_course_task` in the background. + +That task goes through the module store and builds a +`CourseDiscussionConfigurationData` object that has all the relevant course +discussion configuration information such as the course key, the provider type, +whether in-context discussions are enabled, whether graded units are enabled, +when unit level visibility is enabled. Other plugin configuration and a list +of discussion contexts for which discussions are enabled. Each discussion +context has a usage key, a title (the units name) an external id +(the cs_comments_service id), it's ordering in the course, and additional +context. It then sends its own signal that has the discussion configuration +object attached. + +Finally, the handler for this discussion change signal, takes the information +from the discussion change signal and compares it to the topics in the +database, and does the following; + +- If it sees discussions contexts (units) without topics it creates new topics + link entries for the new units. This will happen if you create a new unit, + or enable discussions for a unit that previously had them disabled. + +- If it sees discussion topics without contexts it disables/archives them. + This could happen when a unit is deleted or if a unit that previously had + discussions enabled, now has discussions disabled. + +- If it sees any other change, i.e. unit name change etc. it applies that + change to the database as well. + From 0ceba001acf504347232477e2b409a34af2b1e40 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 1 Nov 2022 09:31:27 +0000 Subject: [PATCH 02/16] fix: Change approach to processing course topics (#31200) Course topics are now created by traversing the entire course structure from top to bottom instead of starting at the sequential level and then moving up or down as needed. This also introduces a lot of debug logs to pontetially find the reason why under some circumstances new units don't get processed and end up without a discussions topic. --- .../core/djangoapps/discussions/README.rst | 2 +- .../core/djangoapps/discussions/handlers.py | 7 ++- openedx/core/djangoapps/discussions/tasks.py | 63 +++++++++++-------- 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/openedx/core/djangoapps/discussions/README.rst b/openedx/core/djangoapps/discussions/README.rst index 00bb8a2e78de..5c1127d324cc 100644 --- a/openedx/core/djangoapps/discussions/README.rst +++ b/openedx/core/djangoapps/discussions/README.rst @@ -51,7 +51,7 @@ object attached. Finally, the handler for this discussion change signal, takes the information from the discussion change signal and compares it to the topics in the -database, and does the following; +database, and does the following: - If it sees discussions contexts (units) without topics it creates new topics link entries for the new units. This will happen if you create a new unit, diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index 8787da82cee2..c894a6a53b8c 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -61,14 +61,16 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration lookup_key = topic_link.usage_key or topic_link.external_id topic_context = new_topic_map.pop(lookup_key, None) if topic_context is None: + log.info(f"[DEBUG INF-291] Unit was deleted or discussion disabled: {lookup_key}") topic_link.enabled_in_context = False try: # If the section/subsection/unit a topic is in is deleted, add that context to title. topic_link.title = "{section}|{subsection}|{unit}".format(**topic_link.context) except KeyError: - # It's possible the context is if the topic link was created before the context field was added. + # It's possible the context is empty if the link was created before the context field was added. pass else: + log.info(f"[DEBUG INF-291] Unit topic already exists, will be updated: {lookup_key}") topic_link.enabled_in_context = True topic_link.ordering = topic_context.ordering topic_link.title = topic_context.title @@ -78,6 +80,9 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration topic_link.save() log.info(f"Creating new discussion topic links for {course_key}") + log.info(f"[DEBUG INF-291] Discovered new units with keys: {[str(key) for key in new_topic_map.keys()]}") + log.info(f"[DEBUG INF-291] New unit names: {[topic.title for topic in new_topic_map.values()]}") + DiscussionTopicLink.objects.bulk_create([ DiscussionTopicLink( context_key=course_key, diff --git a/openedx/core/djangoapps/discussions/tasks.py b/openedx/core/djangoapps/discussions/tasks.py index 9cbfac57d599..0113c63d831a 100644 --- a/openedx/core/djangoapps/discussions/tasks.py +++ b/openedx/core/djangoapps/discussions/tasks.py @@ -51,37 +51,48 @@ def update_discussions_settings_from_course(course_key: CourseKey) -> CourseDisc provider_type = discussions_config.provider_type def iter_discussable_units(): - subsections = store.get_items(course_key, qualifiers={"category": "sequential"}) # Start at 99 so that the initial increment starts it at 100. # This leaves the first 100 slots for the course wide topics, which is only a concern if there are more # than that many. idx = 99 - for subsection in subsections: - section = store.get_item(subsection.parent) - for unit in subsection.get_children(): - # Increment index even for skipped units so that the index is more stable and won't change - # if settings change, only if a unit is added or removed. - idx += 1 - # If unit-level visibility is enabled and the unit doesn't have discussion enabled, skip it. - if unit_level_visibility and not getattr(unit, "discussion_enabled", False): + log.info(f"[DEBUG INF-291] Unit-level visibility enabled: {unit_level_visibility}") + for section in course.get_children(): + if section.location.block_type != "chapter": + continue + for subsection in section.get_children(): + if subsection.location.block_type != "sequential": continue - # If the unit is in a graded section and graded sections aren't enabled skip it. - if subsection.graded and not enable_graded_units: - continue - # If the unit is an exam, skip it. - if subsection.is_practice_exam or subsection.is_proctored_enabled or subsection.is_time_limited: - continue - yield DiscussionTopicContext( - usage_key=unit.location, - title=unit.display_name, - group_id=None, - ordering=idx, - context={ - "section": section.display_name, - "subsection": subsection.display_name, - "unit": unit.display_name, - }, - ) + for unit in subsection.get_children(): + if unit.location.block_type != 'vertical': + continue + log.info(f"[DEBUG INF-291] Processing unit: {unit.location}") + # Increment index even for skipped units so that the index is more stable and won't change + # if settings change, only if a unit is added or removed. + idx += 1 + # If unit-level visibility is enabled and the unit doesn't have discussion enabled, skip it. + if unit_level_visibility and not getattr(unit, "discussion_enabled", False): + log.info(f"[DEBUG INF-291] Skipping unit because discussion is disbled: {unit.location}") + continue + # If the unit is in a graded section and graded sections aren't enabled skip it. + if subsection.graded and not enable_graded_units: + log.info(f"[DEBUG INF-291] Skipping unit because it's in a graded subsection: {unit.location}") + continue + # If the unit is an exam, skip it. + if subsection.is_practice_exam or subsection.is_proctored_enabled or subsection.is_time_limited: + log.info(f"[DEBUG INF-291] Skipping unit because it's in an exam: {unit.location}") + continue + log.info(f"[DEBUG INF-291] Topic will be created for unit: {unit.location}") + yield DiscussionTopicContext( + usage_key=unit.location, + title=unit.display_name, + group_id=None, + ordering=idx, + context={ + "section": section.display_name, + "subsection": subsection.display_name, + "unit": unit.display_name, + }, + ) with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): course = store.get_course(course_key) From a4fa75f1079124ce14000e6a4c4064d10802bb1d Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Fri, 18 Nov 2022 11:49:29 +0000 Subject: [PATCH 03/16] fix: delay creation of course topics after course publish (#31307) When running in a sharded MongoDB setup it's possible that querying the modulestore right after the course publish signal will not return the latest data. This commit adds a delay similar to the one used in other places in the codebase for a similar reason. --- cms/djangoapps/contentstore/signals/handlers.py | 5 ++++- openedx/core/djangoapps/discussions/handlers.py | 5 ----- openedx/core/djangoapps/discussions/tasks.py | 6 ------ 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index 44bfdea32908..76711eaa4ddc 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/cms/djangoapps/contentstore/signals/handlers.py @@ -145,7 +145,10 @@ def listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable= if CoursewareSearchIndexer.indexing_is_enabled() and CourseAboutSearchIndexer.indexing_is_enabled(): update_search_index.delay(course_key_str, datetime.now(UTC).isoformat()) - update_discussions_settings_from_course_task.delay(course_key_str) + update_discussions_settings_from_course_task.apply_async( + args=[course_key_str], + countdown=settings.DISCUSSION_SETTINGS['COURSE_PUBLISH_TASK_DELAY'], + ) # Send to a signal for catalog info changes as well, but only once we know the transaction is committed. transaction.on_commit(lambda: emit_catalog_info_changed_signal(course_key)) diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index c894a6a53b8c..f43aaf4b09eb 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -61,7 +61,6 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration lookup_key = topic_link.usage_key or topic_link.external_id topic_context = new_topic_map.pop(lookup_key, None) if topic_context is None: - log.info(f"[DEBUG INF-291] Unit was deleted or discussion disabled: {lookup_key}") topic_link.enabled_in_context = False try: # If the section/subsection/unit a topic is in is deleted, add that context to title. @@ -70,7 +69,6 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration # It's possible the context is empty if the link was created before the context field was added. pass else: - log.info(f"[DEBUG INF-291] Unit topic already exists, will be updated: {lookup_key}") topic_link.enabled_in_context = True topic_link.ordering = topic_context.ordering topic_link.title = topic_context.title @@ -80,9 +78,6 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration topic_link.save() log.info(f"Creating new discussion topic links for {course_key}") - log.info(f"[DEBUG INF-291] Discovered new units with keys: {[str(key) for key in new_topic_map.keys()]}") - log.info(f"[DEBUG INF-291] New unit names: {[topic.title for topic in new_topic_map.values()]}") - DiscussionTopicLink.objects.bulk_create([ DiscussionTopicLink( context_key=course_key, diff --git a/openedx/core/djangoapps/discussions/tasks.py b/openedx/core/djangoapps/discussions/tasks.py index 0113c63d831a..1985da1c9aab 100644 --- a/openedx/core/djangoapps/discussions/tasks.py +++ b/openedx/core/djangoapps/discussions/tasks.py @@ -55,7 +55,6 @@ def iter_discussable_units(): # This leaves the first 100 slots for the course wide topics, which is only a concern if there are more # than that many. idx = 99 - log.info(f"[DEBUG INF-291] Unit-level visibility enabled: {unit_level_visibility}") for section in course.get_children(): if section.location.block_type != "chapter": continue @@ -65,23 +64,18 @@ def iter_discussable_units(): for unit in subsection.get_children(): if unit.location.block_type != 'vertical': continue - log.info(f"[DEBUG INF-291] Processing unit: {unit.location}") # Increment index even for skipped units so that the index is more stable and won't change # if settings change, only if a unit is added or removed. idx += 1 # If unit-level visibility is enabled and the unit doesn't have discussion enabled, skip it. if unit_level_visibility and not getattr(unit, "discussion_enabled", False): - log.info(f"[DEBUG INF-291] Skipping unit because discussion is disbled: {unit.location}") continue # If the unit is in a graded section and graded sections aren't enabled skip it. if subsection.graded and not enable_graded_units: - log.info(f"[DEBUG INF-291] Skipping unit because it's in a graded subsection: {unit.location}") continue # If the unit is an exam, skip it. if subsection.is_practice_exam or subsection.is_proctored_enabled or subsection.is_time_limited: - log.info(f"[DEBUG INF-291] Skipping unit because it's in an exam: {unit.location}") continue - log.info(f"[DEBUG INF-291] Topic will be created for unit: {unit.location}") yield DiscussionTopicContext( usage_key=unit.location, title=unit.display_name, From 85ad6ed7448615e99904ee33ec07470cb33e613b Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Wed, 12 Oct 2022 12:00:32 +0500 Subject: [PATCH 04/16] fix: removed switch experience bar from legacy (#31138) Co-authored-by: adeel.tajamul --- .../discussion/discussion_board_fragment.html | 1 - .../_switch_experience_fragment.html | 74 ------------------- 2 files changed, 75 deletions(-) delete mode 100644 lms/templates/discussion/_switch_experience_fragment.html diff --git a/lms/djangoapps/discussion/templates/discussion/discussion_board_fragment.html b/lms/djangoapps/discussion/templates/discussion/discussion_board_fragment.html index a99739f07c48..be7bce1733e9 100644 --- a/lms/djangoapps/discussion/templates/discussion/discussion_board_fragment.html +++ b/lms/djangoapps/discussion/templates/discussion/discussion_board_fragment.html @@ -23,7 +23,6 @@ data-sort-preference="${sort_preference}" data-flag-moderator="${json.dumps(flag_moderator)}" data-user-group-id="${user_group_id}"> - <%include file="_switch_experience_fragment.html" />
## Breadcrumb navigation
diff --git a/lms/templates/discussion/_switch_experience_fragment.html b/lms/templates/discussion/_switch_experience_fragment.html deleted file mode 100644 index c718bed6dcad..000000000000 --- a/lms/templates/discussion/_switch_experience_fragment.html +++ /dev/null @@ -1,74 +0,0 @@ -## mako - -<%page expression_filter="h"/> - -<%! -from django.utils.translation import ugettext as _ -%> - -% if show_banner: -
- % if show_mfe: -
- ${_("Welcome to the new discussions experience. Please share your feedback.")} -
- - - - - - - ${_('View legacy experience')} - - -
-
- % else: -
- ${_("You are currently using the old discussions experience that will be deprecated soon. If you are facing any issues with the new experience, please share using the feedback button.")} -
- - ${_("Switch to new experience")} - - %endif - - % if share_feedback_url: - - ${_("Share feedback")} - - % endif -
-% endif - - - From 4455bc0c38891adf8eb25b29307875f3da5fae3d Mon Sep 17 00:00:00 2001 From: Demid Date: Wed, 12 Oct 2022 14:41:06 +0300 Subject: [PATCH 05/16] feat: add "count_flagged" query param to the learner thread endpoint (#31048) --- lms/djangoapps/discussion/rest_api/api.py | 2 +- lms/djangoapps/discussion/rest_api/views.py | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index e73278fb8818..570a47dacf10 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -892,7 +892,7 @@ def get_learner_active_thread_list(request, course_key, query_params): request: The django request objects used for build_absolute_uri course_key: The key of the course query_params: Parameters to fetch data from comments service. It must contain - user_id, course_id, page, per_page, group_id + user_id, course_id, page, per_page, group_id, count_flagged Returns: diff --git a/lms/djangoapps/discussion/rest_api/views.py b/lms/djangoapps/discussion/rest_api/views.py index 3c3e4c545dab..da55651463b2 100644 --- a/lms/djangoapps/discussion/rest_api/views.py +++ b/lms/djangoapps/discussion/rest_api/views.py @@ -571,6 +571,9 @@ class LearnerThreadView(APIView): * page: The (1-indexed) page to retrieve (default is 1) * page_size: The number of items per page (default is 10) + + * count_flagged: If True, return the count of flagged comments for each thread. + (can only be used by moderators or above) """ authentication_classes = ( @@ -590,6 +593,7 @@ def get(self, request, course_id=None): course_key = CourseKey.from_string(course_id) page_num = request.GET.get('page', 1) threads_per_page = request.GET.get('page_size', 10) + count_flagged = request.GET.get('count_flagged', False) discussion_id = None username = request.GET.get('username', None) user = get_object_or_404(User, username=username) @@ -604,7 +608,8 @@ def get(self, request, course_id=None): "per_page": threads_per_page, "course_id": str(course_key), "user_id": user.id, - "group_id": group_id + "group_id": group_id, + "count_flagged": count_flagged, } return get_learner_active_thread_list(request, course_key, query_params) From 7bcf5e3c5c43df0414ea6a4954a7def241732625 Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Thu, 13 Oct 2022 15:09:19 +0500 Subject: [PATCH 06/16] fix: updated topic sorting for discussions MFE (#31124) * fix: updated topic sorting for discussions MFE * feat: updated topic sorting test case --- lms/djangoapps/discussion/rest_api/api.py | 59 ++++++++++++------- .../discussion/rest_api/tests/test_api.py | 24 ++++---- 2 files changed, 50 insertions(+), 33 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 570a47dacf10..4791b57034e3 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -4,8 +4,8 @@ from __future__ import annotations import itertools +import re from collections import defaultdict - from enum import Enum from typing import Dict, Iterable, List, Literal, Optional, Set, Tuple from urllib.parse import urlencode, urlunparse @@ -22,12 +22,8 @@ from opaque_keys.edx.locator import CourseKey from rest_framework import status from rest_framework.exceptions import PermissionDenied -from rest_framework.response import Response from rest_framework.request import Request - -from xmodule.course_module import CourseBlock -from xmodule.modulestore.django import modulestore -from xmodule.tabs import CourseTabList +from rest_framework.response import Response from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.courseware.courses import get_course_with_access @@ -41,48 +37,53 @@ from openedx.core.djangoapps.django_comment_common.comment_client.comment import Comment from openedx.core.djangoapps.django_comment_common.comment_client.course import ( get_course_commentable_counts, - get_course_user_stats, + get_course_user_stats ) from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread -from openedx.core.djangoapps.django_comment_common.comment_client.utils import CommentClientRequestError, \ - CommentClient500Error +from openedx.core.djangoapps.django_comment_common.comment_client.utils import ( + CommentClient500Error, + CommentClientRequestError +) from openedx.core.djangoapps.django_comment_common.models import ( FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_GROUP_MODERATOR, FORUM_ROLE_MODERATOR, CourseDiscussionSettings, - Role, + Role ) from openedx.core.djangoapps.django_comment_common.signals import ( comment_created, comment_deleted, comment_edited, + comment_flagged, comment_voted, thread_created, thread_deleted, thread_edited, - thread_voted, thread_flagged, - comment_flagged, + thread_voted ) from openedx.core.djangoapps.user_api.accounts.api import get_account_settings from openedx.core.lib.exceptions import CourseNotFoundError, DiscussionNotFoundError, PageNotFoundError -from ..config.waffle import ENABLE_LEARNERS_STATS +from xmodule.course_module import CourseBlock +from xmodule.modulestore.django import modulestore +from xmodule.tabs import CourseTabList +from ..config.waffle import ENABLE_LEARNERS_STATS from ..django_comment_client.base.views import ( track_comment_created_event, track_comment_deleted_event, track_thread_created_event, track_thread_deleted_event, track_thread_viewed_event, - track_voted_event, + track_voted_event ) from ..django_comment_client.utils import ( get_group_id_for_user, get_user_role_names, has_discussion_privileges, - is_commentable_divided, + is_commentable_divided ) from ..toggles import ENABLE_DISCUSSION_MODERATION_REASON_CODES from .exceptions import CommentNotFoundError, DiscussionBlackOutException, DiscussionDisabledError, ThreadNotFoundError @@ -92,7 +93,7 @@ can_delete, get_editable_fields, get_initializable_comment_fields, - get_initializable_thread_fields, + get_initializable_thread_fields ) from .serializers import ( CommentSerializer, @@ -101,14 +102,14 @@ ThreadSerializer, TopicOrdering, UserStatsSerializer, - get_context, + get_context ) - from .utils import ( + add_stats_for_users_with_no_discussion_content, discussion_open_for_user, + get_usernames_for_course, get_usernames_from_search_string, - add_stats_for_users_with_no_discussion_content, - set_attribute, get_usernames_for_course, + set_attribute ) User = get_user_model() @@ -383,7 +384,23 @@ def get_courseware_topics( for xblock in discussion_xblocks: xblocks_by_category[xblock.discussion_category].append(xblock) - for category in xblocks_by_category.keys(): + def sort_categories(category_list): + """ + Sorts the given iterable containing alphanumeric correctly. + Required arguments: + category_list -- list of categories. + """ + def convert(text): + if text.isdigit(): + return int(text) + return text + + def alphanum_key(key): + return [convert(c) for c in re.split('([0-9]+)', key)] + + return sorted(category_list, key=alphanum_key) + + for category in sort_categories(xblocks_by_category.keys()): children = [] for xblock in xblocks_by_category[category]: if not topic_ids or xblock.discussion_id in topic_ids: diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index a36ba5580ad9..83c8e9dda9ea 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -408,17 +408,17 @@ def test_many(self): "B": {"id": "non-courseware-2"}, } self.store.update_item(self.course, self.user.id) - self.make_discussion_xblock("courseware-1", "A", "1") - self.make_discussion_xblock("courseware-2", "A", "2") - self.make_discussion_xblock("courseware-3", "B", "1") - self.make_discussion_xblock("courseware-4", "B", "2") - self.make_discussion_xblock("courseware-5", "C", "1") + self.make_discussion_xblock("courseware-1", "Week 1", "1") + self.make_discussion_xblock("courseware-2", "Week 1", "2") + self.make_discussion_xblock("courseware-3", "Week 10", "1") + self.make_discussion_xblock("courseware-4", "Week 10", "2") + self.make_discussion_xblock("courseware-5", "Week 9", "1") actual = self.get_course_topics() expected = { "courseware_topics": [ self.make_expected_tree( None, - "A", + "Week 1", [ self.make_expected_tree("courseware-1", "1"), self.make_expected_tree("courseware-2", "2"), @@ -426,17 +426,17 @@ def test_many(self): ), self.make_expected_tree( None, - "B", + "Week 9", + [self.make_expected_tree("courseware-5", "1")] + ), + self.make_expected_tree( + None, + "Week 10", [ self.make_expected_tree("courseware-3", "1"), self.make_expected_tree("courseware-4", "2"), ] ), - self.make_expected_tree( - None, - "C", - [self.make_expected_tree("courseware-5", "1")] - ), ], "non_courseware_topics": [ self.make_expected_tree("non-courseware-1", "A"), From 49c634091676020e1713a925794aea29157db1ad Mon Sep 17 00:00:00 2001 From: SaadYousaf Date: Fri, 30 Sep 2022 14:51:37 +0500 Subject: [PATCH 07/16] fix: filter out discussion topics that have not started yet for instructor paced courses --- lms/djangoapps/discussion/rest_api/api.py | 9 +++- .../discussion/rest_api/tests/test_api.py | 47 +++++++++++++++---- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 4791b57034e3..89b02eb66165 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -6,9 +6,13 @@ import itertools import re from collections import defaultdict +from datetime import datetime + from enum import Enum from typing import Dict, Iterable, List, Literal, Optional, Set, Tuple from urllib.parse import urlencode, urlunparse +from pytz import UTC + from django.conf import settings from django.contrib.auth import get_user_model @@ -379,10 +383,13 @@ def get_courseware_topics( courseware_topics = [] existing_topic_ids = set() + now = datetime.now(UTC) + discussion_xblocks = get_accessible_discussion_xblocks(course, request.user) xblocks_by_category = defaultdict(list) for xblock in discussion_xblocks: - xblocks_by_category[xblock.discussion_category].append(xblock) + if course.self_paced or (xblock.start and xblock.start < now): + xblocks_by_category[xblock.discussion_category].append(xblock) def sort_categories(category_list): """ diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index 83c8e9dda9ea..b8c8a3d5086e 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -503,7 +503,6 @@ def test_access_control(self): ways in which a user may not have access are: * Module is visible to staff only - * Module has a start date in the future * Module is accessible only to a group the user is not in Also, there is a case that ensures that a category with no accessible @@ -575,12 +574,7 @@ def test_access_control(self): self.make_expected_tree("courseware-3", "Cohort B"), self.make_expected_tree("courseware-1", "Everybody"), ] - ), - self.make_expected_tree( - None, - "Second", - [self.make_expected_tree("courseware-5", "Future Start Date")] - ), + ) ], "non_courseware_topics": [ self.make_expected_tree("non-courseware-topic-id", "Test Topic"), @@ -605,7 +599,6 @@ def test_access_control(self): None, "Second", [ - self.make_expected_tree("courseware-5", "Future Start Date"), self.make_expected_tree("courseware-4", "Staff Only"), ] ), @@ -616,6 +609,44 @@ def test_access_control(self): } assert staff_actual == staff_expected + def test_un_released_discussion_topic(self): + """ + Test discussion topics that have not yet started + """ + staff = StaffFactory.create(course_key=self.course.id) + with self.store.bulk_operations(self.course.id, emit_signals=False): + self.store.update_item(self.course, self.user.id) + self.make_discussion_xblock( + "courseware-2", + "First", + "Released", + start=datetime.now(UTC) - timedelta(days=1) + ) + self.make_discussion_xblock( + "courseware-3", + "First", + "Future release", + start=datetime.now(UTC) + timedelta(days=1) + ) + + self.request.user = staff + staff_actual = self.get_course_topics() + staff_expected = { + "courseware_topics": [ + self.make_expected_tree( + None, + "First", + [ + self.make_expected_tree("courseware-2", "Released"), + ] + ), + ], + "non_courseware_topics": [ + self.make_expected_tree("non-courseware-topic-id", "Test Topic"), + ], + } + assert staff_actual == staff_expected + def test_discussion_topic(self): """ Tests discussion topic details against a requested topic id From 92fd10d634f84fd283b584b471f10dc49d8c1c42 Mon Sep 17 00:00:00 2001 From: SaadYousaf Date: Fri, 28 Oct 2022 18:44:56 +0500 Subject: [PATCH 08/16] refactor: remove todo to add tracking for reported content email notification --- lms/djangoapps/discussion/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/djangoapps/discussion/tasks.py b/lms/djangoapps/discussion/tasks.py index 90ece99da47a..7c2e68f31dc7 100644 --- a/lms/djangoapps/discussion/tasks.py +++ b/lms/djangoapps/discussion/tasks.py @@ -115,7 +115,6 @@ def send_ace_message_for_reported_content(context): # lint-amnesty, pylint: dis ) log.info(f'Sending forum reported content email notification with context {message_context}') ace.send(message) - # TODO: add tracking for reported content email def _track_notification_sent(message, context): From 5a1304e87c5eda9eccc5d4d6754f56cad2a4d603 Mon Sep 17 00:00:00 2001 From: SaadYousaf Date: Thu, 27 Oct 2022 17:00:37 +0500 Subject: [PATCH 09/16] feat: add discussion reported and unreported events --- .../django_comment_client/base/views.py | 86 ++++++++++++++++++ lms/djangoapps/discussion/rest_api/api.py | 15 ++-- .../discussion/rest_api/tests/test_api.py | 87 ++++++++++++++++++- lms/djangoapps/discussion/views.py | 21 +---- 4 files changed, 182 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/discussion/django_comment_client/base/views.py b/lms/djangoapps/discussion/django_comment_client/base/views.py index c842b2271446..6072ebd902a5 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/views.py +++ b/lms/djangoapps/discussion/django_comment_client/base/views.py @@ -263,6 +263,92 @@ def track_comment_deleted_event(request, course, comment): track_forum_event(request, event_name, course, comment, event_data) +def track_thread_reported_event(request, course, thread): + """ + Send analytics event for a reported thread. + """ + event_name = _EVENT_NAME_TEMPLATE.format(obj_type='thread', action_name='reported') + event_data = { + 'body': thread.body[:TRACKING_MAX_FORUM_BODY], + 'content_type': 'Post', + 'commentable_id': thread.get('commentable_id', ''), + } + if hasattr(thread, 'username'): + event_data['target_username'] = thread.get('username', '') + add_truncated_title_to_event_data(event_data, thread.get('title', '')) + track_forum_event(request, event_name, course, thread, event_data) + + +def track_comment_reported_event(request, course, comment): + """ + Send analytics event for a reported response or comment. + """ + obj_type = 'comment' if comment.get('parent_id') else 'response' + event_name = _EVENT_NAME_TEMPLATE.format(obj_type=obj_type, action_name='reported') + event_data = { + 'body': comment.body[:TRACKING_MAX_FORUM_BODY], + 'commentable_id': comment.get('commentable_id', ''), + 'content_type': obj_type.capitalize(), + } + if hasattr(comment, 'username'): + event_data['target_username'] = comment.get('username', '') + track_forum_event(request, event_name, course, comment, event_data) + + +def track_thread_unreported_event(request, course, thread): + """ + Send analytics event for a unreported thread. + """ + event_name = _EVENT_NAME_TEMPLATE.format(obj_type='thread', action_name='unreported') + event_data = { + 'body': thread.body[:TRACKING_MAX_FORUM_BODY], + 'content_type': 'Post', + 'commentable_id': thread.get('commentable_id', ''), + 'reported_status_cleared': not bool(thread.get('abuse_flaggers', [])), + } + if hasattr(thread, 'username'): + event_data['target_username'] = thread.get('username', '') + add_truncated_title_to_event_data(event_data, thread.get('title', '')) + track_forum_event(request, event_name, course, thread, event_data) + + +def track_comment_unreported_event(request, course, comment): + """ + Send analytics event for a unreported response or comment. + """ + obj_type = 'comment' if comment.get('parent_id') else 'response' + event_name = _EVENT_NAME_TEMPLATE.format(obj_type=obj_type, action_name='unreported') + event_data = { + 'body': comment.body[:TRACKING_MAX_FORUM_BODY], + 'commentable_id': comment.get('commentable_id', ''), + 'content_type': obj_type.capitalize(), + 'reported_status_cleared': not bool(comment.get('abuse_flaggers', [])), + } + if hasattr(comment, 'username'): + event_data['target_username'] = comment.get('username', '') + track_forum_event(request, event_name, course, comment, event_data) + + +def track_discussion_reported_event(request, course, cc_content): + """ + Helper method for discussion reported events. + """ + if cc_content.type == 'thread': + track_thread_reported_event(request, course, cc_content) + else: + track_comment_reported_event(request, course, cc_content) + + +def track_discussion_unreported_event(request, course, cc_content): + """ + Helper method for discussion unreported events. + """ + if cc_content.type == 'thread': + track_thread_unreported_event(request, course, cc_content) + else: + track_comment_unreported_event(request, course, cc_content) + + def permitted(func): """ View decorator to verify the user is authorized to access this endpoint. diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 89b02eb66165..768a56340a0f 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -34,7 +34,7 @@ from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from lms.djangoapps.discussion.toggles import ENABLE_DISCUSSIONS_MFE, ENABLE_LEARNERS_TAB_IN_DISCUSSIONS_MFE from lms.djangoapps.discussion.toggles_utils import reported_content_email_notification_enabled -from lms.djangoapps.discussion.views import is_user_moderator +from lms.djangoapps.discussion.views import is_privileged_user from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, DiscussionTopicLink, Provider from openedx.core.djangoapps.discussions.utils import get_accessible_discussion_xblocks from openedx.core.djangoapps.django_comment_common import comment_client @@ -81,7 +81,9 @@ track_thread_created_event, track_thread_deleted_event, track_thread_viewed_event, - track_voted_event + track_voted_event, + track_discussion_reported_event, + track_discussion_unreported_event, ) from ..django_comment_client.utils import ( get_group_id_for_user, @@ -1196,7 +1198,7 @@ def _do_extra_actions(api_content, cc_content, request_fields, actions_form, con if field == "following": _handle_following_field(form_value, context["cc_requester"], cc_content) elif field == "abuse_flagged": - _handle_abuse_flagged_field(form_value, context["cc_requester"], cc_content) + _handle_abuse_flagged_field(form_value, context["cc_requester"], cc_content, request) elif field == "voted": _handle_voted_field(form_value, cc_content, api_content, request, context) elif field == "read": @@ -1215,11 +1217,13 @@ def _handle_following_field(form_value, user, cc_content): user.unfollow(cc_content) -def _handle_abuse_flagged_field(form_value, user, cc_content): +def _handle_abuse_flagged_field(form_value, user, cc_content, request): """mark or unmark thread/comment as abused""" course_key = CourseKey.from_string(cc_content.course_id) + course = get_course_with_access(request.user, 'load', course_key) if form_value: cc_content.flagAbuse(user, cc_content) + track_discussion_reported_event(request, course, cc_content) if ENABLE_DISCUSSIONS_MFE.is_enabled(course_key) and reported_content_email_notification_enabled( course_key): if cc_content.type == 'thread': @@ -1227,8 +1231,9 @@ def _handle_abuse_flagged_field(form_value, user, cc_content): else: comment_flagged.send(sender='flag_abuse_for_comment', user=user, post=cc_content) else: - remove_all = bool(is_user_moderator(course_key, User.objects.get(id=user.id))) + remove_all = bool(is_privileged_user(course_key, User.objects.get(id=user.id))) cc_content.unFlagAbuse(user, cc_content, remove_all) + track_discussion_unreported_event(request, course, cc_content) def _handle_voted_field(form_value, cc_content, api_content, request, context): diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index b8c8a3d5086e..15e6fb8cfb05 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -21,6 +21,7 @@ from opaque_keys.edx.locator import CourseLocator from pytz import UTC from rest_framework.exceptions import PermissionDenied + from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase @@ -2830,7 +2831,8 @@ def test_vote_count_two_users( @ddt.data(*itertools.product([True, False], [True, False])) @ddt.unpack - def test_abuse_flagged(self, old_flagged, new_flagged): + @mock.patch("eventtracking.tracker.emit") + def test_abuse_flagged(self, old_flagged, new_flagged, mock_emit): """ Test attempts to edit the "abuse_flagged" field. @@ -2857,12 +2859,33 @@ def test_abuse_flagged(self, old_flagged, new_flagged): assert httpretty.last_request().method == 'PUT' assert parsed_body(httpretty.last_request()) == {'user_id': [str(self.user.id)]} + expected_event_name = 'edx.forum.thread.reported' if new_flagged else 'edx.forum.thread.unreported' + expected_event_data = { + 'body': 'Original body', + 'id': 'test_thread', + 'content_type': 'Post', + 'commentable_id': 'original_topic', + 'url': '', + 'user_course_roles': [], + 'user_forums_roles': [FORUM_ROLE_STUDENT], + 'target_username': self.user.username, + 'title_truncated': False, + 'title': 'Original Title', + } + if not new_flagged: + expected_event_data['reported_status_cleared'] = False + + actual_event_name, actual_event_data = mock_emit.call_args[0] + self.assertEqual(actual_event_name, expected_event_name) + self.assertEqual(actual_event_data, expected_event_data) + @ddt.data( (False, True), (True, True), ) @ddt.unpack - def test_thread_un_abuse_flag_for_moderator_role(self, is_author, remove_all): + @mock.patch("eventtracking.tracker.emit") + def test_thread_un_abuse_flag_for_moderator_role(self, is_author, remove_all, mock_emit): """ Test un-abuse flag for moderator role. @@ -2883,6 +2906,25 @@ def test_thread_un_abuse_flag_for_moderator_role(self, is_author, remove_all): query_params.update({'all': ['True']}) assert parsed_body(httpretty.last_request()) == query_params + expected_event_name = 'edx.forum.thread.unreported' + expected_event_data = { + 'body': 'Original body', + 'id': 'test_thread', + 'content_type': 'Post', + 'commentable_id': 'original_topic', + 'url': '', + 'user_course_roles': [], + 'user_forums_roles': [FORUM_ROLE_STUDENT, FORUM_ROLE_ADMINISTRATOR], + 'target_username': self.user.username, + 'title_truncated': False, + 'title': 'Original Title', + 'reported_status_cleared': False, + } + + actual_event_name, actual_event_data = mock_emit.call_args[0] + self.assertEqual(actual_event_name, expected_event_name) + self.assertEqual(actual_event_data, expected_event_data) + def test_invalid_field(self): self.register_thread() with pytest.raises(ValidationError) as assertion: @@ -3356,7 +3398,8 @@ def test_vote_count_two_users( @ddt.data(*itertools.product([True, False], [True, False])) @ddt.unpack - def test_abuse_flagged(self, old_flagged, new_flagged): + @mock.patch("eventtracking.tracker.emit") + def test_abuse_flagged(self, old_flagged, new_flagged, mock_emit): """ Test attempts to edit the "abuse_flagged" field. @@ -3383,12 +3426,31 @@ def test_abuse_flagged(self, old_flagged, new_flagged): assert httpretty.last_request().method == 'PUT' assert parsed_body(httpretty.last_request()) == {'user_id': [str(self.user.id)]} + expected_event_name = 'edx.forum.response.reported' if new_flagged else 'edx.forum.response.unreported' + expected_event_data = { + 'body': 'Original body', + 'id': 'test_comment', + 'content_type': 'Response', + 'commentable_id': 'dummy', + 'url': '', + 'user_course_roles': [], + 'user_forums_roles': [FORUM_ROLE_STUDENT], + 'target_username': self.user.username, + } + if not new_flagged: + expected_event_data['reported_status_cleared'] = False + + actual_event_name, actual_event_data = mock_emit.call_args[0] + self.assertEqual(actual_event_name, expected_event_name) + self.assertEqual(actual_event_data, expected_event_data) + @ddt.data( (False, True), (True, True), ) @ddt.unpack - def test_comment_un_abuse_flag_for_moderator_role(self, is_author, remove_all): + @mock.patch("eventtracking.tracker.emit") + def test_comment_un_abuse_flag_for_moderator_role(self, is_author, remove_all, mock_emit): """ Test un-abuse flag for moderator role. @@ -3409,6 +3471,23 @@ def test_comment_un_abuse_flag_for_moderator_role(self, is_author, remove_all): query_params.update({'all': ['True']}) assert parsed_body(httpretty.last_request()) == query_params + expected_event_name = 'edx.forum.response.unreported' + expected_event_data = { + 'body': 'Original body', + 'id': 'test_comment', + 'content_type': 'Response', + 'commentable_id': 'dummy', + 'url': '', + 'user_course_roles': [], + 'user_forums_roles': [FORUM_ROLE_STUDENT, FORUM_ROLE_ADMINISTRATOR], + 'target_username': self.user.username, + 'reported_status_cleared': False, + } + + actual_event_name, actual_event_data = mock_emit.call_args[0] + self.assertEqual(actual_event_name, expected_event_name) + self.assertEqual(actual_event_data, expected_event_data) + @ddt.data( FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index 12212e337c9c..4d14ea50748e 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -761,31 +761,16 @@ def is_privileged_user(course_key: CourseKey, user: User): """ Returns True if user has one of following course role Administrator, Moderator, Group Moderator, Community TA, - Global Staff, Course Instructor or Course Staff. + or Global Staff. """ forum_roles = [ FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_GROUP_MODERATOR, FORUM_ROLE_MODERATOR, - FORUM_ROLE_ADMINISTRATOR + FORUM_ROLE_ADMINISTRATOR, ] has_course_role = Role.user_has_role_for_course(user, course_key, forum_roles) - return GlobalStaff().has_user(user) or is_course_staff(course_key, user) or has_course_role - - -def is_user_moderator(course_key: CourseKey, user: User): - """ - Returns True if user has one of following course role - Administrator, Moderator, Group Moderator, Community TA. - """ - forum_roles = [ - FORUM_ROLE_COMMUNITY_TA, - FORUM_ROLE_GROUP_MODERATOR, - FORUM_ROLE_MODERATOR, - FORUM_ROLE_ADMINISTRATOR - ] - has_course_role = Role.user_has_role_for_course(user, course_key, forum_roles) - return has_course_role + return GlobalStaff().has_user(user) or has_course_role class DiscussionBoardFragmentView(EdxFragmentView): From d7bb1cad4e98f8015d6029412de36a096f46efa1 Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Thu, 3 Nov 2022 16:53:50 +0500 Subject: [PATCH 10/16] feat: added post filters in learners tab api (#31191) Co-authored-by: adeel.tajamul --- lms/djangoapps/discussion/rest_api/api.py | 6 + .../discussion/rest_api/tests/test_views.py | 155 ++++++++++++++++++ lms/djangoapps/discussion/rest_api/views.py | 28 ++++ 3 files changed, 189 insertions(+) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 768a56340a0f..25183c65481b 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -1003,9 +1003,15 @@ def get_learner_active_thread_list(request, course_key, query_params): group_id = query_params.get('group_id', None) user_id = query_params.get('user_id', None) + count_flagged = query_params.get('count_flagged', None) if user_id is None: return Response({'detail': 'Invalid user id'}, status=status.HTTP_400_BAD_REQUEST) + if count_flagged and not context["has_moderation_privilege"]: + raise PermissionDenied("count_flagged can only be set by users with moderation roles.") + if "flagged" in query_params.keys() and not context["has_moderation_privilege"]: + raise PermissionDenied("Flagged filter is only available for moderators") + if group_id is None: comment_client_user = comment_client.User(id=user_id, course_id=course_key) else: diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index b856efeb836c..b69b60df9cd4 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -1452,6 +1452,7 @@ def test_delete_nonexistent_thread(self): assert response.status_code == 404 +@ddt.ddt @httpretty.activate @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class LearnerThreadViewAPITest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @@ -1601,6 +1602,160 @@ def test_not_authenticated(self): """ assert True + @ddt.data("None", "discussion", "question") + def test_thread_type_by(self, thread_type): + """ + Tests the thread_type parameter + + Arguments: + thread_type (str): Value of thread_type can be 'None', + 'discussion' and 'question' + """ + threads = [make_minimal_cs_thread({ + "id": "test_thread", + "course_id": str(self.course.id), + "commentable_id": "test_topic", + "user_id": str(self.user.id), + "username": self.user.username, + "created_at": "2015-04-28T00:00:00Z", + "updated_at": "2015-04-28T11:11:11Z", + "title": "Test Title", + "body": "Test body", + "votes": {"up_count": 4}, + "comments_count": 5, + "unread_comments_count": 3, + })] + expected_cs_comments_response = { + "collection": threads, + "page": 1, + "num_pages": 1, + } + self.register_get_user_response(self.user) + self.register_user_active_threads(self.user.id, expected_cs_comments_response) + response = self.client.get( + self.url, + { + "course_id": str(self.course.id), + "username": self.user.username, + "thread_type": thread_type, + } + ) + assert response.status_code == 200 + self.assert_last_query_params({ + "user_id": [str(self.user.id)], + "course_id": [str(self.course.id)], + "page": ["1"], + "per_page": ["10"], + "thread_type": [thread_type], + "sort_key": ['activity'], + "count_flagged": ["False"] + }) + + @ddt.data( + ("last_activity_at", "activity"), + ("comment_count", "comments"), + ("vote_count", "votes") + ) + @ddt.unpack + def test_order_by(self, http_query, cc_query): + """ + Tests the order_by parameter for active threads + + Arguments: + http_query (str): Query string sent in the http request + cc_query (str): Query string used for the comments client service + """ + threads = [make_minimal_cs_thread({ + "id": "test_thread", + "course_id": str(self.course.id), + "commentable_id": "test_topic", + "user_id": str(self.user.id), + "username": self.user.username, + "created_at": "2015-04-28T00:00:00Z", + "updated_at": "2015-04-28T11:11:11Z", + "title": "Test Title", + "body": "Test body", + "votes": {"up_count": 4}, + "comments_count": 5, + "unread_comments_count": 3, + })] + expected_cs_comments_response = { + "collection": threads, + "page": 1, + "num_pages": 1, + } + self.register_get_user_response(self.user) + self.register_user_active_threads(self.user.id, expected_cs_comments_response) + response = self.client.get( + self.url, + { + "course_id": str(self.course.id), + "username": self.user.username, + "order_by": http_query, + } + ) + assert response.status_code == 200 + self.assert_last_query_params({ + "user_id": [str(self.user.id)], + "course_id": [str(self.course.id)], + "page": ["1"], + "per_page": ["10"], + "sort_key": [cc_query], + "count_flagged": ["False"] + }) + + @ddt.data("flagged", "unanswered", "unread", "unresponded") + def test_status_by(self, post_status): + """ + Tests the post_status parameter + + Arguments: + post_status (str): Value of post_status can be 'flagged', + 'unanswered' and 'unread' + """ + threads = [make_minimal_cs_thread({ + "id": "test_thread", + "course_id": str(self.course.id), + "commentable_id": "test_topic", + "user_id": str(self.user.id), + "username": self.user.username, + "created_at": "2015-04-28T00:00:00Z", + "updated_at": "2015-04-28T11:11:11Z", + "title": "Test Title", + "body": "Test body", + "votes": {"up_count": 4}, + "comments_count": 5, + "unread_comments_count": 3, + })] + expected_cs_comments_response = { + "collection": threads, + "page": 1, + "num_pages": 1, + } + self.register_get_user_response(self.user) + self.register_user_active_threads(self.user.id, expected_cs_comments_response) + response = self.client.get( + self.url, + { + "course_id": str(self.course.id), + "username": self.user.username, + "status": post_status, + } + ) + if post_status == "flagged": + assert response.status_code == 403 + else: + assert response.status_code == 200 + self.assert_last_query_params({ + "user_id": [str(self.user.id)], + "course_id": [str(self.course.id)], + "page": ["1"], + "per_page": ["10"], + post_status: ['True'], + "sort_key": ['activity'], + "count_flagged": ["False"] + }) + @ddt.ddt @httpretty.activate diff --git a/lms/djangoapps/discussion/rest_api/views.py b/lms/djangoapps/discussion/rest_api/views.py index da55651463b2..c04fc7e48471 100644 --- a/lms/djangoapps/discussion/rest_api/views.py +++ b/lms/djangoapps/discussion/rest_api/views.py @@ -574,6 +574,15 @@ class LearnerThreadView(APIView): * count_flagged: If True, return the count of flagged comments for each thread. (can only be used by moderators or above) + + * thread_type: The type of thread to filter None, "discussion" or "question". + + * order_by: Sort order for threads "last_activity_at", "comment_count" or + "vote_count". + + * status: Filter for threads "flagged", "unanswered", "unread". + + * group_id: Filter threads w.r.t cohorts (Cohort ID). """ authentication_classes = ( @@ -594,6 +603,15 @@ def get(self, request, course_id=None): page_num = request.GET.get('page', 1) threads_per_page = request.GET.get('page_size', 10) count_flagged = request.GET.get('count_flagged', False) + thread_type = request.GET.get('thread_type') + order_by = request.GET.get('order_by') + order_by_mapping = { + "last_activity_at": "activity", + "comment_count": "comments", + "vote_count": "votes" + } + order_by = order_by_mapping.get(order_by, 'activity') + post_status = request.GET.get('status', None) discussion_id = None username = request.GET.get('username', None) user = get_object_or_404(User, username=username) @@ -610,7 +628,17 @@ def get(self, request, course_id=None): "user_id": user.id, "group_id": group_id, "count_flagged": count_flagged, + "thread_type": thread_type, + "sort_key": order_by, } + if post_status: + if post_status not in ['flagged', 'unanswered', 'unread', 'unresponded']: + raise ValidationError({ + "status": [ + f"Invalid value. '{post_status}' must be 'flagged', 'unanswered', 'unread' or 'unresponded" + ] + }) + query_params[post_status] = True return get_learner_active_thread_list(request, course_key, query_params) From 6e0e08475744ba7c0423db144342e1e74d040bcb Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Mon, 7 Nov 2022 11:15:01 +0500 Subject: [PATCH 11/16] feat: added sort by recent activity for activity stats api (#31252) Co-authored-by: adeel.tajamul --- lms/djangoapps/discussion/rest_api/api.py | 2 +- lms/djangoapps/discussion/rest_api/forms.py | 1 + lms/djangoapps/discussion/rest_api/tests/test_views.py | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 25183c65481b..50461dd2b255 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -1700,7 +1700,7 @@ def get_course_discussion_user_stats( order_by = order_by or UserOrdering.BY_FLAGS else: order_by = order_by or UserOrdering.BY_ACTIVITY - if order_by != UserOrdering.BY_ACTIVITY: + if order_by == UserOrdering.BY_FLAGS: raise ValidationError({"order_by": "Invalid value"}) if not ENABLE_LEARNERS_STATS.is_enabled(course_key): diff --git a/lms/djangoapps/discussion/rest_api/forms.py b/lms/djangoapps/discussion/rest_api/forms.py index b7a9286c2857..3663b41738bc 100644 --- a/lms/djangoapps/discussion/rest_api/forms.py +++ b/lms/djangoapps/discussion/rest_api/forms.py @@ -24,6 +24,7 @@ class UserOrdering(TextChoices): BY_ACTIVITY = 'activity' BY_FLAGS = 'flagged' + BY_RECENT_ACTIVITY = 'recency' class _PaginationForm(Form): diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index b69b60df9cd4..118b4fcc291f 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -3115,9 +3115,11 @@ def test_moderator_user(self): @ddt.data( ("moderator", "flagged", "flagged"), ("moderator", "activity", "activity"), + ("moderator", "recency", "recency"), ("moderator", None, "flagged"), ("user", None, "activity"), ("user", "activity", "activity"), + ("user", "recency", "recency"), ) @ddt.unpack @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) From 09dbf8d456e7c8e3c30c9fe2686b79c57195c195 Mon Sep 17 00:00:00 2001 From: Mehak Nasir <67791278+mehaknasir@users.noreply.github.com> Date: Fri, 11 Nov 2022 14:56:01 +0500 Subject: [PATCH 12/16] feat: added not responded filter for fetch thread list api (#31275) --- lms/djangoapps/discussion/rest_api/api.py | 2 +- lms/djangoapps/discussion/rest_api/forms.py | 2 +- lms/djangoapps/discussion/rest_api/tests/test_views.py | 2 +- lms/djangoapps/discussion/rest_api/views.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 50461dd2b255..5a104bba43b7 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -873,7 +873,7 @@ def get_thread_list( } if view: - if view in ["unread", "unanswered"]: + if view in ["unread", "unanswered", "unresponded"]: query_params[view] = "true" else: ValidationError({ diff --git a/lms/djangoapps/discussion/rest_api/forms.py b/lms/djangoapps/discussion/rest_api/forms.py index 3663b41738bc..c7aa8c894b13 100644 --- a/lms/djangoapps/discussion/rest_api/forms.py +++ b/lms/djangoapps/discussion/rest_api/forms.py @@ -59,7 +59,7 @@ class ThreadListGetForm(_PaginationForm): count_flagged = ExtendedNullBooleanField(required=False) flagged = ExtendedNullBooleanField(required=False) view = ChoiceField( - choices=[(choice, choice) for choice in ["unread", "unanswered"]], + choices=[(choice, choice) for choice in ["unread", "unanswered", "unresponded"]], required=False, ) order_by = ChoiceField( diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 118b4fcc291f..400c1be4aa11 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -955,7 +955,7 @@ def test_basic(self): "per_page": ["10"], }) - @ddt.data("unread", "unanswered") + @ddt.data("unread", "unanswered", "unresponded") def test_view_query(self, query): threads = [make_minimal_cs_thread()] self.register_get_user_response(self.user) diff --git a/lms/djangoapps/discussion/rest_api/views.py b/lms/djangoapps/discussion/rest_api/views.py index c04fc7e48471..6831765fbf87 100644 --- a/lms/djangoapps/discussion/rest_api/views.py +++ b/lms/djangoapps/discussion/rest_api/views.py @@ -361,7 +361,7 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet): * view: "unread" for threads the requesting user has not read, or "unanswered" for question threads with no marked answer. Only one - can be selected. + can be selected, or unresponded for discussion type posts with no response * requested_fields: (list) Indicates which additional fields to return for each thread. (supports 'profile_image') From c91d194bc161e222961556a33b63d887741be1b0 Mon Sep 17 00:00:00 2001 From: SaadYousaf Date: Mon, 14 Nov 2022 13:49:16 +0500 Subject: [PATCH 13/16] fix: legacy coverage and add missing properties for reported events --- .../django_comment_client/base/tests.py | 7 ++++--- .../django_comment_client/base/views.py | 17 +++++++++++++++-- .../discussion/rest_api/tests/test_api.py | 8 ++++++++ 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index 6254579d6835..717845fade17 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -138,7 +138,8 @@ def call_view( "group_id": self.student_cohort.id, "closed": False, "type": "thread", - "commentable_id": "non_team_dummy_id" + "commentable_id": "non_team_dummy_id", + "body": "test body" } ) request = RequestFactory().post("dummy_url", post_params or {}) @@ -1663,7 +1664,7 @@ def test_comment_actions(self, user, commentable_id, status_code, mock_request): commentable_id = getattr(self, commentable_id) self._setup_mock( user, mock_request, - {"closed": False, "commentable_id": commentable_id, "thread_id": "dummy_thread"}, + {"closed": False, "commentable_id": commentable_id, "thread_id": "dummy_thread", "body": 'dummy body'}, ) for action in ["upvote_comment", "downvote_comment", "un_flag_abuse_for_comment", "flag_abuse_for_comment"]: response = self.client.post( @@ -1684,7 +1685,7 @@ def test_threads_actions(self, user, commentable_id, status_code, mock_request): commentable_id = getattr(self, commentable_id) self._setup_mock( user, mock_request, - {"closed": False, "commentable_id": commentable_id}, + {"closed": False, "commentable_id": commentable_id, "body": "dummy body"}, ) for action in ["upvote_thread", "downvote_thread", "un_flag_abuse_for_thread", "flag_abuse_for_thread", "follow_thread", "unfollow_thread"]: diff --git a/lms/djangoapps/discussion/django_comment_client/base/views.py b/lms/djangoapps/discussion/django_comment_client/base/views.py index 6072ebd902a5..29bf054cd539 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/views.py +++ b/lms/djangoapps/discussion/django_comment_client/base/views.py @@ -270,8 +270,11 @@ def track_thread_reported_event(request, course, thread): event_name = _EVENT_NAME_TEMPLATE.format(obj_type='thread', action_name='reported') event_data = { 'body': thread.body[:TRACKING_MAX_FORUM_BODY], + 'truncated': len(thread.body) > TRACKING_MAX_FORUM_BODY, 'content_type': 'Post', 'commentable_id': thread.get('commentable_id', ''), + 'thread_type': thread.get('thread_type', ''), + 'group_id': thread.get('group_id', ''), } if hasattr(thread, 'username'): event_data['target_username'] = thread.get('username', '') @@ -287,6 +290,7 @@ def track_comment_reported_event(request, course, comment): event_name = _EVENT_NAME_TEMPLATE.format(obj_type=obj_type, action_name='reported') event_data = { 'body': comment.body[:TRACKING_MAX_FORUM_BODY], + 'truncated': len(comment.body) > TRACKING_MAX_FORUM_BODY, 'commentable_id': comment.get('commentable_id', ''), 'content_type': obj_type.capitalize(), } @@ -302,9 +306,13 @@ def track_thread_unreported_event(request, course, thread): event_name = _EVENT_NAME_TEMPLATE.format(obj_type='thread', action_name='unreported') event_data = { 'body': thread.body[:TRACKING_MAX_FORUM_BODY], + 'truncated': len(thread.body) > TRACKING_MAX_FORUM_BODY, 'content_type': 'Post', 'commentable_id': thread.get('commentable_id', ''), 'reported_status_cleared': not bool(thread.get('abuse_flaggers', [])), + 'thread_type': thread.get('thread_type', ''), + 'group_id': thread.get('group_id', ''), + } if hasattr(thread, 'username'): event_data['target_username'] = thread.get('username', '') @@ -320,6 +328,7 @@ def track_comment_unreported_event(request, course, comment): event_name = _EVENT_NAME_TEMPLATE.format(obj_type=obj_type, action_name='unreported') event_data = { 'body': comment.body[:TRACKING_MAX_FORUM_BODY], + 'truncated': len(comment.body) > TRACKING_MAX_FORUM_BODY, 'commentable_id': comment.get('commentable_id', ''), 'content_type': obj_type.capitalize(), 'reported_status_cleared': not bool(comment.get('abuse_flaggers', [])), @@ -767,9 +776,10 @@ def flag_abuse_for_thread(request, course_id, thread_id): """ course_key = CourseKey.from_string(course_id) user = cc.User.from_django_user(request.user) + course = get_course_by_id(course_key) thread = cc.Thread.find(thread_id) thread.flagAbuse(user, thread) - + track_discussion_reported_event(request, course, thread) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -790,7 +800,7 @@ def un_flag_abuse_for_thread(request, course_id, thread_id): has_access(request.user, 'staff', course) ) thread.unFlagAbuse(user, thread, remove_all) - + track_discussion_unreported_event(request, course, thread) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -804,8 +814,10 @@ def flag_abuse_for_comment(request, course_id, comment_id): """ course_key = CourseKey.from_string(course_id) user = cc.User.from_django_user(request.user) + course = get_course_by_id(course_key) comment = cc.Comment.find(comment_id) comment.flagAbuse(user, comment) + track_discussion_unreported_event(request, course, comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -826,6 +838,7 @@ def un_flag_abuse_for_comment(request, course_id, comment_id): ) comment = cc.Comment.find(comment_id) comment.unFlagAbuse(user, comment, remove_all) + track_discussion_unreported_event(request, course, comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index 15e6fb8cfb05..aab2be06c27a 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -2871,6 +2871,9 @@ def test_abuse_flagged(self, old_flagged, new_flagged, mock_emit): 'target_username': self.user.username, 'title_truncated': False, 'title': 'Original Title', + 'thread_type': 'discussion', + 'group_id': None, + 'truncated': False, } if not new_flagged: expected_event_data['reported_status_cleared'] = False @@ -2919,6 +2922,9 @@ def test_thread_un_abuse_flag_for_moderator_role(self, is_author, remove_all, mo 'title_truncated': False, 'title': 'Original Title', 'reported_status_cleared': False, + 'thread_type': 'discussion', + 'group_id': None, + 'truncated': False, } actual_event_name, actual_event_data = mock_emit.call_args[0] @@ -3433,6 +3439,7 @@ def test_abuse_flagged(self, old_flagged, new_flagged, mock_emit): 'content_type': 'Response', 'commentable_id': 'dummy', 'url': '', + 'truncated': False, 'user_course_roles': [], 'user_forums_roles': [FORUM_ROLE_STUDENT], 'target_username': self.user.username, @@ -3477,6 +3484,7 @@ def test_comment_un_abuse_flag_for_moderator_role(self, is_author, remove_all, m 'id': 'test_comment', 'content_type': 'Response', 'commentable_id': 'dummy', + 'truncated': False, 'url': '', 'user_course_roles': [], 'user_forums_roles': [FORUM_ROLE_STUDENT, FORUM_ROLE_ADMINISTRATOR], From c996b15aa7dc0935ff2c16d507fb68d8c23002ad Mon Sep 17 00:00:00 2001 From: SaadYousaf Date: Thu, 17 Nov 2022 15:33:32 +0500 Subject: [PATCH 14/16] fix: add fix for search event for learner search in discussions --- .../django_comment_client/base/views.py | 11 +++++++++ lms/djangoapps/discussion/rest_api/api.py | 23 +++++++++---------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/discussion/django_comment_client/base/views.py b/lms/djangoapps/discussion/django_comment_client/base/views.py index 29bf054cd539..8e5f598270ee 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/views.py +++ b/lms/djangoapps/discussion/django_comment_client/base/views.py @@ -167,6 +167,17 @@ def track_voted_event(request, course, obj, vote_value, undo_vote=False): track_forum_event(request, event_name, course, obj, event_data) +def track_forum_search_event(request, course, search_event_data): + """ + Send analytics event for discussions related search. + """ + event_name = 'edx.forum.searched' + + context = contexts.course_context_from_course_id(course.id) + with tracker.get_tracker().context(event_name, context): + tracker.emit(event_name, search_event_data) + + def track_thread_viewed_event(request, course, thread): """ Send analytics event for a viewed thread. diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 5a104bba43b7..552460dba6e3 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -21,7 +21,6 @@ from django.http import Http404 from django.urls import reverse from edx_django_utils.monitoring import function_trace -from eventtracking import tracker from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseKey from rest_framework import status @@ -84,6 +83,7 @@ track_voted_event, track_discussion_reported_event, track_discussion_unreported_event, + track_forum_search_event ) from ..django_comment_client.utils import ( get_group_id_for_user, @@ -1723,25 +1723,24 @@ def get_course_discussion_user_stats( comma_separated_usernames, matched_users_count, matched_users_pages = get_usernames_from_search_string( course_key, username_search_string, page, page_size ) + search_event_data = { + 'query': username_search_string, + 'search_type': 'Learner', + 'page': params.get('page'), + 'sort_key': params.get('sort_key'), + 'total_results': matched_users_count, + } + course = _get_course(course_key, request.user) + track_forum_search_event(request, course, search_event_data) if not comma_separated_usernames: return DiscussionAPIPagination(request, 0, 1).get_paginated_response({ "results": [], }) + params['usernames'] = comma_separated_usernames course_stats_response = get_course_user_stats(course_key, params) - tracker.emit( - 'edx.forum.searched', - { - 'query': username_search_string, - 'search_type': 'Learner', - 'page': params.get('page'), - 'sort_key': params.get('sort_key'), - 'total_results': course_stats_response.get('total_results'), - } - ) - if comma_separated_usernames: updated_course_stats = add_stats_for_users_with_no_discussion_content( course_stats_response["user_stats"], From 5940348727925c74f3999b0e0b45fa60629d02ce Mon Sep 17 00:00:00 2001 From: SaadYousaf Date: Tue, 15 Nov 2022 15:45:32 +0500 Subject: [PATCH 15/16] feat: add discussion event coverage for legacy experience. --- .../django_comment_client/base/tests.py | 9 ++++++--- .../django_comment_client/base/views.py | 17 ++++++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index 717845fade17..9a5f042b4952 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -139,7 +139,7 @@ def call_view( "closed": False, "type": "thread", "commentable_id": "non_team_dummy_id", - "body": "test body" + "body": "test body", } ) request = RequestFactory().post("dummy_url", post_params or {}) @@ -412,7 +412,7 @@ def test_create_thread(self, mock_request): self.create_thread_helper(mock_request) @ddt.data( - (ModuleStoreEnum.Type.split, 3, 6, 38), + (ModuleStoreEnum.Type.split, 3, 6, 42), ) @ddt.unpack @count_queries @@ -528,6 +528,7 @@ def test_delete_thread(self, mock_request): self._set_mock_request_data(mock_request, { "user_id": str(self.student.id), "closed": False, + "body": "test body", }) test_thread_id = "test_thread_id" request = RequestFactory().post("dummy_url", {"id": test_thread_id}) @@ -546,6 +547,7 @@ def test_delete_comment(self, mock_request): self._set_mock_request_data(mock_request, { "user_id": str(self.student.id), "closed": False, + "body": "test body", }) test_comment_id = "test_comment_id" request = RequestFactory().post("dummy_url", {"id": test_comment_id}) @@ -1595,7 +1597,8 @@ def test_delete_comment(self, user, comment_author, commentable_id, status_code, "commentable_id": commentable_id, "user_id": str(comment_author.id), "username": comment_author.username, - "course_id": str(self.course.id) + "course_id": str(self.course.id), + "body": "test body", }) response = self.client.post( diff --git a/lms/djangoapps/discussion/django_comment_client/base/views.py b/lms/djangoapps/discussion/django_comment_client/base/views.py index 8e5f598270ee..d88c6e063a4f 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/views.py +++ b/lms/djangoapps/discussion/django_comment_client/base/views.py @@ -524,11 +524,11 @@ def update_thread(request, course_id, thread_id): user = request.user # The following checks should avoid issues we've seen during deploys, where end users are hitting an updated server # while their browser still has the old client code. This will avoid erasing present values in those cases. + course = get_course_with_access(user, 'load', course_key) if "thread_type" in request.POST: thread.thread_type = request.POST["thread_type"] if "commentable_id" in request.POST: commentable_id = request.POST["commentable_id"] - course = get_course_with_access(user, 'load', course_key) if thread_context == "course" and not discussion_category_id_access(course, user, commentable_id): return JsonError(_("Topic doesn't exist")) else: @@ -538,6 +538,7 @@ def update_thread(request, course_id, thread_id): thread_edited.send(sender=None, user=user, post=thread) + track_thread_edited_event(request, course, thread, None) if request.is_ajax(): return ajax_content_response(request, course_key, thread.to_dict()) else: @@ -616,9 +617,12 @@ def delete_thread(request, course_id, thread_id): this is ajax only """ course_key = CourseKey.from_string(course_id) + course = get_course_with_access(request.user, 'load', course_key) thread = cc.Thread.find(thread_id) thread.delete() thread_deleted.send(sender=None, user=request.user, post=thread) + + track_thread_deleted_event(request, course, thread) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -631,6 +635,7 @@ def update_comment(request, course_id, comment_id): handles static and ajax submissions """ course_key = CourseKey.from_string(course_id) + course = get_course_with_access(request.user, 'load', course_key) comment = cc.Comment.find(comment_id) if 'body' not in request.POST or not request.POST['body'].strip(): return JsonError(_("Body can't be empty")) @@ -639,6 +644,7 @@ def update_comment(request, course_id, comment_id): comment_edited.send(sender=None, user=request.user, post=comment) + track_comment_edited_event(request, course, comment, None) if request.is_ajax(): return ajax_content_response(request, course_key, comment.to_dict()) else: @@ -672,10 +678,13 @@ def openclose_thread(request, course_id, thread_id): ajax only """ course_key = CourseKey.from_string(course_id) + course = get_course_with_access(request.user, 'load', course_key) thread = cc.Thread.find(thread_id) - thread.closed = request.POST.get('closed', 'false').lower() == 'true' + close_thread = request.POST.get('closed', 'false').lower() == 'true' + thread.closed = close_thread thread.save() + track_thread_lock_unlock_event(request, course, thread, None, close_thread) return JsonResponse({ 'content': prepare_content(thread.to_dict(), course_key), 'ability': get_ability(course_key, thread.to_dict(), request.user), @@ -704,9 +713,11 @@ def delete_comment(request, course_id, comment_id): ajax only """ course_key = CourseKey.from_string(course_id) + course = get_course_with_access(request.user, 'load', course_key) comment = cc.Comment.find(comment_id) comment.delete() comment_deleted.send(sender=None, user=request.user, post=comment) + track_comment_deleted_event(request, course, comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -828,7 +839,7 @@ def flag_abuse_for_comment(request, course_id, comment_id): course = get_course_by_id(course_key) comment = cc.Comment.find(comment_id) comment.flagAbuse(user, comment) - track_discussion_unreported_event(request, course, comment) + track_discussion_reported_event(request, course, comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) From 6a461395f474f1b5244f2b487282cf3a6ea06635 Mon Sep 17 00:00:00 2001 From: Mehak Nasir <67791278+mehaknasir@users.noreply.github.com> Date: Thu, 1 Dec 2022 16:17:19 +0500 Subject: [PATCH 16/16] fix: thread will only be returned for matching course ID (#31340) --- lms/djangoapps/discussion/rest_api/api.py | 6 +++++- lms/djangoapps/discussion/rest_api/tests/test_api.py | 8 ++++++++ lms/djangoapps/discussion/rest_api/views.py | 3 ++- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 552460dba6e3..d9e53ceafb1e 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -1465,7 +1465,7 @@ def update_comment(request, comment_id, update_data): return api_comment -def get_thread(request, thread_id, requested_fields=None): +def get_thread(request, thread_id, requested_fields=None, course_id=None): """ Retrieve a thread. @@ -1476,6 +1476,8 @@ def get_thread(request, thread_id, requested_fields=None): thread_id: The id for the thread to retrieve + course_id: the id of the course the threads belongs to + requested_fields: Indicates which additional fields to return for thread. (i.e. ['profile_image']) """ @@ -1489,6 +1491,8 @@ def get_thread(request, thread_id, requested_fields=None): "user_id": str(request.user.id), } ) + if course_id and course_id != cc_thread.course_id: + raise ThreadNotFoundError("Thread not found.") return _serialize_discussion_entities(request, context, [cc_thread], requested_fields, DiscussionEntity.thread)[0] diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index aab2be06c27a..8204ca0218f0 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -3993,6 +3993,14 @@ def test_group_access(self, role_name, course_is_cohorted, thread_group_state): except ThreadNotFoundError: assert expected_error + def test_course_id_mismatch(self): + """ + Test if the api throws not found exception if course_id from params mismatches course_id in thread + """ + self.register_thread() + get_thread(self.request, self.thread_id, 'different_course_id') + assert ThreadNotFoundError + @mock.patch('lms.djangoapps.discussion.rest_api.api._get_course', mock.Mock()) class CourseTopicsV2Test(ModuleStoreTestCase): diff --git a/lms/djangoapps/discussion/rest_api/views.py b/lms/djangoapps/discussion/rest_api/views.py index 6831765fbf87..daed977e2690 100644 --- a/lms/djangoapps/discussion/rest_api/views.py +++ b/lms/djangoapps/discussion/rest_api/views.py @@ -527,7 +527,8 @@ def retrieve(self, request, thread_id=None): Implements the GET method for thread ID """ requested_fields = request.GET.get('requested_fields') - return Response(get_thread(request, thread_id, requested_fields)) + course_id = request.GET.get('course_id') + return Response(get_thread(request, thread_id, requested_fields, course_id)) def create(self, request): """