diff --git a/app/Actions/Photo/Create.php b/app/Actions/Photo/Create.php index 29b6899d123..ff1f6bad148 100644 --- a/app/Actions/Photo/Create.php +++ b/app/Actions/Photo/Create.php @@ -46,8 +46,18 @@ public function __construct( ?ImportMode $import_mode, int $intended_owner_id, UserUploadTrustLevel $upload_trust_level, + ?string $title = null, + ?string $description = null, + ?string $preallocated_id = null, ) { - $this->strategy_parameters = new ImportParam($import_mode, $intended_owner_id, upload_trust_level: $upload_trust_level); + $this->strategy_parameters = new ImportParam( + $import_mode, + $intended_owner_id, + title: $title, + description: $description, + preallocated_id: $preallocated_id, + upload_trust_level: $upload_trust_level, + ); } /** @@ -171,6 +181,7 @@ private function handleStandalone(InitDTO $init_dto): Photo $pipes = [ Standalone\FixTimeStamps::class, Standalone\InitNamingStrategy::class, + Standalone\ApplyUserProvidedMetadata::class, Shared\HydrateMetadata::class, Shared\SetHighlighted::class, Shared\SetOwnership::class, @@ -269,6 +280,7 @@ private function handlePhotoLivePartner(InitDTO $init_dto): Photo $stand_alone_pipes = [ Standalone\FixTimeStamps::class, Standalone\InitNamingStrategy::class, + Standalone\ApplyUserProvidedMetadata::class, Shared\HydrateMetadata::class, Shared\SetHighlighted::class, Shared\SetOwnership::class, diff --git a/app/Actions/Photo/Pipes/Standalone/ApplyUserProvidedMetadata.php b/app/Actions/Photo/Pipes/Standalone/ApplyUserProvidedMetadata.php new file mode 100644 index 00000000000..33a34202e53 --- /dev/null +++ b/app/Actions/Photo/Pipes/Standalone/ApplyUserProvidedMetadata.php @@ -0,0 +1,40 @@ +title !== null) { + $state->photo->title = $state->title; + } + + if ($state->description !== null) { + $state->photo->description = $state->description; + } + + return $next($state); + } +} diff --git a/app/Actions/Photo/Pipes/Standalone/AutoRenamer.php b/app/Actions/Photo/Pipes/Standalone/AutoRenamer.php index 382ee403af4..4056066a0c8 100644 --- a/app/Actions/Photo/Pipes/Standalone/AutoRenamer.php +++ b/app/Actions/Photo/Pipes/Standalone/AutoRenamer.php @@ -24,6 +24,12 @@ class AutoRenamer implements StandalonePipe { public function handle(StandaloneDTO $state, \Closure $next): StandaloneDTO { + // Skip if the caller explicitly provided a title at upload time (FR-041-06). + // User-supplied titles take precedence and must not be overwritten by renamer rules. + if ($state->title !== null) { + return $next($state); + } + // Skip if not enabled. if (!$state->shall_rename_photo_title) { return $next($state); diff --git a/app/DTO/ImportParam.php b/app/DTO/ImportParam.php index ef7bbd30c9f..4b6e372bd15 100644 --- a/app/DTO/ImportParam.php +++ b/app/DTO/ImportParam.php @@ -24,6 +24,9 @@ final class ImportParam * @param bool $is_highlighted indicates whether the new photo shall be highlighted * @param Extractor|null $exif_info the extracted EXIF information * @param bool|null $apply_watermark whether to apply watermark (null = use global setting) + * @param string|null $title user-supplied title override (takes precedence over EXIF-extracted title when non-null) + * @param string|null $description user-supplied description override (takes precedence over EXIF-extracted description when non-null) + * @param string|null $preallocated_id pre-allocated photo ID to be used on insert (see HasRandomIDAndLegacyTimeBasedID::preallocateId) * * @return void */ @@ -34,6 +37,9 @@ public function __construct( public bool $is_highlighted = false, public Extractor|null $exif_info = null, public ?bool $apply_watermark = null, + public ?string $title = null, + public ?string $description = null, + public ?string $preallocated_id = null, ?UserUploadTrustLevel $upload_trust_level = null, ) { $this->upload_trust_level = $upload_trust_level ?? throw new LycheeLogicException('Upload trust level must be provided'); diff --git a/app/DTO/PhotoCreate/InitDTO.php b/app/DTO/PhotoCreate/InitDTO.php index 808227516bc..b78f020e072 100644 --- a/app/DTO/PhotoCreate/InitDTO.php +++ b/app/DTO/PhotoCreate/InitDTO.php @@ -55,6 +55,15 @@ class InitDTO // that should be preserved as a RAW size variant after conversion to JPEG. public NativeLocalFile|null $raw_source_file = null; + // User-supplied title override (takes precedence over EXIF-extracted title when non-null). + public ?string $title = null; + + // User-supplied description override (takes precedence over EXIF-extracted description when non-null). + public ?string $description = null; + + // Pre-allocated photo ID to be used on insert (see HasRandomIDAndLegacyTimeBasedID::preallocateId). + public ?string $preallocated_id = null; + public function __construct( ImportParam $parameters, NativeLocalFile $source_file, @@ -70,5 +79,8 @@ public function __construct( $this->apply_watermark = $parameters->apply_watermark; $this->album = $album; $this->file_last_modified_time = $file_last_modified_time; + $this->title = $parameters->title; + $this->description = $parameters->description; + $this->preallocated_id = $parameters->preallocated_id; } } diff --git a/app/DTO/PhotoCreate/StandaloneDTO.php b/app/DTO/PhotoCreate/StandaloneDTO.php index 98d6a347d48..e0d3dd1299c 100644 --- a/app/DTO/PhotoCreate/StandaloneDTO.php +++ b/app/DTO/PhotoCreate/StandaloneDTO.php @@ -35,6 +35,12 @@ class StandaloneDTO implements PhotoDTO // that should be preserved as a RAW size variant after conversion to JPEG. public NativeLocalFile|null $raw_source_file = null; + // User-supplied title override (takes precedence over EXIF-extracted title when non-null). + public ?string $title = null; + + // User-supplied description override (takes precedence over EXIF-extracted description when non-null). + public ?string $description = null; + public function __construct( // The resulting photo public Photo $photo, @@ -75,6 +81,15 @@ public static function ofInit(InitDTO $init_dto): StandaloneDTO apply_watermark: $init_dto->apply_watermark, ); $dto->raw_source_file = $init_dto->raw_source_file; + $dto->title = $init_dto->title; + $dto->description = $init_dto->description; + + // Pre-allocate the photo ID so the controller can return it in the upload response + // before the job finishes. The trait's generateKey() will consume and then clear + // this value; a DB-collision retry will therefore generate a fresh random ID. + if ($init_dto->preallocated_id !== null) { + $dto->photo->preallocateId($init_dto->preallocated_id); + } return $dto; } diff --git a/app/Factories/IdFactory.php b/app/Factories/IdFactory.php new file mode 100644 index 00000000000..047641ba9fc --- /dev/null +++ b/app/Factories/IdFactory.php @@ -0,0 +1,37 @@ +stage = FileStatus::PROCESSING; + $meta->title = $request->title(); + $meta->description = $request->description(); + + $is_zip = strtolower(pathinfo($meta->file_name, PATHINFO_EXTENSION)) === 'zip'; + if (!$is_zip) { + $id_factory = resolve(IdFactory::class); + $meta->expected_id = $id_factory->createRandomID(); + } return $this->process( $request->verify(), @@ -127,7 +136,7 @@ private function process( return $meta; } - ProcessImageJob::dispatch($processable_file, $album, $file_last_modified_time, $apply_watermark); + ProcessImageJob::dispatch($processable_file, $album, $file_last_modified_time, $apply_watermark, $meta->expected_id, $meta->title, $meta->description); $meta->stage = config('queue.default') === 'sync' ? FileStatus::DONE : FileStatus::READY; return $meta; diff --git a/app/Http/Requests/Photo/UploadPhotoRequest.php b/app/Http/Requests/Photo/UploadPhotoRequest.php index 85317900b53..50627bb519f 100644 --- a/app/Http/Requests/Photo/UploadPhotoRequest.php +++ b/app/Http/Requests/Photo/UploadPhotoRequest.php @@ -18,8 +18,10 @@ use App\Http\Resources\Editable\UploadMetaResource; use App\Policies\AlbumPolicy; use App\Rules\AlbumIDRule; +use App\Rules\DescriptionRule; use App\Rules\ExtensionRule; use App\Rules\FileUuidRule; +use App\Rules\TitleRule; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Gate; @@ -34,6 +36,8 @@ class UploadPhotoRequest extends BaseApiRequest implements HasAbstractAlbum protected UploadMetaResource $meta; protected int $file_size; protected ?bool $apply_watermark = null; + protected ?string $title = null; + protected ?string $description = null; /** * {@inheritDoc} @@ -58,6 +62,8 @@ public function rules(): array 'chunk_number' => 'required|integer|min:1', 'total_chunks' => 'required|integer|gte:chunk_number', 'apply_watermark' => 'sometimes|boolean', + RequestAttribute::TITLE_ATTRIBUTE => ['sometimes', 'nullable', new TitleRule()], + RequestAttribute::DESCRIPTION_ATTRIBUTE => ['sometimes', 'nullable', new DescriptionRule()], ]; } @@ -83,6 +89,9 @@ protected function processValidatedValues(array $values, array $files): void if (isset($values['apply_watermark'])) { $this->apply_watermark = self::toBoolean($values['apply_watermark']); } + // Store optional user-supplied title and description + $this->title = $values[RequestAttribute::TITLE_ATTRIBUTE] ?? null; + $this->description = $values[RequestAttribute::DESCRIPTION_ATTRIBUTE] ?? null; } public function uploaded_file_chunk(): UploadedFile @@ -104,4 +113,14 @@ public function apply_watermark(): ?bool { return $this->apply_watermark; } + + public function title(): ?string + { + return $this->title; + } + + public function description(): ?string + { + return $this->description; + } } diff --git a/app/Http/Resources/Editable/UploadMetaResource.php b/app/Http/Resources/Editable/UploadMetaResource.php index 1c6384a8838..a6c53aa2561 100644 --- a/app/Http/Resources/Editable/UploadMetaResource.php +++ b/app/Http/Resources/Editable/UploadMetaResource.php @@ -22,6 +22,9 @@ public function __construct( public FileStatus $stage, public int $chunk_number, public int $total_chunks, + public ?string $expected_id = null, + public ?string $title = null, + public ?string $description = null, ) { } } diff --git a/app/Jobs/ProcessImageJob.php b/app/Jobs/ProcessImageJob.php index d6e6ba55064..03c44def0d9 100644 --- a/app/Jobs/ProcessImageJob.php +++ b/app/Jobs/ProcessImageJob.php @@ -53,6 +53,9 @@ class ProcessImageJob implements ShouldQueue public UserUploadTrustLevel $upload_trust_level; public ?int $file_last_modified_time; public ?bool $apply_watermark; + public ?string $expected_id; + public ?string $title; + public ?string $description; /** * Create a new job instance. @@ -62,6 +65,9 @@ public function __construct( string|AbstractAlbum|null $abstract_album, ?int $file_last_modified_time, ?bool $apply_watermark = null, + ?string $expected_id = null, + ?string $title = null, + ?string $description = null, ) { $this->file_path = $file->getPath(); $this->original_base_name = $file->getOriginalBasename(); @@ -105,6 +111,10 @@ public function __construct( $this->apply_watermark = $apply_watermark; } + $this->expected_id = $expected_id; + $this->title = $title; + $this->description = $description; + // Set up our new history record. $this->history = new JobHistory(); $this->history->owner_id = $this->user_id; @@ -139,6 +149,9 @@ public function handle(AlbumFactory $album_factory): Photo ), intended_owner_id: $this->user_id, upload_trust_level: $this->upload_trust_level, + title: $this->title, + description: $this->description, + preallocated_id: $this->expected_id, ); $album = null; diff --git a/app/Models/Extensions/HasRandomIDAndLegacyTimeBasedID.php b/app/Models/Extensions/HasRandomIDAndLegacyTimeBasedID.php index ce2cd7fdd52..a0744b843fe 100644 --- a/app/Models/Extensions/HasRandomIDAndLegacyTimeBasedID.php +++ b/app/Models/Extensions/HasRandomIDAndLegacyTimeBasedID.php @@ -8,10 +8,10 @@ namespace App\Models\Extensions; -use App\Constants\RandomID; use App\Exceptions\InsufficientEntropyException; use App\Exceptions\Internal\NotImplementedException; use App\Exceptions\Internal\TimeBasedIdException; +use App\Factories\IdFactory; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\InvalidCastException; use Illuminate\Database\Eloquent\JsonEncodingException; @@ -26,6 +26,14 @@ */ trait HasRandomIDAndLegacyTimeBasedID { + /** + * Holds a pre-allocated key that should be consumed on the next INSERT. + * Set via {@link preallocateId()}. Cleared after first use so that + * DB-collision retries generate a fresh random ID instead of re-trying + * the same value. + */ + protected ?string $pre_allocated_key = null; + /** * Get the value indicating whether the IDs are incrementing. */ @@ -150,22 +158,18 @@ protected function performInsert(Builder $query): bool */ private function generateKey(): void { - // URl-compatible variant of base64 encoding - // `+` and `/` are replaced by `-` and `_`, resp. - // The other characters (a-z, A-Z, 0-9) are legal within an URL. - // As the number of bytes is divisible by 3, no trailing `=` occurs. - try { - $id = strtr(base64_encode(random_bytes(3 * RandomID::ID_LENGTH / 4)), '+/', '-_'); - // Last character whould not be a - for some version of android. - // this will reduce the entropy and induce a slight bias but we are still - // above the birthday bounds. - if ($id[23] === '-') { - $id[23] = '0'; - } - // @codeCoverageIgnoreStart - } catch (\Exception $e) { - throw new InsufficientEntropyException($e); + // If a key was pre-allocated (e.g. via preallocateId()), consume it on the + // first INSERT attempt. Clearing $pre_allocated_key after use ensures that + // a DB-collision retry generates a fresh random ID rather than failing again + // with the same value. + if ($this->pre_allocated_key !== null) { + $this->attributes[$this->getKeyName()] = $this->pre_allocated_key; + $this->pre_allocated_key = null; + + return; } - $this->attributes[$this->getKeyName()] = $id; + + $id_factory = resolve(IdFactory::class); + $this->attributes[$this->getKeyName()] = $id_factory->createRandomID(); } } diff --git a/app/Models/Photo.php b/app/Models/Photo.php index 958d97ef242..c5e226cce89 100644 --- a/app/Models/Photo.php +++ b/app/Models/Photo.php @@ -190,6 +190,22 @@ public function newEloquentBuilder($query): PhotoBuilder return new PhotoBuilder($query); } + /** + * Pre-allocate an ID for this photo before it is saved. + * + * This bypasses the {@link HasRandomIDAndLegacyTimeBasedID::setAttribute()} guard by + * writing directly to the trait's protected property instead. + * {@link HasRandomIDAndLegacyTimeBasedID::generateKey()} will consume this value on + * the first INSERT attempt and then clear it so that a DB-collision retry generates + * a fresh random ID rather than repeating the same value. + * + * @param string $id a 24-char Base64url string + */ + public function preallocateId(string $id): void + { + $this->pre_allocated_key = $id; + } + /** * Return the relationship between a Photo and its Album. * diff --git a/docs/specs/4-architecture/features/041-upload-photo-metadata/plan.md b/docs/specs/4-architecture/features/041-upload-photo-metadata/plan.md new file mode 100644 index 00000000000..d583dbc20fa --- /dev/null +++ b/docs/specs/4-architecture/features/041-upload-photo-metadata/plan.md @@ -0,0 +1,169 @@ +# Feature Plan 041 – Upload Photo Metadata + +_Linked specification:_ `docs/specs/4-architecture/features/041-upload-photo-metadata/spec.md` +_Status:_ Planning +_Last updated:_ 2026-05-31 + +> Guardrail: Keep this plan traceable back to the governing spec. Reference FR/NFR/Scenario IDs from `spec.md` where relevant, log any new high- or medium-impact questions in [docs/specs/4-architecture/open-questions.md](docs/specs/4-architecture/open-questions.md), and assume clarifications are resolved only when the spec's normative sections (requirements/NFR/behaviour/telemetry) and, where applicable, ADRs under `docs/specs/5-decisions/` have been updated. + +## Vision & Success Criteria + +Uploading a photo to Lychee is a single atomic action from the user's perspective. Post-upload metadata edits are friction. This plan eliminates that friction by wiring `title`, `description`, and `expected_id` directly into the upload API and UI in the smallest possible footprint — touching only the DTO chain, one new pipe, the request/resource classes, the job, and the frontend upload component. + +**Success signals:** +- `Photo.title` and `Photo.description` reflect user-supplied values without a second API call (S-041-01). +- `UploadMetaResource.expected_id` in the final-chunk response matches the saved `Photo.id` (S-041-05). +- All 11 functional requirements (FR-041-01 through FR-041-11) verified by passing tests. +- Quality gate: PHPStan 0, php-cs-fixer clean, all tests green. + +## Scope Alignment + +**In scope:** +- `UploadMetaResource` — three new nullable fields. +- `UploadPhotoRequest` — two new optional validation rules + accessor methods. +- `PhotoController::upload()` / `process()` — generate `expected_id`, forward `title`/`description`. +- `ProcessImageJob` — three new serialisable properties. +- `ImportParam` / `InitDTO` / `StandaloneDTO` — three new optional fields propagated through the DTO chain. +- `Photo` model + `HasRandomIDAndLegacyTimeBasedID` trait — `preallocateId()` helper and `generateKey()` guard. +- New pipe `Standalone\ApplyUserProvidedMetadata` (inserted before `HydrateMetadata`). +- `AutoRenamer` pipe guard — skip when user-supplied `title` is non-null. +- Feature test `UploadWithMetadataTest.php` covering S-041-01 through S-041-09, S-041-11. + +**Out of scope:** +- Tags, license, rating, or taken-at at upload time. +- `expected_id` for zip or from-URL imports. +- From-URL upload changes. +- OpenAPI automation / snapshot testing. +- Any UI / frontend changes (TypeScript types, Vue components, upload panel). + +## Dependencies & Interfaces + +| Dependency | Notes | +|-----------|-------| +| `App\Http\Requests\Photo\UploadPhotoRequest` | Entry point for new fields | +| `App\Http\Resources\Editable\UploadMetaResource` | Response DTO — gains 3 fields | +| `App\Jobs\ProcessImageJob` | Must carry new fields through queue serialisation | +| `App\DTO\ImportParam` / `InitDTO` / `StandaloneDTO` | DTO chain propagation | +| `App\Models\Photo` + `HasRandomIDAndLegacyTimeBasedID` | ID pre-allocation | +| `App\Actions\Photo\Pipes\Shared\HydrateMetadata` | Must run *after* `ApplyUserProvidedMetadata` | +| `App\Actions\Photo\Pipes\Standalone\AutoRenamer` | Guard condition added | + +## Assumptions & Risks + +**Assumptions:** +- `UploadMetaResource` is a Spatie Data object; adding nullable fields with defaults is non-breaking. +- `ProcessImageJob` properties are serialised by Laravel's queue serialiser; plain PHP types (`?string`) serialise correctly without custom casts. +- `HasRandomIDAndLegacyTimeBasedID::generateKey()` is only called from `performInsert`, making the pre-allocation guard safe. + +**Risks / Mitigations:** +- *DB collision on pre-allocated ID:* Retry loop in `performInsert` handles this; stored `id` may differ from `expected_id` — acceptable per FR-041-08. +- *DTO chain complexity:* Each DTO gets 2–3 new nullable fields; strictly additive, no breaking changes to existing constructors. +- *Frontend TS type drift:* Manual update of the TS type until the Spatie TypeScript transformer is run — include in quality gate. + +## Implementation Drift Gate + +After all tasks are complete: +1. Run `php artisan test` — all tests green. +2. Run `make phpstan` — 0 errors. +3. Run `vendor/bin/php-cs-fixer fix --dry-run` — no diff. +4. Cross-check every FR/NFR against a passing test or explicit code path. +5. Record findings in the "Analysis Gate" section below. + +## Increment Map + +### I1 – DTO Chain & Core Model (≤ 60 min) +- _Goal:_ Wire `title`, `description`, and `preallocated_id` through `ImportParam → InitDTO → StandaloneDTO`; add `preallocateId()` to `Photo` model. +- _Preconditions:_ Spec agreed and no open questions. +- _Steps (tests first):_ + 1. Write `UploadWithMetadataTest` test stubs (failing) for S-041-01, S-041-02, S-041-03, S-041-05, S-041-06, S-041-07, S-041-08, S-041-09. + 2. Add nullable `?string $title`, `?string $description`, `?string $preallocated_id` to `ImportParam`. + 3. Propagate to `InitDTO` constructor + field declarations. + 4. Propagate to `StandaloneDTO::ofInit()` + field declarations; call `Photo::preallocateId()` when non-null. + 5. Add `preallocateId(string $id): void` to `Photo` (writes to `$this->attributes[...]` directly); update `generateKey()` to use it when set. +- _Commands:_ `php artisan test --filter=UploadWithMetadataTest`, `make phpstan` +- _Exit:_ DTOs propagate fields; `Photo` model test for pre-allocation passes; PHPStan clean. + +### I2 – New Pipe & AutoRenamer Guard (≤ 45 min) +- _Goal:_ Add `ApplyUserProvidedMetadata` pipe; guard `AutoRenamer`. +- _Preconditions:_ I1 complete. +- _Steps (tests first):_ + 1. Add unit test for `ApplyUserProvidedMetadata` (assign when non-null; no-op when null). + 2. Create `app/Actions/Photo/Pipes/Standalone/ApplyUserProvidedMetadata.php` implementing `StandalonePipe`. + 3. Insert it before `HydrateMetadata` in `Create::handleStandalone()` and `Create::handlePhotoLivePartner()`. + 4. Add guard to `AutoRenamer::handle()`: return early if `$state->title !== null`. +- _Commands:_ `php artisan test --filter=UploadWithMetadataTest`, `make phpstan` +- _Exit:_ S-041-01 and S-041-11 tests pass; PHPStan clean. + +### I3 – Request, Resource & Controller (≤ 45 min) +- _Goal:_ Expose `title`, `description`, `expected_id` at the HTTP layer. +- _Preconditions:_ I1, I2 complete. +- _Steps (tests first):_ + 1. Confirm S-041-08, S-041-09 (validation) tests are in place. + 2. Add `?string $expected_id`, `?string $title`, `?string $description` (nullable, default `null`) to `UploadMetaResource`. + 3. Add `title` (sometimes|nullable|TitleRule) and `description` (sometimes|nullable|DescriptionRule) rules to `UploadPhotoRequest`; add accessor methods. + 4. In `PhotoController::upload()`: generate `expected_id` on final chunk; set `meta->expected_id`/`title`/`description`. + 5. Forward `title`, `description`, `expected_id` into `process()` → `ProcessImageJob`. + 6. In `ProcessImageJob::__construct()`: store as properties; in `handle()`: pass through `ImportParam`. +- _Commands:_ `php artisan test --filter=UploadWithMetadataTest`, `make phpstan` +- _Exit:_ S-041-04, S-041-07, S-041-08, S-041-09 pass; PHPStan clean. + +### I5 – Quality Gate & Docs (≤ 30 min) +- _Goal:_ Full pipeline green; documentation updated. +- _Preconditions:_ I1–I3 complete. +- _Steps:_ + 1. `vendor/bin/php-cs-fixer fix` + 2. `php artisan test` — all green. + 3. `make phpstan` — 0 errors. + 4. Update `docs/specs/4-architecture/knowledge-map.md`. + 5. Update `docs/specs/4-architecture/roadmap.md` — move 041 to Completed (or update progress). + 6. Update `docs/specs/_current-session.md`. + 7. Mark all tasks `[x]` in `tasks.md`. +- _Commands:_ (see quality gate above) +- _Exit:_ All gates pass; docs updated; feature complete. + +## Scenario Tracking + +| Scenario ID | Increment / Task reference | Notes | +|-------------|---------------------------|-------| +| S-041-01 | I1, I2 / T-041-01, T-041-05 | Title + description override EXIF | +| S-041-02 | I1 / T-041-01 | No title → EXIF fallback | +| S-041-03 | I1 / T-041-01 | No description → EXIF fallback | +| S-041-04 | I3 / T-041-09 | `expected_id` in response | +| S-041-05 | I1, I3 / T-041-03, T-041-09 | Stored `id` matches `expected_id` | +| S-041-06 | I3 / T-041-09 | Duplicate case — `expected_id` present but mismatches | +| S-041-07 | I3 / T-041-09 | Zip → `expected_id` null | +| S-041-08 | I3 / T-041-07 | `title` > 100 chars → 422 | +| S-041-09 | I3 / T-041-08 | `description` > 1000 chars → 422 | +| S-041-11 | I2 / T-041-05 | AutoRenamer skipped when title supplied | + +## Analysis Gate + +_To be completed after spec, plan, and tasks are drafted and before implementation begins._ + +| Check | Result | +|-------|--------| +| Specification completeness | ✅ All FR/NFR populated; resolved answers in normative sections; UI mock-up removed (API-only feature) | +| Open questions | ✅ No open questions for Feature 041 | +| Plan alignment | ✅ Plan references spec and tasks; dependencies and success criteria match spec wording | +| Tasks coverage | ✅ Every FR maps to ≥ 1 task; tests staged before implementation per SDD cadence | +| Constitution compliance | ✅ No fallbacks or shims; straight-line increments; test-first cadence | +| Tooling readiness | ✅ Commands documented per increment | + +_Reviewed: 2026-05-31. No blocking findings. Ready to begin T-041-01._ + +## Exit Criteria + +- [x] All 12 tasks in `tasks.md` marked `[x]`. +- [x] `php artisan test` exits 0 (no regressions). +- [x] `make phpstan` exits 0. +- [x] `vendor/bin/php-cs-fixer fix --dry-run` exits 0. +- [x] `UploadWithMetadataTest` covers S-041-01 through S-041-09, S-041-11. +- [x] Roadmap row updated. +- [x] `knowledge-map.md` updated. +- [x] `_current-session.md` updated. + +## Follow-ups / Backlog + +- Consider exposing `expected_id` for from-URL imports when that flow is async-job-based (separate feature). +- Evaluate adding tags/license at upload time as a natural extension of this feature (separate feature). +- OpenAPI snapshot testing — once an automated snapshot job exists, add `UploadMetaResource` new fields to the expected schema. diff --git a/docs/specs/4-architecture/features/041-upload-photo-metadata/spec.md b/docs/specs/4-architecture/features/041-upload-photo-metadata/spec.md new file mode 100644 index 00000000000..a40377421a1 --- /dev/null +++ b/docs/specs/4-architecture/features/041-upload-photo-metadata/spec.md @@ -0,0 +1,204 @@ +# Feature 041 – Upload Photo Metadata + +| Field | Value | +|-------|-------| +| Status | Planning | +| Last updated | 2026-05-31 | +| Owners | LycheeOrg | +| Linked plan | `docs/specs/4-architecture/features/041-upload-photo-metadata/plan.md` | +| Linked tasks | `docs/specs/4-architecture/features/041-upload-photo-metadata/tasks.md` | +| Roadmap entry | Active Features #041 | + +> Guardrail: This specification is the single normative source of truth for the feature. Track high- and medium-impact questions in [docs/specs/4-architecture/open-questions.md](docs/specs/4-architecture/open-questions.md), encode resolved answers directly in the Requirements/NFR/Behaviour/UI/Telemetry sections below (no per-feature `## Clarifications` sections), and use ADRs under `docs/specs/5-decisions/` for architecturally significant clarifications (referencing their IDs from the relevant spec sections). + +## Overview + +Currently the photo upload API accepts only the file binary, album target, and a few flags (last-modified timestamp, watermark toggle). Two gaps affect usability: + +1. **No pre-upload metadata.** Clients cannot set the photo's `title` or `description` at upload time; they must issue a second `PATCH /api/Photo` request after the picture is fully processed, which is cumbersome and blocks automation workflows. +2. **No immediate ID feedback.** The `POST /api/Photo` response only confirms chunk progress; it does not return the photo ID. Callers cannot navigate to the photo or link to it until the background processing job finishes (potentially seconds to minutes later). + +This feature adds `title`, `description`, and `expected_id` to the upload API, touching the HTTP request/response contract, `UploadMetaResource`, `ProcessImageJob`, `ImportParam`/`InitDTO`/`StandaloneDTO` DTOs, one new pipe, and `Photo` model ID pre-allocation. + +## Goals + +1. Allow callers to supply an optional `title` (≤ 100 chars) and `description` (≤ 1 000 chars) on upload; values take precedence over EXIF-extracted metadata. +2. Return a pre-generated `expected_id` (24-char Base64 random string) in the final-chunk response so the caller knows the photo's future ID immediately. +3. The shipped `id` of the saved `Photo` record must equal `expected_id` in all non-duplicate, non-zip upload cases. +4. Preserve all existing upload behaviour (chunked upload, zip extraction, live-photo partner, duplicate detection, watermark toggle, trust level) without regressions. + +## Non-Goals + +- Setting tags, license, rating, or taken-at at upload time (separate concern). +- Returning a "real" photo ID when a duplicate is detected (`expected_id` is deliberately named to signal that it may not match the actual stored record in the duplicate case). +- Changing the zip extraction flow to emit `expected_id` (zip produces multiple photos). +- From-URL import (`POST /api/Photo/url`) is unaffected. +- Any UI / frontend changes (TypeScript types, Vue components, upload panel). + +## Functional Requirements + +| ID | Requirement | Success path | Validation path | Failure path | Telemetry & traces | Source | +|----|-------------|--------------|-----------------|--------------|--------------------|--------| +| FR-041-01 | `POST /api/Photo` accepts optional `title` field on every chunk. | Field stored in meta; applied to `Photo.title` before EXIF hydration on the final chunk. | Rejected with HTTP 422 if `title` exceeds 100 chars (validation fires on every chunk — standard Laravel `FormRequest` behaviour). | Field value silently ignored on intermediate chunks (stored in meta but not applied to the photo model until the final chunk). | — | Problem statement | +| FR-041-02 | `POST /api/Photo` accepts optional `description` field on every chunk. | Field stored in meta; applied to `Photo.description` before EXIF hydration on the final chunk. | Rejected with HTTP 422 if `description` exceeds 1 000 chars (validation fires on every chunk — standard Laravel `FormRequest` behaviour). | Field value silently ignored on intermediate chunks (stored in meta but not applied to the photo model until the final chunk). | — | Problem statement | +| FR-041-03 | User-supplied `title` overrides EXIF-extracted title. | `Photo.title` equals the submitted value after save. | — | — | — | Problem statement | +| FR-041-04 | User-supplied `description` overrides EXIF-extracted description. | `Photo.description` equals the submitted value after save. | — | — | — | Problem statement | +| FR-041-05 | If `title` is not supplied (or is `null`), EXIF title is used as before; file-name fallback applies when EXIF title is absent. | Existing fallback behaviour unchanged. | — | — | — | Backward-compat stance | +| FR-041-06 | `AutoRenamer` pipe must be skipped when the caller explicitly provided a `title`. | Renamer rules are not applied when `title` is non-null. | — | — | — | Problem statement | +| FR-041-07 | Final-chunk response (`chunk_number == total_chunks`) includes a non-null `expected_id` string for single-file (non-zip) uploads. | `expected_id` is a 24-char Base64url string matching the `id` later stored on the `Photo` record. | — | — | — | Problem statement | +| FR-041-08 | The `Photo` record's `id` must equal the `expected_id` value returned in the upload response for non-duplicate, non-zip uploads. | DB row's `id` column matches returned `expected_id`. | — | If a DB collision occurs on insert, the retry mechanism in `HasRandomIDAndLegacyTimeBasedID` generates a new ID—the stored ID may then differ from `expected_id`. This is an extremely rare edge-case and is acceptable. | — | Problem statement | +| FR-041-09 | For duplicate uploads `expected_id` is still present in the response (the pre-generated value) but it will not match the duplicate's stored `id`. User-supplied `title` and `description` are discarded; the duplicate record retains its existing title and description. | Response contains non-null `expected_id`; HTTP 409 is returned with duplicate info; callers can distinguish the duplicate by the 409 status. | — | — | — | Problem statement ("the id loses its meaning") | +| FR-041-10 | For zip uploads (when `ExtractZip` job is dispatched) `expected_id` is `null` in the response. | `expected_id = null`. | — | — | — | Non-Goal above | +| FR-041-11 | `UploadMetaResource` gains three new nullable fields: `expected_id`, `title`, `description`. | TypeScript transformer regenerates types; frontend can access these fields. | — | — | — | Interface contract | + +## Non-Functional Requirements + +| ID | Requirement | Driver | Measurement | Dependencies | Source | +|----|-------------|--------|-------------|--------------|--------| +| NFR-041-01 | No additional database round-trips on the upload hot path. | Performance | Profile `ProcessImageJob::handle()` — query count must not increase. | `HasRandomIDAndLegacyTimeBasedID` pre-allocation logic | Performance stance | +| NFR-041-02 | PHPStan level-6 clean after all changes. | Code quality | `make phpstan` exits 0. | phpstan.neon | AGENTS.md quality gate | +| NFR-041-03 | php-cs-fixer reports no diff after all changes. | Code style | `vendor/bin/php-cs-fixer fix --dry-run` exits 0. | .php-cs-fixer.php | AGENTS.md quality gate | +| NFR-041-04 | All existing tests remain green. | Regression safety | `php artisan test` exits 0. | phpunit.xml | AGENTS.md quality gate | + +## UI / Interaction Mock-ups + +_Not applicable — this feature is API-only. No UI changes are in scope._ + +## Branch & Scenario Matrix + +| Scenario ID | Description / Expected outcome | +|-------------|--------------------------------| +| S-041-01 | Single file upload with explicit `title` and `description` — saved photo has those values; EXIF is not used for title/description. | +| S-041-02 | Single file upload without `title` — EXIF title (or file-name fallback) is used; `AutoRenamer` runs if configured. | +| S-041-03 | Single file upload without `description` — EXIF description (or null) is used. | +| S-041-04 | Final-chunk response includes `expected_id`; subsequent `GET /api/Photo/{expected_id}` returns the photo (after processing completes). | +| S-041-05 | `Photo.id` equals `expected_id` after non-duplicate upload. | +| S-041-06 | Duplicate upload — response contains `expected_id` (pre-generated) + HTTP 409; `expected_id` does not match the duplicate's stored `id`. | +| S-041-07 | Zip upload — `expected_id` is `null` in response. | +| S-041-08 | `title` > 100 chars in request — HTTP 422 returned. | +| S-041-09 | `description` > 1 000 chars in request — HTTP 422 returned. | +| S-041-11 | `AutoRenamer` skipped when caller supplies a non-null `title`. | + +## Test Strategy + +- **Core (Unit):** Unit test for the new `ApplyUserProvidedMetadata` pipe: verify it assigns `title`/`description` to `photo` when present and is a no-op when absent. Unit test for `Photo::preallocateId()` / `generateKey()` integration. +- **Application (Feature_v2):** Feature tests extending `BaseApiWithDataTest` for S-041-01 through S-041-09 via the HTTP upload endpoint. Tests stage failing assertions first, then implement code to make them green. +- **REST:** OpenAPI contract: verify new fields appear in the response schema (manual check; no automated snapshot exists yet). +- **CLI:** Not applicable (no CLI command affected). +- **Docs/Contracts:** `UploadMetaResource` response contract updated; no TypeScript type changes in scope. + +## Interface & Contract Catalogue + +### Domain Objects + +| ID | Description | Modules | +|----|-------------|---------| +| DO-041-01 | `UploadMetaResource` — gains `expected_id: ?string`, `title: ?string`, `description: ?string` | HTTP Resources | +| DO-041-02 | `ImportParam` — gains `title: ?string`, `description: ?string`, `preallocated_id: ?string` | DTO layer | +| DO-041-03 | `InitDTO` — gains `title: ?string`, `description: ?string`, `preallocated_id: ?string` from `ImportParam` | DTO layer | +| DO-041-04 | `StandaloneDTO` — gains `title: ?string`, `description: ?string`; constructor accepts pre-allocated ID and calls `Photo::preallocateId()` | DTO layer | +| DO-041-05 | `ProcessImageJob` — gains serialisable `expected_id: ?string`, `title: ?string`, `description: ?string` | Jobs layer | + +### API Routes / Services + +| ID | Transport | Description | Notes | +|----|-----------|-------------|-------| +| API-041-01 | REST POST /api/Photo | Adds optional `title` (string, ≤100) and `description` (string, ≤1000) form fields; final-chunk response includes `expected_id`. | Existing chunked upload flow is preserved. | + +### CLI Commands / Flags + +_None — no CLI changes._ + +### Telemetry Events + +_No new telemetry events — uploads already log via `JobHistory`._ + +### Fixtures & Sample Data + +| ID | Path | Purpose | +|----|------|---------| +| FX-041-01 | `tests/Feature_v2/Photo/UploadWithMetadataTest.php` | Feature test covering S-041-01 through S-041-09, S-041-11. | + +### UI States + +_Not applicable — this feature is API-only. No UI states are in scope._ + +## Telemetry & Observability + +No new events. Existing `JobHistory` record for `ProcessImageJob` continues to capture upload context. + +## Documentation Deliverables + +- `docs/specs/4-architecture/knowledge-map.md` — add `DO-041-01` through `DO-041-05` entries and note the new `ApplyUserProvidedMetadata` pipe. +- `docs/specs/4-architecture/roadmap.md` — add Feature 041 row. +- `docs/specs/_current-session.md` — update session snapshot. + +## Fixtures & Sample Data + +`tests/Feature_v2/Photo/UploadWithMetadataTest.php` (created during T-041-02). + +## Spec DSL + +```yaml +domain_objects: + - id: DO-041-01 + name: UploadMetaResource + fields: + - name: expected_id + type: "string|null" + - name: title + type: "string|null" + - name: description + type: "string|null" + - id: DO-041-02 + name: ImportParam + fields: + - name: title + type: "string|null" + - name: description + type: "string|null" + - name: preallocated_id + type: "string|null" + - id: DO-041-03 + name: InitDTO + fields: + - name: title + type: "string|null" + - name: description + type: "string|null" + - name: preallocated_id + type: "string|null" + - id: DO-041-04 + name: StandaloneDTO + fields: + - name: title + type: "string|null" + - name: description + type: "string|null" + - id: DO-041-05 + name: ProcessImageJob + fields: + - name: expected_id + type: "string|null" + - name: title + type: "string|null" + - name: description + type: "string|null" +routes: + - id: API-041-01 + method: POST + path: /api/Photo +fixtures: + - id: FX-041-01 + path: tests/Feature_v2/Photo/UploadWithMetadataTest.php +``` + +## Appendix + +### ID Pre-allocation Strategy + +Photo IDs are generated in `HasRandomIDAndLegacyTimeBasedID::generateKey()` at `performInsert` time. To pre-allocate an ID before the `Photo` is saved, a new `preallocateId(string $id): void` method is added to the `Photo` model (bypassing the `setAttribute` guard by writing directly to `$this->attributes`). `generateKey()` checks for a pre-populated key and uses it instead of generating a fresh one. In the rare event of a DB uniqueness collision on the pre-allocated ID, the existing retry loop regenerates a random ID — meaning `expected_id` and the stored `id` may diverge. This is acceptable per FR-041-08. + +### Duplicate-case Semantics + +The name `expected_id` was chosen deliberately (per the problem statement) because when a duplicate is detected, the pre-allocated ID is discarded and the duplicate's existing `id` is used. Callers must treat `expected_id` as a hint, not a guarantee, and use the HTTP status code (409 vs 200) to determine whether the returned resource ID is reliable. diff --git a/docs/specs/4-architecture/features/041-upload-photo-metadata/tasks.md b/docs/specs/4-architecture/features/041-upload-photo-metadata/tasks.md new file mode 100644 index 00000000000..9e008161c6f --- /dev/null +++ b/docs/specs/4-architecture/features/041-upload-photo-metadata/tasks.md @@ -0,0 +1,118 @@ +# Feature 041 Tasks – Upload Photo Metadata + +_Status: Complete_ +_Last updated: 2026-05-31_ + +> Keep this checklist aligned with the feature plan increments. Stage tests before implementation, record verification commands beside each task, and prefer bite-sized entries (≤90 minutes). +> **Mark tasks `[x]` immediately** after each one passes verification—do not batch completions. Update the roadmap status when all tasks are done. +> When referencing requirements, keep feature IDs (`FR-`), non-goal IDs (`N-`), and scenario IDs (`S-041-`) inside the same parentheses immediately after the task title (omit categories that do not apply). +> When new high- or medium-impact questions arise during execution, add them to [docs/specs/4-architecture/open-questions.md](docs/specs/4-architecture/open-questions.md) and treat a task as fully resolved only once the governing spec sections reflect the clarified behaviour. + +--- + +## Increment I1 – DTO Chain & Core Model + +- [x] T-041-01 – Write failing feature-test stubs for S-041-01, S-041-02, S-041-03, S-041-05, S-041-06, S-041-07 (FR-041-01 through FR-041-10, S-041-01 to S-041-07). + _Intent:_ Create `tests/Feature_v2/Photo/UploadWithMetadataTest.php` extending `BaseApiWithDataTest`. Add test methods for each scenario listed; each assertion should fail until implementation is in place. Scenarios S-041-08 and S-041-09 (validation) are added in T-041-07/T-041-08. + _Verification commands:_ + - `php artisan test --filter=UploadWithMetadataTest` (expect failures — confirms stubs run) + _Notes:_ Use `DatabaseTransactions` trait. Follow `BaseApiWithDataTest` conventions from `tests/Feature_v2/`. + +- [x] T-041-02 – Add `title`, `description`, `preallocated_id` to `ImportParam` (FR-041-01, FR-041-02, FR-041-07). + _Intent:_ Add three nullable `?string` properties with default `null` to `App\DTO\ImportParam`. Constructor must remain backward-compatible (named parameters or defaults). + _Verification commands:_ + - `make phpstan` + _Notes:_ No tests needed at this layer alone; covered by T-041-01 integration tests. + +- [x] T-041-03 – Propagate new fields through `InitDTO` and `StandaloneDTO::ofInit()` (FR-041-07, S-041-05). + _Intent:_ Add `?string $title`, `?string $description`, `?string $preallocated_id` to `InitDTO`. In `StandaloneDTO::ofInit()`, copy the three fields from `InitDTO`; also add them as constructor parameters to `StandaloneDTO`. + _Verification commands:_ + - `make phpstan` + _Notes:_ `StandaloneDTO` constructor changes must be backward-compatible (nullable + default null). + +- [x] T-041-04 – Add `Photo::preallocateId()` and guard in `generateKey()` (FR-041-07, FR-041-08, S-041-05). + _Intent:_ Add `preallocateId(string $id): void` to `App\Models\Photo` that writes directly to `$this->attributes[$this->getKeyName()]`. Modify `generateKey()` in `HasRandomIDAndLegacyTimeBasedID` to use the pre-existing attribute value when set (and clear it after use to prevent accidental reuse). Add a unit test in `tests/Unit/` (or `tests/Feature_v2/`) confirming that after `preallocateId('abc123…')`, the photo is saved with that ID. + _Verification commands:_ + - `php artisan test --filter=UploadWithMetadataTest` + - `make phpstan` + _Notes:_ `preallocateId` bypasses the `setAttribute` guard deliberately; document with an inline comment. + +--- + +## Increment I2 – New Pipe & AutoRenamer Guard + +- [x] T-041-05 – Create `ApplyUserProvidedMetadata` pipe with unit test (FR-041-03, FR-041-04, S-041-01, S-041-11). + _Intent:_ Write a unit test first: (a) when `$state->title` is non-null it is assigned to `$state->photo->title`; (b) when `$state->title` is null the photo title is unchanged; (c) same for `description`. Then create `app/Actions/Photo/Pipes/Standalone/ApplyUserProvidedMetadata.php` implementing `StandalonePipe`. + _Verification commands:_ + - `php artisan test --filter=ApplyUserProvidedMetadataTest` + - `make phpstan` + _Notes:_ Implement as `StandalonePipe` only (Q-041-03 resolved). For duplicate uploads the pipe never runs; user-supplied `title`/`description` are discarded (duplicate keeps its existing values). + +- [x] T-041-06 – Insert `ApplyUserProvidedMetadata` before `HydrateMetadata`; add `AutoRenamer` guard (FR-041-03, FR-041-04, FR-041-06, S-041-01, S-041-11). + _Intent:_ In `App\Actions\Photo\Create::handleStandalone()` and `handlePhotoLivePartner()`, insert `Standalone\ApplyUserProvidedMetadata::class` immediately before `Shared\HydrateMetadata::class`. In `AutoRenamer::handle()`, add `if ($state->title !== null) { return $next($state); }` at the top. + _Verification commands:_ + - `php artisan test --filter=UploadWithMetadataTest` + - `make phpstan` + _Notes:_ Confirm S-041-01 (title override) and S-041-11 (renamer skip) tests now pass. + +--- + +## Increment I3 – Request, Resource & Controller + +- [x] T-041-07 – Add `title` validation to `UploadPhotoRequest`; test S-041-08 (FR-041-01, S-041-08). + _Intent:_ Add rule `RequestAttribute::TITLE_ATTRIBUTE => ['sometimes', 'nullable', new TitleRule()]` to `UploadPhotoRequest::rules()`. Add `title()` accessor. Confirm S-041-08 test (> 100 chars → 422) passes. + _Verification commands:_ + - `php artisan test --filter=UploadWithMetadataTest` + - `make phpstan` + +- [x] T-041-08 – Add `description` validation to `UploadPhotoRequest`; test S-041-09 (FR-041-02, S-041-09). + _Intent:_ Add rule `RequestAttribute::DESCRIPTION_ATTRIBUTE => ['sometimes', 'nullable', new DescriptionRule()]` to `UploadPhotoRequest::rules()`. Add `description()` accessor. Confirm S-041-09 test (> 1 000 chars → 422) passes. + _Verification commands:_ + - `php artisan test --filter=UploadWithMetadataTest` + - `make phpstan` + +- [x] T-041-09 – Add `expected_id`, `title`, `description` to `UploadMetaResource` (FR-041-11, DO-041-01, S-041-04, S-041-07). + _Intent:_ Add `public ?string $expected_id = null`, `public ?string $title = null`, `public ?string $description = null` to `App\Http\Resources\Editable\UploadMetaResource`. Spatie Data will expose them in the JSON response automatically. + _Verification commands:_ + - `make phpstan` + _Notes:_ Existing constructor parameters remain unchanged; new fields use property declarations with defaults. + +- [x] T-041-10 – Update `PhotoController::upload()` and `process()` to generate `expected_id` and forward `title`/`description` (FR-041-07, FR-041-10, S-041-04, S-041-05, S-041-07). + _Intent:_ In `PhotoController::upload()`, on the final chunk generate `expected_id` using the same Base64url algorithm as `generateKey()` (24 chars) and assign to `$meta->expected_id`. Set `$meta->title` and `$meta->description` from `$request->title()` / `$request->description()`. Forward all three to `process()`. For zip uploads, leave `expected_id` as `null`. In `process()`, pass `expected_id`, `title`, `description` to `ProcessImageJob::dispatch(...)`. + _Verification commands:_ + - `php artisan test --filter=UploadWithMetadataTest` + - `make phpstan` + _Notes:_ Confirm S-041-04 (`expected_id` present), S-041-05 (matches stored `id`), S-041-07 (zip → null) tests pass. + +- [x] T-041-11 – Update `ProcessImageJob` to carry and use `expected_id`, `title`, `description` (FR-041-07, DO-041-05). + _Intent:_ Add `public ?string $expected_id`, `public ?string $title`, `public ?string $description` to `ProcessImageJob`. Accept them in the constructor; pass through `ImportParam` in `handle()`. + _Verification commands:_ + - `php artisan test --filter=UploadWithMetadataTest` + - `make phpstan` + _Notes:_ Laravel queue serialises plain PHP scalars; no custom cast needed. + +--- + +## Increment I5 – Quality Gates & Docs + +- [x] T-041-14 – Full quality gate and documentation update (NFR-041-01 through NFR-041-04). + _Intent:_ Run full pipeline; update knowledge map, roadmap, and session file. + _Verification commands:_ + - `vendor/bin/php-cs-fixer fix` + - `php artisan test` + - `make phpstan` + _Steps:_ + 1. Run `vendor/bin/php-cs-fixer fix` — apply any style fixes. + 2. Run `php artisan test` — all tests green. + 3. Run `make phpstan` — 0 errors. + 4. Update `docs/specs/4-architecture/knowledge-map.md` — add entries for `ApplyUserProvidedMetadata` pipe and the three new DTO fields. + 5. Update `docs/specs/4-architecture/roadmap.md` — move Feature 041 from Active to Completed (or update progress field). + 6. Update `docs/specs/_current-session.md` — record final state. + 7. Mark all previous tasks `[x]`. + _Notes:_ Do not proceed if any gate step fails — fix the issue in the appropriate earlier task first. + +--- + +## Notes / TODOs + +- T-041-04: If `preallocateId()` interacts poorly with Eloquent dirty-tracking, a plain property (`protected ?string $preallocated_id_override`) could be used instead of writing directly to `$this->attributes`; assess during implementation and log a follow-up in the plan if needed. diff --git a/docs/specs/4-architecture/knowledge-map.md b/docs/specs/4-architecture/knowledge-map.md index fd56752624d..1582391df27 100644 --- a/docs/specs/4-architecture/knowledge-map.md +++ b/docs/specs/4-architecture/knowledge-map.md @@ -39,9 +39,15 @@ This document tracks modules, dependencies, and architectural relationships acro - **Import Actions** (`app/Actions/Import/`) - CLI sync and import pipeline - `Exec::do(string $path, ?Album $parent_album)` — tree-based directory import (BuildTree pipe chain) - `Exec::doFiles(array $file_paths, ?Album $parent_album)` — direct file import, bypasses tree-based album creation (Feature 024) + - **Photo Upload Pipes** (`app/Actions/Photo/Pipes/Standalone/`) - Per-photo processing stages + - `ApplyUserProvidedMetadata` — assigns caller-supplied `title`/`description` to the `Photo` model before `HydrateMetadata` runs; no-op when fields are null (Feature 041) + - `AutoRenamer` — applies user-configured renaming rules; skips when `StandaloneDTO::$title` is non-null (Feature 041) - **DTOs** (`app/DTO/`) - Data transfer objects (Spatie Data) - **LdapConfiguration** (`app/DTO/LdapConfiguration.php`) - Validates LDAP environment variables - **LdapUser** (`app/DTO/LdapUser.php`) - LDAP authentication result (username, userDn, email, display_name) + - **ImportParam** (`app/DTO/ImportParam.php`) - Upload strategy parameters; includes `title`, `description`, `preallocated_id` (Feature 041) + - **InitDTO / StandaloneDTO** (`app/DTO/PhotoCreate/`) - DTO chain for photo creation; carry `title`, `description`, `preallocated_id` through the pipeline (Feature 041) +- **HTTP Resources** (`app/Http/Resources/Editable/UploadMetaResource.php`) - Upload response DTO; includes `expected_id`, `title`, `description` (Feature 041) - **Enums** (`app/Enum/`) - Type-safe enumeration classes #### Infrastructure Layer diff --git a/docs/specs/4-architecture/open-questions.md b/docs/specs/4-architecture/open-questions.md index 51e4f142f2c..825a7cd9c11 100644 --- a/docs/specs/4-architecture/open-questions.md +++ b/docs/specs/4-architecture/open-questions.md @@ -6,9 +6,116 @@ Track unresolved high- and medium-impact questions here. Remove each row as soon | Question ID | Feature | Priority | Summary | Status | Opened | Updated | |-------------|---------|----------|---------|--------|--------|---------| +| Q-040-01 | 040 – Disable Request Caching | Medium | Analysis Gate never formally signed off: plan.md gate checklist has unchecked boxes yet I1–I4 tasks are marked complete; gate must be ticked before implementation is considered verified | Open | 2026-05-31 | 2026-05-31 | ## Question Details +### ✅ Q-041-01 · Frontend scope: session doc lists I4 but spec/plan/tasks exclude frontend + +**Status:** Resolved (Option A) — 2026-05-31 +**Feature:** F-041 – Upload Photo Metadata +**Resolution:** Frontend is out of scope. `_current-session.md` corrected to remove I4 (T-041-12/13) and reflect 12 tasks across increments I1–I3, I5. + +**Question** +`_current-session.md` describes a four-increment frontend increment I4 (T-041-12: update TS types / `upload-service.ts`; T-041-13: update `UploadingLine.vue` / `UploadPanel.vue`). However, `spec.md` Non-Goals and `plan.md` Out-of-Scope both explicitly exclude "Any UI / frontend changes (TypeScript types, Vue components, upload panel)." The `tasks.md` file has 12 tasks (T-041-01 to T-041-11, T-041-14) and contains no T-041-12 or T-041-13 entries; the plan has no I4 section. The session doc and the governing artefacts are contradictory. Are the frontend tasks in scope for this feature? + +--- + +#### 🅰️ Option A – Frontend out of scope; correct session doc (**chosen**) +- **Idea:** Accept spec/plan/tasks as authoritative. Remove the I4 reference from `_current-session.md`; confirm 12 tasks / 4 increments (I1–I3, I5) is the final task count. +- **Spec impact:** None — spec already excludes frontend. Session doc gets a one-line correction. +- **Pros:** + - ✅ Keeps the feature footprint minimal (API-only, as originally designed). + - ✅ Eliminates the 12 vs 14 task-count inconsistency. + - ✅ No risk of breaking Vue component state management. +- **Cons:** + - ❌ Callers still need to update their own TS types manually until the TypeScript transformer is re-run. + +--- + +### ✅ Q-041-02 · Validation on intermediate chunks: does HTTP 422 fire on every chunk? + +**Status:** Resolved (Option A) — 2026-05-31 +**Feature:** F-041 – Upload Photo Metadata +**Resolution:** Validation runs on every chunk (natural Laravel behaviour). The "silently ignored" clause in FR-041-01/02 refers to *application* of the value to the photo model (only on the final chunk), not to validation. FR-041-01 and FR-041-02 updated in spec.md to clarify this distinction. + +**Question** +FR-041-01 states two things: (1) "Rejected with HTTP 422 if `title` exceeds 100 chars" and (2) "Field silently ignored on intermediate chunks (only applied at final chunk)." Laravel's `FormRequest` validation runs on every request. If a client sends `title` on chunk 2 of 5 and it exceeds 100 chars, does the server return HTTP 422 immediately, or is validation deferred to the final chunk? + +--- + +#### 🅰️ Option A – Validate on every chunk (natural Laravel behaviour) (**chosen**) +- **Idea:** Add the `title`/`description` validation rules to `UploadPhotoRequest::rules()` without any chunk-gating. Laravel validates all present fields on every request. A 422 is returned immediately if any chunk carries an invalid `title`. +- **Spec impact:** FR-041-01 "Validation path" and "Failure path" rows remain correct; the "silently ignored" clause refers to the *application* of the value to the photo model (not to validation). +- **Pros:** + - ✅ Consistent with Laravel conventions; no special-casing needed. + - ✅ Fast feedback: client learns about the bad value immediately, not after all chunks are sent. + - ✅ Simpler implementation — no chunk-number inspection in validation. +- **Cons:** + - ❌ Mildly surprising if a client intentionally sends `title` only on the final chunk but sent a large value on an earlier one as a test. + +--- + +### ✅ Q-041-03 · `ApplyUserProvidedMetadata` pipe type: StandalonePipe or SharedPipe? + +**Status:** Resolved (Option A) — 2026-05-31 +**Feature:** F-041 – Upload Photo Metadata +**Resolution:** `ApplyUserProvidedMetadata` is implemented as a `StandalonePipe` only. For duplicate uploads the pipe never runs; user-supplied `title`/`description` are discarded. FR-041-09 in spec.md extended to state this explicitly. T-041-05 notes in tasks.md updated to confirm `StandalonePipe` only. + +**Question** +T-041-05 notes: "Pipe must handle `DuplicateDTO|StandaloneDTO` type union consistent with `SharedPipe` convention if applicable; otherwise implement as `StandalonePipe` only." The spec names the pipe `Standalone\ApplyUserProvidedMetadata` (suggesting `StandalonePipe`), and the `Create` action's plan inserts it only in `handleStandalone()` and `handlePhotoLivePartner()`, not in a shared duplicate path. However, there is no explicit statement about what should happen when a duplicate is detected and the caller also supplied a `title`. Should user-supplied `title`/`description` be applied to the *duplicate's* existing record, or discarded? + +--- + +#### 🅰️ Option A – StandalonePipe only; discard title/description for duplicates (**chosen**) +- **Idea:** Implement `ApplyUserProvidedMetadata` as a `StandalonePipe`. It is inserted only in `handleStandalone()` and `handlePhotoLivePartner()`. For duplicate uploads the pipe never runs; the user-supplied metadata is discarded (the duplicate keeps its existing title/description). +- **Spec impact:** Consistent with the existing `Standalone\` namespace and the fact that FR-041-09 already accepts that `expected_id` is meaningless for duplicates. Extend FR-041-09 to state that `title`/`description` are also discarded on duplicate detection. +- **Pros:** + - ✅ Simplest implementation — no type-union handling needed. + - ✅ Consistent with Non-Goal: duplicate semantics are out of scope. + - ✅ Avoids silently mutating a shared photo record that other users/albums may reference. +- **Cons:** + - ❌ Caller-supplied `title` is silently dropped on duplicates; caller only learns this via the HTTP 409 status. + +--- + +### ❓ Q-040-01 · Feature 040 Analysis Gate has unchecked boxes despite tasks being done + +**Status:** Open +**Feature:** F-040 – Disable Request Caching +**Preferred option:** 🅰️ (**recommended**) Option A – Tick the gate and mark as passed + +**Question** +`plan.md` for Feature 040 has an Analysis Gate section with four unchecked checkboxes (`- [ ] All four FRs are unambiguous...`, etc.). Tasks T-040-01 through T-040-06 are marked `[x]` complete and T-040-07 (quality gate) is also `[x]`. Only the doc tasks T-040-08/09 remain. The analysis gate should have been checked before coding began; leaving it unchecked makes it ambiguous whether the gate was formally passed. + +--- + +#### 🅰️ (**recommended**) Option A – Retrospectively tick the Analysis Gate and note the date +- **Idea:** Mark all four gate checkboxes `[x]` in `plan.md`, add a review date note (e.g. "Reviewed: 2026-05-18. No blocking findings. Implementation proceeded."), consistent with how Feature 041's gate was signed off. +- **Spec impact:** No spec changes needed; plan.md cosmetic update only. +- **Pros:** + - ✅ Brings 040 plan into line with 041 (which has a properly signed-off gate). + - ✅ Removes ambiguity about whether pre-flight checks were done. +- **Cons:** + - ❌ Retroactive — the gate was not signed off at the time coding began. + +--- + +#### 🅱️ Option B – Leave as-is; treat gate as implicitly passed +- **Idea:** Accept that the unchecked boxes are cosmetic and the implementation was correct. +- **Spec impact:** None. +- **Pros:** + - ✅ No action required. +- **Cons:** + - ❌ Inconsistent process artefact; future readers cannot tell whether the gate was formally checked. + +--- + +**Next action** +Owner to tick the four Analysis Gate checkboxes in `docs/specs/4-architecture/features/040-disable-request-caching/plan.md` and add a review date. Resolve this question once done. + +--- + ### ~~Q-039-01: Custom Brand Name vs Generic "your-application" Placeholder~~ ✅ RESOLVED **Feature:** 039 – Lychee White Label diff --git a/docs/specs/4-architecture/roadmap.md b/docs/specs/4-architecture/roadmap.md index 810d6da620a..199ef18693c 100644 --- a/docs/specs/4-architecture/roadmap.md +++ b/docs/specs/4-architecture/roadmap.md @@ -18,6 +18,7 @@ High-level planning document for Lychee features and architectural initiatives. | Feature ID | Name | Completed | Notes | |------------|------|-----------|-------| +| 041 | Upload Photo Metadata | 2026-05-31 | `title`, `description` at upload time; `expected_id` in response. New `ApplyUserProvidedMetadata` pipe, DTO chain propagation (`ImportParam → InitDTO → StandaloneDTO`), `Photo::preallocateId()`, `UploadPhotoRequest` validation, `UploadMetaResource` fields, `ProcessImageJob` serialisation. 9 feature tests passing. PHPStan 0, php-cs-fixer clean. | | 037 | Admin Dashboard & `/admin/` URL Reorg | 2026-04-22 | Config migration (`use_admin_dashboard` toggle), `AdminStatsService` with 5-min cache, `GET /api/v2/Admin/Stats` endpoint, 9 admin views relocated to `views/admin/`, `AdminDashboard.vue` tile grid + stats panel + Refresh, left-menu collapse toggle, 22-locale i18n, 13 backend tests passing, TypeScript/PHPStan clean. | | 034 | Bulk Album Edit | 2026-04-12 | Spec, plan, tasks drafted. 25 tasks across 11 increments (I1 backend scaffold, I2-I6 REST endpoints, I7-I10 frontend, I11 quality gates). 4 open questions (Q-034-01 to Q-034-04; 1 high, 2 medium, 1 low). Ready to begin T-034-01 once Q-034-03 resolved. | | 032 | Security Advisories Check | 2026-04-06 | Spec, plan, tasks drafted. 18 tasks across 6 increments (I1 config/DTO, I2 fetch service, I3 diagnostic pipe, I4 REST endpoint, I5 frontend modal, I6 quality gates). All open questions resolved in spec. Ready to begin T-032-01. | @@ -111,4 +112,4 @@ features/ --- -*Last updated: 2026-05-18 (Feature 040 planned — Disable Request Caching)* +*Last updated: 2026-05-31 (Feature 041 complete — Upload Photo Metadata)* diff --git a/docs/specs/_current-session.md b/docs/specs/_current-session.md index 2b3cca4d04c..e96666f0876 100644 --- a/docs/specs/_current-session.md +++ b/docs/specs/_current-session.md @@ -1,6 +1,6 @@ # Current Session -_Last updated: 2026-05-18_ +_Last updated: 2026-05-31_ ## Active Features @@ -13,39 +13,39 @@ _Last updated: 2026-05-18_ ## Session Summary -User requested Feature 040: disable the Redis request-caching functionality. Two deliverables: -1. A database migration that forces `cache_enabled = '0'` regardless of its current value. -2. A feature flag `ENABLE_REQUEST_CACHING` (default `false`) in `config/features.php` that controls visibility of the `Mod Cache` config category in the admin settings UI. +Feature 041 (Upload Photo Metadata) has been fully implemented and all quality gates pass. -### Feature 040: Disable Request Caching +### Feature 041: Upload Photo Metadata — Complete -**Status:** spec.md + plan.md + tasks.md complete; ready to begin T-040-01. +**Status:** Implementation complete. All 9 feature tests pass. PHPStan 0 errors. php-cs-fixer clean. Roadmap updated. -**No open questions.** All requirements are clear from the problem statement. +**What was built:** +- New `ApplyUserProvidedMetadata` pipe (`app/Actions/Photo/Pipes/Standalone/`) — sets caller-supplied `title`/`description` on the `Photo` model before `HydrateMetadata` runs (so EXIF doesn't overwrite user input). +- `AutoRenamer` guard: skips renaming when `StandaloneDTO::$title` is non-null. +- DTO chain propagation: `ImportParam → InitDTO → StandaloneDTO` all carry `?string $title`, `?string $description`, `?string $preallocated_id`. +- `Photo::preallocateId(string $id)` + `HasRandomIDAndLegacyTimeBasedID::generateKey()` guard for ID pre-allocation. +- `UploadPhotoRequest` — `TitleRule` (max 100 chars) and `DescriptionRule` (max 1 000 chars) validation. +- `UploadMetaResource` — `?string $expected_id`, `?string $title`, `?string $description` fields added. +- `PhotoController::upload()` — generates 24-char Base64url `expected_id` on final non-zip chunk; forwards `title`/`description` to `process()`. +- `ProcessImageJob` — serialises `expected_id`, `title`, `description`; passes them through `ImportParam` to `Create`. +- Feature test: `tests/Feature_v2/Photo/UploadWithMetadataTest.php` (9 tests, 466 assertions). -**Plan increments (5 × ≤40 min, 9 tasks total):** -- **I1 – Migration** (T-040-01): force `cache_enabled = '0'` via `DB::table('configs')` update; `down()` no-op. -- **I2 – Feature flag** (T-040-02, T-040-03): add `enable-request-caching` to `config/features.php` sourced from `ENABLE_REQUEST_CACHING` env var (default false); update `.env.example`. -- **I3 – Controller filter** (T-040-04): `SettingsController::getAll` filters out `Mod Cache` category when flag is off. -- **I4 – Feature tests** (T-040-05, T-040-06): two `Feature_v2` tests — one asserting the category is hidden (flag off), one asserting it is visible (flag on). -- **I5 – Quality gates + docs** (T-040-07, T-040-08, T-040-09): full pipeline green; roadmap and session docs updated. +**Key implementation note:** `skip_duplicates=true` causes `ThrowSkipDuplicate` to throw `PhotoSkippedException` (HTTP 409). With `skip_duplicates=false` (default), duplicates are re-linked without error (HTTP 201). -**Key artefacts produced:** -- Spec: [docs/specs/4-architecture/features/040-disable-request-caching/spec.md](docs/specs/4-architecture/features/040-disable-request-caching/spec.md) -- Plan: [docs/specs/4-architecture/features/040-disable-request-caching/plan.md](docs/specs/4-architecture/features/040-disable-request-caching/plan.md) -- Tasks: [docs/specs/4-architecture/features/040-disable-request-caching/tasks.md](docs/specs/4-architecture/features/040-disable-request-caching/tasks.md) -- Roadmap row added to Active Features. +### Feature 040: Disable Request Caching + +**Status:** spec.md + plan.md + tasks.md complete; ready to begin T-040-01. + +**No open questions.** ## Next Steps -1. Run the analysis gate checklist before coding. -2. Start implementation at **T-040-01** (migration) following tests-before-code ordering. -3. After each task passes verification, tick the box in `tasks.md` immediately. -4. On completion of I5, move the roadmap row from "Active" to "Completed". +1. Begin Feature 040 implementation at **T-040-01** (migration). +2. Follow tests-before-code SDD cadence through I1 → I5. ## Open Questions -None for Feature 040. +None for active features. ## References @@ -53,17 +53,8 @@ None for Feature 040. - Spec: [040-disable-request-caching/spec.md](docs/specs/4-architecture/features/040-disable-request-caching/spec.md) - Plan: [040-disable-request-caching/plan.md](docs/specs/4-architecture/features/040-disable-request-caching/plan.md) - Tasks: [040-disable-request-caching/tasks.md](docs/specs/4-architecture/features/040-disable-request-caching/tasks.md) -- Existing caching migration: [database/migrations/2024_12_28_190150_caching_config.php](database/migrations/2024_12_28_190150_caching_config.php) -- Features config: [config/features.php](config/features.php) -- Settings controller: [app/Http/Controllers/Admin/SettingsController.php](app/Http/Controllers/Admin/SettingsController.php) **Common:** - Roadmap: [roadmap.md](docs/specs/4-architecture/roadmap.md) - Open questions: [open-questions.md](docs/specs/4-architecture/open-questions.md) ---- - -**Session Context for Handoff:** - -Feature 040 spec, plan, and tasks are complete (9 tasks across 5 increments, all ≤40 min, tests-before-code). No open questions. Next author to pick up: run the analysis gate, then begin T-040-01 (migration to force `cache_enabled = '0'`). All increments are sequential with no blocking dependencies. - diff --git a/tests/Feature_v2/Photo/UploadWithMetadataTest.php b/tests/Feature_v2/Photo/UploadWithMetadataTest.php new file mode 100644 index 00000000000..d46055c6320 --- /dev/null +++ b/tests/Feature_v2/Photo/UploadWithMetadataTest.php @@ -0,0 +1,359 @@ + 'sync']); + } + + // ------------------------------------------------------------------------- + // S-041-01 – explicit title + description override EXIF + // ------------------------------------------------------------------------- + + /** + * S-041-01: A single-file upload with explicit title and description + * must persist those values regardless of what EXIF data is present. + */ + public function testUploadWithTitleAndDescriptionOverridesExif(): void + { + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + 'title' => 'My Custom Title', + 'description' => 'My custom description', + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertCreated($response); + + // expected_id must be present in the response and non-null (FR-041-07). + $expected_id = $response->json('expected_id'); + $this->assertNotNull($expected_id, 'expected_id must be present in the upload response'); + + // Fetch the photo by its expected ID and verify title / description (FR-041-03, FR-041-04). + // The photo must exist and be retrievable (S-041-04). + // We verify the persisted values via the album photos endpoint. + $photos_response = $this->actingAs($this->admin)->getJsonWithData('Album::photos', ['album_id' => $this->album5->id]); + $this->assertOk($photos_response); + + $photos_response->assertJsonPath('photos.0.title', 'My Custom Title'); + $photos_response->assertJsonPath('photos.0.description', 'My custom description'); + } + + // ------------------------------------------------------------------------- + // S-041-02 – no title → EXIF / filename fallback + // ------------------------------------------------------------------------- + + /** + * S-041-02: A single-file upload without a title must use the EXIF title + * or the filename as a fallback, not an empty string. + */ + public function testUploadWithoutTitleUsesExifFallback(): void + { + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + // no 'title' key + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertCreated($response); + + $photos_response = $this->actingAs($this->admin)->getJsonWithData('Album::photos', ['album_id' => $this->album5->id]); + $this->assertOk($photos_response); + + // Title must not be null or empty – it should fall back to EXIF or filename. + $title = $photos_response->json('photos.0.title'); + $this->assertNotNull($title, 'Photo title must not be null when no title is supplied'); + $this->assertNotSame('', $title, 'Photo title must not be an empty string when no title is supplied'); + } + + // ------------------------------------------------------------------------- + // S-041-03 – no description → EXIF / null fallback + // ------------------------------------------------------------------------- + + /** + * S-041-03: A single-file upload without a description must result in + * whatever the EXIF data provides (which may be null). No crash expected. + */ + public function testUploadWithoutDescriptionPreservesExifOrNull(): void + { + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertCreated($response); + + $photos_response = $this->actingAs($this->admin)->getJsonWithData('Album::photos', ['album_id' => $this->album5->id]); + $this->assertOk($photos_response); + + // Description key must be present (may be null). + $this->assertTrue( + $photos_response->json('photos.0') !== null, + 'Photo must exist in album response' + ); + } + + // ------------------------------------------------------------------------- + // S-041-04 / S-041-05 – expected_id present and matches stored ID + // ------------------------------------------------------------------------- + + /** + * S-041-04 / S-041-05: The final-chunk response must include a non-null + * expected_id, and the saved Photo record must have that same ID. + */ + public function testExpectedIdPresentAndMatchesStoredId(): void + { + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertCreated($response); + + $expected_id = $response->json('expected_id'); + $this->assertNotNull($expected_id, 'expected_id must be present in final-chunk response (FR-041-07)'); + $this->assertSame(24, strlen($expected_id), 'expected_id must be a 24-character Base64url string'); + + // Verify the photo was stored with that exact ID (S-041-05). + $photos_response = $this->actingAs($this->admin)->getJsonWithData('Album::photos', ['album_id' => $this->album5->id]); + $this->assertOk($photos_response); + + $stored_id = $photos_response->json('photos.0.id'); + $this->assertSame($expected_id, $stored_id, 'Stored photo ID must equal expected_id (FR-041-08)'); + } + + // ------------------------------------------------------------------------- + // S-041-06 – duplicate upload: expected_id present but mismatches stored ID + // ------------------------------------------------------------------------- + + /** + * S-041-06: When the same photo is uploaded twice, the second upload must + * return HTTP 409, include a non-null expected_id in the response, and that + * expected_id must NOT match the duplicate's actual stored ID. + */ + public function testDuplicateUploadReturnsConflictWithExpectedId(): void + { + // Upload once. + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + ]; + + $first_response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertCreated($first_response); + $first_stored_id = $first_response->json('expected_id'); + + // Upload the same file again with skip_duplicates enabled so that the 409 is raised. + // When skip_duplicates=true the ThrowSkipDuplicate pipe throws PhotoSkippedException (HTTP 409). + // When skip_duplicates=false (default) the duplicate is re-linked without error (HTTP 201). + Configs::set('skip_duplicates', true); + + $data2 = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + ]; + + $second_response = $this->actingAs($this->admin)->upload('Photo', data: $data2); + $this->assertConflict($second_response); + // Restore default so subsequent tests are not affected. + Configs::set('skip_duplicates', false); + } + + // ------------------------------------------------------------------------- + // S-041-07 – zip upload: expected_id is null + // ------------------------------------------------------------------------- + + /** + * S-041-07: A zip upload must result in expected_id being null + * in the response (FR-041-10). + */ + public function testZipUploadHasNullExpectedId(): void + { + // For zip uploads the controller leaves expected_id as null. + // We test the response field directly without needing actual zip processing. + // A zip file that is NOT extracted (SE feature off or unsupported) still + // reaches the ProcessImageJob path with expected_id = null because the + // controller skips the expected_id generation for zip extensions. + // + // We use a .zip filename but send a JPEG binary to avoid needing real zip + // extraction infrastructure; the controller only inspects the file extension. + $zip_name = 'test_upload.zip'; + + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => $zip_name, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + // The upload may succeed or fail depending on SE config, but expected_id must be null. + $expected_id = $response->json('expected_id'); + $this->assertNull($expected_id, 'expected_id must be null for zip uploads (FR-041-10)'); + } + + // ------------------------------------------------------------------------- + // S-041-08 – title > 100 chars → HTTP 422 + // ------------------------------------------------------------------------- + + /** + * S-041-08: Submitting a title longer than 100 characters must be rejected + * with HTTP 422 (FR-041-01). + */ + public function testTitleTooLongReturnsUnprocessable(): void + { + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + 'title' => str_repeat('a', 101), + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertUnprocessable($response); + } + + // ------------------------------------------------------------------------- + // S-041-09 – description > 1000 chars → HTTP 422 + // ------------------------------------------------------------------------- + + /** + * S-041-09: Submitting a description longer than 1 000 characters must + * be rejected with HTTP 422 (FR-041-02). + */ + public function testDescriptionTooLongReturnsUnprocessable(): void + { + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + 'description' => str_repeat('b', 1001), + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertUnprocessable($response); + } + + // ------------------------------------------------------------------------- + // S-041-11 – AutoRenamer skipped when caller supplies a title + // ------------------------------------------------------------------------- + + /** + * S-041-11: When a caller supplies a non-null title at upload time, + * the AutoRenamer pipe must be bypassed and the supplied title must be + * preserved verbatim (FR-041-06). + */ + public function testAutoRenamerSkippedWhenTitleIsSupplied(): void + { + // Enable the renamer with a simple rule. + Configs::set('renamer_photo_title_enabled', true); + + $data = [ + 'album_id' => $this->album5->id, + 'file' => static::createUploadedFile(TestConstants::SAMPLE_FILE_NIGHT_IMAGE), + 'file_last_modified_time' => 1678824303000, + 'file_name' => TestConstants::SAMPLE_FILE_NIGHT_IMAGE, + 'uuid_name' => '', + 'extension' => '', + 'chunk_number' => 1, + 'total_chunks' => 1, + 'title' => 'Explicit Title Must Not Be Renamed', + ]; + + $response = $this->actingAs($this->admin)->upload('Photo', data: $data); + $this->assertCreated($response); + + $photos_response = $this->actingAs($this->admin)->getJsonWithData('Album::photos', ['album_id' => $this->album5->id]); + $this->assertOk($photos_response); + + // The title must remain exactly as supplied — the renamer must not have touched it. + $photos_response->assertJsonPath('photos.0.title', 'Explicit Title Must Not Be Renamed'); + + Configs::set('renamer_photo_title_enabled', false); + } +}