From 3fb7c5fbcf0a4fafad88c0ae1481990c1d456506 Mon Sep 17 00:00:00 2001 From: Jim Hodapp Date: Wed, 5 Aug 2026 15:53:25 -0500 Subject: [PATCH 01/17] feat(members): add an existing user to an organization Adds the UI for the new backend capability: an admin can grant an existing Refactor account membership of another organization, and can remove a member from one organization without touching their account. - UserApi.lookupByEmail hits GET /users?email= and returns 0 or 1 result. A null result deliberately covers both "no such email" and "a real user you may not see"; the copy says only "No user found with that email." so the UI can't be used to enumerate accounts. - AddMemberDialog gains an "Add existing member" tab for admins and super admins, with an explicit Find step, a confirmation card, and a role select whose Member option submits the User role. - MemberCard gains a non-destructive "Remove from organization" action that DELETEs the /role sub-route, kept visually distinct from Delete. - Surfaces the user_already_in_organization, last_organization_admin and user_belongs_to_multiple_organizations conflicts with their own messages. - Drops the dead top-level useUserList/UserApi.list, which called a GET /users route that now requires an email parameter. --- .../add-member-dialog-existing.test.tsx | 219 +++++++++++++ .../members/member-card-remove.test.tsx | 171 ++++++++++ .../ui/members/add-member-button.tsx | 4 + .../ui/members/add-member-dialog.tsx | 294 ++++++++++++++---- src/components/ui/members/member-card.tsx | 92 +++++- .../ui/members/member-container.tsx | 1 + src/lib/api/organization-errors.ts | 25 ++ src/lib/api/organizations/users.ts | 52 +++- src/lib/api/users.ts | 38 +-- src/test-utils/msw-handlers.ts | 20 ++ src/types/user.ts | 12 + 11 files changed, 837 insertions(+), 91 deletions(-) create mode 100644 __tests__/components/members/add-member-dialog-existing.test.tsx create mode 100644 __tests__/components/members/member-card-remove.test.tsx diff --git a/__tests__/components/members/add-member-dialog-existing.test.tsx b/__tests__/components/members/add-member-dialog-existing.test.tsx new file mode 100644 index 00000000..67f89c60 --- /dev/null +++ b/__tests__/components/members/add-member-dialog-existing.test.tsx @@ -0,0 +1,219 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { http, HttpResponse } from "msw"; +import { server } from "@/test-utils/msw-server"; +import { AddMemberDialog } from "@/components/ui/members/add-member-dialog"; +import { Role, type UserRoleState } from "@/types/user"; +import { toast } from "sonner"; + +// ── Module mocks ────────────────────────────────────────────────────────────── + +// A concrete base URL so msw can match the real API calls this dialog makes. +vi.mock("@/site.config", () => ({ + siteConfig: { + env: { + backendServiceURL: "http://localhost:4000", + backendApiVersion: "1.0.0-test", + }, + }, +})); + +vi.mock("sonner", () => ({ + toast: { error: vi.fn(), success: vi.fn() }, +})); + +vi.mock("@/lib/hooks/use-current-organization", () => ({ + useCurrentOrganization: () => ({ currentOrganizationId: "org-1" }), +})); + +vi.mock("@/lib/timezone-utils", () => ({ + getBrowserTimezone: () => "UTC", +})); + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +const adminRole: UserRoleState = { + status: "success", + role: Role.Admin, + hasAccess: true, +}; + +const memberRole: UserRoleState = { + status: "success", + role: Role.User, + hasAccess: true, +}; + +const ADA = { + id: "user-9", + first_name: "Ada", + last_name: "Lovelace", + email: "ada@example.com", +}; + +/** Lookup handler that only answers for Ada's exact email. */ +function lookupHandler(matches: typeof ADA | null) { + return http.get("*/users", ({ request }) => { + const email = new URL(request.url).searchParams.get("email"); + const found = matches && email === matches.email ? [matches] : []; + return HttpResponse.json({ status_code: 200, data: found }); + }); +} + +function renderDialog(currentUserRoleState?: UserRoleState) { + render( + + ); +} + +/** Switches to the "Add existing member" tab, finds Ada, waits for the card. */ +async function findAda(user: ReturnType) { + await user.click(screen.getByRole("tab", { name: "Add existing member" })); + await user.type(screen.getByLabelText("Email"), ADA.email); + await user.click(screen.getByRole("button", { name: "Find" })); + await screen.findByText("Ada Lovelace"); +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe("AddMemberDialog – add existing member gating", () => { + it("does not offer the existing-member mode to a plain member", () => { + renderDialog(memberRole); + + expect( + screen.queryByRole("tab", { name: "Add existing member" }) + ).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Create Member" }) + ).toBeInTheDocument(); + }); + + it("offers the existing-member mode to an org admin, not only a super admin", () => { + renderDialog(adminRole); + + expect( + screen.getByRole("tab", { name: "Add existing member" }) + ).toBeInTheDocument(); + }); +}); + +describe("AddMemberDialog – existing member lookup", () => { + it("reports no match without hinting at visibility, and keeps the add button disabled", async () => { + server.use(lookupHandler(null)); + const user = userEvent.setup(); + renderDialog(adminRole); + + await user.click(screen.getByRole("tab", { name: "Add existing member" })); + await user.type(screen.getByLabelText("Email"), "nobody@example.com"); + await user.click(screen.getByRole("button", { name: "Find" })); + + expect( + await screen.findByText("No user found with that email.") + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Add to organization" }) + ).toBeDisabled(); + }); + + it("shows the returned name and email on a match", async () => { + server.use(lookupHandler(ADA)); + const user = userEvent.setup(); + renderDialog(adminRole); + + await findAda(user); + + expect(screen.getByText("Ada Lovelace")).toBeInTheDocument(); + expect(screen.getByText(ADA.email)).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Add to organization" }) + ).toBeEnabled(); + }); +}); + +describe("AddMemberDialog – attaching an existing member", () => { + /** Captures every POST to the membership sub-route. */ + function captureAttach() { + const calls: { url: string; body: unknown }[] = []; + server.use( + lookupHandler(ADA), + http.post( + "*/organizations/:organizationId/users/:userId/role", + async ({ request, params }) => { + calls.push({ url: request.url, body: await request.json() }); + return HttpResponse.json({ + status_code: 200, + data: { id: params.userId }, + }); + } + ) + ); + return calls; + } + + it("posts the selected role to the member's /role sub-route", async () => { + const calls = captureAttach(); + const user = userEvent.setup(); + renderDialog(adminRole); + + await findAda(user); + await user.click(screen.getByRole("combobox")); + await user.click(await screen.findByRole("option", { name: "Admin" })); + await user.click(screen.getByRole("button", { name: "Add to organization" })); + + await waitFor(() => expect(calls).toHaveLength(1)); + expect(calls[0].url).toBe( + `http://localhost:4000/organizations/org-1/users/${ADA.id}/role` + ); + expect(calls[0].body).toEqual({ role: "Admin" }); + }); + + it("defaults to the Member option, which submits the User role", async () => { + const calls = captureAttach(); + const user = userEvent.setup(); + renderDialog(adminRole); + + await findAda(user); + expect(screen.getByRole("combobox")).toHaveTextContent("Member"); + await user.click(screen.getByRole("button", { name: "Add to organization" })); + + await waitFor(() => expect(calls).toHaveLength(1)); + expect(calls[0].body).toEqual({ role: "User" }); + }); + + it("surfaces the already-a-member conflict rather than a generic error", async () => { + server.use( + lookupHandler(ADA), + http.post("*/organizations/:organizationId/users/:userId/role", () => + HttpResponse.json( + { + error: "user_already_in_organization", + message: "This user is already a member of this organization.", + }, + { status: 409 } + ) + ) + ); + const user = userEvent.setup(); + renderDialog(adminRole); + + await findAda(user); + await user.click(screen.getByRole("button", { name: "Add to organization" })); + + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + "This user is already a member of this organization." + ) + ); + }); +}); diff --git a/__tests__/components/members/member-card-remove.test.tsx b/__tests__/components/members/member-card-remove.test.tsx new file mode 100644 index 00000000..28dd289d --- /dev/null +++ b/__tests__/components/members/member-card-remove.test.tsx @@ -0,0 +1,171 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { http, HttpResponse } from "msw"; +import { server } from "@/test-utils/msw-server"; +import { MemberCard } from "@/components/ui/members/member-card"; +import { Role, type UserRoleState } from "@/types/user"; +import { toast } from "sonner"; +import { createMockUser } from "../../test-utils"; + +// ── Module mocks ────────────────────────────────────────────────────────────── + +// A concrete base URL so msw can match the real API calls this card makes. +vi.mock("@/site.config", () => ({ + siteConfig: { + env: { + backendServiceURL: "http://localhost:4000", + backendApiVersion: "1.0.0-test", + }, + }, +})); + +vi.mock("sonner", () => ({ + toast: { error: vi.fn(), success: vi.fn() }, +})); + +vi.mock("@/lib/hooks/use-current-organization", () => ({ + useCurrentOrganization: () => ({ currentOrganizationId: "org-1" }), +})); + +const mockAuthStore = vi.fn(); +vi.mock("@/lib/providers/auth-store-provider", () => ({ + AuthStoreProvider: ({ children }: { children: React.ReactNode }) => children, + useAuthStore: (selector: (state: unknown) => unknown) => + selector(mockAuthStore()), +})); + +vi.mock("@/lib/api/coaching-relationships", () => ({ + useCoachingRelationshipMutation: () => ({ createNested: vi.fn() }), +})); + +// Role derivation is pure and covered elsewhere; stub it so the card renders +// without needing full relationship fixtures. +vi.mock("@/lib/utils/user-roles", () => ({ + getUserDisplayRoles: () => [], + getUserCoaches: () => [], +})); + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +const adminRole: UserRoleState = { + status: "success", + role: Role.Admin, + hasAccess: true, +}; + +function renderCard() { + const cardUser = createMockUser({ + id: "user-1", + first_name: "Ada", + last_name: "Lovelace", + invite_status: null, + }); + render( + + ); +} + +/** + * Records both the membership DELETE and the account DELETE, so a regression + * that removes the whole account instead of the membership is visible. + */ +function captureDeletes(membershipResponse: () => Response) { + const roleCalls: string[] = []; + const accountCalls: string[] = []; + server.use( + http.delete( + "*/organizations/:organizationId/users/:userId/role", + ({ request }) => { + roleCalls.push(request.url); + return membershipResponse(); + } + ), + http.delete("*/organizations/:organizationId/users/:userId", ({ request }) => { + accountCalls.push(request.url); + return HttpResponse.json({ status_code: 200, data: null }); + }) + ); + return { roleCalls, accountCalls }; +} + +async function openRemoveDialog(user: ReturnType) { + await user.click(screen.getByRole("button")); // row actions menu (icon-only) + await user.click(await screen.findByText("Remove from organization")); +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "error").mockImplementation(() => {}); + vi.spyOn(console, "log").mockImplementation(() => {}); + mockAuthStore.mockReturnValue({ isACoach: true, userSession: { id: "me" } }); +}); + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe("MemberCard – remove from organization", () => { + it("offers the action to an admin viewing another member", async () => { + const user = userEvent.setup(); + renderCard(); + + await user.click(screen.getByRole("button")); + + expect( + await screen.findByText("Remove from organization") + ).toBeInTheDocument(); + }); + + it("deletes the membership sub-route, never the member's account", async () => { + const { roleCalls, accountCalls } = captureDeletes(() => + HttpResponse.json({ status_code: 200, data: null }) + ); + const user = userEvent.setup(); + renderCard(); + + await openRemoveDialog(user); + expect( + await screen.findByText( + /Remove them from this organization only\. Their account and any other organizations are unaffected\./ + ) + ).toBeInTheDocument(); + await user.click(screen.getByRole("button", { name: "Remove" })); + + await waitFor(() => expect(roleCalls).toHaveLength(1)); + expect(roleCalls[0]).toBe( + "http://localhost:4000/organizations/org-1/users/user-1/role" + ); + expect(roleCalls[0].endsWith("/role")).toBe(true); + expect(accountCalls).toEqual([]); + }); + + it("surfaces the last-admin conflict rather than a generic error", async () => { + captureDeletes(() => + HttpResponse.json( + { + error: "last_organization_admin", + message: + "This user is the only admin of this organization. Assign another admin before removing them.", + }, + { status: 409 } + ) + ); + const user = userEvent.setup(); + renderCard(); + + await openRemoveDialog(user); + await user.click(screen.getByRole("button", { name: "Remove" })); + + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + "This user is the only admin of this organization. Assign another admin before removing them." + ) + ); + }); +}); diff --git a/src/components/ui/members/add-member-button.tsx b/src/components/ui/members/add-member-button.tsx index 06362069..10b370e1 100644 --- a/src/components/ui/members/add-member-button.tsx +++ b/src/components/ui/members/add-member-button.tsx @@ -4,16 +4,19 @@ import { useEffect, useState } from "react"; import { Button } from "@/components/ui/button"; import { Plus } from "lucide-react"; import { AddMemberDialog } from "./add-member-dialog"; +import { UserRoleState } from "@/types/user"; interface AddMemberButtonProps { onMemberAdded: () => void; /// Force the AddMemberDialog to open openAddMemberDialog: boolean; + currentUserRoleState: UserRoleState; } export function AddMemberButton({ onMemberAdded, openAddMemberDialog, + currentUserRoleState, }: AddMemberButtonProps) { const [open, setOpen] = useState(false); @@ -31,6 +34,7 @@ export function AddMemberButton({ open={open} onOpenChange={setOpen} onMemberAdded={onMemberAdded} + currentUserRoleState={currentUserRoleState} /> ); diff --git a/src/components/ui/members/add-member-dialog.tsx b/src/components/ui/members/add-member-dialog.tsx index 28b359f1..f11e71a5 100644 --- a/src/components/ui/members/add-member-dialog.tsx +++ b/src/components/ui/members/add-member-dialog.tsx @@ -14,9 +14,27 @@ import { } from "@/components/ui/dialog"; import { Label } from "@/components/ui/label"; import { Input } from "@/components/ui/input"; +import { + Select, + SelectTrigger, + SelectValue, + SelectContent, + SelectItem, +} from "@/components/ui/select"; +import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; import { useUserMutation } from "@/lib/api/organizations/users"; -import { organizationArchivedMessage } from "@/lib/api/organization-errors"; -import { NewUser } from "@/types/user"; +import { UserApi } from "@/lib/api/users"; +import { + organizationArchivedMessage, + userAlreadyInOrganizationMessage, +} from "@/lib/api/organization-errors"; +import { + NewUser, + Role, + UserLookupResult, + UserRoleState, + isAdminOrSuperAdmin, +} from "@/types/user"; import { useCurrentOrganization } from "@/lib/hooks/use-current-organization"; import { toast } from "sonner"; import { getBrowserTimezone } from "@/lib/timezone-utils"; @@ -26,16 +44,19 @@ interface AddMemberDialogProps { open: boolean; onOpenChange: (open: boolean) => void; onMemberAdded: () => void; + /// Omitted for callers that only offer creating a brand new member + currentUserRoleState?: UserRoleState; } export function AddMemberDialog({ open, onOpenChange, onMemberAdded, + currentUserRoleState, }: AddMemberDialogProps) { const { currentOrganizationId } = useCurrentOrganization(); - const { createNested: createUserNested } = useUserMutation( + const { createNested: createUserNested, attachExisting } = useUserMutation( currentOrganizationId ); const [formData, setFormData] = useState({ @@ -44,6 +65,16 @@ export function AddMemberDialog({ displayName: "", email: "", }); + const [lookupEmail, setLookupEmail] = useState(""); + const [foundUser, setFoundUser] = useState(null); + const [lookupMessage, setLookupMessage] = useState(null); + const [isLookingUp, setIsLookingUp] = useState(false); + const [existingRole, setExistingRole] = useState(Role.User); + const [isAdding, setIsAdding] = useState(false); + + // Org admins get this too, not just super admins + const canAddExisting = + !!currentUserRoleState && isAdminOrSuperAdmin(currentUserRoleState); const handleInputChange = (e: React.ChangeEvent) => { const { name, value } = e.target; @@ -86,70 +117,215 @@ export function AddMemberDialog({ } }; + const handleFind = async () => { + setFoundUser(null); + setLookupMessage(null); + setIsLookingUp(true); + + try { + const result = await UserApi.lookupByEmail(lookupEmail); + // A null result also covers a real user outside this admin's scope. The + // backend makes those cases indistinguishable, so the copy must too. + if (result) { + setFoundUser(result); + } else { + setLookupMessage("No user found with that email."); + } + } catch (error) { + console.error("Error looking up user:", error); + setLookupMessage( + isForbiddenError(error) + ? PERMISSION_DENIED_MESSAGE + : "There was an error looking up that email." + ); + } finally { + setIsLookingUp(false); + } + }; + + const resetLookup = () => { + setLookupEmail(""); + setFoundUser(null); + setLookupMessage(null); + setExistingRole(Role.User); + }; + + const handleAddExisting = async () => { + if (!foundUser) return; + setIsAdding(true); + + try { + await attachExisting(currentOrganizationId, foundUser.id, existingRole); + onMemberAdded(); + toast.success( + `${foundUser.first_name} ${foundUser.last_name} added to this organization` + ); + resetLookup(); + onOpenChange(false); + } catch (error) { + console.error("Error adding existing user:", error); + toast.error( + userAlreadyInOrganizationMessage(error) ?? + organizationArchivedMessage(error) ?? + (isForbiddenError(error) + ? PERMISSION_DENIED_MESSAGE + : "There was an error adding the member") + ); + } finally { + setIsAdding(false); + } + }; + + const createMemberForm = ( +
+
+
+ + +
+
+ + +
+
+ + +
+
+ + +
+
+
+ + + +
+
+ ); + + const addExistingForm = ( +
+

+ This person already has a Refactor account. Adding them here gives them + access to this organization using their existing profile. +

+
+ +
+ setLookupEmail(e.target.value)} + placeholder="Enter email address" + /> + +
+
+ {lookupMessage && ( +

{lookupMessage}

+ )} + {foundUser && ( +
+

+ {foundUser.first_name} {foundUser.last_name} +

+

{foundUser.email}

+
+ )} +
+ + +
+ + + +
+ ); + return ( Add New Member - Create a new member account. They'll receive an email with a - link to set up their password. + {canAddExisting + ? "Create a new member account, or add someone who already has a Refactor account." + : "Create a new member account. They'll receive an email with a link to set up their password."} -
-
-
- - -
-
- - -
-
- - -
-
- - -
-
-
- - - -
-
+ {canAddExisting ? ( + + + Create new member + Add existing member + + + {createMemberForm} + + + {addExistingForm} + + + ) : ( + createMemberForm + )} ); diff --git a/src/components/ui/members/member-card.tsx b/src/components/ui/members/member-card.tsx index 993a7f67..5c86ed6f 100644 --- a/src/components/ui/members/member-card.tsx +++ b/src/components/ui/members/member-card.tsx @@ -12,7 +12,17 @@ import { DropdownMenuItem, DropdownMenuSeparator, } from "@/components/ui/dropdown-menu"; -import { MoreHorizontal, Send, Trash2 } from "lucide-react"; +import { MoreHorizontal, Send, Trash2, UserMinus } from "lucide-react"; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@/components/ui/alert-dialog"; import { Dialog, DialogContent, @@ -39,7 +49,11 @@ import { } from "@/types/user"; import { RelationshipRole } from "@/types/relationship-role"; import { useCoachingRelationshipMutation } from "@/lib/api/coaching-relationships"; -import { organizationArchivedMessage } from "@/lib/api/organization-errors"; +import { + lastOrganizationAdminMessage, + organizationArchivedMessage, + userBelongsToMultipleOrganizationsMessage, +} from "@/lib/api/organization-errors"; import { toast } from "sonner"; interface MemberCardProps { @@ -76,7 +90,7 @@ export function MemberCard({ // Get coaches for this user const coaches = getUserCoaches(userId, userRelationships); - const { error: deleteError, deleteNested: deleteUser } = useUserMutation( + const { deleteNested: deleteUser, removeFromOrganization } = useUserMutation( currentOrganizationId ); const { createNested: createRelationship } = @@ -94,17 +108,43 @@ export function MemberCard({ if (!confirm("Are you sure you want to delete this member?")) { return; } - await deleteUser(currentOrganizationId, userId); + + try { + await deleteUser(currentOrganizationId, userId); + toast.success("Member deleted successfully"); + } catch (error) { + console.error("Error deleting member:", error); + toast.error( + userBelongsToMultipleOrganizationsMessage(error) ?? + organizationArchivedMessage(error) ?? + (isForbiddenError(error) + ? PERMISSION_DENIED_MESSAGE + : "Error deleting member") + ); + } onRefresh(); + }; + + const handleRemoveFromOrganization = async () => { + setIsRemoving(true); - if (deleteError) { - console.error("Error deleting member:", deleteError); - toast.error("Error deleting member"); + try { + await removeFromOrganization(currentOrganizationId, userId); + toast.success(`${firstName} ${lastName} removed from this organization`); + setRemoveDialogOpen(false); onRefresh(); - return; + } catch (error) { + console.error("Error removing member from organization:", error); + toast.error( + lastOrganizationAdminMessage(error) ?? + organizationArchivedMessage(error) ?? + (isForbiddenError(error) + ? PERMISSION_DENIED_MESSAGE + : "Error removing member from this organization") + ); + } finally { + setIsRemoving(false); } - toast.success("Member deleted successfully"); - onRefresh(); }; const handleResendInvite = async () => { @@ -155,6 +195,8 @@ export function MemberCard({ const [assignMode, setAssignMode] = useState(RelationshipRole.Coach); const [selectedMember, setSelectedMember] = useState(null); const [assignedMember, setAssignedMember] = useState(null); + const [removeDialogOpen, setRemoveDialogOpen] = useState(false); + const [isRemoving, setIsRemoving] = useState(false); const handleCreateCoachingRelationship = async () => { if (!selectedMember || !assignedMember) return; @@ -267,6 +309,9 @@ export function MemberCard({ {canDeleteUser && ( <> {userId !== currentUserId && } + setRemoveDialogOpen(true)}> + Remove from organization + )} + {/* Remove from organization confirmation */} + + + + + Remove {firstName} {lastName} from this organization + + + Remove them from this organization only. Their account and any + other organizations are unaffected. + + + + Cancel + { + e.preventDefault(); + handleRemoveFromOrganization(); + }} + disabled={isRemoving} + > + {isRemoving ? "Removing..." : "Remove"} + + + + + {/* Assign Coach/Coachee Modal */} diff --git a/src/components/ui/members/member-container.tsx b/src/components/ui/members/member-container.tsx index e7875f0c..ffcdf05a 100644 --- a/src/components/ui/members/member-container.tsx +++ b/src/components/ui/members/member-container.tsx @@ -59,6 +59,7 @@ export function MemberContainer({ )} diff --git a/src/lib/api/organization-errors.ts b/src/lib/api/organization-errors.ts index 7fa0cc3c..cc4b034e 100644 --- a/src/lib/api/organization-errors.ts +++ b/src/lib/api/organization-errors.ts @@ -38,6 +38,31 @@ export const organizationArchivedMessage = (error: unknown): string | null => "This organization is archived and can't accept new changes." ); +export const userAlreadyInOrganizationMessage = ( + error: unknown +): string | null => + orgErrorMessage( + error, + "user_already_in_organization", + "This user is already a member of this organization." + ); + +export const lastOrganizationAdminMessage = (error: unknown): string | null => + orgErrorMessage( + error, + "last_organization_admin", + "This user is the only admin of this organization. Assign another admin before removing them." + ); + +export const userBelongsToMultipleOrganizationsMessage = ( + error: unknown +): string | null => + orgErrorMessage( + error, + "user_belongs_to_multiple_organizations", + "This user belongs to other organizations. Remove them from this organization instead of deleting their account." + ); + /** Longest organization name the backend accepts (characters, trimmed). */ export const ORGANIZATION_NAME_MAX_LENGTH = 255; diff --git a/src/lib/api/organizations/users.ts b/src/lib/api/organizations/users.ts index 05d645ea..880adf62 100644 --- a/src/lib/api/organizations/users.ts +++ b/src/lib/api/organizations/users.ts @@ -1,8 +1,9 @@ // Interacts with the organizations/{organizationId}/users endpoints +import { useSWRConfig } from "swr"; import { Id } from "@/types/general"; import { EntityApi } from "../entity-api"; -import { User, NewUser } from "@/types/user"; +import { User, NewUser, Role } from "@/types/user"; import { ORGANIZATIONS_BASEURL } from "../organizations"; const ORGANIZATIONS_USERS_BASEURL = (organizationId: Id) => @@ -78,6 +79,33 @@ export const UserApi = { {} ); }, + + /** + * Grants an existing user membership of this organization with the given role. + * The account itself is shared, not copied. + */ + attachExisting: async ( + organizationId: Id, + userId: Id, + role: Role + ): Promise => + EntityApi.createFn<{ role: Role }, User>( + `${ORGANIZATIONS_USERS_BASEURL(organizationId)}/${userId}/role`, + { role } + ), + + /** + * Removes a user's membership of this organization only. Their account and + * any other organizations are left untouched. + */ + removeFromOrganization: async ( + organizationId: Id, + userId: Id + ): Promise => { + await EntityApi.deleteFn( + `${ORGANIZATIONS_USERS_BASEURL(organizationId)}/${userId}/role` + ); + }, }; /** @@ -101,10 +129,12 @@ export const useUserList = (organizationId: Id) => { /** * Hook for user mutations. - * Provides methods to create, update, and delete users. + * Provides methods to create, update, and delete users, plus the membership + * actions (attach an existing user, remove one) that sit outside standard CRUD. */ export const useUserMutation = (organizationId: Id) => { - return EntityApi.useEntityMutation( + const { mutate } = useSWRConfig(); + const mutation = EntityApi.useEntityMutation( ORGANIZATIONS_USERS_BASEURL(organizationId), { create: UserApi.create, @@ -114,4 +144,20 @@ export const useUserMutation = (organizationId: Id) => { deleteNested: UserApi.deleteNested, } ); + + const invalidate = (id: Id) => + EntityApi.invalidateEntityCache(mutate, ORGANIZATIONS_USERS_BASEURL(id)); + + return { + ...mutation, + attachExisting: async (orgId: Id, userId: Id, role: Role) => { + const user = await UserApi.attachExisting(orgId, userId, role); + invalidate(orgId); + return user; + }, + removeFromOrganization: async (orgId: Id, userId: Id) => { + await UserApi.removeFromOrganization(orgId, userId); + invalidate(orgId); + }, + }; }; diff --git a/src/lib/api/users.ts b/src/lib/api/users.ts index 5d166aef..74a5db8f 100644 --- a/src/lib/api/users.ts +++ b/src/lib/api/users.ts @@ -3,7 +3,13 @@ import { siteConfig } from "@/site.config"; import { Id } from "@/types/general"; import { EntityApi } from "./entity-api"; -import { User, NewUserPassword, defaultUser } from "@/types/user"; +import { + User, + NewUserPassword, + UserLookupResult, + defaultUser, +} from "@/types/user"; +import { buildQueryString } from "./query-params"; export const USERS_BASEURL: string = `${siteConfig.env.backendServiceURL}/users`; @@ -12,10 +18,19 @@ export const USERS_BASEURL: string = `${siteConfig.env.backendServiceURL}/users` */ export const UserApi = { /** - * Fetches a list of users. + * Looks up a single user by exact (case-insensitive) email address. + * + * @param email The email address to look for + * @returns The matching user, or null when there is no match or the caller + * may not see them. The backend makes those two cases indistinguishable so + * the endpoint can't be used to enumerate accounts. */ - list: async (): Promise => - EntityApi.listFn(USERS_BASEURL, {}), + lookupByEmail: async (email: string): Promise => { + const results = await EntityApi.getFn( + `${USERS_BASEURL}${buildQueryString({ email })}` + ); + return results[0] ?? null; + }, /** * Fetches a single user by ID. @@ -60,21 +75,6 @@ export const UserApi = { }, }; -/** - * Hook for fetching a list of users. - */ -export const useUserList = () => { - const { entities, isLoading, isError, refresh } = - EntityApi.useEntityList(USERS_BASEURL, () => UserApi.list()); - - return { - users: entities, - isLoading, - isError, - refresh, - }; -}; - /** * Hook for fetching a single user. */ diff --git a/src/test-utils/msw-handlers.ts b/src/test-utils/msw-handlers.ts index 621244f1..cfaf6c9a 100644 --- a/src/test-utils/msw-handlers.ts +++ b/src/test-utils/msw-handlers.ts @@ -60,4 +60,24 @@ export const handlers = [ return new HttpResponse(null, { status: 200 }); }), + // User lookup by exact email: 0 or 1 results, never a 404. Tests that care + // about a match override this with server.use(). + http.get("*/users", ({ request }) => { + const email = new URL(request.url).searchParams.get("email"); + if (!email) { + return new HttpResponse(null, { status: 400 }); + } + return HttpResponse.json({ status_code: 200, data: [] }); + }), + + // Grant an existing user membership of an organization + http.post("*/organizations/:organizationId/users/:userId/role", () => { + return HttpResponse.json({ status_code: 200, data: { id: "user-1" } }); + }), + + // Remove a user's membership of an organization (account untouched) + http.delete("*/organizations/:organizationId/users/:userId/role", () => { + return HttpResponse.json({ status_code: 200, data: null }); + }), + ]; diff --git a/src/types/user.ts b/src/types/user.ts index 3e83be8b..14ca82bf 100644 --- a/src/types/user.ts +++ b/src/types/user.ts @@ -43,6 +43,18 @@ export interface User { invite_status: InviteStatus | null; } +/** + * Narrow projection returned by the email lookup. Deliberately not `User`: the + * server sends only these fields, so reaching for roles or timezone on a lookup + * result fails at compile time instead of rendering undefined. + */ +export interface UserLookupResult { + id: Id; + first_name: string; + last_name: string; + email: string; +} + export interface NewUser { first_name: string; last_name: string; From ea5354ebbc725cb1b49d18e9b29b49887cd7ebff Mon Sep 17 00:00:00 2001 From: Jim Hodapp Date: Thu, 6 Aug 2026 12:29:03 -0500 Subject: [PATCH 02/17] fix(members): let admins discard a found user before adding them Adds a Clear button to the lookup result card, so finding the wrong person is recoverable without closing the dialog. Also invalidates the match when the email field is edited. Previously the found user survived an edit, so an admin who searched one address, then retyped another without pressing Find, would add the first person while the field displayed the second. --- .../add-member-dialog-existing.test.tsx | 33 +++++++++++++++++++ .../ui/members/add-member-dialog.tsx | 32 ++++++++++++++---- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/__tests__/components/members/add-member-dialog-existing.test.tsx b/__tests__/components/members/add-member-dialog-existing.test.tsx index 67f89c60..4a4d9098 100644 --- a/__tests__/components/members/add-member-dialog-existing.test.tsx +++ b/__tests__/components/members/add-member-dialog-existing.test.tsx @@ -141,6 +141,39 @@ describe("AddMemberDialog – existing member lookup", () => { }); }); +describe("AddMemberDialog – discarding a found user", () => { + it("clears the found user when Clear is pressed", async () => { + server.use(lookupHandler(ADA)); + const user = userEvent.setup(); + renderDialog(adminRole); + + await findAda(user); + await user.click(screen.getByRole("button", { name: /^Clear / })); + + expect(screen.queryByText("Ada Lovelace")).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Add to organization" }) + ).toBeDisabled(); + expect(screen.getByLabelText("Email")).toHaveValue(""); + }); + + /// Editing the email must invalidate the match it produced, or the add button + /// acts on a stale selection while the field shows a different address. + it("discards the found user when the email is edited after finding", async () => { + server.use(lookupHandler(ADA)); + const user = userEvent.setup(); + renderDialog(adminRole); + + await findAda(user); + await user.type(screen.getByLabelText("Email"), "x"); + + expect(screen.queryByText("Ada Lovelace")).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Add to organization" }) + ).toBeDisabled(); + }); +}); + describe("AddMemberDialog – attaching an existing member", () => { /** Captures every POST to the membership sub-route. */ function captureAttach() { diff --git a/src/components/ui/members/add-member-dialog.tsx b/src/components/ui/members/add-member-dialog.tsx index f11e71a5..ac85ff2c 100644 --- a/src/components/ui/members/add-member-dialog.tsx +++ b/src/components/ui/members/add-member-dialog.tsx @@ -150,6 +150,15 @@ export function AddMemberDialog({ setExistingRole(Role.User); }; + // A found user belongs to the email that produced it, so editing the field + // invalidates it. Without this the Add button can act on a stale selection + // while the field shows a different address. + const handleLookupEmailChange = (value: string) => { + setLookupEmail(value); + setFoundUser(null); + setLookupMessage(null); + }; + const handleAddExisting = async () => { if (!foundUser) return; setIsAdding(true); @@ -247,7 +256,7 @@ export function AddMemberDialog({ name="lookupEmail" type="email" value={lookupEmail} - onChange={(e) => setLookupEmail(e.target.value)} + onChange={(e) => handleLookupEmailChange(e.target.value)} placeholder="Enter email address" /> )}
From 7f89f652578a0a6add4f86476bf6699e3b91ab1d Mon Sep 17 00:00:00 2001 From: Jim Hodapp Date: Thu, 6 Aug 2026 12:37:58 -0500 Subject: [PATCH 03/17] feat(members): pre-assign a coach when adding a member Both the create-new and add-existing flows now offer an optional coach picker listing the organization's current members. The selection posts a coaching relationship after the member exists, reusing the existing POST /organizations/{id}/coaching_relationships endpoint. The two calls are not atomic, so a failed assignment reports the member as added with guidance to assign a coach from the member list. The reverse failure cannot happen, and a coachless member is recoverable from the UI. A found user is kept off their own coach list. --- .../add-member-dialog-existing.test.tsx | 122 +++++++++++++++++- .../ui/members/add-member-button.tsx | 6 +- .../ui/members/add-member-dialog.tsx | 82 +++++++++++- .../ui/members/member-container.tsx | 1 + 4 files changed, 201 insertions(+), 10 deletions(-) diff --git a/__tests__/components/members/add-member-dialog-existing.test.tsx b/__tests__/components/members/add-member-dialog-existing.test.tsx index 4a4d9098..cb0d5489 100644 --- a/__tests__/components/members/add-member-dialog-existing.test.tsx +++ b/__tests__/components/members/add-member-dialog-existing.test.tsx @@ -4,7 +4,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { http, HttpResponse } from "msw"; import { server } from "@/test-utils/msw-server"; import { AddMemberDialog } from "@/components/ui/members/add-member-dialog"; -import { Role, type UserRoleState } from "@/types/user"; +import { Role, type User, type UserRoleState } from "@/types/user"; import { toast } from "sonner"; // ── Module mocks ────────────────────────────────────────────────────────────── @@ -20,7 +20,7 @@ vi.mock("@/site.config", () => ({ })); vi.mock("sonner", () => ({ - toast: { error: vi.fn(), success: vi.fn() }, + toast: { error: vi.fn(), success: vi.fn(), warning: vi.fn() }, })); vi.mock("@/lib/hooks/use-current-organization", () => ({ @@ -61,13 +61,24 @@ function lookupHandler(matches: typeof ADA | null) { }); } -function renderDialog(currentUserRoleState?: UserRoleState) { +/** Existing members offered as coach candidates. */ +const GRACE = { + id: "user-2", + first_name: "Grace", + last_name: "Hopper", +} as unknown as User; + +function renderDialog( + currentUserRoleState?: UserRoleState, + organizationMembers?: User[] +) { render( ); } @@ -141,6 +152,111 @@ describe("AddMemberDialog – existing member lookup", () => { }); }); +describe("AddMemberDialog – pre-assigning a coach", () => { + /** Captures relationship POSTs. `relationshipFails` makes them 500. */ + function captureRelationships(relationshipFails = false) { + const calls: unknown[] = []; + server.use( + lookupHandler(ADA), + http.post("*/organizations/:organizationId/users/:userId/role", () => + HttpResponse.json({ status_code: 200, data: { id: ADA.id } }) + ), + http.post("*/organizations/:organizationId/users", () => + HttpResponse.json({ status_code: 201, data: { id: "user-new" } }) + ), + http.post( + "*/organizations/:organizationId/coaching_relationships", + async ({ request }) => { + calls.push(await request.json()); + return relationshipFails + ? HttpResponse.json({ error: "boom" }, { status: 500 }) + : HttpResponse.json({ status_code: 201, data: { id: "rel-1" } }); + } + ) + ); + return calls; + } + + async function pickCoach(user: ReturnType) { + await user.click(screen.getByLabelText("Coach (optional)")); + await user.click(await screen.findByRole("option", { name: "Grace Hopper" })); + } + + it("assigns the chosen coach to a newly created member", async () => { + const calls = captureRelationships(); + const user = userEvent.setup(); + renderDialog(adminRole, [GRACE]); + + await user.type(screen.getByLabelText("First Name"), "Ada"); + await user.type(screen.getByLabelText("Last Name"), "Lovelace"); + await user.type(screen.getByLabelText("Display Name"), "Ada"); + await user.type(screen.getByLabelText("Email"), "new@example.com"); + await pickCoach(user); + await user.click(screen.getByRole("button", { name: "Create Member" })); + + await waitFor(() => expect(calls).toHaveLength(1)); + expect(calls[0]).toEqual({ coach_id: GRACE.id, coachee_id: "user-new" }); + }); + + it("assigns the chosen coach to an attached existing member", async () => { + const calls = captureRelationships(); + const user = userEvent.setup(); + renderDialog(adminRole, [GRACE]); + + await findAda(user); + await pickCoach(user); + await user.click(screen.getByRole("button", { name: "Add to organization" })); + + await waitFor(() => expect(calls).toHaveLength(1)); + expect(calls[0]).toEqual({ coach_id: GRACE.id, coachee_id: ADA.id }); + }); + + it("does not create a relationship when no coach is chosen", async () => { + const calls = captureRelationships(); + const user = userEvent.setup(); + renderDialog(adminRole, [GRACE]); + + await findAda(user); + await user.click(screen.getByRole("button", { name: "Add to organization" })); + + await waitFor(() => expect(toast.success).toHaveBeenCalled()); + expect(calls).toHaveLength(0); + }); + + /// The member is already added at this point, so the add must not read as failed. + it("still reports the member as added when the coach assignment fails", async () => { + captureRelationships(true); + const user = userEvent.setup(); + renderDialog(adminRole, [GRACE]); + + await findAda(user); + await pickCoach(user); + await user.click(screen.getByRole("button", { name: "Add to organization" })); + + await waitFor(() => expect(toast.warning).toHaveBeenCalled()); + expect(toast.warning).toHaveBeenCalledWith( + expect.stringContaining("Assign one from the member list") + ); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it("keeps the found user off their own coach list", async () => { + server.use(lookupHandler(ADA)); + const user = userEvent.setup(); + renderDialog(adminRole, [GRACE, ADA as unknown as User]); + + await findAda(user); + await user.click(screen.getByLabelText("Coach (optional)")); + + expect( + await screen.findByRole("option", { name: "Grace Hopper" }) + ).toBeInTheDocument(); + expect( + screen.queryByRole("option", { name: "Ada Lovelace" }) + ).not.toBeInTheDocument(); + }); +}); + describe("AddMemberDialog – discarding a found user", () => { it("clears the found user when Clear is pressed", async () => { server.use(lookupHandler(ADA)); diff --git a/src/components/ui/members/add-member-button.tsx b/src/components/ui/members/add-member-button.tsx index 10b370e1..7a3e9f8c 100644 --- a/src/components/ui/members/add-member-button.tsx +++ b/src/components/ui/members/add-member-button.tsx @@ -4,19 +4,22 @@ import { useEffect, useState } from "react"; import { Button } from "@/components/ui/button"; import { Plus } from "lucide-react"; import { AddMemberDialog } from "./add-member-dialog"; -import { UserRoleState } from "@/types/user"; +import { User, UserRoleState } from "@/types/user"; interface AddMemberButtonProps { onMemberAdded: () => void; /// Force the AddMemberDialog to open openAddMemberDialog: boolean; currentUserRoleState: UserRoleState; + /// Candidates offered when pre-assigning a coach + organizationMembers?: User[]; } export function AddMemberButton({ onMemberAdded, openAddMemberDialog, currentUserRoleState, + organizationMembers, }: AddMemberButtonProps) { const [open, setOpen] = useState(false); @@ -35,6 +38,7 @@ export function AddMemberButton({ onOpenChange={setOpen} onMemberAdded={onMemberAdded} currentUserRoleState={currentUserRoleState} + organizationMembers={organizationMembers} /> ); diff --git a/src/components/ui/members/add-member-dialog.tsx b/src/components/ui/members/add-member-dialog.tsx index ac85ff2c..457bf929 100644 --- a/src/components/ui/members/add-member-dialog.tsx +++ b/src/components/ui/members/add-member-dialog.tsx @@ -23,6 +23,7 @@ import { } from "@/components/ui/select"; import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; import { useUserMutation } from "@/lib/api/organizations/users"; +import { useCoachingRelationshipMutation } from "@/lib/api/coaching-relationships"; import { UserApi } from "@/lib/api/users"; import { organizationArchivedMessage, @@ -31,6 +32,7 @@ import { import { NewUser, Role, + User, UserLookupResult, UserRoleState, isAdminOrSuperAdmin, @@ -38,7 +40,10 @@ import { import { useCurrentOrganization } from "@/lib/hooks/use-current-organization"; import { toast } from "sonner"; import { getBrowserTimezone } from "@/lib/timezone-utils"; -import { isForbiddenError, PERMISSION_DENIED_MESSAGE } from "@/types/general"; +import { Id, isForbiddenError, PERMISSION_DENIED_MESSAGE } from "@/types/general"; + +/// Sentinel for the "no coach" option, since Select cannot hold an empty value. +const NO_COACH = "none"; interface AddMemberDialogProps { open: boolean; @@ -46,6 +51,8 @@ interface AddMemberDialogProps { onMemberAdded: () => void; /// Omitted for callers that only offer creating a brand new member currentUserRoleState?: UserRoleState; + /// Candidates offered when pre-assigning a coach. Omitted hides the field. + organizationMembers?: User[]; } export function AddMemberDialog({ @@ -53,12 +60,16 @@ export function AddMemberDialog({ onOpenChange, onMemberAdded, currentUserRoleState, + organizationMembers, }: AddMemberDialogProps) { const { currentOrganizationId } = useCurrentOrganization(); const { createNested: createUserNested, attachExisting } = useUserMutation( currentOrganizationId ); + const { createNested: createRelationship } = useCoachingRelationshipMutation( + currentOrganizationId + ); const [formData, setFormData] = useState({ firstName: "", lastName: "", @@ -71,11 +82,32 @@ export function AddMemberDialog({ const [isLookingUp, setIsLookingUp] = useState(false); const [existingRole, setExistingRole] = useState(Role.User); const [isAdding, setIsAdding] = useState(false); + const [coachId, setCoachId] = useState(NO_COACH); // Org admins get this too, not just super admins const canAddExisting = !!currentUserRoleState && isAdminOrSuperAdmin(currentUserRoleState); + // Runs only after the member exists, so a failure here leaves a coachless + // member rather than blocking the add. Returns whether the coach was assigned. + const assignSelectedCoach = async (coacheeId: Id): Promise => { + if (coachId === NO_COACH) return true; + + try { + await createRelationship(currentOrganizationId, { + coach_id: coachId, + coachee_id: coacheeId, + }); + return true; + } catch (error) { + console.error("Error assigning coach:", error); + return false; + } + }; + + const coachAssignmentFailedMessage = + "They were added, but assigning the coach failed. Assign one from the member list."; + const handleInputChange = (e: React.ChangeEvent) => { const { name, value } = e.target; setFormData((prev) => ({ @@ -96,15 +128,22 @@ export function AddMemberDialog({ }; try { - await createUserNested(currentOrganizationId, newUser); + const created = await createUserNested(currentOrganizationId, newUser); + const coachAssigned = await assignSelectedCoach(created.id); setFormData({ firstName: "", lastName: "", displayName: "", email: "", }); + setCoachId(NO_COACH); onMemberAdded(); - toast.success(`New Member ${formData.firstName} ${formData.lastName} added successfully`); + const name = `${formData.firstName} ${formData.lastName}`; + if (coachAssigned) { + toast.success(`New Member ${name} added successfully`); + } else { + toast.warning(`${name} added. ${coachAssignmentFailedMessage}`); + } onOpenChange(false); } catch (error) { console.error("Error creating user:", error); @@ -148,6 +187,7 @@ export function AddMemberDialog({ setFoundUser(null); setLookupMessage(null); setExistingRole(Role.User); + setCoachId(NO_COACH); }; // A found user belongs to the email that produced it, so editing the field @@ -165,10 +205,14 @@ export function AddMemberDialog({ try { await attachExisting(currentOrganizationId, foundUser.id, existingRole); + const coachAssigned = await assignSelectedCoach(foundUser.id); onMemberAdded(); - toast.success( - `${foundUser.first_name} ${foundUser.last_name} added to this organization` - ); + const name = `${foundUser.first_name} ${foundUser.last_name}`; + if (coachAssigned) { + toast.success(`${name} added to this organization`); + } else { + toast.warning(`${name} added to this organization. ${coachAssignmentFailedMessage}`); + } resetLookup(); onOpenChange(false); } catch (error) { @@ -185,6 +229,30 @@ export function AddMemberDialog({ } }; + /// Optional coach picker. `excludeId` keeps a member off their own coach list. + const coachField = (excludeId?: string) => + organizationMembers && + organizationMembers.length > 0 && ( +
+ + +
+ ); + const createMemberForm = (
@@ -233,6 +301,7 @@ export function AddMemberDialog({ required />
+ {coachField()}
@@ -307,6 +376,7 @@ export function AddMemberDialog({
+ {coachField(foundUser?.id)} {lookupMessage && (

{lookupMessage}

)} - {foundUser && ( + {foundUser.some && (

- {foundUser.first_name} {foundUser.last_name} + {foundUser.val.first_name} {foundUser.val.last_name} +

+

+ {foundUser.val.email}

-

{foundUser.email}

@@ -351,12 +355,12 @@ export function AddMemberDialog({
- {coachField(foundUser?.id)} + {coachField(foundUser.some ? foundUser.val.id : undefined)} diff --git a/src/components/ui/members/member-card.tsx b/src/components/ui/members/member-card.tsx index 5c86ed6f..c500b66e 100644 --- a/src/components/ui/members/member-card.tsx +++ b/src/components/ui/members/member-card.tsx @@ -80,7 +80,7 @@ export function MemberCard({ currentUserRoleState, }: MemberCardProps) { const { currentOrganizationId } = useCurrentOrganization(); - const { isACoach, userSession } = useAuthStore((state: AuthStore) => state); + const { userSession } = useAuthStore((state: AuthStore) => state); // Extract user properties const { id: userId, first_name: firstName, last_name: lastName, email } = user; @@ -96,8 +96,6 @@ export function MemberCard({ const { createNested: createRelationship } = useCoachingRelationshipMutation(currentOrganizationId); - console.log("is a coach", isACoach); - // Only admins and super admins can delete users (but not themselves) const canDeleteUser = currentUserRoleState.hasAccess && @@ -112,6 +110,7 @@ export function MemberCard({ try { await deleteUser(currentOrganizationId, userId); toast.success("Member deleted successfully"); + onRefresh(); } catch (error) { console.error("Error deleting member:", error); toast.error( @@ -122,7 +121,6 @@ export function MemberCard({ : "Error deleting member") ); } - onRefresh(); }; const handleRemoveFromOrganization = async () => { diff --git a/src/lib/api/organizations/users.ts b/src/lib/api/organizations/users.ts index 656dc529..7c50b5f8 100644 --- a/src/lib/api/organizations/users.ts +++ b/src/lib/api/organizations/users.ts @@ -92,8 +92,8 @@ export const UserApi = { userId: Id, role: Role, coachId?: Id - ): Promise => - EntityApi.createFn( + ): Promise => + EntityApi.createFn( `${ORGANIZATIONS_USERS_BASEURL(organizationId)}/${userId}/role`, { role, ...(coachId ? { coach_id: coachId } : {}) } ), @@ -160,9 +160,8 @@ export const useUserMutation = (organizationId: Id) => { role: Role, coachId?: Id ) => { - const user = await UserApi.attachExisting(orgId, userId, role, coachId); + await UserApi.attachExisting(orgId, userId, role, coachId); invalidate(orgId); - return user; }, removeFromOrganization: async (orgId: Id, userId: Id) => { await UserApi.removeFromOrganization(orgId, userId); diff --git a/src/lib/api/users.ts b/src/lib/api/users.ts index 74a5db8f..c4563153 100644 --- a/src/lib/api/users.ts +++ b/src/lib/api/users.ts @@ -10,6 +10,7 @@ import { defaultUser, } from "@/types/user"; import { buildQueryString } from "./query-params"; +import { type Option, Some, None } from "@/types/option"; export const USERS_BASEURL: string = `${siteConfig.env.backendServiceURL}/users`; @@ -25,11 +26,11 @@ export const UserApi = { * may not see them. The backend makes those two cases indistinguishable so * the endpoint can't be used to enumerate accounts. */ - lookupByEmail: async (email: string): Promise => { + lookupByEmail: async (email: string): Promise> => { const results = await EntityApi.getFn( `${USERS_BASEURL}${buildQueryString({ email })}` ); - return results[0] ?? null; + return results.length > 0 ? Some(results[0]) : None; }, /** diff --git a/src/lib/hooks/use-assigned-actions.ts b/src/lib/hooks/use-assigned-actions.ts index 957c19e7..30c0d884 100644 --- a/src/lib/hooks/use-assigned-actions.ts +++ b/src/lib/hooks/use-assigned-actions.ts @@ -59,6 +59,9 @@ export function useAssignedActions( enrichedSessions: sessions, isLoading: sessionsLoading, isError: sessionsError, + // TODO(rs#374): pass the current organization once GET /users/{id}/actions + // accepts one. Scoping these sessions alone would strip context off + // out-of-organization actions rather than hide them. } = useEnrichedCoachingSessionsForUser( userId, oneYearAgo, diff --git a/src/lib/hooks/use-session-context-fetch.ts b/src/lib/hooks/use-session-context-fetch.ts index 3dbe528b..cbe7c5ca 100644 --- a/src/lib/hooks/use-session-context-fetch.ts +++ b/src/lib/hooks/use-session-context-fetch.ts @@ -23,6 +23,9 @@ export function useSessionContextFetch(userId: string | null) { enrichedSessions: sessions, isLoading, isError, + // TODO(rs#374): pass the current organization once GET /users/{id}/actions + // accepts one. Scoping these sessions alone would strip context off + // out-of-organization actions rather than hide them. } = useEnrichedCoachingSessionsForUser(userId, oneYearAgo, oneYearFromNow, [ CoachingSessionInclude.Relationship, CoachingSessionInclude.Goal, diff --git a/src/types/organization.ts b/src/types/organization.ts index c11b7c4f..5a04b2a1 100644 --- a/src/types/organization.ts +++ b/src/types/organization.ts @@ -96,7 +96,9 @@ export function organizationInitials(name: string | undefined): string { // back to the word's first two letters ("Acme" -> "AC"). const parts = words[0].match(/\p{Lu}+\p{Ll}*|\p{Ll}+/gu) ?? []; const letters = - parts.length > 1 ? parts.map((part) => part[0]) : Array.from(words[0]); + parts.length > 1 + ? parts.map((part) => Array.from(part)[0]) + : Array.from(words[0]); return letters.slice(0, 2).join("").toUpperCase(); } From 4163914003e66a5de56c4ba4a69dbe1128137124 Mon Sep 17 00:00:00 2001 From: Jim Hodapp Date: Fri, 7 Aug 2026 11:47:11 -0500 Subject: [PATCH 10/17] fix(members): source the product and organization names from config The Add Member dialog hardcoded "Refactor", which is not the product name. Both mentions now read siteConfig.name, and the existing-member copy names the organization being added to rather than saying "this organization". Falls back to "you are viewing" for the brief window before the current organization resolves, so the sentence never renders with a gap in it. --- .../ui/members/add-member-dialog.tsx | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/components/ui/members/add-member-dialog.tsx b/src/components/ui/members/add-member-dialog.tsx index ced87ec9..f2943cf1 100644 --- a/src/components/ui/members/add-member-dialog.tsx +++ b/src/components/ui/members/add-member-dialog.tsx @@ -40,6 +40,7 @@ import { useCurrentOrganization } from "@/lib/hooks/use-current-organization"; import { type Option, None } from "@/types/option"; import { toast } from "sonner"; import { getBrowserTimezone } from "@/lib/timezone-utils"; +import { siteConfig } from "@/site.config"; import { isForbiddenError, PERMISSION_DENIED_MESSAGE } from "@/types/general"; /// Sentinel for the "no coach" option, since Select cannot hold an empty value. @@ -62,10 +63,11 @@ export function AddMemberDialog({ currentUserRoleState, organizationMembers, }: AddMemberDialogProps) { - const { currentOrganizationId } = useCurrentOrganization(); + const { currentOrganizationId, currentOrganization } = + useCurrentOrganization(); const { createNested: createUserNested, attachExisting } = useUserMutation( - currentOrganizationId + currentOrganizationId, ); const [formData, setFormData] = useState({ firstName: "", @@ -127,7 +129,7 @@ export function AddMemberDialog({ organizationArchivedMessage(error) ?? (isForbiddenError(error) ? PERMISSION_DENIED_MESSAGE - : "There was an error adding the member") + : "There was an error adding the member"), ); } }; @@ -151,7 +153,7 @@ export function AddMemberDialog({ setLookupMessage( isForbiddenError(error) ? PERMISSION_DENIED_MESSAGE - : "There was an error looking up that email." + : "There was an error looking up that email.", ); } finally { setIsLookingUp(false); @@ -185,7 +187,7 @@ export function AddMemberDialog({ currentOrganizationId, target.id, existingRole, - selectedCoachId + selectedCoachId, ); onMemberAdded(); const name = `${target.first_name} ${target.last_name}`; @@ -199,7 +201,7 @@ export function AddMemberDialog({ organizationArchivedMessage(error) ?? (isForbiddenError(error) ? PERMISSION_DENIED_MESSAGE - : "There was an error adding the member") + : "There was an error adding the member"), ); } finally { setIsAdding(false); @@ -291,8 +293,10 @@ export function AddMemberDialog({ const addExistingForm = (

- This person already has a Refactor account. Adding them here gives them - access to this organization using their existing profile. + Add a user that already has an account. Adding them here gives them + access to the organization{" "} + {currentOrganization?.name ?? "you are viewing"} using their existing + profile.

@@ -375,7 +379,7 @@ export function AddMemberDialog({ Add New Member {canAddExisting - ? "Create a new member account, or add someone who already has a Refactor account." + ? `Create a new member account, or add someone who already has a ${siteConfig.name} account.` : "Create a new member account. They'll receive an email with a link to set up their password."} From a6b9e61241a987acf6e528070a76409a662d8c90 Mon Sep 17 00:00:00 2001 From: Jim Hodapp Date: Fri, 7 Aug 2026 12:21:32 -0500 Subject: [PATCH 11/17] fix(members): discard a lookup that resolves after the email changes Editing the email cleared the found user, but an in-flight lookup that resolved afterwards wrote its result back over that. The confirmation card then showed an account the email field no longer named, and Add attached that account instead of the one on screen. Each lookup now carries a token. Editing the field or starting another lookup bumps it, and a reply whose token is stale is dropped. Also takes the product name as a prop rather than importing siteConfig. The coding standards prohibit leaf components reading global config; the members page reads it once and threads it down. --- .../add-member-dialog-existing.test.tsx | 36 +++++++++++++++++++ .../members/add-member-dialog.test.tsx | 21 +++++++++-- src/app/organizations/[id]/members/page.tsx | 2 ++ .../ui/members/add-member-button.tsx | 3 ++ .../ui/members/add-member-dialog.tsx | 20 ++++++++--- .../ui/members/member-container.tsx | 3 ++ 6 files changed, 78 insertions(+), 7 deletions(-) diff --git a/__tests__/components/members/add-member-dialog-existing.test.tsx b/__tests__/components/members/add-member-dialog-existing.test.tsx index 5d8bba20..181422d9 100644 --- a/__tests__/components/members/add-member-dialog-existing.test.tsx +++ b/__tests__/components/members/add-member-dialog-existing.test.tsx @@ -77,6 +77,7 @@ function renderDialog( open onOpenChange={vi.fn()} onMemberAdded={vi.fn()} + productName="Refactor Coach" currentUserRoleState={currentUserRoleState} organizationMembers={organizationMembers} /> @@ -391,4 +392,39 @@ describe("AddMemberDialog – attaching an existing member", () => { ) ); }); + it("discards a lookup that lands after the email was edited", async () => { + // The reply is held open so the email can be edited while it is in flight. + let release: () => void = () => {}; + const held = new Promise((resolve) => { + release = resolve; + }); + server.use( + http.get("*/users", async ({ request }) => { + const email = new URL(request.url).searchParams.get("email"); + if (email === ADA.email) await held; + return HttpResponse.json({ + status_code: 200, + data: email === ADA.email ? [ADA] : [], + }); + }) + ); + const user = userEvent.setup(); + renderDialog(adminRole); + + await user.click(screen.getByRole("tab", { name: "Add existing member" })); + await user.type(screen.getByLabelText("Email"), ADA.email); + await user.click(screen.getByRole("button", { name: "Find" })); + + // Retype before the reply arrives, then let it land. + await user.clear(screen.getByLabelText("Email")); + await user.type(screen.getByLabelText("Email"), "someone.else@example.com"); + release(); + + // Ada must not reappear: the card belongs to an address the field no longer + // shows, and Add would attach her instead of the address on screen. + await waitFor(() => + expect(screen.getByRole("button", { name: "Add to organization" })).toBeDisabled() + ); + expect(screen.queryByText("Ada Lovelace")).not.toBeInTheDocument(); + }); }); diff --git a/__tests__/components/members/add-member-dialog.test.tsx b/__tests__/components/members/add-member-dialog.test.tsx index cbc80722..d2ab9b9f 100644 --- a/__tests__/components/members/add-member-dialog.test.tsx +++ b/__tests__/components/members/add-member-dialog.test.tsx @@ -59,7 +59,12 @@ describe("AddMemberDialog write-freeze handling", () => { }) ); render( - + ); fillForm(); @@ -75,7 +80,12 @@ describe("AddMemberDialog write-freeze handling", () => { it("shows the permission-denied message on a 403", async () => { mockCreateNested.mockRejectedValueOnce(apiError(403, { error: "forbidden" })); render( - + ); fillForm(); @@ -91,7 +101,12 @@ describe("AddMemberDialog write-freeze handling", () => { it("falls back to the generic message for other errors", async () => { mockCreateNested.mockRejectedValueOnce(new Error("network")); render( - + ); fillForm(); diff --git a/src/app/organizations/[id]/members/page.tsx b/src/app/organizations/[id]/members/page.tsx index d3faf37f..6482dc3f 100644 --- a/src/app/organizations/[id]/members/page.tsx +++ b/src/app/organizations/[id]/members/page.tsx @@ -13,6 +13,7 @@ import { ForbiddenError } from "@/components/ui/errors/forbidden-error"; import { MemberContainer } from "@/components/ui/members/member-container"; import { PageContainer } from "@/components/ui/page-container"; import { shouldDenyMembersPageAccess } from "./access-control"; +import { siteConfig } from "@/site.config"; export default function MembersPage({ params, @@ -92,6 +93,7 @@ export default function MembersPage({ onRefresh={handleRefresh} isLoading={isRelationshipsLoading || isUsersLoading} openAddMemberDialog={openAddMemberDialog} + productName={siteConfig.name} /> ); diff --git a/src/components/ui/members/add-member-button.tsx b/src/components/ui/members/add-member-button.tsx index 7a3e9f8c..31ea2166 100644 --- a/src/components/ui/members/add-member-button.tsx +++ b/src/components/ui/members/add-member-button.tsx @@ -13,6 +13,7 @@ interface AddMemberButtonProps { currentUserRoleState: UserRoleState; /// Candidates offered when pre-assigning a coach organizationMembers?: User[]; + productName: string; } export function AddMemberButton({ @@ -20,6 +21,7 @@ export function AddMemberButton({ openAddMemberDialog, currentUserRoleState, organizationMembers, + productName, }: AddMemberButtonProps) { const [open, setOpen] = useState(false); @@ -39,6 +41,7 @@ export function AddMemberButton({ onMemberAdded={onMemberAdded} currentUserRoleState={currentUserRoleState} organizationMembers={organizationMembers} + productName={productName} /> ); diff --git a/src/components/ui/members/add-member-dialog.tsx b/src/components/ui/members/add-member-dialog.tsx index f2943cf1..ba88d755 100644 --- a/src/components/ui/members/add-member-dialog.tsx +++ b/src/components/ui/members/add-member-dialog.tsx @@ -2,7 +2,7 @@ import type React from "react"; -import { useState } from "react"; +import { useRef, useState } from "react"; import { Button } from "@/components/ui/button"; import { Dialog, @@ -40,7 +40,6 @@ import { useCurrentOrganization } from "@/lib/hooks/use-current-organization"; import { type Option, None } from "@/types/option"; import { toast } from "sonner"; import { getBrowserTimezone } from "@/lib/timezone-utils"; -import { siteConfig } from "@/site.config"; import { isForbiddenError, PERMISSION_DENIED_MESSAGE } from "@/types/general"; /// Sentinel for the "no coach" option, since Select cannot hold an empty value. @@ -54,6 +53,8 @@ interface AddMemberDialogProps { currentUserRoleState?: UserRoleState; /// Candidates offered when pre-assigning a coach. Omitted hides the field. organizationMembers?: User[]; + /// Product name, threaded from the page rather than read from global config. + productName: string; } export function AddMemberDialog({ @@ -62,6 +63,7 @@ export function AddMemberDialog({ onMemberAdded, currentUserRoleState, organizationMembers, + productName, }: AddMemberDialogProps) { const { currentOrganizationId, currentOrganization } = useCurrentOrganization(); @@ -82,6 +84,8 @@ export function AddMemberDialog({ const [existingRole, setExistingRole] = useState(Role.User); const [isAdding, setIsAdding] = useState(false); const [coachId, setCoachId] = useState(NO_COACH); + /// Identifies the newest lookup, so a slower earlier one cannot land on top of it. + const lookupRequest = useRef(0); // Org admins get this too, not just super admins const canAddExisting = @@ -135,12 +139,17 @@ export function AddMemberDialog({ }; const handleFind = async () => { + const request = ++lookupRequest.current; setFoundUser(None); setLookupMessage(null); setIsLookingUp(true); try { const result = await UserApi.lookupByEmail(lookupEmail); + // Editing the email, or starting another lookup, supersedes this one. + // Without the check a slow reply repopulates the card for an address the + // field no longer shows, and Add attaches that user instead. + if (lookupRequest.current !== request) return; // None also covers a real user outside this admin's scope. The backend // makes those cases indistinguishable, so the copy must too. if (result.some) { @@ -149,6 +158,7 @@ export function AddMemberDialog({ setLookupMessage("No user found with that email."); } } catch (error) { + if (lookupRequest.current !== request) return; console.error("Error looking up user:", error); setLookupMessage( isForbiddenError(error) @@ -156,7 +166,7 @@ export function AddMemberDialog({ : "There was an error looking up that email.", ); } finally { - setIsLookingUp(false); + if (lookupRequest.current === request) setIsLookingUp(false); } }; @@ -172,9 +182,11 @@ export function AddMemberDialog({ // invalidates it. Without this the Add button can act on a stale selection // while the field shows a different address. const handleLookupEmailChange = (value: string) => { + lookupRequest.current += 1; setLookupEmail(value); setFoundUser(None); setLookupMessage(null); + setIsLookingUp(false); }; const handleAddExisting = async () => { @@ -379,7 +391,7 @@ export function AddMemberDialog({ Add New Member {canAddExisting - ? `Create a new member account, or add someone who already has a ${siteConfig.name} account.` + ? `Create a new member account, or add someone who already has a ${productName} account.` : "Create a new member account. They'll receive an email with a link to set up their password."} diff --git a/src/components/ui/members/member-container.tsx b/src/components/ui/members/member-container.tsx index 341d8fb1..727e1c5e 100644 --- a/src/components/ui/members/member-container.tsx +++ b/src/components/ui/members/member-container.tsx @@ -15,6 +15,7 @@ interface MemberContainerProps { onRefresh: () => void; isLoading: boolean; openAddMemberDialog: boolean; + productName: string; } export function MemberContainer({ @@ -25,6 +26,7 @@ export function MemberContainer({ isLoading, /// Force the AddMemberDialog to open openAddMemberDialog, + productName, }: MemberContainerProps) { const { setIsACoach, isACoach } = useAuthStore((state) => state); const currentUserRoleState = useCurrentUserRole(); @@ -61,6 +63,7 @@ export function MemberContainer({ openAddMemberDialog={openAddMemberDialog} currentUserRoleState={currentUserRoleState} organizationMembers={displayUsers} + productName={productName} /> )}
From ee49fecfa8f1182f5cfee93c7bb96cdfb51cc07e Mon Sep 17 00:00:00 2001 From: Jim Hodapp Date: Fri, 7 Aug 2026 12:27:32 -0500 Subject: [PATCH 12/17] test(members): import ReactNode directly instead of the React namespace The annotation referenced React without importing it. Types are erased and __tests__ sits outside the tsc include, so it never surfaced. --- __tests__/components/members/member-card-remove.test.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/__tests__/components/members/member-card-remove.test.tsx b/__tests__/components/members/member-card-remove.test.tsx index 28dd289d..d763cf7b 100644 --- a/__tests__/components/members/member-card-remove.test.tsx +++ b/__tests__/components/members/member-card-remove.test.tsx @@ -1,4 +1,5 @@ import { render, screen, waitFor } from "@testing-library/react"; +import type { ReactNode } from "react"; import userEvent from "@testing-library/user-event"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { http, HttpResponse } from "msw"; @@ -30,7 +31,7 @@ vi.mock("@/lib/hooks/use-current-organization", () => ({ const mockAuthStore = vi.fn(); vi.mock("@/lib/providers/auth-store-provider", () => ({ - AuthStoreProvider: ({ children }: { children: React.ReactNode }) => children, + AuthStoreProvider: ({ children }: { children: ReactNode }) => children, useAuthStore: (selector: (state: unknown) => unknown) => selector(mockAuthStore()), })); From fda7966f7182ab1cf1c0e51a22f6611a381436d4 Mon Sep 17 00:00:00 2001 From: Jim Hodapp Date: Fri, 7 Aug 2026 12:43:50 -0500 Subject: [PATCH 13/17] feat(members): reject an existing member at lookup, not on submit Finding someone who already belongs to the organization built the whole confirmation card, so an admin picked a role and a coach before the request came back 409. The lookup now checks the member list it already has and says so immediately. The server check stays: the member list can be stale, and is absent entirely for callers that do not pass one. Both paths share one message string so they cannot drift. This makes the coach picker's self-exclusion unreachable, since the picker and the new check read the same member list, so a found user can no longer appear in it. Removed the filter and reframed the test that covered it. --- .../add-member-dialog-existing.test.tsx | 29 ++++++++++++--- .../ui/members/add-member-dialog.tsx | 36 ++++++++++++------- src/lib/api/organization-errors.ts | 7 +++- 3 files changed, 53 insertions(+), 19 deletions(-) diff --git a/__tests__/components/members/add-member-dialog-existing.test.tsx b/__tests__/components/members/add-member-dialog-existing.test.tsx index 181422d9..0bfe0e1c 100644 --- a/__tests__/components/members/add-member-dialog-existing.test.tsx +++ b/__tests__/components/members/add-member-dialog-existing.test.tsx @@ -267,10 +267,10 @@ describe("AddMemberDialog – pre-assigning a coach", () => { expect(toast.warning).not.toHaveBeenCalled(); }); - it("keeps the found user off their own coach list", async () => { + it("offers the organization's existing members as coaches", async () => { server.use(lookupHandler(ADA)); const user = userEvent.setup(); - renderDialog(adminRole, [GRACE, ADA as unknown as User]); + renderDialog(adminRole, [GRACE]); await findAda(user); await user.click(screen.getByLabelText("Coach (optional)")); @@ -278,9 +278,6 @@ describe("AddMemberDialog – pre-assigning a coach", () => { expect( await screen.findByRole("option", { name: "Grace Hopper" }) ).toBeInTheDocument(); - expect( - screen.queryByRole("option", { name: "Ada Lovelace" }) - ).not.toBeInTheDocument(); }); }); @@ -427,4 +424,26 @@ describe("AddMemberDialog – attaching an existing member", () => { ); expect(screen.queryByText("Ada Lovelace")).not.toBeInTheDocument(); }); + it("reports an existing member at lookup time instead of on submit", async () => { + server.use(lookupHandler(ADA)); + const alreadyAMember = { + id: ADA.id, + first_name: ADA.first_name, + last_name: ADA.last_name, + } as unknown as User; + const user = userEvent.setup(); + renderDialog(adminRole, [GRACE, alreadyAMember]); + + await user.click(screen.getByRole("tab", { name: "Add existing member" })); + await user.type(screen.getByLabelText("Email"), ADA.email); + await user.click(screen.getByRole("button", { name: "Find" })); + + await screen.findByText("This user is already a member of this organization."); + // No confirmation card, so there is no role or coach to fill in and no + // request to send. + expect(screen.queryByText("Ada Lovelace")).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Add to organization" }) + ).toBeDisabled(); + }); }); diff --git a/src/components/ui/members/add-member-dialog.tsx b/src/components/ui/members/add-member-dialog.tsx index ba88d755..6bf6b765 100644 --- a/src/components/ui/members/add-member-dialog.tsx +++ b/src/components/ui/members/add-member-dialog.tsx @@ -27,6 +27,7 @@ import { UserApi } from "@/lib/api/users"; import { organizationArchivedMessage, userAlreadyInOrganizationMessage, + USER_ALREADY_IN_ORGANIZATION_MESSAGE, } from "@/lib/api/organization-errors"; import { NewUser, @@ -152,11 +153,19 @@ export function AddMemberDialog({ if (lookupRequest.current !== request) return; // None also covers a real user outside this admin's scope. The backend // makes those cases indistinguishable, so the copy must too. - if (result.some) { - setFoundUser(result); - } else { + if (result.none) { setLookupMessage("No user found with that email."); + return; } + // Say so now rather than letting them pick a role and a coach first only + // for the request to come back 409. The server still enforces this, since + // the member list can be stale and may be absent entirely. + const found = result.val; + if (organizationMembers?.some((member) => member.id === found.id)) { + setLookupMessage(USER_ALREADY_IN_ORGANIZATION_MESSAGE); + return; + } + setFoundUser(result); } catch (error) { if (lookupRequest.current !== request) return; console.error("Error looking up user:", error); @@ -220,8 +229,11 @@ export function AddMemberDialog({ } }; - /// Optional coach picker. `excludeId` keeps a member off their own coach list. - const coachField = (excludeId?: string) => + /// Optional coach picker, listing the organization's existing members. + /// + /// No self-exclusion is needed: a brand new member is not in the list yet, and + /// an existing one is rejected at lookup before the card ever renders. + const coachField = () => organizationMembers && organizationMembers.length > 0 && (
@@ -232,13 +244,11 @@ export function AddMemberDialog({ No coach - {organizationMembers - .filter((member) => member.id !== excludeId) - .map((member) => ( - - {member.first_name} {member.last_name} - - ))} + {organizationMembers.map((member) => ( + + {member.first_name} {member.last_name} + + ))}
@@ -371,7 +381,7 @@ export function AddMemberDialog({
- {coachField(foundUser.some ? foundUser.val.id : undefined)} + {coachField()}