Skip to content

Remove custom domain from tenants collection#972

Open
busbyk wants to merge 6 commits intorevert-926-revert-1.7.0from
revert-926-revert-1.7.0-remove-custom-domain
Open

Remove custom domain from tenants collection#972
busbyk wants to merge 6 commits intorevert-926-revert-1.7.0from
revert-926-revert-1.7.0-remove-custom-domain

Conversation

@busbyk
Copy link
Collaborator

@busbyk busbyk commented Mar 4, 2026

Description

Removes the customDomain field from the tenants collection in the database, consolidating custom domain configuration into the hardcoded AVALANCHE_CENTERS constant in src/utilities/tenancy/avalancheCenters.ts. This eliminates a dual-source-of-truth problem where custom domains existed in both the database and the hardcoded constant, which could lead to mismatches.

Related Issues

Follow-up to #947.

Key Changes

  • Removed customDomain field from the Tenants collection schema (src/collections/Tenants/index.ts) and generated types
  • Added getEmailDomain() utility to avalancheCenters.ts — derives an email-safe domain from the custom domain by stripping www. prefix and port numbers
  • Refactored isTenantDomainScoped() — now uses findCenterByDomain() from AVALANCHE_CENTERS instead of querying the database for custom domains, with a subdomain fallback
  • Updated domain-scoped access controlvalidateDomainAccessBeforeLogin and byGlobalRoleOrTenantRoleAssignmentOrDomain now use the new getEmailDomain() utility
  • Simplified seed and bootstrap scripts — removed all customDomain references from seed data
  • Migration20260303_233752_remove_custom_domain_from_tenants.ts drops the custom_domain column from the tenants table
  • Fixed e2e tests — updated tenant cookie assertions to use slugs instead of numeric IDs (the payload-tenant cookie now stores slugs)

How to test

  1. Run pnpm seed:standalone — verify it completes without errors
  2. Run pnpm dev and visit nwac.localhost:3000 — verify the site resolves correctly via subdomain
  3. Visit localhost:3000/admin — verify the admin panel loads and tenant selector works
  4. Run pnpm tsc and pnpm lint — verify no type or lint errors
  5. Run pnpm test — verify all tests pass

Screenshots / Demo video

N/A — no visual changes

Migration Explanation

Migration: 20260303_233752_remove_custom_domain_from_tenants

  • Up: Drops the custom_domain column from the tenants table (ALTER TABLE tenants DROP COLUMN custom_domain)
  • Down: Re-adds the column as nullable text (ALTER TABLE tenants ADD custom_domain text)
  • Data loss: The column values are not preserved on rollback — they would need to be re-seeded. This is acceptable because the custom domain data now lives in the AVALANCHE_CENTERS constant.

@github-actions
Copy link

github-actions bot commented Mar 4, 2026

@busbyk busbyk self-assigned this Mar 4, 2026
@busbyk busbyk marked this pull request as ready for review March 6, 2026 16:20
Copy link
Collaborator

@rchlfryn rchlfryn left a comment

Choose a reason for hiding this comment

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

@busbyk it looks like this contains changes from other PRs. do you mind taking a look and cleaning it up?

busbyk and others added 3 commits March 7, 2026 11:33
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…urce of truth

Eliminates the dual-source mismatch between the hardcoded AVALANCHE_CENTERS
constant and the database tenants collection for custom domains. All custom
domain lookups now use AVALANCHE_CENTERS, with a new getEmailDomain() utility
for email domain matching that normalizes www. prefixes and ports.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace TenantIds with TenantSlugs in e2e test assertions since the
payload-tenant cookie now stores slugs instead of numeric IDs. Remove
the deprecated TenantIds constant.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@busbyk busbyk force-pushed the revert-926-revert-1.7.0-remove-custom-domain branch from c191d39 to 5cda597 Compare March 7, 2026 19:34
@busbyk
Copy link
Collaborator Author

busbyk commented Mar 7, 2026

@busbyk it looks like this contains changes from other PRs. do you mind taking a look and cleaning it up?

Fixed!

@busbyk busbyk mentioned this pull request Mar 7, 2026
Copy link
Collaborator

@rchlfryn rchlfryn left a comment

Choose a reason for hiding this comment

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

Overall looks good - couple of comments

Comment on lines +28 to +33
it('handles domains shared by multiple centers (returns first match)', () => {
// Multiple centers use 'alaskasnow.org': aaic, cac, earac, hac, vac
// Should return the first one found (aaic)
const result = findCenterByDomain('alaskasnow.org')
expect(result).toBe('aaic')
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I just realized how problematic this is. I don't think we should hold up this PR or #947 based on this because this is the same behavior we had before and we only need to solve for this when we onboard any of the alaskasnow.org centers. But I'll start a thread in Slack to discuss.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine to side step for now.

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.

2 participants