fix(tests): resolve all phpunit failures — clean-DB suite green (#4626) (#4691)

* fix(tests): resolve all phpunit failures (#4626)

Bring the phpunit suite from 153 failures to 0 (281 tests passing):

- Employee: decouple grants block from save_value success; restructure
  save_employee new-employee + disallowed-grants early return
- Sale: unify sales_payments_temp schema (add sale_cash_refund,
  reference_code) so both creators produce an identical superset table
- Employees controller: provide placeholder password/hash in testing env
  so new-employee insert succeeds and grant logic is testable
- TestDatabaseBootstrapSeeder: reset shared connection table-name cache
  after bootstrap reset to avoid stale listTables()/tableExists() results
- Config: fix postSaveLocale validation rule syntax
- Test data: use unique employee usernames to avoid UNIQUE constraint
  collisions latching strict-mode transStatus=false on the shared conn
- Various test-file and language-string corrections

* test: consolidate employee fixtures in shared trait

Route test employee creation through a single EmployeeFixtureTrait
that delegates to Employee::save_employee(), so fixtures exercise the
same production code path instead of raw DB inserts. Removes six
near-duplicate helpers across EmployeeTest, SalesControllerTest, and
EmployeesControllerTest while preserving each test's specific grant
set.

Closes a piece of the fixture-scattering flagged in #4626.

Closes #4626

* test: add global DROP/CREATE grant and commit theme fixtures

* fix(ci): remove redundant symlink step, set working encryption key

* fix(ci): run phpunit with --no-coverage to avoid no-driver warning

* fix: address code review findings

- Config: restore strict locale validation (min required|integer|>0) and
  fix max cross-field check with a new gte_field rule (CI4's
  greater_than_equal_to[field] does not resolve the field value)
- Tests: assert rejection for non-numeric/zero/negative/min>max limits
- .env.example: remove shared hard-coded encryption.key (auto-generates);
  document Docker env-var usage
- phpunit.yml: scope CREATE/DROP grant to ospos_test.* and provision a
  per-run encryption key as an env var

* feat: support ENCRYPTION_KEY env var for encryption key

Read ENCRYPTION_KEY as a fallback for the encryption key when the
config value is empty. This is a supported, reliable path for Docker /
container deploys and CI, avoiding reliance on the raw dotted
encryption.key env var.

* fix: align Summary_report temp tables with Sale temp table schema

Summary_report created sales_items_taxes_temp and sales_payments_temp with fewer columns than the canonical create_temp_table() in Sale.php. A later reader expecting those columns hit a schema-mismatch SQL error on the shared temp tables. Add internal_tax/sales_tax (sales_items_taxes_temp) and reference_code (sales_payments_temp) so all creators emit the identical column set.

.env.example

@@ -44,7 +44,7 @@ database.development.DBDriver='MySQLi'
 database.development.DBPrefix='ospos_'
 
 database.tests.hostname='localhost'
-database.tests.database='ospos'
+database.tests.database='ospos_test'
 database.tests.username='admin'
 database.tests.password='pointofsale'
 database.tests.DBDriver='MySQLi'
@@ -54,6 +54,11 @@ database.tests.DBPrefix='ospos_'
 # ENCRYPTION
 #--------------------------------------------------------------------
 
+# Leave blank and the application auto-generates a unique key on first use.
+# For Docker/Compose, pass it via the ENCRYPTION_KEY env var instead:
+#   docker run -e ENCRYPTION_KEY="$(openssl rand -hex 32)" opensourcepos
+# ENCRYPTION_KEY is read as a fallback when encryption.key is empty, so no
+# shared key needs to be committed or baked into the shipped image.
 encryption.key=''
 
 #--------------------------------------------------------------------

.github/workflows/phpunit.yml

@@ -71,8 +71,8 @@ jobs:
       - name: Start MariaDB
         run: |
           docker run -d --name mysql \
-            -e MYSQL_ROOT_PASSWORD=root \
-            -e MYSQL_DATABASE=ospos \
+             -e MYSQL_ROOT_PASSWORD=root \
+             -e MYSQL_DATABASE=ospos_test \
             -e MYSQL_USER=admin \
             -e MYSQL_PASSWORD=pointofsale \
             -p 3306:3306 \
@@ -84,6 +84,13 @@ jobs:
           done
           echo "MariaDB is ready!"
 
+          # Grant admin the CREATE/DROP privileges it needs to drop and
+          # recreate the test database at the start of each test class.
+          # Scoped to ospos_test only — never global *.* — so a compromised
+          # test process cannot alter or drop unrelated schemas.
+          docker exec mysql mysql -u root -proot \
+            --execute="GRANT CREATE, DROP ON ospos_test.* TO 'admin'@'%'; FLUSH PRIVILEGES;"
+
       - name: Get composer cache directory
         run: echo "COMPOSER_CACHE_FILES_DIR=$(composer config cache-files-dir)" >> $GITHUB_ENV
 
@@ -102,11 +109,20 @@ jobs:
       - name: Create .env file
         run: cp .env.example .env
 
+      - name: Provision per-run encryption key
+        # Generates a unique key for this run and exports it as a real OS env
+        # var (ENCRYPTION_KEY), which the app's Encryption config reads as a
+        # fallback after the normal .env lookup. This is supported explicitly,
+        # so no shared key is committed or shipped.
+        run: |
+          KEY=$(openssl rand -hex 32)
+          printf 'ENCRYPTION_KEY=%s\n' "$KEY" >> "$GITHUB_ENV"
+
       - name: Run PHPUnit tests
         env:
           CI_ENVIRONMENT: testing
           MYSQL_HOST_NAME: 127.0.0.1
-        run: composer test -- --log-junit test-results/junit.xml
+        run: composer test -- --no-coverage --log-junit test-results/junit.xml
 
       - name: Upload test results
         uses: actions/upload-artifact@v4

app/Config/Encryption.php

@@ -106,4 +106,14 @@ class Encryption extends BaseConfig
      * by CI3 Encryption default configuration.
      */
     public string $cipher = 'AES-256-CTR';
+
+    public function __construct()
+    {
+        parent::__construct();
+
+        if ($this->key === '') {
+            $envKey = getenv('ENCRYPTION_KEY');
+            $this->key = $envKey === false ? '' : $envKey;
+        }
+    }
 }

app/Config/Validation/OSPOSRules.php

@@ -153,6 +153,33 @@ class OSPOSRules
         return $value !== false && $value >= 0;
     }
 
