diff --git a/.server-changes/fix-project-settings-toast.md b/.server-changes/fix-project-settings-toast.md new file mode 100644 index 0000000000..0619c4aa01 --- /dev/null +++ b/.server-changes/fix-project-settings-toast.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Renaming a project now keeps you on the project settings page and tells you what happened, instead of silently moving you to the tasks page or clearing the form with no explanation. diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.general/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.general/route.tsx index ccc61fc5ac..a26287ab46 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.general/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.general/route.tsx @@ -23,7 +23,7 @@ import { resolveOrgIdFromSlug } from "~/models/organization.server"; import { ProjectSettingsService } from "~/services/projectSettings.server"; import { logger } from "~/services/logger.server"; import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder"; -import { organizationPath, v3ProjectPath } from "~/utils/pathBuilder"; +import { organizationPath, v3ProjectSettingsGeneralPath } from "~/utils/pathBuilder"; import { useState } from "react"; function createSchema( @@ -60,9 +60,21 @@ function createSchema( ]); } +type FormAction = "rename" | "delete"; + +export function submissionFor(lastSubmission: unknown, formAction: FormAction) { + return lastSubmission && + typeof lastSubmission === "object" && + "formAction" in lastSubmission && + lastSubmission.formAction === formAction + ? lastSubmission + : undefined; +} + const Params = z.object({ organizationSlug: z.string(), projectParam: z.string(), + envParam: z.string(), }); export const action = dashboardAction( @@ -75,9 +87,16 @@ export const action = dashboardAction( }, async ({ user, ability, request, params }) => { const userId = user.id; - const { organizationSlug, projectParam } = params; + const { organizationSlug, projectParam, envParam } = params; + + const settingsPath = v3ProjectSettingsGeneralPath( + { slug: organizationSlug }, + { slug: projectParam }, + { slug: envParam } + ); const formData = await request.formData(); + const formAction = formData.get("action") as FormAction; const schema = createSchema({ getSlugMatch: (slug) => { @@ -87,7 +106,7 @@ export const action = dashboardAction( const submission = parseWithZod(formData, { schema }); if (submission.status !== "success") { - return json(submission.reply()); + return json({ ...submission.reply(), formAction }); } const projectSettingsService = new ProjectSettingsService(); @@ -98,7 +117,10 @@ export const action = dashboardAction( ); if (membershipResultOrFail.isErr()) { - return json({ errors: { body: membershipResultOrFail.error.type } }, { status: 404 }); + return json( + { ...submission.reply({ formErrors: ["Project not found"] }), formAction }, + { status: 404 } + ); } const { projectId } = membershipResultOrFail.value; @@ -107,7 +129,7 @@ export const action = dashboardAction( case "rename": { if (!ability.can("manage", { type: "project" })) { throw await redirectWithErrorMessage( - v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }), + settingsPath, request, "You don't have permission to rename this project" ); @@ -126,13 +148,16 @@ export const action = dashboardAction( logger.error("Failed to rename project", { error: resultOrFail.error, }); - return json({ errors: { body: "Failed to rename project" } }, { status: 400 }); + return json( + { ...submission.reply({ formErrors: ["Failed to rename project"] }), formAction }, + { status: 400 } + ); } } } return redirectWithSuccessMessage( - v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }), + settingsPath, request, `Project renamed to ${submission.value.projectName}` ); @@ -140,7 +165,7 @@ export const action = dashboardAction( case "delete": { if (!ability.can("manage", { type: "project" })) { throw await redirectWithErrorMessage( - v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }), + settingsPath, request, "You don't have permission to delete this project" ); @@ -157,7 +182,7 @@ export const action = dashboardAction( error: resultOrFail.error, }); return redirectWithErrorMessage( - v3ProjectPath({ slug: organizationSlug }, { slug: projectParam }), + settingsPath, request, `Project ${projectParam} could not be deleted` ); @@ -185,7 +210,7 @@ export default function GeneralSettingsPage() { const [renameForm, { projectName }] = useForm({ id: "rename-project", // TODO: type this - lastResult: lastSubmission as any, + lastResult: submissionFor(lastSubmission, "rename") as any, shouldRevalidate: "onSubmit", onValidate({ formData }) { return parseWithZod(formData, { @@ -201,7 +226,7 @@ export default function GeneralSettingsPage() { const [deleteForm, { projectSlug }] = useForm({ id: "delete-project", // TODO: type this - lastResult: lastSubmission as any, + lastResult: submissionFor(lastSubmission, "delete") as any, shouldValidate: "onInput", shouldRevalidate: "onSubmit", onValidate({ formData }) { @@ -250,6 +275,7 @@ export default function GeneralSettingsPage() { }} /> {projectName.errors} + {renameForm.errors} ({ + dashboardAction: (_options: unknown, handler: unknown) => handler, + dashboardLoader: (_options: unknown, handler: unknown) => handler, +})); + +vi.mock("~/models/organization.server", () => ({ + resolveOrgIdFromSlug: vi.fn().mockResolvedValue("org_1"), +})); + +const renameFails = { value: false }; + +vi.mock("~/services/projectSettings.server", () => ({ + ProjectSettingsService: class { + verifyProjectMembership() { + return okAsync({ projectId: "proj_1" }); + } + renameProject() { + return renameFails.value ? errAsync({ type: "other" as const }) : okAsync(undefined); + } + deleteProject() { + return okAsync(undefined); + } + }, +})); + +const SETTINGS_PATH = "/orgs/o/projects/p/env/prod/settings/general"; +const ORG_PATH = "/orgs/o"; + +// Mirrors the read in app/root.tsx's loader. +async function rootLoaderHop(cookie: string | null) { + const session = await getSession(cookie); + const toastMessage = session.get("toastMessage"); + return { toastMessage, setCookie: await commitSession(session) }; +} + +function asRequestCookie(setCookie: string) { + return setCookie.split(";")[0]; +} + +async function runAction(action: "rename" | "delete", allowed: boolean) { + const body = new URLSearchParams( + action === "rename" ? { action, projectName: "New name" } : { action, projectSlug: "p" } + ); + + try { + return (await (generalSettingsAction as any)({ + user: { id: "user_1" }, + ability: { can: () => allowed }, + request: new Request(`https://app.example.com${SETTINGS_PATH}`, { method: "POST", body }), + params: { organizationSlug: "o", projectParam: "p", envParam: "prod" }, + context: {}, + searchParams: undefined, + })) as Response; + } catch (thrown) { + return thrown as Response; + } +} + +async function toastFor(response: Response) { + const hop = await rootLoaderHop(asRequestCookie(response.headers.get("Set-Cookie")!)); + return hop.toastMessage?.message; +} + +describe("toast flash through a redirect chain", () => { + it("is lost when the redirect target redirects again", async () => { + const request = new Request(`https://app.example.com${SETTINGS_PATH}`, { method: "POST" }); + const response = await redirectWithErrorMessage("/orgs/o/projects/p", request, "Denied"); + + const projectRootHop = await rootLoaderHop( + asRequestCookie(response.headers.get("Set-Cookie")!) + ); + expect(projectRootHop.toastMessage?.message).toBe("Denied"); + + const tasksPageHop = await rootLoaderHop(asRequestCookie(projectRootHop.setCookie)); + expect(tasksPageHop.toastMessage).toBeUndefined(); + }); +}); + +describe("general settings redirects target a page that renders", () => { + it("sends a denied rename back to the settings page with the message", async () => { + const response = await runAction("rename", false); + + expect(response.headers.get("Location")).toBe(SETTINGS_PATH); + expect(await toastFor(response)).toBe("You don't have permission to rename this project"); + }); + + it("sends a denied delete back to the settings page with the message", async () => { + const response = await runAction("delete", false); + + expect(response.headers.get("Location")).toBe(SETTINGS_PATH); + expect(await toastFor(response)).toBe("You don't have permission to delete this project"); + }); + + // The deleted project's settings page is gone and no org-level page renders, so a + // successful delete keeps its original destination and its message is not shown. + it("leaves a successful delete pointed at the organization root", async () => { + const response = await runAction("delete", true); + + expect(response.headers.get("Location")).toBe(ORG_PATH); + }); +}); + +describe("general settings failures reach the form", () => { + it("returns a form-level error when the rename fails", async () => { + renameFails.value = true; + const response = await runAction("rename", true); + renameFails.value = false; + + expect(response.status).toBe(400); + expect(await response.json()).toMatchObject({ + error: { "": ["Failed to rename project"] }, + }); + }); + + // A SubmissionResult carries no form identity, so both forms would otherwise show it. + it("scopes the rename failure to the rename form", async () => { + renameFails.value = true; + const response = await runAction("rename", true); + renameFails.value = false; + + const result = await response.json(); + + expect(submissionFor(result, "rename")).toEqual(result); + expect(submissionFor(result, "delete")).toBeUndefined(); + }); +});