feat: schema changes for trial users entity to allow searching by external user id

[vivesing]

PR Summary: Add externalUserId lookup for TrialUser

Goal: Expose the external_user_id DB index (added in mysticat-data-service) through the JS data-access layer for TrialUser, so callers can look up trial users by their external IDP user ID.

Review feedback addressed

• Blocking: index.d.ts was missing type declarations for the new accessors (TS consumers would get a compile error) — fixed.
• Suggestion: documented that externalUserId isn't guaranteed unique, so findBy* returns an arbitrary match — callers needing all matches should use allBy*.
• Nit: added the not-found IT assertion to pin the null contract.
• Nit: flagged via comment thaf the 5-index cap enforced bySchemaBuilder, so the next lookup index needs budget consideration.

[MysticatBot]

Hey @vivesing,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - type declaration has a return-type bug that will break TS consumers.
Complexity: LOW - small diff, single entity, single package.
Changes: Adds an externalUserId lookup index to the TrialUser entity with corresponding type declarations and a not-found integration test (3 files).

Must fix before merge

1. [Important] findByExternalUserIdAndUpdatedAt return type is Promise<TrialUser[]> but should be Promise<TrialUser | null> per findBy* convention - index.d.ts:60 (details inline)

Non-blocking (1): minor issues and suggestions

• suggestion: Add a positive-match integration test for findByExternalUserId (and ideally the compound *AndUpdatedAt accessor) to prove the index resolves records, not just the not-found path - test/it/trial-user/trial-user.test.js

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 59s | Cost: $2.95 | Commit: f1233f550323bf889c3487b14ab1dc1b79de4c47
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[calvarezg]

Hey @vivesing,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - the change is low-risk (types, comments, one test), but two items should land before merge.
Complexity: MEDIUM - small diff, but it touches the data-access model/schema surface.
Changes: Adds hand-written index.d.ts type declarations for the externalUserId lookup accessors, explanatory schema comments, and a not-found integration test; the underlying index itself already shipped in the merged sibling PR 1869 (3 files).

Must fix before merge

1. [Important] externalUserId non-uniqueness caveat is missing from the consumer-facing type surface - packages/spacecat-shared-data-access/src/models/trial-user/index.d.ts:56 (details inline)
2. [Important] Three of the four newly-declared accessors have no test coverage - packages/spacecat-shared-data-access/test/it/trial-user/trial-user.test.js:91 (details inline)

Non-blocking (4): minor issues and suggestions

• nit: PR title/description says "schema changes ... to allow searching by external user id", but the index already existed on main (shipped in the merged sibling PR); this diff is type declarations, comments, and one test. Correct the description so reviewers and ops do not assume a data-layer migration.
• suggestion: externalUserId has no validate (max length / character allowlist), unlike sibling attributes in the same schema; it originates from an external IDP claim, so a cheap validator would harden the write path - packages/spacecat-shared-data-access/src/models/trial-user/trial-user.schema.js
• nit: findByExternalUserIdAndUpdatedAt / allByExternalUserIdAndUpdatedAt match exactly on updatedAt, which is server-set on every write, so a caller cannot supply it without already holding the record; a one-line note on intended use (e.g. pagination continuation) would help - packages/spacecat-shared-data-access/src/models/trial-user/index.d.ts
• nit: the index-budget comment does not note that addReference('belongs_to', ...) also consumes the same 5-index budget; one clause would prevent a future author silently exceeding it - packages/spacecat-shared-data-access/src/models/trial-user/trial-user.schema.js

Out of scope, worth tracking

• The externalUserId lookup has no provider dimension, so it cannot disambiguate which IDP issued an id; two providers can assign the same subject id to different people. Provider scoping was removed org-wide previously, and callers live in other repos, so this is a follow-up design conversation, not a change for this PR.
• index.d.ts still declares getProvider / setProvider / allByProvider* / findByProvider*, which reference a provider attribute the schema no longer defines. This is pre-existing drift (not touched by this diff) but is a live compile-green / runtime-crash trap for TS consumers. No CI check catches .d.ts-vs-schema drift in either direction.
• The explicit organizationId index duplicates the auto-generated belongs_to index composite, burning one index slot for no extra query capability (pre-existing).
• The "5-index cap" comment reflects a DynamoDB-era model; the backend is now Postgres via PostgREST, so the cap is a self-imposed constant rather than a live GSI-per-table limit. Worth re-justifying or re-framing.
• The DB-level uniqueness stance for external_user_id lives in mysticat-data-service (different repo); confirm it does not contradict the "not enforced unique" caveat.

Previously flagged, now resolved

• Prior return-type finding on the compound accessor - current code returns Promise<TrialUser | null>, consistent with the findBy* convention.

[calvarezg]

issue (blocking): The non-uniqueness caveat is invisible to TypeScript consumers.

externalUserId is optional and not enforced unique, so findByExternalUserId / findByExternalUserIdAndUpdatedAt return one arbitrary match. That warning currently lives only as a comment in trial-user.schema.js. Consumers import the typed interface from index.d.ts and never open the schema file, so the caveat never reaches editor hovers or generated docs.

Fix: add a JSDoc block above findByExternalUserId and findByExternalUserIdAndUpdatedAt here carrying the same caveat and pointing callers to allByExternalUserId when they need every match. Note: no .d.ts in this package currently uses JSDoc, so this is a small net-new convention rather than matching an existing one.

[calvarezg]

issue (blocking): Three of the four newly-declared accessors have no test coverage.

This PR declares allByExternalUserId, allByExternalUserIdAndUpdatedAt, findByExternalUserId, and findByExternalUserIdAndUpdatedAt in index.d.ts. Only findByExternalUserId is exercised (a pre-existing positive test plus this PR's new not-found case). A repo-wide search finds no test touching the other three. Because index.d.ts is hand-maintained and JS callers ignore it, a wrong param order or a dropped sort-key composite would be caught by nothing - not a test, not the type checker.

Fix: add at least one IT per untested accessor, following the sibling allByOrganizationId test as a template. A single case that seeds two trial users sharing an externalUserId, then asserts allByExternalUserId returns both while findByExternalUserId returns exactly one, also pins the documented "arbitrary match" contract.