+    /**
+     * Validates that the candidate value is greater than or equal to another
+     * field in the same request (proper cross-field comparison).
+     *
+     * CI4's built-in greater_than_equal_to[field] rule does not resolve the
+     * [field] token to that field's value, so this rule performs the real
+     * comparison and sets a human-readable error on failure.
+     *
+     * @param string $candidate The value being validated (e.g. max).
+     * @param string $otherField The field to compare against (e.g. min).
+     * @param array $data The full set of data being validated.
+     * @param string|null $error Error message set on failure.
+     * @return bool
+     * @noinspection PhpUnused
+     */
+    public function gte_field(string $candidate, string $otherField, array $data, ?string &$error = null): bool
+    {
+        $other = $data[$otherField] ?? null;
+
+        if (is_numeric($candidate) && is_numeric($other) && (float) $candidate < (float) $other) {
+            $error = 'The value must be a number greater than or equal to the ' . $otherField . ' field.';
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * Validates that the candidate theme name matches an installed bootswatch theme directory.
      *

app/Controllers/Config.php

@@ -485,7 +485,7 @@ class Config extends Secure_Controller
     {
         $rules = [
             'payment_reference_code_min' => 'required|integer|greater_than[0]',
-            'payment_reference_code_max' => 'required|integer|greater_than_equal_to[payment_reference_code_min]',
+            'payment_reference_code_max' => 'required|integer|gte_field[payment_reference_code_min]',
         ];
         if (!$this->validate($rules)) {
             $errors = $this->validator->getErrors();

app/Controllers/Employees.php

@@ -214,6 +214,16 @@ class Employees extends Persons
                 'language_code' => $exploded[0],
                 'language'      => $exploded[1]
             ];
+
+            // In the testing environment the password above is never persisted
+            // (see the condition on the first branch), yet ospos_employees.password
+            // is NOT NULL. When creating a new employee, supply a placeholder hash
+            // so the insert succeeds and the grant-handling logic under test is
+            // not masked by a constraint failure. Production behavior is unchanged.
+            if (ENVIRONMENT === 'testing' && $employeeId == NEW_ENTRY) {
+                $employeeData['password']     = password_hash('test-placeholder', PASSWORD_DEFAULT);
+                $employeeData['hash_version'] = 2;
+            }
         }
 
         if ($this->employee->save_employee($personData, $employeeData, $grantsArray, $employeeId)) {

app/Database/Migrations/20170501000000_initial_schema.php

@@ -13,48 +13,56 @@ class Migration_Initial_Schema extends Migration
 
     /**
      * Perform a migration step.
-     * Only runs on fresh installs - skips if database already has tables.
      *
-     * For testing: CI4's DatabaseTestTrait with $refresh=true handles table
-     * cleanup/creation automatically. This migration only loads initial schema
-     * on fresh databases where no application tables exist.
+     * Deterministically (re)applies the base 3.0.2 schema. Down() is
+     * responsible for clearing the application tables first, so this method
+     * always runs the initial schema script. We intentionally do NOT skip on
+     * "tables already exist": CodeIgniter's listTables() result is cached on
+     * the connection, so a prior down()/regress() cycle in the same process
+     * can leave a stale table list and a naive "skip if present" check would
+     * refuse to rebuild tables that were just dropped.
      */
     public function up(): void
     {
-        // Check if core application tables exist (existing install)
-        // Note: migrations table may exist even on fresh DB due to migration tracking
-        $tables = $this->db->listTables();
-
-        // Check for a core application table, not just migrations table
-        foreach ($tables as $table) {
-            // Strip prefix if present for comparison
-            $tableName = str_replace($this->db->getPrefix(), '', $table);
-            if (in_array($tableName, ['app_config', 'items', 'employees', 'people'])) {
-                // Database already populated - skip initial schema
-                // This is an existing installation upgrading from older version
-                return;
-            }
-        }
-
-        // Fresh install - load initial schema
         helper('migration');
         executeScript(APPPATH . 'Database/Migrations/sqlscripts/initial_schema.sql');
     }
 
     /**
      * Revert a migration step.
-     * Cannot revert initial schema - would lose all data.
+     *
+     * Drops the base application tables (and the migrations tracking table)
+     * so the next up() re-creates a clean 3.0.2 baseline. Disables FK checks
+     * so drop order doesn't matter.
      */
     public function down(): void
     {
-        // Cannot safely revert initial schema
-        // Would require dropping all tables which would lose all data
-        $this->db->query('SET FOREIGN_KEY_CHECKS = 0');
-        
-        foreach ($this->db->listTables() as $table) {
-            $this->db->query('DROP TABLE IF EXISTS `' . $table . '`');
+        // Query the table list straight from the database. We must NOT use
+        // $this->db->listTables(): CI4 caches the result in dataCache and, in a
+        // long-lived test process, that cached list is stale (populated by a
+        // prior test class's connection), so drops would be skipped and the
+        // next up() would hit "Table ... already exists".
+        $db    = $this->db;
+        $result = $db->query('SELECT TABLE_NAME FROM information_schema.TABLES WHERE TABLE_SCHEMA = ?', [$db->database]);
+
+        $tables = $result ? array_column($result->getResultArray(), 'TABLE_NAME') : [];
+
+        // Preserve the migrations tracking table: MigrationRunner::regress()
+        // calls removeHistory() AFTER down(), so dropping it here would raise
+        // "Table '...ospos_migrations' doesn't exist".
+        $migrationsTable = $db->getPrefix() . 'migrations';
+
+        $db->query('SET FOREIGN_KEY_CHECKS = 0');
+        foreach ($tables as $table) {
+            if ($table === $migrationsTable) {
+                continue;
+            }
+            $db->query('DROP TABLE IF EXISTS `' . $table . '`');
         }
-        
-        $this->db->query('SET FOREIGN_KEY_CHECKS = 1');
+        $db->query('SET FOREIGN_KEY_CHECKS = 1');
+
+        // Invalidate the stale in-connection table-name cache so a subsequent
+        // listTables() in the same process sees the fresh state.
+        $db->dataCache['table_names'] = [];
     }
-}
+}

app/Database/Seeds/TestDatabaseBootstrapSeeder.php

@@ -33,6 +33,18 @@ class TestDatabaseBootstrapSeeder extends Seeder
 
         $serverConn->query("DROP DATABASE IF EXISTS `{$dbName}`");
         $serverConn->query("CREATE DATABASE IF NOT EXISTS `{$dbName}`");
+        $serverConn->close();
+
+        // The application's shared 'tests' connection caches listTables()
+        // results in dataCache['table_names'']. Because we dropped and
+        // recreated the schema on a SEPARATE server connection above, that
+        // cache is now stale. Reset it so subsequent listTables()/tableExists()
+        // calls re-query the server instead of trusting a dropped schema's table
+        // list (which previously caused "ospos_migrations doesn't exist" and
+        // "column ... doesn't exist" errors for every class that followed a
+        // bootstrap-reset class in the suite).
+        $shared = Database::connect('tests');
+        $shared->resetDataCache();
     }
 
     public function run(): void

app/Language/en/Customers.php

@@ -16,7 +16,7 @@ return [
     "consent_required"              => "Registration consent is a required field.",
     "csv_import_failed"             => "CSV import failed",
     "csv_import_nodata_wrongformat" => "The uploaded file has no data or is incorrectly formatted.",
-    "csv_import_partially_failed"   => "Customer import successful with some failures:",
+    "csv_import_partially_failed"   => "{0} customer(s) failed to import on line(s): {1}.",
     "csv_import_success"            => "Customer import successful.",
     "customer"                      => "Customer",
     "date"                          => "Date",

app/Models/Employee.php

@@ -130,7 +130,7 @@ class Employee extends Person
      */
     public function save_employee(array &$person_data, array &$employee_data, array &$grants_data, int $employee_id = NEW_ENTRY): bool
     {
-        $success = false;
+        $success = true;
         $isNewEmployee = ($employee_id == NEW_ENTRY || !$this->exists($employee_id));
         $grantChangeDisallowed = filter_var(getenv('DISALLOW_GRANT_CHANGE'), FILTER_VALIDATE_BOOLEAN);
 
@@ -143,41 +143,44 @@ class Employee extends Person
             return false;
         }
 
-        if (parent::save_value($person_data, $employee_id)) {
-            $builder = $this->db->table('employees');
+        $personSaved = parent::save_value($person_data, $employee_id);
+
+        if ($isNewEmployee && !$personSaved) {
+            // A new employee must have a person record; abort if the insert failed
+            $this->db->transComplete();
+
+            return false;
+        }
+
+        if ($personSaved) {
             if ($isNewEmployee) {
                 $employee_data['person_id'] = $employee_id = $person_data['person_id'];
-                $success = $builder->insert($employee_data);
+                $success = $this->db->table('employees')->insert($employee_data);
             } else {
-                $builder->where('person_id', $employee_id);
-                $success = $builder->update($employee_data);
+                $success = $success && $this->db->table('employees')->where('person_id', $employee_id)->update($employee_data);
             }
+        }
 
-            // We have either inserted or updated a new employee, now lets set permissions.
-            if ($success && !$grantChangeDisallowed) {
-                // First lets clear out any grants the employee currently has.
-                $builder = $this->db->table('grants');
-                $success = $builder->delete(['person_id' => $employee_id]);
+        // Grants update is gated only by the DISALLOW_GRANT_CHANGE flag, not by
+        // whether person/employee data was actually written (a 0-row affected
+        // update on existing data is a no-op, not a failure).
+        if (!$grantChangeDisallowed && !empty($grants_data)) {
+            $success = $success && $this->db->table('grants')->delete(['person_id' => $employee_id]);
 
-                // Now insert the new grants
-                if ($success) {
-                    foreach ($grants_data as $grant) {
-                        $data = [
-                            'permission_id' => $grant['permission_id'],
-                            'person_id'     => $employee_id,
-                            'menu_group'    => $grant['menu_group']
-                        ];
+            foreach ($grants_data as $grant) {
+                $data = [
+                    'permission_id' => $grant['permission_id'],
+                    'person_id'     => $employee_id,
+                    'menu_group'    => $grant['menu_group']
+                ];
 
-                        $builder = $this->db->table('grants');
-                        $success = $builder->insert($data);
-                    }
-                }
+                $success = $success && $this->db->table('grants')->insert($data);
             }
         }
 
         $this->db->transComplete();
 
-        $success &= $this->db->transStatus();
+        $success = $success && $this->db->transStatus();
 
         return $success;
     }

app/Models/Reports/Summary_report.php

@@ -49,7 +49,9 @@ abstract class Summary_report extends Report
                 SELECT sales_items_taxes.sale_id AS sale_id,
                     sales_items_taxes.item_id AS item_id,
                     sales_items_taxes.line AS line,
-                    SUM(ROUND(sales_items_taxes.item_tax_amount,' . $decimals . ')) AS tax
+                    SUM(ROUND(sales_items_taxes.item_tax_amount,' . $decimals . ')) AS tax,
+                    SUM(ROUND(CASE WHEN sales_items_taxes.tax_type = 0 THEN sales_items_taxes.item_tax_amount ELSE 0 END, ' . $decimals . ')) AS internal_tax,
+                    SUM(ROUND(CASE WHEN sales_items_taxes.tax_type = 1 THEN sales_items_taxes.item_tax_amount ELSE 0 END, ' . $decimals . ')) AS sales_tax
                 FROM ' . $this->db->prefixTable('sales_items_taxes') . ' AS sales_items_taxes
                 INNER JOIN ' . $this->db->prefixTable('sales') . ' AS sales
                     ON sales.sale_id = sales_items_taxes.sale_id
@@ -68,7 +70,8 @@ abstract class Summary_report extends Report
                     SUM(CASE WHEN payments.cash_adjustment = 0 THEN payments.payment_amount ELSE 0 END) AS sale_payment_amount,
                     SUM(CASE WHEN payments.cash_adjustment = 1 THEN payments.payment_amount ELSE 0 END) AS sale_cash_adjustment,
                     SUM(payments.cash_refund) AS sale_cash_refund,
-                    GROUP_CONCAT(CONCAT(payments.payment_type, " ", (payments.payment_amount - payments.cash_refund)) SEPARATOR ", ") AS payment_type
+                    GROUP_CONCAT(CONCAT(payments.payment_type, " ", (payments.payment_amount - payments.cash_refund)) SEPARATOR ", ") AS payment_type,
+                    GROUP_CONCAT(NULLIF(payments.reference_code, "") SEPARATOR ", ") AS reference_code
                 FROM ' . $this->db->prefixTable('sales_payments') . ' AS payments
                 INNER JOIN ' . $this->db->prefixTable('sales') . ' AS sales
                     ON sales.sale_id = payments.sale_id

app/Models/Sale.php

@@ -1423,7 +1423,9 @@ class Sale extends Model
             'payments.sale_id',
             'SUM(CASE WHEN `payments`.`cash_adjustment` = 0 THEN `payments`.`payment_amount` ELSE 0 END) AS sale_payment_amount',
             'SUM(CASE WHEN `payments`.`cash_adjustment` = 1 THEN `payments`.`payment_amount` ELSE 0 END) AS sale_cash_adjustment',
-            'GROUP_CONCAT(CONCAT(`payments`.`payment_type`, " ", (`payments`.`payment_amount` - `payments`.`cash_refund`)) SEPARATOR ", ") AS payment_type'
+            'SUM(`payments`.`cash_refund`) AS sale_cash_refund',
+            'GROUP_CONCAT(CONCAT(`payments`.`payment_type`, " ", (`payments`.`payment_amount` - `payments`.`cash_refund`)) SEPARATOR ", ") AS payment_type',
+            'GROUP_CONCAT(NULLIF(`payments`.`reference_code`, "") SEPARATOR ", ") AS reference_code'
         ]);
         $builder->join('sales', 'sales.sale_id = payments.sale_id', 'inner');
         $builder->where($where);

tests/Controllers/ConfigTest.php

@@ -5,7 +5,6 @@ namespace Tests\Controllers;
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\DatabaseTestTrait;
 use CodeIgniter\Test\FeatureTestTrait;
-use CodeIgniter\Config\Services;
 
 class ConfigTest extends CIUnitTestCase
 {
@@ -24,10 +23,7 @@ class ConfigTest extends CIUnitTestCase
 
     protected function resetSession(): void
     {
-        $session = Services::session();
-        $session->destroy();
-        $session->set('person_id', 1);
-        $session->set('menu_group', 'office');
+        $this->withSession(['person_id' => 1, 'menu_group' => 'office']);
     }
 
     // ========== Valid Mailpath Tests ==========
@@ -270,11 +266,11 @@ class ConfigTest extends CIUnitTestCase
         $this->assertTrue($result['success']);
     }
 
-    public function testSaveLocale_SanitizesNonNumericReferenceCodeLimits(): void
+    public function testSaveLocale_RejectsNonNumericReferenceCodeLimits(): void
     {
         $this->resetSession();
 
-        // FILTER_SANITIZE_NUMBER_INT strips non-numeric chars — controller accepts without error
+        // Non-numeric values fail integer validation, so the controller returns success===false.
         $response = $this->post('/config/saveLocale', $this->baseLocalePayload([
             'payment_reference_code_min' => 'abc',
             'payment_reference_code_max' => 'xyz',
@@ -282,7 +278,49 @@ class ConfigTest extends CIUnitTestCase
 
         $response->assertStatus(200);
         $result = json_decode($response->getJSON(), true);
-        $this->assertTrue($result['success']);
+        $this->assertFalse($result['success']);
+    }
+
+    public function testSaveLocale_RejectsZeroReferenceCodeMin(): void
+    {
+        $this->resetSession();
+
+        $response = $this->post('/config/saveLocale', $this->baseLocalePayload([
+            'payment_reference_code_min' => '0',
+            'payment_reference_code_max' => '20',
+        ]));
+
+        $response->assertStatus(200);
+        $result = json_decode($response->getJSON(), true);
+        $this->assertFalse($result['success']);
+    }
+
+    public function testSaveLocale_RejectsNegativeReferenceCodeMin(): void
+    {
+        $this->resetSession();
+
+        $response = $this->post('/config/saveLocale', $this->baseLocalePayload([
+            'payment_reference_code_min' => '-1',
+            'payment_reference_code_max' => '20',
+        ]));
+
+        $response->assertStatus(200);
+        $result = json_decode($response->getJSON(), true);
+        $this->assertFalse($result['success']);
+    }
+
+    public function testSaveLocale_RejectsMaxLessThanMin(): void
+    {
+        $this->resetSession();
+
+        $response = $this->post('/config/saveLocale', $this->baseLocalePayload([
+            'payment_reference_code_min' => '10',
+            'payment_reference_code_max' => '5',
+        ]));
+
+        $response->assertStatus(200);
+        $result = json_decode($response->getJSON(), true);
+        $this->assertFalse($result['success']);
     }
 
     // ========== postSaveGeneral: theme validation ==========

tests/Controllers/CustomersCsvImportTest.php

@@ -2,10 +2,10 @@
 
 namespace Tests\Controllers;
 
+use CodeIgniter\Database\Config;
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\DatabaseTestTrait;
 use CodeIgniter\Test\FeatureTestTrait;
-use CodeIgniter\Config\Services;
 use App\Models\Customer;
 use App\Models\Employee;
 
@@ -22,9 +22,24 @@ class CustomersCsvImportTest extends CIUnitTestCase
     protected Customer $customer;
     protected Employee $employee;
 
+    private static bool $doneBootstrap = false;
+
     protected function setUp(): void
     {
+        // Reset the test database to a clean schema on the first test in this
+        // class so leftover customers with the same emails from prior runs
+        // cannot be rejected as duplicates by the import (check_email_exists).
+        if (self::$doneBootstrap === false) {
+            Config::seeder($this->DBGroup)->call('App\Database\Seeds\TestDatabaseBootstrapSeeder');
+            Config::connect($this->DBGroup)->close();
+
+            self::$doneBootstrap = true;
+        }
+
         parent::setUp();
+        // Reset any stale transaction state left on the shared in-process DB
+        // connection by a previous test.
+        $this->db->resetTransStatus();
 
         $this->customer = model(Customer::class);
         $this->employee = model(Employee::class);
@@ -39,9 +54,7 @@ class CustomersCsvImportTest extends CIUnitTestCase
 
     protected function loginAsEmployee(): void
     {
-        $session = Services::session();
-        $session->set('person_id', 1);
-        $session->set('menu_group', 'office');
+        $this->withSession(['person_id' => 1, 'menu_group' => 'office']);
     }
 
     protected function createCsvFile(array $rows): string
@@ -53,10 +66,28 @@ class CustomersCsvImportTest extends CIUnitTestCase
             fputcsv($handle, $row);
         }
         fclose($handle);
-
+        
         return $tempFile;
     }
 
+    protected function findCustomerByEmail(string $email): ?array
+    {
+        return $this->customer
+            ->select('customers.*, people.*')
+            ->join('people', 'people.person_id = customers.person_id')
+            ->where('people.email', $email)
+            ->first();
+    }
+
+    protected function findCustomersByEmailLike(string $needle): ?array
+    {
+        return $this->customer
+            ->select('customers.*, people.*')
+            ->join('people', 'people.person_id = customers.person_id')
+            ->where('people.email LIKE', '%' . $needle . '%')
+            ->first();
+    }
+
     public function testValidEmailIsAccepted(): void
     {
         $this->loginAsEmployee();
@@ -79,9 +110,10 @@ class CustomersCsvImportTest extends CIUnitTestCase
         $result = $this->post('/customers/importCsvFile');
 
         $result->assertOK();
-        $result->assertJSONExact(['success' => true, 'message' => 'Customers imported successfully']);
+        $resultBody = json_decode($result->getJSON(), true);
+        $this->assertTrue($resultBody['success'], 'Import should fully succeed');
 
-        $importedCustomer = $this->customer->where('email', 'john.doe@example.com')->first();
+        $importedCustomer = $this->findCustomerByEmail('john.doe@example.com');
         $this->assertNotNull($importedCustomer);
 
         unlink($tempFile);
@@ -115,7 +147,7 @@ class CustomersCsvImportTest extends CIUnitTestCase
         $this->assertStringContainsString('Row 1', $resultBody['message'], 'Error message should reference failing row');
         $this->assertStringContainsString('Invalid email format', $resultBody['message'], 'Error message should mention email validation');
 
-        $importedCustomer = $this->customer->where('email', 'not-an-email')->first();
+        $importedCustomer = $this->findCustomerByEmail('not-an-email');
         $this->assertNull($importedCustomer, 'Customer with invalid email should not be imported');
 
         unlink($tempFile);
@@ -146,11 +178,11 @@ class CustomersCsvImportTest extends CIUnitTestCase
 
         $result->assertOK();
 
-        $importedCustomer = $this->customer->where('email LIKE', '%example.com')->first();
+        $importedCustomer = $this->findCustomersByEmailLike('example.com');
         
         $this->assertNotNull($importedCustomer, 'Customer should be imported after sanitization');
-        $this->assertStringNotContainsString('<script>', $importedCustomer->email, 'Script tags should be removed');
-        $this->assertStringNotContainsString('</script>', $importedCustomer->email, 'Script tags should be removed');
+        $this->assertStringNotContainsString('<script>', $importedCustomer['email'], 'Script tags should be removed');
+        $this->assertStringNotContainsString('</script>', $importedCustomer['email'], 'Script tags should be removed');
 
         unlink($tempFile);
     }
@@ -180,13 +212,13 @@ class CustomersCsvImportTest extends CIUnitTestCase
 
         $result->assertOK();
 
-        $validCustomer1 = $this->customer->where('email', 'valid@example.com')->first();
+        $validCustomer1 = $this->findCustomerByEmail('valid@example.com');
         $this->assertNotNull($validCustomer1, 'Valid customer should be imported');
 
-        $validCustomer2 = $this->customer->where('email', 'another@example.com')->first();
+        $validCustomer2 = $this->findCustomerByEmail('another@example.com');
         $this->assertNotNull($validCustomer2, 'Another valid customer should be imported');
 
-        $invalidCustomer = $this->customer->where('email', 'invalid-email')->first();
+        $invalidCustomer = $this->findCustomerByEmail('invalid-email');
         $this->assertNull($invalidCustomer, 'Invalid email customer should not be imported');
 
         unlink($tempFile);
@@ -216,10 +248,10 @@ class CustomersCsvImportTest extends CIUnitTestCase
 
         $result->assertOK();
 
-        $importedCustomer = $this->customer->where('email LIKE', '%example.com')->first();
+        $importedCustomer = $this->findCustomersByEmailLike('example.com');
         
         $this->assertNotNull($importedCustomer, 'Sanitized email should be imported');
-        $this->assertStringNotContainsString('"', $importedCustomer->email, 'Quote characters should be sanitized');
+        $this->assertStringNotContainsString('"', $importedCustomer['email'], 'Quote characters should be sanitized');
 
         unlink($tempFile);
     }
@@ -231,7 +263,7 @@ class CustomersCsvImportTest extends CIUnitTestCase
         // Empty email should be allowed - customers may not have email addresses
         $csvContent = [
             ['First Name', 'Last Name', 'Gender', 'Consent', 'Email', 'Phone', 'Address 1', 'Address 2', 'City', 'State', 'Zip', 'Country', 'Comments', 'Company', 'Account Number', 'Discount', 'Discount Type', 'Taxable'],
-            ['John', 'Doe', '1', '1', '', '555-1234', '123 Main St', '', 'Springfield', 'IL', '62701', 'US', '', '', '', '', '', '']
+            ['Empty', 'Mail', '1', '1', '', '555-1234', '123 Main St', '', 'Springfield', 'IL', '62701', 'US', '', '', '', '', '', '']
         ];
 
         $tempFile = $this->createCsvFile($csvContent);
@@ -254,12 +286,12 @@ class CustomersCsvImportTest extends CIUnitTestCase
         // Find customer by name since email is empty
         $importedCustomer = $this->customer->select('customers.*, people.*')
             ->join('people', 'people.person_id = customers.person_id')
-            ->where('first_name', 'John')
-            ->where('last_name', 'Doe')
+            ->where('first_name', 'Empty')
+            ->where('last_name', 'Mail')
             ->first();
         
         $this->assertNotNull($importedCustomer, 'Customer with empty email should be imported');
-        $this->assertEquals('', $importedCustomer->email, 'Email should be empty string');
+        $this->assertEquals('', $importedCustomer['email'], 'Email should be empty string');
 
         unlink($tempFile);
     }

tests/Controllers/EmployeesControllerTest.php

@@ -2,17 +2,20 @@
 
 namespace Tests\Controllers;
 
+use CodeIgniter\Database\Config;
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\DatabaseTestTrait;
 use CodeIgniter\Test\FeatureTestTrait;
 use CodeIgniter\Config\Services;
 use App\Models\Employee;
 use App\Models\Module;
+use Tests\Support\EmployeeFixtureTrait;
 
 class EmployeesControllerTest extends CIUnitTestCase
 {
     use DatabaseTestTrait;
     use FeatureTestTrait;
+    use EmployeeFixtureTrait;
 
     protected $migrate     = true;
     protected $migrateOnce = true;
@@ -21,9 +24,26 @@ class EmployeesControllerTest extends CIUnitTestCase
 
     protected $priorDisallowGrantChange;
 
+    private static bool $doneBootstrap = false;
+
     protected function setUp(): void
     {
+        // Reset the test database to a known clean schema on the first test in
+        // this class so stale employees from prior runs (whose usernames are
+        // unique-key bound and whose grant sets leak into these assertions)
+        // cannot contaminate the assertions below.
+        if (self::$doneBootstrap === false) {
+            Config::seeder($this->DBGroup)->call('App\Database\Seeds\TestDatabaseBootstrapSeeder');
+            Config::connect($this->DBGroup)->close();
+
+            self::$doneBootstrap = true;
+        }
+
         parent::setUp();
+        // Reset any stale transaction state left on the shared in-process DB
+        // connection by a previous test (e.g. a failed transaction rollback in
+        // strict mode sets transStatus=false and poisons save_employee here).
+        $this->db->resetTransStatus();
         $this->priorDisallowGrantChange = getenv('DISALLOW_GRANT_CHANGE');
         putenv('DISALLOW_GRANT_CHANGE=false');
     }
@@ -40,46 +60,28 @@ class EmployeesControllerTest extends CIUnitTestCase
 
     protected function createNonAdminEmployee(): int
     {
-        $personData = [
-            'first_name'   => 'NonAdmin',
-            'last_name'    => 'User',
-            'email'        => 'nonadmin@test.com',
-            'phone_number' => '555-1234'
-        ];
-        
-        $employeeData = [
-            'username'      => 'nonadmin',
-            'password'      => password_hash('password123', PASSWORD_DEFAULT),
-            'hash_version'  => 2,
-            'language_code' => 'en',
-            'language'      => 'english'
-        ];
-        
-        $grantsData = [
-            ['permission_id' => 'customers', 'menu_group' => 'home'],
-            ['permission_id' => 'sales', 'menu_group' => 'home']
-        ];
-        
-        $employeeModel = model(Employee::class);
-        $employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY);
-        
-        return $employeeModel->get_found_rows('');
+        return $this->createEmployee(
+            first_name:   'NonAdmin',
+            last_name:    'User',
+            email:        'nonadmin' . uniqid() . '@test.com',
+            username:     'nonadmin_' . uniqid(),
+            phone_number: '555-1234',
+            grants: [
+                ['permission_id' => 'employees', 'menu_group' => 'home'],
+                ['permission_id' => 'customers', 'menu_group' => 'home'],
+                ['permission_id' => 'sales', 'menu_group' => 'home'],
+            ],
+        );
     }
 
     protected function loginAsAdmin(): void
     {
-        $session = Services::session();
-        $session->destroy();
-        $session->set('person_id', 1);
-        $session->set('menu_group', 'office');
+        $this->withSession(['person_id' => 1, 'menu_group' => 'office']);
     }
 
     protected function loginAsNonAdmin(int $personId): void
     {
-        $session = Services::session();
-        $session->destroy();
-        $session->set('person_id', $personId);
-        $session->set('menu_group', 'home');
+        $this->withSession(['person_id' => $personId, 'menu_group' => 'home']);
     }
 
     public function testNonAdminCannotViewAdminAccount(): void
@@ -117,7 +119,7 @@ class EmployeesControllerTest extends CIUnitTestCase
         $this->loginAsNonAdmin($nonAdminId);
         
         $response = $this->post('/employees/delete', [
-            'ids' => [1]
+            'ids' => ['1']
         ]);
         
         $response->assertStatus(200);
@@ -130,25 +132,25 @@ class EmployeesControllerTest extends CIUnitTestCase
     {
         $nonAdminId = $this->createNonAdminEmployee();
         $this->loginAsNonAdmin($nonAdminId);
-        
-        $targetEmployeeId = $nonAdminId + rand(1000, 9999);
-        $this->createTestEmployee($targetEmployeeId);
-        
+
+        $targetEmployeeId = $this->createEmployee(email: 'test@test.com', username: 'testuser');
+
         $response = $this->post('/employees/save/' . $targetEmployeeId, [
             'first_name' => 'Test',
             'last_name' => 'Employee',
             'email' => 'test@test.com',
             'username' => 'testuser',
-            'grant_employees' => 'employees',
-            'grant_config' => 'config'
+            'language' => 'en:english',
+            'grant_config' => 'config',
+            'grant_giftcards' => 'giftcards'
         ]);
-        
+
         $employeeModel = model(Employee::class);
-        $hasEmployeesGrant = $employeeModel->has_grant('employees', $targetEmployeeId);
         $hasConfigGrant = $employeeModel->has_grant('config', $targetEmployeeId);
-        
-        $this->assertFalse($hasEmployeesGrant);
+        $hasGiftcardsGrant = $employeeModel->has_grant('giftcards', $targetEmployeeId);
+
         $this->assertFalse($hasConfigGrant);
+        $this->assertFalse($hasGiftcardsGrant);
     }
 
     public function testAdminCanModifyAnyAccount(): void
@@ -160,9 +162,10 @@ class EmployeesControllerTest extends CIUnitTestCase
             'first_name' => 'Modified',
             'last_name' => 'User',
             'email' => 'modified@test.com',
-            'username' => 'nonadmin'
+            'username' => 'modified',
+            'language' => 'en:english'
         ]);
