-
Notifications
You must be signed in to change notification settings - Fork 56
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
Automatically configuring video settings for the Talk project. #242
base: development
Are you sure you want to change the base?
Changes from 14 commits
ecd2a5c
2579880
07fc00b
af2191e
c5d487a
b57dda0
c87348f
0adbf20
2b18319
d0d81cd
06ee69e
0a7f382
9cfb2f0
a340e38
6dd84ae
9fe1191
463cd05
1d3617f
b2a9bd3
8766a3e
7bcca21
bc5a53a
60df621
68f221f
bea82c0
a845cf4
3eb6d17
a3ef9fc
d98fbde
7947d31
1b6ce53
99717c9
f28ab8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,26 @@ | ||
import logging | ||
|
||
import jwt | ||
from django.conf import settings | ||
from django.http import Http404 | ||
from django_scopes import scopes_disabled | ||
from pretalx_venueless.forms import VenuelessSettingsForm | ||
from rest_framework import viewsets | ||
from rest_framework.authentication import get_authorization_header | ||
from rest_framework.decorators import ( | ||
api_view, | ||
authentication_classes, | ||
permission_classes, | ||
) | ||
from rest_framework.response import Response | ||
|
||
from pretalx.api.serializers.event import EventSerializer | ||
from pretalx.common import exceptions | ||
from pretalx.common.exceptions import AuthenticationFailedError | ||
from pretalx.event.models import Event | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class EventViewSet(viewsets.ReadOnlyModelViewSet): | ||
serializer_class = EventSerializer | ||
|
@@ -24,3 +41,120 @@ def get_object(self): | |
if self.request.user.has_perm(self.permission_required, self.request.event): | ||
return self.request.event | ||
raise Http404() | ||
|
||
|
||
@api_view(http_method_names=["POST"]) | ||
odkhang marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (complexity): Consider consolidating the configuration and saving logic into a single function while keeping token validation separate. The endpoint can be simplified by combining the configuration and saving logic while keeping token validation separate. This reduces error handling complexity while maintaining security. Here's how: @api_view(http_method_names=["POST"])
@authentication_classes([])
@permission_classes([])
def configure_video_settings(request):
video_settings = request.data.get("video_settings")
try:
payload = get_payload_from_token(request, video_settings)
event_slug = payload["event_slug"]
video_tokens = payload["video_tokens"]
with scopes_disabled():
event_instance = Event.objects.get(slug=event_slug)
video_settings_form = VenuelessSettingsForm(
event=event_instance,
data={
"token": video_tokens[0],
"url": f"{settings.EVENTYAY_VIDEO_BASE_PATH}/api/v1/worlds/{event_slug}/"
}
)
if not video_settings_form.is_valid():
logger.error("Failed to configure video settings - Validation errors: %s", video_settings_form.errors)
return Response(
{"status": "error", "message": "Validation errors", "errors": video_settings_form.errors},
status=400
)
video_settings_form.save()
logger.info("Video settings configured successfully for event %s", event_slug)
return Response({"status": "success"}, status=200)
except Event.DoesNotExist:
logger.error("Event with slug %s does not exist", event_slug)
return Response(
{"status": "error", "message": f"Event with slug {event_slug} not found"},
status=404
)
except AuthenticationFailedError as e:
logger.error("Authentication failed: %s", e)
return Response({"status": "error", "message": "Authentication failed"}, status=401)
except (ValueError, jwt.InvalidTokenError) as e:
logger.error("Error configuring video settings: %s", e)
return Response({"status": "error", "message": "Error configuring video settings"}, status=400) This refactor:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As reminded in group chat, use constants from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, I have updated the code please help me to check it. |
||
@authentication_classes([]) | ||
@permission_classes([]) | ||
def configure_video_settings(request): | ||
""" | ||
Configure video settings for an event | ||
@param request: request object | ||
@return response object | ||
""" | ||
video_settings = request.data.get("video_settings") | ||
payload = get_payload_from_token(request, video_settings) | ||
event_slug = payload.get("event_slug") | ||
video_tokens = payload.get("video_tokens") | ||
|
||
if not video_tokens: | ||
raise ValueError("Video tokens not found in the payload") | ||
|
||
try: | ||
with scopes_disabled(): | ||
event_instance = Event.objects.get(slug=event_slug) | ||
save_video_settings_information(event_slug, video_tokens, event_instance) | ||
except Event.DoesNotExist: | ||
logger.error("Event with slug %s does not exist.", event_slug) | ||
return Response( | ||
{ | ||
"status": "error", | ||
"message": "Event with slug {} not found.".format(event_slug), | ||
}, | ||
status=404, | ||
) | ||
except ValueError as e: | ||
logger.error("Error configuring video settings: %s", e) | ||
return Response( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this API endpoint suddenly behaves differently from DjangoRestFramework? If our API is implemented with DjangoRestFramework, we should follow its convention, its style. Don't create inconsistency. The HTTP status other than 2xx already indicates error, not need to add {
'field_1': ['Error 1', 'Error 2'],
'field_2': ['Error 1', 'Error 2'],
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @hongquan, thanks for the comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't show errors on the template, but the errors will be looked at by developers when debugging. Never assume that you can just write once and every thing will work smoothly. When the system doesn't work as you expected, you will have to use Postman or some HTTP client tool to call this API and find out what is wrong. Btw, if you follow DRF style, will the callee (video system) breaks. If it doesn't break, there is no reason to make it different. |
||
{"status": "error", "message": "Error configuring video settings."}, | ||
status=400, | ||
) | ||
except AuthenticationFailedError as e: | ||
logger.error("Authentication failed: %s", e) | ||
return Response( | ||
{"status": "error", "message": "Authentication failed."}, status=401 | ||
) | ||
|
||
|
||
def get_payload_from_token(request, video_settings): | ||
""" | ||
Verify the token and return the payload | ||
@param request: request object | ||
@param video_settings: dict containing video settings | ||
@return: dict containing payload data from the token | ||
""" | ||
try: | ||
auth_header = get_authorization_header(request).split() | ||
if not auth_header: | ||
raise exceptions.AuthenticationFailedError("No authorization header") | ||
|
||
if len(auth_header) != 2 or auth_header[0].lower() != b"bearer": | ||
raise exceptions.AuthenticationFailedError( | ||
"Invalid token format. Must be 'Bearer <token>'" | ||
) | ||
|
||
token_decode = jwt.decode( | ||
odkhang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auth_header[1], video_settings.get("secret"), algorithms=["HS256"] | ||
) | ||
|
||
event_slug = token_decode.get("slug") | ||
video_tokens = token_decode.get("video_tokens") | ||
|
||
if not event_slug or not video_tokens: | ||
raise exceptions.AuthenticationFailedError("Invalid token payload") | ||
|
||
return {"event_slug": event_slug, "video_tokens": video_tokens} | ||
|
||
except jwt.ExpiredSignatureError: | ||
raise exceptions.AuthenticationFailedError("Token has expired") | ||
except jwt.InvalidTokenError: | ||
raise exceptions.AuthenticationFailedError("Invalid token") | ||
|
||
|
||
def save_video_settings_information(event_slug, video_tokens, event_instance): | ||
""" | ||
Save video settings information | ||
@param event_slug: A string representing the event slug | ||
@param video_tokens: A list of video tokens | ||
@param event_instance: An instance of the event | ||
@return: Response object | ||
""" | ||
video_settings_data = { | ||
"token": video_tokens[0], | ||
odkhang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"url": "{}/api/v1/worlds/{}/".format( | ||
settings.EVENTYAY_VIDEO_BASE_PATH, event_slug | ||
), | ||
} | ||
|
||
video_settings_form = VenuelessSettingsForm( | ||
event=event_instance, data=video_settings_data | ||
) | ||
|
||
if video_settings_form.is_valid(): | ||
video_settings_form.save() | ||
logger.info("Video settings configured successfully for event %s.", event_slug) | ||
return Response({"status": "success"}, status=200) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use enum from |
||
else: | ||
logger.error( | ||
"Failed to configure video settings for event %s - Validation errors: %s.", | ||
event_slug, | ||
video_settings_form.errors, | ||
) | ||
return Response( | ||
{ | ||
"status": "error", | ||
"message": "Validation errors", | ||
"errors": video_settings_form.errors, | ||
}, | ||
status=400, | ||
) |
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.
issue (complexity): Consider restructuring the code to use middleware, services, and simplified endpoint handlers.
The code could be simplified while maintaining all functionality by:
This restructuring: