app/Database/Migrations/20260427000000_AccountingModule.php (2)

78-145: 🏗️ Heavy lift

No foreign key constraints defined — referential integrity is unenforced.

accounting_items.entry_id → accounting_entries.entry_id, accounting_items.account_id → accounts.account_id, accounting_entries.journal_id → journals.journal_id, and accounts.parent_id → accounts.account_id are all logical FK relationships with no database-level enforcement. For financial data this means orphaned line items (e.g., after a journal is deleted) will go undetected.

♻️ Example: add FK constraints via Forge

// After createTable('accounting_entries'):
$this->forge->addForeignKey('journal_id', 'journals', 'journal_id', 'CASCADE', 'RESTRICT');
$this->forge->processIndexes('accounting_entries');

// After createTable('accounting_items'):
$this->forge->addForeignKey('entry_id',   'accounting_entries', 'entry_id',   'CASCADE', 'RESTRICT');
$this->forge->addForeignKey('account_id', 'accounts',           'account_id', 'RESTRICT', 'RESTRICT');
$this->forge->processIndexes('accounting_items');

The down() method would need to call $this->forge->dropForeignKey() before dropping tables.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 78
- 145, The migration creates accounting_entries, accounting_items and accounts
relations but never adds FK constraints, so referential integrity
(accounting_items.entry_id → accounting_entries.entry_id,
accounting_items.account_id → accounts.account_id, accounting_entries.journal_id
→ journals.journal_id, accounts.parent_id → accounts.account_id) is unenforced;
fix by calling $this->forge->addForeignKey(...) after creating the respective
tables (use the same column names: entry_id, item_id/account_id, journal_id,
parent_id) with appropriate ON DELETE/ON UPDATE actions (e.g., CASCADE for
entry→items and RESTRICT for account links) and then
$this->forge->processIndexes('<table_name>') for each table, and update the
down() method to call $this->forge->dropForeignKey('<table>', '<column>') before
dropping the tables so rollbacks remove FKs cleanly.

---

147-173: ⚡ Quick win

Use Query Builder instead of raw SQL string concatenation for seed inserts.

All seed and metadata inserts use $this->db->query() with string concatenation. While values are hardcoded literals (no injection risk here), this violates the project's guideline on parameterized queries and bypasses the query builder's automatic prefix handling. Use $this->db->table()->insert() / insertBatch() instead:

♻️ Proposed refactor for seed inserts

-// Insert into modules
-$this->db->query("INSERT INTO " . $this->db->prefixTable('modules') . " (name_lang_key, desc_lang_key, sort, module_id) VALUES ('module_accounting', 'module_accounting_desc', 90, 'accounting')");
-
-// Insert into permissions
-$this->db->query("INSERT INTO " . $this->db->prefixTable('permissions') . " (permission_id, module_id) VALUES ('accounting', 'accounting')");
+$this->db->table('modules')->insert([
+    'name_lang_key' => 'module_accounting',
+    'desc_lang_key' => 'module_accounting_desc',
+    'sort'          => 90,
+    'module_id'     => 'accounting',
+]);
+$this->db->table('permissions')->insert([
+    'permission_id' => 'accounting',
+    'module_id'     => 'accounting',
+]);