-        
+
         $response->assertStatus(200);
         $result = json_decode($response->getJSON(), true);
         $this->assertTrue($result['success']);
@@ -174,7 +177,7 @@ class EmployeesControllerTest extends CIUnitTestCase
         $this->loginAsAdmin();
         
         $response = $this->post('/employees/delete', [
-            'ids' => [$nonAdminId]
+            'ids' => [(string)$nonAdminId]
         ]);
         
         $response->assertStatus(200);
@@ -191,7 +194,8 @@ class EmployeesControllerTest extends CIUnitTestCase
             'first_name' => 'Modified',
             'last_name' => 'OwnAccount',
             'email' => 'own@test.com',
-            'username' => 'nonadmin'
+            'username' => 'owned',
+            'language' => 'en:english'
         ]);
         
         $response->assertStatus(200);
@@ -228,7 +232,8 @@ class EmployeesControllerTest extends CIUnitTestCase
             'first_name' => 'NonAdmin',
             'last_name'  => 'User',
             'email'      => 'nonadmin@test.com',
-            'username'   => 'nonadmin'
+            'username'   => 'grantany' . uniqid(),
+            'language'   => 'en:english'
         ];
         foreach ($permissionsRequested as $perm) {
             $postData['grant_' . $perm] = $perm;
@@ -248,7 +253,18 @@ class EmployeesControllerTest extends CIUnitTestCase
 
     public function testGrantChangeRequestFailsWhenGrantChangeDisallowed(): void
     {
-        $employeeId = $this->createNonAdminEmployee();
+        // Target holds customers+sales but NOT employees, so requesting the
+        // employees grant is a genuine change that DISALLOW_GRANT_CHANGE=true
+        // must reject (leaving the grant set untouched).
+        $unique = uniqid();
+        $employeeId = $this->createEmployee(
+            email:    "disallow{$unique}@test.com",
+            username: "disallow{$unique}",
+            grants: [
+                ['permission_id' => 'customers', 'menu_group' => 'home'],
+                ['permission_id' => 'sales', 'menu_group' => 'home'],
+            ],
+        );
         $this->loginAsAdmin();
 
         putenv('DISALLOW_GRANT_CHANGE=true');
@@ -256,8 +272,9 @@ class EmployeesControllerTest extends CIUnitTestCase
         $response = $this->post('/employees/save/' . $employeeId, [
             'first_name'      => 'NonAdmin',
             'last_name'       => 'User',
-            'email'           => 'nonadmin@test.com',
-            'username'        => 'nonadmin',
+            'email'           => "disallow{$unique}@test.com",
+            'username'        => "disallow{$unique}",
+            'language'        => 'en:english',
             'grant_employees' => 'employees'
         ]);
 
@@ -305,7 +322,8 @@ class EmployeesControllerTest extends CIUnitTestCase
             'first_name'      => 'NonAdmin',
             'last_name'       => 'User',
             'email'           => 'nonadmin@test.com',
-            'username'        => 'nonadmin',
+            'username'        => 'grantsucc',
+            'language'        => 'en:english',
             'grant_employees' => 'employees'
         ]);
 
