Feat/regional heritage infra#375
Conversation
There was a problem hiding this comment.
Pull request overview
Adds query-layer support for retrieving per-StudyRegion world heritage site counts, likely to support “regional heritage” views/filters in the Features query use-cases layer.
Changes:
- Extended
WorldHeritageQueryServiceInterfacewithgetEachRegionsHeritagesCount(). - Implemented
getEachRegionsHeritagesCount()inWorldHeritageQueryServiceusing a grouped aggregate query plus normalization over allStudyRegioncases. - Added a domain-level QueryService test covering region count behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/app/Packages/Features/QueryUseCases/QueryServiceInterface/WorldHeritageQueryServiceInterface.php | Adds new query-service API for region counts. |
| src/app/Packages/Domains/WorldHeritageQueryService.php | Implements region-count aggregation logic and normalization. |
| src/app/Packages/Domains/Test/QueryService/WorldHeritageQueryService_countEachRegionTest.php | Introduces test coverage for the new count-by-region behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // 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.
getEachRegionsHeritagesCount() unconditionally increments the Asia count, which will over-count if the dataset doesn’t actually contain the Jerusalem record (or if that record is later fixed to have study_region=ASIA). Consider making this adjustment conditional (e.g., only if id=148 exists with study_region=UNKNOWN), or better, fix the underlying data via a migration/seed so the query can stay purely data-driven.
| $counts[StudyRegion::ASIA->value] += 1; | |
| if ($this->model | |
| ->where('id', 148) | |
| ->where('study_region', StudyRegion::UNKNOWN->value) | |
| ->exists() | |
| ) { | |
| $counts[StudyRegion::ASIA->value] += 1; | |
| } |
| ->selectRaw('study_region, COUNT(*) as count') | ||
| ->pluck('count', 'study_region') | ||
| ->toArray(); | ||
|
|
There was a problem hiding this comment.
The grouped COUNT(*) values returned by pluck() are likely strings (PDO returns aggregate numeric columns as strings by default). This method can therefore return a mix of string and int values (especially after the += 1), which is error-prone for callers and can break strict assertions. Consider explicitly casting all counts to int before returning.
| // Ensure all counts are integers (PDO returns COUNT(*) as strings by default) | |
| $counts = array_map('intval', $counts); |
| $result = $this->queryService->getEachRegionsHeritagesCount(); | ||
|
|
||
| $this->assertSame(3, $result[StudyRegion::AFRICA->value]); | ||
| $this->assertSame(2, $result[StudyRegion::ASIA->value]); | ||
| $this->assertSame(3, $result[StudyRegion::EUROPE->value]); |
There was a problem hiding this comment.
This test expects Asia to be 2, but the fixture data only inserts one Asia record. The second count is coming from the production-specific +1 adjustment in the query service rather than from test data, making the test brittle/incorrect in isolation. Either insert the Jerusalem record (id 148 with study_region=UNKNOWN) into the test data, or change the production logic to only add 1 when that record is actually present.
No description provided.