Fix/api detail layout#387
Fix/api detail layout#387zigzagdev wants to merge 3 commits intochore/integrate-image_url-into-image-tablefrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the World Heritage query layer to change how list/detail payloads source image/thumbnail data (moving away from thumbnail_image_id/image_url fields toward the images relation), and removes the now-unneeded “get by ids” query method.
Changes:
- Removed
getHeritagesByIds()from the query service interface and implementation. - Updated list payload generation to derive
image_urlfrom relatedimages(attempting to load only the primary image). - Removed the
thumbnail_image_idfield andthumbnail()relation fromWorldHeritage, plus some unused imports.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/Packages/Features/QueryUseCases/QueryServiceInterface/WorldHeritageQueryServiceInterface.php | Removes getHeritagesByIds() from the public query-service contract. |
| src/app/Packages/Domains/WorldHeritageQueryService.php | Updates list/detail payload logic for images; removes getHeritagesByIds() implementation; minor refactors. |
| src/app/Models/WorldHeritage.php | Removes thumbnail_image_id fillable field and thumbnail() relationship. |
| src/app/Models/Image.php | Removes unused SoftDeletes import. |
| src/app/Models/Country.php | Removes unused import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }, | ||
| 'images' => function ($imagesQuery) { | ||
| $imagesQuery->where('is_primary', true)->limit(1); |
There was a problem hiding this comment.
The eager-load constraint ->limit(1) inside with(['images' => ...]) will limit the total number of loaded images across all parent heritages, not 1 per heritage, so most items will end up with no image. Prefer modeling a dedicated primaryImage/thumbnail relationship (e.g., hasOne/ofMany) and eager load that, or drop the limit(1) and select the primary in PHP.
| $imagesQuery->where('is_primary', true)->limit(1); | |
| $imagesQuery->where('is_primary', true); |
| 'longitude' => $heritage->longitude, | ||
| 'short_description' => $heritage->short_description, | ||
| 'thumbnail_id' => $thumbnailModel?->id, | ||
| 'image_url' => $heritage->images->first()?->url, | ||
| 'unesco_site_url' => $heritage->unesco_site_url, |
There was a problem hiding this comment.
buildWorldHeritagePayload() now reads $heritage->images->first(). In searchHeritages(), models come from WorldHeritageReadQueryServiceInterface::findByIdsPreserveOrder() which (in the current codebase) does not eager-load images, so this will trigger an N+1 query (one extra query per heritage) and can significantly slow list/search responses. Consider eager-loading the needed image relation in the read query service (or calling $models->loadMissing(['images' => ...]) before mapping).
| } | ||
|
|
||
| // id: 148 (Old City of Jerusalem) is stored as Unknown | ||
| // but geographically belongs to Asia | ||
| $counts[StudyRegion::ASIA->value] += 1; |
There was a problem hiding this comment.
$counts[StudyRegion::ASIA->value] += 1; is now an unexplained hard-coded adjustment after the explanatory comment was removed. This makes it hard to maintain/verify the correctness of the counts; please restore the rationale (or encode this exception in a dedicated resolver/data source) so future changes don’t accidentally remove or duplicate this correction.
| 'images' => function ($imagesQuery) { | ||
| $imagesQuery->where('is_primary', true)->limit(1); | ||
| }, |
There was a problem hiding this comment.
This PR changes how list thumbnails are sourced (from related images with is_primary instead of the image_url column). There are existing tests for getAllHeritages(), but they don't assert the thumbnail/image behavior; please add/extend tests to verify that each heritage returns the expected primary image (and that multiple heritages each get their own primary image).
| public function getHeritageById( | ||
| int $id | ||
| ): WorldHeritageDto; | ||
|
|
||
| public function getHeritagesByIds( | ||
| array $ids, | ||
| int $currentPage, | ||
| int $perPage | ||
| ): PaginationDto; | ||
|
|
||
| public function searchHeritages( |
There was a problem hiding this comment.
getHeritagesByIds() was removed from this interface, but there is still a caller (GetWorldHeritageByIdsUseCase calls $worldHeritageQueryService->getHeritagesByIds(...)). This will cause a runtime/compile failure when resolving the interface implementation. Either keep the method on the interface/service, or remove/update the use case (and any route/controller wiring) to use an alternative query.
No description provided.