diff --git a/.secretlintrc.json b/.secretlintrc.json index 468e4653393..80925fb41fa 100644 --- a/.secretlintrc.json +++ b/.secretlintrc.json @@ -49,7 +49,8 @@ "/69010382388f9de5869ad6e558/", "/process\\.env\\./", "/this\\._settingsCache\\.get/", - "/this\\.options\\.password/" + "/this\\.options\\.password/", + "/signupDetails\\.password/" ] }, { diff --git a/apps/admin-x-framework/src/api/members.ts b/apps/admin-x-framework/src/api/members.ts index fccbf61a50a..05a76f04405 100644 --- a/apps/admin-x-framework/src/api/members.ts +++ b/apps/admin-x-framework/src/api/members.ts @@ -128,10 +128,10 @@ export type Member = { }; last_seen_at: string | null; last_commented_at: string | null; - // Custom field values keyed by field key, present only when requested via - // `include=custom_fields` (behind the `membersCustomFields` flag). Values - // are type-dependent: string for text-backed fields, an object for - // composites like address — hence `unknown`; consumers narrow per field type. + // Values of the custom fields a publisher has defined on member records, keyed by + // field. Optional because a site that has defined none gets no key at all, rather than + // an empty object. Values differ by field type, a string for text and an object for an + // address, so they arrive as unknown and each consumer narrows to what it expects. custom_fields?: Record; can_comment?: boolean; commenting?: { @@ -545,8 +545,9 @@ export interface EditMemberData { tiers?: Array<{ id: string; expiry_at?: string | null }>; // Merge semantics: only the keys present are written; `null` clears a // value. The value union is derived from the shared schemas, so a field type - // added there is writable here without this line being edited. Requires the - // `membersCustomFields` flag server-side. + // added there is writable here without this line being edited. Every key is checked + // against the fields the site has defined, so naming one that does not exist is + // rejected rather than ignored. custom_fields?: Record; } diff --git a/apps/admin/src/members/components/bulk-action-modals/import-members-gate.tsx b/apps/admin/src/members/components/bulk-action-modals/import-members-gate.tsx index 45665a55ffb..b4cbbcf3696 100644 --- a/apps/admin/src/members/components/bulk-action-modals/import-members-gate.tsx +++ b/apps/admin/src/members/components/bulk-action-modals/import-members-gate.tsx @@ -24,7 +24,7 @@ interface ImportMembersGateProps { * to one, until the flag goes and the baseline is deleted. * * Custom fields are not this flag's decision. The redesign ships whether or not custom fields - * exist, and asks `membersCustomFields` itself for whether to offer them. + * exist. */ export function ImportMembersGate(props: ImportMembersGateProps) { const importRedesignEnabled = useFeatureFlag('membersImportRedesign'); diff --git a/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/import-members-modal.tsx b/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/import-members-modal.tsx index 8ea1d1676af..708b94c726f 100644 --- a/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/import-members-modal.tsx +++ b/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/import-members-modal.tsx @@ -50,10 +50,8 @@ import { isImportMembersCompleteResponse, useImportMembers, } from '@tryghost/admin-x-framework/api/members'; -import { - memberCustomFieldCsvColumns, - useBrowseMemberCustomFields, -} from '@tryghost/admin-x-framework/api/member-custom-fields'; +import { memberCustomFieldCsvColumns } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import { useCustomFieldDefinitions } from '@/shared/member-custom-fields/use-definitions'; import { parseCSV } from '@/members/components/bulk-action-modals/import-members/csv'; import { useCallback, useEffect, useLayoutEffect, useMemo, useReducer, useRef } from 'react'; import { useFeatureFlag } from '@tryghost/admin-x-framework/hooks'; @@ -77,39 +75,24 @@ export function ImportMembersModal({ const { mutateAsync: importMembers } = useImportMembers(); const importMemberTier = useFeatureFlag('importMemberTier'); - // Whether custom fields exist at all is their own flag's answer, not this dialog's: the - // redesigned import ships on `membersImportRedesign` and has to be a plain column-to-member-field - // mapper while custom fields are still an experiment. Off, they are absent from every part of - // this file — not fetched, not offered as a target, not creatable. - const customFieldsEnabled = useFeatureFlag('membersCustomFields'); - // Defined custom fields become mapping targets. Browse returns active fields only, which - // are the ones the importer writes to. - const { data: customFieldsData, isError: customFieldsFailed } = useBrowseMemberCustomFields({ - enabled: customFieldsEnabled, - }); + const canCreateCustomFields = useFeatureFlag('membersCustomFields'); + const { data: customFieldsData, isError: customFieldsFailed } = useCustomFieldDefinitions(); // A field created from the mapping step is in here the moment it is created: the create // mutation puts it into the cached list, so there is no window where a row points at a // column the picker cannot name yet. - // - // The flag is asked again rather than left to the disabled query above: disabling stops the - // fetch, not the read, so a cache another screen had warmed would still be served here. const customFieldColumns = useMemo( - () => - customFieldsEnabled - ? memberCustomFieldCsvColumns(customFieldsData?.members_custom_fields ?? []) - : [], - [customFieldsEnabled, customFieldsData], + () => memberCustomFieldCsvColumns(customFieldsData?.members_custom_fields ?? []), + [customFieldsData], ); // The file-reader effect waits for this before its first parse: the custom field // definitions must be loaded or auto-detection would miss custom_fields.* columns on a // fast upload. It flips false -> true once and stays true (a refetch keeps data defined), // so readiness never re-triggers the read. - // Ready, or never going to be. Neither a failed query nor a disabled one has any - // representation in `data`, so waiting on `data` alone leaves the file unparsed and the step - // on a spinner with nothing said — for a query whose only job is to add targets to a list. + // Ready, or never going to be. A failed query has no representation in `data`, so waiting + // on `data` alone leaves the file unparsed and the step on a spinner with nothing said — + // for a query whose only job is to add targets to a list. // Failing it costs the custom fields; blocking on it costs the import. - const customFieldsReady = - !customFieldsEnabled || customFieldsData !== undefined || customFieldsFailed; + const customFieldsReady = customFieldsData !== undefined || customFieldsFailed; // Detection options are read inside the effect through this ref rather than as deps, so // a later refetch of the options can't re-run the read and overwrite a mapping the user // has begun editing. @@ -487,7 +470,7 @@ export function ImportMembersModal({ {(state.status === 'MAPPING' || state.status === 'UPLOADING') && state.fileData !== null && ( = ({ const emailValueSource = useEmailPostValueSource(); const labelValueSource = useLabelValueSource(); const { valueSource: tierValueSource, hasMultipleTiers } = useTierValueSource(); - const customFieldsEnabled = useFeatureFlag('membersCustomFields'); // The archived-inclusive browse, fetched eagerly: this is the query the hydration gate // in Members waits on once a filter names a custom field, and a pill reaches the URL on // the first keystroke — if the gate finds this cache cold it unmounts the whole page to @@ -143,9 +141,7 @@ const MembersFilters: React.FC = ({ // fetching it here is what keeps that wait confined to fresh page loads. Archived // fields ride along so a saved segment on a since-archived field still renders its // read-only pill. - const { data: customFieldsData } = useBrowseMemberCustomFieldsIncludingArchived({ - enabled: customFieldsEnabled, - }); + const { data: customFieldsData } = useCustomFieldDefinitionsIncludingArchived(); const catalogCustomFields = customFieldsData?.members_custom_fields ?? EMPTY_CUSTOM_FIELDS; // The picker offers active fields only. const customFields = useMemo( @@ -181,7 +177,6 @@ const MembersFilters: React.FC = ({ emailTrackOpens, emailTrackClicks, siteTimezone, - customFieldsEnabled, customFields, archivedCustomFields, }); diff --git a/apps/admin/src/members/custom-fields/filter-renderer.tsx b/apps/admin/src/members/custom-fields/filter-renderer.tsx index 69f65b5d497..8718fede17e 100644 --- a/apps/admin/src/members/custom-fields/filter-renderer.tsx +++ b/apps/admin/src/members/custom-fields/filter-renderer.tsx @@ -3,10 +3,8 @@ import { CUSTOM_FIELDS_PREFIX, CUSTOM_FIELD_OPERATORS } from '@/members/member-f import { CUSTOM_FIELD_SET_OPERATORS } from './addressing'; import { FilterSegmentInput, FilterSegmentSelect } from '@tryghost/shade/patterns'; import { createOperatorOptions, listsOperator } from '@/shared/filters'; -import { - memberCustomFieldParts, - useBrowseMemberCustomFieldsIncludingArchived, -} from '@tryghost/admin-x-framework/api/member-custom-fields'; +import { memberCustomFieldParts } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import { useCustomFieldDefinitionsIncludingArchived } from '@/shared/member-custom-fields/use-definitions'; import type { CustomRendererProps } from '@tryghost/shade/patterns'; const CustomFieldFilterRenderer: React.FC> = ({ @@ -17,7 +15,7 @@ const CustomFieldFilterRenderer: React.FC> = ({ onOperatorChange, readOnly, }) => { - const { data } = useBrowseMemberCustomFieldsIncludingArchived(); + const { data } = useCustomFieldDefinitionsIncludingArchived(); const definitions = data?.members_custom_fields ?? []; const fieldKey = (field.key ?? '').slice(CUSTOM_FIELDS_PREFIX.length); diff --git a/apps/admin/src/members/detail/member-custom-fields-field.tsx b/apps/admin/src/members/detail/member-custom-fields-field.tsx index 336a028e3f8..c915e0fb70d 100644 --- a/apps/admin/src/members/detail/member-custom-fields-field.tsx +++ b/apps/admin/src/members/detail/member-custom-fields-field.tsx @@ -25,9 +25,9 @@ import { import { toast } from 'sonner'; import { formatMemberCustomFieldValue, - useBrowseMemberCustomFields, userTypeForField, } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import { useCustomFieldDefinitions } from '@/shared/member-custom-fields/use-definitions'; import { useEditMember } from '@tryghost/admin-x-framework/api/members'; import type { EditableAddressValue, EditableCustomFieldValue } from './member-detail-edit'; import type { MemberCustomField } from '@tryghost/admin-x-framework/api/member-custom-fields'; @@ -304,7 +304,7 @@ const MemberCustomFieldsField: React.FC = ({ customFields, disabled, }) => { - const { data, isLoading } = useBrowseMemberCustomFields(); + const { data, isLoading } = useCustomFieldDefinitions(); const fields = data?.members_custom_fields ?? []; const values = getEditableCustomFieldValues(customFields); const [editingField, setEditingField] = React.useState(null); diff --git a/apps/admin/src/members/detail/member-detail-custom-fields.acceptance.test.tsx b/apps/admin/src/members/detail/member-detail-custom-fields.acceptance.test.tsx index 590abae40be..4b97c8ef49b 100644 --- a/apps/admin/src/members/detail/member-detail-custom-fields.acceptance.test.tsx +++ b/apps/admin/src/members/detail/member-detail-custom-fields.acceptance.test.tsx @@ -11,8 +11,6 @@ import { } from '@test-utils/acceptance'; import { memberDetailScreen } from './member-detail.screen'; -const FLAGS = { labs: { membersCustomFields: true } }; - const FIELDS = [ { key: 'job_title', @@ -41,10 +39,8 @@ const ADDRESS = { line1: '1 Main St', city: 'Berlin', postal_code: '10115', coun /** * The world the member detail screen reads at mount, plus the custom-fields - * definitions. Values ride the member read payload (`custom_fields`), exactly - * as the API returns them when the membersCustomFields flag is on. The world - * is stateful: a PUT's merge patch is applied (null deletes), so the refetch - * a save triggers returns the saved state. + * definitions. The world is stateful: a PUT's merge patch is applied (null deletes), so + * the refetch a save triggers returns the saved state. */ function fakeMemberDetailWorld(m: Member, initialValues: Record) { let current: Record = { ...m }; @@ -80,7 +76,7 @@ describe('Member detail custom fields', () => { it('renders the member’s values as a read-only record, addresses as one line', async () => { const m = member({ name: 'Ada Lovelace' }); fakeMemberDetailWorld(m, { job_title: 'Editor', home_address: ADDRESS }); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); await expect.element(memberDetailScreen.fieldValue('Editor')).toBeVisible(); await expect @@ -95,7 +91,7 @@ describe('Member detail custom fields', () => { it('saves one field through its own editor without touching the page Save', async () => { const m = member({ name: 'Ada Lovelace' }); const editApi = fakeMemberDetailWorld(m, { job_title: 'Editor', company: 'Ghost' }); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); await memberDetailScreen.editFieldButton('Job title').click(); await modal().getByLabelText('Job title').fill('Publisher'); @@ -114,7 +110,7 @@ describe('Member detail custom fields', () => { it('leaves an unsaved page edit intact when a custom field is saved', async () => { const m = member({ name: 'Ada Lovelace' }); const editApi = fakeMemberDetailWorld(m, { job_title: 'Editor' }); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); // Dirty the page draft by editing the name, without saving it. await memberDetailScreen.nameInput().fill('Ada L.'); @@ -139,7 +135,7 @@ describe('Member detail custom fields', () => { it('clears a value by saving an emptied editor (null merge patch)', async () => { const m = member({ name: 'Ada Lovelace' }); const editApi = fakeMemberDetailWorld(m, { job_title: 'Editor' }); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); await memberDetailScreen.editFieldButton('Job title').click(); await modal().getByLabelText('Job title').fill(''); @@ -156,7 +152,7 @@ describe('Member detail custom fields', () => { it('a dirty editor refuses casual dismissal; a pristine one closes freely', async () => { const m = member({ name: 'Ada Lovelace' }); fakeMemberDetailWorld(m, { job_title: 'Editor' }); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); // Dirty: Escape must NOT close it — typed values can't be lost to a // stray key or click; Cancel is the one explicit discard. @@ -177,7 +173,7 @@ describe('Member detail custom fields', () => { it('cancelling the editor discards the edit', async () => { const m = member({ name: 'Ada Lovelace' }); const editApi = fakeMemberDetailWorld(m, { job_title: 'Editor' }); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); await memberDetailScreen.editFieldButton('Job title').click(); await modal().getByLabelText('Job title').fill('Publisher'); @@ -190,7 +186,7 @@ describe('Member detail custom fields', () => { it('saves an address that leaves sub-fields the country does not use empty', async () => { const m = member({ name: 'Ada Lovelace' }); const editApi = fakeMemberDetailWorld(m, {}); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); // Hong Kong has no postal code, so leaving it empty is a complete address // rather than an incomplete one. @@ -212,7 +208,7 @@ describe('Member detail custom fields', () => { it('blocks saving a malformed country code with an inline error, then saves once fixed', async () => { const m = member({ name: 'Ada Lovelace' }); const editApi = fakeMemberDetailWorld(m, {}); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); await memberDetailScreen.editFieldButton('Home address').click(); await modal().getByLabelText('Address line 1').fill('1 Main St'); @@ -256,7 +252,7 @@ describe('Member detail custom fields', () => { }, { status: 422 }, ); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); await memberDetailScreen.editFieldButton('Job title').click(); await modal().getByLabelText('Job title').fill('Editor'); @@ -278,7 +274,7 @@ describe('Member detail custom fields', () => { events: [], meta: { pagination: { page: 1, limit: 5, pages: 1, total: 0, next: null, prev: null } }, }); - await renderAdminApp(`/members/${m.id}`, FLAGS); + await renderAdminApp(`/members/${m.id}`); await expect.element(memberDetailScreen.nameInput()).toBeVisible(); await expect.element(memberDetailScreen.customFieldsSection()).not.toBeInTheDocument(); diff --git a/apps/admin/src/members/detail/member-detail.tsx b/apps/admin/src/members/detail/member-detail.tsx index 78e9a752257..7be17ccbd64 100644 --- a/apps/admin/src/members/detail/member-detail.tsx +++ b/apps/admin/src/members/detail/member-detail.tsx @@ -46,7 +46,6 @@ import { import { toast } from 'sonner'; import { useBrowseNewsletters } from '@tryghost/admin-x-framework/api/newsletters'; import { useBrowseTiers } from '@tryghost/admin-x-framework/api/tiers'; -import { useFeatureFlag } from '@tryghost/admin-x-framework/hooks'; import { useUnsavedChangesGuard } from '@/hooks/use-unsaved-changes-guard'; import type { MemberEditableFields } from './member-detail-edit'; @@ -73,14 +72,10 @@ const MemberDetailPage: React.FC = ({ const backPath = deriveMemberDetailBackPath(location.search); const isCreating = memberId === CREATE_ID; - // Values ride the member payload (`include=custom_fields`) but the include - // only exists behind the flag, so it must not be sent on flag-off sites. - const customFieldsEnabled = useFeatureFlag('membersCustomFields'); - // `include=tiers` mirrors the Ember route so complimentary tiers arrive with the member. const { data, isLoading, error, refetch } = useMember(memberId, { enabled: !!memberId && !isCreating, - searchParams: { include: customFieldsEnabled ? 'tiers,custom_fields' : 'tiers' }, + searchParams: { include: 'tiers' }, defaultErrorHandler: false, }); const member = data?.members?.[0]; @@ -469,7 +464,7 @@ const MemberDetailPage: React.FC = ({ modal, never through this page's Save. Existing members only — the create contract doesn't take values yet, and a value can't exist before its member does. */} - {customFieldsEnabled && member && ( + {member && ( > = [ const NICKNAME_FIELD = { key: 'nickname', name: 'Nickname', - type: 'text', + type: 'short_text', status: 'active', created_at: '2026-08-05T00:00:00.000Z', updated_at: null, @@ -761,11 +761,8 @@ describe('Import members custom fields', () => { await expect.element(importMembersScreen.leaveConfirmationText()).toBeVisible(); }); - // The redesigned dialog ships on its own flag, ahead of custom fields. Off, it has to be a - // plain mapping of columns onto the member fields Ghost already has, with nothing about - // custom fields anywhere in it — including on a site that has some defined. - describe('with custom fields off', () => { - it('offers no custom field, and no way to make one', async () => { + describe('without field management', () => { + it('offers the defined fields, but no way to make another', async () => { const { browseApi } = fakeCustomFieldsWorld([NICKNAME_FIELD]); await renderAdminApp('/members', WITHOUT_CUSTOM_FIELDS); await openMappingStep(); @@ -775,13 +772,10 @@ describe('Import members custom fields', () => { // Exact, or "Email" also matches "Subscribed to emails". await expect.element(importMembersScreen.option('Email', { exact: true })).toBeVisible(); - await expect.element(importMembersScreen.option('Nickname')).not.toBeInTheDocument(); + await expect.element(importMembersScreen.option('Nickname')).toBeVisible(); await expect.element(importMembersScreen.addCustomFieldOption()).not.toBeInTheDocument(); - // Not merely unrendered: the definitions are never asked for. That query also gates the - // first parse of the file, so leaving it enabled and unanswerable would hold the mapping - // step on a spinner — which reaching the table above already rules out. - expect(browseApi.requests).toHaveLength(0); + expect(browseApi.requests.length).toBeGreaterThan(0); }); it('says no field matches a search rather than offering to make one', async () => { diff --git a/apps/admin/src/members/use-member-filter-fields.test.ts b/apps/admin/src/members/use-member-filter-fields.test.ts index d485a178709..a1f8f387809 100644 --- a/apps/admin/src/members/use-member-filter-fields.test.ts +++ b/apps/admin/src/members/use-member-filter-fields.test.ts @@ -261,7 +261,6 @@ describe('useMemberFilterFields', () => { it('gives each defined custom field its own named entry', () => { const { result } = renderHook(() => useMemberFilterFields({ - customFieldsEnabled: true, customFields: [ { key: 'shipping_address', name: 'Shipping address', type: 'address' }, { key: 'job_title', name: 'Job title', type: 'short_text' }, @@ -282,7 +281,6 @@ describe('useMemberFilterFields', () => { it('omits the custom fields group when no fields are defined', () => { const { result } = renderHook(() => useMemberFilterFields({ - customFieldsEnabled: true, customFields: [], siteTimezone: 'UTC', }), @@ -291,14 +289,8 @@ describe('useMemberFilterFields', () => { expect(result.current.map((group) => group.group)).not.toContain('Custom fields'); }); - it('omits the custom fields group when the flag is off', () => { - const { result } = renderHook(() => - useMemberFilterFields({ - customFieldsEnabled: false, - customFields: [{ key: 'job_title', name: 'Job title', type: 'short_text' }], - siteTimezone: 'UTC', - }), - ); + it('omits the custom fields group when none are passed at all', () => { + const { result } = renderHook(() => useMemberFilterFields({ siteTimezone: 'UTC' })); expect(result.current.map((group) => group.group)).not.toContain('Custom fields'); }); diff --git a/apps/admin/src/members/use-member-filter-fields.ts b/apps/admin/src/members/use-member-filter-fields.ts index 5d7f002839e..058a7f76137 100644 --- a/apps/admin/src/members/use-member-filter-fields.ts +++ b/apps/admin/src/members/use-member-filter-fields.ts @@ -41,7 +41,6 @@ interface UseMemberFilterFieldsOptions { membersTrackSources?: boolean; emailTrackOpens?: boolean; emailTrackClicks?: boolean; - customFieldsEnabled?: boolean; customFields?: Array<{ key: string; name: string; type: MemberCustomField['type'] }>; // Archived fields still referenced by the current filter. Rendered as disabled, // removable-only pills so a saved segment stays visible and undoable even though @@ -281,7 +280,6 @@ export function useMemberFilterFields({ membersTrackSources = false, emailTrackOpens = false, emailTrackClicks = false, - customFieldsEnabled = false, customFields = NO_CUSTOM_FIELDS, archivedCustomFields = NO_ARCHIVED_CUSTOM_FIELDS, siteTimezone = 'UTC', @@ -407,56 +405,54 @@ export function useMemberFilterFields({ // for "Shipping address" directly rather than reaching it through a generic // "Custom field" door. A simple field filters on its value; a composite field's // renderer opens its parts (plus "Any") in the pill. - if (customFieldsEnabled) { - const customFieldFields = customFields.map((field) => - createFieldConfig(`custom_fields.${field.key}`, { - label: field.name, - // The dropdown entry and the added filter show the field type's own icon - // rather than a generic custom-field mark. - icon: React.createElement(CustomFieldIcon, { type: field.type, className: 'size-4' }), - // The field's type decides its parts and operators, so the operator - // control lives in the renderer, after any part is chosen. - renderOperatorInValue: true, - customRenderer: (props) => - React.createElement( - CustomFieldFilterRenderer, - props as React.ComponentProps, - ), - }), - ); + const customFieldFields = customFields.map((field) => + createFieldConfig(`custom_fields.${field.key}`, { + label: field.name, + // The dropdown entry and the added filter show the field type's own icon + // rather than a generic custom-field mark. + icon: React.createElement(CustomFieldIcon, { type: field.type, className: 'size-4' }), + // The field's type decides its parts and operators, so the operator + // control lives in the renderer, after any part is chosen. + renderOperatorInValue: true, + customRenderer: (props) => + React.createElement( + CustomFieldFilterRenderer, + props as React.ComponentProps, + ), + }), + ); - // An archived field the current filter still references: a disabled, - // removable-only pill with an archive icon. Its key is already in the filter, - // so the picker's own de-dup keeps it out of the add-list — it only ever - // renders as an existing pill. - const archivedFieldFields = archivedCustomFields.map((field) => - createFieldConfig(`custom_fields.${field.key}`, { - label: field.name, - icon: React.createElement(LucideIcon.Archive, { className: 'size-4' }), - // Read-only: the operator and value stay visible so the segment reads - // clearly, but the field is gone from the picker, so the pill can only - // be removed, never re-edited. - readOnly: true, - renderOperatorInValue: true, - customRenderer: (props) => - React.createElement( - CustomFieldFilterRenderer, - props as React.ComponentProps, - ), - }), - ); + // An archived field the current filter still references: a disabled, + // removable-only pill with an archive icon. Its key is already in the filter, + // so the picker's own de-dup keeps it out of the add-list — it only ever + // renders as an existing pill. + const archivedFieldFields = archivedCustomFields.map((field) => + createFieldConfig(`custom_fields.${field.key}`, { + label: field.name, + icon: React.createElement(LucideIcon.Archive, { className: 'size-4' }), + // Read-only: the operator and value stay visible so the segment reads + // clearly, but the field is gone from the picker, so the pill can only + // be removed, never re-edited. + readOnly: true, + renderOperatorInValue: true, + customRenderer: (props) => + React.createElement( + CustomFieldFilterRenderer, + props as React.ComponentProps, + ), + }), + ); - const allCustomFieldFields = [...customFieldFields, ...archivedFieldFields]; + const allCustomFieldFields = [...customFieldFields, ...archivedFieldFields]; - // Nothing defined yet means nothing to filter on, so the group stays out of - // the picker entirely rather than showing a section that can't be used. - if (allCustomFieldFields.length > 0) { - groups.push({ - group: 'Custom fields', - fields: allCustomFieldFields, - previewLimit: CUSTOM_FIELDS_PREVIEW_LIMIT, - }); - } + // Nothing defined yet means nothing to filter on, so the group stays out of + // the picker entirely rather than showing a section that can't be used. + if (allCustomFieldFields.length > 0) { + groups.push({ + group: 'Custom fields', + fields: allCustomFieldFields, + previewLimit: CUSTOM_FIELDS_PREVIEW_LIMIT, + }); } if (activeNewsletters.length > 1) { @@ -552,7 +548,6 @@ export function useMemberFilterFields({ fields, emailFiltersEnabled, emailValueSource, - customFieldsEnabled, emailTrackClicks, emailTrackOpens, hasMultipleTiers, diff --git a/apps/admin/src/settings/advanced/labs/private-features.tsx b/apps/admin/src/settings/advanced/labs/private-features.tsx index 66d19bf00ab..3e67973300b 100644 --- a/apps/admin/src/settings/advanced/labs/private-features.tsx +++ b/apps/admin/src/settings/advanced/labs/private-features.tsx @@ -79,9 +79,16 @@ const features: Feature[] = [ }, { title: 'Member custom fields', - description: 'Let admins create and manage custom field definitions for members', + description: + 'Let admins create and manage custom field definitions for members, and choose which field each Stripe checkout answer is stored in', flag: 'membersCustomFields', }, + { + title: 'Stripe checkout collection', + description: + 'Let admins turn on shipping address, phone number and tax number collection for a tier, asked by Stripe checkout and stored against the member', + flag: 'stripeCheckoutCollection', + }, { title: 'Members import redesign', description: diff --git a/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx b/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx index 4957360f34d..27e638e87c8 100644 --- a/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx @@ -47,9 +47,10 @@ function stripeSettings() { }); } -// The harness composes the flag into settings and config; Stripe rides along in settings. +// The harness composes the flags into settings and config; Stripe rides along in settings. +// Collection puts the card on the tier; field management adds the destination pickers. const flagOn = { - labs: { membersCustomFields: true }, + labs: { stripeCheckoutCollection: true, membersCustomFields: true }, boot: { browseSettings: { response: stripeSettings() } }, }; @@ -213,11 +214,8 @@ describe('Tier checkout collection', () => { await expect.element(modal.getByRole('button', { name: 'Saved' })).toBeVisible(); await expect.poll(() => putApi.requests.length).toBe(1); - const sent = ( - putApi.lastRequest?.body as { - tiers_checkout_config: [{ shipping: { collect: boolean; allowed_countries?: string[] } }]; - } - ).tiers_checkout_config[0]; + const sent = (putApi.lastRequest?.body as { tiers_checkout_config: [Record] }) + .tiers_checkout_config[0]; expect(sent).toMatchObject({ shipping: { collect: true, @@ -400,4 +398,72 @@ describe('Tier checkout collection', () => { await page.getByRole('button', { name: 'Leave' }).click(); await expect(settingsScreen.tierDetailModal()).toHaveCount(0); }); + + describe('without field management', () => { + const collectionOnly = { + labs: { stripeCheckoutCollection: true, membersCustomFields: false }, + boot: { browseSettings: { response: stripeSettings() } }, + }; + + it('shows the toggles and no destination pickers', async () => { + checkoutWorld(); + await renderAdminApp('/settings', collectionOnly); + + const modal = await openSupporterModal(); + await expect.element(modal.getByLabelText('Collect shipping address')).toBeVisible(); + await modal.getByLabelText('Collect shipping address').click(); + await expect(modal.getByLabelText('Save address as')).toHaveCount(0); + await expect(modal.getByLabelText('Save recipient name as')).toHaveCount(0); + }); + + it('saves the port default keys, with nothing to validate', async () => { + const putApi = checkoutWorld(); + await renderAdminApp('/settings', collectionOnly); + + const modal = await openSupporterModal(); + await expect.element(modal.getByLabelText('Collect shipping address')).not.toBeChecked(); + + await modal.getByLabelText('Collect shipping address').click(); + await modal.getByRole('button', { name: 'Save' }).click(); + await expect.element(modal.getByRole('button', { name: 'Saved' })).toBeVisible(); + + // "Saved" is the tier save's signal; the checkout write is chained after it. + await expect.poll(() => putApi.requests.length).toBe(1); + expect( + (putApi.lastRequest?.body as { tiers_checkout_config: [Record] }) + .tiers_checkout_config[0], + ).toMatchObject({ + shipping: { + collect: true, + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }); + }); + + it('keeps a binding that was already chosen', async () => { + const putApi = checkoutWorld([supporterConfig]); + await renderAdminApp('/settings', collectionOnly); + + const modal = await openSupporterModal(); + await expect.element(modal.getByLabelText('Collect shipping address')).toBeChecked(); + + await modal.getByLabelText('Collect phone number').click(); + await modal.getByRole('button', { name: 'Save' }).click(); + await expect.element(modal.getByRole('button', { name: 'Saved' })).toBeVisible(); + + // "Saved" is the tier save's signal; the checkout write is chained after it. + await expect.poll(() => putApi.requests.length).toBe(1); + expect( + (putApi.lastRequest?.body as { tiers_checkout_config: [Record] }) + .tiers_checkout_config[0], + ).toMatchObject({ + shipping: { + collect: true, + name: { custom_field_key: nameField.key }, + address: { custom_field_key: addressField.key }, + }, + }); + }); + }); }); diff --git a/apps/admin/src/settings/membership/tiers/tier-checkout-collection.tsx b/apps/admin/src/settings/membership/tiers/tier-checkout-collection.tsx index 3169ae9e394..a97cfb7d178 100644 --- a/apps/admin/src/settings/membership/tiers/tier-checkout-collection.tsx +++ b/apps/admin/src/settings/membership/tiers/tier-checkout-collection.tsx @@ -30,7 +30,11 @@ import { type StripePort, } from '@tryghost/checkout'; import { JSONError, getErrorMessage } from '@tryghost/admin-x-framework/errors'; -import { type ErrorMessages, useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { + type ErrorMessages, + useFeatureFlag, + useHandleError, +} from '@tryghost/admin-x-framework/hooks'; import { Text } from '@tryghost/shade/primitives'; import { type MemberCustomField, @@ -94,6 +98,25 @@ const refusedDestination = (error: unknown): string | undefined => { return port && isStripePort(port) ? DESTINATION_ERROR[port] : undefined; }; +/** + * Which custom field a value collected during checkout gets saved into. This covers only + * the three values Stripe collects itself: the recipient's name, their address, and their + * phone number. Questions the publisher adds to the checkout are not covered, and are + * refused unless the field they name already exists. + * + * A publisher allowed to manage custom fields gets a picker for each of those three values + * in the tier's settings, and their choice is used. A publisher who is not allowed gets no + * picker, so the choice is made here: keep whatever field that value is already saved + * into, and when there is none, use a fixed default field for that value, which the server + * creates on the site if it is missing. + * + * Preferring the field already in use over the default is why the order matters. Without + * it, a publisher who had chosen a field of their own while they could manage fields would + * find their shipping addresses silently saving into a different field the next time + * anyone changed an unrelated setting on that tier. + */ +const destinationFor = (port: StripePort, chosen: string | null) => chosen ?? PORT_FIELD[port].key; + export type TierCheckoutCollectionHandle = { /** * Check the configuration without persisting anything, painting errors inline. Meant to @@ -274,7 +297,8 @@ const TierCheckoutCollection = forwardRef< const { mutateAsync: editCheckoutConfig } = useEditTierCheckoutConfig(); const handleError = useHandleError(); - const { data: fieldsData } = useBrowseMemberCustomFields(); + const canManageFields = useFeatureFlag('membersCustomFields'); + const { data: fieldsData } = useBrowseMemberCustomFields({ enabled: canManageFields }); const allFields = fieldsData?.members_custom_fields ?? []; // What each collected value may be kept in is the server's rule, so it is read from the // shared port table rather than restated here: anything wider would invite a pick the @@ -324,19 +348,21 @@ const TierCheckoutCollection = forwardRef< return undefined; }; if (state.shipping.collect) { - newErrors.shippingField = destinationError( - state.shipping.addressFieldKey, - eligible[STRIPE_PORT.shippingAddress], - ); - newErrors.shippingName = destinationError( - state.shipping.nameFieldKey, - eligible[STRIPE_PORT.shippingName], - ); + if (canManageFields) { + newErrors.shippingField = destinationError( + state.shipping.addressFieldKey, + eligible[STRIPE_PORT.shippingAddress], + ); + newErrors.shippingName = destinationError( + state.shipping.nameFieldKey, + eligible[STRIPE_PORT.shippingName], + ); + } if (state.shipping.countriesMode === 'specific' && !state.shipping.allowedCountries.length) { newErrors.shippingCountries = 'Choose at least one country you deliver to'; } } - if (state.phone.collect) { + if (state.phone.collect && canManageFields) { // Two collections MAY share a destination: writes apply in a fixed order and the // last wins, which is the designed behaviour for shared fields — so no distinctness // rule here, deliberately. @@ -390,15 +416,25 @@ const TierCheckoutCollection = forwardRef< ...(state.shipping.countriesMode === 'specific' ? { allowed_countries: state.shipping.allowedCountries } : {}), - name: { custom_field_key: state.shipping.nameFieldKey! }, - address: { custom_field_key: state.shipping.addressFieldKey! }, + name: { + custom_field_key: destinationFor( + STRIPE_PORT.shippingName, + state.shipping.nameFieldKey, + ), + }, + address: { + custom_field_key: destinationFor( + STRIPE_PORT.shippingAddress, + state.shipping.addressFieldKey, + ), + }, } : { collect: false }, tax_number: { collect: state.taxNumber.collect }, phone: state.phone.collect ? { collect: true, - custom_field_key: state.phone.customFieldKey!, + custom_field_key: destinationFor(STRIPE_PORT.phone, state.phone.customFieldKey), } : { collect: false }, }, @@ -406,7 +442,7 @@ const TierCheckoutCollection = forwardRef< setSavedSerialized(JSON.stringify(effectiveState(state))); return true; } catch (error) { - const blamed = refusedDestination(error); + const blamed = canManageFields ? refusedDestination(error) : undefined; if (blamed) { // Reported without a toast, the way the picker's own create does it: shown in // place, but a refusal only the server can detect still has to reach error @@ -425,7 +461,7 @@ const TierCheckoutCollection = forwardRef< }), // buildErrors and dirty close over render-scope values, so everything they read has // to invalidate the handle. - [dirty, editCheckoutConfig, fieldsData, handleError, state], + [canManageFields, dirty, editCheckoutConfig, fieldsData, handleError, state], ); if (failed) { @@ -525,36 +561,40 @@ const TierCheckoutCollection = forwardRef< )} - { - setErrors((current) => ({ ...current, shippingField: undefined })); - setState((current) => ({ - ...current, - shipping: { ...current.shipping, addressFieldKey: key }, - })); - }} - /> - { - setErrors((current) => ({ ...current, shippingName: undefined })); - setState((current) => ({ - ...current, - shipping: { ...current.shipping, nameFieldKey: key }, - })); - }} - /> + {canManageFields && ( + <> + { + setErrors((current) => ({ ...current, shippingField: undefined })); + setState((current) => ({ + ...current, + shipping: { ...current.shipping, addressFieldKey: key }, + })); + }} + /> + { + setErrors((current) => ({ ...current, shippingName: undefined })); + setState((current) => ({ + ...current, + shipping: { ...current.shipping, nameFieldKey: key }, + })); + }} + /> + + )} )} @@ -572,7 +612,7 @@ const TierCheckoutCollection = forwardRef< })) } /> - {state.phone.collect && ( + {state.phone.collect && canManageFields && ( { - const hasCustomFields = useFeatureFlag('membersCustomFields'); + const hasCheckoutCollection = useFeatureFlag('stripeCheckoutCollection'); const { config: globalConfig, settings } = useGlobalData(); // The read is tier-independent — one browse covers every tier — so it hangs only on // the feature being on. That lets the tiers list warm it before any modal opens, and // means a free-tier or new-tier modal costs at most one cached request. - const fetchWanted = hasCustomFields && checkStripeEnabled(settings || [], globalConfig || {}); + const fetchWanted = + hasCheckoutCollection && checkStripeEnabled(settings || [], globalConfig || {}); const { data, error, isError, isFetching } = useBrowseTiersCheckoutConfig({ enabled: fetchWanted, defaultErrorHandler: false, diff --git a/apps/admin/src/shared/member-custom-fields/use-definitions.ts b/apps/admin/src/shared/member-custom-fields/use-definitions.ts new file mode 100644 index 00000000000..7ba446a1a01 --- /dev/null +++ b/apps/admin/src/shared/member-custom-fields/use-definitions.ts @@ -0,0 +1,22 @@ +import { + useBrowseMemberCustomFields, + useBrowseMemberCustomFieldsIncludingArchived, +} from '@tryghost/admin-x-framework/api/member-custom-fields'; + +/** + * The site's custom field definitions, for screens that display or offer them. + * + * These never raise an error to the user. Admin and Ghost's server deploy separately, so + * Admin may be running against a server old enough not to have this endpoint. Every screen + * using these degrades quietly when the list cannot be fetched: the filter bar offers no + * custom fields, the member page shows no custom fields section, a filtered column falls + * back to an unnamed header, and the import dialog maps onto Ghost's built-in member + * fields alone. None of that is what the publisher came to the screen to do. + */ +export const useCustomFieldDefinitions: typeof useBrowseMemberCustomFields = (options) => + useBrowseMemberCustomFields({ ...options, defaultErrorHandler: false }); + +/** As above, and also lists fields the publisher has archived. */ +export const useCustomFieldDefinitionsIncludingArchived: typeof useBrowseMemberCustomFieldsIncludingArchived = + (options) => + useBrowseMemberCustomFieldsIncludingArchived({ ...options, defaultErrorHandler: false }); diff --git a/apps/admin/test-utils/acceptance/resources.ts b/apps/admin/test-utils/acceptance/resources.ts index bdef6c49e32..12e6d93bfaf 100644 --- a/apps/admin/test-utils/acceptance/resources.ts +++ b/apps/admin/test-utils/acceptance/resources.ts @@ -255,12 +255,28 @@ const membersResource = defineResource({ * with `fakeAdminEndpoint`. A spec observing the list grow across a create * declares that growth itself via the function form (`() => fields`). */ -export const fakeMemberCustomFields = defineResource({ +const memberCustomFieldsResource = defineResource({ resource: 'members/custom_fields', envelopeKey: 'members_custom_fields', semantics: { kind: 'passthrough' }, }); +// Whether a spec declared its own definitions. `fakeMembers` serves an empty list on +// behalf of the many specs that never mention custom fields, and handlers registered +// later win, so seeding unconditionally would silently replace a list the spec had +// already declared — and its capture would then never see a request. +let memberCustomFieldsDeclared = false; + +export const fakeMemberCustomFields: typeof memberCustomFieldsResource = (respondWith) => { + memberCustomFieldsDeclared = true; + return memberCustomFieldsResource(respondWith); +}; + +/** Called by the harness between tests, alongside the fake API reset. */ +export function resetDeclaredResources(): void { + memberCustomFieldsDeclared = false; +} + // Members-page chrome: the filter bar mounts with the page and probes these lookups. const labelsResource = defineResource