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

Automatically configuring video settings for the Talk project. #242

Open
wants to merge 33 commits into
base: development
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
ecd2a5c
Enable video plugin
lcduong Dec 2, 2024
2579880
Fix black in pipeline
lcduong Dec 2, 2024
07fc00b
Fix black in pipeline
lcduong Dec 2, 2024
af2191e
Add comment
lcduong Dec 2, 2024
c5d487a
Update code
lcduong Dec 3, 2024
b57dda0
Configure video settings for talk
lcduong Dec 6, 2024
c87348f
Merge branch 'development' of github.com:odkhang/eventyay-talk into a…
odkhang Dec 6, 2024
0adbf20
Fix black pipeline
odkhang Dec 6, 2024
2b18319
Update code
odkhang Dec 6, 2024
d0d81cd
Update code
odkhang Dec 6, 2024
06ee69e
Update code
odkhang Dec 6, 2024
0a7f382
Update code
odkhang Dec 6, 2024
9cfb2f0
Fix black in pipeline
odkhang Dec 6, 2024
a340e38
Add comment
odkhang Dec 6, 2024
6dd84ae
Update code
odkhang Dec 9, 2024
9fe1191
Update code
odkhang Dec 9, 2024
463cd05
Update code
odkhang Dec 9, 2024
1d3617f
using constant from http module instead of string
odkhang Dec 16, 2024
b2a9bd3
using constant from http, using httpstatus
odkhang Dec 16, 2024
8766a3e
Merge branch 'development' into auto-fill-talk-link
odkhang Dec 16, 2024
7bcca21
No need to bother with this outside of CI.
odkhang Dec 16, 2024
bc5a53a
fix black code style
odkhang Dec 16, 2024
60df621
rework code
odkhang Dec 17, 2024
68f221f
fix code style, remove unused import
odkhang Dec 17, 2024
bea82c0
fix inconsistence for API response
odkhang Dec 18, 2024
a845cf4
format code
odkhang Dec 18, 2024
3eb6d17
handle case input form invalid
odkhang Dec 18, 2024
a3ef9fc
fix isort, black code style
odkhang Dec 18, 2024
d98fbde
fix isort, black code style
odkhang Dec 18, 2024
7947d31
re format API response for case validation error
odkhang Dec 19, 2024
1b6ce53
fix pipeline
odkhang Dec 19, 2024
99717c9
make error message more clear for validation error
odkhang Dec 19, 2024
f28ab8e
coding style fix
odkhang Dec 19, 2024
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
1 change: 1 addition & 0 deletions src/pretalx/api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,5 @@
"events/<event>/favourite-talk/",
submission.SubmissionFavouriteDeprecatedView.as_view(),
),
path("configure-video-settings/", event.configure_video_settings),
]
139 changes: 139 additions & 0 deletions src/pretalx/api/views/event.py
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
Expand All @@ -24,3 +41,125 @@ 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"])
Copy link

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:

  1. Moving token validation to middleware:
class VideoTokenMiddleware:
    def authenticate(self, request):
        auth_header = get_authorization_header(request).split()
        if not auth_header or auth_header[0].lower() != b"bearer" or len(auth_header) != 2:
            return None
        try:
            payload = jwt.decode(
                auth_header[1],
                request.data.get("video_settings", {}).get("secret"),
                algorithms=["HS256"],
            )
            return payload
        except jwt.InvalidTokenError:
            return None
  1. Extracting form handling to a service:
class VideoSettingsService:
    @staticmethod
    def configure_settings(event, video_tokens):
        video_settings_data = {
            "token": video_tokens[0],
            "url": f"{settings.EVENTYAY_VIDEO_BASE_PATH}/api/v1/worlds/{event.slug}/"
        }
        form = VenuelessSettingsForm(event=event, data=video_settings_data)
        if form.is_valid():
            form.save()
            return True, None
        return False, form.errors
  1. Simplified endpoint:
@api_view(["POST"])
@authentication_classes([VideoTokenMiddleware])
def configure_video_settings(request):
    payload = request.auth  # Set by middleware
    try:
        with scopes_disabled():
            event = Event.objects.get(slug=payload["slug"])
            success, errors = VideoSettingsService.configure_settings(
                event, payload["video_tokens"]
            )
            if success:
                logger.info("Video settings configured for event %s", event.slug)
                return Response({"status": "success"})
            logger.error("Failed to configure video settings: %s", errors)
            return Response({"status": "error", "errors": errors}, status=400)
    except Event.DoesNotExist:
        logger.error("Event %s not found", payload["slug"])
        return Response({"status": "error", "message": "Event not found"}, status=404)

This restructuring:

  • Separates concerns (auth, business logic, API handling)
  • Reduces nesting
  • Maintains error handling and logging
  • Makes the code more testable

odkhang marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The 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:

  1. Eliminates one layer of function calls and error handling
  2. Consolidates related error types
  3. Maintains separation of token validation logic
  4. Preserves all functionality and security checks

Copy link
Member

Choose a reason for hiding this comment

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

As reminded in group chat, use constants from http module, don't use hardcode for HTTP methods, statuses.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
"""
try:
video_settings = request.data.get("video_settings")

if not video_settings or "secret" not in video_settings:
raise ValueError("Video settings are missing or secret is not provided")

payload = get_payload_from_token(request, video_settings)
event_slug = payload.get("event_slug")
video_tokens = payload.get("video_tokens")

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(
Copy link
Member

@hongquan hongquan Dec 15, 2024

Choose a reason for hiding this comment

The 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 status field.
In case of validation error (error due to user submitting data in wrong shape), DRF returns errors as a dict like this:

{
   'field_1': ['Error 1', 'Error 2'],
   'field_2': ['Error 1', 'Error 2'],
 }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @hongquan, thanks for the comment.
I implement this REST API which will be called by Video system, it not show the error on template, and it behavior different with others API.
I'm not sure its answered your concern or not, please let me know if you have any else question.

Copy link
Member

Choose a reason for hiding this comment

The 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
"""

if not video_tokens:
raise ValueError("Video tokens list is empty")

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)
Copy link
Member

Choose a reason for hiding this comment

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

Use enum from http module, don't use hardcode 200.

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,
)
3 changes: 3 additions & 0 deletions src/pretalx/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,9 @@ def merge_csp(*options, config=None):
EVENTYAY_TICKET_BASE_PATH = config.get(
"urls", "eventyay-ticket", fallback="https://app-test.eventyay.com/tickets"
)
EVENTYAY_VIDEO_BASE_PATH = config.get(
"urls", "eventyay-video", fallback="https://app-test.eventyay.com/video"
)

SITE_ID = 1
# for now, customer must verified their email at eventyay-ticket, so this check not required
Expand Down
Loading