South Asia Application Layer#45
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new WorldHeritageListQuery data structure and its corresponding factory for handling World Heritage site data, along with comprehensive test coverage. The changes also include fixes to an existing test to ensure proper data isolation.
- Creates a new query object pattern for World Heritage list operations
- Implements factory pattern for building query objects with validation
- Fixes existing test by implementing proper database cleanup and data seeding
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| WorldHeritageListQueryFactoryTest.php | New comprehensive test suite for the factory with positive and negative test cases |
| GetWorldHeritageByIdUseCaseTest.php | Updated existing test with proper database cleanup and corrected test data |
| WorldHeritageListQuery.php | New immutable query object with getter methods and array conversion |
| WorldHeritageListQueryFactory.php | New factory class with validation logic for building query objects |
| return new WorldHeritageListQuery( | ||
| id: Arr::get($request, 'id', null), | ||
| unesco_id: $request['unesco_id'], | ||
| official_name: (string)($request['official_name'] ?? ''), |
There was a problem hiding this comment.
This line casts the required field to an empty string if it's falsy, which contradicts the validation that ensures it's not null. If official_name is required and validated as non-null, the fallback to empty string is unnecessary and could mask validation issues.
| official_name: (string)($request['official_name'] ?? ''), | |
| official_name: $request['official_name'], |
| id: Arr::get($request, 'id', null), | ||
| unesco_id: $request['unesco_id'], | ||
| official_name: (string)($request['official_name'] ?? ''), | ||
| name: (string)($request['name'] ?? ''), |
There was a problem hiding this comment.
This line casts the required field to an empty string if it's falsy, which contradicts the validation that ensures it's not null. If name is required and validated as non-null, the fallback to empty string is unnecessary and could mask validation issues.
| name: (string)($request['name'] ?? ''), | |
| name: $request['name'], |
| unesco_id: $request['unesco_id'], | ||
| official_name: (string)($request['official_name'] ?? ''), | ||
| name: (string)($request['name'] ?? ''), | ||
| country: (string)($request['country'] ?? ''), |
There was a problem hiding this comment.
This line casts the required field to an empty string if it's falsy, which contradicts the validation that ensures it's not null. If country is required and validated as non-null, the fallback to empty string is unnecessary and could mask validation issues.
| country: (string)($request['country'] ?? ''), | |
| country: $request['country'], |
| official_name: (string)($request['official_name'] ?? ''), | ||
| name: (string)($request['name'] ?? ''), | ||
| country: (string)($request['country'] ?? ''), | ||
| region: (string)($request['region'] ?? ''), |
There was a problem hiding this comment.
This line casts the required field to an empty string if it's falsy, which contradicts the validation that ensures it's not null. If region is required and validated as non-null, the fallback to empty string is unnecessary and could mask validation issues.
| region: (string)($request['region'] ?? ''), | |
| region: (string)$request['region'], |
| name: (string)($request['name'] ?? ''), | ||
| country: (string)($request['country'] ?? ''), | ||
| region: (string)($request['region'] ?? ''), | ||
| category: (string)($request['category'] ?? ''), |
There was a problem hiding this comment.
This line casts the required field to an empty string if it's falsy, which contradicts the validation that ensures it's not null. If category is required and validated as non-null, the fallback to empty string is unnecessary and could mask validation issues.
| category: (string)($request['category'] ?? ''), | |
| category: (string)$request['category'], |
| country: (string)($request['country'] ?? ''), | ||
| region: (string)($request['region'] ?? ''), | ||
| category: (string)($request['category'] ?? ''), | ||
| year_inscribed: (int)($request['year_inscribed'] ?? 0), |
There was a problem hiding this comment.
This line casts the required field to 0 if it's falsy, which contradicts the validation that ensures it's not null. If year_inscribed is required and validated as non-null, the fallback to 0 is unnecessary and could mask validation issues.
| year_inscribed: (int)($request['year_inscribed'] ?? 0), | |
| year_inscribed: (int)$request['year_inscribed'], |
|
|
||
| namespace App\Packages\Features\QueryUseCases\ListQuery; | ||
|
|
||
| class WorldHeritageListQuery |
There was a problem hiding this comment.
There is an extra space between class and WorldHeritageListQuery. It should be class WorldHeritageListQuery.
| class WorldHeritageListQuery | |
| class WorldHeritageListQuery |
| return $this->unesco_site_url; | ||
| } | ||
|
|
||
| public function toArray(): array |
There was a problem hiding this comment.
The toArray() method includes the category field but it's missing from the array structure (line 131). This inconsistency could lead to missing data when converting the object to array format.
|
|
||
| private function refresh(): void | ||
| { | ||
| if (env('APP_ENV') === 'testing') { |
There was a problem hiding this comment.
Using env() directly in application code is not recommended. Consider using config() or App::environment() instead for better performance and consistency.
| if (env('APP_ENV') === 'testing') { | |
| if (config('app.env') === 'testing') { |
What I have done