Skip to content

feat!: remove cs_comments_service support for forum's search APIs - #36820

Merged
ormsbee merged 1 commit into
openedx:masterfrom
edly-io:feat/remove_forum_v1_search_api
Jun 26, 2025
Merged

feat!: remove cs_comments_service support for forum's search APIs#36820
ormsbee merged 1 commit into
openedx:masterfrom
edly-io:feat/remove_forum_v1_search_api

Conversation

@Ali-Salman29

Copy link
Copy Markdown
Contributor

Description

This pull request removes legacy code related to the "search" api functionality within the discussion forum v1 app. The rationale behind this change is to clean up deprecated or unused code paths, thereby simplifying the codebase and reducing potential maintenance overhead.

Implications for users:
• There should be no impact on end-users, as the removed code pertains to deprecated functionality that is no longer in active use.
• Developers working on the discussion forum module will benefit from a cleaner codebase, making future enhancements and maintenance more straightforward.

@Ali-Salman29
Ali-Salman29 force-pushed the feat/remove_forum_v1_search_api branch 2 times, most recently from 03619b7 to 5493ca6 Compare June 3, 2025 16:20
@Ali-Salman29

Copy link
Copy Markdown
Contributor Author

@ormsbee Can you please review this?

@Ali-Salman29

Copy link
Copy Markdown
Contributor Author

@ormsbee,
Could you please review this since we have a couple of other PRs that we need to merge after this?

@ormsbee

ormsbee commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

I will have a review done today. Thank you.

@ormsbee ormsbee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm still looking through the test code, but here's my first pass.

Comment thread openedx/core/djangoapps/django_comment_common/comment_client/user.py Outdated
Comment thread openedx/core/djangoapps/django_comment_common/comment_client/thread.py Outdated
Comment thread openedx/core/djangoapps/django_comment_common/comment_client/thread.py Outdated
Comment thread lms/djangoapps/discussion/django_comment_client/tests/group_id.py
Comment thread lms/djangoapps/discussion/django_comment_client/tests/group_id.py
Comment thread lms/djangoapps/discussion/django_comment_client/tests/group_id.py

@ormsbee ormsbee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few minor requests and questions.

Comment thread lms/djangoapps/discussion/tests/test_views_v2.py Outdated
Comment thread lms/djangoapps/discussion/rest_api/tests/utils.py Outdated
Comment on lines +1306 to +1311
actual_has_group = "group_id" in thread_func_params
expected_has_group = (
course_is_cohorted and role_name in (
FORUM_ROLE_STUDENT, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_GROUP_MODERATOR
)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please explain in a bit more detail about how this works?

@Ali-Salman29 Ali-Salman29 Jun 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test verifies thread visibility based on cohort membership and user roles. It tests whether the group_id is passed to the user_thread function or not.

  • If a user is part of a cohort, a group_id (representing the cohort) is passed in the thread query.
  • Only threads associated with that group_id are returned.
  • If a user is not part of the cohort but has elevated permissions (e.g., Community TA or Group Moderator), the same group_id is passed.
  • In both cases, the API should return only the threads linked to the specified group_id.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ormsbee Are we in the state of merging this?

@Ali-Salman29
Ali-Salman29 force-pushed the feat/remove_forum_v1_search_api branch from 544b0c8 to c5dec63 Compare June 17, 2025 12:34
@Ali-Salman29
Ali-Salman29 requested a review from ormsbee June 17, 2025 13:01
@Ali-Salman29
Ali-Salman29 force-pushed the feat/remove_forum_v1_search_api branch from c5dec63 to 215a235 Compare June 24, 2025 09:09
This will force the use of the new v2 forum's APIs for searching.
@Ali-Salman29
Ali-Salman29 force-pushed the feat/remove_forum_v1_search_api branch from 7e8da63 to 54cd8e5 Compare June 24, 2025 13:48
@Ali-Salman29

Copy link
Copy Markdown
Contributor Author

@ormsbee All issues have been fixed. Can we merge this PR?

@ormsbee
ormsbee merged commit e0fbb96 into openedx:master Jun 26, 2025
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants