Skip to content

Comments

fix: move user_profiles migration from app-level to core package#672

Open
mmcintosh wants to merge 1 commit intoSonicJs-Org:mainfrom
mmcintosh:fix/user-profiles-core-migration
Open

fix: move user_profiles migration from app-level to core package#672
mmcintosh wants to merge 1 commit intoSonicJs-Org:mainfrom
mmcintosh:fix/user-profiles-core-migration

Conversation

@mmcintosh
Copy link
Contributor

Description

The user_profiles table migration is missing from the core package (packages/core/), causing 500 errors on the admin user edit page (/admin/users/:id/edit) and user update (PUT /admin/users/:id) for any app consuming @sonicjs-cms/core.

Root Cause

The table was introduced across three PRs:

PR Date What it did
#508 Jan 14 Created user_profiles as an app-level migration (my-sonicjs-app/migrations/018_user_profiles.sql)
#510 Jan 15 Refactored profile routes to use Drizzle ORM (app-level)
#512 Jan 20 Added raw SQL queries against user_profiles in core (packages/core/src/routes/admin-users.ts) — without adding a core migration

This means packages/core depends on a table it never creates. The E2E test for this feature was permanently skipped with this comment acknowledging the gap:

// Skip entire test suite in CI - requires user_profiles table migration
// which may not be applied to the CI database. Feature works locally.

Affected Code Paths

All in packages/core/src/routes/admin-users.ts:

  • Line 810-815 (GET edit page): SELECT ... FROM user_profiles WHERE user_id = ?
  • Line 969 (PUT update): SELECT id FROM user_profiles WHERE user_id = ?
  • Lines 974-978 (PUT update): UPDATE user_profiles SET ... WHERE user_id = ?
  • Lines 987-989 (PUT update): INSERT INTO user_profiles (...) VALUES (...)

Changes

Core Migration

  • Add packages/core/migrations/032_user_profiles.sql — creates the user_profiles table with the same schema as the app-level migration
  • All DDL uses CREATE TABLE IF NOT EXISTS / CREATE INDEX IF NOT EXISTS / CREATE TRIGGER IF NOT EXISTS for idempotency — safe for databases that already have the table from the app-level migration

Migration Auto-Detection

  • Add auto-detection block in packages/core/src/services/migrations.ts (autoDetectAppliedMigrations()) that checks for existing user_profiles table and marks migration 032 as applied if found
  • Follows the established pattern used for migrations 009 (system_logs) and 018 (settings)

E2E Tests Un-skipped

  • Remove test.describe.skip() from tests/e2e/38-user-profile-edit.spec.ts
  • 5 E2E tests now active: profile section display, save profile data, update existing profile, website URL validation, no-profile-fields scenario
  • Individual test.skip() guards inside each test are preserved (runtime safety nets for setup failures, unrelated to migration)

Unit Tests Added

  • New packages/core/src/__tests__/routes/admin-users-profile.test.ts — 8 tests covering:
    • GET edit page: profile found, no profile, user not found, database error
    • PUT update: update existing profile, create new profile, skip when no profile fields, profile database error

Testing

Unit Tests

  • Added 8 unit tests for user profile query logic
  • All unit tests passing (npx vitest run src/__tests__/routes/admin-users-profile.test.ts)

E2E Tests

  • Un-skipped 5 existing E2E tests for user profile editing
  • All E2E tests passing (previously skipped due to missing migration)

Technical Details

Files Changed

File Action Description
packages/core/migrations/032_user_profiles.sql Added Core migration — CREATE TABLE, INDEX, TRIGGER
packages/core/src/services/migrations.ts Modified Auto-detection for migration 032 (+15 lines)
packages/core/src/db/migrations-bundle.ts Auto-generated Rebuilt by npm run build to include migration 032
tests/e2e/38-user-profile-edit.spec.ts Modified Removed test.describe.skip, kept individual guards
packages/core/src/__tests__/routes/admin-users-profile.test.ts Added 8 unit tests for profile query logic

Table Schema

CREATE TABLE IF NOT EXISTS user_profiles (
  id TEXT PRIMARY KEY,
  user_id TEXT NOT NULL UNIQUE REFERENCES users(id) ON DELETE CASCADE,
  display_name TEXT,
  bio TEXT,
  company TEXT,
  job_title TEXT,
  website TEXT,
  location TEXT,
  date_of_birth INTEGER,
  created_at INTEGER NOT NULL DEFAULT (strftime('%s', 'now') * 1000),
  updated_at INTEGER NOT NULL DEFAULT (strftime('%s', 'now') * 1000)
);

Performance

No performance impact — this migration only runs once during bootstrap. The IF NOT EXISTS guards make it a no-op for databases that already have the table.

Breaking Changes

None. This is additive — it creates a table that was already expected to exist.

Migration Notes

  • Existing deployments (with app-level migration already applied): The auto-detection in MigrationService.autoDetectAppliedMigrations() will detect the existing table and mark migration 032 as applied without re-running it.
  • New deployments: Migration 032 will run during bootstrap and create the table.
  • No manual steps required for either scenario.

Known Issues

None.

Checklist

  • Code follows project conventions
  • Tests added/updated and passing
  • Type checking passes
  • No console errors or warnings
  • Documentation updated (if needed) — N/A, internal migration

The user_profiles table was introduced in upstream PRs SonicJs-Org#508/SonicJs-Org#510 as an
app-level migration (my-sonicjs-app/migrations/018_user_profiles.sql),
but PR SonicJs-Org#512 added raw SQL queries against it in the core package
(admin-users.ts) without a corresponding core migration. This caused
500 errors on the user edit page for any app using @sonicjs-cms/core
that hadn't independently applied the app-level migration.

- Add core migration 032_user_profiles.sql (IF NOT EXISTS for idempotency)
- Add auto-detection in MigrationService for existing tables
- Un-skip E2E test 38-user-profile-edit.spec.ts (5 tests)
- Add 8 unit tests for user profile query logic on edit/update routes
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.

1 participant