Skip to content
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

feat: Update forum role membership api to DRF ( 11th ) #35343

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 53 additions & 50 deletions lms/djangoapps/instructor/views/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@
from lms.djangoapps.instructor_task.api_helper import AlreadyRunningError, QueueConnectionError
from lms.djangoapps.instructor_task.data import InstructorTaskTypes
from lms.djangoapps.instructor_task.models import ReportStore
from lms.djangoapps.instructor.views.serializer import RoleNameSerializer, UserSerializer, AccessSerializer
from lms.djangoapps.instructor.views.serializer import (
RoleNameSerializer, UserSerializer, AccessSerializer, UpdateForumRoleMembershipSerializer
)
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted
from openedx.core.djangoapps.course_groups.models import CourseUserGroup
Expand Down Expand Up @@ -2814,17 +2816,8 @@ def send_email(request, course_id):
return JsonResponse(response_payload)


@require_POST
@ensure_csrf_cookie
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
@require_course_permission(permissions.EDIT_FORUM_ROLES)
@require_post_params(
unique_student_identifier="email or username of user to change access",
rolename="the forum role",
action="'allow' or 'revoke'",
)
@common_exceptions_400
def update_forum_role_membership(request, course_id):
@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch')
class UpdateForumRoleMembership(APIView):
"""
Modify user's forum role.

Expand All @@ -2833,52 +2826,62 @@ def update_forum_role_membership(request, course_id):
which is limited to instructors.
No one can revoke an instructors FORUM_ROLE_ADMINISTRATOR status.

Query parameters:
- `email` is the target users email
- `rolename` is one of [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_GROUP_MODERATOR,
FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA]
- `action` is one of ['allow', 'revoke']
"""
course_id = CourseKey.from_string(course_id)
course = get_course_by_id(course_id)
has_instructor_access = has_access(request.user, 'instructor', course)
has_forum_admin = has_forum_access(
request.user, course_id, FORUM_ROLE_ADMINISTRATOR
)
permission_classes = (IsAuthenticated, permissions.InstructorPermission)
permission_name = permissions.EDIT_FORUM_ROLES
serializer_class = UpdateForumRoleMembershipSerializer

unique_student_identifier = request.POST.get('unique_student_identifier')
rolename = request.POST.get('rolename')
action = request.POST.get('action')
@method_decorator(ensure_csrf_cookie)
def post(self, request, course_id):
"""
Handles role modification requests for a forum user.

# default roles require either (staff & forum admin) or (instructor)
if not (has_forum_admin or has_instructor_access):
return HttpResponseBadRequest(
"Operation requires staff & forum admin or instructor access"
Query parameters:
- `email` is the target users email
- `rolename` is one of [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_GROUP_MODERATOR,
FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA]
- `action` is one of ['allow', 'revoke']
"""
course_id = CourseKey.from_string(course_id)
course = get_course_by_id(course_id)
has_instructor_access = has_access(request.user, 'instructor', course)
has_forum_admin = has_forum_access(
request.user, course_id, FORUM_ROLE_ADMINISTRATOR
)
Comment on lines +2847 to 2850
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a shortcoming of the permissions.InstructorPermission class? that we can't check these two and the above EDIT_FORUM_ROLES permissions at the same time for a user? Could we adapt the permission class to check for multiple roles or use a different one so that all the permissions checks happen as DRF permission checks?

Copy link
Contributor Author

@awais786 awais786 Aug 27, 2024

Choose a reason for hiding this comment

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

@feanil in separate API which is related with forums I have re-write the functionality.

