From 091e00f25575d48221b9ead46e2177f42561b283 Mon Sep 17 00:00:00 2001 From: Thorsten Bus Date: Sun, 10 May 2026 18:56:38 +0200 Subject: [PATCH] feat(ccli): add CcliImportService for song upsert Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../evidence/task-7-duplicate-throws.txt | 10 ++ .sisyphus/evidence/task-7-en-only-import.txt | 13 ++ .sisyphus/evidence/task-7-restore.txt | 9 ++ .sisyphus/evidence/task-7-rollback.txt | 9 ++ .../notepads/ccli-songselect-import/issues.md | 31 ++++ .../ccli-songselect-import/learnings.md | 6 + app/Exceptions/DuplicateCcliSongException.php | 15 ++ app/Services/CcliImportService.php | 151 ++++++++++++++++++ tests/Feature/CcliImportServiceTest.php | 133 +++++++++++++++ 9 files changed, 377 insertions(+) create mode 100644 .sisyphus/evidence/task-7-duplicate-throws.txt create mode 100644 .sisyphus/evidence/task-7-en-only-import.txt create mode 100644 .sisyphus/evidence/task-7-restore.txt create mode 100644 .sisyphus/evidence/task-7-rollback.txt create mode 100644 .sisyphus/notepads/ccli-songselect-import/issues.md create mode 100644 app/Exceptions/DuplicateCcliSongException.php create mode 100644 app/Services/CcliImportService.php create mode 100644 tests/Feature/CcliImportServiceTest.php diff --git a/.sisyphus/evidence/task-7-duplicate-throws.txt b/.sisyphus/evidence/task-7-duplicate-throws.txt new file mode 100644 index 0000000..4860a84 --- /dev/null +++ b/.sisyphus/evidence/task-7-duplicate-throws.txt @@ -0,0 +1,10 @@ +Command: ddev exec php artisan test --filter='blocks active duplicate ccli_id with DuplicateCcliSongException' + +PASS: Active duplicate CCLI-ID is blocked. + +Verified assertions: +- first import succeeds +- second import throws App\Exceptions\DuplicateCcliSongException +- exception existingSongId matches original Song ID +- exception message contains "existiert bereits" +- Song::count() remains 1 diff --git a/.sisyphus/evidence/task-7-en-only-import.txt b/.sisyphus/evidence/task-7-en-only-import.txt new file mode 100644 index 0000000..4800f39 --- /dev/null +++ b/.sisyphus/evidence/task-7-en-only-import.txt @@ -0,0 +1,13 @@ +Command: ddev exec php artisan test --filter='imports english-only fixture and creates song with default arrangement' + +PASS: CcliImportService imported tests/fixtures/ccli/english-only-multi-verse.txt + +Verified assertions: +- status = created +- Song title = Test Song 1 +- Song ccli_id = 9999001 +- imported_from_ccli_at is set +- ccli_source_url = https://songselect.ccli.com/Songs/9999001 +- default arrangement name = normal, is_default = true +- arrangement label entries = 5 +- slide rows = 9 diff --git a/.sisyphus/evidence/task-7-restore.txt b/.sisyphus/evidence/task-7-restore.txt new file mode 100644 index 0000000..919eb64 --- /dev/null +++ b/.sisyphus/evidence/task-7-restore.txt @@ -0,0 +1,9 @@ +Command: ddev exec php artisan test --filter='restores soft-deleted song and does not duplicate normal arrangement' + +PASS: Soft-deleted CCLI match is restored instead of duplicated. + +Verified assertions: +- restored import status = restored +- returned Song ID matches original soft-deleted Song ID +- restored song is no longer trashed +- only one normal arrangement exists after restore/import diff --git a/.sisyphus/evidence/task-7-rollback.txt b/.sisyphus/evidence/task-7-rollback.txt new file mode 100644 index 0000000..6e2bd21 --- /dev/null +++ b/.sisyphus/evidence/task-7-rollback.txt @@ -0,0 +1,9 @@ +Command: ddev exec php artisan test --filter='rolls back song and log when slide creation fails' + +PASS: Transaction rollback verified via SQLite trigger failure on song_slides insert. + +Verified assertions: +- import throws Illuminate\Database\QueryException +- Song::count() = 0 after failure +- SongSlide::count() = 0 after failure +- ApiRequestLog::count() = 0 after failure diff --git a/.sisyphus/notepads/ccli-songselect-import/issues.md b/.sisyphus/notepads/ccli-songselect-import/issues.md new file mode 100644 index 0000000..07df7ad --- /dev/null +++ b/.sisyphus/notepads/ccli-songselect-import/issues.md @@ -0,0 +1,31 @@ +# 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. diff --git a/.sisyphus/notepads/ccli-songselect-import/learnings.md b/.sisyphus/notepads/ccli-songselect-import/learnings.md index 6cf9603..67db235 100644 --- a/.sisyphus/notepads/ccli-songselect-import/learnings.md +++ b/.sisyphus/notepads/ccli-songselect-import/learnings.md @@ -85,6 +85,12 @@ ### 2026-05-10 CcliPasteParser Implementation - EN/DE side-by-side imports merge only adjacent labels with different raw kinds but the same `CcliLabels::normalizeLabelName()` canonical kind/number, preserving German lyrics in `linesTranslated`. - DDEV/Linux path is `tests/fixtures/ccli` (lowercase); macOS accepted `tests/Fixtures/ccli`, but tests must use lowercase for container portability. +### 2026-05-10 CcliImportService Implementation +- `CcliImportService` mirrors `ProImportService` by wrapping song metadata, global label resolution, slide replacement, default arrangement upsert, arrangement-label recreation, and `ApiRequestLog` success entry in one `DB::transaction()`. +- Active duplicate CCLI IDs are blocked with `DuplicateCcliSongException`; trashed matches are restored and updated in-place via `Song::withTrashed()->where('ccli_id', ...)`. +- CCLI label names should be canonicalized with `CcliLabels::normalizeLabelName($kind.' '.$number)` before `Label::firstOrCreate()`, keeping labels global/shared. +- Import tests can verify rollback deterministically with a temporary SQLite trigger that aborts `song_slides` insert; this proves song + log rows are not persisted after mid-transaction failure. + ### 2026-05-10 CCLI Parser Review Fixes - CCLI SongSelect metadata can appear as `CCLI Song #`, `CCLI-Nr.` or `CCLI-Liednummer`; extraction must ignore `CCLI License/Lizenz` numbers. - Parsed section `kind` is canonicalized via `CcliLabels::normalizeLabelName()`, while the original pasted label remains available in `label`; translation pairing still compares raw label kinds internally. diff --git a/app/Exceptions/DuplicateCcliSongException.php b/app/Exceptions/DuplicateCcliSongException.php new file mode 100644 index 0000000..7329994 --- /dev/null +++ b/app/Exceptions/DuplicateCcliSongException.php @@ -0,0 +1,15 @@ +parser->parse($rawText); + + if ($parsed->ccliId === null || trim($parsed->ccliId) === '') { + throw new RuntimeException('Keine CCLI-Nummer gefunden — bitte vollständige SongSelect-Liedseite einfügen.'); + } + + $song = Song::withTrashed()->where('ccli_id', $parsed->ccliId)->first(); + $status = 'created'; + + if ($song !== null && ! $song->trashed()) { + throw new DuplicateCcliSongException($song->id); + } + + if ($song !== null) { + $status = 'restored'; + } + + return DB::transaction(function () use ($parsed, $sourceUrl, $song, $status, $startedAt): array { + if ($song !== null && $song->trashed()) { + $song->restore(); + } + + $song = $this->upsertSong($parsed, $sourceUrl, $song); + $warnings = []; + + $translationLanguage = Setting::get('default_translation_language', 'DE'); + if ($translationLanguage === null || trim($translationLanguage) === '') { + $warnings[] = 'Keine Standard-Übersetzungssprache gesetzt, DE wird verwendet.'; + } + + $labelIds = []; + $hasTranslation = false; + + foreach ($parsed->sections as $section) { + $label = $this->resolveLabel($section); + $labelIds[] = $label->id; + + $label->songSlides()->delete(); + + foreach ($section->lines as $order => $line) { + $translatedLine = $section->linesTranslated[$order] ?? null; + $hasTranslation = $hasTranslation || ($translatedLine !== null && trim($translatedLine) !== ''); + + SongSlide::create([ + 'label_id' => $label->id, + 'order' => $order + 1, + 'text_content' => $line, + 'text_content_translated' => $translatedLine, + ]); + } + } + + $song->update([ + 'has_translation' => $hasTranslation, + 'imported_from_ccli_at' => now(), + 'ccli_source_url' => $sourceUrl ?? $parsed->sourceUrl, + ]); + + $arrangement = SongArrangement::updateOrCreate( + ['song_id' => $song->id, 'name' => 'normal'], + ['is_default' => true], + ); + + SongArrangementLabel::where('song_arrangement_id', $arrangement->id)->delete(); + + foreach ($labelIds as $order => $labelId) { + SongArrangementLabel::create([ + 'song_arrangement_id' => $arrangement->id, + 'label_id' => $labelId, + 'order' => $order + 1, + ]); + } + + $song = $song->fresh(['arrangements.arrangementLabels.label.songSlides']); + + ApiRequestLog::create([ + 'method' => 'import', + 'endpoint' => 'paste', + 'status' => 'success', + 'request_context' => ['ccli_id' => $parsed->ccliId, 'mode' => $status], + 'response_summary' => "Song {$status}: {$song->title}", + 'response_body' => null, + 'duration_ms' => (int) round((microtime(true) - $startedAt) * 1000), + ]); + + return ['song' => $song, 'status' => $status, 'warnings' => $warnings]; + }); + } + + private function upsertSong(ParsedCcliSong $parsed, ?string $sourceUrl, ?Song $song): Song + { + $songData = [ + 'title' => $parsed->title, + 'author' => $parsed->author, + 'copyright_text' => $parsed->copyrightText, + 'copyright_year' => $parsed->year, + 'publisher' => $parsed->copyrightText, + 'ccli_source_url' => $sourceUrl ?? $parsed->sourceUrl, + ]; + + if ($song !== null) { + $song->update($songData); + + return $song; + } + + return Song::create(array_merge($songData, ['ccli_id' => $parsed->ccliId])); + } + + private function resolveLabel(ParsedCcliSection $section): Label + { + $canonicalLabelName = CcliLabels::normalizeLabelName( + $section->kind.($section->number ? ' '.$section->number : ''), + ); + + return Label::firstOrCreate( + ['name' => $canonicalLabelName], + ['color' => self::DEFAULT_LABEL_COLOR, 'last_imported_at' => now()], + ); + } +} diff --git a/tests/Feature/CcliImportServiceTest.php b/tests/Feature/CcliImportServiceTest.php new file mode 100644 index 0000000..33c07f9 --- /dev/null +++ b/tests/Feature/CcliImportServiceTest.php @@ -0,0 +1,133 @@ +import( + ccliFixture('english-only-multi-verse.txt'), + 'https://songselect.ccli.com/Songs/9999001', + ); + + expect($result['status'])->toBe('created') + ->and($result['warnings'])->toBeArray() + ->and($result['song'])->toBeInstanceOf(Song::class); + + $song = $result['song']->fresh(); + + expect($song->title)->toBe('Test Song 1') + ->and($song->author)->toBe('Test Artist 1') + ->and($song->ccli_id)->toBe('9999001') + ->and($song->copyright_year)->toBe('2024') + ->and($song->has_translation)->toBeFalse() + ->and($song->imported_from_ccli_at)->not->toBeNull() + ->and($song->ccli_source_url)->toBe('https://songselect.ccli.com/Songs/9999001'); + + $arrangement = $song->arrangements()->where('name', 'normal')->first(); + + expect($arrangement)->not->toBeNull() + ->and($arrangement->is_default)->toBeTrue() + ->and(SongArrangementLabel::where('song_arrangement_id', $arrangement->id)->count())->toBe(5) + ->and(SongSlide::count())->toBe(9); +}); + +test('imports english and german fixture and stores translated slide text', function () { + $service = app(CcliImportService::class); + + $result = $service->import(ccliFixture('english-german-side-by-side.txt')); + $song = $result['song']->fresh(); + + expect($result['status'])->toBe('created') + ->and($song->has_translation)->toBeTrue() + ->and(SongSlide::whereNotNull('text_content_translated')->count())->toBe(4) + ->and(SongSlide::where('text_content_translated', 'Deutsche Liedzeile 1 zum gleichen Gedanken')->exists())->toBeTrue(); +}); + +test('blocks active duplicate ccli_id with DuplicateCcliSongException', function () { + $service = app(CcliImportService::class); + $first = $service->import(ccliFixture('english-only-multi-verse.txt')); + + expect(fn () => $service->import(ccliFixture('english-only-multi-verse.txt'))) + ->toThrow(DuplicateCcliSongException::class); + + try { + $service->import(ccliFixture('english-only-multi-verse.txt')); + } catch (DuplicateCcliSongException $exception) { + expect($exception->existingSongId)->toBe($first['song']->id) + ->and($exception->getMessage())->toContain('existiert bereits'); + } + + expect(Song::count())->toBe(1); +}); + +test('restores soft-deleted song and does not duplicate normal arrangement', function () { + $service = app(CcliImportService::class); + $first = $service->import(ccliFixture('english-only-multi-verse.txt')); + $songId = $first['song']->id; + + Song::find($songId)->delete(); + + $result = $service->import(ccliFixture('english-only-multi-verse.txt')); + $restoredSong = Song::withTrashed()->find($songId); + + expect($result['status'])->toBe('restored') + ->and($result['song']->id)->toBe($songId) + ->and($restoredSong->trashed())->toBeFalse() + ->and($restoredSong->arrangements()->where('name', 'normal')->count())->toBe(1); +}); + +test('throws RuntimeException when paste has no ccli id', function () { + $content = "Test Song Title\nTest Artist\n\nVerse 1\nSome lyrics here\n\nChorus\nChorus lyrics\n\n© 2024 Publisher"; + $service = app(CcliImportService::class); + + expect(fn () => $service->import($content))->toThrow(RuntimeException::class); + expect(Song::count())->toBe(0); +}); + +test('import creates ApiRequestLog with metadata only and no lyrics body', function () { + $service = app(CcliImportService::class); + + $service->import(ccliFixture('english-only-multi-verse.txt')); + + $log = ApiRequestLog::latest()->first(); + + expect($log)->not->toBeNull() + ->and($log->method)->toBe('import') + ->and($log->endpoint)->toBe('paste') + ->and($log->status)->toBe('success') + ->and($log->request_context)->toMatchArray(['ccli_id' => '9999001', 'mode' => 'created']) + ->and($log->response_summary)->toBe('Song created: Test Song 1') + ->and($log->response_body)->toBeNull() + ->and($log->response_summary)->not->toContain('Morning light breaks'); +}); + +test('rolls back song and log when slide creation fails', function () { + DB::statement("CREATE TRIGGER fail_ccli_slide_insert BEFORE INSERT ON song_slides BEGIN SELECT RAISE(ABORT, 'slide creation failed'); END"); + + try { + expect(fn () => app(CcliImportService::class)->import(ccliFixture('english-only-multi-verse.txt'))) + ->toThrow(QueryException::class); + } finally { + DB::statement('DROP TRIGGER IF EXISTS fail_ccli_slide_insert'); + } + + expect(Song::count())->toBe(0) + ->and(SongSlide::count())->toBe(0) + ->and(ApiRequestLog::count())->toBe(0); +});