Skip to content

Refactor admin context to data-driven business roles (#3612) - #4122

Open
gilgardosh wants to merge 2 commits into
mainfrom
claude/multi-tenant-data-sources-l5q2rs
Open

Refactor admin context to data-driven business roles (#3612)#4122
gilgardosh wants to merge 2 commits into
mainfrom
claude/multi-tenant-data-sources-l5q2rs

Conversation

@gilgardosh

Copy link
Copy Markdown
Collaborator

Summary

Replaces hardcoded, institution-specific business ID columns and literal UUIDs in the admin context provider with an abstract, data-driven admin_business_roles table. This enables tenants to own arbitrary sets of data sources (banks, credit cards, crypto wallets) and authorities without schema or code changes.

Key Changes

  • New database table: admin_business_roles with enum type for role classification (BANK_ACCOUNT, CREDIT_CARD, CRYPTO_WALLET, DIVIDEND_PAYMENT, VAT_EXCLUDED)

    • Includes RLS policies mirroring existing multi-business scope patterns
    • Primary index on (owner_id, role) for efficient aggregation queries
  • SQL query refactoring: Updated getAdminContexts and updateAdminContext queries to aggregate business IDs by role using correlated subqueries, exposing them as array columns (e.g., bank_account_business_ids, credit_card_business_ids)

  • Provider normalization logic: Rewrote normalizeContext() to derive all data-source and authority arrays from the role aggregates instead of hardcoded columns/UUIDs

    • Removed hardcoded dividend payment UUIDs that were leaking across tenants
    • Consolidated internal wallets derivation with deduplication
  • Legacy input adapter: Added syncProviderBusinessRoles() method to write-through sync deprecated typed mutation inputs (e.g., poalimBusinessId, isracardBusinessId) to admin_business_roles rows, maintaining consistency during the transition period

  • Migrations & seeding:

    • Two new migrations: table creation + backfill from existing columns
    • Updated seed scripts (seed.ts, seed-admin-context.ts, admin-onboarding.provider.ts) to populate roles
    • Updated getBootstrapContext query in onboarding provider
  • Test coverage: Added comprehensive test suite for normalizeContext() covering role aggregation, empty defaults, deduplication, and removal of cross-tenant UUID leaks

Implementation Details

  • The adapter is additive-only (ON CONFLICT DO NOTHING) for legacy inputs; removals are owned by Phase 2's data-source API
  • Defensive null-handling in toIdArray() helper accounts for pgTyped array nullability and test fixtures
  • Dividend payment businesses are re-homed from global hardcoded UUIDs to per-tenant roles via financial_entities lookup
  • All queries use COALESCE with empty array defaults to handle rows without aggregates (e.g., from unit tests)

https://claude.ai/code/session_01C9AcvxDWxpnbZ9oXRp3y6F

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the server’s “admin context” to be data-driven by introducing an admin_business_roles table and updating providers/seed/migrations so role-based business IDs (banks, cards, crypto wallets, dividend payments, VAT-excluded authorities) are derived from data rather than hardcoded columns/UUIDs.

Changes:

  • Adds admin_business_roles (enum + table + RLS + trigger) and a backfill migration from existing user_context columns (plus dividend-payment re-homing).
  • Updates admin-context and onboarding queries to expose per-role aggregated uuid[] columns, and updates normalization logic + legacy-input write-through syncing.
  • Updates seed scripts and adds/extends tests to validate role-driven normalization and VAT_EXCLUDED role seeding.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
scripts/seed.ts Seeds admin_business_roles from the newly created user_context row.
scripts/seed-demo-data.ts Includes admin_business_roles in demo-data cleanup/seed flow.
packages/server/src/modules/onboarding/providers/admin-onboarding.provider.ts Bootstraps context with role aggregates and seeds VAT_EXCLUDED role rows.
packages/server/src/modules/admin-context/providers/admin-context.provider.ts Exposes role aggregates via SQL, normalizes context from aggregates, and syncs legacy inputs into roles.
packages/server/src/modules/admin-context/tests/admin-context.provider.test.ts Adds unit tests for role-driven normalizeContext() behavior (defaults/dedup/no UUID leaks).
packages/server/src/tests/seed-admin-context.integration.test.ts Adds integration coverage for VAT_EXCLUDED role seeding.
packages/server/scripts/seed-admin-context.ts Seeds VAT_EXCLUDED roles as part of admin-context seeding.
packages/migrations/src/run-pg-migrations.ts Registers the new admin-business-roles migrations.
packages/migrations/src/actions/2026-07-30T10-00-00.add-admin-business-roles-table.ts Creates enum/table, RLS policy, index, and updated_at trigger.
packages/migrations/src/actions/2026-07-30T11-00-00.backfill-admin-business-roles.ts Backfills roles from legacy columns and re-homes dividend-payment UUIDs per-tenant.
Suppressed comments (1)

packages/server/src/modules/admin-context/providers/admin-context.provider.ts:311

  • Same as in getAdminContexts: the RETURNING aggregates use array_agg(...) without ORDER BY, so callers can observe nondeterministic ordering in the returned role-derived arrays. Adding ORDER BY inside array_agg makes the API output stable and avoids flaky behavior/tests.
    COALESCE((SELECT array_agg(r.business_id) FROM accounter_schema.admin_business_roles r WHERE r.owner_id = user_context.owner_id AND r.role = 'BANK_ACCOUNT'), '{}'::uuid[]) AS bank_account_business_ids,
    COALESCE((SELECT array_agg(r.business_id) FROM accounter_schema.admin_business_roles r WHERE r.owner_id = user_context.owner_id AND r.role = 'CREDIT_CARD'), '{}'::uuid[]) AS credit_card_business_ids,
    COALESCE((SELECT array_agg(r.business_id) FROM accounter_schema.admin_business_roles r WHERE r.owner_id = user_context.owner_id AND r.role = 'CRYPTO_WALLET'), '{}'::uuid[]) AS crypto_wallet_business_ids,
    COALESCE((SELECT array_agg(r.business_id) FROM accounter_schema.admin_business_roles r WHERE r.owner_id = user_context.owner_id AND r.role = 'DIVIDEND_PAYMENT'), '{}'::uuid[]) AS dividend_payment_business_ids,
    COALESCE((SELECT array_agg(r.business_id) FROM accounter_schema.admin_business_roles r WHERE r.owner_id = user_context.owner_id AND r.role = 'VAT_EXCLUDED'), '{}'::uuid[]) AS vat_excluded_business_ids;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/server/src/modules/admin-context/providers/admin-context.provider.ts Outdated
Comment thread packages/server/src/modules/onboarding/providers/admin-onboarding.provider.ts Outdated
Comment thread scripts/seed.ts Outdated
@gilgardosh
gilgardosh force-pushed the claude/multi-tenant-data-sources-l5q2rs branch from 2f01a9d to 2686bb4 Compare August 4, 2026 10:24
@gilgardosh
gilgardosh temporarily deployed to accounter-fullstack August 4, 2026 10:24 — with GitHub Actions Inactive
@gilgardosh
gilgardosh temporarily deployed to accounter-fullstack August 4, 2026 10:24 — with GitHub Actions Inactive
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@accounter/client 0.1.0-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/gmail-listener 0.1.3-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/green-invoice-graphql 0.8.7-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/hashavshevet-mesh 0.2.13-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/israeli-vat-scraper 0.1.13-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/modern-poalim-scraper 0.10.7-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/payper-mesh 0.2.13-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/scraper-app 0.0.3-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/server 0.2.0-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/shaam-uniform-format-generator 0.2.7-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎
@accounter/shaam6111-generator 0.1.9-alpha-20260804142305-12656c517b9f6de438905718d95c7126d6235231 npm ↗︎ unpkg ↗︎

gilgardosh pushed a commit that referenced this pull request Aug 4, 2026
- migration: add a RESTRICTIVE FOR DELETE policy on admin_business_roles pinning
  deletes to get_current_business_id(). DELETE evaluates only USING, so without
  it a multi-business read scope could delete another in-scope tenant's rows;
  mirrors 2026-05-26T10-00-00.rls-delete-write-target.
- admin-context + onboarding queries: add ORDER BY r.business_id inside every
  array_agg so the role-derived uuid[] arrays are deterministically ordered.
- scripts/seed.ts: parameterize the owner_id filter ($1 + [adminEntityId])
  instead of interpolating it into the SQL string.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C9AcvxDWxpnbZ9oXRp3y6F
@gilgardosh
gilgardosh temporarily deployed to accounter-fullstack August 4, 2026 10:30 — with GitHub Actions Inactive
@gilgardosh
gilgardosh temporarily deployed to accounter-fullstack August 4, 2026 10:30 — with GitHub Actions Inactive
@gilgardosh gilgardosh self-assigned this Aug 4, 2026
claude added 2 commits August 4, 2026 17:21
Replace the hardcoded, institution-specific logic in admin-context.provider's
normalizeContext (#3612) with an abstract, per-tenant admin_business_roles table.
Bank / credit-card / crypto data sources, dividend-payment businesses, and
VAT-report-excluded businesses are now sourced from role rows instead of
enumerated columns and literal UUIDs, so a tenant can own an arbitrary set of
data sources without a schema/code change.

- migrations: add accounter_schema.admin_business_role enum + admin_business_roles
  table (multi-business-scope RLS, FORCE); backfill roles from the existing
  user_context columns and re-home the two previously global dividend UUIDs to
  their true owner (fixes a latent cross-tenant leak)
- provider: aggregate the five role arrays into the context queries; rewrite
  normalizeContext to read them; add a gated write-through so the legacy
  per-institution updateAdminContext inputs keep roles in sync
- writers: onboarding + seedAdminCore create VAT_EXCLUDED roles for the authority
  businesses; the personal seed derives roles from the seeded context
- tests: unit coverage for the role-derived arrays (incl. no hardcoded UUIDs);
  integration coverage that seedAdminCore classifies the authorities

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C9AcvxDWxpnbZ9oXRp3y6F
- migration: add a RESTRICTIVE FOR DELETE policy on admin_business_roles pinning
  deletes to get_current_business_id(). DELETE evaluates only USING, so without
  it a multi-business read scope could delete another in-scope tenant's rows;
  mirrors 2026-05-26T10-00-00.rls-delete-write-target.
- admin-context + onboarding queries: add ORDER BY r.business_id inside every
  array_agg so the role-derived uuid[] arrays are deterministically ordered.
- scripts/seed.ts: parameterize the owner_id filter ($1 + [adminEntityId])
  instead of interpolating it into the SQL string.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C9AcvxDWxpnbZ9oXRp3y6F
@gilgardosh
gilgardosh force-pushed the claude/multi-tenant-data-sources-l5q2rs branch from 9b60aed to 622ed1b Compare August 4, 2026 14:21
@gilgardosh
gilgardosh temporarily deployed to accounter-fullstack August 4, 2026 14:21 — with GitHub Actions Inactive
@gilgardosh
gilgardosh temporarily deployed to accounter-fullstack August 4, 2026 14:21 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants