Skip to content

Fix/api detail layout #387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zigzagdev wants to merge 3 commits into
base: chore/integrate-image_url-into-image-table
Choose a base branch
from
Open

Fix/api detail layout #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cd28c8
refactor: remove thumbnail relation and image_url from WorldHeritage …
zigzagdev Mar 28, 2026
d152539
fix: replace image_url/thumbnail with images relation in WorldHeritag…
zigzagdev Mar 28, 2026
41b8ab1
refactor: update WorldHeritageQueryServiceInterface
zigzagdev Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/app/Models/Country.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

namespace App\Models;

use App\Models\WorldHeritage;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsToMany;

Expand Down
1 change: 0 additions & 1 deletion src/app/Models/Image.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;

class Image extends Model
{
Expand Down
5 changes: 0 additions & 5 deletions src/app/Models/WorldHeritage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class WorldHeritage extends Model
'longitude',
'short_description',
'unesco_site_url',
'thumbnail_image_id',
];

protected $hidden = [
Expand All @@ -61,10 +60,6 @@ public function images(): HasMany
->orderBy('sort_order', 'asc');
}

public function thumbnail(): BelongsTo
{
return $this->belongsTo(Image::class, 'thumbnail_image_id');
}
protected function casts(): array
{
return [
Expand Down
145 changes: 10 additions & 135 deletions src/app/Packages/Domains/WorldHeritageQueryService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,11 @@ public function __construct(
private readonly WorldHeritageSearchPort $searchPort,
) {}

/**
* 一覧（最大30件）: サムネのみ、state_party/state_party_code を要件通りに整形
*/
public function getAllHeritages(
int $currentPage,
int $perPage,
string $order
): PaginationDto
{
): PaginationDto {
$items = WorldHeritage::query()
->select([
'world_heritage_sites.id',
Expand All @@ -50,7 +46,7 @@ public function getAllHeritages(
'world_heritage_sites.latitude',
'world_heritage_sites.longitude',
'world_heritage_sites.short_description',
'world_heritage_sites.image_url',
'world_heritage_sites.unesco_site_url',
])
->with([
'countries' => function ($q) {
Expand All @@ -60,7 +56,10 @@ public function getAllHeritages(
'countries.name_jp',
'countries.region',
]);
}
},
'images' => function ($imagesQuery) {
$imagesQuery->where('is_primary', true)->limit(1);
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
$imagesQuery->where('is_primary', true)->limit(1);
$imagesQuery->where('is_primary', true);

Copilot uses AI. Check for mistakes.
},
Comment on lines +60 to +62
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
])
->orderBy('world_heritage_sites.id', $order)
->paginate($perPage, page: $currentPage);
Expand Down Expand Up @@ -107,12 +106,12 @@ public function getHeritageById(int $id): WorldHeritageDto
$imageCollection = new ImageDtoCollection();
$images = ($heritage->images ?? collect())->values();

foreach ($images as $idx => $img) {
foreach ($images as $img) {
$imageCollection->add(new ImageDto(
id: $img->id,
url: $img->url,
sortOrder: $img->sort_order,
isPrimary: $idx === 0,
isPrimary: (bool) $img->is_primary,
));
}

Expand Down Expand Up @@ -189,121 +188,6 @@ public function getHeritageById(int $id): WorldHeritageDto
]);
}

public function getHeritagesByIds(array $ids, int $currentPage, int $perPage): PaginationDto
{
$paginator = $this->model
->select([
'id',
'official_name',
'name',
'name_jp',
'country',
'region',
'study_region',
'category',
'criteria',
'year_inscribed',
'area_hectares',
'buffer_zone_hectares',
'is_endangered',
'latitude',
'longitude',
'short_description',
'unesco_site_url',
'thumbnail_image_id',
])
->with([
'countries' => function ($countriesQuery) {
$countriesQuery->withPivot(['is_primary'])->orderBy('countries.state_party_code', 'asc')->orderBy(
'site_state_parties.inscription_year',
'asc',
);
},
'thumbnail' => function ($thumbnailQuery) {
$thumbnailQuery->select([
'images.id',
'images.world_heritage_id',
'path',
'sort_order',
]);
},
])
->whereIn('id', $ids)
->paginate($perPage, ['*'], 'current_page', $currentPage)
->through(function ($heritage) {
$countries = $heritage->countries ?? collect();

$codes = $countries
->pluck('state_party_code')
->filter()
->map($this->statePartyCodeNormalize(...))
->unique()
->values();

$statePartyName = null;
$statePartyCodes = null;
$stateParties = [];

if ($codes->count() === 1) {
$onlyCode = $codes->first();

$countryModel = $heritage->countries->first(
fn($country) => $this->statePartyCodeNormalize($country->state_party_code) === $onlyCode,
);

$statePartyName = $countryModel?->name;
$statePartyCodes = null;
} elseif ($codes->count() > 1) {
$statePartyName = null;
$statePartyCodes = $codes->all();
}

$statePartiesMeta = [];

foreach ($countries as $country) {
$code = strtoupper($country->state_party_code);
if ($code === '' || $code === '0') {
continue;
}

$statePartiesMeta[$code] = [
'is_primary' => (bool) data_get($country, 'pivot.is_primary', false),
];
}

return [
'id' => $heritage->id,
'official_name' => $heritage->official_name,
'name' => $heritage->name,
'name_jp' => $heritage->name_jp,
'country' => $heritage->country,
'region' => $heritage->study_region,
'category' => $heritage->category,
'criteria' => $heritage->criteria,
'state_party' => $statePartyName,
'state_party_code' => $statePartyCodes,
'year_inscribed' => $heritage->year_inscribed,
'area_hectares' => $heritage->area_hectares,
'buffer_zone_hectares' => $heritage->buffer_zone_hectares,
'is_endangered' => (bool) $heritage->is_endangered,
'latitude' => $heritage->latitude,
'longitude' => $heritage->longitude,
'short_description' => $heritage->short_description,
'unesco_site_url' => $heritage->unesco_site_url,
'state_parties' => $stateParties,
'state_parties_meta' => $statePartiesMeta,
];
});

$paginationArray = $paginator->toArray();
$dtoCollection = $this->buildDtoFromCollection($paginationArray['data']);

return new PaginationDto(
collection: $dtoCollection,
pagination: collect($paginationArray)->except('data')->toArray(),
);
}

public function searchHeritages(AlgoliaSearchListQuery $query): PaginationDto
{
$result = $this->searchPort->search(
Expand Down Expand Up @@ -359,8 +243,6 @@ public function getEachRegionsHeritagesCount(): array
$counts[$region->value] ??= 0;
}

// id: 148 (Old City of Jerusalem) is stored as Unknown
// but geographically belongs to Asia
$counts[StudyRegion::ASIA->value] += 1;
Comment on lines 244 to 246
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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.

Copilot uses AI. Check for mistakes.

return $counts;
Expand All @@ -377,6 +259,7 @@ private function buildWorldHeritagePayload($heritage): array
->unique()
->values();

$statePartyName = null;
$statePartyCodeValue = null;
$statePartyCodeList = [];

Expand All @@ -385,16 +268,11 @@ private function buildWorldHeritagePayload($heritage): array
$primaryCountry = $countryRelations->first(
fn($country) => strtoupper($country->state_party_code) === $onlyStateParty,
);

$statePartyName = $primaryCountry?->name_en ?? $heritage->country ?? null;
} elseif ($statePartyCodeCollection->count() > 1) {
$statePartyName = null;
$statePartyCodeValue = $statePartyCodeCollection->all();
$statePartyCodeList = $statePartyCodeCollection->all();
} else {
$statePartyName = null;
$statePartyCodeValue = null;
$statePartyCodeList = [];
}

$statePartiesMeta = [];
Expand All @@ -409,8 +287,6 @@ private function buildWorldHeritagePayload($heritage): array
];
}

$thumbnailModel = $heritage->thumbnail;

return [
'id' => $heritage->id,
'official_name' => $heritage->official_name,
Expand All @@ -430,11 +306,10 @@ private function buildWorldHeritagePayload($heritage): array
'latitude' => $heritage->latitude,
'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,
Comment on lines 307 to 310
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
'state_parties' => $statePartyCodeList,
'state_parties_meta' => $statePartiesMeta,
'image_url' => $heritage->image_url,
];
}

Expand Down
6 changes: 0 additions & 6 deletions ...kages/Features/QueryUseCases/QueryServiceInterface/WorldHeritageQueryServiceInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,6 @@ public function getHeritageById(
int $id
): WorldHeritageDto;

public function getHeritagesByIds(
array $ids,
int $currentPage,
int $perPage
): PaginationDto;

public function searchHeritages(
AlgoliaSearchListQuery $query
): PaginationDto;
Expand Down
Loading