-// Seed some basic data
-$this->db->query("INSERT INTO " . $this->db->prefixTable('journals') . " (name, code, type) VALUES 
-    ('Sales Journal', 'SALES', 'Sale'),
-    ('Purchase Journal', 'PURCH', 'Purchase'),
-    ('Cash Journal', 'CASH', 'Cash'),
-    ('General Journal', 'GEN', 'General')
-");
-
-$this->db->query("INSERT INTO " . $this->db->prefixTable('accounts') . " (code, name, type) VALUES 
-    ('1000', 'Cash and Cash Equivalents', 'Asset'),
-    ...
-");
+$this->db->table('journals')->insertBatch([
+    ['name' => 'Sales Journal',    'code' => 'SALES', 'type' => 'Sale'],
+    ['name' => 'Purchase Journal', 'code' => 'PURCH', 'type' => 'Purchase'],
+    ['name' => 'Cash Journal',     'code' => 'CASH',  'type' => 'Cash'],
+    ['name' => 'General Journal',  'code' => 'GEN',   'type' => 'General'],
+]);
+$this->db->table('accounts')->insertBatch([
+    ['code' => '1000', 'name' => 'Cash and Cash Equivalents', 'type' => 'Asset'],
+    ['code' => '1100', 'name' => 'Accounts Receivable',       'type' => 'Asset'],
+    ['code' => '1200', 'name' => 'Inventory',                 'type' => 'Asset'],
+    ['code' => '2000', 'name' => 'Accounts Payable',          'type' => 'Liability'],
+    ['code' => '3000', 'name' => 'Owner Equity',              'type' => 'Equity'],
+    ['code' => '4000', 'name' => 'Sales Revenue',             'type' => 'Income'],
+    ['code' => '5000', 'name' => 'Cost of Goods Sold',        'type' => 'Expense'],
+    ['code' => '6000', 'name' => 'Operating Expenses',        'type' => 'Expense'],
+]);

Apply the same pattern for the down() DELETE statements (lines 187–189), replacing raw queries with $this->db->table('grants')->where('permission_id', 'accounting')->delete() etc.

As per coding guidelines: "Use parameterized queries to prevent SQL injection" (app/Database/Migrations/**/*.php).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 147
- 173, Replace raw SQL queries that call $this->db->query(...) and use
$this->db->prefixTable(...) with Query Builder calls: locate the inserts for
'modules', 'permissions', 'grants', 'journals', and 'accounts' and change them
to $this->db->table('modules')->insert(... ) or ->insertBatch(...) as
appropriate (use insert for single row and insertBatch for multiple rows). Do
the same in down() by replacing raw DELETE queries with
$this->db->table('grants')->where('permission_id','accounting')->delete(),
$this->db->table('permissions')->where('permission_id','accounting')->delete(),
and $this->db->table('modules')->where('module_id','accounting')->delete().
Ensure values are provided as arrays so the query builder handles prefixes and
parameterization.

app/Models/Account.php (1)

21-80: ⚡ Quick win

Missing PHP 8.1+ return type and parameter type declarations on all methods.

Per the coding guidelines, PHP 8.1+ code requires proper type declarations. None of the five public/protected methods in this model declare parameter types or return types.

♻️ Proposed fix

-public function get_info($account_id)
+public function get_info(int $account_id): object

-public function get_all()
+public function get_all(): array

-public function get_by_code($code)
+public function get_by_code(string $code): ?object

-public function exists($account_id)
+public function exists(int $account_id): bool

-public function save_value(&$account_data, $account_id = false)
+public function save_value(array &$account_data, int|false $account_id = false): bool

As per coding guidelines: "Write PHP 8.1+ compatible code with proper type declarations" (app/Models/**/*.php).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Models/Account.php` around lines 21 - 80, Update all public methods in
Account model to include PHP 8.1+ parameter and return type declarations:
annotate get_info($account_id) with a typed $account_id (int|string) and a
return type (object or specific model row class / stdClass), get_all() with
return type (ResultInterface or iterable/array as appropriate),
get_by_code(string $code): ?object, exists(int|string $account_id): bool, and
save_value(array &$account_data, int|string|false $account_id = false): bool;
ensure nullable returns use ? and imported types (or fully-qualified names) are
used consistently, and adjust any callers/tests if types change.

app/Models/Accounting_entry.php (1)

73-93: ⚡ Quick win

Items are deleted before balance validation - verify rollback restores them.

On update, existing items are deleted (lines 74-78) before the balance validation (lines 80-93). If validation fails and transRollback() is called, the transaction should restore the deleted items. However, this ordering is risky—if any code path between delete and rollback causes an exception, items could be orphaned.

Consider validating balance before deleting existing items.

Safer ordering: validate before delete

 public function save_entry(&$entry_data, &$items_data, $entry_id = false)
 {
+    // Validate items balance BEFORE starting transaction
+    $total_debit = 0.0;
+    $total_credit = 0.0;
+    
+    foreach ($items_data as $item) {
+        $total_debit += (float) $item['debit'];
+        $total_credit += (float) $item['credit'];
+    }
+    
+    if (abs($total_debit - $total_credit) > 0.01) {
+        return false;
+    }
+
     $this->db->transStart();

     // Save Header
     if (!$entry_id || !$this->exists($entry_id)) {
         // ... insert logic
     } else {
         // ... update logic
     }

     // Delete existing items for update
     if ($entry_id) {
         $builder = $this->db->table('accounting_items');
         $builder->where('entry_id', $entry_id);
         $builder->delete();
     }

-    // Validate items balance
-    $total_debit = 0.0;
-    $total_credit = 0.0;
-    
-    foreach ($items_data as $item) {
-        $total_debit += (float) $item['debit'];
-        $total_credit += (float) $item['credit'];
-    }
-    
-    if (abs($total_debit - $total_credit) > 0.01) {
-        // Unbalanced entry, rollback
-        $this->db->transRollback();
-        return false;
-    }

     // Save Items
     foreach ($items_data as $item) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Models/Accounting_entry.php` around lines 73 - 93, Existing
accounting_items are deleted prior to validating the new items' balance, which
risks orphaning rows if an exception occurs before transRollback; change the
logic in the Accounting_entry model so that the balance check on $items_data
(the loop computing $total_debit/$total_credit and the abs(...) check) runs
before any deletion of existing items for $entry_id, and only after the
validation passes perform the delete via the
$this->db->table('accounting_items')->where('entry_id', $entry_id)->delete();
ensure this delete remains inside the same DB transaction so transRollback can
restore changes if later steps fail.

app/Controllers/Accounting.php (2)

25-30: 💤 Low value

getIndex() and getReports() are nearly identical.

Both methods fetch the same data (balance_sheet, profit_loss) and render different views. Consider extracting a private helper to reduce duplication.

Also applies to: 53-58

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Controllers/Accounting.php` around lines 25 - 30, Extract the duplicated
data-fetching into a private helper inside the Accounting class: create a method
(e.g., private function renderReportView(string $view): string) that calls
$this->accounting_entry->get_balance_sheet() and
$this->accounting_entry->get_profit_loss(), builds the $data array, and returns
view($view, $data); then refactor getIndex() and getReports() to simply call
this helper with 'accounting/dashboard' and the other report view respectively
so both methods are one-liners that delegate to the new helper.

---

38-51: 💤 Low value

Consider encapsulating query logic in the model.

The getEntries() method contains direct database query building that would be better placed in the Accounting_entry model. This improves testability and follows CodeIgniter 4 patterns where models handle data access.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Controllers/Accounting.php` around lines 38 - 51, The getEntries()
controller currently builds the DB query directly; move that logic into the
Accounting_entry model by adding a method (e.g., getRecentEntries or findRecent)
that constructs the builder with the same select, joins (journals j, people p),
where('e.deleted', 0), orderBy('e.date','DESC') and limit(100) and returns the
result set; then update Accounting::getEntries() to instantiate or load
Accounting_entry (e.g., $this->accountingEntryModel or new Accounting_entry())
and call the new model method to get $entries and pass them to
view('accounting/manage_entries', $data). Ensure the model method uses the
proper table alias/primary key and returns getResult() so controller no longer
contains query building code.

app/Views/reports/listing.php (1)

87-110: 💤 Low value

Panel title doesn't match content scope.

The panel heading uses lang('Reports.top_50_items') but the panel also contains "Top Categories" links. Consider using a more generic title like lang('Reports.top_reports').

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Views/reports/listing.php` around lines 87 - 110, The panel heading
currently calls lang('Reports.top_50_items') but the panel also includes links
for reports_top_categories, so change the title to a more generic key (e.g.,
lang('Reports.top_reports')) to accurately reflect both items and categories;
update the call in the panel-heading h3 (replace lang('Reports.top_50_items')
with lang('Reports.top_reports')) and ensure the new language key exists in your
language files or add it if missing.

app/Database/Migrations/20260430000000_AddTopReportsPermissions.php (1)

12-21: 💤 Low value

Query builder approach would be more idiomatic.

While the static analysis SQL injection warnings are false positives (no user input), using CodeIgniter's query builder would be more consistent with the codebase patterns seen in other migrations.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Database/Migrations/20260430000000_AddTopReportsPermissions.php` around
lines 12 - 21, Replace raw SQL in AddTopReportsPermissions::up() with
CodeIgniter's query builder: use $this->db->table(...)->insert(...) (or
insertBatch for multiple rows) for the 'permissions' and 'grants' inserts
instead of $this->db->query(...). Keep using
$this->db->prefixTable('permissions') / prefixTable('grants') to resolve table
names, and perform two inserts (or batch) into the permissions table for
'reports_top_items' and 'reports_top_categories' and corresponding inserts into
the grants table for person_id 1 and menu_group 'home'.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/Database/Migrations/20260428000000_AccountingMexico.php (1)

11-11: ⚡ Quick win

Prefer $this->db over \Config\Database::connect().

The database connection is available to you directly through $this->db in CI4 migrations. Using \Config\Database::connect() bypasses the migration's own pre-wired connection property and is non-idiomatic. Both resolve to the same default group in practice, but $this->db is the idiomatic approach and ensures consistency if a $DBGroup is ever set on this migration.

♻️ Proposed fix (applies to both `up()` and `down()`)

-        $db = \Config\Database::connect();
-
-        // 1. Update existing default accounts to SAT codes
-        $db->table('accounts')->...
+        // 1. Update existing default accounts to SAT codes
+        $this->db->table('accounts')->...

Remove the $db local variable entirely and replace all $db-> calls with $this->db->.

Also applies to: 50-50

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260428000000_AccountingMexico.php` at line 11,
Replace the non-idiomatic manual connection in the migration by removing the
local $db = \Config\Database::connect() and use the migration's pre-wired
connection ($this->db) instead; update all occurrences of $db->... inside the
AccountingMexico class (both up() and down() methods) to $this->db->...,
ensuring you do not introduce a new local variable and that any $db references
are fully replaced.

app/Database/Migrations/20260427000000_AccountingModule.php (1)

31-32: ⚡ Quick win

Use the proper CI4 Forge ENUM syntax with a constraint array.

The CI4 Forge API requires ENUM to be declared with 'type' => 'ENUM' and 'constraint' => [...] as an array of allowed values. The current form — 'type' => 'ENUM("Asset", ...)' — embeds raw MySQL syntax in the type string, bypassing CI4's type-normalization layer. This happens to work on MySQL via raw passthrough but will break on non-MySQL databases and is non-idiomatic.

♻️ Proposed fix for both ENUM fields

-            'type' => [
-                'type' => 'ENUM("Asset", "Liability", "Equity", "Income", "Expense")',
-                'default' => 'Asset',
-            ],
+            'type' => [
+                'type'       => 'ENUM',
+                'constraint' => ['Asset', 'Liability', 'Equity', 'Income', 'Expense'],
+                'default'    => 'Asset',
+            ],

-            'type' => [
-                'type' => 'ENUM("Sale", "Purchase", "Cash", "Bank", "General")',
-                'default' => 'General',
-            ],
+            'type' => [
+                'type'       => 'ENUM',
+                'constraint' => ['Sale', 'Purchase', 'Cash', 'Bank', 'General'],
+                'default'    => 'General',
+            ],

Also applies to: 65-66

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 31
- 32, The migration uses raw MySQL ENUM syntax ('type' => 'ENUM("Asset",
"Liability", "Equity", "Income", "Expense")') which must be converted to CI4
Forge's ENUM form; in the AccountingModule migration replace those entries with
'type' => 'ENUM' and add 'constraint' =>
['Asset','Liability','Equity','Income','Expense'] (and do the same for the
second ENUM at the other location), so update the field definitions in the
AccountingModule class to use 'type' => 'ENUM' plus the corresponding
'constraint' array.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/Database/Migrations/20260427000000_AccountingModule.php (6)

80-113: Add secondary indexes on foreign-key/filter columns in accounting_entries and accounting_items.

accounting_entries.journal_id and accounting_items.entry_id/account_id are the primary join and filter columns for every ledger query (trial balance, per-journal reports, account drill-downs) but have no secondary indexes. These tables will grow proportionally with transaction volume; full-table scans will be the norm without them.

♻️ Proposed addition

         $this->forge->addKey('entry_id', true);
+        $this->forge->addKey('journal_id');
         $this->forge->createTable('accounting_entries');

         $this->forge->addKey('item_id', true);
+        $this->forge->addKey('entry_id');
+        $this->forge->addKey('account_id');
         $this->forge->createTable('accounting_items');

Also applies to: 116-146

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 80
- 113, Add non-unique secondary indexes for the join/filter columns so queries
don't trigger full-table scans: in the migration that defines the
accounting_entries table add a secondary index on journal_id (use
$this->forge->addKey('journal_id', false) before
createTable('accounting_entries')), and in the migration that defines
accounting_items add secondary indexes on entry_id and account_id (use
$this->forge->addKey('entry_id', false) and $this->forge->addKey('account_id',
false) before createTable('accounting_items')). Ensure you add these using the
same Forge API used for the primary key so the indexes are created alongside the
tables.

---

149-152: ⚡ Quick win

Replace remaining $this->db->query() calls with the query builder.

Lines 149, 152, 164–169, 171–180 (up()) and 194–196 (down()) still use raw SQL via $this->db->query() with prefixTable() string concatenation. While the concatenated values are all hardcoded literals (no actual injection risk), this pattern is flagged by the static analysis tool and is inconsistent with the already-fixed grant insert (lines 157–161) and with the coding guideline to use parameterized queries for migrations.

♻️ Example refactor for modules/permissions inserts and down() deletes

-        $this->db->query("INSERT INTO " . $this->db->prefixTable('modules') . " (name_lang_key, desc_lang_key, sort, module_id) VALUES ('module_accounting', 'module_accounting_desc', 90, 'accounting')");
+        $this->db->table('modules')->insert([
+            'name_lang_key' => 'module_accounting',
+            'desc_lang_key' => 'module_accounting_desc',
+            'sort'          => 90,
+            'module_id'     => 'accounting',
+        ]);

-        $this->db->query("INSERT INTO " . $this->db->prefixTable('permissions') . " (permission_id, module_id) VALUES ('accounting', 'accounting')");
+        $this->db->table('permissions')->insert([
+            'permission_id' => 'accounting',
+            'module_id'     => 'accounting',
+        ]);

For the multi-row journal and account seeds, use insertBatch():

-        $this->db->query("INSERT INTO " . $this->db->prefixTable('journals') . " (name, code, type) VALUES 
-            ('Sales Journal', 'SALES', 'Sale'),
-            ...
-        ");
+        $this->db->table('journals')->insertBatch([
+            ['name' => 'Sales Journal',    'code' => 'SALES', 'type' => 'Sale'],
+            ['name' => 'Purchase Journal', 'code' => 'PURCH', 'type' => 'Purchase'],
+            ['name' => 'Cash Journal',     'code' => 'CASH',  'type' => 'Cash'],
+            ['name' => 'General Journal',  'code' => 'GEN',   'type' => 'General'],
+        ]);

-        $this->db->query("DELETE FROM " . $this->db->prefixTable('grants') . " WHERE permission_id = 'accounting'");
-        $this->db->query("DELETE FROM " . $this->db->prefixTable('permissions') . " WHERE module_id = 'accounting'");
-        $this->db->query("DELETE FROM " . $this->db->prefixTable('modules') . " WHERE module_id = 'accounting'");
+        $this->db->table('grants')->where('permission_id', 'accounting')->delete();
+        $this->db->table('permissions')->where('module_id', 'accounting')->delete();
+        $this->db->table('modules')->where('module_id', 'accounting')->delete();

As per coding guidelines, migrations must use parameterized queries / query builder to prevent SQL injection.

Also applies to: 163-180, 194-196

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 149
- 152, Replace the remaining raw SQL calls using $this->db->query(...) and
prefixTable(...) inside the AccountingModule migration's up() and down() with
the query builder: use $this->db->table('modules')->insert([...]) for single-row
inserts (replace the modules insert),
$this->db->table('permissions')->insert([...]) for the permissions insert, use
insertBatch([...]) for the multi-row journal/account seed blocks, and replace
DELETE queries in down() with $this->db->table(...)->where(...)->delete() (use
parameter arrays for values). Locate usages of $this->db->query, prefixTable,
and the up()/down() methods in this migration and convert each raw SQL statement
to the corresponding query builder call with parameterized data structures.

---

80-146: Add indexes on foreign-key/filter columns in accounting_entries and accounting_items.

accounting_entries.journal_id and accounting_items.entry_id/account_id are high-cardinality join and filter columns with no secondary indexes. Ledger queries (e.g., trial balance, per-journal reports) will perform full-table scans as transaction volume grows.

♻️ Proposed addition

         $this->forge->addKey('entry_id', true);
+        $this->forge->addKey('journal_id');     // index for journal-filtered queries
         $this->forge->createTable('accounting_entries');

         $this->forge->addKey('item_id', true);
+        $this->forge->addKey('entry_id');       // index for entry→items joins
+        $this->forge->addKey('account_id');     // index for account-filtered reports
         $this->forge->createTable('accounting_items');

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 80
- 146, Add non-primary indexes on high-cardinality FK/filter columns to avoid
full-table scans: in the migration where createTable('accounting_entries') is
defined, call $this->forge->addKey('journal_id') (non-unique) before
createTable; and in the accounting_items section, call
$this->forge->addKey('entry_id') and $this->forge->addKey('account_id') (both
non-unique) before createTable('accounting_items'); locate these changes next to
the existing addKey('entry_id', true) and addKey('item_id', true) calls so the
new indexes are applied during table creation.

---

65-68: ⚡ Quick win

journals.type ENUM uses a raw SQL string — use the documented constraint array pattern.

The CI4 Forge documentation consistently documents ENUM fields as:

'type' => 'ENUM', 'constraint' => ['value1', 'value2', ...]

The canonical Forge example defines ENUM as 'type' => 'ENUM', 'constraint' => ['publish', 'pending', 'draft'], 'default' => 'pending' — i.e. the values go in constraint, not embedded in the type string. Additionally, starting with v4.5.0, SQLSRV Forge converts ENUM data types to VARCHAR(n), meaning ENUM has special cross-driver handling that the raw string form bypasses. The accounts.type field in the same migration (lines 31-33) uses the correct array pattern; the journals.type field (line 66) does not.

♻️ Proposed fix

-            'type' => [
-                'type' => 'ENUM("Sale", "Purchase", "Cash", "Bank", "General")',
-                'default' => 'General',
-            ],
+            'type' => [
+                'type'       => 'ENUM',
+                'constraint' => ['Sale', 'Purchase', 'Cash', 'Bank', 'General'],
+                'default'    => 'General',
+            ],

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 65
- 68, The journals.type field in the AccountingModule migration uses a raw SQL
ENUM string; change it to the documented Forge array pattern by replacing the
raw type string with 'type' => 'ENUM', add a 'constraint' =>
['Sale','Purchase','Cash','Bank','General'], and keep 'default' => 'General' so
it matches how accounts.type is defined and lets Forge/SQLSRV handle ENUM
conversion correctly.

---

149-152: ⚡ Quick win

Replace remaining $this->db->query() calls with the query builder.

Lines 149, 152, 164–169, 171–180 (up()) and 194–196 (down()) use $this->db->query() with prefixTable() string concatenation. While all concatenated values are hardcoded literals (no live injection vector), this pattern is flagged by the static analyser and is inconsistent with the already-fixed grant insert at lines 157–161 which correctly uses the query builder.

♻️ Example refactor (modules/permissions inserts and down() deletes)

-        $this->db->query("INSERT INTO " . $this->db->prefixTable('modules') . " (name_lang_key, desc_lang_key, sort, module_id) VALUES ('module_accounting', 'module_accounting_desc', 90, 'accounting')");
+        $this->db->table('modules')->insert([
+            'name_lang_key' => 'module_accounting',
+            'desc_lang_key' => 'module_accounting_desc',
+            'sort'          => 90,
+            'module_id'     => 'accounting',
+        ]);

-        $this->db->query("INSERT INTO " . $this->db->prefixTable('permissions') . " (permission_id, module_id) VALUES ('accounting', 'accounting')");
+        $this->db->table('permissions')->insert([
+            'permission_id' => 'accounting',
+            'module_id'     => 'accounting',
+        ]);

For the multi-row seeds, use insertBatch():

-        $this->db->query("INSERT INTO " . $this->db->prefixTable('journals') . " (name, code, type) VALUES 
-            ('Sales Journal', 'SALES', 'Sale'),
-            ('Purchase Journal', 'PURCH', 'Purchase'),
-            ('Cash Journal', 'CASH', 'Cash'),
-            ('General Journal', 'GEN', 'General')
-        ");
+        $this->db->table('journals')->insertBatch([
+            ['name' => 'Sales Journal',    'code' => 'SALES', 'type' => 'Sale'],
+            ['name' => 'Purchase Journal', 'code' => 'PURCH', 'type' => 'Purchase'],
+            ['name' => 'Cash Journal',     'code' => 'CASH',  'type' => 'Cash'],
+            ['name' => 'General Journal',  'code' => 'GEN',   'type' => 'General'],
+        ]);

-        $this->db->query("DELETE FROM " . $this->db->prefixTable('grants') . " WHERE permission_id = 'accounting'");
-        $this->db->query("DELETE FROM " . $this->db->prefixTable('permissions') . " WHERE module_id = 'accounting'");
-        $this->db->query("DELETE FROM " . $this->db->prefixTable('modules') . " WHERE module_id = 'accounting'");
+        $this->db->table('grants')->where('permission_id', 'accounting')->delete();
+        $this->db->table('permissions')->where('module_id', 'accounting')->delete();
+        $this->db->table('modules')->where('module_id', 'accounting')->delete();

As per coding guidelines, migrations must use parameterized queries to prevent SQL injection.

Also applies to: 163-180, 194-196

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 149
- 152, Replace raw $this->db->query(...) calls in up() and down() with the query
builder: use $this->db->table($this->db->prefixTable('modules'))->insert([...])
for the single-row modules insert (replace the query that inserts
'module_accounting'), use
$this->db->table($this->db->prefixTable('permissions'))->insertBatch([...]) for
the multi-row permissions seed (replace the existing $this->db->query inserts),
and in down() use
$this->db->table($this->db->prefixTable('modules'))->where('module_id',
'accounting')->delete() and similarly
$this->db->table($this->db->prefixTable('permissions'))->where(...)->delete()
for cleanup; keep hardcoded values as array elements passed to
insert/insertBatch to ensure parameterized queries.

---

65-68: ⚡ Quick win

ENUM field definition uses a raw SQL string — use the CI4 Forge constraint array instead.

The accounts table in the same migration (lines 30-34) correctly uses the constraint array pattern for ENUM. The journals.type field uses a raw type string with the ENUM values embedded, which bypasses CI4 Forge's internal type handling and may behave unexpectedly on non-MySQL databases or future CI4 versions.

♻️ Proposed fix

-            'type' => [
-                'type' => 'ENUM("Sale", "Purchase", "Cash", "Bank", "General")',
-                'default' => 'General',
-            ],
+            'type' => [
+                'type'       => 'ENUM',
+                'constraint' => ['Sale', 'Purchase', 'Cash', 'Bank', 'General'],
+                'default'    => 'General',
+            ],

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 65
- 68, The ENUM definition for the journals.type field in the AccountingModule
migration uses a raw SQL string; update it to use the Forge-style constraint
array instead: change the field definition for 'type' to use 'type' => 'ENUM',
'constraint' => ['Sale','Purchase','Cash','Bank','General'], and keep the
'default' => 'General' so CI4 Forge handles platform-specific SQL correctly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/Database/Migrations/20260427000000_AccountingModule.php (1)

80-150: ⚡ Quick win

Foreign key constraints missing on the two core financial tables.

accounting_entries and accounting_items declare indexes on their FK columns but don't register actual database-level FK constraints. This means:

• accounting_items can reference non-existent entries or accounts.
• Deleting an accounting_entries row leaves orphaned accounting_items rows silently.

The rest of the OSPOS schema enforces referential integrity via FK constraints (e.g., ospos_grants_ibfk_2 on employees.person_id). The accounting tables should be consistent with that pattern.

CI4 forge's addForeignKey() must be called before createTable():

♻️ Proposed addition for `accounting_entries` and `accounting_items`

 $this->forge->addKey('entry_id', true);
 $this->forge->addKey('journal_id');
+$this->forge->addForeignKey('journal_id', 'journals', 'journal_id', 'RESTRICT', 'RESTRICT');
 $this->forge->createTable('accounting_entries');

 // ...

 $this->forge->addKey('item_id', true);
 $this->forge->addKey('entry_id');
 $this->forge->addKey('account_id');
+$this->forge->addForeignKey('entry_id', 'accounting_entries', 'entry_id', 'CASCADE', 'CASCADE');
+$this->forge->addForeignKey('account_id', 'accounts', 'account_id', 'RESTRICT', 'RESTRICT');
 $this->forge->createTable('accounting_items');

The CASCADE on entry_id means deleting a header automatically removes its lines; RESTRICT on account_id prevents deleting an account while it has posted items.

Note: accounts.parent_id is self-referential and would require a separate ALTER TABLE after createTable('accounts') — adding it as a follow-up is acceptable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Database/Migrations/20260427000000_AccountingModule.php` around lines 80
- 150, The migration omits DB-level foreign keys for accounting_entries and
accounting_items: add calls to $this->forge->addForeignKey before each
createTable to enforce referential integrity (e.g., on accounting_entries
addForeignKey('journal_id','journals','journal_id','RESTRICT','CASCADE') and
addForeignKey('employee_id','employees','employee_id','RESTRICT','SET NULL' or
appropriate); for accounting_items
addForeignKey('entry_id','accounting_entries','entry_id','CASCADE','CASCADE')
and addForeignKey('account_id','accounts','account_id','RESTRICT','RESTRICT') so
items cannot reference missing accounts and entries are cascaded on delete;
ensure addForeignKey calls are placed prior to
createTable('accounting_entries')/createTable('accounting_items'); note
self-referential accounts.parent_id should be added later via ALTER TABLE if
needed.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes