Fix Infra Layer#88
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the infrastructure layer to update the database schema and data models from using auto-incrementing IDs to using UNESCO heritage site IDs as primary keys. This change aligns the system more closely with UNESCO's official identification system.
Key changes:
- Replaced auto-incrementing
idwith UNESCO site IDs as primary keys in the WorldHeritage model and related code - Updated country code standards from ISO2 to ISO3 format throughout the codebase
- Restructured seeder files to use the new identification system and proper country code mappings
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| WorldHeritageSeeder.php | New seeder file with UNESCO heritage sites using official site IDs |
| SiteStatePartySeeder.php | New seeder for site-country relationships with ISO3 country codes |
| JapaneseWorldHeritageSeeder.php | Removed old seeder file that used different ID scheme |
| DatabaseSeeder.php | Updated to use new seeder classes |
| CountrySeeder.php | Enhanced with ISO2 to ISO3 country code mapping |
| WorldHeritageRepository.php | Updated to use UNESCO IDs instead of auto-incrementing IDs |
| WorldHeritageEntity.php | Removed UNESCO ID field and updated validation patterns |
| WorldHeritage.php | Updated model to use non-incrementing primary key |
| Test files | Updated test data and assertions to match new ID scheme |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| 'short_description' => '氷期後のブナの自然拡散史を示すヨーロッパ各地の原生的ブナ林群から成る越境・連続資産。', | ||
| 'image_url' => null, | ||
| 'unesco_site_url' => 'https://whc.unesco.org/en/list/1133', | ||
| 'created_at' => $now, 'updated_at' => $now, |
There was a problem hiding this comment.
[nitpick] Missing spaces around array elements makes the code harder to read. Consider formatting as 'created_at' => $now, 'updated_at' => $now.
| ['state_party_code'=>'SVK','world_heritage_site_id'=>1133,'is_primary'=>true ,'inscription_year'=>2007,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'DEU','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2011,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'UKR','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2007,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'POL','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2021,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'ROU','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2017,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'CHN','world_heritage_site_id'=>1442,'is_primary'=>true ,'inscription_year'=>2014,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'KAZ','world_heritage_site_id'=>1442,'is_primary'=>false,'inscription_year'=>2014,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'KGZ','world_heritage_site_id'=>1442,'is_primary'=>false,'inscription_year'=>2014,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'UZB','world_heritage_site_id'=>1662,'is_primary'=>true ,'inscription_year'=>2023,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'TJK','world_heritage_site_id'=>1662,'is_primary'=>false,'inscription_year'=>2023,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'TKM','world_heritage_site_id'=>1662,'is_primary'=>false,'inscription_year'=>2023,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'JPN','world_heritage_site_id'=>661 ,'is_primary'=>true,'inscription_year'=>1993,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'JPN','world_heritage_site_id'=>688 ,'is_primary'=>true,'inscription_year'=>1994,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'JPN','world_heritage_site_id'=>662 ,'is_primary'=>true,'inscription_year'=>1993,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'JPN','world_heritage_site_id'=>663 ,'is_primary'=>true,'inscription_year'=>1993,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'JPN','world_heritage_site_id'=>1142,'is_primary'=>true,'inscription_year'=>2004,'created_at'=>$now,'updated_at'=>$now], | ||
| ['state_party_code'=>'JPN','world_heritage_site_id'=>1418,'is_primary'=>true,'inscription_year'=>2013,'created_at'=>$now,'updated_at'=>$now], |
There was a problem hiding this comment.
[nitpick] Inconsistent spacing around array operators and extra space before comma after 'true'. Format consistently as 'key' => 'value' with proper spacing.
| ['state_party_code'=>'SVK','world_heritage_site_id'=>1133,'is_primary'=>true ,'inscription_year'=>2007,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'DEU','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2011,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'UKR','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2007,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'POL','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2021,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'ROU','world_heritage_site_id'=>1133,'is_primary'=>false,'inscription_year'=>2017,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'CHN','world_heritage_site_id'=>1442,'is_primary'=>true ,'inscription_year'=>2014,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'KAZ','world_heritage_site_id'=>1442,'is_primary'=>false,'inscription_year'=>2014,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'KGZ','world_heritage_site_id'=>1442,'is_primary'=>false,'inscription_year'=>2014,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'UZB','world_heritage_site_id'=>1662,'is_primary'=>true ,'inscription_year'=>2023,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'TJK','world_heritage_site_id'=>1662,'is_primary'=>false,'inscription_year'=>2023,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'TKM','world_heritage_site_id'=>1662,'is_primary'=>false,'inscription_year'=>2023,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'JPN','world_heritage_site_id'=>661 ,'is_primary'=>true,'inscription_year'=>1993,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'JPN','world_heritage_site_id'=>688 ,'is_primary'=>true,'inscription_year'=>1994,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'JPN','world_heritage_site_id'=>662 ,'is_primary'=>true,'inscription_year'=>1993,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'JPN','world_heritage_site_id'=>663 ,'is_primary'=>true,'inscription_year'=>1993,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'JPN','world_heritage_site_id'=>1142,'is_primary'=>true,'inscription_year'=>2004,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code'=>'JPN','world_heritage_site_id'=>1418,'is_primary'=>true,'inscription_year'=>2013,'created_at'=>$now,'updated_at'=>$now], | |
| ['state_party_code' => 'SVK', 'world_heritage_site_id' => 1133, 'is_primary' => true, 'inscription_year' => 2007, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'DEU', 'world_heritage_site_id' => 1133, 'is_primary' => false, 'inscription_year' => 2011, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'UKR', 'world_heritage_site_id' => 1133, 'is_primary' => false, 'inscription_year' => 2007, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'POL', 'world_heritage_site_id' => 1133, 'is_primary' => false, 'inscription_year' => 2021, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'ROU', 'world_heritage_site_id' => 1133, 'is_primary' => false, 'inscription_year' => 2017, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'CHN', 'world_heritage_site_id' => 1442, 'is_primary' => true, 'inscription_year' => 2014, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'KAZ', 'world_heritage_site_id' => 1442, 'is_primary' => false, 'inscription_year' => 2014, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'KGZ', 'world_heritage_site_id' => 1442, 'is_primary' => false, 'inscription_year' => 2014, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'UZB', 'world_heritage_site_id' => 1662, 'is_primary' => true, 'inscription_year' => 2023, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'TJK', 'world_heritage_site_id' => 1662, 'is_primary' => false, 'inscription_year' => 2023, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'TKM', 'world_heritage_site_id' => 1662, 'is_primary' => false, 'inscription_year' => 2023, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'JPN', 'world_heritage_site_id' => 661, 'is_primary' => true, 'inscription_year' => 1993, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'JPN', 'world_heritage_site_id' => 688, 'is_primary' => true, 'inscription_year' => 1994, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'JPN', 'world_heritage_site_id' => 662, 'is_primary' => true, 'inscription_year' => 1993, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'JPN', 'world_heritage_site_id' => 663, 'is_primary' => true, 'inscription_year' => 1993, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'JPN', 'world_heritage_site_id' => 1142, 'is_primary' => true, 'inscription_year' => 2004, 'created_at' => $now, 'updated_at' => $now], | |
| ['state_party_code' => 'JPN', 'world_heritage_site_id' => 1418, 'is_primary' => true, 'inscription_year' => 2013, 'created_at' => $now, 'updated_at' => $now], |
| if (!$code3) { | ||
| throw new RuntimeException("ISO2 code {$code2} has no ISO3 mapping."); |
There was a problem hiding this comment.
The error message could be more helpful by suggesting what action to take. Consider: 'ISO2 code {$code2} has no ISO3 mapping. Please add the mapping to the iso2to3 array.'
| if (!$code3) { | |
| throw new RuntimeException("ISO2 code {$code2} has no ISO3 mapping."); | |
| throw new RuntimeException("ISO2 code {$code2} has no ISO3 mapping. Please add the mapping to the iso2to3 array."); |
| @@ -76,7 +74,7 @@ public function insertHeritage( | |||
| } | |||
|
|
|||
| $heritage->state_party = !empty($codes) ? implode(',', $codes) : null; | |||
There was a problem hiding this comment.
Empty line with no purpose. The removed $heritage->save(); call should either be replaced with an actual save operation or this section needs proper handling of the model persistence.
|
|
||
| private function refresh(): void | ||
| { | ||
| if (env('APP_ENV') === 'testing') { |
There was a problem hiding this comment.
Using env() directly in code is not recommended. Use config('app.env') instead or inject the environment through dependency injection.
| if (env('APP_ENV') === 'testing') { | |
| if (config('app.env') === 'testing') { |
| @@ -32,14 +34,16 @@ private function refresh(): void | |||
| if (env('APP_ENV') === 'testing') { | |||
There was a problem hiding this comment.
Using env() directly in code is not recommended. Use config('app.env') instead or inject the environment through dependency injection.
| if (env('APP_ENV') === 'testing') { | |
| if (config('app.env') === 'testing') { |
what i have done