Automatically configuring video settings for the Talk project. #242
Conversation
Reviewer's Guide by SourceryThis PR implements automatic video settings configuration for the Talk project by adding new API endpoints and functionality to handle video configuration when 'is_video_creation' is selected during event creation. The implementation includes JWT-based authentication, error handling, and settings persistence. Sequence diagram for video settings configurationsequenceDiagram
actor User
participant TicketProject
participant VideoProject
participant TalkProject
User->>TicketProject: Select 'is_video_creation'
TicketProject->>VideoProject: Call create_world API
VideoProject->>TalkProject: Call configure_video_settings API
TalkProject->>TalkProject: Validate token
TalkProject->>TalkProject: Retrieve event instance
TalkProject->>TalkProject: Save video settings
TalkProject-->>VideoProject: Return success or error response
Class diagram for video settings configurationclassDiagram
class EventViewSet {
+get_object()
}
class VenuelessSettingsForm {
+is_valid()
+save()
}
class Event {
+slug
}
class configure_video_settings {
+configure_video_settings(request)
+get_payload_from_token(request, video_settings)
+save_video_settings_information(event_slug, video_tokens, event_instance)
}
EventViewSet --> Event
configure_video_settings --> Event
configure_video_settings --> VenuelessSettingsForm
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The auth header validation logic could be more secure by checking the bearer token presence first (link)
Overall Comments:
- Consider adding request payload validation before processing the JWT token to ensure all required fields are present and properly formatted.
- The JWT token validation could be strengthened by adding token expiration checks and proper error handling for malformed tokens.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/pretalx/api/views/event.py
Outdated
| @@ -24,3 +41,97 @@ | |||
| if self.request.user.has_perm(self.permission_required, self.request.event): | |||
| return self.request.event | |||
| raise Http404() | |||
|
|
|||
|
|
|||
| @api_view(http_method_names=["POST"]) | |||
There was a problem hiding this comment.
issue (complexity): Consider restructuring the code to use middleware, services, and simplified endpoint handlers.
The code could be simplified while maintaining all functionality by:
- 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- 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- 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
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- The JWT secret should be validated before use in token decoding. Consider adding validation to handle cases where video_settings is None or doesn't contain a secret key, potentially using a default secret from settings.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- The configure_video_settings view function is missing a return statement - it should return the response from save_video_settings_information()
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/pretalx/api/views/event.py
Outdated
| @@ -24,3 +41,120 @@ | |||
| if self.request.user.has_perm(self.permission_required, self.request.event): | |||
| return self.request.event | |||
| raise Http404() | |||
|
|
|||
|
|
|||
| @api_view(http_method_names=["POST"]) | |||
There was a problem hiding this comment.
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:
- Eliminates one layer of function calls and error handling
- Consolidates related error types
- Maintains separation of token validation logic
- Preserves all functionality and security checks
src/pretalx/api/views/event.py
Outdated
| ) | ||
| except ValueError as e: | ||
| logger.error("Error configuring video settings: %s", e) | ||
| return Response( |
There was a problem hiding this comment.
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'],
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/pretalx/api/views/event.py
Outdated
| @@ -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"]) | |||
There was a problem hiding this comment.
As reminded in group chat, use constants from http module, don't use hardcode for HTTP methods, statuses.
There was a problem hiding this comment.
thanks, I have updated the code please help me to check it.
src/pretalx/api/views/event.py
Outdated
| ) | ||
| except ValueError as e: | ||
| logger.error("Error configuring video settings: %s", e) | ||
| return Response( |
There was a problem hiding this comment.
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.
src/pretalx/api/views/event.py
Outdated
| 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.
Use enum from http module, don't use hardcode 200.
|
Hi @hongquan, 404 error Validation error Failed authentication |
This PR resolves fossasia/eventyay-tickets#465
Summary by Sourcery
Implement automatic video settings configuration for events by introducing a new API endpoint 'configure_video_settings' that synchronizes data between the talk and video projects.
New Features:
Enhancements: