Eventyay Common: Create a dashboard and enhance settings page #498
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new dashboard for Eventyay Common, enhancing the user interface with event overviews and quick actions. It also refines the settings page by integrating video and talk system controls directly into the event settings, improving user experience and feature accessibility. Sequence diagram for enabling video system in event settingssequenceDiagram
actor User
participant UI as Settings UI
participant View as EventUpdate View
participant Task as CreateWorldTask
participant Plugin as Video Plugin
User->>UI: Click Enable Video
UI->>View: POST enable_video_system
View->>View: Check permissions
View->>Task: create_world.delay()
Task->>Task: Setup video world
Task->>Plugin: Install and configure
Task-->>View: Success response
View-->>UI: Redirect to settings
UI-->>User: Show enabled status
Class diagram for the new EventWidgetGenerator and task classesclassDiagram
class EventWidgetGenerator {
+get_event_query(qs: QuerySet, nmax: int, lazy: bool)
+format_event_daterange(event: Event, tz: DstTzInfo)
+format_event_times(event: Event, tz: DstTzInfo, request: HttpRequest)
+generate_video_button(event: Event)
+generate_talk_button(event: Event)
+generate_widget(event: Event, request: HttpRequest, lazy: bool)
}
class CreateWorldTask {
+add_plugin(event: Event, plugin_name: str)
+attach_plugin_to_event(plugin_name: str, event_slug: str)
+save_video_settings_information(event_id: str, video_settings: dict)
+check_installed_plugin(plugin_name: str)
+extract_jwt_config(world_data: dict)
+setup_video_plugin(world_data: dict)
+on_success(retval: Any, task_id: str, args: tuple, kwargs: dict)
}
class SendEventTask {
+on_success(retval: Any, task_id: str, args: tuple, kwargs: dict)
}
class Task
CreateWorldTask --|> Task
SendEventTask --|> Task
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Please remove "Setting for" in front of component Change "Enabled/Disabled" into a button. |
@mariobehling Could give me more detail with the button "Enabled/Disabled" here? |
Just keep the feature as is and change the UI into a button. If the feature is disabled and can be enabled show a pop up "Do you want to enable the feature? Yes / Cancel" If the feature is enabled and the user clicks it show a pop-up "The feature cannot be disabled, but if you do not publish it, it will not show up publicly. Ok". |
This reverts commit 8ef4ec9.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @HungNgien - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding caching for the widget queries to optimize performance of the lazy loading implementation
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
I tested everything locally, and the talk_hostname in the server's configuration file, which points to the talk component, always ends with "talk" |
There was a problem hiding this comment.
Thanks for sharing. Please make the following changes.
-
Links to talk component on dashboard and settings page are not working.
Current link is https://app-test.eventyay.com/orga/event/nvw3u
Expected is https://app-test.eventyay.com/talk/orga/event/nvw3u
Also check settings page links, please. -
We need to standardize the description. We are using sometimes singular and sometimes plural for tickets and talks. Please change the blue menu items:
ticket -> tickets
talk -> talks
Also check on settings page. -
Pop-up "Component not enabled!"
- Effect: Avoid moving effect from top and moving out effect. Simply display pop-up as fast as possible.
- Follow the design of other existing pop-ups.
a) Add the same icon on the left.
b) Use same white overlay as other pop-ups.
c) Add a pop-up heading "Component not enabled."
d) Change "Close" to "Ok"
|
|
|
Still not resolved: Links to talk component on dashboard and settings page are not working. |
Thank you. The pop up looks good. Please increase the spacing to on the edges to match the existing pop up. Clicking "Yes" on the button does not do anything yet. |
So the reason is the talk_hostname of .cfg file is missing a backslash at the end which make the urljoin failed |
|
|
Yes, thanks. |
|
Please also resolve conflicts. |
There was a problem hiding this comment.
Hey @HungNgien - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding rate limiting to the API endpoints to prevent potential abuse
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
| response = requests.post( | ||
| settings.TALK_HOSTNAME + "/webhook/team/", json=payload, headers=headers | ||
| urljoin(settings.TALK_HOSTNAME, "webhook/team/"), | ||
| json=payload, | ||
| headers=headers | ||
| ) |
There was a problem hiding this comment.
🚨 suggestion (security): Add timeout parameters to HTTP requests to prevent hanging tasks
Add explicit timeouts to prevent requests from hanging indefinitely:
response = requests.post(
url,
json=payload,
headers=headers,
timeout=(3.05, 27) # (connect timeout, read timeout)
)| response = requests.post( | |
| settings.TALK_HOSTNAME + "/webhook/team/", json=payload, headers=headers | |
| urljoin(settings.TALK_HOSTNAME, "webhook/team/"), | |
| json=payload, | |
| headers=headers | |
| ) | |
| response = requests.post( | |
| urljoin(settings.TALK_HOSTNAME, "webhook/team/"), | |
| json=payload, | |
| headers=headers, | |
| timeout=(3.05, 27) # (connect timeout, read timeout) | |
| ) |
| settings.TALK_HOSTNAME + "/webhook/team/", json=payload, headers=headers | ||
| urljoin(settings.TALK_HOSTNAME, "webhook/team/"), | ||
| json=payload, | ||
| headers=headers | ||
| ) | ||
| response.raise_for_status() # Raise exception for bad status codes | ||
| except requests.RequestException as e: |
There was a problem hiding this comment.
suggestion (bug_risk): Enhance error handling to provide more context about failed requests
Include more details in the error log to help with debugging:
except requests.RequestException as e:
logger.error('Error sending webhook to talk component: %s. URL: %s, Status: %s',
str(e), e.request.url if hasattr(e, 'request') else 'unknown',
e.response.status_code if hasattr(e, 'response') else 'unknown')Suggested implementation:
except requests.RequestException as e:
logger.error('Error sending webhook to talk component: %s. URL: %s, Status: %s',
str(e), e.request.url if hasattr(e, 'request') else 'unknown',
e.response.status_code if hasattr(e, 'response') else 'unknown')The logger import should be added at the top of the file with the other imports. If there's already a logger defined elsewhere in the codebase that's being used, you may want to use that instead of creating a new one. In that case, you'd need to import the existing logger instead of adding the logging import.
Update dashboard of Eventyay Common
Enable feature
Enabled
Enhance settings page
This PR resolves #443
Summary by Sourcery
New Features:
Summary by Sourcery
New Features: