-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: added endpoint for priviledged roles to delete threads of a user #37030
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
86c8a30 to
ccfbd82
Compare
| """ | ||
|
|
||
| authentication_classes = ( | ||
| JwtAuthentication, BearerAuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser, |
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.
Access should only be allowed to verified and active users.
| execute_task = request.GET.get("execute", "false").lower() == "true" | ||
| if (not username) or (not course_id): | ||
| raise BadRequest("username and course_id are required.") | ||
| course_or_org = request.GET.get("course_or_org", "course") |
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.
course and org should be 2 different boolean attributes.
| try: | ||
| user = User.objects.get(username=username) | ||
| except User.DoesNotExist: | ||
| raise BadRequest("Invalid request") # pylint: disable=raise-missing-from |
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.
404 error is more appropriate here because the error is record does not exist.
| query_params = { | ||
| "course_id": {"$in": course_ids}, | ||
| "author_id": str(user.id), | ||
| "_type": "Comment" | ||
| } | ||
| comment_count = Comment()._collection.count_documents(query_params) # pylint: disable=protected-access | ||
|
|
||
| query_params["_type"] = "CommentThread" | ||
| thread_count = CommentThread()._collection.count_documents(query_params) # pylint: disable=protected-access | ||
|
|
||
| query_params["username"] = username | ||
| query_params.pop("_type", None) | ||
|
|
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.
This code should be moved to django_comment_common models or utils.
|
|
||
| for course_id in course_ids: | ||
| if ( | ||
| CourseStaffRole(course_id).has_user(request.user) or |
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.
This check can be optimized by fetching all active user course staff and course instructor roles. fetch only course_ids then comparing them with the enrollment list
| course_id = CourseKey.from_string(course_id) | ||
| org_id = course_id.org | ||
| course_ids = CourseEnrollment.objects.filter(user=user).values_list('course_id', flat=True) | ||
| course_ids = [c_id for c_id in course_ids if c_id.org == org_id] |
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.
Can we filter org directly in the CourseEnrollment query if possible. This approach will query on each course
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.
This will not query database for org. course_id is CourseKeyField. It is basically a string field that gets processed after query and contains attributes
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.
Yes, It may not add query. But filtering may save extra lines of code
CourseEnrollment.objects.filter(user=user, course__org=org_id).values_list('course_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.
This will not work... In database it is just a string, not a table
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.
In my opinion, this should work because CourseEnrollment has a foreign key to CourseOverview, and CourseOverview includes the org field.
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
openedx#37030) * feat: added endpoint for priviledged roles to delete threads of a user * chore: moved forum calls to django comment common app * fix: fixed nits
|
Is there a UI that goes with this API? |
|
openedx#37030) * feat: added endpoint for priviledged roles to delete threads of a user * chore: moved forum calls to django comment common app * fix: fixed nits
Added an API to allow priviledged users to bulk delete posts of a user
Following roles are allowed to perform this action
Purpose of this endpoint is to allow instructors and privileged users to delete all spam posts of a single user in one action.
Ticket: INF-2026