Add modular import:db command for InvoicePlane v1 to v2 migration#418
Add modular import:db command for InvoicePlane v1 to v2 migration#418Copilot wants to merge 21 commits intofeature/98-report-builder-onlyfrom
Conversation
Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
@copilot with 40+ tables needing to be imported and in the future separate imports also bring necessary (product imports, for example), best things is to start on those separate imports per phenomenon right now.
Filament has those import classes, work with those.
Make import classes and their services per phenomenon, so we don't forget any table. I can think of:
- Users
- Clients => Relations
- Expenses (easy, InvoicePlane-v1 doesn't have expenses)
- Invoices
- Quotes
- Payments
- Projects
- Tasks
- Email templates
- Invoice Groups (numberings)
Go over the migrations Systematically and see if you can find any other import possibilities
I don't like an imported database of 10+ tables while the original has 40+ tables.
The import service can then call the individual (InvoiceImportService::class)->handle() or something like that to do the actual import.
Which is coincidentally also the service class the Filament Import resource will call
Make surw there are plenty of phpunit tests. Including tests for edge cases
Understood. I'll refactor the implementation to:
Starting the refactor now. |
- Add 'import_v1' database connection in config - Create ImportServiceInterface and AbstractImportService base - Implement modular import services: * TaxRatesImportService * ProductsImportService (categories, units, products) * ClientsImportService (relations, contacts, addresses) * NumberingImportService (invoice groups) * InvoicesImportService (with InvoiceStatus enum) * QuotesImportService (with QuoteStatus enum) * PaymentsImportService (with PaymentMethod enum) - Create ImportOrchestrator to coordinate all services - Update command to read from storage/app/private/imports - Use Laravel DB connections instead of shell restoration Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
- UsersImportService (ip_users) - ProjectsImportService (ip_projects, ip_tasks) - EmailTemplatesImportService (ip_email_templates) - CustomFieldsImportService (ip_custom_fields, ip_custom_values) - SettingsImportService (ip_settings) - NotesImportService (ip_notes) - Update ImportOrchestrator to include all services - Add required ID mappings (users, projects, custom_fields) Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Completed comprehensive refactoring with modular architecture: ✅ 13 Modular Import Services Created
✅ Key Changes
Command: Each service can be used independently for future Filament imports. Ready for testing and documentation updates. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
- ImportOrchestrator: Scope users to company via whereHas, attach new users - UsersImportService: Validate emails before import - PaymentsImportService: Use PaymentStatus::COMPLETED enum instead of 'paid' - InvoicesImportService: Optimize item loading with groupBy (prevent N+1 queries) - QuotesImportService: Optimize item loading with groupBy (prevent N+1 queries) - ImportInvoicePlaneV1CommandTest: Use enum comparisons and filter by amount - IMPORT_README.md: Fix documentation to match actual command signature (filename, not path) - IMPORT_README.md: Update cleanup notes (database kept for debugging) Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Addressed all 9 unresolved code review comments (commit 805d9dc): Critical Issues Fixed:
Performance Optimizations: Documentation: Remaining items (lower priority):
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request implements a complete InvoicePlane v1→v2 database migration system consisting of a new Changes
Sequence DiagramsequenceDiagram
actor User
participant Command as import:db Command
participant Orchestrator as ImportOrchestrator
participant TempDB as Temporary DB
participant Services as Import Services
participant Models as Eloquent Models
participant TargetDB as Target Database
User->>Command: php artisan import:db dump.sql --company_id=1
Command->>Orchestrator: import(filename, companyId)
Orchestrator->>Orchestrator: Resolve/Create Company
Orchestrator->>Orchestrator: Ensure User exists
Orchestrator->>TempDB: Create temporary database
Orchestrator->>TempDB: Restore dump via mysql command
loop For each service in dependency order
Orchestrator->>Services: execute import(companyId, idMappings)
Services->>TempDB: Query v1 tables
Services->>Services: Resolve foreign keys from idMappings
Services->>Models: Create/Update records
Models->>TargetDB: Persist entities
Services->>Orchestrator: Return stats
Orchestrator->>Orchestrator: Aggregate stats
end
Orchestrator->>TempDB: Drop temporary database
Orchestrator->>Command: Return aggregated stats
Command->>User: Display import summary table + success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
| private function setupImportDatabase(): void | ||
| { | ||
| DB::connection('import_v1')->statement('DROP TABLE IF EXISTS ip_clients'); | ||
| DB::connection('import_v1')->statement('DROP TABLE IF EXISTS ip_contacts'); | ||
|
|
||
| DB::connection('import_v1')->statement(' | ||
| CREATE TABLE ip_clients ( | ||
| client_id INT PRIMARY KEY, | ||
| client_name VARCHAR(255), | ||
| client_vat_id VARCHAR(255), | ||
| client_active TINYINT, | ||
| client_address_1 VARCHAR(255), | ||
| client_address_2 VARCHAR(255), | ||
| client_city VARCHAR(255), | ||
| client_state VARCHAR(255), | ||
| client_zip VARCHAR(255), | ||
| client_country VARCHAR(255) | ||
| ) | ||
| '); | ||
|
|
||
| DB::connection('import_v1')->statement(' | ||
| CREATE TABLE ip_contacts ( | ||
| contact_id INT PRIMARY KEY, | ||
| client_id INT, | ||
| contact_name VARCHAR(255), | ||
| contact_email VARCHAR(255), | ||
| contact_phone VARCHAR(255) | ||
| ) | ||
| '); |
There was a problem hiding this comment.
This unit test suite depends on DB::connection('import_v1') to create/drop MySQL tables, but the repo’s test environment uses SQLite in-memory by default. To keep CI green, configure import_v1 to an in-memory SQLite connection during tests (and purge the connection) or skip these tests unless MySQL is available.
| private function setupImportDatabase(): void | ||
| { | ||
| DB::connection('import_v1')->statement('DROP TABLE IF EXISTS ip_tax_rates'); | ||
| DB::connection('import_v1')->statement(' | ||
| CREATE TABLE ip_tax_rates ( | ||
| tax_rate_id INT PRIMARY KEY, | ||
| tax_rate_name VARCHAR(255), | ||
| tax_rate_percent DECIMAL(8,3) | ||
| ) | ||
| '); |
There was a problem hiding this comment.
These unit tests directly use DB::connection('import_v1'), but the test environment is configured for SQLite in-memory (no MySQL server/driver), so this will fail in CI. Consider overriding database.connections.import_v1 to an in-memory SQLite connection in setUp() (and purging the connection) or skipping these tests unless MySQL is available.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (3)
Modules/Core/Services/ImportInvoicePlaneV1Service.php (3)
23-23:⚠️ Potential issue | 🟠 MajorGenerate a per-run temp DB name.
A fixed staging schema lets concurrent imports drop or overwrite each other's data. Store the temp DB name on the service instance and use that value for every create/query/drop call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Services/ImportInvoicePlaneV1Service.php` at line 23, The temp DB currently uses a fixed constant TEMP_DB_NAME which causes collisions; replace that constant with an instance property (e.g. private string $tempDbName) assigned once per service instantiation (generate a unique name using timestamp/uniqid or random suffix in the ImportInvoicePlaneV1Service constructor or init method) and then use $this->tempDbName everywhere the code currently references TEMP_DB_NAME (create/database creation, query execution, and drop logic — look for methods like createDatabase, runQueries, dropDatabase or any calls that interpolate TEMP_DB_NAME) so every import run uses its own consistent per-instance temp DB name.
125-153:⚠️ Potential issue | 🟠 MajorUse
import_v1throughout instead ofmysql+exec().This still shells out to the CLI and talks to the default connection, so the import can create/drop schemas on the primary DB server instead of the isolated import connection. The later
DB::connection('mysql')reads inherit the same problem.#!/bin/bash # Verify the repo defines a dedicated import connection and this service bypasses it. rg -n -C2 "import_v1" config/database.php rg -n -C2 "database\.connections\.mysql|DB::connection\('mysql'\)|exec\(" Modules/Core/Services/ImportInvoicePlaneV1Service.phpExpected:
config/database.phpdefinesimport_v1, and this service should not still be hardcoded tomysqlorexec().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Services/ImportInvoicePlaneV1Service.php` around lines 125 - 153, The code currently shells out with exec() and uses DB::statement()/DB::connection('mysql') which targets the default connection; update createTemporaryDatabase() and restoreDump() to use the dedicated import connection (DB::connection('import_v1')) instead of the default 'mysql' and remove shell exec usage; specifically, replace DB::statement('DROP DATABASE...') and DB::statement('CREATE DATABASE...') in createTemporaryDatabase() with DB::connection('import_v1')->statement(...) calls, and in restoreDump() stop building a mysql CLI command and instead execute the dump against the import connection (e.g., using DB::connection('import_v1')->unprepared() or the import connection's driver methods to run the SQL file) rather than exec(), ensuring all subsequent DB::connection('mysql') usages are switched to DB::connection('import_v1').
68-80:⚠️ Potential issue | 🟠 MajorMake Step 4 atomic.
Any exception after the first insert leaves a partially imported tenant in v2. Wrap the import writes in a transaction, or explicitly delete the newly created company on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Services/ImportInvoicePlaneV1Service.php` around lines 68 - 80, The sequence of import calls (importTaxRates, importProductFamilies, importProductUnits, importProducts, importClients, importInvoiceGroups, importInvoices, importQuotes, importPayments) must be made atomic: wrap these write operations inside a database transaction (e.g. DB::beginTransaction()/DB::commit() with DB::rollBack() on catch, or use DB::transaction(fn() => { ... })) around the block in ImportInvoicePlaneV1Service so any exception rolls back all inserts and the method still returns/throws cleanly; if using a transaction is not possible in this context, ensure the catch path explicitly deletes the newly created company/tenant and cleans up created records before rethrowing the exception. Ensure you call rollback (or delete the company) in the catch and rethrow or return the error so partial imports are never left behind.
🧹 Nitpick comments (10)
Modules/Core/Enums/ModelType.php (1)
61-73: Silent fallback to INVOICE may mask import data issues.The
fromString()method defaults unrecognized$typevalues toself::INVOICE(line 71), which could silently misclassify imported notes. WhenNotesImportServiceencounters an unknownentity_typefrom v1, it will convert it toINVOICE, but thengetModelId()will try to look up theentity_idin the invoices mapping—returningnullif not found and silently skipping the note.Consider throwing an exception or logging a warning for unrecognized types to aid debugging during migration:
♻️ Proposed alternative with explicit handling
public static function fromString(string $type): self { return match (strtolower($type)) { 'invoice' => self::INVOICE, 'quote' => self::QUOTE, 'client' => self::CLIENT, 'payment' => self::PAYMENT, 'project' => self::PROJECT, 'task' => self::TASK, 'expense' => self::EXPENSE, - default => self::INVOICE, + default => throw new \InvalidArgumentException("Unknown ModelType: {$type}"), }; }Alternatively, if a fallback is desired, return
nullor use a nullable return type so the caller can handle unknown types explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Enums/ModelType.php` around lines 61 - 73, The fromString() method in ModelType currently defaults unknown types to self::INVOICE which silently misclassifies imports; update ModelType::fromString to stop returning a bogus default and instead either throw a descriptive exception (e.g., InvalidArgumentException with the original $type) or change the signature to return ?ModelType (nullable) and return null for unrecognized values so callers (like NotesImportService::getModelId) can handle unknown entity_type explicitly; ensure any callers are updated to handle the exception or null return (and add a clear log/warning path in NotesImportService when an unknown type is encountered).config/database.php (1)
63-78: Consider documentingIMPORT_DB_*environment variables.The new
import_v1connection uses dedicated environment variables (IMPORT_DB_HOST,IMPORT_DB_DATABASE, etc.) that aren't documented in.env.example. Operators following the example file won't know these can be configured separately from the main database.Additionally, unlike the main
mysqlconnection, this connection lacks SSL options. If the v1 import database is on a remote server, you may want to add:'options' => extension_loaded('pdo_mysql') ? array_filter([ PDO::MYSQL_ATTR_SSL_CA => env('IMPORT_MYSQL_ATTR_SSL_CA'), ]) : [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/database.php` around lines 63 - 78, Add documentation entries for the IMPORT_DB_* environment variables used by the import_v1 connection (IMPORT_DB_URL, IMPORT_DB_HOST, IMPORT_DB_PORT, IMPORT_DB_DATABASE, IMPORT_DB_USERNAME, IMPORT_DB_PASSWORD, IMPORT_DB_SOCKET) to .env.example so operators know they can configure the v1 import DB separately; also add an 'options' key to the import_v1 connection (mirroring the main mysql connection) that conditionally sets PDO::MYSQL_ATTR_SSL_CA from an IMPORT_MYSQL_ATTR_SSL_CA env var when pdo_mysql is loaded to enable SSL configuration for remote import databases.Modules/Core/Commands/IMPORT_README.md (1)
94-94: Minor: Capitalize "PayPal" correctly.Per static analysis, the official spelling is "PayPal" with two capital P's.
📝 Proposed fix
-- 4 → paypal +- 4 → PayPal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Commands/IMPORT_README.md` at line 94, Replace the lowercase "paypal" in the README list entry "- 4 → paypal" with the correct trademark capitalization "PayPal" so the entry reads "- 4 → PayPal"; update any other occurrences of the exact token "paypal" in IMPORT_README.md to "PayPal" for consistency.Modules/Core/Services/Import/ImportOrchestrator.php (1)
190-201: Unusedcleanup()method is intentional but could be documented.The static analysis correctly identifies this as unused, but the comment at line 56-57 explains it's retained for debugging. Consider adding a
@internalor@codeCoverageIgnoreannotation, or simply removing it if cleanup is never needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Services/Import/ImportOrchestrator.php` around lines 190 - 201, The private cleanup() method in ImportOrchestrator is intentionally unused for now; either annotate it so static analysis and coverage tools know it’s deliberate or remove it: add an annotation like `@internal` or `@codeCoverageIgnore` to the cleanup() method declaration (or, if you truly never need the debug cleanup, delete the cleanup() method and its comment), referencing the cleanup() method in the ImportOrchestrator class to locate the code.Modules/Core/Services/Import/QuotesImportService.php (1)
73-97: Consider adding type hint for$v1Itemsparameter.The
$v1Itemsparameter lacks a type hint, which reduces code clarity and IDE support.♻️ Proposed fix
- private function importQuoteItems($v1Items, int $v2QuoteId): void + private function importQuoteItems(\Illuminate\Support\Collection $v1Items, int $v2QuoteId): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Services/Import/QuotesImportService.php` around lines 73 - 97, The importQuoteItems method's $v1Items lacks a type hint; update the importQuoteItems signature to accept a concrete iterable type (e.g., iterable $v1Items or array $v1Items) and add a matching docblock (`@param` iterable|array<object> $v1Items) so IDEs and static analyzers know what to expect; change the function declaration for importQuoteItems and its docblock only (leave internal logic unchanged) and ensure callers still pass an array/iterable.Modules/Core/Tests/Unit/Services/Import/ProductsImportServiceTest.php (1)
177-191: Test could assert on$statsto validate import execution.Similar to the other import tests, asserting on
$statswould make the test more robust and address the static analysis warning.♻️ Proposed enhancement
/* Act */ $stats = $this->service->import($this->company->id, $this->idMappings); /* Assert */ + $this->assertEquals(1, $stats['product_units']); $unit = ProductUnit::where('company_id', $this->company->id)->first(); $this->assertEquals('Item', $unit->unit_name_plrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Tests/Unit/Services/Import/ProductsImportServiceTest.php` around lines 177 - 191, The test it_handles_unit_name_plural_fallback currently ignores the return value $stats from $this->service->import; add assertions that validate $stats to mirror other import tests (e.g., expected counts or keys like imported/updated/failed) so static analysis and robustness are satisfied. After calling $this->service->import(...) assert that $stats is the expected structure and values (not null, correct totals) before asserting the ProductUnit record; reference the $stats variable and the service->import(...) call when adding these assertions. Ensure the assertions align with the shape used across other tests in the suite to keep consistency.Modules/Core/Tests/Unit/Services/Import/ClientsImportServiceTest.php (1)
222-247: Test could assert on$statsto validate import execution.The static analysis flags
$statsas unused. While the test correctly validates the side effect (relation status), asserting on$statswould make the test more robust and silence the warning.♻️ Proposed enhancement
/* Act */ $stats = $this->service->import($this->company->id, $this->idMappings); /* Assert */ + $this->assertEquals(1, $stats['clients']); $relation = Relation::where('company_id', $this->company->id)->first(); $this->assertEquals('inactive', $relation->relation_status->value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Tests/Unit/Services/Import/ClientsImportServiceTest.php` around lines 222 - 247, The test it_handles_inactive_clients captures $stats from $this->service->import but never asserts on it; update the test to assert expected import statistics after calling Import/ClientsImportService::import (referenced as $this->service->import) — for example verify $stats contains the expected counts (e.g. relations imported/skipped or similar keys your import returns) and/or that $stats indicates one processed record; keep the existing Relation::where(...)->first() assertion intact and add one or two assertions on $stats to validate the import execution and silence the unused-variable warning.Modules/Core/Services/Import/AbstractImportService.php (1)
20-47: Table existence caching implemented correctly.The caching mechanism now prevents redundant
SHOW TABLESqueries, addressing the previous review concern. However, as per coding guidelines, consider catching\Throwableinstead of just\Exceptionto handleErrorandErrorExceptioncases.♻️ Optional: Catch Throwable for comprehensive error handling
- } catch (\Exception $e) { + } catch (\Throwable $e) { $this->tableExistsCache[$tableName] = false; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Services/Import/AbstractImportService.php` around lines 20 - 47, The catch block in tableExists currently catches \Exception only, which misses PHP Errors; update the catch in the tableExists method to catch \Throwable (e.g., catch (\Throwable $e)) so tableExistsCache[$tableName] is set to false and false returned for all failures; ensure the change is made inside the AbstractImportService::tableExists method and continues to set $this->tableExistsCache[$tableName] = false before returning, preserving the IMPORT_CONNECTION and tableExistsCache logic.Modules/Core/Services/Import/CustomFieldsImportService.php (1)
5-5: Remove the deadModelTypeparse.PHPMD is already flagging
$modelTypeas unused here. IfmapModelType()stays the source of truth, drop the extra parse and its import.♻️ Suggested cleanup
-use Modules\Core\Enums\ModelType; use Modules\Core\Models\CustomField; use Modules\Core\Models\CustomFieldValue; @@ - $modelType = ModelType::fromString($v1Value->entity_type ?? 'invoice'); - $modelId = $this->resolveModelId($v1Value->entity_type ?? 'invoice', $v1Value->entity_id ?? null);Also applies to: 56-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Services/Import/CustomFieldsImportService.php` at line 5, Remove the dead ModelType parsing: delete the unused import "ModelType" and remove the block that parses/assigns $modelType (the unused variable flagged by PHPMD) and rely solely on the existing mapModelType() implementation; search for references to ModelType, $modelType and mapModelType() in CustomFieldsImportService.php and remove the parsing code around lines where $modelType is assigned so only mapModelType() provides the model type.Modules/Core/Tests/Feature/ImportInvoicePlaneV1CommandTest.php (1)
288-291: Split the failure-path test into explicit Act and Assert phases.This is the only test still combining sections, and keeping the pending command in a variable matches the rest of the file’s structure.
As per coding guidelines, "All tests must have an 'act' section where variables are defined before assertions" and "Tests must have inline comment blocks above sections (/* Arrange */, /* Act */, /* Assert */).🧪 Suggested cleanup
- /* Act & Assert */ - $this->artisan('import:db', [ + /* Act */ + $result = $this->artisan('import:db', [ 'dumpfile' => $nonExistentFile, - ])->assertFailed(); + ]); + + /* Assert */ + $result->assertFailed();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Modules/Core/Tests/Feature/ImportInvoicePlaneV1CommandTest.php` around lines 288 - 291, The test currently combines Act and Assert by calling $this->artisan(...)->assertFailed() inline; split into explicit sections by introducing an Act phase that assigns the pending command to a variable (e.g. $command = $this->artisan('import:db', ['dumpfile' => $nonExistentFile]);) and then an Assert phase that calls $command->assertFailed(); also add the /* Act */ and /* Assert */ inline comment blocks above those sections to match the file's structure and guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@IMPLEMENTATION_SUMMARY.md`:
- Around line 100-103: The documentation has two issues: the payment method
"paypal" should be capitalized "PayPal", and the Quote status list includes
"canceled" which doesn't exist in the codebase—update the
IMPLEMENTATION_SUMMARY.md to change "paypal" to "PayPal" and remove or reconcile
"canceled" from the Quote statuses so it matches the QuoteStatus enum (which
currently defines DRAFT, SENT, VIEWED, APPROVED, REJECTED); if you intend to
support a canceled state instead, add a CANCELED case to the QuoteStatus enum
and handle it wherever QuoteStatus is used.
In `@Modules/Core/Commands/IMPORT_README.md`:
- Around line 217-220: Update the README bullet that says "Temporary database is
cleaned up after import" to match ImportOrchestrator's behavior by stating the
temporary database is retained for debugging and must be dropped manually;
locate the conflicting bullet in the import docs and replace the cleanup claim
with a note like "Temporary database is kept for debugging purposes and should
be manually dropped if needed" so the documentation aligns with the
ImportOrchestrator implementation.
In `@Modules/Core/Services/Import/ClientsImportService.php`:
- Around line 94-100: The Communication::create payload is using incorrect
attribute names (communicationable_id, communicationable_type,
communication_type, communication_value) so values won't persist; update the
create calls in ClientsImportService (the blocks that call
Communication::create) to use the model's expected fields
contactable_id/contactable_type (or contactable_type and contactable_value as
described) and contactable_value for the channel value, keep is_primary and
company_id as-is and set contactable_type to Contact::class and
contactable_value to the email/phone value from $v1Contact; apply the same
change to the second occurrence around the other Communication::create call.
- Around line 49-58: The address payload is using v1 keys 'zip' and 'state'
which don't map to v2 columns; update the Address::create call in
ClientsImportService (the block that creates Address with $this->companyId and
$relation->id) to use 'postal_code' instead of 'zip' and 'state_or_province'
instead of 'state' and keep mapping values from $v1Client->client_zip and
$v1Client->client_state respectively so imported postcode/state persist in v2.
In `@Modules/Core/Services/Import/ImportOrchestrator.php`:
- Around line 64-117: The CREATE DATABASE line in restoreDump() interpolates
$database directly (DB::connection()->statement("CREATE DATABASE IF NOT EXISTS
`{$database}`")), creating an SQL injection risk; validate and sanitize the
database name before use (e.g. enforce an allowed pattern like
/^[A-Za-z0-9$_]+$/ and throw if it fails) or map to a known whitelist, then
safely interpolate the validated identifier into the CREATE DATABASE statement
in the restoreDump method; also change the final catch from catch(\Exception $e)
to catch(\Throwable $e) so restoreDump() rethrows any Error or Exception as a
RuntimeException.
In `@Modules/Core/Services/Import/NotesImportService.php`:
- Around line 38-44: The call to the non-existent method mapModelType() in the
Note::create block will fatal; replace its use by using the already-created
ModelType instance ($modelType from ModelType::fromString()) and pass its string
value into the notable_type field (i.e., use $modelType->value) so notable_type
receives the correct morph key instead of calling a missing method.
In `@Modules/Core/Services/Import/NumberingImportService.php`:
- Around line 52-54: The code queries for numbering types using literal strings
('invoice' and 'quote') in Numbering::where and within applyNumberingLogic();
replace these literals with the NumberingType enum values (e.g.,
NumberingType::INVOICE->value and NumberingType::QUOTE->value) and update any
uses inside applyNumberingLogic() to use the enum instead of plain strings; also
ensure the NumberingType enum is imported/used at the top of
NumberingImportService so the replacements compile.
- Around line 31-39: Replace the hardcoded string 'invoice' passed to
Numbering::create with the enum constant NumberingType::INVOICE to match the
Numbering model's enum casting; update the call that creates the model
(Numbering::create([... 'type' => ...])) to use NumberingType::INVOICE so the
value aligns with the NumberingType enum.
In `@Modules/Core/Services/Import/PaymentsImportService.php`:
- Around line 39-51: The PaymentsImportService currently creates a Payment but
doesn't record the v1→v2 mapping, so downstream steps using
$this->idMappings['payments'] can't resolve linked data; modify the block that
calls Payment::create (in PaymentsImportService) to capture the returned Payment
model (e.g., $payment = Payment::create(...)) and then set
$this->idMappings['payments'][$v1Payment->id] = $payment->id (or ->getKey()),
then increment $this->stats['payments'] as before so later import stages can
resolve payment-linked records.
In `@Modules/Core/Services/Import/ProjectsImportService.php`:
- Around line 38-44: The Project creation is assigning the wrong field name so
descriptions are dropped; update the Project::create call in
ProjectsImportService (the array passed to Project::create) to use the database
column name "description" instead of "project_description" (i.e., set
'description' => $v1Project->project_description ?? null) so the imported
description is persisted; verify no other mismatched keys exist in the same
create array.
- Around line 62-69: In ProjectsImportService, the Task::create call is missing
the required customer_id; update the Task creation to include a valid
customer_id (e.g., set 'customer_id' => $v1Task->customer_id or the
mapped/translated customer ID if you maintain an ID map in this import service),
ensuring you use the same mapping/lookup logic you use elsewhere in
ProjectsImportService so the NOT NULL foreign key constraint is satisfied.
In `@Modules/Core/Services/Import/SettingsImportService.php`:
- Around line 30-38: The SettingsImportService is calling
Setting::updateOrCreate with a company_id key that doesn't exist in the settings
table; add a company_id column to the settings migration (e.g.,
unsignedBigInteger company_id, nullable or constrained as needed) or a new
migration to add it, and implement a SettingRepository with an upsert method
(e.g., upsertSetting(int $companyId, string $settingKey, string $settingValue))
that encapsulates the upsert logic and company scoping; then replace the direct
Setting::updateOrCreate call in SettingsImportService with a call to
SettingRepository::upsertSetting($this->companyId, $v1Setting->setting_key,
$v1Setting->setting_value ?? '') so the service uses the repository and no
longer depends on a non-existent column directly.
In `@Modules/Core/Services/Import/TaxRatesImportService.php`:
- Around line 30-36: In TaxRatesImportService replace the direct model call to
TaxRate::firstOrCreate (which creates $v2TaxRate from $v1TaxRate) with the
repository upsert/firstOrCreate method and use the correct column names and
required fields: map tax_name -> name and tax_rate -> rate, and include the
missing code and tax_rate_type attributes when building the payload; call the
repository method (e.g., TaxRateRepository::firstOrCreate or upsertTaxRate)
instead of TaxRate::firstOrCreate so the service follows repository usage.
In `@Modules/Core/Services/ImportInvoicePlaneV1Service.php`:
- Around line 378-409: The import currently calls importInvoiceItems (and
similar helpers used in invoice/quote import paths) for each invoice which
causes a DB query per document and repeated tableExists() checks; instead,
preload each relevant item table once (e.g., invoice_items, quote_items) before
iterating invoices/quotes by fetching all rows, group them in memory by parent
ID (invoice_id/quote_id), then update importInvoiceItems/importQuoteItems to
read from these in-memory groups (or accept the grouped array) and remove
per-document tableExists()/query logic so you iterate locally rather than
running thousands of DB roundtrips.
- Around line 59-62: The chosen user returned by getValidUserId() may not be
linked to the target company created by createCompany(), so update
ImportInvoicePlaneV1Service to validate inputs early and ensure the user is
attached to the company right after setting $this->companyId and $this->userId:
after calling $this->userId = $this->getValidUserId(), check whether that user
is a member of $this->companyId and if not create the association (e.g. via the
domain's CompanyUser/tenant membership API or by setting the user's company
relation) or throw an exception; update both the constructor/initializer and any
later code paths (see the similar block around lines 103-119) to enforce this
link consistently.
- Around line 350-358: The Numbering::create call is always setting 'type' =>
'invoice', causing quote imports (importQuotes) to point to invoice numberings;
update the code so when creating numberings for quotes you set 'type' => 'quote'
(and keep invoice creations as 'invoice'), specifically adjust the
Numbering::create invocation(s) used for quote_group_id mapping in
importQuotes() (and the similar block at the other occurrence) so the created
numbering's 'type' field reflects the resource (quote vs invoice) instead of
hardcoding 'invoice'.
- Around line 171-183: The tableExists method is swallowing all exceptions and
returning false, hiding DB connection/permission/query errors; update
tableExists(string $tableName) to catch and handle Error, ErrorException, and
Throwable separately instead of a blanket catch(\Exception), rethrow or log
non-"table missing" errors while only returning false for an expected "table not
found" outcome; locate the DB::select call with self::TEMP_DB_NAME inside
tableExists and implement differentiated error handling (e.g., catch specific
exceptions to rethrow or processLogger->error with details, and only treat
actual "no rows/zero count" as false).
- Around line 64-83: Move the temporary DB creation and dump restore into the
try so failures during restore still trigger the finally cleanup: call
createTemporaryDatabase() and restoreDump($dumpFile) immediately inside the try
block (before calling importTaxRates(), importProducts(), etc.), leaving
dropTemporaryDatabase() in the finally block so the staging DB is always removed
even if restoreDump() throws.
- Around line 199-206: The current use of TaxRate::firstOrCreate in
ImportInvoicePlaneV1Service collapses distinct source rows with identical
name/rate; replace the TaxRate::firstOrCreate call with TaxRate::create so each
$v1TaxRate always inserts a new v2 row (preserving 1:1 duplicates), e.g. call
TaxRate::create([...]) with the same attributes ('company_id', 'tax_name' =>
$v1TaxRate->tax_rate_name ?? 'Tax', 'tax_rate' => $v1TaxRate->tax_rate_percent
?? 0), and ensure you keep the returned $v2TaxRate model and record the mapping
from $v1TaxRate->tax_rate_id to $v2TaxRate->id if the code later relies on that
mapping.
---
Duplicate comments:
In `@Modules/Core/Services/ImportInvoicePlaneV1Service.php`:
- Line 23: The temp DB currently uses a fixed constant TEMP_DB_NAME which causes
collisions; replace that constant with an instance property (e.g. private string
$tempDbName) assigned once per service instantiation (generate a unique name
using timestamp/uniqid or random suffix in the ImportInvoicePlaneV1Service
constructor or init method) and then use $this->tempDbName everywhere the code
currently references TEMP_DB_NAME (create/database creation, query execution,
and drop logic — look for methods like createDatabase, runQueries, dropDatabase
or any calls that interpolate TEMP_DB_NAME) so every import run uses its own
consistent per-instance temp DB name.
- Around line 125-153: The code currently shells out with exec() and uses
DB::statement()/DB::connection('mysql') which targets the default connection;
update createTemporaryDatabase() and restoreDump() to use the dedicated import
connection (DB::connection('import_v1')) instead of the default 'mysql' and
remove shell exec usage; specifically, replace DB::statement('DROP DATABASE...')
and DB::statement('CREATE DATABASE...') in createTemporaryDatabase() with
DB::connection('import_v1')->statement(...) calls, and in restoreDump() stop
building a mysql CLI command and instead execute the dump against the import
connection (e.g., using DB::connection('import_v1')->unprepared() or the import
connection's driver methods to run the SQL file) rather than exec(), ensuring
all subsequent DB::connection('mysql') usages are switched to
DB::connection('import_v1').
- Around line 68-80: The sequence of import calls (importTaxRates,
importProductFamilies, importProductUnits, importProducts, importClients,
importInvoiceGroups, importInvoices, importQuotes, importPayments) must be made
atomic: wrap these write operations inside a database transaction (e.g.
DB::beginTransaction()/DB::commit() with DB::rollBack() on catch, or use
DB::transaction(fn() => { ... })) around the block in
ImportInvoicePlaneV1Service so any exception rolls back all inserts and the
method still returns/throws cleanly; if using a transaction is not possible in
this context, ensure the catch path explicitly deletes the newly created
company/tenant and cleans up created records before rethrowing the exception.
Ensure you call rollback (or delete the company) in the catch and rethrow or
return the error so partial imports are never left behind.
---
Nitpick comments:
In `@config/database.php`:
- Around line 63-78: Add documentation entries for the IMPORT_DB_* environment
variables used by the import_v1 connection (IMPORT_DB_URL, IMPORT_DB_HOST,
IMPORT_DB_PORT, IMPORT_DB_DATABASE, IMPORT_DB_USERNAME, IMPORT_DB_PASSWORD,
IMPORT_DB_SOCKET) to .env.example so operators know they can configure the v1
import DB separately; also add an 'options' key to the import_v1 connection
(mirroring the main mysql connection) that conditionally sets
PDO::MYSQL_ATTR_SSL_CA from an IMPORT_MYSQL_ATTR_SSL_CA env var when pdo_mysql
is loaded to enable SSL configuration for remote import databases.
In `@Modules/Core/Commands/IMPORT_README.md`:
- Line 94: Replace the lowercase "paypal" in the README list entry "- 4 →
paypal" with the correct trademark capitalization "PayPal" so the entry reads "-
4 → PayPal"; update any other occurrences of the exact token "paypal" in
IMPORT_README.md to "PayPal" for consistency.
In `@Modules/Core/Enums/ModelType.php`:
- Around line 61-73: The fromString() method in ModelType currently defaults
unknown types to self::INVOICE which silently misclassifies imports; update
ModelType::fromString to stop returning a bogus default and instead either throw
a descriptive exception (e.g., InvalidArgumentException with the original $type)
or change the signature to return ?ModelType (nullable) and return null for
unrecognized values so callers (like NotesImportService::getModelId) can handle
unknown entity_type explicitly; ensure any callers are updated to handle the
exception or null return (and add a clear log/warning path in NotesImportService
when an unknown type is encountered).
In `@Modules/Core/Services/Import/AbstractImportService.php`:
- Around line 20-47: The catch block in tableExists currently catches \Exception
only, which misses PHP Errors; update the catch in the tableExists method to
catch \Throwable (e.g., catch (\Throwable $e)) so tableExistsCache[$tableName]
is set to false and false returned for all failures; ensure the change is made
inside the AbstractImportService::tableExists method and continues to set
$this->tableExistsCache[$tableName] = false before returning, preserving the
IMPORT_CONNECTION and tableExistsCache logic.
In `@Modules/Core/Services/Import/CustomFieldsImportService.php`:
- Line 5: Remove the dead ModelType parsing: delete the unused import
"ModelType" and remove the block that parses/assigns $modelType (the unused
variable flagged by PHPMD) and rely solely on the existing mapModelType()
implementation; search for references to ModelType, $modelType and
mapModelType() in CustomFieldsImportService.php and remove the parsing code
around lines where $modelType is assigned so only mapModelType() provides the
model type.
In `@Modules/Core/Services/Import/ImportOrchestrator.php`:
- Around line 190-201: The private cleanup() method in ImportOrchestrator is
intentionally unused for now; either annotate it so static analysis and coverage
tools know it’s deliberate or remove it: add an annotation like `@internal` or
`@codeCoverageIgnore` to the cleanup() method declaration (or, if you truly never
need the debug cleanup, delete the cleanup() method and its comment),
referencing the cleanup() method in the ImportOrchestrator class to locate the
code.
In `@Modules/Core/Services/Import/QuotesImportService.php`:
- Around line 73-97: The importQuoteItems method's $v1Items lacks a type hint;
update the importQuoteItems signature to accept a concrete iterable type (e.g.,
iterable $v1Items or array $v1Items) and add a matching docblock (`@param`
iterable|array<object> $v1Items) so IDEs and static analyzers know what to
expect; change the function declaration for importQuoteItems and its docblock
only (leave internal logic unchanged) and ensure callers still pass an
array/iterable.
In `@Modules/Core/Tests/Feature/ImportInvoicePlaneV1CommandTest.php`:
- Around line 288-291: The test currently combines Act and Assert by calling
$this->artisan(...)->assertFailed() inline; split into explicit sections by
introducing an Act phase that assigns the pending command to a variable (e.g.
$command = $this->artisan('import:db', ['dumpfile' => $nonExistentFile]);) and
then an Assert phase that calls $command->assertFailed(); also add the /* Act */
and /* Assert */ inline comment blocks above those sections to match the file's
structure and guidelines.
In `@Modules/Core/Tests/Unit/Services/Import/ClientsImportServiceTest.php`:
- Around line 222-247: The test it_handles_inactive_clients captures $stats from
$this->service->import but never asserts on it; update the test to assert
expected import statistics after calling Import/ClientsImportService::import
(referenced as $this->service->import) — for example verify $stats contains the
expected counts (e.g. relations imported/skipped or similar keys your import
returns) and/or that $stats indicates one processed record; keep the existing
Relation::where(...)->first() assertion intact and add one or two assertions on
$stats to validate the import execution and silence the unused-variable warning.
In `@Modules/Core/Tests/Unit/Services/Import/ProductsImportServiceTest.php`:
- Around line 177-191: The test it_handles_unit_name_plural_fallback currently
ignores the return value $stats from $this->service->import; add assertions that
validate $stats to mirror other import tests (e.g., expected counts or keys like
imported/updated/failed) so static analysis and robustness are satisfied. After
calling $this->service->import(...) assert that $stats is the expected structure
and values (not null, correct totals) before asserting the ProductUnit record;
reference the $stats variable and the service->import(...) call when adding
these assertions. Ensure the assertions align with the shape used across other
tests in the suite to keep consistency.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 982cbbdd-a562-44f0-87aa-66aa06ec2bc1
📒 Files selected for processing (29)
IMPLEMENTATION_SUMMARY.mdModules/Core/Commands/IMPORT_README.mdModules/Core/Commands/ImportInvoicePlaneV1Command.phpModules/Core/Enums/ModelType.phpModules/Core/Providers/CoreServiceProvider.phpModules/Core/Services/Import/AbstractImportService.phpModules/Core/Services/Import/ClientsImportService.phpModules/Core/Services/Import/CustomFieldsImportService.phpModules/Core/Services/Import/EmailTemplatesImportService.phpModules/Core/Services/Import/ImportOrchestrator.phpModules/Core/Services/Import/ImportServiceInterface.phpModules/Core/Services/Import/InvoicesImportService.phpModules/Core/Services/Import/NotesImportService.phpModules/Core/Services/Import/NumberingImportService.phpModules/Core/Services/Import/PaymentsImportService.phpModules/Core/Services/Import/ProductsImportService.phpModules/Core/Services/Import/ProjectsImportService.phpModules/Core/Services/Import/QuotesImportService.phpModules/Core/Services/Import/SettingsImportService.phpModules/Core/Services/Import/TaxRatesImportService.phpModules/Core/Services/Import/UsersImportService.phpModules/Core/Services/ImportInvoicePlaneV1Service.phpModules/Core/Tests/Feature/ImportInvoicePlaneV1CommandTest.phpModules/Core/Tests/Fixtures/test_invoiceplane_v1_dump.sqlModules/Core/Tests/Unit/Services/Import/ClientsImportServiceTest.phpModules/Core/Tests/Unit/Services/Import/ProductsImportServiceTest.phpModules/Core/Tests/Unit/Services/Import/TaxRatesImportServiceTest.phpModules/Core/Tests/Unit/Services/ImportInvoicePlaneV1ServiceTest.phpconfig/database.php
| private function tableExists(string $tableName): bool | ||
| { | ||
| try { | ||
| $result = DB::select( | ||
| "SELECT COUNT(*) as count FROM information_schema.tables | ||
| WHERE table_schema = ? AND table_name = ?", | ||
| [self::TEMP_DB_NAME, $tableName] | ||
| ); | ||
|
|
||
| return $result[0]->count > 0; | ||
| } catch (\Exception $e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Don't swallow database errors as "table missing".
Returning false for every exception turns connection/permission/query failures into silent skips, so the import can appear successful with missing data.
As per coding guidelines, "Catch Error, ErrorException, and Throwable separately".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Modules/Core/Services/ImportInvoicePlaneV1Service.php` around lines 171 -
183, The tableExists method is swallowing all exceptions and returning false,
hiding DB connection/permission/query errors; update tableExists(string
$tableName) to catch and handle Error, ErrorException, and Throwable separately
instead of a blanket catch(\Exception), rethrow or log non-"table missing"
errors while only returning false for an expected "table not found" outcome;
locate the DB::select call with self::TEMP_DB_NAME inside tableExists and
implement differentiated error handling (e.g., catch specific exceptions to
rethrow or processLogger->error with details, and only treat actual "no
rows/zero count" as false).
| $numbering = Numbering::create([ | ||
| 'company_id' => $this->companyId, | ||
| 'type' => 'invoice', | ||
| 'name' => $group->invoice_group_name, | ||
| 'next_id' => $group->invoice_group_next_id ?? 1, | ||
| 'left_pad' => 0, | ||
| 'format' => $group->invoice_group_prefix ?? 'INV', | ||
| 'prefix' => $group->invoice_group_prefix ?? 'INV', | ||
| ]); |
There was a problem hiding this comment.
Don't wire quotes to invoice numberings.
All imported numberings are created with type = 'invoice', but importQuotes() reuses the same mapping for quote_group_id. That leaves quotes pointing at the wrong numbering type.
Also applies to: 465-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Modules/Core/Services/ImportInvoicePlaneV1Service.php` around lines 350 -
358, The Numbering::create call is always setting 'type' => 'invoice', causing
quote imports (importQuotes) to point to invoice numberings; update the code so
when creating numberings for quotes you set 'type' => 'quote' (and keep invoice
creations as 'invoice'), specifically adjust the Numbering::create invocation(s)
used for quote_group_id mapping in importQuotes() (and the similar block at the
other occurrence) so the created numbering's 'type' field reflects the resource
(quote vs invoice) instead of hardcoding 'invoice'.
- ClientsImportService: Use postal_code/state_or_province for Address - ClientsImportService: Use contactable_type/contactable_value for Communication - NotesImportService: Use $modelType->value instead of non-existent mapModelType() - NumberingImportService: Use NumberingType::INVOICE enum instead of string - ImportOrchestrator: Add database name validation (alphanumeric + $_) - ImportOrchestrator: Catch \Throwable instead of \Exception for better error handling Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane-v2/sessions/51c0a25b-ecc7-4eaf-b336-17e8eb8d4430 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
- UsersImportService: Attach users to company (both new and existing) - NumberingImportService: Use NumberingType enum values in where clauses - NumberingImportService: Fix max() logic - pluck all numbers, extract numeric parts, find max - SettingsImportService: Remove company_id (Settings table is global) - TaxRatesImportService: Use correct field names (name, rate, code, tax_rate_type) - TaxRatesImportService: Add TaxRateType::SALES as default type Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane-v2/sessions/981e7574-6fa1-4881-9063-eaee7c105870 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 17 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 5 file(s) based on 17 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
@copilot apply changes based on the comments in this thread
apply changes based on the comments in this thread
- CustomFieldsImportService: Add resolveModelId() method to fix fatal error - CustomFieldsImportService: Use ModelType::fromString()->value directly - ImportOrchestrator: Remove unused Storage import - ImportOrchestrator: Create database on import_v1 server using PDO - ClientsImportService: Remove unused ip_client_notes from getTables() - ImportInvoicePlaneV1CommandTest: Fix argument name (filename not dumpfile) - ImportInvoicePlaneV1CommandTest: Copy fixture to storage/app/private/imports - Unit tests: Configure import_v1 to use SQLite in-memory for CI compatibility - IMPLEMENTATION_SUMMARY.md: Fix usage examples (filename only, not full path) Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane-v2/sessions/a0fb0fe9-f728-4766-a34e-ad002b4363a1 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
@copilot
The correct mapping should be:
// For CustomField:
CustomField::create([
'company_id' => $this->companyId,
'fieldable_type' => $v1Field->custom_field_table ?? 'invoices', // Changed from custom_field_table
'custom_field_label' => $v1Field->custom_field_label ?? 'Custom Field',
// Remove custom_field_column - it doesn't exist in v2
]);
// For CustomFieldValue:
CustomFieldValue::create([
'company_id' => $this->companyId,
'custom_field_id' => $customFieldId,
'fieldable_id' => $v1Value->entity_id ?? null, // Changed from model_id
'fieldable_type' => $this->mapModelType($v1Value->entity_type ?? 'invoice'), // Changed from model_type
'custom_field_value' => $v1Value->custom_field_value ?? '',
]);
Verify each finding against the current code and only fix it if needed.
In @Modules/Core/Services/Import/EmailTemplatesImportService.php around lines 30
- 38, The EmailTemplate::create call is using incorrect prefixed keys so columns
like title/type/subject/body/from_name/from_email are never set; update the
associative array passed to EmailTemplate::create to map the correct column
names (use 'title', 'type', 'subject', 'body', 'from_name', 'from_email') and
assign them from the existing $v1Template properties (e.g.
$v1Template->email_template_title etc.), keeping 'company_id' as
$this->companyId.
Verify each finding against the current code and only fix it if needed.
In @Modules/Core/Services/Import/ImportOrchestrator.php around lines 151 - 166,
getValidUserId() currently uses User::first() and creates a new user without
associating it to the current tenant; replace that logic to scope the lookup to
users belonging to $this->companyId (use whereHas('companies', fn($q) =>
$q->where('companies.id', $this->companyId))) and when creating a default user,
attach it to the company via $user->companies()->attach($this->companyId)
(follow the pattern in AbstractSeeder::findOrCreateUser() for querying and
attaching).
Note::create([
'company_id' => $this->companyId,
'notable_id' => $modelId,
- 'notable_type' => $this->mapModelType($v1Note->entity_type ?? 'invoice'),
+ 'notable_type' => $modelType->value,
'title' => $v1Note->note_title ?? 'Note',
'content' => $v1Note->note ?? '',
]);
+use Modules\Core\Enums\NumberingType;
+
// In importInvoiceGroups():
$numbering = Numbering::create([
'company_id' => $this->companyId,
- 'type' => 'invoice',
+ 'type' => NumberingType::INVOICE,
'name' => $group->invoice_group_name,
// ...
]);
+use Modules\Core\Enums\NumberingType;
+
// Line 52-54:
$numberings = Numbering::where('company_id', $companyId)
- ->where('type', 'invoice')
+ ->where('type', NumberingType::INVOICE)
->get();
// Line 77-79:
$quoteNumberings = Numbering::where('company_id', $companyId)
- ->where('type', 'quote')
+ ->where('type', NumberingType::QUOTE)
->get();
[Services/Import/ProjectsImportService.php](https://github.com/InvoicePlane/InvoicePlane-v2/pull/418/changes#r2835813262) Lines 38 - 42
Project status assignment may cause type mismatch. The code assigns a raw string from v1 data to 'project_status', but the Project model casts this field to ProjectStatus enum. The v1 data may not contain valid enum values like 'planned', 'active', etc.
Either:
Add a mapping method similar to mapInvoiceStatus() to convert v1 project statuses to v2 ProjectStatus enum values
Validate that v1 project_status values match the v2 enum values
Default to a safe value like ProjectStatus::ACTIVE->value if the status is unknown
[ore/Services/Import/ProjectsImportService.php](https://github.com/InvoicePlane/InvoicePlane-v2/pull/418/changes#r2835813237) Line 69
Task status assignment may cause type mismatch. Similar to the project status issue, the code assigns a raw string from v1 data to 'task_status', but the Task model casts this field to TaskStatus enum. Without validation or mapping, this could fail if v1 uses different status values.
Add a mapping method or validate against TaskStatus enum values.
[Services/Import/ProjectsImportService.php](https://github.com/InvoicePlane/InvoicePlane-v2/pull/418/changes#r2835813266) Lines 63 - 71
Missing required 'customer_id' field when creating tasks. The Task model requires a 'customer_id' field (line 21 in Task.php), but the import service only sets 'company_id', 'project_id', and task-specific fields. This will cause a database error or constraint violation.
Add customer_id to the create() call:
Task::create([
'company_id' => $this->companyId,
'project_id' => $projectId,
'customer_id' => $this->idMappings['projects_to_clients'][$projectId] ?? null, // Need to track project->client mapping
// ... rest of fields
]);
However, this requires tracking which client each project belongs to.
Missing required 'customer_id' field when creating tasks. The Task model requires a 'customer_id' field (line 21 in Task.php), but the import service only sets 'company_id', 'project_id', and task-specific fields. This will cause a database error or constraint violation.
Add customer_id to the create() call:
Task::create([
'company_id' => $this->companyId,
'project_id' => $projectId,
'customer_id' => $this->idMappings['projects_to_clients'][$projectId] ?? null, // Need to track project->client mapping
// ... rest of fields
]);
However, this requires tracking which client each project belongs to.
+ $project = Project::find($projectId);
+
Task::create([
'company_id' => $this->companyId,
+ 'customer_id' => $project->customer_id,
'project_id' => $projectId,
'task_name' => $v1Task->task_name,
'task_description' => $v1Task->task_description ?? null,
'task_status' => $v1Task->task_status ?? 'pending',
'task_price' => $v1Task->task_price ?? 0,
]);
Setting model does not have company_id field. The Setting model (Setting.php) doesn't show a company_id property in its docblock (lines 10-14), and settings appear to be global rather than company-specific. However, the SettingsImportService tries to scope settings by company_id.
Either:
Remove company_id from the Setting import if settings are global
Verify that the settings table has been updated to support multi-tenancy with a company_id column
Document this design decision
Verify each finding against the current code and only fix it if needed.
In @Modules/Core/Services/Import/SettingsImportService.php around lines 30 - 38,
The SettingsImportService is calling Setting::updateOrCreate with a company_id
key that doesn't exist in the settings table; add a company_id column to the
settings migration (e.g., unsignedBigInteger company_id, nullable or constrained
as needed) or a new migration to add it, and implement a SettingRepository with
an upsert method (e.g., upsertSetting(int $companyId, string $settingKey, string
$settingValue)) that encapsulates the upsert logic and company scoping; then
replace the direct Setting::updateOrCreate call in SettingsImportService with a
call to SettingRepository::upsertSetting($this->companyId,
$v1Setting->setting_key, $v1Setting->setting_value ?? '') so the service uses
the repository and no longer depends on a non-existent column directly.
Verify each finding against the current code and only fix it if needed.
In @Modules/Core/Services/Import/TaxRatesImportService.php around lines 30 - 36,
In TaxRatesImportService replace the direct model call to TaxRate::firstOrCreate
(which creates $v2TaxRate from $v1TaxRate) with the repository
upsert/firstOrCreate method and use the correct column names and required
fields: map tax_name -> name and tax_rate -> rate, and include the missing code
and tax_rate_type attributes when building the payload; call the repository
method (e.g., TaxRateRepository::firstOrCreate or upsertTaxRate) instead of
TaxRate::firstOrCreate so the service follows repository usage.
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
@copilot address the remaining code-review comments and mark them as outdated after they have been addressed.
After that go over all my code and apply my rules:
- SOLID programming
- DRY Programming
- Dynamic Programming
- Early Returns
Import System - Complete Migration Solution
Implements database migration from InvoicePlane v1 to v2 via
php artisan import:db <filename> [--company_id=<id>]with modular architecture, comprehensive testing, and production-ready optimizations.Latest Fixes (commit)
resolveModelId()methodModelType::fromString()->valuedirectly (no undefinedmapModelType())Storageimportip_client_notesfromgetTables()filenamenotdumpfile)storage/app/private/importsbefore runningimport_v1to use SQLite in-memory for CI compatibilityPrevious Fixes
postal_codeandstate_or_provincecontactable_typeandcontactable_value$modelType->valueNumberingType::INVOICEenum for type safety\Throwablefor comprehensive error coverageArchitecture
13 Modular Import Services:
Key Components:
ImportServiceInterface- Contract for all import servicesAbstractImportService- Base class with common functionalityImportOrchestrator- Coordinates all services with proper user scoping and securityModelTypeenum - Consistent model class mappingsNumberingTypeenum - Type-safe numbering typesTaxRateTypeenum - Type-safe tax rate typesFeatures
Multi-Tenancy Support:
--company_idoptioncompany_userpivot tableData Integrity:
Security:
Performance Optimizations:
Database Handling:
import_v1in config/database.phpCommand Usage
Place your v1 dump file in
storage/app/private/imports/directory:Testing
Comprehensive PHPUnit Coverage:
Edge Cases Covered:
All tests follow AAA (Arrange, Act, Assert) pattern with proper setup/teardown.
Schema Coverage
~25+ tables imported from InvoicePlane v1:
Documentation
Detailed documentation in
Modules/Core/Commands/IMPORT_README.mdcovering:✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
New Features
php artisan import:dbcommandDocumentation
Tests