Skip to content

Commit b08c814

Browse files
fix(onboarding): only record the profile step once the API stored it
Addresses review feedback on the profile step: - The server actions return an explicit {stored, error} outcome, so a transport failure or a rejection without an "errors" array is no longer read as success. The gate writes its marker and announces the submitted answers only when the row exists, and the skip path awaits the same outcome instead of assuming it. - The local marker is keyed per tenant, so answering for one tenant no longer silences the step for another the same browser later opens. - The profile step detail is a discriminated union: only a submitted step carries the answers. - The detail route refuses with 405 instead of merely being hidden from the schema. - The onboarding guide no longer claims zero backend coupling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent c61c8d8 commit b08c814

10 files changed

Lines changed: 425 additions & 55 deletions

File tree

api/src/backend/api/tests/test_onboarding_profile.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,3 +141,15 @@ def test_members_without_permissions_can_answer(
141141
response = _submit(authenticated_client_no_permissions_rbac, ANSWERS)
142142

143143
assert response.status_code == status.HTTP_201_CREATED
144+
145+
def test_detail_route_is_refused(self, authenticated_client):
146+
# One row per tenant, so the collection is the only meaningful read;
147+
# the detail route must refuse rather than serve a second shape.
148+
_submit(authenticated_client, ANSWERS)
149+
profile = TenantOnboardingProfile.objects.get()
150+
151+
response = authenticated_client.get(
152+
reverse("onboarding-profile-detail", kwargs={"pk": profile.id})
153+
)
154+
155+
assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED

api/src/backend/api/v1/views.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9023,6 +9023,12 @@ def get_serializer_class(self):
90239023
return TenantOnboardingProfileCreateSerializer
90249024
return super().get_serializer_class()
90259025

9026+
def retrieve(self, request, *args, **kwargs):
9027+
# The resource is a single row per tenant, so the collection is the
9028+
# only meaningful read. `extend_schema(exclude=True)` hides the detail
9029+
# route from the docs; this is what actually closes it.
9030+
raise MethodNotAllowed(method="GET")
9031+
90269032
def _stored_response(self, profile, http_status):
90279033
return Response(
90289034
TenantOnboardingProfileSerializer(
Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
const {
4+
fetchMock,
5+
getAuthHeadersMock,
6+
handleApiErrorMock,
7+
handleApiResponseMock,
8+
} = vi.hoisted(() => ({
9+
fetchMock: vi.fn(),
10+
getAuthHeadersMock: vi.fn(),
11+
handleApiErrorMock: vi.fn(),
12+
handleApiResponseMock: vi.fn(),
13+
}));
14+
15+
vi.mock("next/cache", () => ({ revalidatePath: vi.fn() }));
16+
vi.mock("next/navigation", () => ({ redirect: vi.fn() }));
17+
18+
vi.mock("@/lib", () => ({
19+
apiBaseUrl: "https://api.example.com/api/v1",
20+
getAuthHeaders: getAuthHeadersMock,
21+
}));
22+
23+
vi.mock("@/lib/server-actions-helper", () => ({
24+
handleApiError: handleApiErrorMock,
25+
handleApiResponse: handleApiResponseMock,
26+
}));
27+
28+
import {
29+
isOnboardingProfileRecorded,
30+
skipOnboardingProfile,
31+
submitOnboardingProfile,
32+
} from "./profile";
33+
34+
const ANSWERS = {
35+
declared_cloud_accounts: "11-50",
36+
declared_team_size: "2-5",
37+
declared_role: "security",
38+
} as const;
39+
40+
describe("onboarding profile actions", () => {
41+
beforeEach(() => {
42+
vi.stubGlobal("fetch", fetchMock);
43+
fetchMock.mockReset();
44+
getAuthHeadersMock.mockReset().mockResolvedValue({});
45+
handleApiErrorMock.mockReset();
46+
handleApiResponseMock.mockReset().mockResolvedValue({ data: { id: "p1" } });
47+
});
48+
49+
it("reports a stored profile and sends the declared buckets", async () => {
50+
// Given
51+
fetchMock.mockResolvedValue(new Response("{}", { status: 201 }));
52+
53+
// When
54+
const result = await submitOnboardingProfile(ANSWERS);
55+
56+
// Then
57+
expect(result).toEqual({ stored: true });
58+
const [url, init] = fetchMock.mock.calls[0];
59+
expect(url).toBe("https://api.example.com/api/v1/onboarding-profiles");
60+
expect(JSON.parse(init.body).data).toEqual({
61+
type: "onboarding-profiles",
62+
attributes: ANSWERS,
63+
});
64+
});
65+
66+
it("records a skip as its own payload", async () => {
67+
// Given
68+
fetchMock.mockResolvedValue(new Response("{}", { status: 201 }));
69+
70+
// When
71+
const result = await skipOnboardingProfile();
72+
73+
// Then
74+
expect(result).toEqual({ stored: true });
75+
expect(JSON.parse(fetchMock.mock.calls[0][1].body).data.attributes).toEqual(
76+
{
77+
skipped: true,
78+
},
79+
);
80+
});
81+
82+
it("treats a transport failure as not stored", async () => {
83+
// Given — the request never reaches the API.
84+
fetchMock.mockRejectedValue(new Error("network down"));
85+
86+
// When
87+
const result = await submitOnboardingProfile(ANSWERS);
88+
89+
// Then
90+
expect(result.stored).toBe(false);
91+
expect(result.error).toBeTruthy();
92+
expect(handleApiErrorMock).toHaveBeenCalledTimes(1);
93+
});
94+
95+
it("treats a rejection without an errors array as not stored", async () => {
96+
// Given — e.g. a 403, which `handleApiResponse` returns as a bare error.
97+
fetchMock.mockResolvedValue(new Response("{}", { status: 403 }));
98+
handleApiResponseMock.mockResolvedValue({
99+
error: "Forbidden",
100+
status: 403,
101+
});
102+
103+
// When
104+
const result = await submitOnboardingProfile(ANSWERS);
105+
106+
// Then
107+
expect(result).toEqual({ stored: false, error: "Forbidden" });
108+
});
109+
110+
it("surfaces the API's own message when the payload carries one", async () => {
111+
// Given
112+
fetchMock.mockResolvedValue(new Response("{}", { status: 400 }));
113+
handleApiResponseMock.mockResolvedValue({
114+
error: "Bad request",
115+
errors: [{ detail: "This field is required." }],
116+
});
117+
118+
// When
119+
const result = await submitOnboardingProfile(ANSWERS);
120+
121+
// Then
122+
expect(result).toEqual({
123+
stored: false,
124+
error: "This field is required.",
125+
});
126+
});
127+
128+
it("treats a thrown server error as not stored", async () => {
129+
// Given — `handleApiResponse` throws on 5xx.
130+
fetchMock.mockResolvedValue(new Response("{}", { status: 500 }));
131+
handleApiResponseMock.mockRejectedValue(new Error("server error"));
132+
133+
// When
134+
const result = await skipOnboardingProfile();
135+
136+
// Then
137+
expect(result.stored).toBe(false);
138+
expect(handleApiErrorMock).toHaveBeenCalledTimes(1);
139+
});
140+
141+
it("refuses answers outside the declared vocabulary", async () => {
142+
// When / Then — validation happens before any request.
143+
await expect(
144+
submitOnboardingProfile({
145+
...ANSWERS,
146+
declared_role: "ceo",
147+
} as unknown as typeof ANSWERS),
148+
).rejects.toThrow();
149+
expect(fetchMock).not.toHaveBeenCalled();
150+
});
151+
152+
it.each([
153+
["an empty list", { data: [] }, false],
154+
["a stored row", { data: [{ id: "p1" }] }, true],
155+
])("reads %s as recorded=%s", async (_, payload, expected) => {
156+
// Given
157+
fetchMock.mockResolvedValue(
158+
new Response(JSON.stringify(payload), { status: 200 }),
159+
);
160+
161+
// Then
162+
expect(await isOnboardingProfileRecorded()).toBe(expected);
163+
});
164+
165+
it("returns undefined when the read fails, so the gate fails open", async () => {
166+
// Given
167+
fetchMock.mockResolvedValue(new Response("{}", { status: 500 }));
168+
169+
// Then
170+
expect(await isOnboardingProfileRecorded()).toBeUndefined();
171+
});
172+
});

ui/actions/onboarding/profile.ts

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,14 @@ import {
1414
const ONBOARDING_PROFILES_PATH = "/onboarding-profiles";
1515
const RESOURCE_TYPE = "onboarding-profiles";
1616

17+
// Explicit outcome instead of the raw API payload: the caller records the
18+
// step as resolved only when the row was really written, and a transport
19+
// failure is as unsuccessful as a rejected payload.
20+
export interface OnboardingProfileResult {
21+
stored: boolean;
22+
error?: string;
23+
}
24+
1725
const onboardingProfileAnswersSchema = z.object({
1826
declared_cloud_accounts: z.enum(
1927
Object.values(DECLARED_CLOUD_ACCOUNTS) as [string, ...string[]],
@@ -24,34 +32,68 @@ const onboardingProfileAnswersSchema = z.object({
2432
declared_role: z.enum(Object.values(DECLARED_ROLE) as [string, ...string[]]),
2533
});
2634

27-
const postOnboardingProfile = async (attributes: Record<string, unknown>) => {
35+
const GENERIC_FAILURE = "The onboarding profile could not be saved.";
36+
37+
const failureMessage = (payload: unknown): string | undefined => {
38+
if (typeof payload !== "object" || payload === null) return undefined;
39+
const result = payload as { error?: unknown; errors?: unknown };
40+
if (Array.isArray(result.errors)) {
41+
const detail = (result.errors[0] as { detail?: unknown })?.detail;
42+
if (typeof detail === "string") return detail;
43+
}
44+
return typeof result.error === "string" ? result.error : undefined;
45+
};
46+
47+
const postOnboardingProfile = async (
48+
attributes: Record<string, unknown>,
49+
): Promise<OnboardingProfileResult> => {
2850
const headers = await getAuthHeaders({ contentType: true });
2951
const body = JSON.stringify({
3052
data: { type: RESOURCE_TYPE, attributes },
3153
});
3254

55+
let response: Response;
3356
try {
34-
const response = await fetch(`${apiBaseUrl}${ONBOARDING_PROFILES_PATH}`, {
57+
response = await fetch(`${apiBaseUrl}${ONBOARDING_PROFILES_PATH}`, {
3558
method: "POST",
3659
headers,
3760
body,
3861
});
39-
return handleApiResponse(response);
4062
} catch (error) {
41-
return handleApiError(error);
63+
handleApiError(error);
64+
return { stored: false, error: GENERIC_FAILURE };
65+
}
66+
67+
// `handleApiResponse` reports to Sentry and throws on server errors; the
68+
// status is what decides the outcome, since a rejection can come back
69+
// without an `errors` array.
70+
try {
71+
const payload = await handleApiResponse(response);
72+
if (!response.ok) {
73+
return {
74+
stored: false,
75+
error: failureMessage(payload) ?? GENERIC_FAILURE,
76+
};
77+
}
78+
return { stored: true };
79+
} catch (error) {
80+
handleApiError(error);
81+
return { stored: false, error: GENERIC_FAILURE };
4282
}
4383
};
4484

4585
// Records the three declared buckets. The API keeps the first answer per
4686
// tenant: a repeated submission answers 200 with the stored profile.
4787
export const submitOnboardingProfile = async (
4888
answers: OnboardingProfileAnswers,
49-
) => postOnboardingProfile(onboardingProfileAnswersSchema.parse(answers));
89+
): Promise<OnboardingProfileResult> =>
90+
postOnboardingProfile(onboardingProfileAnswersSchema.parse(answers));
5091

5192
// A skip is a fact worth storing: it separates "declined" from "never
5293
// shown" in the funnel.
53-
export const skipOnboardingProfile = async () =>
54-
postOnboardingProfile({ skipped: true });
94+
export const skipOnboardingProfile =
95+
async (): Promise<OnboardingProfileResult> =>
96+
postOnboardingProfile({ skipped: true });
5597

5698
// Whether the tenant already went through the step on any device. `undefined`
5799
// means the read failed; the gate fails open and does not force the modal.

ui/app/(prowler)/layout.tsx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { ReactNode, Suspense } from "react";
77
import { isOnboardingProfileRecorded } from "@/actions/onboarding/profile";
88
import { getProviders } from "@/actions/providers";
99
import { getScansByState } from "@/actions/scans/scans";
10+
import { auth } from "@/auth.config";
1011
import MainLayout from "@/components/layout/main-layout/main-layout";
1112
import {
1213
OnboardingCheckpointWatcher,
@@ -66,6 +67,9 @@ export default async function RootLayout({
6667
let hasProviders: boolean | undefined = false;
6768
// Same tri-state for the onboarding profile step; only new tenants pay the read.
6869
let profileRecorded: boolean | undefined = true;
70+
// Scopes the step's local marker, so answering for one tenant does not
71+
// silence it for another.
72+
let tenantId: string | null = null;
6973

7074
if (cloudEnabled) {
7175
const [providersData, scansByState] = await Promise.all([
@@ -82,7 +86,12 @@ export default async function RootLayout({
8286
? providersData.data.length > 0
8387
: undefined;
8488
if (hasProviders === false) {
85-
profileRecorded = await isOnboardingProfileRecorded();
89+
const [recorded, session] = await Promise.all([
90+
isOnboardingProfileRecorded(),
91+
auth(),
92+
]);
93+
profileRecorded = recorded;
94+
tenantId = session?.tenantId ?? null;
8695
}
8796
}
8897

@@ -113,6 +122,7 @@ export default async function RootLayout({
113122
<OnboardingProfileGate
114123
hasProviders={hasProviders}
115124
profileRecorded={profileRecorded}
125+
tenantId={tenantId}
116126
/>
117127
{/* Single mount point so the watcher survives post-connect navigation. */}
118128
<OnboardingCheckpointWatcher />

0 commit comments

Comments
 (0)