test: add image contract tests for getHeritageById#388
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the World Heritage “get by id” query service tests to better validate the returned image payload and adjusts the Image model so is_primary is treated as a boolean.
Changes:
- Add dedicated tests for the images payload contract and the “no images” case.
- Introduce an
is_primaryboolean cast on theImageEloquent model.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/app/Packages/Domains/Test/QueryService/WorldHeritageQueryService_getByIdTest.php |
Splits out image-related assertions into dedicated tests and adds an empty-images scenario. |
src/app/Models/Image.php |
Adds a cast for is_primary to ensure boolean semantics at the model layer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private function seedImages(): void | ||
| { | ||
| return | ||
| DB::table('world_heritage_site_images')->insert([ | ||
| [ | ||
| 'world_heritage_site_id' => 1133, |
There was a problem hiding this comment.
seedImages() is declared but never called in this test class. If the intent is to make image assertions independent of DatabaseSeeder/ImageSeeder, call it in setUp() (and ensure the table is clean before inserting); otherwise remove the unused helper to avoid dead code in tests.
| $this->assertNotEmpty($images); | ||
| $this->assertTrue($images[0]['is_primary']); | ||
| $this->assertEquals(0, $images[0]['sort_order']); | ||
| $this->assertFalse($images[1]['is_primary']); | ||
| $this->assertEquals(1, $images[1]['sort_order']); |
There was a problem hiding this comment.
test_images_contract indexes $images[1] but only asserts assertNotEmpty($images), which guarantees at least 1 element. Add an assertion that there are at least 2 images (or guard the second access) so the test fails with a clear message instead of an undefined offset error.
| $this->assertArrayHasKey('id', $img); | ||
| $this->assertArrayHasKey('url', $img); | ||
| $this->assertArrayHasKey('sort_order', $img); | ||
| $this->assertArrayHasKey('is_primary', $img); |
There was a problem hiding this comment.
The image contract assertions no longer verify that url is non-empty. Since world_heritage_site_images.url is non-null and import logic filters out empty URLs, consider restoring an assertion that each returned url is a non-empty string to prevent silent regressions.
| $this->assertArrayHasKey('is_primary', $img); | |
| $this->assertArrayHasKey('is_primary', $img); | |
| $this->assertIsString($img['url']); | |
| $this->assertNotEmpty($img['url']); |
| public function casts(): array | ||
| { | ||
| return [ | ||
| 'is_primary' => 'boolean', | ||
| ]; | ||
| } |
There was a problem hiding this comment.
casts() should follow the same visibility pattern used by other Eloquent models in this repo (e.g., WorldHeritage uses protected function casts(): array). Making this public unnecessarily exposes an internal model hook; switch it to protected to match Laravel conventions and local style.
zigzagdev
changed the title
zigzagdev
merged commit 9dcfd6d
into
chore/integrate-image_url-into-image-table
What
seedImages()to setUp for image test datatest_images_contract()to verify image shape, ordering and is_primarytest_images_empty_when_no_images()to verify empty array is returned when no images exist