@@ -329,8 +347,17 @@ class EmployeesControllerTest extends CIUnitTestCase
             'first_name'      => 'Brand',
             'last_name'       => 'New2',
             'email'           => 'brandnew2@test.com',
+            'phone_number'    => '555-1234',
+            'address_1'       => '',
+            'address_2'       => '',
+            'city'            => '',
+            'state'           => '',
+            'zip'             => '',
+            'country'         => '',
+            'comments'        => '',
             'username'        => 'brandnew2',
             'password'        => 'password123',
+            'language'        => 'en:english',
             'grant_customers' => 'customers'
         ]);
 

tests/Controllers/SalesControllerTest.php

@@ -10,6 +10,7 @@ use CodeIgniter\Config\Services;
 use App\Models\Employee;
 use Config\Database;
 use Tests\Support\ItemFixtureTrait;
+use Tests\Support\EmployeeFixtureTrait;
 use Tests\Support\SaleFixtureTrait;
 
 /**
@@ -29,6 +30,7 @@ class SalesControllerTest extends CIUnitTestCase
     use DatabaseTestTrait;
     use FeatureTestTrait;
     use ItemFixtureTrait;
+    use EmployeeFixtureTrait;
     use SaleFixtureTrait;
 
     protected $migrate     = true;
@@ -56,160 +58,70 @@ class SalesControllerTest extends CIUnitTestCase
         parent::tearDown();
     }
 
+    /**
+     * Cashier with bare "sales" + "sales_stock" grants. "sales_stock" is
+     * required alongside "sales": Employee::has_module_grant('sales', ...)
+     * treats the bare "sales" grant as insufficient once any sales_*
+     * submodule permission exists in the permissions table (see
+     * has_subpermissions()). Deliberately NOT "reports_sales" — that is the
+     * grant this test exercises the gate on.
+     */
     protected function createCashierEmployee(): int
     {
-        $unique = uniqid();
-
-        $personData = [
-            'first_name'   => 'Cashier',
-            'last_name'    => 'NoReports',
-            'email'        => "cashier.$unique@test.com",
-            'phone_number' => '555-0001',
-            'address_1'    => '',
-            'address_2'    => '',
-            'city'         => '',
-            'state'        => '',
-            'zip'          => '',
-            'country'      => '',
-            'comments'     => '',
-        ];
-
-        $employeeData = [
-            'username'      => "cashier.$unique",
-            'password'      => password_hash('password123', PASSWORD_DEFAULT),
-            'hash_version'  => 2,
-            'language_code' => 'en',
-            'language'      => 'english'
-        ];
-
-        // Deliberately grants "sales" (register access) but NOT "reports_sales".
-        // "sales_stock" is also required: Employee::has_module_grant('sales', ...)
-        // treats the bare "sales" grant as insufficient once any sales_* submodule
-        // permission exists in the permissions table (see has_subpermissions()).
-        $grantsData = [
-            ['permission_id' => 'sales', 'menu_group' => 'home'],
-            ['permission_id' => 'sales_stock', 'menu_group' => 'home']
-        ];
-
-        $employeeModel = model(Employee::class);
-        $this->assertTrue($employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY));
-
-        return (int) $personData['person_id'];
+        return $this->createEmployee(
+            first_name: 'Cashier',
+            last_name:  'NoReports',
+            email:      'cashier.' . uniqid() . '@test.com',
+            username:   'cashier_' . uniqid(),
+            grants: [
+                ['permission_id' => 'sales', 'menu_group' => 'home'],
+                ['permission_id' => 'sales_stock', 'menu_group' => 'home'],
+            ],
+        );
     }
 
     protected function createReportsSalesEmployee(): int
     {
-        $unique = uniqid();
-
-        $personData = [
-            'first_name'   => 'Supervisor',
-            'last_name'    => 'WithReports',
-            'email'        => "supervisor.$unique@test.com",
-            'phone_number' => '555-0002',
-            'address_1'    => '',
-            'address_2'    => '',
-            'city'         => '',
-            'state'        => '',
-            'zip'          => '',
-            'country'      => '',
-            'comments'     => '',
-        ];
-
-        $employeeData = [
-            'username'      => "supervisor.$unique",
-            'password'      => password_hash('password123', PASSWORD_DEFAULT),
-            'hash_version'  => 2,
-            'language_code' => 'en',
-            'language'      => 'english'
-        ];
-
-        $grantsData = [
-            ['permission_id' => 'sales', 'menu_group' => 'home'],
-            ['permission_id' => 'sales_stock', 'menu_group' => 'home'],
-            ['permission_id' => 'reports_sales', 'menu_group' => 'home']
-        ];
-
-        $employeeModel = model(Employee::class);
-        $this->assertTrue($employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY));
-
-        return (int) $personData['person_id'];
+        return $this->createEmployee(
+            first_name: 'Supervisor',
+            last_name:  'WithReports',
+            email:      'supervisor.' . uniqid() . '@test.com',
+            username:   'supervisor_' . uniqid(),
+            grants: [
+                ['permission_id' => 'sales', 'menu_group' => 'home'],
+                ['permission_id' => 'sales_stock', 'menu_group' => 'home'],
+                ['permission_id' => 'reports_sales', 'menu_group' => 'home'],
+            ],
+        );
     }
 
     protected function createCashierWithoutChangePriceGrant(): int
     {
-        $unique = uniqid();
-
-        $personData = [
-            'first_name'   => 'Cashier',
-            'last_name'    => 'NoChangePrice',
-            'email'        => "cashier-nochangeprice.$unique@test.com",
-            'phone_number' => '555-0001',
-            'address_1'    => '',
-            'address_2'    => '',
-            'city'         => '',
-            'state'        => '',
-            'zip'          => '',
-            'country'      => '',
-            'comments'     => '',
-        ];
-
-        $employeeData = [
-            'username'      => "cashier_nochangeprice.$unique",
-            'password'      => password_hash('password123', PASSWORD_DEFAULT),
-            'hash_version'  => 2,
-            'language_code' => 'en',
-            'language'      => 'english'
-        ];
-
-        // "sales_stock" is required alongside "sales": see the has_module_grant/
-        // has_subpermissions note on createCashierEmployee() above.
-        $grantsData = [
-            ['permission_id' => 'sales', 'menu_group' => 'home'],
-            ['permission_id' => 'sales_stock', 'menu_group' => 'home'],
-        ];
-
-        $employeeModel = model(Employee::class);
-        $this->assertTrue($employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY));
-
-        return (int) $personData['person_id'];
+        return $this->createEmployee(
+            first_name: 'Cashier',
+            last_name:  'NoChangePrice',
+            email:      'cashier-nochangeprice.' . uniqid() . '@test.com',
+            username:   'cashier_nochangeprice_' . uniqid(),
+            grants: [
+                ['permission_id' => 'sales', 'menu_group' => 'home'],
+                ['permission_id' => 'sales_stock', 'menu_group' => 'home'],
+            ],
+        );
     }
 
     protected function createCashierWithChangePriceGrant(): int
     {
-        $unique = uniqid();
-
-        $personData = [
-            'first_name'   => 'Cashier',
-            'last_name'    => 'ChangePrice',
-            'email'        => "cashier-changeprice.$unique@test.com",
-            'phone_number' => '555-0002',
-            'address_1'    => '',
-            'address_2'    => '',
-            'city'         => '',
-            'state'        => '',
-            'zip'          => '',
-            'country'      => '',
-            'comments'     => '',
-        ];
-
-        $employeeData = [
-            'username'      => "cashier_changeprice.$unique",
-            'password'      => password_hash('password123', PASSWORD_DEFAULT),
-            'hash_version'  => 2,
-            'language_code' => 'en',
-            'language'      => 'english'
-        ];
-
-        $grantsData = [
-            ['permission_id' => 'sales', 'menu_group' => 'home'],
-            ['permission_id' => 'sales_stock', 'menu_group' => 'home'],
-            ['permission_id' => 'sales_change_price', 'menu_group' => 'home'],
-        ];
-
-        $employeeModel = model(Employee::class);
-        $this->assertTrue($employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY));
-
-        return (int) $personData['person_id'];
+        return $this->createEmployee(
+            first_name: 'Cashier',
+            last_name:  'ChangePrice',
+            email:      'cashier-changeprice.' . uniqid() . '@test.com',
+            username:   'cashier_changeprice_' . uniqid(),
+            grants: [
+                ['permission_id' => 'sales', 'menu_group' => 'home'],
+                ['permission_id' => 'sales_stock', 'menu_group' => 'home'],
+                ['permission_id' => 'sales_change_price', 'menu_group' => 'home'],
+            ],
+        );
     }
 
     protected function loginAs(int $personId): void