Related [PR]
(#35366). I will merge 35366 first and then I will rebase this later.

has_instructor_access means user must be instructor.
has_forum_admin means user be added as Administrator in Django_Comment_Common > Roles.


# EXCEPT FORUM_ROLE_ADMINISTRATOR requires (instructor)
if rolename == FORUM_ROLE_ADMINISTRATOR and not has_instructor_access:
return HttpResponseBadRequest("Operation requires instructor access.")
serializer_data = UpdateForumRoleMembershipSerializer(data=request.data)
if not serializer_data.is_valid():
return HttpResponseBadRequest(reason=serializer_data.errors)

if rolename not in [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_GROUP_MODERATOR,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this line, now serializer will do all the validation.

FORUM_ROLE_COMMUNITY_TA]:
return HttpResponseBadRequest(strip_tags(
f"Unrecognized rolename '{rolename}'."
))
user = serializer_data.validated_data.get('unique_student_identifier')
if not user:
return JsonResponse({'error': 'User does not exist.'}, status=400)

user = get_student_from_identifier(unique_student_identifier)
if action == 'allow' and not is_user_enrolled_in_course(user, course_id):
CourseEnrollment.enroll(user, course_id)
try:
update_forum_role(course_id, user, rolename, action)
except Role.DoesNotExist:
return HttpResponseBadRequest("Role does not exist.")
rolename = serializer_data.data['rolename']
action = serializer_data.data['action']

response_payload = {
'course_id': str(course_id),
'action': action,
}
return JsonResponse(response_payload)
# default roles require either (staff & forum admin) or (instructor)
if not (has_forum_admin or has_instructor_access):
return HttpResponseBadRequest(
"Operation requires staff & forum admin or instructor access"
)

# EXCEPT FORUM_ROLE_ADMINISTRATOR requires (instructor)
if rolename == FORUM_ROLE_ADMINISTRATOR and not has_instructor_access:
return HttpResponseBadRequest("Operation requires instructor access.")

if action == 'allow' and not is_user_enrolled_in_course(user, course_id):
CourseEnrollment.enroll(user, course_id)
try:
update_forum_role(course_id, user, rolename, action)
except Role.DoesNotExist:
return HttpResponseBadRequest("Role does not exist.")

response_payload = {
'course_id': str(course_id),
'action': action,
}
return JsonResponse(response_payload)


@require_POST
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/instructor/views/api_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
path('list_background_email_tasks', api.list_background_email_tasks, name='list_background_email_tasks'),
path('list_email_content', api.ListEmailContent.as_view(), name='list_email_content'),
path('list_forum_members', api.list_forum_members, name='list_forum_members'),
path('update_forum_role_membership', api.update_forum_role_membership, name='update_forum_role_membership'),
path('update_forum_role_membership', api.UpdateForumRoleMembership.as_view(), name='update_forum_role_membership'),
path('send_email', api.send_email, name='send_email'),
path('change_due_date', api.change_due_date, name='change_due_date'),
path('reset_due_date', api.reset_due_date, name='reset_due_date'),
Expand Down
31 changes: 30 additions & 1 deletion lms/djangoapps/instructor/views/serializer.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,16 @@
from django.core.exceptions import ValidationError
from django.utils.translation import gettext as _
from rest_framework import serializers
from .tools import get_student_from_identifier

from lms.djangoapps.instructor.access import ROLES
from openedx.core.djangoapps.django_comment_common.models import (
FORUM_ROLE_ADMINISTRATOR,
FORUM_ROLE_COMMUNITY_TA,
FORUM_ROLE_GROUP_MODERATOR,
FORUM_ROLE_MODERATOR
)

from .tools import get_student_from_identifier


class RoleNameSerializer(serializers.Serializer): # pylint: disable=abstract-method
Expand Down Expand Up @@ -59,3 +66,25 @@ def validate_unique_student_identifier(self, value):
return None

return user


class UpdateForumRoleMembershipSerializer(AccessSerializer):
"""
Serializer for managing user's forum role.

This serializer extends the AccessSerializer to allow for different action
choices specific to this API. It validates and processes the data required
to modify user access within a system.

Attributes:
unique_student_identifier (str): The email or username of the user whose access is being modified.
rolename (str): The role name to assign to the user.
action (str): The specific action to perform on the user's access, with options 'activate' or 'deactivate'.
"""
rolename = serializers.ChoiceField(
choices=[
FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR,
FORUM_ROLE_GROUP_MODERATOR, FORUM_ROLE_COMMUNITY_TA
],
help_text="Rolename assign to given user."
)
Loading