fix(settings): add missing ADMINS, VIDEO_SERVER_HOSTNAME, CACHE_TICKE…#2976
Draft
Rachit7168 wants to merge 29 commits intofossasia:devfrom
Draft
fix(settings): add missing ADMINS, VIDEO_SERVER_HOSTNAME, CACHE_TICKE…#2976Rachit7168 wants to merge 29 commits intofossasia:devfrom
Rachit7168 wants to merge 29 commits intofossasia:devfrom
Conversation
…TS_HOURS, FETCH_ECB_RATES Made-with: Cursor
Contributor
Reviewer's GuideAdds missing configuration settings (ADMINS, VIDEO_SERVER_HOSTNAME, CACHE_TICKETS_HOURS, FETCH_ECB_RATES) to the central config and wires them into Django settings, plus guards video world creation when the video server hostname is not configured. Sequence diagram for video world creation with VIDEO_SERVER_HOSTNAME guardsequenceDiagram
participant TasksModule as eventyay_common_tasks
participant Settings as Django_settings
participant VideoServer as Video_server
TasksModule->>Settings: read VIDEO_SERVER_HOSTNAME
alt VIDEO_SERVER_HOSTNAME not set
Settings-->>TasksModule: None
TasksModule->>TasksModule: log warning and return None
else VIDEO_SERVER_HOSTNAME set
Settings-->>TasksModule: base URL
TasksModule->>TasksModule: build payload and headers
TasksModule->>VideoServer: POST /worlds with payload
VideoServer-->>TasksModule: response or error
end
Class diagram for updated BaseSettings configuration fieldsclassDiagram
class BaseSettings {
bool admin_audit_comments_asked
str call_for_speaker_login_button_label
list~tuple~ admins
HttpUrl video_server_hostname
int cache_tickets_hours
bool fetch_ecb_rates
}
class ConfProxy {
+linkedin_client_id
+linkedin_client_secret
+admins
+video_server_hostname
+cache_tickets_hours
+fetch_ecb_rates
}
class DjangoSettingsModule {
+ADMINS
+VIDEO_SERVER_HOSTNAME
+CACHE_TICKETS_HOURS
+FETCH_ECB_RATES
}
ConfProxy <|-- BaseSettings
ConfProxy --> DjangoSettingsModule : provides_values_for
Flow diagram for conditional use of external video serverflowchart TD
A[Eventyay app _create_world task] --> B{VIDEO_SERVER_HOSTNAME configured?}
B -- No --> C[Log warning and skip video world creation]
B -- Yes --> D[Build video traits and payload]
D --> E[Call external video server API]
E --> F[Process response]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses runtime AttributeError incidents caused by code referencing undefined Django settings by introducing the missing configuration keys in the centralized settings module and adding a safe-guard in the video world creation task.
Changes:
- Add
ADMINS,VIDEO_SERVER_HOSTNAME,CACHE_TICKETS_HOURS, andFETCH_ECB_RATEStoapp/eventyay/config/settings.py(Pydantic config fields + exported Django settings constants). - Skip video world creation in
create_worldwhenVIDEO_SERVER_HOSTNAMEis not configured, avoiding invalid URL usage and noisy task failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/eventyay/eventyay_common/tasks.py | Adds a configuration guard to skip video world creation when VIDEO_SERVER_HOSTNAME is unset. |
| app/eventyay/config/settings.py | Defines missing settings in BaseSettings and exports corresponding uppercase Django settings constants. |
Contributor
Author
|
@hongquan , |
Member
|
Please learn from #2154 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add ADMINS, VIDEO_SERVER_HOSTNAME, CACHE_TICKETS_HOURS, FETCH_ECB_RATES to settings. Code was trying to access these but they weren't defined.
Partial part of Issue : #1546
and inactive PR : #2154
Summary by Sourcery
Define missing configuration settings and wire them into Django settings, including safer handling when the video server is not configured.
Bug Fixes: