Eventyay-Common: Add option to create video access to the event #385
Conversation
Reviewer's Guide by SourceryThis pull request implements a new feature to add an option for creating video access to events in the Eventyay-Common system. The changes include adding a checkbox in the event creation and edit pages, which triggers an API call to Eventyay-video to create a World/Event in video when checked. The implementation involves modifications to several files, including new functions, form fields, model changes, and UI updates. Sequence diagram for creating video access during event creationsequenceDiagram
actor User
participant EventForm
participant Backend
participant VideoAPI
User->>EventForm: Fill event details and check 'Create Video'
EventForm->>Backend: Submit event creation form
Backend->>Backend: Check create permission
alt User has permission
Backend->>VideoAPI: POST /api/v1/create-world/
VideoAPI-->>Backend: Video world created
else User lacks permission
Backend-->>User: Permission denied
end
User journey diagram for event creation with video accessjourney
title Event Creation with Video Access
section Event Creation
Fill Event Details: 5: User
Check 'Create Video' Option: 4: User
Submit Form: 5: User
section Backend Processing
Validate Permissions: 4: System
Create Event: 5: System
Create Video World: 3: System
section Confirmation
Display Success Message: 5: System
Redirect to Event List: 4: System
Class diagram for Event model changesclassDiagram
class Event {
+Boolean is_video_create
+add_video_call()
}
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 - here's some feedback:
Overall Comments:
- Consider extracting the
is_video_createlogic into a separate method to reduce duplication between event creation and update views. - In the
create_worldtask, consider using more specific exception handling instead of catching the genericRequestException. - For improved security, consider using a separate secret key for JWT encoding in the
generate_tokenfunction, rather thansettings.SECRET_KEY.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 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/pretix/eventyay_common/tasks.py
Outdated
| @@ -103,6 +103,48 @@ def send_event_webhook(self, user_id, event, action): | |||
| except self.MaxRetriesExceededError: | |||
| logger.error("Max retries exceeded for sending organizer webhook.") | |||
|
|
|||
| @shared_task(bind=True, max_retries=5, default_retry_delay=60) # Retries up to 5 times with a 60-second delay | |||
| def create_world(self, is_video_create, data): | |||
There was a problem hiding this comment.
suggestion: Consider improving error handling in create_world function
While the function catches and logs exceptions, it might be beneficial to add more specific error handling for different types of request exceptions. This could help in diagnosing and addressing specific issues more effectively.
def create_world(self, is_video_create, data):
try:
# Existing function logic here
except requests.exceptions.ConnectionError as e:
logger.error(f"Connection error: {e}")
raise self.retry(exc=e)
except requests.exceptions.Timeout as e:
logger.error(f"Request timed out: {e}")
raise self.retry(exc=e)
except requests.exceptions.RequestException as e:
logger.error(f"Request failed: {e}")
raise self.retry(exc=e)
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def generate_token(request): |
There was a problem hiding this comment.
🚨 suggestion (security): Reconsider token generation method for improved security
The current token generation method uses a simple SHA256 hash of the email, which might not be sufficiently secure. Consider using a more robust method, such as a cryptographically secure random number generator, to create these tokens.
import secrets
def generate_token(request):
"""
Generate secure token for video system
"""
return secrets.token_urlsafe(32)
| timezone=basics_data.get('timezone'), | ||
| locale=basics_data.get('locale'), has_permission=check_create_permission(self.request), | ||
| token=generate_token(self.request)) | ||
| create_world.delay(is_video_create=foundation_data.get('is_video_create'), data=data) |
There was a problem hiding this comment.
suggestion (bug_risk): Implement safeguard against duplicate world creation
The create_world task is called in both the done and form_valid methods. Consider adding a check to prevent duplicate world creation if a world already exists for this event.
| create_world.delay(is_video_create=foundation_data.get('is_video_create'), data=data) | |
| if not World.objects.filter(event_id=data['event_id']).exists(): | |
| create_world.delay(is_video_create=foundation_data.get('is_video_create'), data=data) | |
| else: | |
| messages.warning(self.request, _("A world for this event already exists.")) |
| @@ -248,6 +263,21 @@ | |||
| self.sform.save() | |||
|
|
|||
| tickets.invalidate_cache.apply_async(kwargs={'event': self.request.event.pk}) | |||
|
|
|||
| if check_create_permission(self.request): | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting video creation logic into separate methods for improved modularity.
The done and form_valid methods have become more complex with the addition of video creation logic. To improve readability and maintainability, consider extracting the video creation related code into a separate method. This will make the code more modular while maintaining the new functionality. Here's an example of how you could refactor the form_valid method:
class EventUpdate(DecoupleMixin, EventSettingsViewMixin, EventPermissionRequiredMixin, MetaDataEditorMixin, UpdateView):
# ... existing code ...
@transaction.atomic
def form_valid(self, form):
self._save_decoupled(self.sform)
self.sform.save()
tickets.invalidate_cache.apply_async(kwargs={'event': self.request.event.pk})
self._handle_video_creation(form)
messages.success(self.request, _('Your changes have been saved.'))
return super().form_valid(form)
def _handle_video_creation(self, form):
if check_create_permission(self.request):
form.instance.is_video_create = form.cleaned_data.get('is_video_create')
elif not check_create_permission(self.request) and form.cleaned_data.get('is_video_create'):
form.instance.is_video_create = True
else:
form.instance.is_video_create = False
data = dict(id=form.cleaned_data.get('slug'), title=form.cleaned_data.get('name').data,
timezone=self.sform.cleaned_data.get('timezone'), locale=self.sform.cleaned_data.get('locale'),
has_permission=check_create_permission(self.request), token=generate_token(self.request))
create_world.delay(is_video_create=form.cleaned_data.get('is_video_create'), data=data)This refactoring extracts the video creation logic into a separate _handle_video_creation method, making the form_valid method cleaner and easier to understand. You can apply a similar approach to the done method in the wizard view. This change improves the separation of concerns without introducing unnecessary complexity.
src/pretix/eventyay_common/tasks.py
Outdated
| try: | ||
| requests.post( | ||
| "{}/api/v1/create-world/".format(settings.VIDEO_SERVER_HOSTNAME), | ||
| json= payload, |
| @param request: user request | ||
| @return: jwt | ||
| """ | ||
| uid_token = encode_email(request.user.email) |
There was a problem hiding this comment.
The uid_token seems not to carry any info for client side to extract. If so, please use a short method with secrets module to generate it, as @sourcery-ai suggested.
There was a problem hiding this comment.
Why do you still have to create uid_token in a very complex way?
|
@odkhang Postgres tests are failing after merged dev branch here. Please check. Thank you |
Hi @mariobehling, tests are passed |
| @param request: user request | ||
| @return: jwt | ||
| """ | ||
| uid_token = encode_email(request.user.email) |
There was a problem hiding this comment.
Why do you still have to create uid_token in a very complex way?
This PR related to issue #358 Eventyay-Common: Add option to create video access to the event
Implement a checkbox in Event create/edit page, if it checked, will trigger API call to Eventyay-video to create World/Event in video.
Summary by Sourcery
Add functionality to create a video platform for events by introducing a new checkbox in the event creation and editing forms. Implement a background task to handle the video system creation when the option is selected, and update the project settings to include a video server hostname configuration.
New Features:
Enhancements:
Build: