Skip to content

Commit 4d2b24b

Browse files
markgohoclaude
markgoho
and
claude
committed
fix: harden legacy membership draft flow
Prevent false-success draft states, surface rebuild issues as warnings, and add UI coverage for the legacy membership draft action. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 5b5dc46 commit 4d2b24b

14 files changed

Lines changed: 198 additions & 41 deletions

‎functions/src/admin-unclaimed-profiles-api/routes/draft-unclaimed-profile.test.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ describe("POST /:email/draft", () => {
2626
({ email: requestEmail }: { email: string }): Promise<{
2727
success: true;
2828
slug: string;
29-
rebuildTriggered: boolean;
29+
warning?: string;
3030
}> => {
3131
if (profileNotFound || requestEmail === "nonexistent@example.com") {
3232
return Promise.reject(
@@ -45,7 +45,10 @@ describe("POST /:email/draft", () => {
4545
return Promise.resolve({
4646
success: true,
4747
slug: "test-slug",
48-
rebuildTriggered,
48+
...(!rebuildTriggered && {
49+
warning:
50+
"Profile was set to draft, but the site rebuild did not trigger. The change may not appear immediately.",
51+
}),
4952
});
5053
},
5154
);

‎functions/src/admin-unclaimed-profiles-api/routes/draft-unclaimed-profile.ts‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { ERROR_IDS } from "../../constants/error-ids.js";
22
import type { Logger } from "../../shared-api/types/logger.js";
33
import { handleRouteError } from "../../shared-api/utils/route-error-handler.js";
4+
import type { DraftUnclaimedProfileResponse } from "../schemas/unclaimed-profile-schemas.js";
45
import type { UnclaimedProfileAdminService } from "../services/interface.js";
56

67
export async function draftUnclaimedProfileLogic({
@@ -15,7 +16,7 @@ export async function draftUnclaimedProfileLogic({
1516
unclaimedProfileAdminService: UnclaimedProfileAdminService;
1617
logger: Logger;
1718
set: { status?: number | string };
18-
}): Promise<{ success: true; slug: string; warning?: string } | { error: string }> {
19+
}): Promise<DraftUnclaimedProfileResponse> {
1920
try {
2021
logger.info("Admin draft unclaimed profile request", {
2122
adminUid,
@@ -31,19 +32,10 @@ export async function draftUnclaimedProfileLogic({
3132
adminUid,
3233
email,
3334
slug: result.slug,
34-
rebuildTriggered: result.rebuildTriggered,
35+
warning: result.warning,
3536
});
3637

37-
return {
38-
success: true,
39-
slug: result.slug,
40-
...(result.rebuildTriggered
41-
? {}
42-
: {
43-
warning:
44-
"Profile was set to draft, but the site rebuild did not trigger. The change may not appear immediately.",
45-
}),
46-
};
38+
return result;
4739
} catch (error: unknown) {
4840
return handleRouteError({
4941
error,

‎functions/src/admin-unclaimed-profiles-api/services/draft-unclaimed-profile.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,12 @@ import {
99
ValidationError,
1010
} from "../../shared-api/errors/http-error.js";
1111
import type { Logger } from "../../shared-api/types/logger.js";
12+
import type { DraftUnclaimedProfileSuccessResponse } from "../schemas/unclaimed-profile-schemas.js";
1213

1314
export async function draftUnclaimedProfile(options: {
1415
email: string;
1516
logger: Logger;
16-
}): Promise<{ success: true; slug: string; rebuildTriggered: boolean }> {
17+
}): Promise<DraftUnclaimedProfileSuccessResponse> {
1718
const { email, logger } = options;
1819

1920
try {
@@ -57,6 +58,7 @@ export async function draftUnclaimedProfile(options: {
5758
} catch (rebuildError) {
5859
rebuildTriggered = false;
5960
logger.error("Failed to trigger Hugo rebuild after drafting unclaimed profile", {
61+
errorId: ERROR_IDS.API_HUGO_REBUILD_FAILED,
6062
email,
6163
slug,
6264
error: rebuildError,
@@ -68,15 +70,18 @@ export async function draftUnclaimedProfile(options: {
6870
return {
6971
success: true,
7072
slug,
71-
rebuildTriggered,
73+
...(!rebuildTriggered && {
74+
warning:
75+
"Profile was set to draft, but the site rebuild did not trigger. The change may not appear immediately.",
76+
}),
7277
};
7378
} catch (error) {
7479
if (error instanceof HttpError) {
7580
throw error;
7681
}
7782

7883
logger.error("Failed to draft unclaimed profile", {
79-
errorId: ERROR_IDS.API_FIRESTORE_UPDATE_FAILED,
84+
errorId: ERROR_IDS.API_ADMIN_DRAFT_UNCLAIMED_PROFILE_FAILED,
8085
error,
8186
errorMessage: error instanceof Error ? error.message : "Unknown error",
8287
errorStack: error instanceof Error ? error.stack : undefined,

‎functions/src/admin-unclaimed-profiles-api/services/interface.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export interface UnclaimedProfileAdminService {
3636
draftUnclaimedProfile(options: {
3737
email: string;
3838
logger: Logger;
39-
}): Promise<DraftUnclaimedProfileSuccessResponse & { rebuildTriggered: boolean }>;
39+
}): Promise<DraftUnclaimedProfileSuccessResponse>;
4040

4141
refreshPaymentDates(options: {
4242
logger: Logger;

‎functions/src/admin-unclaimed-profiles-api/test-utils/create-admin-test-plugin.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,7 @@ export function createAdminTestPlugin(overrides?: {
4747
Promise.resolve({
4848
success: true,
4949
slug: "test-slug",
50-
rebuildTriggered: true,
51-
} as DraftUnclaimedProfileSuccessResponse & { rebuildTriggered: boolean }),
50+
} as DraftUnclaimedProfileSuccessResponse),
5251
),
5352
refreshPaymentDates: mock(() =>
5453
Promise.resolve({

‎functions/src/profiles-api/services/profile-store/draft-profile.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { getFirestore } from "firebase-admin/firestore";
22
import { logger } from "firebase-functions/v2";
33
import { PROFILES_COLLECTION } from "../../../collections/index.js";
44
import { ERROR_IDS } from "../../../constants/error-ids.js";
5-
import { HttpError } from "../../../shared-api/errors/http-error.js";
5+
import { HttpError, NotFoundError } from "../../../shared-api/errors/http-error.js";
66
import type { WriteProfileResponse } from "./interface.js";
77

88
/**
@@ -22,8 +22,11 @@ export async function draftProfile(options: {
2222

2323
const existing = await documentReference.get();
2424
if (!existing.exists) {
25-
logger.info("Profile not found in Firestore, skipping draft", { slug });
26-
return { success: true };
25+
logger.warn("Profile not found in Firestore", {
26+
errorId: ERROR_IDS.API_PROFILE_NOT_FOUND,
27+
slug,
28+
});
29+
throw new NotFoundError(`Profile with slug ${slug} not found`);
2730
}
2831

2932
await documentReference.update({

‎members/e2e/pages/admin-unclaimed-profile-detail.page.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ export class AdminUnclaimedProfileDetailPage {
1616

1717
// Actions (reused across tests)
1818
readonly deleteProfileButton: Locator;
19+
readonly draftProfileButton: Locator;
1920
readonly updateEmailButton: Locator;
2021

2122
// Update email form
@@ -27,6 +28,7 @@ export class AdminUnclaimedProfileDetailPage {
2728
readonly loadingText: Locator;
2829
readonly errorMessage: Locator;
2930
readonly successMessage: Locator;
31+
readonly warningMessage: Locator;
3032

3133
constructor(page: Page) {
3234
this.page = page;
@@ -39,6 +41,7 @@ export class AdminUnclaimedProfileDetailPage {
3941
});
4042

4143
this.deleteProfileButton = page.getByRole('button', { name: /Delete Profile/ });
44+
this.draftProfileButton = page.getByRole('button', { name: 'Set Profile to Draft' });
4245
// Update email form
4346
this.updateEmailButton = page.getByRole('button', { name: 'Update Email' });
4447
this.updateEmailInput = page.getByLabel('New Email Address');
@@ -49,6 +52,7 @@ export class AdminUnclaimedProfileDetailPage {
4952
this.loadingText = page.getByText('Loading details...');
5053
this.errorMessage = page.getByRole('alert');
5154
this.successMessage = page.getByRole('status');
55+
this.warningMessage = page.locator('app-alert-banner.warning');
5256
}
5357

5458
async goto(email: string): Promise<void> {
@@ -64,4 +68,10 @@ export class AdminUnclaimedProfileDetailPage {
6468
await this.updateEmailInput.fill(newEmail);
6569
await this.confirmUpdateButton.click();
6670
}
71+
72+
async confirmDraftProfile(): Promise<void> {
73+
await this.draftProfileButton.click();
74+
const dialog = this.page.locator('dialog[open]');
75+
await dialog.getByRole('button', { name: 'Set to Draft' }).click();
76+
}
6777
}

‎members/e2e/tests/admin-unclaimed-profiles.spec.ts‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,4 +563,114 @@ test.describe('Admin Unclaimed Profiles', () => {
563563
await expect(unclaimedProfilePage.updateEmailInput).not.toBeVisible();
564564
await expect(unclaimedProfilePage.updateEmailButton).toBeVisible();
565565
});
566+
567+
test('admin sets profile to draft successfully', async ({ authenticatedAdminPage }) => {
568+
const mockProfile = mockUnclaimedProfiles[0]!;
569+
let draftRequestMade = false;
570+
571+
await authenticatedAdminPage.route(
572+
'**/api/admin/unclaimed-profiles/alice.unclaimed@example.com',
573+
async (route) => {
574+
if (route.request().method() === 'GET') {
575+
await route.fulfill({
576+
status: 200,
577+
contentType: 'application/json',
578+
body: JSON.stringify(mockProfile),
579+
});
580+
return;
581+
}
582+
await route.continue();
583+
},
584+
);
585+
586+
await authenticatedAdminPage.route(
587+
'**/api/admin/unclaimed-profiles/alice.unclaimed@example.com/draft',
588+
async (route) => {
589+
if (route.request().method() === 'POST') {
590+
draftRequestMade = true;
591+
await route.fulfill({
592+
status: 200,
593+
contentType: 'application/json',
594+
body: JSON.stringify({ success: true, slug: 'alice-unclaimed' }),
595+
});
596+
return;
597+
}
598+
await route.continue();
599+
},
600+
);
601+
602+
const unclaimedProfilePage = new AdminUnclaimedProfileDetailPage(authenticatedAdminPage);
603+
await unclaimedProfilePage.goto('alice.unclaimed@example.com');
604+
await unclaimedProfilePage.waitForProfileDetails();
605+
606+
// === Open and confirm draft dialog ===
607+
await expect(unclaimedProfilePage.draftProfileButton).toBeVisible();
608+
await unclaimedProfilePage.confirmDraftProfile();
609+
610+
// === Verify success state ===
611+
await expect(unclaimedProfilePage.successMessage).toBeVisible({ timeout: 5000 });
612+
await expect(unclaimedProfilePage.successMessage).toContainText(
613+
'Profile alice-unclaimed was set to draft.',
614+
);
615+
expect(draftRequestMade).toBe(true);
616+
});
617+
618+
test('admin sees rebuild warning after setting profile to draft', async ({
619+
authenticatedAdminPage,
620+
}) => {
621+
const mockProfile = mockUnclaimedProfiles[0]!;
622+
623+
await authenticatedAdminPage.route(
624+
'**/api/admin/unclaimed-profiles/alice.unclaimed@example.com',
625+
async (route) => {
626+
if (route.request().method() === 'GET') {
627+
await route.fulfill({
628+
status: 200,
629+
contentType: 'application/json',
630+
body: JSON.stringify(mockProfile),
631+
});
632+
return;
633+
}
634+
await route.continue();
635+
},
636+
);
637+
638+
await authenticatedAdminPage.route(
639+
'**/api/admin/unclaimed-profiles/alice.unclaimed@example.com/draft',
640+
async (route) => {
641+
if (route.request().method() === 'POST') {
642+
await route.fulfill({
643+
status: 200,
644+
contentType: 'application/json',
645+
body: JSON.stringify({
646+
success: true,
647+
slug: 'alice-unclaimed',
648+
warning:
649+
'Profile was set to draft, but the site rebuild did not trigger. The change may not appear immediately.',
650+
}),
651+
});
652+
return;
653+
}
654+
await route.continue();
655+
},
656+
);
657+
658+
const unclaimedProfilePage = new AdminUnclaimedProfileDetailPage(authenticatedAdminPage);
659+
await unclaimedProfilePage.goto('alice.unclaimed@example.com');
660+
await unclaimedProfilePage.waitForProfileDetails();
661+
662+
// === Open and confirm draft dialog ===
663+
await expect(unclaimedProfilePage.draftProfileButton).toBeVisible();
664+
await unclaimedProfilePage.confirmDraftProfile();
665+
666+
// === Verify success and warning states ===
667+
await expect(unclaimedProfilePage.successMessage).toBeVisible({ timeout: 5000 });
668+
await expect(unclaimedProfilePage.successMessage).toContainText(
669+
'Profile alice-unclaimed was set to draft.',
670+
);
671+
await expect(unclaimedProfilePage.warningMessage).toBeVisible({ timeout: 5000 });
672+
await expect(unclaimedProfilePage.warningMessage).toContainText(
673+
'Profile was set to draft, but the site rebuild did not trigger. The change may not appear immediately.',
674+
);
675+
});
566676
});

‎members/src/app/admin/api-types/admin-unclaimed-profiles-api.types.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,16 @@ export interface ApiListUnclaimedProfilesResponse {
3232
total: number;
3333
}
3434

35-
export interface ApiDraftUnclaimedProfileResponse {
35+
interface ApiErrorResponse {
36+
error: string;
37+
}
38+
39+
export interface ApiDraftUnclaimedProfileSuccessResponse {
3640
success: true;
3741
slug: string;
3842
warning?: string;
3943
}
44+
45+
export type ApiDraftUnclaimedProfileResponse =
46+
| ApiDraftUnclaimedProfileSuccessResponse
47+
| ApiErrorResponse;

‎members/src/app/admin/services/admin-members.service.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type {
1111
import type { ApiMemberResponse } from '../../api-types/api-member-response';
1212
import type {
1313
ApiDraftUnclaimedProfileResponse,
14+
ApiDraftUnclaimedProfileSuccessResponse,
1415
ApiListUnclaimedProfilesResponse,
1516
ApiUnclaimedProfileResponse,
1617
} from '../api-types/admin-unclaimed-profiles-api.types';
@@ -237,14 +238,16 @@ export class AdminMembersService {
237238
);
238239
}
239240

240-
async draftUnclaimedProfile(email: string): Promise<ApiDraftUnclaimedProfileResponse> {
241+
async draftUnclaimedProfile(email: string): Promise<ApiDraftUnclaimedProfileSuccessResponse> {
241242
// Authorization header added automatically by authInterceptor
242-
return firstValueFrom(
243+
const result = await firstValueFrom(
243244
this.httpClient.post<ApiDraftUnclaimedProfileResponse>(
244245
`/api/admin/unclaimed-profiles/${email}/draft`,
245246
{},
246247
),
247248
);
249+
250+
return assertApiSuccess(result);
248251
}
249252

250253
async refreshPaymentDates(): Promise<{

0 commit comments

Comments
 (0)