Temporary #348
Temporary #348
Conversation
📝 WalkthroughWalkthroughIntroduces a comprehensive report builder system enabling customizable invoice and quote template generation. Adds data structures (BlockDTO, GridPositionDTO), database models with relationships and factories, configuration enums, service layer for template and block management, Filament admin UI for drag-and-drop block editing, block rendering handlers, file-based template persistence, and extensive test coverage. Includes migrations, seeders, Blade templates, and a minor whitespace-handling improvement to Contact.php. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Filament as Filament Admin
participant ReportBuilder as ReportBuilder Page
participant RTS as ReportTemplateService
participant RBS as ReportBlockService
participant Repository as FileRepository
participant Storage as File Storage
User->>Filament: Navigate to Template Designer
Filament->>ReportBuilder: Load ReportTemplate
ReportBuilder->>RTS: loadBlocks(template)
RTS->>Repository: get(companyId, slug)
Repository->>Storage: Read template.json
Storage-->>Repository: Block data
Repository-->>RTS: Array of blocks
RTS-->>ReportBuilder: BlockDTOs with positions
User->>ReportBuilder: Drag system block to band
ReportBuilder->>RTS: cloneSystemBlock(type, newId, position)
RTS-->>ReportBuilder: BlockDTO with clonedFrom flag
User->>ReportBuilder: Configure block fields
ReportBuilder->>RBS: saveBlockFields(block, fields)
RBS->>Storage: Write fields JSON
User->>ReportBuilder: Save template layout
ReportBuilder->>RTS: persistBlocks(template, blocks)
RTS->>Repository: save(companyId, slug, grouped_blocks)
Repository->>Storage: Write template.json
Storage-->>Repository: Success
Repository-->>RTS: Persisted
RTS-->>ReportBuilder: Complete
ReportBuilder-->>User: Template saved
sequenceDiagram
participant App
participant ReportRenderer as ReportRenderer
participant RTS as ReportTemplateService
participant BlockFactory as BlockFactory
participant Handler as Block Handler
participant Storage as Storage
App->>ReportRenderer: renderToHtml(template, invoice)
ReportRenderer->>RTS: loadBlocks(template)
RTS->>Storage: Load blocks from file
Storage-->>RTS: Block array
RTS-->>ReportRenderer: BlockDTOs sorted by position
loop For each block by band
ReportRenderer->>BlockFactory: make(block.type)
BlockFactory-->>ReportRenderer: Handler instance
ReportRenderer->>Handler: render(blockDTO, invoice, company)
Handler-->>ReportRenderer: HTML string
end
ReportRenderer->>ReportRenderer: Wrap in PDF HTML template
ReportRenderer-->>App: Rendered HTML
sequenceDiagram
participant User
participant Designer as ReportBuilder
participant Service as ReportTemplateService
participant GridSnapper as GridSnapperService
participant Validator as Validator
User->>Designer: Click "Clone Block"
Designer->>Service: cloneSystemBlock(blockType, newId, position)
Service->>Service: Get system block definition
Service->>GridSnapper: snap(position)
GridSnapper-->>Service: Snapped GridPositionDTO
Service->>Validator: validateBlocks([cloned_block])
Validator->>Validator: Check id, type, position presence
Validator->>GridSnapper: validate(position)
GridSnapper-->>Validator: Valid/Invalid
Validator-->>Service: Result
Service->>Service: Create BlockDTO with clonedFrom metadata
Service-->>Designer: New BlockDTO
Designer->>Designer: Add to appropriate band
Designer-->>User: Block cloned and positioned
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This is a substantial feature addition introducing 60+ new files with complex interdependent logic. The changes span multiple architectural layers (DTOs, models, services, repositories, handlers, UI components) with intricate workflows for template management, block rendering, grid snapping validation, and file persistence. The reviewer must understand the report builder domain, validate service layer orchestration, examine enum usage across the codebase, verify Filament integration patterns, assess block handler implementations, and validate comprehensive test coverage. The heterogeneous nature (different file types: models, services, handlers, views, templates, migrations) with varying logic density across each component demands careful, methodical review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In @Modules/Core/Database/Seeders/ReportBlocksSeeder.php:
- Around line 100-106: ReportBlocksSeeder is accessing $block['config'] without
checking it exists, causing undefined key and later fatal when setting
['fields']; change the assignment to safely handle missing/non-array config—use
a null-coalescing or existence check to set $config to an array when
$block['config'] is absent (e.g., $config = isset($block['config']) &&
is_array($block['config']) ? $block['config'] : []), then set $config['fields']
= []; and continue to write the JSON; update the block in the ReportBlocksSeeder
(the section where $path, $config and Storage::disk('local')->put are used) so
it never tries to index into null.
In
@Modules/Core/Filament/Admin/Resources/ReportBlocks/Tables/ReportBlocksTable.php:
- Around line 5-7: The imports for ActionGroup, DeleteAction, and EditAction are
using the page-actions namespace (Filament\Actions\*) but these are table row
actions; update their use statements to the table-actions namespace
(Filament\Tables\Actions\ActionGroup, Filament\Tables\Actions\DeleteAction,
Filament\Tables\Actions\EditAction) so the ReportBlocksTable class uses the
correct action classes for table rows.
In
@Modules/Core/Filament/Admin/Resources/ReportTemplates/Tables/ReportTemplatesTable.php:
- Around line 5-9: The file imports table action classes from the wrong
namespace (Filament\Actions\*); replace those imports with the table-specific
namespace (Filament\Tables\Actions\Action, Filament\Tables\Actions\ActionGroup,
Filament\Tables\Actions\DeleteAction, Filament\Tables\Actions\EditAction,
Filament\Tables\Actions\ViewAction) so the table record actions (where Action,
ActionGroup, DeleteAction, EditAction, ViewAction are referenced) use the
correct Filament Tables classes and avoid runtime errors.
In @Modules/Core/Handlers/DetailItemsBlockHandler.php:
- Line 59: The code is using the wrong relationship property
($invoice->invoice_items) which returns null; change it to use the model
relationship property $invoice->invoiceItems (the invoiceItems() relation) so
the actual items are iterated, and ensure callers eager-load that relation
(e.g., ->with('invoiceItems')) to avoid lazy-loading/N+1 queries when
DetailItemsBlockHandler iterates the items.
In @Modules/Core/Handlers/DetailItemTaxBlockHandler.php:
- Around line 62-64: DetailItemTaxBlockHandler is using non-existent
$invoice->tax_rates and $invoice->subtotal and applies the full subtotal to each
tax rate; change the logic to iterate invoice items (use the relation that
returns InvoiceItem records), for each item read its item_tax_rate_id and item
subtotal (invoice_item_subtotal or the item amount), look up the tax rate (via
the TaxRate model or an injected lookup), compute tax per item as item_subtotal
* (rate/100) and aggregate those per tax-rate id, then render the aggregated tax
amount for each tax rate instead of using $invoice->subtotal or
$invoice->tax_rates. Ensure references to $invoice->subtotal and
$invoice->tax_rates are removed and use the invoice items relation and
item_tax_rate_id/amount fields (and TaxRate->rate) to compute values.
In @Modules/Core/Handlers/FooterQrCodeBlockHandler.php:
- Line 35: The current img tag construction in FooterQrCodeBlockHandler
(building a URL to api.qrserver.com with $size and generateQrData output) must
be replaced: validate $size to be a sane positive integer within configured
bounds, stop sending raw invoice URLs to an external service, and generate the
QR locally using a PHP library (e.g., endroid/qr-code or bacon-qr-code) to
either emit a data:URI or store a protected local asset; wrap generation in
try/catch and on failure render a safe fallback (placeholder image or empty QR
container) so template rendering never breaks, and update generateQrData usage
to provide privacy-safe payloads (e.g., tokenized or hashed URL) or document the
privacy change per policy.
In @Modules/Core/resources/Templates/Views/templates/quotes/default.blade.php:
- Line 92: The template uses unescaped formatted_* properties that don't exist;
add accessors getFormattedAddressAttribute on the Customer model and
getFormattedDescriptionAttribute, getFormattedTermsAttribute,
getFormattedFooterAttribute on the Quote model, and make them return sanitized
HTML (use e() to escape, convert newlines with nl2br if needed, then wrap in
Illuminate\Support\HtmlString) so the {!! !!} output is safe; import HtmlString
and ensure the accessor names match the properties
($quote->customer->formatted_address, $quote->formatted_description,
$quote->formatted_terms, $quote->formatted_footer).
In @Modules/Core/Services/ReportRenderer.php:
- Line 215: Update the template usage in ReportRenderer (where the invoice title
is generated) to reference the correct Invoice model property: replace the
twig/templated expression `{$invoice->number}` with `{$invoice->invoice_number}`
so it accesses the defined Invoice::invoice_number property (look for the title
line producing "Invoice {$invoice->number}" and swap to the correct property
name).
In @phpunit.txt:
- Around line 39-51: NumberGenerator::generate() is returning null in
NumberingCompanyIsolationTest (so $firstNumber is null); inspect and fix the
generate flow used by
NumberGenerator::forNumberingId($numbering->id)->generate(): ensure the
numbering record has a valid last_id/default (set non-null default in factory or
DB) and that NumberGenerator::generate() handles a null/absent last_id by
initializing it (and always returns the formatted string), and verify the
formatting logic/path that builds the string (e.g., format template, pad logic)
returns a string not null; update the test factory to populate required fields
(last_id, format) or make generate() robust to missing fields so the call in
NumberingCompanyIsolationTest yields 'EXP-0001' rather than null.
🟠 Major comments (24)
Modules/Core/Handlers/FooterTotalsBlockHandler.php-42-44 (1)
42-44: Fixempty()checks on numeric fields that can be zero.Lines 42 and 54 use
empty()to check numeric values:
- Line 42:
!empty($invoice->discount)- A discount of0is valid but would be treated as empty- Line 54:
!empty($invoice->paid)- A paid amount of0is valid but would be treated as emptyIn PHP,
empty(0)returnstrue, which would incorrectly hide these rows when the value is legitimately zero. For numeric fields, useisset()or explicitly check fornull.🐛 Proposed fix for numeric empty checks
- if ( ! empty($config['show_discount']) && ! empty($invoice->discount)) { + if ( ! empty($config['show_discount']) && isset($invoice->discount)) { $html .= '<tr><td>Discount:</td><td>' . $this->formatCurrency($invoice->discount ?? 0, $invoice->currency_code) . '</td></tr>'; }- if ( ! empty($config['show_paid']) && ! empty($invoice->paid)) { + if ( ! empty($config['show_paid']) && isset($invoice->paid)) { $html .= '<tr><td>Paid:</td><td>' . $this->formatCurrency($invoice->paid ?? 0, $invoice->currency_code) . '</td></tr>'; }Also applies to: 54-56
Modules/Core/Tests/Unit/ReportBlockWidthTest.php-30-30 (1)
30-30: RemovemarkTestIncomplete()calls from fully implemented tests.All test methods are fully implemented with proper Arrange/Act/Assert sections and meaningful assertions, yet they're marked as incomplete on Lines 30, 46, 62, 78, 102, and 122. The
markTestIncomplete()method should only be used for tests that are not yet fully implemented or require additional work.These tests appear complete and should execute normally without being marked incomplete.
♻️ Remove markTestIncomplete calls
/* Assert */ $this->assertEquals('one_third', $width->value); $this->assertEquals(4, $gridWidth); - $this->markTestIncomplete('Test implementation complete but marked incomplete as per requirements'); }Apply the same change to all six test methods in this file.
Also applies to: 46-46, 62-62, 78-78, 102-102, 122-122
phpunit.txt-2-10 (1)
2-10: Fix data provider argument mismatch for Spain Facturae tests.The
handlerProviderdata provider supplies 2 arguments but the test methods only accept 1 parameter. This causes all tests to skip the "Facturae (Spain)" dataset, reducing test coverage.Update the test methods in
FormatHandlersTestto accept both parameters from the data provider, or adjust the provider to match the method signatures.Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php-38-61 (1)
38-61: Tests are incomplete and don't validate the edit action.All tests in this file are marked incomplete with contradictory messages ("implementation complete but marked incomplete"). These tests only validate basic Eloquent ORM behavior (CRUD,
toArray()) rather than the actual "Report Builder block edit action" described in the class docblock.Based on coding guidelines requiring meaningful tests that validate actual behavior:
- Remove
markTestIncomplete()calls if tests are truly complete- Add tests that actually exercise the edit action:
- Test form pre-population with block data
- Test form submission and updates
- Test validation and error handling
- Test the actual Filament edit page/action behavior
The current tests don't match the stated purpose of the test suite.
Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ListReportTemplates.php-18-33 (1)
18-33: Add error handling and user feedback to template creation.The action has several reliability concerns:
- Null company handling: If both
session('current_company_id')andauth()->user()->companies()->first()return null, the template creation will fail without a clear error message- No data validation: The action assumes
$data['name']and$data['template_type']exist without validation- Unused result: The created
$templateis not used—typically you'd redirect to the edit page or show a success notification- No error handling: If
createTemplate()throws an exception, it will bubble up without user-friendly messaging🔧 Suggested improvements
CreateAction::make() ->action(function (array $data) { $company = Company::query()->find(session('current_company_id')); if ( ! $company) { $company = auth()->user()->companies()->first(); } + + if ( ! $company) { + Notification::make() + ->title('No company found') + ->danger() + ->send(); + return; + } - $template = app(ReportTemplateService::class)->createTemplate( + $template = app(ReportTemplateService::class)->createTemplate( $company, $data['name'], $data['template_type'], [] ); + + Notification::make() + ->title('Template created successfully') + ->success() + ->send(); + + return redirect()->to(ReportTemplateResource::getUrl('edit', ['record' => $template])); }) ->modalWidth('full'),Committable suggestion skipped: line range outside the PR's diff.
phpunit.txt-152-330 (1)
152-330: Fix missing Filament 3.3 form actions component in settings template.The component
filament-panels::form.actionsdoes not exist in Filament 3.3.14, causing allSettingsTestcases to fail withViewException.In Filament 3.x, form actions are defined via
Action::make()in the Page class methods, not rendered through a Blade component. Remove the<x-filament-panels::form.actions>lines fromModules/Core/resources/views/filament/admin/pages/settings.blade.php(lines 5–8). Filament will render form actions automatically when properly defined in the corresponding Page class.Modules/Core/Tests/Unit/ReportTemplateTest.php-105-117 (1)
105-117: Move template creation to the arrange section.The template creation should be in the
/* arrange */section, and the actual method being tested (isCloneable()) should be called in the/* act */section with a variable assignment. This applies to several tests in this file.♻️ Proposed fix for proper test structure
#[Test] #[Group('unit')] public function is_cloneable_returns_true_when_active(): void { + /* arrange */ $template = ReportTemplate::create([ 'company_id' => $this->company->id, 'name' => 'Active Template', 'slug' => 'active_template', 'template_type' => 'invoice', 'is_system' => false, 'is_active' => true, ]); + /* act */ + $result = $template->isCloneable(); + + /* assert */ - $this->assertTrue($template->isCloneable()); + $this->assertTrue($result); }Apply the same pattern to tests on lines 121-133, 137-149, 153-165, and 169-182.
Modules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php-25-25 (1)
25-25: Escape field ID to prevent JavaScript injection.The field ID is interpolated directly into the JavaScript string without proper escaping. If the field ID contains quotes or special characters, it could break the JavaScript or lead to injection.
🔒 Proposed fix using JSON encoding
- @dragstart="event.dataTransfer.setData('fieldId', '{{ $field['id'] }}')" + @dragstart="event.dataTransfer.setData('fieldId', {{ json_encode($field['id']) }})"Modules/Core/Filament/Admin/Resources/ReportBlocks/Pages/ListReportBlocks.php-18-27 (1)
18-27: Add user feedback and error handling to the create action.The custom action calls
createReportBlock()but doesn't provide user feedback (success notification) or handle potential errors. Additionally, there's no redirect after creation, which may leave the user on the same page without visual confirmation.♻️ Proposed improvement with feedback and error handling
+ /** + * Get the header actions for the list page. + * + * @return array<\Filament\Actions\Action> + */ protected function getHeaderActions(): array { return [ CreateAction::make() ->action(function (array $data) { - app(ReportBlockService::class)->createReportBlock($data); + try { + $block = app(ReportBlockService::class)->createReportBlock($data); + + $this->getCreatedNotification()?->send(); + + return redirect()->route('filament.admin.resources.report-blocks.edit', ['record' => $block]); + } catch (\Exception $e) { + \Illuminate\Support\Facades\Log::error('Failed to create report block', ['error' => $e->getMessage()]); + throw $e; + } }) ->modalWidth('full'), ]; }Note: Verify that
createReportBlock()returns the created model instance.Committable suggestion skipped: line range outside the PR's diff.
Modules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php-11-11 (1)
11-11: Initialize the hidden input with existing data when editing.The hidden input is hardcoded to
value="[]", which means any existing field configuration will be lost when editing a block. The input should be initialized with the current field configuration.🐛 Proposed fix to support editing existing blocks
- <input type="hidden" name="data[fields_canvas]" value="[]"/> + <input type="hidden" name="data[fields_canvas]" value="{{ json_encode($record?->data['fields_canvas'] ?? []) }}"/>Additionally, update the JavaScript initialization to load existing fields:
function initFieldsCanvas() { return { - canvasFields: [], + canvasFields: @json($record?->data['fields_canvas'] ?? []), availableFields: @json($availableFields),Modules/Core/Tests/Feature/GridSnapperTest.php-66-69 (1)
66-69: Separate "Act" and "Assert" sections per coding guidelines.The coding guidelines require an explicit "act" section where variables are defined BEFORE assertions. Currently, act and assert are combined.
♻️ Suggested refactor
- /* Act & Assert */ - $this->assertTrue($this->gridSnapper->validate($validPosition)); - $this->assertFalse($this->gridSnapper->validate($invalidPositionX)); - $this->assertFalse($this->gridSnapper->validate($invalidPositionWidth)); + /* Act */ + $isValidPositionValid = $this->gridSnapper->validate($validPosition); + $isInvalidPositionXValid = $this->gridSnapper->validate($invalidPositionX); + $isInvalidPositionWidthValid = $this->gridSnapper->validate($invalidPositionWidth); + + /* Assert */ + $this->assertTrue($isValidPositionValid); + $this->assertFalse($isInvalidPositionXValid); + $this->assertFalse($isInvalidPositionWidthValid);As per coding guidelines, tests must have inline comment blocks with separate sections and variables defined in "Act" before assertions.
Modules/Core/Filament/Admin/Resources/ReportTemplates/ReportTemplateResource.php-32-36 (1)
32-36: Add docblock above thegetRelations()method.Per coding guidelines, Filament Resource methods require docblocks even when the method returns an empty array.
Based on learnings, docblocks are required for resource generation methods.
📝 Proposed docblock addition
+ /** + * Define the relations for this resource. + * + * Relations: none + */ public static function getRelations(): array { return [ ]; }Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/CreateReportTemplate.php-44-57 (1)
44-57: Add null safety check for company resolution.The company resolution logic has a potential null pointer issue. If
session('current_company_id')returns null andauth()->user()->companies()->first()also returns null (when a user has no associated companies), thecreateTemplatecall will receive a null company, likely causing a runtime error.🛡️ Proposed fix with proper null handling
protected function handleRecordCreation(array $data): Model { $company = Company::find(session('current_company_id')); if ( ! $company) { $company = auth()->user()->companies()->first(); } + + if ( ! $company) { + throw new \RuntimeException('No company found for the current user. Cannot create report template.'); + } return app(ReportTemplateService::class)->createTemplate( $company, $data['name'], $data['template_type'], [] ); }Modules/Core/Filament/Admin/Resources/ReportTemplates/ReportTemplateResource.php-22-25 (1)
22-25: Add docblock above theform()method.Per coding guidelines, Filament Resource methods require docblocks documenting their purpose and the fields they handle.
Based on learnings, docblocks are required for resource generation methods.
📝 Proposed docblock addition
+ /** + * Define the form schema for creating/editing report templates. + * + * Fields: name, template_type, is_active, is_system + */ public static function form(Schema $schema): Schema { return ReportTemplateForm::configure($schema); }Modules/Core/Filament/Admin/Resources/ReportBlocks/ReportBlockResource.php-27-30 (1)
27-30: Add docblock above thetable()method.Per coding guidelines, Filament Resource methods should have docblocks documenting the table columns and actions they provide.
Based on learnings, docblocks are required for resource generation methods.
📝 Proposed docblock addition
+ /** + * Define the table for listing report blocks. + * + * Columns: name, block_type, width, data_source, default_band, is_active, is_system + * Actions: edit, delete + */ public static function table(Table $table): Table { return ReportBlocksTable::configure($table); }Modules/Core/Filament/Admin/Resources/ReportTemplates/ReportTemplateResource.php-27-30 (1)
27-30: Add docblock above thetable()method.Per coding guidelines, Filament Resource methods require docblocks documenting the table columns and actions.
Based on learnings, docblocks are required for resource generation methods.
📝 Proposed docblock addition
+ /** + * Define the table for listing report templates. + * + * Columns: id, name, slug, template_type, is_system, is_active, created_at + * Actions: view, edit, design, clone, delete + */ public static function table(Table $table): Table { return ReportTemplatesTable::configure($table); }Modules/Core/Tests/Unit/BlockFactoryTest.php-10-14 (1)
10-14: Unit tests should extendAbstractTestCase, notAbstractAdminPanelTestCase.Per coding guidelines, unit tests should extend
AbstractTestCase. These tests don't require admin panel context.♻️ Proposed fix
-use Modules\Core\Tests\AbstractAdminPanelTestCase; +use Modules\Core\Tests\AbstractTestCase;-class BlockFactoryTest extends AbstractAdminPanelTestCase +class BlockFactoryTest extends AbstractTestCaseAs per coding guidelines, all tests must extend abstract test cases from
Modules/Core/Tests/.Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php-57-57 (1)
57-57: RemovemarkTestIncomplete()calls from completed tests.Each test ends with
markTestIncomplete()after valid assertions. This causes tests to be reported as incomplete rather than passed, obscuring actual results. Since the tests contain full implementations with assertions, remove these calls.🐛 Proposed fix (apply to all affected tests)
$this->assertEquals('company_name', $config['fields'][0]['id']); - $this->markTestIncomplete('Test implementation complete but marked incomplete as per requirements'); }Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php-9-18 (1)
9-18: Unit tests should extendAbstractTestCase, notAbstractAdminPanelTestCase.Per coding guidelines, unit tests in
Tests/Unit/should extendAbstractTestCase.AbstractAdminPanelTestCaseis intended for feature tests that require admin panel context. These tests only interact with services and storage, not the Filament admin panel.♻️ Proposed fix
-use Modules\Core\Tests\AbstractAdminPanelTestCase; +use Modules\Core\Tests\AbstractTestCase;-class ReportBlockServiceFieldsTest extends AbstractAdminPanelTestCase +class ReportBlockServiceFieldsTest extends AbstractTestCaseAs per coding guidelines, all tests must extend abstract test cases from
Modules/Core/Tests/.Modules/Core/Filament/Admin/Resources/ReportBlocks/Pages/EditReportBlock.php-36-52 (1)
36-52: Extract duplicated form data preparation logic.The
mountUsingandfillFormcallbacks contain identical logic for preparing form data. Extract this to a private method to eliminate duplication and ensure consistency.♻️ Proposed fix
+ private function prepareFormData(): array + { + $data = $this->record->toArray(); + $data['is_active'] = (bool) ($data['is_active'] ?? true); + + foreach (['width', 'block_type', 'data_source', 'default_band'] as $field) { + if (isset($data[$field]) && $data[$field] instanceof BackedEnum) { + $data[$field] = $data[$field]->value; + } + } + + return $data; + } + public function editAction(): Action { return Action::make('edit') ->label('Edit Modal') ->schema(fn (Schema $schema) => ReportBlockForm::configure($schema)) - ->mountUsing(function (Schema $schema) { - $data = $this->record->toArray(); - $data['is_active'] = (bool) ($data['is_active'] ?? true); - if (isset($data['width']) && $data['width'] instanceof BackedEnum) { - $data['width'] = $data['width']->value; - } - $schema->fill($data); + ->mountUsing(function (Schema $schema) { + $schema->fill($this->prepareFormData()); }) - ->fillForm(function () { - $data = $this->record->toArray(); - $data['is_active'] = (bool) ($data['is_active'] ?? true); - if (isset($data['width']) && $data['width'] instanceof BackedEnum) { - $data['width'] = $data['width']->value; - } - - return $data; + ->fillForm(function () { + return $this->prepareFormData(); })Modules/Core/Services/ReportBlockService.php-1-13 (1)
1-13: Add missingLogfacade import.The file uses
\Illuminate\Support\Facades\Loginline multiple times (lines 99, 105, 123, 130, 164, 195) instead of importing it at the top. Per coding guidelines, always use full namespace imports.♻️ Proposed fix
use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use JsonException;Then replace all
\Illuminate\Support\Facades\Log::calls withLog::.Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php-90-94 (1)
90-94: Extract repeated block lookup logic into a helper method.The same block lookup query appears three times (lines 90-94, 167-171, 240-244). This violates the DRY principle and makes maintenance harder.
♻️ Proposed refactor
Add a private helper method:
private function findBlockByIdentifier(string $blockIdentifier): ?ReportBlock { return ReportBlock::query() ->where('id', $blockIdentifier) ->orWhere('slug', $blockIdentifier) ->orWhere('block_type', $blockIdentifier) ->first(); }Then replace all three occurrences:
- $block = ReportBlock::query() - ->where('id', $blockSlug) - ->orWhere('slug', $blockSlug) - ->orWhere('block_type', $blockSlug) - ->first(); + $block = $this->findBlockByIdentifier($blockSlug);Also applies to: 167-171, 240-244
Modules/Core/DTOs/BlockDTO.php-68-79 (1)
68-79: System blocks created without an ID.The
system()factory method doesn't set anidfor the block, leaving it as an empty string (from the property default at line 39). This could cause issues when blocks are added to collections or persisted. Consider either requiring anidparameter or documenting that callers must set the ID after creation.💡 Suggested fix: require ID parameter
- public static function system(string $type, GridPositionDTO $position, array $config): self + public static function system(string $id, string $type, GridPositionDTO $position, array $config): self { $dto = new self(); + $dto->setId($id); $dto->setType($type); $dto->setPosition($position);Modules/Core/Tests/Unit/ReportTemplateServiceTest.php-230-240 (1)
230-240: Change mock method name fromloadtoget.The test mocks
method('load')at line 231, but the actualReportTemplateFileRepositoryhas aget()method, notload(). The service calls$this->fileRepository->get()(line 184 of ReportTemplateService.php), so the mock must match:->method('get')instead of:
->method('load')
🟡 Minor comments (12)
Modules/Core/resources/views/filament/admin/resources/report-template-resource/pages/design-report-template.blade.php-290-290 (1)
290-290: Remove debug background color.Line 290 has
background: #FF0000(red), which appears to be a debug artifact. This should either be removed or changed to the intended background color.🐛 Proposed fix
- <div class="bg-[#FF0000] dark:bg-gray-900"> + <div class="bg-white dark:bg-gray-900">Modules/Core/resources/views/filament/admin/resources/report-template-resource/pages/design-report-template.blade.php-304-304 (1)
304-304: Fix invalid Tailwind class.Line 304 has
font-[#429AE1]which is invalid Tailwind syntax. Thefont-prefix is for font-family or font-weight, not colors. This should likely befont-boldortext-[#429AE1]for color.🐛 Proposed fix
- class="text-sm font-[#429AE1] text-white uppercase tracking-tight overflow-hidden text-ellipsis w-full" + class="text-sm font-bold text-white uppercase tracking-tight overflow-hidden text-ellipsis w-full"Modules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.php-68-68 (1)
68-68: Complete tests withmarkTestIncomplete()calls need clarification.All 8 tests in this suite contain substantive implementations with valid assertions, but each ends with
$this->markTestIncomplete('Test implementation complete but marked incomplete as per requirements'). While the message indicates this was intentional, including completed yet marked-incomplete tests in a PR requires justification. Either remove themarkTestIncomplete()calls to allow tests to run, or document why these tests must remain incomplete during the PR review.Applies to lines: 68, 106, 132, 177, 206, 242, 286, 323
Modules/Core/Tests/Feature/UpdateReportTemplateTest.php-105-149 (1)
105-149: Test name suggests grid snapping but doesn't verify snapping behavior.The test
it_snaps_blocks_to_grid_on_updateuses already grid-aligned coordinates (x: 0, y: 0). To actually test snapping, the input should have non-grid-aligned values and assertions should verify they were adjusted.Consider renaming to
it_persists_blocks_with_valid_positionsor update the test to verify actual snapping behavior with non-aligned inputs.Modules/Core/Tests/Feature/BlockCloningTest.php-60-97 (1)
60-97: Add missing/* Act */section for test clarity.The test
it_identifies_system_templateslacks a clear/* Act */section. The creation and modification of the template should be in the Act section, with only data setup in Arrange.💚 Proposed fix
- /* arrange */ + /* Arrange */ $company = Company::factory()->create(); $user = User::factory()->create(); $user->companies()->attach($company); session(['current_company_id' => $company->id]); $systemBlocks = [ // ... block definition ]; + /* Act */ $template = $this->service->createTemplate( $company, 'System Template', 'invoice', $systemBlocks ); $template->is_system = true; $template->save(); - /* assert */ + /* Assert */ $this->assertTrue($template->is_system);Modules/Core/Tests/Feature/BlockCloningTest.php-36-57 (1)
36-57: Capitalize section comments.Per coding guidelines, section comments should use capitalized format:
/* Arrange */,/* Act */,/* Assert */.💚 Proposed fix
- /* arrange */ + /* Arrange */ $company = Company::factory()->create();- /* act */ + /* Act */ $clonedBlock = $this->service->cloneSystemBlock($blockType, $newId, $position);- /* assert */ + /* Assert */ $this->assertEquals($newId, $clonedBlock->getId());Modules/Core/Tests/Unit/BlockFactoryTest.php-94-104 (1)
94-104: Test name should be prefixed withit_.Per coding guidelines, unit test function names must be prefixed with
it_and make grammatical sense. Also, this test is missing section comments.💚 Proposed fix
#[Test] #[Group('unit')] - public function all_returned_types_are_creatable(): void + public function it_ensures_all_returned_types_are_creatable(): void { + /* Arrange */ $blockTypes = BlockFactory::all(); + /* Act & Assert */ foreach ($blockTypes as $block) { $handler = BlockFactory::make($block['type']); $this->assertNotNull($handler); } }Modules/Core/Tests/Feature/CreateReportTemplateTest.php-111-117 (1)
111-117: Remove unused variable or use assertion.The static analysis correctly identifies that
$_templateis unused. Either remove the variable assignment or add an assertion to validate the returned template.🔧 Proposed fix
/* act */ - $_template = $this->service->createTemplate( + $this->service->createTemplate( $company, 'Test Template', 'invoice', $blocks );Modules/Core/Services/ReportTemplateService.php-292-294 (1)
292-294: Remove unused$configvariable.Static analysis correctly identifies that
$configis assigned but never used. If the config is not needed ingetSystemBlocks(), remove this line.🔧 Proposed fix
foreach ($dbBlocks as $dbBlock) { - $config = $this->getBlockConfig($dbBlock); - // Map widths to grid units for the designer using the enum method $width = $dbBlock->width->getGridWidth();Modules/Core/Tests/Unit/BlockTransformerTest.php-247-247 (1)
247-247: Test name must start withit_prefix.According to the coding guidelines, unit test functions must be prefixed with
it_and make grammatical sense. This test name should beit_preserves_data_in_roundtrip_conversionor similar.As per coding guidelines.
📝 Proposed fix
- public function roundtrip_conversion_preserves_data(): void + public function it_preserves_data_in_roundtrip_conversion(): voidModules/Core/DTOs/BlockDTO.php-84-109 (1)
84-109: Config array is not deep-cloned, creating shared reference.At line 100,
$original->getConfig()returns an array reference that is directly assigned to the cloned block. In PHP, arrays containing objects or nested arrays can lead to unintended shared state. Modifications to nested structures in the cloned block's config will affect the original block's config.Consider deep-cloning the config array to ensure true independence between original and cloned blocks.
🔧 Proposed fix using json encode/decode for deep clone
- $dto->setPosition($newPosition); - $dto->setConfig($original->getConfig()); + $dto->setPosition($newPosition); + // Deep clone the config array to avoid shared references + $originalConfig = $original->getConfig(); + $dto->setConfig($originalConfig ? json_decode(json_encode($originalConfig), true) : []);Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php-417-443 (1)
417-443: Add defensive checks in save method.The method accesses array keys that may not exist (e.g.,
$block['type']at line 427,$block['id']at line 429) without validation. This could cause undefined index warnings or errors.🛡️ Suggested defensive checks
foreach ($band['blocks'] as $block) { + // Validate required fields + if (!isset($block['id'])) { + Log::warning('Block missing id field', ['block' => $block]); + continue; + } + // Ensure the block data has all necessary fields before passing to service if ( ! isset($block['type'])) {Committable suggestion skipped: line range outside the PR's diff.
| // Save default config to JSON if it doesn't exist | ||
| $path = 'report_blocks/' . $filename . '.json'; | ||
| if ( ! Storage::disk('local')->exists($path)) { | ||
| $config = $block['config']; | ||
| $config['fields'] = []; // Start with no fields as requested for drag/drop | ||
| Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT)); | ||
| } |
There was a problem hiding this comment.
Undefined array key when accessing $block['config'].
Only one block in the $blocks array has a config key (Item Tax Details). For all other blocks, accessing $block['config'] on line 103 will trigger an "Undefined array key" warning and result in null being assigned, which then has ['fields'] set on it — causing a fatal error.
🐛 Proposed fix
// Save default config to JSON if it doesn't exist
$path = 'report_blocks/' . $filename . '.json';
if ( ! Storage::disk('local')->exists($path)) {
- $config = $block['config'];
+ $config = $block['config'] ?? [];
$config['fields'] = []; // Start with no fields as requested for drag/drop
Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Save default config to JSON if it doesn't exist | |
| $path = 'report_blocks/' . $filename . '.json'; | |
| if ( ! Storage::disk('local')->exists($path)) { | |
| $config = $block['config']; | |
| $config['fields'] = []; // Start with no fields as requested for drag/drop | |
| Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT)); | |
| } | |
| // Save default config to JSON if it doesn't exist | |
| $path = 'report_blocks/' . $filename . '.json'; | |
| if ( ! Storage::disk('local')->exists($path)) { | |
| $config = $block['config'] ?? []; | |
| $config['fields'] = []; // Start with no fields as requested for drag/drop | |
| Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT)); | |
| } |
🤖 Prompt for AI Agents
In @Modules/Core/Database/Seeders/ReportBlocksSeeder.php around lines 100 - 106,
ReportBlocksSeeder is accessing $block['config'] without checking it exists,
causing undefined key and later fatal when setting ['fields']; change the
assignment to safely handle missing/non-array config—use a null-coalescing or
existence check to set $config to an array when $block['config'] is absent
(e.g., $config = isset($block['config']) && is_array($block['config']) ?
$block['config'] : []), then set $config['fields'] = []; and continue to write
the JSON; update the block in the ReportBlocksSeeder (the section where $path,
$config and Storage::disk('local')->put are used) so it never tries to index
into null.
| use Filament\Actions\ActionGroup; | ||
| use Filament\Actions\DeleteAction; | ||
| use Filament\Actions\EditAction; |
There was a problem hiding this comment.
Incorrect namespace for table actions.
The imports use Filament\Actions\* which is for page-level actions. Table row actions should use Filament\Tables\Actions\* namespace.
Proposed fix
-use Filament\Actions\ActionGroup;
-use Filament\Actions\DeleteAction;
-use Filament\Actions\EditAction;
+use Filament\Tables\Actions\ActionGroup;
+use Filament\Tables\Actions\DeleteAction;
+use Filament\Tables\Actions\EditAction;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use Filament\Actions\ActionGroup; | |
| use Filament\Actions\DeleteAction; | |
| use Filament\Actions\EditAction; | |
| use Filament\Tables\Actions\ActionGroup; | |
| use Filament\Tables\Actions\DeleteAction; | |
| use Filament\Tables\Actions\EditAction; |
🤖 Prompt for AI Agents
In
@Modules/Core/Filament/Admin/Resources/ReportBlocks/Tables/ReportBlocksTable.php
around lines 5 - 7, The imports for ActionGroup, DeleteAction, and EditAction
are using the page-actions namespace (Filament\Actions\*) but these are table
row actions; update their use statements to the table-actions namespace
(Filament\Tables\Actions\ActionGroup, Filament\Tables\Actions\DeleteAction,
Filament\Tables\Actions\EditAction) so the ReportBlocksTable class uses the
correct action classes for table rows.
| use Filament\Actions\Action; | ||
| use Filament\Actions\ActionGroup; | ||
| use Filament\Actions\DeleteAction; | ||
| use Filament\Actions\EditAction; | ||
| use Filament\Actions\ViewAction; |
There was a problem hiding this comment.
Incorrect namespace for table actions.
Table actions in Filament should use Filament\Tables\Actions\* namespace, not Filament\Actions\*. The current imports will cause runtime errors as these classes are meant for page-level actions, not table record actions.
🐛 Proposed fix
-use Filament\Actions\Action;
-use Filament\Actions\ActionGroup;
-use Filament\Actions\DeleteAction;
-use Filament\Actions\EditAction;
-use Filament\Actions\ViewAction;
+use Filament\Tables\Actions\Action;
+use Filament\Tables\Actions\ActionGroup;
+use Filament\Tables\Actions\DeleteAction;
+use Filament\Tables\Actions\EditAction;
+use Filament\Tables\Actions\ViewAction;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use Filament\Actions\Action; | |
| use Filament\Actions\ActionGroup; | |
| use Filament\Actions\DeleteAction; | |
| use Filament\Actions\EditAction; | |
| use Filament\Actions\ViewAction; | |
| use Filament\Tables\Actions\Action; | |
| use Filament\Tables\Actions\ActionGroup; | |
| use Filament\Tables\Actions\DeleteAction; | |
| use Filament\Tables\Actions\EditAction; | |
| use Filament\Tables\Actions\ViewAction; |
🤖 Prompt for AI Agents
In
@Modules/Core/Filament/Admin/Resources/ReportTemplates/Tables/ReportTemplatesTable.php
around lines 5 - 9, The file imports table action classes from the wrong
namespace (Filament\Actions\*); replace those imports with the table-specific
namespace (Filament\Tables\Actions\Action, Filament\Tables\Actions\ActionGroup,
Filament\Tables\Actions\DeleteAction, Filament\Tables\Actions\EditAction,
Filament\Tables\Actions\ViewAction) so the table record actions (where Action,
ActionGroup, DeleteAction, EditAction, ViewAction are referenced) use the
correct Filament Tables classes and avoid runtime errors.
|
|
||
| $html .= '</tr></thead><tbody>'; | ||
|
|
||
| foreach (($invoice->invoice_items ?? []) as $item) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "Modules/Core/Handlers/DetailItemsBlockHandler.php"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 3241
🏁 Script executed:
# Search for where DetailItemsBlockHandler is instantiated
rg -n "DetailItemsBlockHandler|->invoice_items" --type=phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 522
🏁 Script executed:
# Also check for eager-loading patterns with invoice_items
rg -n "with\(.*invoice_items|eager|load.*invoice" --type=phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 1707
🏁 Script executed:
# Search for handler usage in main code (exclude tests)
rg -n "BlockHandlerInterface|render\(" --type=php -g '!**/Tests/**' | head -30Repository: InvoicePlane/InvoicePlane-v2
Length of output: 3466
🏁 Script executed:
# Find the Invoice model to understand the invoice_items relationship
fd "Invoice.php" --type=php | head -5Repository: InvoicePlane/InvoicePlane-v2
Length of output: 242
🏁 Script executed:
# Check for where blocks are rendered/rendered and how invoice is passed
rg -n "render\(" --type=php -B 3 -A 3 -g '!**/Tests/**' | grep -A 5 -B 5 "DetailItemsBlockHandler\|invoice_items"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 1391
🏁 Script executed:
cat -n "Modules/Core/Services/ReportRenderer.php" | head -80Repository: InvoicePlane/InvoicePlane-v2
Length of output: 3372
🏁 Script executed:
# Check the Invoice model to understand the invoice_items relationship
fd "Invoice.php" -path "*/Models/*"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 240
🏁 Script executed:
# Get the Invoice model content
cat -n "Modules/Invoices/Models/Invoice.php" | grep -A 10 -B 2 "invoice_items\|invoiceItems"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 463
🏁 Script executed:
# Find where renderToHtml is called to see if invoice_items is eager-loaded
rg -n "renderToHtml|->with\(" --type=php -B 5 -A 5Repository: InvoicePlane/InvoicePlane-v2
Length of output: 7898
🏁 Script executed:
# Check if there's any eager loading in the Invoice retrieval flow
rg -n "Invoice.*with\(|with.*invoice" --type=php -g '!**/Tests/**' | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 422
🏁 Script executed:
# Let's look at the full Invoice model to see relationship definitions
wc -l "Modules/Invoices/Models/Invoice.php"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 111
🏁 Script executed:
cat -n "Modules/Invoices/Models/Invoice.php"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 6682
🏁 Script executed:
# Check if there's any eager loading middleware or trait for the Invoice model
rg -n "Illuminate\|eloquent|Load|with" "Modules/Invoices/Models/Invoice.php" | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check where ReportRenderer is called and if invoices are retrieved with eager loading
rg -n "ReportRenderer|renderToHtml|renderToPdf" --type=php -B 5 -A 5 -g '!**/Tests/**' | head -40Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2857
🏁 Script executed:
# Check if there's a controller or service that calls ReportRenderer
fd -name "*.php" -path "*/Controllers/*" -o -path "*/Services/*" | xargs rg -l "ReportRenderer" 2>/dev/null | head -5Repository: InvoicePlane/InvoicePlane-v2
Length of output: 411
🏁 Script executed:
rg -n "ReportRenderer" --type=php -g '!**/Tests/**' -lRepository: InvoicePlane/InvoicePlane-v2
Length of output: 112
🏁 Script executed:
# Now search for where renderToPdf or renderToHtml is called
rg -n "renderToPdf\|renderToHtml" --type=php -B 10 -A 5 -g '!**/Tests/**'Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check tests to understand the usage pattern
rg -n "renderToPdf\|renderToHtml\|ReportRenderer" --type=php -B 5 -A 5Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Search for any controller or action that handles invoice rendering/PDF
rg -n "invoice.*pdf\|render.*invoice\|export" --type=php -i | grep -i "controller\|action" | head -10Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Look for Filament actions or resources that might use ReportRenderer
fd -name "*.php" | xargs grep -l "ReportRenderer" 2>/dev/nullRepository: InvoicePlane/InvoicePlane-v2
Length of output: 304
🏁 Script executed:
# Search more broadly for where invoices are passed to rendering
rg -n "new ReportRenderer\|@inject.*ReportRenderer\|renderToPdf\|renderToHtml" --type=phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check if there's a Livewire component that uses the renderer
fd -name "*.php" -path "*/Livewire/*" -o -path "*/Actions/*" | xargs grep -l "ReportRenderer\|render.*invoice" 2>/dev/null | head -5Repository: InvoicePlane/InvoicePlane-v2
Length of output: 302
🏁 Script executed:
# Better search for files that contain ReportRenderer
rg "ReportRenderer" --type=php --files-with-matchesRepository: InvoicePlane/InvoicePlane-v2
Length of output: 163
🏁 Script executed:
# Search in tests for usage patterns
rg -n "renderToPdf\|renderToHtml" --type=php | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Look for Filament actions or service calls related to invoices and PDF
rg -n "->pdf\|->download\|->preview" --type=php -g '!**/migrations/**' -g '!**/database/**' | grep -i invoice | head -10Repository: InvoicePlane/InvoicePlane-v2
Length of output: 476
🏁 Script executed:
# Search for PDF or download actions related to invoices
rg "pdf\|download\|preview" --type=php -g '!**/migrations/**' -g '!**/database/**' -i | grep -i "invoice\|action" | head -15Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Look at test file to understand usage
cat -n "Modules/Core/Tests/Feature/ReportRenderingTest.php" | head -100Repository: InvoicePlane/InvoicePlane-v2
Length of output: 3856
🏁 Script executed:
cat -n "Modules/Core/Tests/Feature/ReportRenderingTest.php" | sed -n '100,200p'Repository: InvoicePlane/InvoicePlane-v2
Length of output: 3404
🏁 Script executed:
# Check what the render method signature looks like
rg -n "public function render" "Modules/Core/Services/ReportRenderer.php"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 329
🏁 Script executed:
# Get the full ReportRenderer file
cat -n "Modules/Core/Services/ReportRenderer.php"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 12653
Use the correct relationship property name invoiceItems instead of invoice_items.
Line 59 accesses $invoice->invoice_items, but the Invoice model defines the relationship as invoiceItems() (camelCase). In Laravel, accessing the snake_case property invoice_items returns null, so the ?? operator always provides an empty array, effectively hiding the actual issue. Additionally, even with the correct property name, accessing the relationship will trigger a lazy-load query if invoiceItems hasn't been eager-loaded, which can cause N+1 query problems if this handler is called multiple times. Change $invoice->invoice_items to $invoice->invoiceItems, and verify that calling code eager-loads the relationship (e.g., ->with('invoiceItems')).
🤖 Prompt for AI Agents
In @Modules/Core/Handlers/DetailItemsBlockHandler.php at line 59, The code is
using the wrong relationship property ($invoice->invoice_items) which returns
null; change it to use the model relationship property $invoice->invoiceItems
(the invoiceItems() relation) so the actual items are iterated, and ensure
callers eager-load that relation (e.g., ->with('invoiceItems')) to avoid
lazy-loading/N+1 queries when DetailItemsBlockHandler iterates the items.
| if ( ! empty($config['show_tax_amount'])) { | ||
| $taxAmount = ($invoice->subtotal ?? 0) * (($taxRate->rate ?? 0) / 100); | ||
| $html .= '<td>' . $this->formatCurrency($taxAmount, $invoice->currency_code) . '</td>'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the DetailItemTaxBlockHandler.php file
find . -name "DetailItemTaxBlockHandler.php" -type fRepository: InvoicePlane/InvoicePlane-v2
Length of output: 125
🏁 Script executed:
# Explore the file structure to understand the context
fd -t f "\.php$" | grep -i tax | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 906
🏁 Script executed:
# Search for Invoice and InvoiceItem models to understand tax relationships
fd -t f "\.php$" | xargs grep -l "class Invoice\|class InvoiceItem" | head -10Repository: InvoicePlane/InvoicePlane-v2
Length of output: 764
🏁 Script executed:
# Read the full DetailItemTaxBlockHandler.php file to understand context
cat -n Modules/Core/Handlers/DetailItemTaxBlockHandler.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 2903
🏁 Script executed:
# Read TaxRate model to understand its relationships
cat -n Modules/Core/Models/TaxRate.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 2030
🏁 Script executed:
# Read the InvoiceTaxRateService to understand how taxes are calculated
cat -n Modules/Core/Services/InvoiceTaxRateService.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 178
🏁 Script executed:
# Find and read the Invoice model to understand the tax_rates relationship
find . -path "*/Invoices/Models/Invoice.php" -type f -exec cat -n {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 6682
🏁 Script executed:
# Find and read the InvoiceItem model
find . -path "*/Invoices/Models/InvoiceItem.php" -type f -exec cat -n {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2765
🏁 Script executed:
# Search for where invoice.tax_rates is set or populated
rg "tax_rates" -A 3 -B 3 --type php | head -100Repository: InvoicePlane/InvoicePlane-v2
Length of output: 9229
🏁 Script executed:
# Search for where invoice->tax_rates might be set or computed
rg "tax_rates" -B 5 -A 5 --type php Modules/Invoices/Models/Invoice.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Search for accessors or computed properties on Invoice
rg "protected function|public function" Modules/Invoices/Models/Invoice.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 827
🏁 Script executed:
# Check if there's an accessor or relation method that might provide tax_rates
rg "\$invoice->tax_rates|->tax_rates\(\)|Relation::hasManyThrough" --type php | head -50Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Search for tests that might show how tax_rates is supposed to work
fd -t f "Test\.php|test\.php" | xargs rg "DetailItemTaxBlockHandler\|invoice.*tax_rates" | head -30Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Look for appendable attributes or other ways tax_rates could be added to Invoice
rg "append|cast|getAttribute|setAttribute" Modules/Invoices/Models/Invoice.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 96
🏁 Script executed:
# Search entire codebase for where tax_rates collection on invoice is created
rg "invoice.*tax_rates\s*=" --type php | head -30Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check if there's a relation method that loads tax rates from items
rg "HasManyThrough|tax_rates\(\)|getTaxRates" --type php Modules/Invoices | head -50Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Look for any place where invoice tax_rates might be loaded/computed via items
rg "invoiceItems.*map|invoiceItems.*pluck" --type php | head -30Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check DetailItemTaxBlockHandler more carefully - how is it called and what data is passed
rg -B 10 -A 10 "DetailItemTaxBlockHandler" --type php | head -80Repository: InvoicePlane/InvoicePlane-v2
Length of output: 1701
🏁 Script executed:
# Search for how invoice object is prepared before being passed to the handler
rg "new DetailItemTaxBlockHandler\|->render" --type php -B 5 -A 5 | head -100Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check if there's an accessor for tax_rates using PHP magic methods
rg "__get|__call|Illuminate\\Support" Modules/Invoices/Models/Invoice.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Look at what Invoice model extends or uses that might provide dynamic properties
head -50 Modules/Invoices/Models/Invoice.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 2135
🏁 Script executed:
# Search for any facade or helper that adds tax_rates to invoice
rg "tax_rates" --type php Modules/Invoices | grep -v migration | grep -v factory | head -40Repository: InvoicePlane/InvoicePlane-v2
Length of output: 258
🏁 Script executed:
# Check if there's any relation loading or accessor methods defined elsewhere
find Modules/Invoices -name "*.php" -type f -exec grep -l "tax_rates\|getTaxRates" {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 157
🏁 Script executed:
# Check if Invoice has any computed/appended attributes or boot methods
cat -n Modules/Invoices/Models/Invoice.php | grep -A 200 "class Invoice"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 4471
🏁 Script executed:
# Look for BlockHandlerInterface to understand the contract
find . -name "BlockHandlerInterface.php" -type f -exec cat -n {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 1037
🏁 Script executed:
# Check where DetailItemTaxBlockHandler is used in the application
find . -type f -name "*.php" -exec grep -l "DetailItemTaxBlockHandler" {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 125
🏁 Script executed:
# Look for any feature or test files that might show the expected behavior
find . -path "*/Tests/*" -name "*Tax*" -o -path "*/Tests/*" -name "*Invoice*" | xargs grep -l "tax" | head -5Repository: InvoicePlane/InvoicePlane-v2
Length of output: 227
🏁 Script executed:
# Read the InvoiceTaxRateServiceTest to understand the expected behavior
cat -n Modules/Invoices/Tests/Unit/InvoiceTaxRateServiceTest.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 1719
🏁 Script executed:
# Read the TaxRatesTest to see if there's any setup for invoice tax_rates
cat -n Modules/Core/Tests/Feature/TaxRatesTest.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 4681
🏁 Script executed:
# Read the InvoicesTest to see how invoices are tested with taxes
cat -n Modules/Invoices/Tests/Feature/InvoicesTest.php | head -150Repository: InvoicePlane/InvoicePlane-v2
Length of output: 5963
🏁 Script executed:
# Look for where the invoice object might get tax_rates loaded dynamically
rg "load\(|with\(|append\(" Modules/Invoices --type php | grep -i tax | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check if there's an invoice_tax table or pivot table that might explain the relationship
rg "invoice_tax|pivot" --type php Modules/Invoices/Database/Migrations | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 206
🏁 Script executed:
# Search for any dynamic property loading or append methods
rg "appends|append\(|getAttribute|__get" --type php Modules/Invoices/Models/Invoice.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check if tax_rates might be loaded through a HasManyThrough or similar pattern
find Modules -name "*.php" -type f -exec grep -l "HasManyThrough\|invoiceItems.*tax" {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 174
🏁 Script executed:
# Look for where DetailItemTaxBlockHandler is instantiated to see if tax_rates is added before calling render
find . -name "*.php" -type f -exec grep -B 20 "DetailItemTaxBlockHandler" {} \; | grep -A 20 "new DetailItemTaxBlockHandler\|->render"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Search in services, controllers, or any place where the handler might be called
find . -name "*.php" -type f -exec grep -l "BlockHandler\|DetailItemTax" {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 599
🏁 Script executed:
# Look for where invoice object might be enriched with tax_rates before rendering
rg "tax_rates\s*=" --type php | head -30Repository: InvoicePlane/InvoicePlane-v2
Length of output: 54
🏁 Script executed:
# Check if there's a service or utility that prepares invoice data with tax rates
find Modules/Invoices -name "*Service*.php" -type f | xargs cat | grep -A 10 "tax"Repository: InvoicePlane/InvoicePlane-v2
Length of output: 777
🏁 Script executed:
# Look at the InvoiceItem model again to see if it might be used to collect tax rates
rg "invoiceItems\(\)|items\(\)" --type php Modules/Invoices | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 381
🏁 Script executed:
# Search for HasManyThrough usage patterns
rg "HasManyThrough" --type phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 598
🏁 Script executed:
# Search for the invoice_tax table/migration
find . -name "*.php" -type f -exec grep -l "invoice_tax" {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 532
🏁 Script executed:
# Look at all migration files for invoice-related tables
find Modules/Invoices/Database/Migrations -name "*.php" -type f -exec basename {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 218
🏁 Script executed:
# Check the actual invoice migrations for any tax-related tables
cat -n Modules/Invoices/Database/Migrations/2010_01_01_000023_create_invoices_table.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 2967
🏁 Script executed:
# Search for subtotal property or relationship on Invoice
rg "subtotal|tax_rates" --type php Modules/Invoices/Models/ -A 3 -B 3Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2849
🏁 Script executed:
# Check if there might be an accessor for subtotal or tax_rates
find Modules/Invoices/Models -name "*.php" -exec grep -A 50 "class Invoice" {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2664
Fix undefined relationships and correct the tax calculation logic.
The handler references $invoice->tax_rates (line 28) and $invoice->subtotal (line 63), neither of which exist in the Invoice model. The Invoice model only defines invoice_item_subtotal, and tax rates are stored at the item level (InvoiceItem.item_tax_rate_id) not invoice-wide.
The current calculation—applying the entire invoice subtotal to each tax rate—is also incorrect. Tax must be calculated per item based on each item's assigned tax rate, not by applying all rates to the total subtotal. Either add the missing relationship and property to the Invoice model, or refactor the handler to iterate over invoice items and their individual tax rates.
🤖 Prompt for AI Agents
In @Modules/Core/Handlers/DetailItemTaxBlockHandler.php around lines 62 - 64,
DetailItemTaxBlockHandler is using non-existent $invoice->tax_rates and
$invoice->subtotal and applies the full subtotal to each tax rate; change the
logic to iterate invoice items (use the relation that returns InvoiceItem
records), for each item read its item_tax_rate_id and item subtotal
(invoice_item_subtotal or the item amount), look up the tax rate (via the
TaxRate model or an injected lookup), compute tax per item as item_subtotal *
(rate/100) and aggregate those per tax-rate id, then render the aggregated tax
amount for each tax rate instead of using $invoice->subtotal or
$invoice->tax_rates. Ensure references to $invoice->subtotal and
$invoice->tax_rates are removed and use the invoice items relation and
item_tax_rate_id/amount fields (and TaxRate->rate) to compute values.
| <br><br> | ||
| <span class="info">{{ mb_strtoupper(trans('ip.bill_to')) }}</span><br>{{ $quote->customer->name }}<br> | ||
| @if ($quote->customer->address) | ||
| {!! $quote->customer->formatted_address !!}<br> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check total file structure
git ls-files | head -50Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2763
🏁 Script executed:
#!/bin/bash
# Search for any quote-related files anywhere
git ls-files | grep -i quoteRepository: InvoicePlane/InvoicePlane-v2
Length of output: 3091
🏁 Script executed:
#!/bin/bash
# Search for any accessor patterns in the codebase
rg "Attribute\(\)" --type=php -B 5 | head -100Repository: InvoicePlane/InvoicePlane-v2
Length of output: 1871
Create formatted field accessors with proper sanitization in models.
The blade template uses unescaped output for properties that don't currently exist as defined accessors: $quote->customer->formatted_address (line 92), $quote->formatted_description (line 123), $quote->formatted_terms (line 158), and $quote->formatted_footer (line 161).
These formatted_* accessors are not defined in the Quote or Relation (Customer) models. When created, ensure they sanitize output to prevent XSS vulnerabilities. Use Laravel's Str::of() or similar utilities to escape HTML entities, especially since the template renders them with {!! !!} unescaped syntax.
Also applies to: 123-123, 158-158, 161-161
🤖 Prompt for AI Agents
In @Modules/Core/resources/Templates/Views/templates/quotes/default.blade.php at
line 92, The template uses unescaped formatted_* properties that don't exist;
add accessors getFormattedAddressAttribute on the Customer model and
getFormattedDescriptionAttribute, getFormattedTermsAttribute,
getFormattedFooterAttribute on the Quote model, and make them return sanitized
HTML (use e() to escape, convert newlines with nl2br if needed, then wrap in
Illuminate\Support\HtmlString) so the {!! !!} output is safe; import HtmlString
and ensure the accessor names match the properties
($quote->customer->formatted_address, $quote->formatted_description,
$quote->formatted_terms, $quote->formatted_footer).
| <x-filament-panels::page> | ||
| <x-filament-panels::form wire:submit="submit"> | ||
| {{ $this->form }} | ||
|
|
||
| <x-filament-panels::form.actions | ||
| :actions="$this->getCachedFormActions()" | ||
| :full-width="$this->hasFullWidthFormActions()" | ||
| /> | ||
| </x-filament-panels::form> | ||
| </x-filament-panels::page> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find Modules/Core -name "*.php" -type f | grep -iE "(settings|page)" | head -20Repository: InvoicePlane/InvoicePlane-v2
Length of output: 1755
🏁 Script executed:
find Modules/Core/Filament/Admin/Pages -name "*.php" -type f 2>/dev/null | sortRepository: InvoicePlane/InvoicePlane-v2
Length of output: 126
🏁 Script executed:
# Check if the template file exists
ls -la Modules/Core/resources/views/filament/admin/pages/settings.blade.php 2>&1
# Search for any Settings-related PHP files
rg -i "class.*settings" Modules/Core --type phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 188
🏁 Script executed:
# Search for any Settings file (not just class definition)
rg "Settings" Modules/Core --type php -l | head -10
# Also check if there are any PHP files in the pages directory matching the template name
find Modules/Core -name "*ettings*.php" -o -name "*settings*.php" 2>/dev/null | grep -i settingsRepository: InvoicePlane/InvoicePlane-v2
Length of output: 231
🏁 Script executed:
cat Modules/Core/Providers/AdminPanelProvider.php | grep -A 5 -B 5 -i settingsRepository: InvoicePlane/InvoicePlane-v2
Length of output: 1014
🏁 Script executed:
# View the entire AdminPanelProvider.php to see how pages are registered
wc -l Modules/Core/Providers/AdminPanelProvider.php
# Search for pages() method or ->pages() configuration
rg "->pages\(" Modules/Core --type php -A 5 | head -30Repository: InvoicePlane/InvoicePlane-v2
Length of output: 543
🏁 Script executed:
# View the AdminPanelProvider.php
cat -n Modules/Core/Providers/AdminPanelProvider.php | head -100
# Also search for pages registration with proper escaping
rg "pages" Modules/Core/Providers/AdminPanelProvider.php -A 3Repository: InvoicePlane/InvoicePlane-v2
Length of output: 4763
🏁 Script executed:
sed -n '170,180p' Modules/Core/Providers/AdminPanelProvider.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 451
Settings Page class is missing and template is not registered in the panel.
While the template structure is valid, the implementation is incomplete. The template references $this->form, $this->getCachedFormActions(), and $this->hasFullWidthFormActions(), which require a corresponding Settings Page class that doesn't exist. Additionally, the template must be registered in AdminPanelProvider::panel() via ->pages(). Currently, only Dashboard::class is registered.
Create the Settings Page class in Modules/Core/Filament/Admin/Pages/Settings.php with the required form schema and submit handler, then register it in the panel configuration.
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Invoice {$invoice->number}</title> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the Invoice model
find . -path "*/Modules/Invoices/Models/Invoice.php" -type fRepository: InvoicePlane/InvoicePlane-v2
Length of output: 109
🏁 Script executed:
# Check if Invoice model exists and examine its structure
if [ -f "Modules/Invoices/Models/Invoice.php" ]; then
echo "=== Invoice Model Content ==="
wc -l Modules/Invoices/Models/Invoice.php
cat -n Modules/Invoices/Models/Invoice.php
else
echo "Invoice model not found at expected path"
fiRepository: InvoicePlane/InvoicePlane-v2
Length of output: 6752
🏁 Script executed:
# Check ReportRenderer.php around line 215 for context
if [ -f "Modules/Core/Services/ReportRenderer.php" ]; then
echo "=== ReportRenderer.php context around line 215 ==="
sed -n '200,230p' Modules/Core/Services/ReportRenderer.php | cat -n
fiRepository: InvoicePlane/InvoicePlane-v2
Length of output: 1042
🏁 Script executed:
# Check for imports in ReportRenderer.php to verify namespace usage
if [ -f "Modules/Core/Services/ReportRenderer.php" ]; then
echo "=== Imports in ReportRenderer.php ==="
head -50 Modules/Core/Services/ReportRenderer.php | cat -n
fiRepository: InvoicePlane/InvoicePlane-v2
Length of output: 1829
Use $invoice->invoice_number instead of $invoice->number.
The Invoice model (Modules\Invoices\Models\Invoice) defines the property as invoice_number, not number. Accessing $invoice->number will fail at runtime. Update line 215 to use the correct property name.
🤖 Prompt for AI Agents
In @Modules/Core/Services/ReportRenderer.php at line 215, Update the template
usage in ReportRenderer (where the invoice title is generated) to reference the
correct Invoice model property: replace the twig/templated expression
`{$invoice->number}` with `{$invoice->invoice_number}` so it accesses the
defined Invoice::invoice_number property (look for the title line producing
"Invoice {$invoice->number}" and swap to the correct property name).
| /* arrange */ | ||
| $company = Company::factory()->create(); | ||
| $user = User::factory()->create(); | ||
| $user->companies()->attach($company); | ||
| session(['current_company_id' => $company->id]); | ||
|
|
||
| $initialBlocks = [ | ||
| [ | ||
| 'id' => 'block_company_header', | ||
| 'type' => 'company_header', | ||
| 'position' => ['x' => 0, 'y' => 0, 'width' => 6, 'height' => 4], | ||
| 'config' => ['show_vat_id' => true], | ||
| 'label' => 'Company Header', | ||
| 'isCloneable' => true, | ||
| 'dataSource' => 'company', | ||
| 'isCloned' => false, | ||
| 'clonedFrom' => null, | ||
| ], | ||
| ]; | ||
|
|
||
| $template = $this->service->createTemplate( | ||
| $company, | ||
| 'Test Template', | ||
| 'invoice', | ||
| $initialBlocks | ||
| ); | ||
|
|
||
| $updatedBlocks = [ | ||
| [ | ||
| 'id' => 'block_company_header', | ||
| 'type' => 'company_header', | ||
| 'position' => ['x' => 2, 'y' => 2, 'width' => 8, 'height' => 6], | ||
| 'config' => ['show_vat_id' => false], | ||
| 'label' => 'Updated Company Header', | ||
| 'isCloneable' => true, | ||
| 'dataSource' => 'company', | ||
| 'isCloned' => false, | ||
| 'clonedFrom' => null, | ||
| ], | ||
| ]; | ||
|
|
||
| /* act */ | ||
| $this->service->updateTemplate($template, $updatedBlocks); | ||
|
|
||
| /* assert */ | ||
| $fileContents = Storage::disk('report_templates')->get( | ||
| "{$company->id}/test-template.json" | ||
| ); | ||
| $savedBlocks = json_decode($fileContents, true); | ||
|
|
||
| $this->assertCount(1, $savedBlocks); | ||
| $this->assertEquals(2, $savedBlocks[0]['position']['x']); | ||
| $this->assertEquals(2, $savedBlocks[0]['position']['y']); | ||
| $this->assertEquals(8, $savedBlocks[0]['position']['width']); | ||
| $this->assertEquals(6, $savedBlocks[0]['position']['height']); | ||
| $this->assertFalse($savedBlocks[0]['config']['show_vat_id']); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ReportTemplateService file
fd -t f -i "reporttemplate" | grep -i service
# Search for the persistBlocks method
ast-grep --pattern $'public function persistBlocks($_, $_) {
$$$
}'Repository: InvoicePlane/InvoicePlane-v2
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Read the ReportTemplateService to find persistBlocks method
cat -n Modules/Core/Services/ReportTemplateService.php | head -200Repository: InvoicePlane/InvoicePlane-v2
Length of output: 7406
🏁 Script executed:
#!/bin/bash
# Read the UpdateReportTemplateTest.php file to verify the assertions
sed -n '47,103p' Modules/Core/Tests/Feature/UpdateReportTemplateTest.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 2210
Fix comment casing and update assertions to access grouped block structure.
Per coding guidelines, section comments must be capitalized: /* Arrange */, /* Act */, /* Assert */.
Additionally, ReportTemplateService::persistBlocks groups blocks by band (header, group_header, details, group_footer, footer). Blocks without a band property default to header. The test assertions incorrectly access the saved JSON as a flat array—$savedBlocks[0] is undefined. Update assertions to access $savedBlocks['header'][0] instead, and change assertCount(1, $savedBlocks) to assertCount(1, $savedBlocks['header']).
Proposed fixes
- /* arrange */
+ /* Arrange */
$company = Company::factory()->create();- /* act */
+ /* Act */
$this->service->updateTemplate($template, $updatedBlocks);- /* assert */
+ /* Assert */
$fileContents = Storage::disk('report_templates')->get(
"{$company->id}/test-template.json"
);
$savedBlocks = json_decode($fileContents, true);
- $this->assertCount(1, $savedBlocks);
- $this->assertEquals(2, $savedBlocks[0]['position']['x']);
- $this->assertEquals(2, $savedBlocks[0]['position']['y']);
- $this->assertEquals(8, $savedBlocks[0]['position']['width']);
- $this->assertEquals(6, $savedBlocks[0]['position']['height']);
- $this->assertFalse($savedBlocks[0]['config']['show_vat_id']);
+ $this->assertCount(1, $savedBlocks['header']);
+ $this->assertEquals(2, $savedBlocks['header'][0]['position']['x']);
+ $this->assertEquals(2, $savedBlocks['header'][0]['position']['y']);
+ $this->assertEquals(8, $savedBlocks['header'][0]['position']['width']);
+ $this->assertEquals(6, $savedBlocks['header'][0]['position']['height']);
+ $this->assertFalse($savedBlocks['header'][0]['config']['show_vat_id']);| FAILED Modules\Core\Tests\Unit\Services\NumberingCompanyIsolationTest >… | ||
| Failed asserting that null matches expected 'EXP-0001'. | ||
|
|
||
| at Modules/Core/Tests/Unit/Services/NumberingCompanyIsolationTest.php:154 | ||
| 150▕ // Generate third number with new format | ||
| 151▕ $thirdNumber = $generator->forNumberingId($numbering->id)->generate(); | ||
| 152▕ | ||
| 153▕ /* Assert */ | ||
| ➜ 154▕ $this->assertEquals('EXP-0001', $firstNumber); | ||
| 155▕ $this->assertEquals('EXP-0002', $secondNumber); | ||
| 156▕ $this->assertEquals('EXP-2025-12-0003', $thirdNumber); // Number continues | ||
| 157▕ } | ||
| 158▕ |
There was a problem hiding this comment.
Critical: Number generator returns null instead of formatted string.
The test expects 'EXP-0001' but $firstNumber is null. This indicates the NumberGenerator::generate() method is failing to produce output, which would break invoice/expense numbering in production.
Investigate why NumberGenerator::forNumberingId($numbering->id)->generate() returns null. Likely causes:
- Missing or incorrect
last_idvalue in the numbering record (see related NOT NULL failures below) - Bug in the number formatting logic
- Missing required fields in the factory or test setup
🤖 Prompt for AI Agents
In @phpunit.txt around lines 39 - 51, NumberGenerator::generate() is returning
null in NumberingCompanyIsolationTest (so $firstNumber is null); inspect and fix
the generate flow used by
NumberGenerator::forNumberingId($numbering->id)->generate(): ensure the
numbering record has a valid last_id/default (set non-null default in factory or
DB) and that NumberGenerator::generate() handles a null/absent last_id by
initializing it (and always returns the formatted string), and verify the
formatting logic/path that builds the string (e.g., format template, pad logic)
returns a string not null; update the test factory to populate required fields
(last_id, format) or make generate() robust to missing fields so the call in
NumberingCompanyIsolationTest yields 'EXP-0001' rather than null.
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.