feature(video): Webapp Integration #1038
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @suhailnadaf509, your pull request is larger than the review limit of 150000 diff characters
There was a problem hiding this comment.
- Please resolve conflicts and add details to the PR description.
- It is not possible that such a huge PR has only such a limited description.
- Please add info what this PR encompasses and very importantly what is does not solve.
- Add screenshots especially about connected areas.
There was a problem hiding this comment.
@suhailnadaf509 also mention what endpoints etc the webapp is served as so we can speed up testing
There was a problem hiding this comment.
🧠 General Review
Thanks for working on this major step of integrating the eventyay-video functionality as a component of the unified eventyay core system.
I’ve taken a detailed look at this PR using AI-assisted analysis to identify both technical and structural issues.
⚙️ Please note: You can — and should — use AI tools yourself (for example ChatGPT or GitHub Copilot Chat) to pre-review your PRs and to generate comprehensive PR descriptions.
📄 A proper PR description is missing.
Please add one that includes:
- Summary (what and why)
- Scope / Non-scope
- Development setup (backend via uv, frontend via Vite)
- Integration details (URLs, auth, WebSocket paths)
- Screenshots (landing page, room, sidebar, mobile, error view)
- Test plan
- Risks and mitigations
🔴 MUST FIX (critical items)
1. Configuration Cleanup (app/eventyay.cfg)
Problems
- Duplicate and conflicting hostnames
- Local URL committed (
url=http://127.0.0.1:8000) - Secret committed (
[django] secret=CHANGEME) - Redis/Celery DB indices inconsistent with comments
base_path=/ticketsleft unchanged while introducing/video
Fix
DJANGO_SECRET_KEY = os.environ.get("DJANGO_SECRET_KEY", "dev-only-not-for-prod")
VIDEO_HOSTNAME = os.environ.get("VIDEO_HOSTNAME", "")
TALK_HOSTNAME = os.environ.get("TALK_HOSTNAME", "")
VIDEO_BASE_PATH = os.environ.get("VIDEO_BASE_PATH", "/video")- Remove hardcoded values and duplicates
- Add
.env.examplewithDJANGO_SECRET_KEY,VIDEO_HOSTNAME,TALK_HOSTNAME,VIDEO_BASE_PATH, Redis/Celery vars - Document environment usage in README
2. Remove Re-added Plugin Folders
Delete eventyay-talk-video/ and eventyay-ticket-video/ — they were already merged via #1029.
3. Multi-domain Module and Migration
eventyay/multidomain/maindomain_urlconf.py and its migration appear.
If multi-domain routing is not yet required, comment out its registration and exclude its migration.
If it is required now, state so in the PR description and ensure a single clean 0001_initial.py.
4. Migrations (Baseline only)
We’re re-initializing the app. Remove old migrations and keep only fresh ones:
rm app/*/migrations/00*.py
uv run python manage.py makemigrations video
uv run python manage.py migrate --plan5. URL Wiring for the SPA
No explicit route ensures GET /video/ → index.html.
Add or confirm:
from django.views.generic import TemplateView
urlpatterns += [
path('video/', TemplateView.as_view(template_name='webapp/index.html')),
]Add a smoke test confirming HTTP 200.
6. Frontend Webapp Placement & Build Contract
- In
vite.config.js, setbase: '/video/'(or from env). - Output →
app/eventyay/webapp/dist. config.jsmust use env variables, not hard-coded URLs.- Add
app/eventyay/webapp/README.mdwith mount path, output, serving (Nginx/Django static), auth (session reuse), and dev proxy steps.
7. Admin and Template Changes
- Clarify which admin/template edits are intentional.
- Restore any unintended removals.
- Verify
/admin/still loads.
8. WebSocket and Live Code
- Document whether WebSocket or auth logic changed.
- If unchanged, say “no change.”
- If changed, list new endpoints and frontend discovery logic.
9. Docker Compose Alignment
- Align services with
VIDEO_BASE_PATHand Redis envs. - Add
healthcheck:entries. - If not ready, remove compose changes and note in PR description.
10. Adopted uv Toolchain (✅ Required)
uv is now mandatory for dependency management.
Add to README.md and PR description:
pip install uv
uv sync --directory app
uv run python manage.py runserver
uv run python manage.py migrate11. GitHub Actions Workflow (✅ Required)
Add .github/workflows/webapp-backend-ci.yml:
name: Webapp & Backend CI
on:
pull_request:
branches: [ enext ]
paths:
- "app/eventyay/webapp/**"
- "app/**.py"
- "app/**/**.py"
- "app/pyproject.toml"
- "app/uv.lock"
jobs:
python-uv-check:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.11"
- name: Install uv
run: pip install uv
- name: Sync dependencies
run: uv sync --directory app
- name: Django checks
run: uv run python app/manage.py check
webapp-build:
runs-on: ubuntu-latest
defaults:
run:
working-directory: app/eventyay/webapp
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: "20"
cache: "npm"
cache-dependency-path: app/eventyay/webapp/package-lock.json
- name: Install
run: npm ci
- name: Build
run: npm run build🟨 Should Fix (after main corrections)
| Area | Issue | Fix |
|---|---|---|
| Redis/Celery config | Inline comments inconsistent | Document final DB index allocation (0 = tickets, 1 = talk, 2 = video). |
| Multi-domain settings | Possibly not ready | Comment out until production ready; add header explaining intent. |
| Templates | Minor changes | Verify no side effects on talk/tickets pages. |
| Static build path | Output directory unclear | Ensure output → app/eventyay/webapp/dist and served from /video/. |
| Docker healthchecks | Missing | Add healthcheck: entries for each service. |
.env and docs |
Incomplete | Add .env.example and expand documentation with scope, setup, integration, screenshots, test plan. |
🧪 Minimal Test Plan (before merge)
Backend
- ✅
uv run python manage.py check→ OK - ✅
uv run python manage.py migrate --plan→ only clean initial steps - ✅
GET /video/→ 200 and serves SPA - ✅
/admin/→ loads without errors - ✅ Env variables load correctly
Frontend
- ✅
npm ci && npm run buildpasses - ✅ Built app functions under
/video/(no 404 chunks) - ⚙️ Optional:
npm run lintandnpm run type-checkpass
Integration
- 🔗 If WebSocket used:
npm run devconnects to expected endpoint; if unchanged, state so explicitly.
Docker
- 🐳
docker compose up --build→ services healthy - 🌐
/video/reachable on expected port and path
✅ Summary Checklist for Follow-Up
- Add proper AI-generated PR description.
- Clean and externalize config (
app/eventyay.cfg). - Remove re-added plugin folders.
- Disable multi-domain module until ready.
- Keep only clean baseline migrations.
- Add explicit URL route for
/video/. - Document webapp build and serving contract.
- Clarify admin and template changes.
- Clarify WebSocket and auth adjustments.
- Align Docker Compose or remove temporarily.
- Finalize
uvtoolchain and document usage. - Add GitHub Actions workflow for backend (
uv) and frontend build. - Document Redis/Celery DB allocations.
- Ensure static serve and healthchecks consistent.
- Add
.env.exampleand expand documentation. - Run and pass the minimal test plan before merge.
Once these are complete, the PR will be in good technical shape for UI/UX review and safe merge into the unified enext system.
There was a problem hiding this comment.
⚠️ Duplication of already-merged modules (Re-add from PR #1029)
I am confused about commit 90cf27c. Am I getting something wrong? This commit re-adds the entire eventyay-talk-video codebase (including workflows, docs, and plugin files) inside this PR.
However, these modules — eventyay-talk-video and eventyay-ticket-video — were already merged into the enext branch in PR #1029.
So this change effectively duplicates the same code and will cause redundant files, duplicate migrations, and potential path conflicts.
Required action
- Rebase your branch onto the latest
enext(which already includes PR #1029): - Remove all vendored copies of
eventyay-talk-videoand.github/workflowsthat were re-added here. - Verify after the rebase that the folder structure matches the unified layout and no nested duplicate directories like
eventyay-talk-video/remain.
|
@mariobehling Earlier, both repositories — eventyay-talk-video and eventyay-ticket-video — were mistakenly added as submodules because of a PR conflict. This commit removes the submodule links and properly adds their actual code so that the files are tracked directly in the repository. It doesn’t duplicate existing work — it just replaces the submodule references with the real codebase. |
The webapp is served at /video/{event_slug}. However, the first access requires an authentication token. To simplify this, please use the common dashboard to open the webapp — either by clicking the “Video” button next to the respective event on /common, or by selecting “Video Configuration” within the event’s dashboard. |
suhailnadaf509
force-pushed
the
webapp-addition
branch
from
ee6ca31 to
2bdf1f5
Compare
|
@sourcery-ai review this PR again |
|
Sorry @mariobehling, your pull request is larger than the review limit of 150000 diff characters |
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the video webapp into the unified eventyay-tickets project, connecting it with the common dashboard. The integration includes updating pretix_venueless plugin to work with eventyay base models, adding Django URL patterns for serving the video SPA, and implementing automatic configuration setup for video functionality.
Key changes include:
- Migration of pretix_venueless plugin from pretix to eventyay base models
- Implementation of video SPA serving through Django with automatic event configuration
- Frontend updates to remove @pretalx/schedule dependency and use local components
Reviewed Changes
Copilot reviewed 70 out of 79 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| eventyay-ticket-video/pretix_venueless/views.py | Updates imports from pretix to eventyay base models |
| eventyay-ticket-video/pretix_venueless/apps.py | Adds dual compatibility for eventyay plugin system |
| app/eventyay/webapp/ | Frontend refactoring to remove external dependencies and use local components |
| app/eventyay/multidomain/maindomain_urlconf.py | Adds video SPA serving endpoints with automatic event configuration |
| app/eventyay/eventyay_common/views/event.py | Implements automatic video plugin configuration |
Files not reviewed (1)
- app/eventyay/webapp/package-lock.json: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| try: # Eventyay fork environment | ||
| from eventyay.base.plugins import PluginConfig as _BasePluginConfig # type: ignore | ||
| except Exception: # pragma: no cover - environment mismatch | ||
| from eventyay.base.plugins import PluginConfig as _BasePluginConfig # Fallback to eventyay |
There was a problem hiding this comment.
The fallback import on line 9 is identical to line 7, making it ineffective. The second import should either be different or the fallback logic should be reconsidered.
| from eventyay.base.plugins import PluginConfig as _BasePluginConfig # Fallback to eventyay | |
| from pretix.base.plugins import PluginConfig as _BasePluginConfig # Fallback to pretix |
There was a problem hiding this comment.
This suggestion would lead to a traceback error as pretix is no longer part of eventyay-tickets
| export function emojifyString(input) { | ||
| if (!input) return | ||
| return markdownIt.renderInline(input) | ||
| // Guard against non-string inputs to avoid markdown-it crashing with state.src.replace | ||
| if (input == null) return '' | ||
| let text = input | ||
| if (typeof text !== 'string') { | ||
| // Attempt to pick a meaningful string if a common shape is used | ||
| if (typeof input.name === 'string') text = input.name | ||
| else if (typeof input.title === 'string') text = input.title | ||
| else if (typeof input.text === 'string') text = input.text | ||
| else { | ||
| // Fallback: do not attempt to render arbitrary objects | ||
| return '' | ||
| } | ||
| } | ||
| return markdownIt.renderInline(text) |
There was a problem hiding this comment.
[nitpick] The function attempts to handle object inputs by checking common properties, but this creates unclear API expectations. Consider documenting this behavior or restricting input to strings only for better maintainability.
There was a problem hiding this comment.
Thanks for the suggestion! The fallback was added mainly to prevent markdown-it from crashing when non-string inputs are passed — this was causing the admin section to throw an error and render a blank screen.
In regards to the documentation I've added a comment here
| // Backwards-compat: server emits 'event.updated' with a payload that contains both | ||
| // 'world' and 'event' keys. Mirror the 'world.updated' handling here. | ||
| 'api::event.updated'({state, commit, dispatch}, payload) { | ||
| const world = payload.world || payload.event || payload | ||
| const rooms = payload.rooms || [] | ||
| const permissions = payload.permissions | ||
| state.world = world | ||
| state.permission = permissions | ||
| commit('updateRooms', rooms) | ||
| }, |
There was a problem hiding this comment.
The backwards compatibility handlers create code duplication with identical logic. Consider extracting the common logic into a shared function to reduce duplication and maintenance burden.
| } | ||
| # Always prepend to guarantee execution before any module scripts | ||
| import json as _json | ||
| content = f"<script>window.venueless={_json.dumps(injected)}</script>" + content |
There was a problem hiding this comment.
Injecting JSON directly into script tags without proper escaping could be vulnerable to XSS if the data contains malicious content. Consider using json.dumps(..., ensure_ascii=True) or proper HTML escaping.
| content = f"<script>window.venueless={_json.dumps(injected)}</script>" + content | |
| safe_json = _json.dumps(injected, ensure_ascii=True).replace("</script>", "<\\/script>") | |
| content = f"<script>window.venueless={safe_json}</script>" + content |
| # In some flows (e.g., anonymous/kiosk or external auth), users can be created | ||
| # without an email. Guard against calling lower() on None. | ||
| if self.email: | ||
| self.email = self.email.lower() |
There was a problem hiding this comment.
While the guard prevents the immediate crash, the model field definition should properly handle the None case. Consider adding null=True, blank=True to the email field definition if users without emails are intentionally supported.
using co-pilot suggestion Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
eventyay-talk-video/INTEGRATION.md
Outdated
There was a problem hiding this comment.
This file is outdated but was newly added in this PR. It should probably be updated.
There was a problem hiding this comment.
Please also check the following:
1. Multidomain Code Present
app/eventyay/multidomain/maindomain_urlconf.py is still included.
Currently, there’s no clear indication that multi-domain routing is part of the minimal baseline.
Please:
-
If multi-domain is not yet required, keep the file but comment out or disable any active imports and ensure it’s not listed in
INSTALLED_APPS. -
If it is intended to go live now, clarify this explicitly in the PR description and confirm that:
- The migration is a single, clean
0001_initial.py. - It does not interfere with unified routing or existing URLs.
- The migration is a single, clean
-
Add a short header comment in the file noting its current status (inactive placeholder vs. active baseline).
2. Make the SPA Route Explicit
In app/eventyay/storage/urls.py, please confirm that a route is defined so that
GET /video/ → serves the SPA entry (index.html), respecting VIDEO_BASE_PATH.
If not yet included, add:
from django.views.generic import TemplateView
urlpatterns += [
path('video/', TemplateView.as_view(template_name='webapp/index.html')),
]Also include a smoke test to assert:
response = self.client.get('/video/')
self.assertEqual(response.status_code, 200)This ensures the frontend loads correctly on refresh and direct navigation.
3. Admin & Templates — Note Intent and Verify Smoke
The PR modifies app/eventyay/base/admin.py and control/templates/pretixcontrol/*.
It looks like a UI cleanup (“fixed admin header rendering”), but the reason and scope should be clear.
Please:
- Add a short note in the PR description summarizing what was changed and why.
- Verify locally that
/admin/still loads correctly and that the header and navigation render without regression. - If any admin registrations were removed temporarily, mention that explicitly.
4. WebSocket / Live Code — Document Status
The PR still modifies files in app/eventyay/features/live/ (e.g. consumers.py, modules/auth.py).
Please:
-
In the PR description, clearly state whether WebSocket endpoints or auth logic changed.
-
If unchanged, just add a line “WebSocket endpoints and auth logic remain the same as before.”
-
If changed, document:
- The new WS endpoint paths.
- How the frontend discovers them (config or env).
- Any new authentication flow if applicable.
5. CI Workflow Missing
No GitHub Actions workflow was added in this PR.
For a large integration like this, CI coverage is required before merge.
Please add .github/workflows/webapp-backend-ci.yml with:
name: Webapp & Backend CI
on:
pull_request:
branches: [ enext ]
paths:
- "app/eventyay/webapp/**"
- "app/**.py"
- "app/**/**.py"
- "app/pyproject.toml"
- "app/uv.lock"
jobs:
python-uv-check:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.11"
- name: Install uv
run: pip install uv
- name: Sync deps
run: uv sync --directory app
- name: Django check
run: uv run python app/manage.py check
webapp-build:
runs-on: ubuntu-latest
defaults:
run:
working-directory: app/eventyay/webapp
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: "20"
cache: "npm"
cache-dependency-path: app/eventyay/webapp/package-lock.json
- name: Install
run: npm ci
- name: Build
run: npm run buildThis ensures that both the backend (uv check) and frontend (Vite build) are validated before merging.
Yes this is a base PR there will soon be subsequent debugging PR's |
| if _PLUGIN_LOCAL.is_dir(): | ||
| p = str(_PLUGIN_LOCAL) | ||
| if p not in sys.path: | ||
| sys.path.insert(0, p) |
There was a problem hiding this comment.
This sounds dangerous. It points to outside the app folder, and thus will break Docker builds etc.
Is this on purpose?
There was a problem hiding this comment.
yes, the plugins are present outside the app directory
There was a problem hiding this comment.
How would docker deployment then work?
There was a problem hiding this comment.
Great catch this was pointing outside the app and would break Docker. I’ve moved the ticket-video plugin to app/plugins/eventyay-ticket-video and updated settings.py to load it from there, so the build stays within the image context
|
@Sak1012 I’ve addressed the following issues in the webapp:
|
| ], | ||
| 'connect-src': ['{dynamic}', '{media}', 'https://checkout.stripe.com', 'https:', 'blob:'], | ||
| 'img-src': ['{static}', '{media}', 'data:', 'https://*.stripe.com'] + img_src, | ||
| 'img-src': ['{static}', '{media}', 'data:', 'https://*.stripe.com', 'https://twemoji.maxcdn.com'] + img_src, |
There was a problem hiding this comment.
Thanks for the update — adding twemoji.maxcdn.com fixes the immediate CSP error, but using an external CDN for emojis isn’t ideal long-term.
Could you please switch to a self-hosted Twemoji or OpenMoji setup instead?
That way we avoid third-party requests, keep the CSP strict (img-src 'self' data:), and stay GDPR-compliant.
You can install and bundle the assets locally with npm install twemoji and serve them from /video/twemoji/ via Vite or Django staticfiles.
|
@mariobehling I've now implemented a self-hosted Twemoji setup, all emoji SVGs are served locally from /twemoji/svg/, CSP is fully strict (img-src 'self' data:), and CDN references have been removed. Documentation and assets are included in this PR |
This reverts commit 0c7b642.
| </p> | ||
| {% endif %} | ||
| <p> | ||
| {% trans "Go to" %} |
There was a problem hiding this comment.
don't use decprecated keyword, replace with translate
There was a problem hiding this comment.
{% trans %} isn't deprecated - both {% trans %} and {% translate %} are valid in Django 5.2. {% translate %} was added as an alias in Django 3.1 for readability, but both work identically. No change needed here.
There was a problem hiding this comment.
For new code, use {% translate %}, don't use old item {% trans %}. It is not correct that {% translate %} is an alias of {% trans %}. When Django introduces new things, it cannot expect users to migrate immediately, so it will keep the old items for a while, for users to migrate the code gradually, then remove it at some release in the future. So, if you see that {% trans %} is still working, it doesn't mean that we should continue to use it. If we continue to use it, we are creating more work to do in the future. It is when Django do the "clean up" step, we will still need to change our code (to replace trans with translate), but because there are too many trans, we need to work harder.
| data-toggle="tooltip">Ticket Teams | ||
| </a> | ||
| <a href='/orga/organizer/{{ organizer.slug }}/teams/' | ||
| <a href='/talk/orga/organiser/{{ organizer.slug }}/teams/' |
There was a problem hiding this comment.
please change 'organiser' to 'organizer', we don't use organiser in talk anymore
There was a problem hiding this comment.
This change was also unintentionally reintroduced during the rebase with the enext branch. These modifications are actually reverting the updates that were originally made in PR #882, I’ll remove them from this PR
app/eventyay/base/models/auth.py
Outdated
| notification_settings: QuerySet[NotificationSetting] | ||
|
|
||
| objects = UserManager().from_queryset(UserQuerySet)() | ||
| objects: UserManager = UserManager() |
There was a problem hiding this comment.
Line number 110 in app/eventyay/agenda/views/talk.py uses '.with_profiles' defined in UserQuerySet. Won't your code affect this url: /<event_slug>/talk/
There was a problem hiding this comment.
You're right, this would cause an issue. I was trying to simplify from UserManager().from_queryset(UserQuerySet)() to UserManager.from_queryset(UserQuerySet)() (removing the unnecessary instance creation), but accidentally removed the .from_queryset() part entirely. Fixed now!
|
@Gagan-Ram , the webhook-related files were unintentionally reintroduced during a rebase with the latest enext branch. These files were previously removed in PR #899, but due to the rebase, they were brought back into the diff. I’ll remove them from this PR -- Great catch! |
|
Ok, merging this. Please follow up about open items in separate PRs. Thank you. |
The video doesn't work on my side. It showed a dialog for setting my name and avatar. After that, it goes blank. |
|
@suhailnadaf509 I did run those commands, that's why a popup can appear to ask for my name and avatar. It is just that, after that popup, the page becomes blank. |
|
After pressing F5 again and again, I got this error: 
|
|
After restarting 
|
|
The "page.markdown" error is due to empty room list After creating a room, I can see the video, but encounter new error: 
Note that, the PostgreSQL error at #1038 (comment) is critical. We can not bring the website to public before it is resolved, or the website will be down very soon. |
7 participants
Summary
This PR adds the video webapp to the unified eventyay-tickets project and connects it with the common dashboard.
What’s Done
Testing
npm installnpm run buildNotes
This brings the video webapp into the main system so users can access it directly without separate setup.