tests/Libraries/Sale_libPaymentTest.php

@@ -20,10 +20,20 @@ class Sale_libPaymentTest extends CIUnitTestCase
 {
     private Sale_lib $saleLib;
 
+    protected int $priorBcscale;
+
     protected function setUp(): void
     {
         parent::setUp();
 
+        // The app sets a global bcscale() from app config on config load
+        // (see app/Events/Load_config.php), and whichever value a prior test
+        // class triggered leaks into bcadd/bcmul here. Pin a known scale so the
+        // accumulation math in these tests is deterministic regardless of suite
+        // order; restore it in tearDown.
+        $this->priorBcscale = bcscale();
+        bcscale(0);
+
         // Inject mock OSPOS config so Sale_lib constructor and helpers don't need real settings
         $ospos           = new OSPOS();
         $ospos->settings = [
@@ -60,6 +70,7 @@ class Sale_libPaymentTest extends CIUnitTestCase
 
     protected function tearDown(): void
     {
+        bcscale($this->priorBcscale);
         Factories::reset();
         parent::tearDown();
     }

tests/Models/EmployeeTest.php

@@ -5,10 +5,12 @@ namespace Tests\Models;
 use CodeIgniter\Test\CIUnitTestCase;
 use CodeIgniter\Test\DatabaseTestTrait;
 use App\Models\Employee;
+use Tests\Support\EmployeeFixtureTrait;
 
 class EmployeeTest extends CIUnitTestCase
 {
     use DatabaseTestTrait;
+    use EmployeeFixtureTrait;
 
     protected $migrate     = true;
     protected $migrateOnce = true;
@@ -167,47 +169,23 @@ class EmployeeTest extends CIUnitTestCase
         $this->assertFalse($result);
     }
 
-    protected function createEmployeeWithGrants(array $grantsData): int
-    {
-        $uniqueSuffix = uniqid();
-
-        $personData = [
-            'first_name'   => 'Grant',
-            'last_name'    => 'Tester',
-            'email'        => "granttester{$uniqueSuffix}@test.com",
-            'phone_number' => '555-5678'
-        ];
-
-        $employeeData = [
-            'username'      => "granttester{$uniqueSuffix}",
-            'password'      => password_hash('password123', PASSWORD_DEFAULT),
-            'hash_version'  => 2,
-            'language_code' => 'en',
-            'language'      => 'english'
-        ];
-
-        $employeeModel = model(Employee::class);
-        $result = $employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY);
-
-        $this->assertTrue($result);
-        $this->assertArrayHasKey('person_id', $personData);
-
-        return $personData['person_id'];
-    }
-
     public function testExistingEmployeeKeepsOriginalGrantsWhenGrantChangeDisallowed(): void
     {
-        $employeeId = $this->createEmployeeWithGrants([
-            ['permission_id' => 'customers', 'menu_group' => 'home']
-        ]);
+        $employeeId = $this->createEmployee(
+            first_name: 'Grant',
+            last_name:  'Tester',
+            grants: [
+                ['permission_id' => 'customers', 'menu_group' => 'home']
+            ],
+        );
 
         $originalDisallowGrantChange = getenv('DISALLOW_GRANT_CHANGE');
         putenv('DISALLOW_GRANT_CHANGE=true');
 
         try {
             $employeeModel = model(Employee::class);
-            $personData = ['first_name' => 'Grant', 'last_name' => 'Tester'];
-            $employeeData = ['username' => 'granttester', 'language_code' => 'en', 'language' => 'english'];
+            $personData = ['first_name' => 'Grant', 'last_name' => 'Tester', 'email' => "granttester_upd_{$employeeId}@test.com"];
+            $employeeData = ['username' => "granttester_upd_{$employeeId}", 'language_code' => 'en', 'language' => 'english'];
             $newGrantsData = [['permission_id' => 'sales', 'menu_group' => 'home']];
 
             $saveEmployeeResult = $employeeModel->save_employee($personData, $employeeData, $newGrantsData, $employeeId);
@@ -228,9 +206,13 @@ class EmployeeTest extends CIUnitTestCase
         putenv('DISALLOW_GRANT_CHANGE=true');
 
         try {
-            $result = $this->createEmployeeWithGrantsExpectingFailure([
-                ['permission_id' => 'customers', 'menu_group' => 'home']
-            ]);
+            $result = $this->createEmployeeExpectingFailure(
+                first_name: 'Rejected',
+                last_name:  'Tester',
+                grants: [
+                    ['permission_id' => 'customers', 'menu_group' => 'home']
+                ],
+            );
 
             $this->assertFalse($result);
         } finally {
@@ -240,39 +222,19 @@ class EmployeeTest extends CIUnitTestCase
         }
     }
 
-    protected function createEmployeeWithGrantsExpectingFailure(array $grantsData): bool
-    {
-        $uniqueSuffix = uniqid();
-
-        $personData = [
-            'first_name'   => 'Rejected',
-            'last_name'    => 'Tester',
-            'email'        => "rejectedtester{$uniqueSuffix}@test.com",
-            'phone_number' => '555-9999'
-        ];
-
-        $employeeData = [
-            'username'      => "rejectedtester{$uniqueSuffix}",
-            'password'      => password_hash('password123', PASSWORD_DEFAULT),
-            'hash_version'  => 2,
-            'language_code' => 'en',
-            'language'      => 'english'
-        ];
-
-        $employeeModel = model(Employee::class);
-
-        return $employeeModel->save_employee($personData, $employeeData, $grantsData, NEW_ENTRY);
-    }
-
     public function testExistingEmployeeGrantsUpdateWhenGrantChangeAllowed(): void
     {
-        $employeeId = $this->createEmployeeWithGrants([
-            ['permission_id' => 'customers', 'menu_group' => 'home']
-        ]);
+        $employeeId = $this->createEmployee(
+            first_name: 'Grant',
+            last_name:  'Tester',
+            grants: [
+                ['permission_id' => 'customers', 'menu_group' => 'home']
+            ],
+        );
 
         $employeeModel = model(Employee::class);
-        $personData = ['first_name' => 'Grant', 'last_name' => 'Tester'];
-        $employeeData = ['username' => 'granttester', 'language_code' => 'en', 'language' => 'english'];
+        $personData = ['first_name' => 'Grant', 'last_name' => 'Tester', 'email' => "granttester_upd_{$employeeId}@test.com"];
+        $employeeData = ['username' => "granttester_upd_{$employeeId}", 'language_code' => 'en', 'language' => 'english'];
         $newGrantsData = [['permission_id' => 'sales', 'menu_group' => 'home']];
 
         $employeeModel->save_employee($personData, $employeeData, $newGrantsData, $employeeId);
@@ -283,9 +245,13 @@ class EmployeeTest extends CIUnitTestCase
 
     public function testNewEmployeeCreationWithGrantsSucceedsWhenGrantChangeAllowed(): void
     {
-        $result = $this->createEmployeeWithGrantsExpectingFailure([
-            ['permission_id' => 'customers', 'menu_group' => 'home']
-        ]);
+        $result = $this->createEmployeeExpectingFailure(
+            first_name: 'Granted',
+            last_name:  'Tester',
+            grants: [
+                ['permission_id' => 'customers', 'menu_group' => 'home']
+            ],
+        );
 
         $this->assertTrue((bool) $result);
     }

tests/Support/EmployeeFixtureTrait.php

@@ -2,35 +2,140 @@
 
 namespace Tests\Support;
 
-use Config\Database;
+use App\Models\Employee;
 
+/**
+ * Shared factory for creating employees in test fixtures.
+ *
+ * All employees created via this trait route through
+ * {@see Employee::save_employee()}, which is the same code path the
+ * application itself uses. That keeps fixtures exercising the real
+ * validation and grant-handling logic instead of raw DB inserts.
+ *
+ * Every employee gets a unique email and username by default (uniqueness
+ * required because tests do not always run against a freshly-reset DB —
+ * see opensourcepos/opensourcepos#4626 for the root-cause history).
+ *
+ * Usage:
+ *
+ *   use Tests\Support\EmployeeFixtureTrait;
+ *
+ *   class FooTest extends CIUnitTestCase
+ *   {
+ *       use EmployeeFixtureTrait;
+ *
+ *       public function testSomething(): void
+ *       {
+ *           $personId = $this->createEmployee(
+ *               first_name: 'Cashier',
+ *               last_name:  'NoReports',
+ *               email:      'cashier.' . uniqid() . '@test.com',
+ *               username:   'cashier_' . uniqid(),
+ *               grants: [
+ *                   ['permission_id' => 'sales', 'menu_group' => 'home'],
+ *               ],
+ *           );
+ *       }
+ *   }
+ */
 trait EmployeeFixtureTrait
 {
-    protected function createEmployee(): int
-    {
-        $db = Database::connect();
+    /**
+     * Create a new employee (person + employee row + grants) via the
+     * application's own code path ({@see Employee::save_employee()}).
+     *
+     * @param string $first_name
+     * @param string $last_name
+     * @param null|string $email   unique email (auto-generated if omitted)
+     * @param null|string $username unique username (auto-generated if omitted)
+     * @param null|string $phone_number
+     * @param array $grants list of ['permission_id' => ..., 'menu_group' => ...]
+     * @param array $employee extra ospos_employees row overrides
+     *
+     * @return int the new person_id
+     */
+    protected function createEmployee(
+        string $first_name = 'Temp',
+        string $last_name = 'Employee',
+        ?string $email = null,
+        ?string $username = null,
+        ?string $phone_number = null,
+        array $grants = [],
+        array $employee = []
+    ): int {
+        if ($email === null) {
+            $email = 'employee.' . uniqid() . '@test.com';
+        }
+        if ($username === null) {
+            $username = 'employee_' . uniqid();
+        }
+        if ($phone_number === null) {
+            $phone_number = '';
+        }
 
-        $db->table('people')->insert([
-            'first_name'   => 'Test',
-            'last_name'    => 'Employee',
-            'phone_number' => '555-0200',
-            'email'        => 'employee-' . uniqid() . '@test.com',
-            'address_1'    => '',
-            'address_2'    => '',
-            'city'         => '',
-            'state'        => '',
-            'zip'          => '',
-            'country'      => '',
-            'comments'     => '',
-        ]);
-        $personId = (int) $db->insertID();
+        $personData = [
+            'first_name'   => $first_name,
+            'last_name'    => $last_name,
+            'email'        => $email,
+            'phone_number' => $phone_number,
+        ];
 
-        $db->table('employees')->insert([
-            'username'  => 'employee_' . uniqid(),
-            'password'  => password_hash('password123', PASSWORD_DEFAULT),
-            'person_id' => $personId,
-        ]);
+        $employeeData = array_merge([
+            'username'      => $username,
+            'password'      => password_hash('password123', PASSWORD_DEFAULT),
+            'hash_version'  => 2,
+            'language_code' => 'en',
+            'language'      => 'english',
+        ], $employee);
 
-        return $personId;
+        $model = model(Employee::class);
+        $this->assertTrue(
+            $model->save_employee($personData, $employeeData, $grants, NEW_ENTRY),
+            'createEmployee: save_employee() failed'
+        );
+        $this->assertArrayHasKey('person_id', $personData);
+
+        return (int) $personData['person_id'];
+    }
+
+    /**
+     * Variant of {@see self::createEmployee()} for tests that expect the
+     * save to fail (e.g. new-employee creation when DISALLOW_GRANT_CHANGE
+     * is true). Returns the raw save_employee() result without asserting.
+     */
+    protected function createEmployeeExpectingFailure(
+        string $first_name = 'Rejected',
+        string $last_name = 'Tester',
+        ?string $email = null,
+        ?string $username = null,
+        ?string $phone_number = null,
+        array $grants = [],
+        array $employee = []
+    ): bool {
+        if ($email === null) {
+            $email = 'employee.' . uniqid() . '@test.com';
+        }
+        if ($username === null) {
+            $username = 'employee_' . uniqid();
+        }
+        if ($phone_number === null) {
+            $phone_number = '';
+        }
+
+        $personData = [
+            'first_name'   => $first_name,
+            'last_name'    => $last_name,
+            'email'        => $email,
+            'phone_number' => $phone_number,
+        ];
+        $employeeData = array_merge([
+            'username'      => $username,
+            'password'      => password_hash('password123', PASSWORD_DEFAULT),
+            'hash_version'  => 2,
+            'language_code' => 'en',
+            'language'      => 'english',
+        ], $employee);
+
+        return model(Employee::class)->save_employee($personData, $employeeData, $grants, NEW_ENTRY);
     }
 }