Skip to content

Implement Supabase schema and fix balance update bug - #59

Open
aj1126 wants to merge 8 commits into
mainfrom
preview
Open

Implement Supabase schema and fix balance update bug#59
aj1126 wants to merge 8 commits into
mainfrom
preview

Conversation

@aj1126

@aj1126 aj1126 commented Jun 25, 2026

Copy link
Copy Markdown
Owner

This pull request introduces two new entities, CategoryEntity and BudgetEntity, and integrates them into the backend data model. It also refactors transaction and household entities to support these new features and simplifies the sync logic by removing account balance adjustments from the synchronization process.

Entity and Data Model Enhancements:

  • Added CategoryEntity and BudgetEntity with full TypeORM integration, including relationships to HouseholdEntity and each other, and registered them in the application module and data source (app.module.ts, data-source.ts, category.entity.ts, budget.entity.ts) [1] [2] [3] [4] [5] [6].
  • Updated TransactionEntity to support category relationships: added categoryId foreign key, a relation to CategoryEntity, and a unique constraint on (householdId, localId) for better data consistency (transaction.entity.ts) [1] [2].
  • Extended HouseholdEntity to include a currency field with a default of 'USD' (household.entity.ts).
  • Added a role field to UserEntity with a default value of 'member' (user.entity.ts).

Sync and Transaction Logic Refactoring:

  • Removed account balance adjustment logic from both SyncService and TransactionsService, simplifying synchronization and transaction creation (sync.service.ts, transactions.service.ts) [1] [2] [3] [4] [5].

Testing Adjustments:

  • Updated sync service tests to remove dependencies on the account repository, reflecting the removal of account balance logic (sync.service.spec.ts) [1] [2] [3].

Migration Cleanup:

  • Removed the initial migration file, likely in preparation for a new schema migration reflecting these model changes (1781759488049-InitialSchema.ts).

@aj1126 aj1126 self-assigned this Jun 25, 2026
@aj1126 aj1126 added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request labels Jun 25, 2026
@aj1126

aj1126 commented Jun 25, 2026

Copy link
Copy Markdown
Owner Author

The failure is in the backend test job from .github/workflows/ci.yml, specifically the test-backend step that runs pnpm turbo run test --filter=@family-accountant/backend with a real PostgreSQL database.

The failing suite is packages/backend/src/test/database-trigger.integration.spec.ts. All 4 tests in that file failed, starting with:

  • should adjust account balance automatically on transaction INSERT

Root cause

The integration test assumes the database trigger updates accounts.balance immediately after writing a transaction, but the test setup is fragile in CI:

  • it uses dropSchema: true and migrationsRun: true
  • it depends on migrations recreating the trigger correctly
  • it manually clears only some tables in beforeEach
  • if the trigger migration was changed, not run as expected, or blocked by FK/table state, every assertion in this file fails together

Because the rest of the backend tests passed and only this trigger suite failed, the problem is most likely that the trigger is not present or not firing in the CI-created database, not that the repository APIs are broken.

Best fix

Make the integration test verify that the trigger exists before asserting balances, and isolate DB state more safely.

1. Strengthen the test setup

In database-trigger.integration.spec.ts, initialize the DB once, then truncate all relevant tables with RESTART IDENTITY CASCADE before each test instead of manual deletes.

Suggested replacement for beforeEach cleanup:

beforeEach(async () => {
  await txRepo.query(`
    TRUNCATE TABLE
      transactions,
      accounts,
      users,
      households
    RESTART IDENTITY CASCADE
  `);

  const user = userRepo.create({
    id: '11111111-1111-1111-1111-111111111111',
    email: 'integration@test.com',
    passwordHash: 'hashed',
    displayName: 'Test User',
    role: 'admin',
  });
  testUser = await userRepo.save(user);

  const household = householdRepo.create({
    id: '22222222-2222-2222-2222-222222222222',
    name: 'Integration Household',
    currency: 'USD',
    ownerId: testUser.id,
  });
  testHousehold = await householdRepo.save(household);

  testUser.householdId = testHousehold.id;
  await userRepo.save(testUser);
});

