-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: added unfollow behaviour in discussion notification #37690
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables users to unsubscribe from notifications for discussion posts and responses they've authored. It introduces a subscription check mechanism that leverages the existing follow/unfollow functionality to prevent sending notifications to users who have unsubscribed from a thread.
Key Changes:
- Added
is_user_subscribed_to_threadmethod to check user subscription status before sending notifications - Updated notification sending logic to respect user subscription preferences for thread creators
- Enhanced test coverage to verify both subscribed and unsubscribed notification scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openedx/core/djangoapps/django_comment_common/comment_client/subscriptions.py | Adds new static method is_user_subscribed_to_thread to check if a user is subscribed to a specific thread |
| lms/djangoapps/discussion/rest_api/discussions_notifications.py | Implements subscription checks in send_new_response_notification and send_new_comment_notification to prevent notifications to unsubscribed thread creators |
| lms/djangoapps/discussion/rest_api/tests/test_tasks_v2.py | Updates existing tests with parameterization to cover both subscribed and unsubscribed scenarios using ddt.data |
Comments suppressed due to low confidence (1)
lms/djangoapps/discussion/rest_api/tests/test_tasks_v2.py:371
- This code will fail with a
NameErrorwhenis_subscribedisFalse. The variableargs_comment_on_responseis only defined inside theif is_subscribed:block (line 329), but these lines execute unconditionally. This entire section (lines 352-371) should be moved inside theif is_subscribed:block, or anelseclause should handle the unsubscribed case appropriately.
# check if the notification is sent to the parent response creator
self.assertEqual([int(user_id) for user_id in args_comment_on_response.user_ids], [self.user_2.id])
self.assertEqual(args_comment_on_response.notification_type, 'new_comment_on_response')
expected_context = {
'replier_name': self.user_3.username,
'post_title': self.thread.title,
'email_content': self.comment.body,
'course_name': self.course.display_name,
'sender_id': self.user_3.id,
'response_id': 2,
'topic_id': None,
'thread_id': 1,
'comment_id': 4,
}
self.assertDictEqual(args_comment_on_response.context, expected_context)
self.assertEqual(
args_comment_on_response.content_url,
_get_mfe_url(self.course.id, self.thread.id)
)
self.assertEqual(args_comment_on_response.app_name, 'discussion')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ) | ||
| ) | ||
| # here |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug comment should be removed before merging. This appears to be a leftover from development.
| # here |
| """ | ||
| Strip starting and ending empty tags from the soup object | ||
| """ | ||
|
|
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary blank line added. This change adds no value and should be removed to keep the diff clean.
| _get_mfe_url(self.course.id, self.thread.id) | ||
| ) | ||
| self.assertEqual(args_comment.app_name, 'discussion') | ||
| # self.assertEqual(handler.call_count, 2) |
Copilot
AI
Nov 27, 2025
•
edited by AhtishamShahid
Loading
edited by AhtishamShahid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code should be removed. If this assertion is no longer valid, remove it completely rather than leaving it commented.
| # self.assertEqual(handler.call_count, 2) | |
| self.assertEqual(handler.call_count, 1) |
cee4ecf to
fd211df
Compare
saraburns1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
verified notification on followed discussion, no notification after unfollowing
122718a to
74438c4
Compare
Description
"At present, users don’t have the option to unsubscribe from notifications for:
This means that users have no choice but to get notifications even if their posts/responses get a very high activity. This can be frustrating for some users, and they may end up opting out.
This PR adds the ability for users to opt out of notifications for posts and responses they’ve authored. It uses the existing mechanism for the follow/unfollow to subscribe and unsubscribe from notifications.
FYI
@saraburns1 @bmtcril