Add report block width options, fix form population, implement field canvas with comprehensive enums, slug-based lookups, theme-aware styling, internationalization, Tailwind reliability improvements, and UI enhancements#334
Conversation
- Extended ReportBlockWidth enum with ONE_THIRD and TWO_THIRDS options - Added getGridWidth() method to calculate grid columns (4, 6, 8, 12) - Updated ReportTemplateService to use new width calculation - Fixed configureBlockAction to properly populate form data with logging - Created ReportBlockService with saveBlockFields/loadBlockFields methods - Added fields-canvas blade view for drag/drop field configuration - Updated ReportBlockForm schema to include field canvas section - Fixed block width rendering in design-report-template blade - Added config column to report_blocks migration - Created comprehensive test suites (all marked incomplete): * ReportBlockWidthTest - Tests enum and grid width calculations * ReportBlockServiceFieldsTest - Tests JSON field storage/loading * ReportBuilderBlockWidthTest - Tests width rendering in designer * ReportBuilderBlockEditTest - Tests form data population - Created ReportBlockFactory and ReportTemplateFactory - Added HasFactory trait to ReportBlock and ReportTemplate models Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
- Created ReportBuilderFieldCanvasIntegrationTest with 8 test scenarios - Added REPORT_BUILDER_ENHANCEMENTS.md documentation - Documents all problems solved and technical implementation - Includes usage examples and file manifest - All 39 tests marked incomplete as per requirements Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
…rces/bands - Created ReportDataSource enum with all data sources (alphabetically sorted) - Created ReportBand enum for report bands - Removed config column from migration (config now comes from JSON files only) - Updated ReportBlock model to cast data_source and default_band as enums - Updated slug generation to include random string for uniqueness - Refactored configureBlockAction to use blockSlug instead of blockType - Added slug property to BlockDTO and BlockTransformer - Updated ReportTemplateService to pass slug with blocks - Updated blade template to pass blockSlug to action - Moved documentation from Modules/Core/Docs to .github directory - Created report-fields.php config with 100+ fields from all data sources - Updated fields-canvas.blade.php to load fields from config - Enhanced field display to show data source - Updated ReportBlockForm to use enums for data_source and default_band selects - Updated ReportBlocksSeeder to use enums and generate unique slugs - Updated ReportBlockFactory to use enums and proper slug generation Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
- Removed all references to $block->config from ReportBlockService - Config now exclusively loaded from JSON files - Fixed BlockDTO syntax error (duplicate return statement) - Updated tests to not set $block->config (no longer a model attribute) - getBlockConfiguration now returns empty array if JSON file doesn't exist Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
All code review comments addressed:
|
- Created ReportBlockType enum with ADDRESS, DETAILS, METADATA, TOTALS types - Added getLabel() and getDescription() methods to enum - Updated ReportBlock model to cast block_type as enum - Updated ReportBlockForm to use enum options instead of database query - Refactored ReportBlocksSeeder to map blocks to appropriate enum types: * company_header, client_header -> ADDRESS * header_invoice_meta, footer_notes, footer_qr_code -> METADATA * invoice_items, invoice_item_tax -> DETAILS * footer_totals -> TOTALS - Updated ReportBlockFactory to use enum for block_type generation - Added 8 translation keys for enum labels and descriptions - All block types now type-safe and consistent across application Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughComprehensive Report Builder enhancement introducing field canvas UI component, new enums and services, block width expansion (one_third, two_thirds), slug-based block lookup, and JSON-based field storage. Includes database migrations, factories, extensive translations, CSS styling, and multiple test suites. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Livewire as ReportBuilder<br/>(Livewire)
participant Service as ReportBlockService
participant Storage as Storage<br/>(JSON)
participant FieldSvc as ReportFieldService
participant BlockDTO as BlockDTO
User->>Livewire: Click "Edit Block"
activate Livewire
Livewire->>Livewire: Lookup block by slug
Livewire->>BlockDTO: Convert block data
BlockDTO->>Livewire: Return BlockDTO with slug
Livewire->>Service: Load existing fields<br/>loadBlockFields()
activate Service
Service->>Storage: Read block config JSON
Storage-->>Service: Field array
Service-->>Livewire: Return fields
deactivate Service
Livewire->>FieldSvc: Get available fields<br/>getAvailableFields()
activate FieldSvc
FieldSvc-->>Livewire: Field catalog
deactivate FieldSvc
Livewire-->>User: Render form + fields canvas
deactivate Livewire
User->>Livewire: Drag fields to canvas
activate Livewire
rect rgb(200, 240, 255)
note right of Livewire: Alpine.js manages<br/>local canvas state
Livewire->>Livewire: `@this.set`('data.fields', canvasFields)
end
Livewire-->>User: Update canvas UI
deactivate Livewire
User->>Livewire: Save block
activate Livewire
Livewire->>Service: Save block + fields<br/>saveBlockFields()
activate Service
Service->>Service: Prepare JSON config
Service->>Storage: Write/overwrite JSON
Storage-->>Service: ✓ Saved
Service-->>Livewire: ✓ Complete
deactivate Service
Livewire-->>User: Success message
deactivate Livewire
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php (1)
54-155: Extract duplicate logic into private method.The
fillFormandmountUsingclosures contain nearly identical logic for loading and preparing block data (enum conversions, defaults, logging). This violates the DRY principle and makes maintenance harder.🔎 Proposed refactor to eliminate duplication
+ /** + * Prepare block data for form population. + */ + private function prepareBlockData(?string $blockSlug): array + { + if (!$blockSlug) { + return []; + } + + $block = ReportBlock::query()->where('slug', $blockSlug)->first(); + + if (!$block) { + return [ + 'name' => '', + 'width' => 'full', + 'block_type' => '', + 'data_source' => '', + 'default_band' => '', + 'is_active' => true, + ]; + } + + $data = $block->toArray(); + + // Ensure all fields are present for entanglement + $data['name'] ??= ''; + $data['block_type'] ??= ''; + + // Handle enum conversions for form + if (isset($data['data_source']) && $data['data_source'] instanceof BackedEnum) { + $data['data_source'] = $data['data_source']->value; + } + $data['data_source'] ??= ''; + + if (isset($data['default_band']) && $data['default_band'] instanceof BackedEnum) { + $data['default_band'] = $data['default_band']->value; + } + $data['default_band'] ??= ''; + + $data['is_active'] = (bool) ($data['is_active'] ?? true); + + // If it's a BackedEnum (width), we need to ensure it's the value + if (isset($data['width']) && $data['width'] instanceof BackedEnum) { + $data['width'] = $data['width']->value; + } + $data['width'] ??= 'full'; + + \Illuminate\Support\Facades\Log::info('Block data prepared (slug: ' . $blockSlug . '):', $data); + + return $data; + } + public function configureBlockAction(): Action { return Action::make('configureBlock') ->schema(fn (Schema $schema) => ReportBlockForm::configure($schema)) - ->fillForm(function (array $arguments) { - $blockSlug = $arguments['blockSlug'] ?? null; - if ( ! $blockSlug) { - return []; - } - - // Look up the block using slug (very unique identifier) - $block = ReportBlock::query()->where('slug', $blockSlug)->first(); - - if ( ! $block) { - return [ - 'name' => '', - 'width' => 'full', - 'block_type' => '', - 'data_source' => '', - 'default_band' => '', - 'is_active' => true, - ]; - } - - $data = $block->toArray(); - - // Ensure all fields are present for entanglement - $data['name'] ??= ''; - $data['block_type'] ??= ''; - - // Handle enum conversions for form - if (isset($data['data_source']) && $data['data_source'] instanceof BackedEnum) { - $data['data_source'] = $data['data_source']->value; - } - $data['data_source'] ??= ''; - - if (isset($data['default_band']) && $data['default_band'] instanceof BackedEnum) { - $data['default_band'] = $data['default_band']->value; - } - $data['default_band'] ??= ''; - - $data['is_active'] = (bool) ($data['is_active'] ?? true); - - // If it's a BackedEnum (width), we need to ensure it's the value - if (isset($data['width']) && $data['width'] instanceof BackedEnum) { - $data['width'] = $data['width']->value; - } - $data['width'] ??= 'full'; - - // Debug output - will show in Livewire component response - \Illuminate\Support\Facades\Log::info('Block data for edit (slug: ' . $blockSlug . '):', $data); - - return $data; + ->fillForm(function (array $arguments) { + return $this->prepareBlockData($arguments['blockSlug'] ?? null); }) ->mountUsing(function (Schema $schema, array $arguments) { - $blockSlug = $arguments['blockSlug'] ?? null; - if ( ! $blockSlug) { - return; - } - - // Look up the block using slug (very unique identifier) - $block = ReportBlock::query()->where('slug', $blockSlug)->first(); - - if ( ! $block) { - $schema->fill([ - 'name' => '', - 'width' => 'full', - 'block_type' => '', - 'data_source' => '', - 'default_band' => '', - 'is_active' => true, - ]); - - return; - } - - $data = $block->toArray(); - - // Ensure all fields are present for entanglement - $data['name'] ??= ''; - $data['block_type'] ??= ''; - - // Handle enum conversions for form - if (isset($data['data_source']) && $data['data_source'] instanceof BackedEnum) { - $data['data_source'] = $data['data_source']->value; - } - $data['data_source'] ??= ''; - - if (isset($data['default_band']) && $data['default_band'] instanceof BackedEnum) { - $data['default_band'] = $data['default_band']->value; - } - $data['default_band'] ??= ''; - - $data['is_active'] = (bool) ($data['is_active'] ?? true); - - // If it's a BackedEnum (width), we need to ensure it's the value - if (isset($data['width']) && $data['width'] instanceof BackedEnum) { - $data['width'] = $data['width']->value; - } - $data['width'] ??= 'full'; - - // Debug output - will show in Livewire component response - \Illuminate\Support\Facades\Log::info('Mounting block config (slug: ' . $blockSlug . '):', $data); - - $schema->fill($data); + $data = $this->prepareBlockData($arguments['blockSlug'] ?? null); + if (!empty($data)) { + $schema->fill($data); + } })
🧹 Nitpick comments (14)
Modules/Core/Database/Factories/ReportTemplateFactory.php (1)
15-28: Consider title case for multi-word names.The
ucfirst()function only capitalizes the first character of the string. For multi-word names generated byfaker->words(3, true), this means only the very first word is capitalized (e.g., "Lorem ipsum dolor" becomes "Lorem ipsum dolor"). Consider usingStr::title()for proper title case.🔎 Proposed refactor
public function definition(): array { $name = $this->faker->words(3, true); return [ 'company_id' => Company::factory(), - 'name' => ucfirst($name), + 'name' => Str::title($name), 'slug' => Str::slug($name), 'description' => $this->faker->optional(0.7)->sentence(), 'template_type' => $this->faker->randomElement(ReportTemplateType::cases()), 'is_system' => false, 'is_active' => true, ]; }.github/REPORT_BUILDER_ENHANCEMENTS.md (1)
132-132: Consider using heading format for test count.For consistency with document structure, consider converting the emphasized text to a heading:
🔎 Suggested formatting
-**Total: 39 test cases** +#### Total: 39 test casesModules/Core/Tests/Unit/ReportBlockWidthTest.php (1)
83-103: Consider consolidating overlapping tests.
it_supports_all_width_valuesandit_calculates_correct_grid_widths_for_12_column_gridtest nearly identical logic—both verify enum cases map to expected grid widths. Consider using a data provider to reduce duplication or removing one of these tests.Also applies to: 105-123
Modules/Core/Enums/ReportBand.php (1)
16-25: Labels should use translation keys for i18n support.The PR objectives mention implementing 125 translation keys and i18n support. These hardcoded English labels break multi-language support. Consider returning translation keys that can be resolved by the caller.
🔎 Proposed fix
public function getLabel(): string { return match ($this) { - self::HEADER => 'Header', - self::GROUP_HEADER => 'Group Header', - self::DETAILS => 'Details', - self::GROUP_FOOTER => 'Group Footer', - self::FOOTER => 'Footer', + self::HEADER => __('ip.report_band_header'), + self::GROUP_HEADER => __('ip.report_band_group_header'), + self::DETAILS => __('ip.report_band_details'), + self::GROUP_FOOTER => __('ip.report_band_group_footer'), + self::FOOTER => __('ip.report_band_footer'), }; }Modules/Core/Database/Seeders/ReportBlocksSeeder.php (1)
102-105: Move directory creation outside the loop.The directory existence check and creation runs on every iteration but only needs to execute once before processing blocks.
🔎 Proposed fix
+ // Ensure directory exists before processing blocks + if (!Storage::disk('local')->exists('report_blocks')) { + Storage::disk('local')->makeDirectory('report_blocks'); + } + foreach ($blocks as $block) { $baseSlug = Str::slug($block['name']); $slug = $baseSlug . '-' . Str::random(8); $filename = $slug; ReportBlock::create([ 'is_active' => true, 'is_system' => true, 'block_type' => $block['block_type'], 'name' => $block['name'], 'slug' => $slug, 'filename' => $filename, 'width' => $block['width'], 'data_source' => $block['data_source'], 'default_band' => $block['default_band'], ]); - // Ensure directory exists - if (!Storage::disk('local')->exists('report_blocks')) { - Storage::disk('local')->makeDirectory('report_blocks'); - } - // Save default config to JSON if it doesn't existModules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php (1)
53-55: Hardcoded English strings should use translation keys.Several user-facing strings are not internationalized, contradicting the PR's i18n goals:
- Line 54: "Drag fields to the canvas to configure block layout"
- Line 64: "Source: " prefix
- Line 89: "Drag fields here to configure block layout"
- Line 113: "Fields will be saved to a JSON file when you save the block configuration."
🔎 Proposed fix for key strings
- <p class="text-xs text-gray-500 dark:text-gray-400 mb-3"> - Drag fields to the canvas to configure block layout - </p> + <p class="text-xs text-gray-500 dark:text-gray-400 mb-3"> + @lang('ip.drag_fields_to_canvas') + </p>- <div class="text-xs opacity-75" x-text="'Source: ' + field.source"></div> + <div class="text-xs opacity-75" x-text="'@lang('ip.source')' + ': ' + field.source"></div>- <p class="text-sm font-medium">Drag fields here to configure block layout</p> + <p class="text-sm font-medium">@lang('ip.drag_fields_here')</p>- <p class="text-xs text-gray-500 dark:text-gray-400 mt-2"> - Fields will be saved to a JSON file when you save the block configuration. - </p> + <p class="text-xs text-gray-500 dark:text-gray-400 mt-2"> + @lang('ip.fields_saved_to_json') + </p>Also applies to: 64-64, 89-89, 112-114
Modules/Core/Services/ReportFieldService.php (3)
15-31: Consider using array_map for more functional code.The foreach loop manually builds the fields array. A more idiomatic approach would use
array_mapwith a closure.🔎 Proposed refactor using array_map
public function getAvailableFields(): array { $config = config('report-fields', []); - $fields = []; - - // Config is already flat, just translate labels - foreach ($config as $field) { - $fields[] = [ + + return array_map(function ($field) { + return [ 'id' => $field['id'], 'label' => trans($field['label']), 'source' => $field['source'], 'format' => $field['format'] ?? null, ]; - } - - return $fields; + }, $config); }
40-58: Consider using array_map and array_filter for more functional code.The foreach loop manually filters and builds the array. A more idiomatic approach would combine
array_filterandarray_map.🔎 Proposed refactor using functional approach
public function getFieldsBySource(string $source): array { $config = config('report-fields', []); - $fields = []; - - // Filter fields by source - foreach ($config as $field) { - if ($field['source'] === $source) { - $fields[] = [ + + $filtered = array_filter($config, fn($field) => $field['source'] === $source); + + return array_map(function ($field) { + return [ 'id' => $field['id'], 'label' => trans($field['label']), 'source' => $field['source'], 'format' => $field['format'] ?? null, ]; - } - } - - return $fields; + }, $filtered); }
65-77: Optimize performance using array_unique and array_column.The current implementation uses
in_array()inside a loop, resulting in O(n²) complexity. Usingarray_unique()witharray_column()would be more efficient.🔎 Proposed performance optimization
public function getDataSources(): array { $config = config('report-fields', []); - $sources = []; - - foreach ($config as $field) { - if (!in_array($field['source'], $sources, true)) { - $sources[] = $field['source']; - } - } - - return $sources; + + return array_values(array_unique(array_column($config, 'source'))); }Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php (1)
100-100: Use Log facade alias for cleaner code.The fully qualified class name
\Illuminate\Support\Facades\Logis verbose. Consider addinguse Illuminate\Support\Facades\Log;at the top and using justLog::info().🔎 Proposed refactor
Add at the top of the file:
use Illuminate\Support\Str; +use Illuminate\Support\Facades\Log; use Livewire\Attributes\On;Then update the log calls:
- \Illuminate\Support\Facades\Log::info('Block data for edit (slug: ' . $blockSlug . '):', $data); + Log::info('Block data for edit (slug: ' . $blockSlug . '):', $data);- \Illuminate\Support\Facades\Log::info('Mounting block config (slug: ' . $blockSlug . '):', $data); + Log::info('Mounting block config (slug: ' . $blockSlug . '):', $data);Also applies to: 152-152
Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php (1)
34-40: Consider using factory for consistency.While creating unsaved
ReportBlockinstances works for unit tests, using the factory (as shown in the integration tests) would provide more consistency and ensure all required enum fields are properly initialized.💡 Optional improvement
-$block = new ReportBlock(); -$block->id = 1; -$block->block_type = 'test_block'; -$block->name = 'Test Block'; -$block->slug = 'test-block'; -$block->filename = 'test-block'; -$block->width = ReportBlockWidth::FULL; +$block = ReportBlock::factory()->make([ + 'block_type' => 'test_block', + 'name' => 'Test Block', + 'slug' => 'test-block', + 'filename' => 'test-block', + 'width' => ReportBlockWidth::FULL, +]);Note: The integration tests use
create()which persists to the database, whilemake()creates an in-memory instance suitable for unit tests.Modules/Core/Tests/Feature/ReportBuilderBlockWidthTest.php (2)
20-33: Remove unused properties and setUp overhead.The
$companyand$templateproperties are created insetUp()but never used in any of the test methods. Each test createsReportBlockinstances independently without associating them to a template or company.🔎 Proposed fix to remove unused setup
- private Company $company; - - private ReportTemplate $template; - - protected function setUp(): void - { - parent::setUp(); - - /* Arrange - Create test company and template */ - $this->company = Company::factory()->create(); - $this->template = ReportTemplate::factory()->create([ - 'company_id' => $this->company->id, - ]); - }If these properties are intended for future integration tests that verify blocks within templates, consider adding a comment explaining the planned usage or defer the setup until those tests are implemented.
169-190: Consider extracting span calculation logic to avoid duplication.Line 184 duplicates the grid span calculation logic from the Blade template. If the template's span logic changes, this test would need manual synchronization.
Consider extracting the span calculation into a shared helper method (e.g., on the
ReportBlockWidthenum or a dedicated service) that both the template and tests can reference. This ensures consistency and makes the business rule explicit.Alternatively, if the span logic is specific to the view layer and not a domain concern, you could document this test as intentionally mirroring template behavior and accept the duplication.
Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php (1)
20-34: Remove unused properties and setUp overhead.Similar to
ReportBuilderBlockWidthTest, the$companyand$templateproperties are created but never used in any test method.🔎 Proposed fix
- private Company $company; - - private ReportTemplate $template; - - protected function setUp(): void - { - parent::setUp(); - - /* Arrange - Create test company and template */ - $this->company = Company::factory()->create(); - $this->template = ReportTemplate::factory()->create([ - 'company_id' => $this->company->id, - ]); - }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.github/REPORT_BUILDER_ENHANCEMENTS.md.github/copilot-instructions.md.junie/guidelines.mdModules/Core/DTOs/BlockDTO.phpModules/Core/Database/Factories/ReportBlockFactory.phpModules/Core/Database/Factories/ReportTemplateFactory.phpModules/Core/Database/Migrations/2026_01_01_184544_create_report_blocks_table.phpModules/Core/Database/Seeders/ReportBlocksSeeder.phpModules/Core/Enums/ReportBand.phpModules/Core/Enums/ReportBlockType.phpModules/Core/Enums/ReportBlockWidth.phpModules/Core/Enums/ReportDataSource.phpModules/Core/Filament/Admin/Resources/ReportBlocks/Schemas/ReportBlockForm.phpModules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.phpModules/Core/Models/ReportBlock.phpModules/Core/Models/ReportTemplate.phpModules/Core/Services/ReportBlockService.phpModules/Core/Services/ReportFieldService.phpModules/Core/Services/ReportTemplateService.phpModules/Core/Tests/Feature/ReportBuilderBlockEditTest.phpModules/Core/Tests/Feature/ReportBuilderBlockWidthTest.phpModules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.phpModules/Core/Tests/Unit/ReportBlockServiceFieldsTest.phpModules/Core/Tests/Unit/ReportBlockWidthTest.phpModules/Core/Transformers/BlockTransformer.phpModules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.phpModules/Core/resources/views/filament/admin/resources/report-template-resource/pages/design-report-template.blade.phpconfig/report-fields.phpresources/css/filament/company/nord.cssresources/lang/en/ip.phptailwind.config.js
🧰 Additional context used
📓 Path-based instructions (9)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Follow SOLID Principles at all times
Prefer early returns for readability
Code must be modular and refactored; avoid inline data setups
Centralize shared functionality in Traits to avoid code duplication
CatchError,ErrorException, andThrowableseparately
Use Laravel's HTTP client instead of curl or Guzzle
Upserts must use repository methods instead ofupdateOrCreate
Extract duplicate code into private/protected methods following Single Responsibility Principle
Use early returns to reduce nesting and improve readability
Validate inputs at the start of methods and abort/throw exceptions early
Extract complex conditions into well-named methods
Use meaningful method names that describe what they do
Files:
Modules/Core/DTOs/BlockDTO.phpModules/Core/Enums/ReportBlockType.phpconfig/report-fields.phpModules/Core/Enums/ReportDataSource.phpModules/Core/Enums/ReportBand.phpModules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.phpModules/Core/Tests/Unit/ReportBlockWidthTest.phpModules/Core/Services/ReportBlockService.phpModules/Core/Filament/Admin/Resources/ReportBlocks/Schemas/ReportBlockForm.phpModules/Core/Database/Factories/ReportTemplateFactory.phpModules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.phpModules/Core/Tests/Unit/ReportBlockServiceFieldsTest.phpModules/Core/Models/ReportBlock.phpModules/Core/Database/Seeders/ReportBlocksSeeder.phpModules/Core/Services/ReportTemplateService.phpModules/Core/resources/views/filament/admin/resources/report-template-resource/pages/design-report-template.blade.phpModules/Core/Tests/Feature/ReportBuilderBlockWidthTest.phpModules/Core/Tests/Feature/ReportBuilderBlockEditTest.phpModules/Core/Services/ReportFieldService.phpModules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.phpModules/Core/Database/Factories/ReportBlockFactory.phpModules/Core/Transformers/BlockTransformer.phpModules/Core/Models/ReportTemplate.phpModules/Core/Database/Migrations/2026_01_01_184544_create_report_blocks_table.phpModules/Core/Enums/ReportBlockWidth.phpresources/lang/en/ip.php
**/DTOs/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/DTOs/*.php: All DTOs must avoid constructors; use static named constructors when necessary
DTOs must rely on getters and setters for data access
Files:
Modules/Core/DTOs/BlockDTO.php
**/Tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/Tests/**/*.php: Test functions must be prefixed withit_and make grammatical sense (e.g.,it_creates_payment,it_validates_invoice_has_customer)
Use#[Test]attribute instead of@testannotations in unit tests
Prefer Fakes and Fixtures over Mocks in unit tests
Place happy paths last in test cases
Reusable test logic (fixtures, setup) must live in abstract test cases, not inline
Tests must include inline comment blocks for sections (/* Arrange /, / Act /, / Assert */)
Tests must be meaningful - validate actual behavior and data, not simple "ok" checks
Use data providers for testing multiple scenarios with the same logic
Never extendTests\TestCase; all tests must extend abstract test cases fromModules/Core/Tests/(AbstractTestCase,AbstractAdminPanelTestCase, orAbstractCompanyPanelTestCase)
Files:
Modules/Core/Tests/Unit/ReportBlockWidthTest.phpModules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.phpModules/Core/Tests/Unit/ReportBlockServiceFieldsTest.phpModules/Core/Tests/Feature/ReportBuilderBlockWidthTest.phpModules/Core/Tests/Feature/ReportBuilderBlockEditTest.php
**/{Controllers,Services,Jobs}/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All API requests must go through the Advanced API Client; no direct API calls in controllers, services, or jobs
Files:
Modules/Core/Services/ReportBlockService.phpModules/Core/Services/ReportTemplateService.phpModules/Core/Services/ReportFieldService.php
**/Filament/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Filament resource generation must keep a configurable
$excludedFieldsarray
Files:
Modules/Core/Filament/Admin/Resources/ReportBlocks/Schemas/ReportBlockForm.phpModules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php
**/Models/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/Models/*.php: Do not use$fillablearray in Laravel Models
Do not usetimestampsorsoftDeletesproperties/traits in Models unless explicitly specified
Use native PHP type hints and utilize$castsfor Enum fields in Models
Files:
Modules/Core/Models/ReportBlock.phpModules/Core/Models/ReportTemplate.php
**/Seeders/**Seeder.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/Seeders/**Seeder.php: Seed 5 default roles (superadmin,admin,assistance,useradmin,user)
Ensure users can belong to accounts when relevant in seeding
Files:
Modules/Core/Database/Seeders/ReportBlocksSeeder.php
**/Transformers/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Transformers must use
toDto()andtoModel()methods
Files:
Modules/Core/Transformers/BlockTransformer.php
**/Migrations/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/Migrations/*.php: Do not use JSON columns in Laravel migrations
Do not use ENUM columns in Laravel migrations
Do not usetimestamps()orsoftDeletes()in Laravel migrations unless explicitly specified
Files:
Modules/Core/Database/Migrations/2026_01_01_184544_create_report_blocks_table.php
🧠 Learnings (12)
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Filament/Resources/*.php : Add docblocks above Filament `form()`, `table()`, and `getRelations()` methods documenting relationships and fields
Applied to files:
Modules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.phpModules/Core/Filament/Admin/Resources/ReportBlocks/Schemas/ReportBlockForm.php.github/REPORT_BUILDER_ENHANCEMENTS.md
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Tests/**/*Export*.php : Export tests must use fakes: `Queue::fake()`, `Storage::fake()`, and verify job dispatching with `Bus::assertChained()`
Applied to files:
Modules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.phpModules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Tests/**/*.php : Use `#[Test]` attribute instead of `test` annotations in unit tests
Applied to files:
Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Tests/**/*.php : Never extend `Tests\TestCase`; all tests must extend abstract test cases from `Modules/Core/Tests/` (`AbstractTestCase`, `AbstractAdminPanelTestCase`, or `AbstractCompanyPanelTestCase`)
Applied to files:
Modules/Core/Tests/Feature/ReportBuilderBlockWidthTest.phpModules/Core/Tests/Feature/ReportBuilderBlockEditTest.php
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Tests/**/*.php : Tests must include inline comment blocks for sections (/* Arrange */, /* Act */, /* Assert */)
Applied to files:
Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Transformers/*.php : Transformers must use `toDto()` and `toModel()` methods
Applied to files:
Modules/Core/Transformers/BlockTransformer.php
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: All DTOs must be transformed using Transformers
Applied to files:
Modules/Core/Transformers/BlockTransformer.php
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Tests/**/*.php : Test functions must be prefixed with `it_` and make grammatical sense (e.g., `it_creates_payment`, `it_validates_invoice_has_customer`)
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/*.php : Follow SOLID Principles at all times
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/*.php : Code must be modular and refactored; avoid inline data setups
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/*.php : Use meaningful method names that describe what they do
Applied to files:
.github/copilot-instructions.md
📚 Learning: 2025-12-30T10:55:34.915Z
Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T10:55:34.915Z
Learning: Applies to **/Migrations/*.php : Do not use JSON columns in Laravel migrations
Applied to files:
Modules/Core/Database/Migrations/2026_01_01_184544_create_report_blocks_table.php
🧬 Code graph analysis (13)
Modules/Core/Tests/Unit/ReportBlockWidthTest.php (3)
Modules/Core/Tests/AbstractTestCase.php (1)
AbstractTestCase(7-15)Modules/Core/Database/Factories/ReportBlockFactory.php (1)
width(49-54)Modules/Core/Enums/ReportBlockWidth.php (1)
getGridWidth(15-23)
Modules/Core/Services/ReportBlockService.php (2)
Modules/Core/Models/ReportBlock.php (1)
ReportBlock(12-34)Modules/Core/Repositories/ReportTemplateFileRepository.php (2)
exists(97-102)get(52-87)
Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php (2)
Modules/Core/Models/ReportBlock.php (1)
ReportBlock(12-34)Modules/Core/Tests/AbstractAdminPanelTestCase.php (1)
AbstractAdminPanelTestCase(10-42)
Modules/Core/Models/ReportBlock.php (2)
Modules/Core/Models/ReportTemplate.php (1)
newFactory(60-63)Modules/Core/Database/Factories/ReportBlockFactory.php (1)
ReportBlockFactory(13-55)
Modules/Core/Database/Seeders/ReportBlocksSeeder.php (1)
Modules/Core/Models/ReportBlock.php (1)
ReportBlock(12-34)
Modules/Core/Services/ReportTemplateService.php (3)
Modules/Core/Database/Factories/ReportBlockFactory.php (1)
width(49-54)Modules/Core/Enums/ReportBlockWidth.php (1)
getGridWidth(15-23)Modules/Core/DTOs/BlockDTO.php (1)
setSlug(180-185)
Modules/Core/Tests/Feature/ReportBuilderBlockWidthTest.php (3)
Modules/Core/Models/ReportBlock.php (1)
ReportBlock(12-34)Modules/Core/Database/Factories/ReportBlockFactory.php (1)
width(49-54)Modules/Core/Enums/ReportBlockWidth.php (1)
getGridWidth(15-23)
Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php (3)
Modules/Core/Models/ReportBlock.php (1)
ReportBlock(12-34)Modules/Core/Tests/AbstractAdminPanelTestCase.php (1)
AbstractAdminPanelTestCase(10-42)Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php (9)
Test(29-58)Test(60-90)Test(92-112)Test(114-138)Test(140-168)Test(170-193)Test(195-226)Test(228-262)setUp(22-27)
Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php (3)
Modules/Core/Models/ReportBlock.php (1)
ReportBlock(12-34)Modules/Core/Services/BaseService.php (1)
update(93-104)Modules/Core/Services/ReportBlockService.php (2)
ReportBlockService(11-140)saveBlockFields(74-95)
Modules/Core/Database/Factories/ReportBlockFactory.php (4)
Modules/Core/Models/ReportBlock.php (1)
ReportBlock(12-34)Modules/Core/Services/ReportBlockService.php (1)
model(13-16)Modules/Core/Database/Factories/ReportTemplateFactory.php (3)
definition(15-28)system(30-35)inactive(37-42)Modules/Core/DTOs/BlockDTO.php (1)
system(66-77)
Modules/Core/Transformers/BlockTransformer.php (1)
Modules/Core/DTOs/BlockDTO.php (2)
setSlug(180-185)getSlug(122-125)
Modules/Core/Models/ReportTemplate.php (2)
Modules/Core/Models/ReportBlock.php (1)
newFactory(30-33)Modules/Core/Database/Factories/ReportTemplateFactory.php (1)
ReportTemplateFactory(11-57)
Modules/Core/Database/Migrations/2026_01_01_184544_create_report_blocks_table.php (1)
Modules/Core/Filament/Admin/Resources/ReportBlocks/ReportBlockResource.php (1)
table(27-30)
🪛 Biome (2.1.2)
resources/css/filament/company/nord.css
[error] 419-419: expected } but instead the file ends
the file ends here
(parse)
🪛 LanguageTool
.github/REPORT_BUILDER_ENHANCEMENTS.md
[grammar] ~22-~22: Ensure spelling is correct
Context: ...n()inReportBuilder.phpto properly lookup blocks usingblock_type` - Added prope...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~198-~198: Use a hyphen to join words.
Context: ...esign-report-template.blade.php- Fixed width rendering -Modules/Core/resource...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
.github/REPORT_BUILDER_ENHANCEMENTS.md
132-132: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
Modules/Core/Database/Factories/ReportTemplateFactory.php
32-32: Avoid unused parameters such as '$attributes'. (undefined)
(UnusedFormalParameter)
39-39: Avoid unused parameters such as '$attributes'. (undefined)
(UnusedFormalParameter)
46-46: Avoid unused parameters such as '$attributes'. (undefined)
(UnusedFormalParameter)
53-53: Avoid unused parameters such as '$attributes'. (undefined)
(UnusedFormalParameter)
Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php
41-41: Avoid unused local variables such as '$block'. (undefined)
(UnusedLocalVariable)
243-243: Avoid unused local variables such as '$block'. (undefined)
(UnusedLocalVariable)
Modules/Core/Database/Factories/ReportBlockFactory.php
37-37: Avoid unused parameters such as '$attributes'. (undefined)
(UnusedFormalParameter)
44-44: Avoid unused parameters such as '$attributes'. (undefined)
(UnusedFormalParameter)
51-51: Avoid unused parameters such as '$attributes'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (42)
.junie/guidelines.md (2)
400-476: Verify no duplication of the Internationalization section.The AI summary indicates this section appears "in two duplicated blocks within the document," but the provided context shows only one occurrence. This could be a false positive in the summary or an actual duplication issue that violates DRY for documentation. Please verify the full file to confirm there are no duplicate blocks.
If there is actual duplication, consolidate to a single authoritative section and remove the duplicate.
400-476: Excellent clarity on i18n patterns.The new Internationalization & Translations section provides clear, actionable guidance aligned with the PR's i18n implementation (125 new translation keys, switched to
trans()). The examples are practical and the coverage checklist (form fields, placeholders, helper text, etc.) gives developers a concrete reference. Documentation placement is logical and the patterns shown match the implementation described in the PR objectives.tailwind.config.js (1)
9-54: LGTM!The safelist configuration is appropriate for preserving dynamically-generated classes used by the Report Builder. The inclusion of grid system, Filament semantic colors, and utility classes ensures reliable class discovery during runtime rendering.
Modules/Core/Enums/ReportBlockWidth.php (3)
7-7: LGTM!The ONE_THIRD case expands the width system appropriately for 1/3-width blocks.
9-9: LGTM!The TWO_THIRDS case completes the width system for 2/3-width blocks.
12-23: LGTM!The
getGridWidth()method correctly maps enum cases to 12-column grid widths. The math is accurate (4/12 = 1/3, 6/12 = 1/2, 8/12 = 2/3, 12/12 = full), and the match expression is exhaustive.Modules/Core/Enums/ReportBlockType.php (1)
1-37: LGTM!The
ReportBlockTypeenum is well-implemented:
- Follows the internationalization guidelines by using
trans()for all user-facing text- Match expressions are exhaustive and cover all enum cases
- Translation keys follow the
ip.prefix convention with snake_case naming- Method names (
getLabel(),getDescription()) are clear and descriptive.github/copilot-instructions.md (1)
205-244: The Internationalization & Translations section is not duplicated. A search of the file reveals only a single occurrence of this section at line 205, with no duplicate elsewhere in the document. The subsection headers (Translation Key Conventions, UI Text Translation Requirements) are part of the same section, not separate duplicates.Likely an incorrect or invalid review comment.
Modules/Core/Database/Factories/ReportTemplateFactory.php (1)
30-56: LGTM!The state methods follow Laravel factory conventions correctly. The unused
$attributesparameter flagged by PHPMD is a false positive—it's part of the standard signature for factory state callbacks, even when not accessed.resources/css/filament/company/nord.css (2)
331-419: Report Builder CSS classes look good (pending syntax fix).The new CSS utility classes for the Report Builder are well-structured and follow a consistent naming convention. The width classes correctly map to grid column spans for a 2-column layout (both
fullandtwo-thirdsusecol-span-2which is appropriate). However, ensure the syntax error flagged above is resolved first.
1-419: [No action required] CSS syntax is valid and all braces are properly balanced (63 opening braces, 63 closing braces). The file structure is complete with no missing closing braces.Likely an incorrect or invalid review comment.
Modules/Core/Database/Migrations/2026_01_01_184544_create_report_blocks_table.php (1)
18-19: LGTM!The migration correctly follows coding guidelines by using string columns instead of ENUM types. The width comment accurately documents the new options (one_third, half, two_thirds, full), and the data_source default of 'company' aligns with the new
ReportDataSource::COMPANYenum case.Modules/Core/Enums/ReportDataSource.php (1)
1-18: LGTM!Clean and straightforward enum implementation. The string-backed cases provide type safety for data sources across the Report Builder feature. The snake_case values are consistent and the enum covers a comprehensive set of data source types.
Modules/Core/Tests/Unit/ReportBlockWidthTest.php (1)
17-31: LGTM!Test follows all coding guidelines: uses
#[Test]attribute,it_prefix naming convention, extendsAbstractTestCase, and includes proper section comments (/* Arrange */,/* Act */,/* Assert */).Modules/Core/Models/ReportTemplate.php (1)
56-63: LGTM!Factory integration follows the established pattern used by
ReportBlockmodel and Laravel conventions. ThenewFactory()method correctly returns the corresponding factory instance.Modules/Core/Database/Factories/ReportBlockFactory.php (2)
17-33: LGTM!Factory definition correctly uses enum cases for type-safe attribute generation. Slug generation with random suffix (
{slugified-name}-{random8chars}) aligns with the slug-based lookup requirements from prior reviews.
35-54: Static analysis warnings are false positives.The unused
$attributesparameter in state callbacks is required by Laravel's factory API signature. PHPMD incorrectly flags this as an issue. No changes needed.Modules/Core/Enums/ReportBand.php (1)
31-54: Inconsistent color semantic pairing between background and border classes.The background and border colors use mismatched semantic names (e.g., HEADER uses
bg-successbutborder-warning, GROUP_HEADER usesbg-infobutborder-danger). This appears unintentional and may confuse maintainers or cause visual inconsistency.If intentional for design contrast, consider adding a comment explaining the rationale. Otherwise, align the color semantics:
🔎 Suggested aligned colors
public function getBorderColorClass(): string { return match ($this) { - self::HEADER => 'border-warning-700 dark:border-warning-800', - self::GROUP_HEADER => 'border-danger-700 dark:border-danger-800', + self::HEADER => 'border-success-700 dark:border-success-800', + self::GROUP_HEADER => 'border-info-700 dark:border-info-800', self::DETAILS => 'border-primary-700 dark:border-primary-800', - self::GROUP_FOOTER => 'border-success-700 dark:border-success-800', - self::FOOTER => 'border-info-700 dark:border-info-800', + self::GROUP_FOOTER => 'border-info-700 dark:border-info-800', + self::FOOTER => 'border-success-700 dark:border-success-800', }; }config/report-fields.php (1)
3-117: LGTM on overall structure.The flat array structure with
sourceproperty for grouping is clean, and translation keys are consistently applied. Field definitions cover all major data sources comprehensively.Modules/Core/Database/Seeders/ReportBlocksSeeder.php (1)
18-83: LGTM on enum adoption.Block definitions correctly use enum constants (
ReportBlockType::ADDRESS,ReportDataSource::COMPANY,ReportBand::GROUP_HEADER, etc.) instead of string literals, aligning with prior review feedback.Modules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php (2)
11-13: Verify$thisandproperty_existsusage in Blade context.In a Blade view included within a Livewire component,
$thismay not always be the component instance, andproperty_existschecks object properties, not dynamic Livewire data. This could silently fail to load existing fields.Consider using Livewire's data passing instead:
// In the Livewire component, pass fields to the view: // return view('...fields-canvas', ['initialCanvasFields' => $this->data['fields'] ?? []]);Then in Blade, simply use
$initialCanvasFieldsdirectly.
25-37: Field deduplication prevents adding the same field twice.The check
!this.canvasFields.find(f => f.id === fieldId)prevents duplicate fields on the canvas. This is intentional behavior but worth noting—if users need the same field multiple times (e.g., in different positions), this would need adjustment.Modules/Core/Transformers/BlockTransformer.php (1)
58-58: LGTM! Slug handling correctly integrated.The slug field is properly added to both serialization paths with appropriate null handling, maintaining symmetry between
toDTO()andtoArray().Also applies to: 81-81
Modules/Core/Models/ReportBlock.php (2)
5-5: LGTM! Factory integration follows Laravel conventions.The
HasFactorytrait andnewFactory()method are correctly implemented following Laravel's factory pattern.Also applies to: 14-14, 27-33
21-24: LGTM! Enum casts properly configured.Enum casts for
block_type,data_source, anddefault_bandalign with coding guidelines requiring enum fields to use$casts.Modules/Core/Filament/Admin/Resources/ReportBlocks/Schemas/ReportBlockForm.php (1)
21-55: LGTM! Form properly internationalized and enum-driven.All labels use
trans()for internationalization, enum classes are used for Select options, and the new fields_canvas ViewField is properly integrated within a collapsible section.Modules/Core/Services/ReportTemplateService.php (2)
295-310: LGTM! Enum-driven width and slug propagation implemented correctly.The changes correctly integrate enum methods and slug-based lookups:
getGridWidth()replaces static width mapping- Slug is properly propagated to system blocks via
createSystemBlock- Enum values are extracted using
.valueproperty fordata_sourceanddefault_band
353-382: LGTM! System block creation signature extended correctly.The method signature now accepts
?string $slugand properly assigns it viasetSlug($slug), enabling slug-based block lookups throughout the system.Modules/Core/resources/views/filament/admin/resources/report-template-resource/pages/design-report-template.blade.php (4)
4-19: LGTM! Excellent refactor to enum-driven configuration.The bands configuration is now dynamically built from
ReportBand::cases(), eliminating hard-coded values and ensuring consistency with the enum definition. This addresses the previous review concerns about hard-coded colors.
180-190: LGTM! Theme-aware styling successfully implemented.Band styling now uses
colorClassandborderClassfrom theReportBandenum, making the UI fully theme-aware and removing all hard-coded color values. This successfully addresses the concerns raised in previous reviews.
225-225: LGTM! Grid span calculation simplified.The grid-column span logic is now simplified to check
block.position.width >= 8for 2-column span, else 1-column. This is much more readable than the previous nested ternary.
232-232: LGTM! Slug-based block lookup implemented.The
configureBlockaction now correctly usesblock.slugfor lookups, aligning with the slug-based architecture introduced in this PR.Also applies to: 249-249
Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php (2)
18-27: LGTM! Test setup follows coding guidelines.The test class correctly:
- Extends
AbstractAdminPanelTestCaseas required- Uses
Storage::fake('local')for isolation- Sets up the service in
setUp()
29-58: LGTM! Test structure follows all coding guidelines.The test correctly:
- Uses
#[Test]attribute- Follows
it_naming convention- Includes proper Arrange/Act/Assert comments
- Is marked incomplete as per requirements
Modules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.php (4)
20-40: LGTM! Integration test setup follows best practices.The test setup correctly:
- Creates persisted test data (Company, ReportTemplate)
- Uses
Storage::fake('local')for isolation- Follows coding guidelines for test structure
42-69: LGTM! Integration test structure follows all coding guidelines.The test correctly:
- Uses
#[Test]attribute- Follows
it_naming convention- Uses factory to create persisted blocks (correct for integration tests)
- Includes proper Arrange/Act/Assert comments
- Is marked incomplete as per requirements
73-107: LGTM! Good separation of concerns test.This test correctly validates that block metadata updates (name, width) are handled separately from field configuration, which is important for the architecture where fields are stored in JSON files while block metadata is in the database.
291-324: LGTM! Comprehensive width compatibility test.This test validates that the field storage system works correctly with all
ReportBlockWidthenum values (ONE_THIRD, HALF, TWO_THIRDS, FULL), ensuring the JSON storage mechanism is decoupled from block width settings.Modules/Core/Tests/Feature/ReportBuilderBlockWidthTest.php (1)
1-218: LGTM - Comprehensive test coverage for block width behavior.The test suite is well-structured and provides thorough coverage of the
ReportBlockWidthenum'sgetGridWidth()mappings and rendering behavior. All coding guidelines are followed:
- Extends
AbstractAdminPanelTestCase✓- Uses
#[Test]attributes ✓- Proper
it_*test naming ✓- Arrange/Act/Assert sections ✓
- Tests marked incomplete as required ✓
Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php (2)
1-260: Good test coverage for block edit behavior.The test suite comprehensively covers block lookup, data population, and configuration scenarios. All coding guidelines are followed correctly.
90-112: Remove the instanceof BackedEnum checks from the test—they are unnecessary.In Laravel 12+, when a model property is cast to an enum class via
$casts, callingtoArray()automatically serializes the enum to its scalar value. Theinstanceof \BackedEnumcheck at lines 105-107 will never execute because$data['width']is already the string'two_thirds', not the enum object. This is confirmed by the casting behavior: the ReportBlock model castswidthtoReportBlockWidth::class, which means the attribute is automatically converted to its backed value ('two_thirds') during serialization.Simplify the test to:
/* Act */ $data = $block->toArray(); /* Assert */ $this->assertEquals('two_thirds', $data['width']);Likely an incorrect or invalid review comment.
resources/lang/en/ip.php (1)
927-1052: LGTM - Comprehensive translation coverage for Report Builder.The translation additions are well-organized and comprehensive:
- 100+ field labels covering all data sources (company, customer, invoice, quote, payment, project, task, expense, relation, sumex)
- Clear block form labels for UI configuration
- Descriptive block type translations with helper text
- Consistent naming conventions throughout
- Professional, clear English translations
These translations align well with the Report Builder enhancements described in the PR objectives and support the new field canvas UI and block configuration features.
| ## Database Changes | ||
|
|
||
| ### Migration: report_blocks table | ||
| Added `config` column for storing block configuration: | ||
| ```php | ||
| $table->text('config')->nullable(); // JSON configuration | ||
| ``` | ||
|
|
||
| Updated width column comment: | ||
| ```php | ||
| $table->string('width')->default('half'); // one_third, half, two_thirds, or full | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Update documentation to reflect config column removal.
The documentation states that a config column was added to report_blocks, but per the past review comments and commit 606b68c, the config column was explicitly removed. Block configuration is now exclusively managed through JSON files in storage/app/report_blocks/. This section needs to be updated or removed.
🔎 Suggested documentation fix
## Database Changes
### Migration: report_blocks table
-Added `config` column for storing block configuration:
-```php
-$table->text('config')->nullable(); // JSON configuration
-```
-
-Updated width column comment:
+Updated default values and comments:
```php
$table->string('width')->default('half'); // one_third, half, two_thirds, or full
+$table->string('data_source')->default('company');+Note: Block field configurations are stored as JSON files in storage/app/report_blocks/{slug}.json rather than in a database column.
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In .github/REPORT_BUILDER_ENHANCEMENTS.md around lines 106–118, the doc
incorrectly states a removed config column was added to report_blocks;
remove the snippet that adds the config column, update the migration section
to reflect the actual migration changes (clarify updated default values/comments
for width and add the data_source default), and add a short note that block
field configurations are stored as JSON files in
storage/app/report_blocks/{slug}.json rather than in a DB column.
</details>
<!-- fingerprinting:phantom:poseidon:puma -->
<!-- This is an auto-generated reply by CodeRabbit -->
There was a problem hiding this comment.
@copilot In .github/REPORT_BUILDER_ENHANCEMENTS.md around lines 106–118, the doc
incorrectly states a removed config column was added to report_blocks;
remove the snippet that adds the config column, update the migration section
to reflect the actual migration changes (clarify updated default values/comments
for width and add the data_source default), and add a short note that block
field configurations are stored as JSON files in
storage/app/report_blocks/{slug}.json rather than in a DB column.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Updated documentation to remove the incorrect config column snippet. Added clarification that block field configurations are stored in JSON files at storage/app/report_blocks/{slug}.json rather than in the database. Updated migration section to reflect actual changes (width and data_source defaults). (commit 678f583)
| public function saveBlockFields(ReportBlock $block, array $fields): void | ||
| { | ||
| // Ensure directory exists | ||
| if (!Storage::disk('local')->exists('report_blocks')) { | ||
| Storage::disk('local')->makeDirectory('report_blocks'); | ||
| } | ||
|
|
||
| // Load existing config from JSON file if it exists, otherwise start fresh | ||
| $filename = $block->filename ?: $block->slug; | ||
| $path = 'report_blocks/' . $filename . '.json'; | ||
|
|
||
| $config = []; | ||
| if (Storage::disk('local')->exists($path)) { | ||
| $content = Storage::disk('local')->get($path); | ||
| $config = json_decode($content, true) ?? []; | ||
| } | ||
|
|
||
| $config['fields'] = $fields; | ||
|
|
||
| // Save to JSON file | ||
| Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT)); | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON operations.
The method lacks try-catch blocks around json_decode operations. If the JSON file is malformed, the operation will silently fail or produce unexpected results. Additionally, line 88 doesn't verify that the decoded content is actually an array.
🔎 Recommended improvements
public function saveBlockFields(ReportBlock $block, array $fields): void
{
// Ensure directory exists
if (!Storage::disk('local')->exists('report_blocks')) {
Storage::disk('local')->makeDirectory('report_blocks');
}
// Load existing config from JSON file if it exists, otherwise start fresh
$filename = $block->filename ?: $block->slug;
$path = 'report_blocks/' . $filename . '.json';
$config = [];
if (Storage::disk('local')->exists($path)) {
- $content = Storage::disk('local')->get($path);
- $config = json_decode($content, true) ?? [];
+ try {
+ $content = Storage::disk('local')->get($path);
+ $decoded = json_decode($content, true, 512, JSON_THROW_ON_ERROR);
+ $config = is_array($decoded) ? $decoded : [];
+ } catch (\JsonException $e) {
+ // Log error and start with empty config
+ \Illuminate\Support\Facades\Log::warning('Failed to decode block config JSON', [
+ 'path' => $path,
+ 'error' => $e->getMessage(),
+ ]);
+ $config = [];
+ }
}
$config['fields'] = $fields;
// Save to JSON file
Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT));
}🤖 Prompt for AI Agents
In Modules/Core/Services/ReportBlockService.php around lines 74 to 95, the JSON
read/write lacks error handling and does not verify the decoded content is an
array; wrap the storage read and json_decode in a try-catch, check json_decode
returned value and json_last_error() to handle malformed JSON (log or throw a
clear exception and fallback to an empty array if appropriate), and also wrap
the storage put in a try-catch to handle write errors; ensure you only assign
$config['fields'] after validating $config is an array (coerce to [] when
invalid) and include descriptive error logging when exceptions occur.
There was a problem hiding this comment.
@copilot In Modules/Core/Services/ReportBlockService.php around lines 74 to 95, the JSON
read/write lacks error handling and does not verify the decoded content is an
array; wrap the storage read and json_decode in a try-catch, check json_decode
returned value and json_last_error() to handle malformed JSON (log or throw a
clear exception and fallback to an empty array if appropriate), and also wrap
the storage put in a try-catch to handle write errors; ensure you only assign
$config['fields'] after validating $config is an array (coerce to [] when
invalid) and include descriptive error logging when exceptions occur.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Enhanced saveBlockFields() with comprehensive error handling:
- Wrapped Storage operations in try-catch blocks
- Validates decoded JSON is array using
json_last_error() - Ensures
$configis array before assigning fields - Logs descriptive errors for malformed JSON and I/O failures
- Throws RuntimeException on write errors with context
- Fallbacks to empty array when JSON is invalid
(commit 678f583)
| #[Test] | ||
| #[Group('feature')] | ||
| public function it_logs_block_data_for_debugging(): void | ||
| { | ||
| /* Arrange */ | ||
| Log::shouldReceive('info') | ||
| ->twice() | ||
| ->with('Block data for edit:', \Mockery::type('array')) | ||
| ->andReturnNull(); | ||
|
|
||
| Log::shouldReceive('info') | ||
| ->twice() | ||
| ->with('Mounting block config with data:', \Mockery::type('array')) | ||
| ->andReturnNull(); | ||
|
|
||
| $block = ReportBlock::factory()->create([ | ||
| 'block_type' => 'test_logging', | ||
| 'name' => 'Test Logging Block', | ||
| ]); | ||
|
|
||
| /* Act */ | ||
| $data = $block->toArray(); | ||
| Log::info('Block data for edit:', $data); | ||
| Log::info('Mounting block config with data:', $data); | ||
|
|
||
| /* Assert */ | ||
| $this->assertTrue(true); // Log assertions are handled by shouldReceive | ||
| $this->markTestIncomplete('Test implementation complete but marked incomplete as per requirements'); | ||
| } |
There was a problem hiding this comment.
Fix mock expectation mismatch - test will fail.
The mock expectations on lines 145-146 and 150-152 use ->twice(), but the Act section (lines 162-163) only calls Log::info() once for each message. This will cause the test to fail with an expectation mismatch.
🔎 Proposed fix
Log::shouldReceive('info')
- ->twice()
+ ->once()
->with('Block data for edit:', \Mockery::type('array'))
->andReturnNull();
Log::shouldReceive('info')
- ->twice()
+ ->once()
->with('Mounting block config with data:', \Mockery::type('array'))
->andReturnNull();🤖 Prompt for AI Agents
In Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php around lines
140-168, the Log::shouldReceive expectations use ->twice() but the Act section
only calls each Log::info once, causing a mock expectation mismatch; update both
Log::shouldReceive()->twice() calls to ->once() (or alternatively add a second
Log::info call in the Act section) so the mock expectations match the actual
calls.
There was a problem hiding this comment.
@copilot In Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php around lines
140-168, the Log::shouldReceive expectations use ->twice() but the Act section
only calls each Log::info once, causing a mock expectation mismatch; update both
Log::shouldReceive()->twice() calls to ->once() (or alternatively add a second
Log::info call in the Act section) so the mock expectations match the actual
calls.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Fixed mock expectations changed from ->twice() to ->once() to match the actual Log::info calls in the Act section. (commit 678f583)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| #[Test] | ||
| #[Group('feature')] | ||
| public function it_logs_block_data_for_debugging(): void | ||
| { | ||
| /* Arrange */ | ||
| Log::shouldReceive('info') | ||
| ->twice() | ||
| ->with('Block data for edit:', \Mockery::type('array')) | ||
| ->andReturnNull(); | ||
|
|
||
| Log::shouldReceive('info') | ||
| ->twice() | ||
| ->with('Mounting block config with data:', \Mockery::type('array')) | ||
| ->andReturnNull(); | ||
|
|
||
| $block = ReportBlock::factory()->create([ | ||
| 'block_type' => 'test_logging', | ||
| 'name' => 'Test Logging Block', | ||
| ]); | ||
|
|
||
| /* Act */ | ||
| $data = $block->toArray(); | ||
| Log::info('Block data for edit:', $data); | ||
| Log::info('Mounting block config with data:', $data); | ||
|
|
||
| /* Assert */ | ||
| $this->assertTrue(true); // Log assertions are handled by shouldReceive | ||
| $this->markTestIncomplete('Test implementation complete but marked incomplete as per requirements'); | ||
| } |
There was a problem hiding this comment.
@copilot In Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php around lines
140-168, the Log::shouldReceive expectations use ->twice() but the Act section
only calls each Log::info once, causing a mock expectation mismatch; update both
Log::shouldReceive()->twice() calls to ->once() (or alternatively add a second
Log::info call in the Act section) so the mock expectations match the actual
calls.
| ## Database Changes | ||
|
|
||
| ### Migration: report_blocks table | ||
| Added `config` column for storing block configuration: | ||
| ```php | ||
| $table->text('config')->nullable(); // JSON configuration | ||
| ``` | ||
|
|
||
| Updated width column comment: | ||
| ```php | ||
| $table->string('width')->default('half'); // one_third, half, two_thirds, or full | ||
| ``` | ||
|
|
There was a problem hiding this comment.
@copilot In .github/REPORT_BUILDER_ENHANCEMENTS.md around lines 106–118, the doc
incorrectly states a removed config column was added to report_blocks;
remove the snippet that adds the config column, update the migration section
to reflect the actual migration changes (clarify updated default values/comments
for width and add the data_source default), and add a short note that block
field configurations are stored as JSON files in
storage/app/report_blocks/{slug}.json rather than in a DB column.
| public function saveBlockFields(ReportBlock $block, array $fields): void | ||
| { | ||
| // Ensure directory exists | ||
| if (!Storage::disk('local')->exists('report_blocks')) { | ||
| Storage::disk('local')->makeDirectory('report_blocks'); | ||
| } | ||
|
|
||
| // Load existing config from JSON file if it exists, otherwise start fresh | ||
| $filename = $block->filename ?: $block->slug; | ||
| $path = 'report_blocks/' . $filename . '.json'; | ||
|
|
||
| $config = []; | ||
| if (Storage::disk('local')->exists($path)) { | ||
| $content = Storage::disk('local')->get($path); | ||
| $config = json_decode($content, true) ?? []; | ||
| } | ||
|
|
||
| $config['fields'] = $fields; | ||
|
|
||
| // Save to JSON file | ||
| Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT)); | ||
| } |
There was a problem hiding this comment.
@copilot In Modules/Core/Services/ReportBlockService.php around lines 74 to 95, the JSON
read/write lacks error handling and does not verify the decoded content is an
array; wrap the storage read and json_decode in a try-catch, check json_decode
returned value and json_last_error() to handle malformed JSON (log or throw a
clear exception and fallback to an empty array if appropriate), and also wrap
the storage put in a try-catch to handle write errors; ensure you only assign
$config['fields'] after validating $config is an array (coerce to [] when
invalid) and include descriptive error logging when exceptions occur.
…, and add error handling - Fixed ReportBuilderBlockEditTest mock expectations from ->twice() to ->once() to match actual Log::info calls - Added slug preservation in BlockDTO::clonedFrom() method (placed after setType as requested) - Updated REPORT_BUILDER_ENHANCEMENTS.md to correctly reflect no config column in migration - Added note about JSON file storage location for block field configurations - Enhanced ReportBlockService::saveBlockFields() with comprehensive error handling: * Wrapped Storage operations in try-catch blocks * Validate JSON decode results and check json_last_error() * Log descriptive errors when exceptions occur * Fallback to empty array for invalid JSON * Ensure config is array before assigning fields * Handle both read and write errors gracefully Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
| | | ||
| */ | ||
|
|
||
| ['id' => 'company_name', 'label' => 'ip.report_field_company_name', 'source' => 'company'], |
There was a problem hiding this comment.
@copilot does it make sense to group by source and then sort by the order the fields came from their tables? or alphabetically
There was a problem hiding this comment.
Grouping by source with alphabetical sorting is the better approach.
Current structure already groups fields by source (via the 'source' property in each field). For sorting within each source:
Alphabetical sorting is preferable because:
- Consistency: Users can quickly find fields in a predictable order
- Maintainability: Easy to add new fields without disrupting order
- UI/UX: The field canvas search/filter works better with alphabetical lists
- Multi-language: Alphabetical order works across different languages when translated
Table order would be less useful because:
- Database column order can change during migrations
- Different data sources may have different conventions
- Users don't necessarily think in terms of table structure
The current flat array structure with 'source' property is already optimal for Alpine.js. If you'd like, I can add a sorting step in ReportFieldService::getAvailableFields() to ensure alphabetical order within each source group.
Report Builder Refactoring - Complete Implementation ✅
Latest Fixes (Code Quality & Error Handling):
1. Fixed Test Mock Expectations (Comment 2656955550)
ReportBuilderBlockEditTestto use->once()instead of->twice()2. Preserved Slug in Cloned DTOs (Comment 2656957361)
$dto->setSlug($original->getSlug())inBlockDTO::clonedFrom()method3. Updated Documentation (Comment 2656959928)
.github/REPORT_BUILDER_ENHANCEMENTS.mdstorage/app/report_blocks/{slug}.json4. Enhanced Error Handling in ReportBlockService (Comment 2656960652)
json_last_error()andjson_decode()return values$configis array before accessing$config['fields']Error Handling Features:
Previous Enhancements:
Complete Enum System - ReportBlockWidth, ReportDataSource, ReportBand, ReportBlockType
Tailwind Reliability - Safelist + custom CSS classes + Filament native classes
Internationalization - 125 translation keys, all using trans()
Performance - Flat config structure, optimized field loading
Field Canvas - Drag/drop interface with JSON storage
Type Safety - All enums provide compile-time checking
Tests - 5 test files with 39 test cases (all marked incomplete)
Files Modified in This Update:
Technical Improvements:
✅ Robust error handling for file I/O operations
✅ Proper validation of JSON decode results
✅ Descriptive logging for debugging issues
✅ Graceful fallbacks for error scenarios
✅ Test mocks aligned with actual behavior
✅ Cloned blocks maintain slug integrity
✅ Accurate documentation reflects implementation
All 4 comments from code review addressed successfully!
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.