2. Add an explicit trigger existence assertion

Right after module setup, verify the migration actually created the trigger/function. This turns a vague balance mismatch into a clear migration failure.

Example:

beforeAll(async () => {
  module = await Test.createTestingModule({
    imports: [
      TypeOrmModule.forRoot({
        type: 'postgres',
        url: databaseUrl,
        entities: [
          UserEntity,
          HouseholdEntity,
          AccountEntity,
          TransactionEntity,
          DebtEntity,
          PaymentEntity,
          CategoryEntity,
          BudgetEntity,
        ],
        synchronize: false,
        dropSchema: true,
        migrationsRun: true,
        migrations: [path.join(__dirname, '../migrations/*{.ts,.js}')],
      }),
      TypeOrmModule.forFeature([
        UserEntity,
        HouseholdEntity,
        AccountEntity,
        TransactionEntity,
        DebtEntity,
        PaymentEntity,
        CategoryEntity,
        BudgetEntity,
      ]),
    ],
  }).compile();

  txRepo = module.get(getRepositoryToken(TransactionEntity));
  accountRepo = module.get(getRepositoryToken(AccountEntity));
  householdRepo = module.get(getRepositoryToken(HouseholdEntity));
  userRepo = module.get(getRepositoryToken(UserEntity));

  const triggers = await txRepo.query(`
    SELECT trigger_name
    FROM information_schema.triggers
    WHERE event_object_table = 'transactions'
  `);

  expect(triggers.length).toBeGreaterThan(0);
});

If the real issue is in the migration

The test file strongly suggests there should be a DB trigger handling:

  • insert
  • update of amount
  • update of accountId
  • soft delete / restore via deletedAt

If CI says all 4 fail, review the migration that creates the transaction balance trigger and ensure it handles these cases:

-- pseudo-logic
IF TG_OP = 'INSERT' AND NEW.deleted_at IS NULL THEN
  UPDATE accounts SET balance = balance + NEW.amount WHERE id = NEW.account_id;
END IF;

IF TG_OP = 'UPDATE' THEN
  -- undo old value if old row counted
  IF OLD.deleted_at IS NULL THEN
    UPDATE accounts SET balance = balance - OLD.amount WHERE id = OLD.account_id;
  END IF;

  -- apply new value if new row counts
  IF NEW.deleted_at IS NULL THEN
    UPDATE accounts SET balance = balance + NEW.amount WHERE id = NEW.account_id;
  END IF;
END IF;

That logic is required for the expectations in:

  • insert test: lines 126–159
  • amount update test: lines 161–195
  • account reassignment test: lines 197–250
  • soft delete/restore test: lines 252–298

Source: database-trigger.integration.spec.ts

Recommended change summary

  1. Keep the CI database setup as-is in ci.yml.
  2. Fix the trigger migration if it no longer creates or attaches the trigger to transactions.
  3. Improve database-trigger.integration.spec.ts by:
    • truncating tables instead of piecemeal deletes
    • asserting trigger presence in beforeAll

The highest-probability solution is: repair or reapply the transaction balance trigger migration, because the failing suite is explicitly validating trigger-driven balance updates and all trigger-related assertions failed together.

@aj1126

aj1126 commented Jun 25, 2026

Copy link
Copy Markdown
Owner Author

The failing job is the lint job in .github/workflows/ci.yml, specifically this step:

- run: pnpm turbo run lint

The blocking error is in packages/backend/src/modules/transactions/transactions.service.ts:

  • line 70: 'tx' is assigned a value but never used

Current code:

async softDelete(id: string, householdId: string): Promise<void> {
  const tx = await this.findOne(id, householdId);
  await this.repo.softDelete(id);
}

Solution

Since findOne(id, householdId) is only being used to validate existence and access, remove the unused variable and keep the call for its side effects:

