86 lines
4.7 KiB
Markdown
86 lines
4.7 KiB
Markdown
# CCLI SongSelect Import — Issues
|
|
|
|
## [2026-05-10] Known Issues / Gotchas
|
|
|
|
### T1: Fixture corpus requires structurally accurate CCLI format
|
|
- Plan originally asked for "real CCLI text pastes" but since we can't access SongSelect, agent creates synthetic fixtures.
|
|
- Synthetic fixtures MUST match exact CCLI format: title line, blank, section label, lyrics, blank, footer with © and CCLI #.
|
|
- The parser tests depend on these fixtures — structural accuracy is critical.
|
|
|
|
### T7: ProImportService method name
|
|
- Plan was corrected: the public method is `ProImportService::import(UploadedFile $file)`, not `upsertSong()`.
|
|
- When mirroring the pattern, READ app/Services/ProImportService.php before implementing.
|
|
|
|
### No Admin Role
|
|
- No `admin` role or Policy exists in the codebase.
|
|
- CCLI Settings section is visible to ALL authenticated users.
|
|
- Document this decision, don't create a policy gate.
|
|
|
|
### AGENDA_KEYS whitelist
|
|
- SettingsController has a `const AGENDA_KEYS` array.
|
|
- T4 MUST add `'default_translation_language'` to this array OR update the validation to include it.
|
|
- Failure to update AGENDA_KEYS = PATCH /settings will silently ignore the new key.
|
|
|
|
### Translation Pairing Label Direction
|
|
- CCLI paste can have English labels; local songs may have German labels (Strophe, Refrain).
|
|
- CcliLabels::normalizeLabelName() normalizes BOTH directions to canonical English before pairing.
|
|
- Do NOT assume same language on both sides.
|
|
|
|
### T7: Global Label Slide Replacement Caveat
|
|
- The requested CCLI import pattern deletes `songSlides()` on the resolved global `Label` before recreating slides.
|
|
- Because labels are shared globally, a later import using the same canonical label name replaces that label's slide text globally; this intentionally matches the task spec and existing `ProImportService` pattern, but remains a design caveat for future per-song slide ownership work.
|
|
|
|
### T8: Translation Pairing Leaves Missing Sections Non-Fatal
|
|
- `CcliTranslationPairingService` intentionally does not throw when a local arrangement label is absent from the CCLI paste.
|
|
- Missing labels are returned in `unmatched_labels`, and their mapping entries keep empty slide placeholders so `distributed_text` still aligns with the local arrangement shape.
|
|
|
|
## Code Quality Review (2026-05-11)
|
|
|
|
### Found
|
|
- **app/Services/CcliPasteParser.php:17-19** — Empty `if` block in constructor.
|
|
Constructor accepts optional closures (`$sectionDetector`, `$metadataDetector`) but the body
|
|
contains `if ($sectionDetector !== null || $metadataDetector !== null) { }` with no statements.
|
|
Dead code — same category as forbidden empty `catch` blocks. Remove the entire `if` statement.
|
|
|
|
### Passed
|
|
- Pint: 9 files clean (after fixing `-rw-------` perms on DTO + Service files)
|
|
- Tests: 503 passed, 0 failed
|
|
- Build: vite production build clean
|
|
- No `as any`, `@ts-ignore`, `console.log` in Vue
|
|
- No `@` error suppression in PHP
|
|
- German Du-form throughout (no `Sie`/`Ihre`)
|
|
- All `final` + `readonly` correct
|
|
- data-testid coverage complete on interactive Vue elements
|
|
|
|
### Side note — file permissions
|
|
Several new PHP files were created with `-rw-------` (owner-only). DDEV web container could not
|
|
read them, breaking Pint. Fixed by `chmod 644`. Likely an editor/umask issue worth investigating
|
|
in the dev environment to avoid recurrence.
|
|
|
|
### Resolution (2026-05-11 re-review)
|
|
- Empty `if` block in `CcliPasteParser.php` removed; constructor is now `) {}`.
|
|
- Pint PASS on the file.
|
|
- Tests: 503 passed.
|
|
- Build: clean.
|
|
- Vue: no forbidden patterns.
|
|
- Final verdict: APPROVE.
|
|
|
|
## QA Finding (2026-05-11): API auth broken via DDEV URL
|
|
|
|
**Root cause**: `APP_URL=http://pp-planer.test` in `.env`, but DDEV serves on `https://pp-planer.ddev.site`.
|
|
`config/sanctum.php` derives `SANCTUM_STATEFUL_DOMAINS` from `APP_URL` host → list contains `pp-planer.test` only.
|
|
Cookie-based auth on `/api/*` routes (`auth:sanctum` middleware) fails when accessed via `pp-planer.ddev.site`.
|
|
|
|
**Symptom**:
|
|
- `/api/ccli/preview` with `Accept: application/json` → `401 Unauthenticated`
|
|
- `/api/ccli/preview` with default `Accept: */*` (Vue dialog's fetch) → 200 HTML (auth redirect), then `res.json()` throws "Unexpected token '<'" → caught → dialog shows "Netzwerkfehler. Bitte versuche es erneut."
|
|
- E2E tests (`ccli-paste-import.spec.ts` tests 6+7) skip silently via `test.skip()` guards when API fails, masking this in CI.
|
|
|
|
**Fix**: Add to `.env` and `.env.example`:
|
|
```
|
|
SANCTUM_STATEFUL_DOMAINS=pp-planer.ddev.site,pp-planer.test,localhost,127.0.0.1
|
|
```
|
|
Or change `APP_URL` to `https://pp-planer.ddev.site` if DDEV is the canonical dev URL.
|
|
|
|
**Bookmarklet has matching issue**: `BookmarkletController` uses `Config::get('app.url')` → bookmarklet opens `http://pp-planer.test/songs/import-from-ccli-paste?prefill=...` instead of the ddev URL.
|