fix: invitations list limited to sender org = current org#418
fix: invitations list limited to sender org = current org#418
Conversation
There was a problem hiding this comment.
Pull request overview
This PR scopes “pending invitations” listing to the current/sender organisation by introducing senderOrganisationId on invitations, enforcing sender-org membership checks in tRPC, and updating the dashboard UI/tests accordingly.
Changes:
- Add
senderOrganisationIdto invitations (model + migration) and set it when creating invitations. - Update
invitation.listto require an organisationId and only return pending invitations for that sender organisation. - Update invite-organisation dashboard (query + mutation) and unit tests to pass the organisation context.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/server/trpc/routers/invitation.test.ts | Updates tests to provide sender org context for list/create calls. |
| src/server/trpc/routers/organisation.ts | Ensures org member invites set senderOrganisationId. |
| src/server/trpc/routers/invitation.ts | Adds sender-org membership enforcement for create and org-scoped list input/logic. |
| src/server/repositories/Invitation.ts | Scopes pending invitation listing by senderOrganisationId. |
| src/models/Invitation.ts | Adds senderOrganisationId to the shared invitation schema. |
| src/app/(private)/(dashboards)/invite-organisation/page.tsx | Passes current organisationId into invitation.list query. |
| src/app/(private)/(dashboards)/invite-organisation/CreateInvitationModal.tsx | Sends senderOrganisationId on create and updates invalidation for parameterised list queries. |
| migrations/1774658921005_invitation_sender_organisation.ts | Adds/backfills sender_organisation_id and introduces FK + NOT NULL constraint. |
| bin/cmd.ts | Extends CLI invitation creation to include senderOrganisationId. |
| AGENTS.md | Documents CamelCasePlugin naming expectations for query builder vs raw SQL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Backfill: assign all existing invitations to the admin organisation | ||
| await sql` | ||
| UPDATE invitation | ||
| SET "sender_organisation_id" = ( | ||
| SELECT id FROM organisation WHERE name = 'Common Knowledge' LIMIT 1 | ||
| ) |
There was a problem hiding this comment.
The backfill relies on an organisation named 'Common Knowledge' existing. In environments where it doesn't, the subquery returns NULL and the subsequent NOT NULL alteration will fail. Consider backfilling from existing data instead (e.g., set sender_organisation_id = organisation_id), or create/select a guaranteed default sender org.
| // Backfill: assign all existing invitations to the admin organisation | |
| await sql` | |
| UPDATE invitation | |
| SET "sender_organisation_id" = ( | |
| SELECT id FROM organisation WHERE name = 'Common Knowledge' LIMIT 1 | |
| ) | |
| // Backfill from existing invitation data to avoid relying on a specific organisation seed | |
| await sql` | |
| UPDATE invitation | |
| SET "sender_organisation_id" = "organisation_id" |
| // Add foreign key constraint | ||
| await db.schema | ||
| .alterTable("invitation") | ||
| .addForeignKeyConstraint( | ||
| "invitationSenderOrganisationIdFKey", | ||
| ["senderOrganisationId"], | ||
| "organisation", | ||
| ["id"], | ||
| (cb) => cb.onDelete("set null").onUpdate("cascade"), | ||
| ) | ||
| .execute(); | ||
|
|
||
| // Make column not null after backfill | ||
| await db.schema | ||
| .alterTable("invitation") | ||
| .alterColumn("senderOrganisationId", (col) => col.setNotNull()) | ||
| .execute(); |
There was a problem hiding this comment.
This FK is configured with onDelete('set null') but the column is made NOT NULL immediately afterwards. If a sender organisation is deleted, Postgres will try to set sender_organisation_id to NULL and the delete will fail. Either keep the column nullable (and handle NULLs in queries) or change the FK action to RESTRICT/CASCADE to match the NOT NULL constraint.
| .object({ | ||
| name: z.string(), | ||
| email: z.string().email(), | ||
| senderOrganisationId: z.string(), |
There was a problem hiding this comment.
The create input schema allows an empty senderOrganisationId (and the client currently sends "" when organisationId is not set). This will always fail the membership check and surfaces as a FORBIDDEN error, which is confusing and not a validation error. Consider validating senderOrganisationId as non-empty (and/or UUID) at the zod layer so this becomes a BAD_REQUEST with field errors.
| senderOrganisationId: z.string(), | |
| senderOrganisationId: z | |
| .string() | |
| .trim() | |
| .min(1, "Sender organisation is required"), |
| createInvitationMutate({ | ||
| senderOrganisationId: senderOrganisationId ?? "", | ||
| organisationId, |
There was a problem hiding this comment.
senderOrganisationId is sent as an empty string when no organisation is selected/loaded. That will deterministically fail server-side (FORBIDDEN) and shows as a generic toast error. Prefer blocking submission until organisationId is available (e.g., disable the submit button / early-return with a user-facing message) and avoid sending placeholder "" values.
| const result = await caller.list({ organisationId: senderOrg.id }); | ||
| expect(Array.isArray(result)).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The updated list tests only assert that the result is an array; they don’t verify that invitations are actually filtered to the requested sender organisation. Add a regression test that creates invitations with two different senderOrganisationIds and asserts that caller.list({organisationId}) only returns those from that organisation.
| test("list only returns invitations for the requested sender organisation", async () => { | |
| const senderOrg = await createSenderOrg(); | |
| const otherSenderOrg = await createSenderOrg(); | |
| const superadmin = await createTestUser(UserRole.Superadmin, senderOrg.id); | |
| const caller = makeCaller(superadmin); | |
| const senderOrgEmailOne = `invite-${uuidv4()}@example.com`; | |
| const senderOrgEmailTwo = `invite-${uuidv4()}@example.com`; | |
| const otherSenderOrgEmail = `invite-${uuidv4()}@example.com`; | |
| await caller.create({ | |
| email: senderOrgEmailOne, | |
| organisationId: senderOrg.id, | |
| }); | |
| await caller.create({ | |
| email: senderOrgEmailTwo, | |
| organisationId: senderOrg.id, | |
| }); | |
| await caller.create({ | |
| email: otherSenderOrgEmail, | |
| organisationId: otherSenderOrg.id, | |
| }); | |
| expect(await findPendingInvitationsByEmail(senderOrgEmailOne)).toHaveLength(1); | |
| expect(await findPendingInvitationsByEmail(senderOrgEmailTwo)).toHaveLength(1); | |
| expect(await findPendingInvitationsByEmail(otherSenderOrgEmail)).toHaveLength(1); | |
| const result = await caller.list({ organisationId: senderOrg.id }); | |
| expect(Array.isArray(result)).toBe(true); | |
| expect(result.map((invitation) => invitation.email)).toEqual( | |
| expect.arrayContaining([senderOrgEmailOne, senderOrgEmailTwo]), | |
| ); | |
| expect(result.map((invitation) => invitation.email)).not.toContain( | |
| otherSenderOrgEmail, | |
| ); | |
| expect(result).toHaveLength(2); | |
| expect( | |
| result.every( | |
| (invitation) => invitation.senderOrganisationId === senderOrg.id, | |
| ), | |
| ).toBe(true); | |
| }); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…onknowledge/ts-mapped into fix/list-invitations-limited-to-org
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Backfill: assign all existing invitations to the admin organisation | ||
| await sql` | ||
| UPDATE invitation | ||
| SET "sender_organisation_id" = ( | ||
| SELECT id FROM organisation WHERE name = 'Common Knowledge' LIMIT 1 | ||
| ) | ||
| WHERE "sender_organisation_id" IS NULL | ||
| `.execute(db); | ||
|
|
There was a problem hiding this comment.
Migration backfill hard-codes an organisation named 'Common Knowledge'. If that row doesn’t exist in an environment, the UPDATE will keep sender_organisation_id NULL and the subsequent setNotNull() will fail, breaking the migration. Consider backfilling from existing invitation.organisation_id (or another deterministic value) and/or explicitly creating/validating the fallback organisation before enforcing NOT NULL.
| // Backfill: assign all existing invitations to the admin organisation | |
| await sql` | |
| UPDATE invitation | |
| SET "sender_organisation_id" = ( | |
| SELECT id FROM organisation WHERE name = 'Common Knowledge' LIMIT 1 | |
| ) | |
| WHERE "sender_organisation_id" IS NULL | |
| `.execute(db); | |
| // Backfill deterministically from the invitation's existing organisation | |
| await sql` | |
| UPDATE invitation | |
| SET "sender_organisation_id" = "organisation_id" | |
| WHERE "sender_organisation_id" IS NULL | |
| `.execute(db); | |
| const remainingNullSenderOrganisationIds = await sql<{ count: string }>` | |
| SELECT COUNT(*)::text AS count | |
| FROM invitation | |
| WHERE "sender_organisation_id" IS NULL | |
| `.execute(db); | |
| if (Number(remainingNullSenderOrganisationIds.rows[0]?.count ?? 0) > 0) { | |
| throw new Error( | |
| "Migration 1774658921005_invitation_sender_organisation failed: some invitation rows do not have an organisation_id to backfill sender_organisation_id", | |
| ); | |
| } |
| z | ||
| .object({ | ||
| name: z.string(), | ||
| email: z.string().email(), | ||
| senderOrganisationId: z.string(), | ||
| organisationId: z.string().nullish(), | ||
| organisationName: z.string().nullish(), | ||
| mapSelections: z |
There was a problem hiding this comment.
The new senderOrganisationId is accepted as an arbitrary string. Because findOrganisationForUser() compares against a UUID column, invalid values (including "") can trigger database errors and end up as INTERNAL_SERVER_ERROR. Prefer validating IDs at the boundary (e.g., zod uuid/min length) so callers get a clear BAD_REQUEST instead of a 500.
| createInvitationMutate({ | ||
| senderOrganisationId: senderOrganisationId ?? "", | ||
| organisationId, | ||
| organisationName, | ||
| email, |
There was a problem hiding this comment.
This passes senderOrganisationId as an empty string when useOrganisations() hasn’t resolved yet. That can cause the server to error (UUID comparison) and surface as an INTERNAL_SERVER_ERROR. Consider blocking submit until senderOrganisationId is available (or showing an explicit error) instead of sending "".
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.