async softDelete(id: string, householdId: string): Promise<void> {
  await this.findOne(id, householdId);
  await this.repo.softDelete(id);
}

Why this fixes it

  • findOne(...) still enforces:
    • transaction exists
    • transaction belongs to the household
  • lint no longer sees an unused local variable
  • behavior stays the same

Optional cleanup

The logs also show warnings in the same file for any usage at lines 68 and 80 in the lint output. Those warnings are not failing the job right now, but if you want a cleaner lint result, replace any with a concrete type where those values are declared.

@aj1126

aj1126 commented Jun 25, 2026

Copy link
Copy Markdown
Owner Author

To configure your TypeORM data source to handle both your local PostgreSQL setup and a secure cloud database connection dynamically, you can adjust your connection options based on the environment.

Since you are running locally without Docker and want to transition seamlessly to a cloud host (like [Aiven](https://aiven.io/free-postgresql-database) or [Supabase](https://supabase.com/database)), you can use an environment variable like NODE_ENV or check if the connection string points to a cloud instance to enforce SSL conditionally.

Here is the cleanest way to refactor your data-source.ts file using native TypeScript:

import { DataSource } from 'typeorm';
import * as dotenv from 'dotenv';
import * as path from 'path';

// Ensure environment variables are loaded
dotenv.config({ path: path.join(__dirname, '../../.env') });

const isProduction = process.env.NODE_ENV === 'production';
const dbUrl = process.env.DATABASE_URL;

// Automatically enable SSL if running in production or connecting to a remote cloud DB
const hasRemoteConnection = dbUrl && !dbUrl.includes('localhost') && !dbUrl.includes('127.0.0.1');
const shouldEnableSSL = isProduction || hasRemoteConnection;

export const AppDataSource = new DataSource({
  type: 'postgres',
  url: dbUrl || 'postgres://fa_user:fa_pass@localhost:5432/family_accountant',
  synchronize: !isProduction, // Use migrations for production/cloud environments
  logging: !isProduction,
  entities: [path.join(__dirname, '/**/*.entity{.ts,.js}')],
  migrations: [path.join(__dirname, '/migrations/*{.ts,.js}')],
  subscribers: [],
  // Dynamic SSL Configuration
  ssl: shouldEnableSSL
    ? {
        // rejectUnauthorized: false bypasses strict CA validation certificates 
        // which is standard for serverless environments like Neon or Supabase
        rejectUnauthorized: false,
      }
    : false,
});

Why This Workflow Works Cleanly

  1. Fallback Logic: If DATABASE_URL isn't present in your local .env, it gracefully falls back to your local credentials (postgres://fa_user:fa_pass@localhost:5432/family_accountant), keeping your local offline workflow intact.
  2. Smart Detection: By checking if the connection string points away from localhost via hasRemoteConnection, it turns on SSL automatically if you pass it a cloud string, even if you are testing it locally before pushing it to your CI pipeline.
  3. No Migration Blocks: Turning on rejectUnauthorized: false allows TypeORM to handshake cleanly with cloud instances without requiring you to manually download and point to specific root certificates in your repository.

Make sure your production startup script passes NODE_ENV=production so that TypeORM locks down logging and prepares for standard migrations rather than dropping columns implicitly.

@aj1126

aj1126 commented Jun 25, 2026

Copy link
Copy Markdown
Owner Author

@copilot check the pr and comments and fix the issues

Copilot AI commented Jun 25, 2026

Copy link
Copy Markdown

@copilot check the pr and comments and fix the issues

Addressed in commit 6142537.

I fixed the lint blocker in transactions.service.ts (softDelete no longer keeps an unused variable), hardened database-trigger.integration.spec.ts setup (TRUNCATE with RESTART IDENTITY CASCADE + explicit trigger assertion), and added migration 1781760000000-AddBalanceTriggerAndDefaults.ts to ensure the transaction balance trigger and required default columns (users.role, households.currency) exist in CI PostgreSQL runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants