-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add Group Community TA audience for cohorted content_reported notifications #94
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
base: release-ulmo
Are you sure you want to change the base?
Conversation
…eported notifications
|
I will work on it tomorrow. |
| """ | ||
| if not cohort_ids: | ||
| return [] | ||
|
|
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.
We can use this as subquery for group_moderator_user_ids_qs below.
This will ensure DB is not hit twice -> will lower the latency.
Passing the QuerySet directly into the next filter allows the database to handle the join internally.
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.
I’ve updated this to pass the cohort users queryset directly into the Role filter so the database performs the join internally, avoiding an extra DB hit and improving latency.
| ).values_list("users__id", flat=True) | ||
|
|
||
| # If no users in cohort, short-circuit | ||
| cohort_user_ids = list(cohort_users_qs) |
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.
Not required. what if a large course contain thousands of users. We don't want to evaluate the QS in Python memory just to pass it as IN clause to hit database packet size limits.
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.
Makes sense — thanks for calling this out.
I’ve removed the Python list evaluation and now keep this as a queryset to avoid memory overhead and large IN clauses.
| course_id=self.course_key, | ||
| name=FORUM_ROLE_GROUP_MODERATOR, | ||
| users__id__in=cohort_user_ids | ||
| ).values_list("users__id", flat=True) |
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.
should include .distinct(). If a user is associated with multiple roles or groups in a way that creates duplicate rows.
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.
I’ve added .distinct() to ensure we don’t return duplicate user IDs when users are associated via multiple roles or relationships.
asharma4-sonata
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.
Hi @pavanhalesh, the logic here correctly identifies the Group Moderators for specific cohorts. However, I have a few concerns regarding memory safety and query efficiency at scale, which I have detailed in the inline comments.
eaa880e to
3d90a94
Compare
|
Thanks for the review and detailed guidance, Amal. |
Description
Adds a new notification audience filter to include Group Community TA / Group Moderator users for cohorted content_reported notifications.
Updates the discussion notification payload to include the cohort id so notifications can target the correct Group TAs.
How it works
When content is reported from a cohorted discussion thread, we pass cohort_for_group_tas=[<cohort_id>].
The new GroupTAinCohortFilter returns only users who:
belong to that cohort, and
have the Group Moderator role for that course.
Testing
Verified backend logic via Django shell by creating Cohort A/B, creating users, assigning Group Moderator role, and validating:
Cohort A → group_ta_a
Cohort B → group_ta_b
Removing TA from cohort → returns empty list for that cohort
End-to-end UI validation is currently blocked locally due to the Discussion/MongoDB connectivity issue in devstack.
Notes
No DB migrations.
@asharma4-sonata Updated branch/PR/commit naming to match conventions (no internal IDs in titles/messages).
Raised PR from fork targeting edx/edx-platform:release/ulmo using branch feature/group-community-ta-content-reported-audience.
Please review.