From bd57f276cb14143651f8bb5ee2822fdee0f9ff40 Mon Sep 17 00:00:00 2001 From: Luis Azevedo Date: Wed, 9 Sep 2026 11:33:15 +0100 Subject: [PATCH 01/17] Updated newsletter sending status copy in post analytics (#30620) ref https://linear.app/ghost/issue/BER-3946/update-newsletter-sending-status-copy The pending-send card shown on the Overview and Newsletter tabs while an email is going out read "This newsletter is still sending". The "still" gives the message a negative tone for what is a normal in-progress state. Replaced it with "Your newsletter is being sent", which matches the tone of the sending banner and the post-publish success modal. --- apps/admin/src/posts/analytics/newsletter/newsletter.tsx | 2 +- .../analytics/overview/components/newsletter-overview.tsx | 2 +- .../src/posts/analytics/post-analytics.acceptance.test.tsx | 6 ++---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/apps/admin/src/posts/analytics/newsletter/newsletter.tsx b/apps/admin/src/posts/analytics/newsletter/newsletter.tsx index 11024689f49..2092af41416 100644 --- a/apps/admin/src/posts/analytics/newsletter/newsletter.tsx +++ b/apps/admin/src/posts/analytics/newsletter/newsletter.tsx @@ -438,7 +438,7 @@ const Newsletter: React.FC = () => {
= ({
diff --git a/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx b/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx index 0402493caf2..e55ade25df5 100644 --- a/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx +++ b/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx @@ -238,7 +238,7 @@ describe('Post analytics overview', () => { await expect.element(page.getByText('Sending emails')).toBeVisible(); await expect.element(page.getByText(/500 of 1,000/)).toBeVisible(); - await expect.element(page.getByText('This newsletter is still sending')).toBeVisible(); + await expect.element(page.getByText('Your newsletter is being sent')).toBeVisible(); await expect.element(postAnalyticsScreen.uniqueVisitors()).toHaveTextContent('250'); await postAnalyticsScreen.newsletterTab().click(); @@ -495,9 +495,7 @@ describe('Post analytics overview', () => { }); await expect.element(page.getByText('Newsletter performance')).toBeVisible(); - await expect - .element(page.getByText('This newsletter is still sending')) - .not.toBeInTheDocument(); + await expect.element(page.getByText('Your newsletter is being sent')).not.toBeInTheDocument(); await expect.element(postAnalyticsScreen.emailSendingStatusBanner()).not.toBeInTheDocument(); await expect.poll(() => statusApi.requests.length).toBe(1); await app.unmount(); From b74ac8e3763a9f8512300cf0740f612f40c12fcb Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Wed, 9 Sep 2026 05:37:11 -0700 Subject: [PATCH 02/17] Fixed automation email analytics filtering (#30430) closes [NY-1537](https://linear.app/ghost/issue/NY-1537/) closes [NY-1535](https://linear.app/ghost/issue/NY-1535/) ## Context Automation emails were switched to the bulk Mailgun domain in [#29728](). As part of that work, responsibility for applying `bulkEmail:mailgun:tag` (ex: `blog-{site_id}`) moved from the shared Mailgun client to individual callers. Newsletter sends continued applying the configured site tag, but automation sends bypassed that provider and initially sent only the `automation-email` classification. Automation analytics had originally been introduced in [#29333]() with a filter containing only `automation-email`. [#30093]() restored the configured site tag on outgoing automation emails. Automation analytics intentionally continued querying only `automation-email` during the rollout window so events from previously sent emails would not be excluded. That rollout window has now passed. This change completes the transition by querying automation events using both `automation-email` and the configured site tag, preventing sites on shared Mailgun domains from fetching one another's automation events. ### Relevant commits * [`58244a1`]() switched automation emails to the bulk Mailgun domain and moved configured tag handling out of the shared Mailgun client. * [`f84ed756`]() introduced automation email analytics using the `automation-email` filter. * [`d64be44f`]() restored the configured site tag on outgoing automation emails while leaving analytics filtering unchanged for the transition period. ## Testing Manually tested, and confirmed that automation email analytics jobs include the `bulkEmail:mailgun:tag` in the query (via a temporary console log), and confirmed that email opens still populate for automations. --- .../server/services/email-analytics/index.ts | 9 ++++++--- .../services/email-analytics/index.test.ts | 20 ++++++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/ghost/core/core/server/services/email-analytics/index.ts b/ghost/core/core/server/services/email-analytics/index.ts index c581799c971..a7c4cd8173e 100644 --- a/ghost/core/core/server/services/email-analytics/index.ts +++ b/ghost/core/core/server/services/email-analytics/index.ts @@ -96,8 +96,11 @@ export const init = ({ }); const newsletterMailgunTags = ['bulk-email']; - if (config.get('bulkEmail:mailgun:tag')) { - newsletterMailgunTags.push(config.get('bulkEmail:mailgun:tag')); + const automationMailgunTags = [AUTOMATION_EMAIL_TAG]; + const mailgunTagFromConfig = config.get('bulkEmail:mailgun:tag'); + if (mailgunTagFromConfig) { + newsletterMailgunTags.push(mailgunTagFromConfig); + automationMailgunTags.push(mailgunTagFromConfig); } prometheusClient?.registerCounter({ @@ -141,7 +144,7 @@ export const init = ({ domainEvents, event: StartAutomationEmailAnalyticsJobEvent, queries, - mailgunTags: [AUTOMATION_EMAIL_TAG], + mailgunTags: automationMailgunTags, jobNames: { latestNonOpened: 'email-analytics-automation-latest-others', missing: 'email-analytics-automation-missing', diff --git a/ghost/core/test/unit/server/services/email-analytics/index.test.ts b/ghost/core/test/unit/server/services/email-analytics/index.test.ts index a949d5f9e7c..dea9e0f42f3 100644 --- a/ghost/core/test/unit/server/services/email-analytics/index.test.ts +++ b/ghost/core/test/unit/server/services/email-analytics/index.test.ts @@ -16,7 +16,6 @@ describe('email analytics service', function () { trackEmailDeliveredAndOpened: sinon.stub(), }; const config = { get: sinon.stub() }; - config.get.withArgs('bulkEmail:mailgun:tag').returns('custom-mailgun-tag'); const domainEvents = { subscribe: sinon.stub() }; const metrics = { metric: sinon.stub() }; const settingsCache = { get: sinon.stub() }; @@ -29,6 +28,8 @@ describe('email analytics service', function () { let dependencies: Parameters[0]; beforeEach(function () { + config.get.reset(); + config.get.withArgs('bulkEmail:mailgun:tag').returns('custom-mailgun-tag'); newslettersInit = sinon.stub(newsletters, 'init'); automationsInit = sinon.stub(automations, 'init'); giftsInit = sinon.stub(gifts, 'init'); @@ -70,7 +71,7 @@ describe('email analytics service', function () { sinon.restore(); }); - it('initializes newsletter, automation, and gift analytics', function () { + it('initializes newsletter, automation, and gift analytics with configured Mailgun tags', function () { init(dependencies); sinon.assert.calledOnceWithExactly( @@ -110,7 +111,7 @@ describe('email analytics service', function () { event: { name: 'StartAutomationEmailAnalyticsJobEvent', }, - mailgunTags: [AUTOMATION_EMAIL_TAG], + mailgunTags: [AUTOMATION_EMAIL_TAG, 'custom-mailgun-tag'], jobNames: { latestNonOpened: 'email-analytics-automation-latest-others', missing: 'email-analytics-automation-missing', @@ -159,6 +160,19 @@ describe('email analytics service', function () { ); }); + it('does not add a site tag to automation analytics when none is configured', function () { + config.get.withArgs('bulkEmail:mailgun:tag').returns(undefined); + + init(dependencies); + + sinon.assert.calledOnceWithExactly( + automationsInit, + sinon.match({ + mailgunTags: [AUTOMATION_EMAIL_TAG], + }), + ); + }); + it('registers Prometheus metrics for member stat aggregation', function () { const registerCounter = sinon.stub(); From c6aa1bf367a4b3b928b056728be2c680164b1977 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Wed, 9 Sep 2026 08:19:28 -0500 Subject: [PATCH 03/17] Added the Code injection pane to the React editor's settings sidebar (#30608) no ref --- ...ettings-code-injection.acceptance.test.tsx | 396 ++++++++++++++++++ apps/admin/src/editor/editor.screen.ts | 3 + apps/admin/src/editor/settings/README.md | 20 + .../settings/code-injection-section.tsx | 80 ++++ .../editor/settings/post-settings-sidebar.tsx | 2 + .../testing/test-data/src/selectors/editor.ts | 4 + 6 files changed, 505 insertions(+) create mode 100644 apps/admin/src/editor/editor-settings-code-injection.acceptance.test.tsx create mode 100644 apps/admin/src/editor/settings/code-injection-section.tsx diff --git a/apps/admin/src/editor/editor-settings-code-injection.acceptance.test.tsx b/apps/admin/src/editor/editor-settings-code-injection.acceptance.test.tsx new file mode 100644 index 00000000000..c18344a32ee --- /dev/null +++ b/apps/admin/src/editor/editor-settings-code-injection.acceptance.test.tsx @@ -0,0 +1,396 @@ +import { describe, expect, it } from 'vitest'; +import { userEvent } from 'vitest/browser'; +import { buildLexicalParagraph } from '@tryghost/test-data'; +import { + codeInjectionFootLabel, + codeInjectionHeadLabel, + codeInjectionPageFootLabel, + codeInjectionPageHeadLabel, +} from '@tryghost/test-data/selectors/editor'; + +import { + currentUserResponse, + fakeAdminEndpoint, + fakeMembers, + fakeNewsletters, + fakePages, + fakePosts, + fakeSnippets, + fakeTiers, + post, + renderAdminApp, + staffRole, + unsavedChangesGuarded, + type EndpointCapture, + type StaffRoleName, +} from '@test-utils/acceptance'; +import { editorScreen } from '@/editor/editor.screen'; + +const POST_ID = 'abc123'; +const CURRENT_USER_ID = '1'; +const FLAG_ON = { labs: { editorReact: true } }; +const LOADED_AT = '2026-01-01T00:00:00.000Z'; +const PUBLISHED_AT = '2025-12-01T10:00:00.000Z'; +const ROUTE = new RegExp(`^/posts/${POST_ID}/\\?`); +const PAGE_ROUTE = new RegExp(`^/pages/${POST_ID}/\\?`); +// The panel's own width, and the width the wide pane widens it to. +const PANEL_WIDTH = 350; +const WIDE_PANEL_WIDTH = 500; +const BACK_LABEL = 'Close code injection panel'; +const ROW_LABEL = 'Code injection'; + +// A settings save waits on the engine's queue, so these journeys outlast the default timeout. +const SLOW = 20_000; +const POLL = { timeout: 10_000 }; +// Under the 3s autosave debounce, so only an undebounced field save can satisfy it. +const FIELD_POLL = { timeout: 2_000 }; + +type SavedPost = ReturnType; + +function submittedPost(capture: EndpointCapture): Record { + const body = capture.lastRequest?.body as { posts: Record[] } | undefined; + return body?.posts[0] ?? {}; +} + +function asRole(name: StaffRoleName) { + const me = currentUserResponse(); + me.users[0].roles = [staffRole({ name })]; + return { ...FLAG_ON, boot: { browseMe: { response: me } } }; +} + +function editorChrome() { + fakeSnippets([]); + fakePosts([]); + // The header's publish inputs and preview read these beyond the boot table. + fakeMembers([]); + fakeNewsletters([]); + fakeTiers([]); + fakeAdminEndpoint('GET', /^\/slugs\/post\//, ({ url }) => ({ + slugs: [{ slug: decodeURIComponent(url.split('/slugs/post/')[1].split('/')[0]) }], + })); +} + +/** A post that answers saves the way Ghost does: submitted fields back, fresh token. */ +function fakeSavablePost(overrides: Partial = {}) { + editorChrome(); + let current = post({ + id: POST_ID, + title: 'Hello from React', + slug: 'hello-from-react', + status: 'draft', + lexical: buildLexicalParagraph('Hello from React'), + updated_at: LOADED_AT, + published_at: null, + codeinjection_head: null, + codeinjection_foot: null, + tags: [], + ...overrides, + }); + let saves = 0; + + fakeAdminEndpoint('GET', ROUTE, () => ({ posts: [current] })); + + return fakeAdminEndpoint('PUT', ROUTE, ({ body }) => { + saves += 1; + const submitted = (body as { posts: Partial[] }).posts[0]; + current = { ...current, ...submitted, updated_at: `2026-01-01T00:00:0${saves}.000Z` }; + return { posts: [current] }; + }); +} + +/** The same post fixture served on the pages collection the page editor reads. */ +function fakeSavablePage() { + editorChrome(); + fakePages([]); + const current = post({ + id: POST_ID, + title: 'Hello from React', + slug: 'hello-from-react', + status: 'draft', + lexical: buildLexicalParagraph('Hello from React'), + updated_at: LOADED_AT, + published_at: null, + codeinjection_head: null, + codeinjection_foot: null, + tags: [], + }); + + fakeAdminEndpoint('GET', PAGE_ROUTE, () => ({ pages: [current] })); + fakeAdminEndpoint('PUT', PAGE_ROUTE, () => ({ pages: [current] })); +} + +function sidebarWidthPx(): number { + return editorScreen.settingsSidebar().element().getBoundingClientRect().width; +} + +function headEditor() { + return editorScreen.settingsCodeInjection(codeInjectionHeadLabel); +} + +function footEditor() { + return editorScreen.settingsCodeInjection(codeInjectionFootLabel); +} + +async function openCodeInjection() { + await editorScreen.settingsToggle().click(); + await expect.element(editorScreen.settingsSidebar()).toBeVisible(); + await editorScreen.settingsSubviewRow(ROW_LABEL).click(); + await expect.element(editorScreen.settingsSubviewPane()).toBeVisible(); + await expect.element(headEditor()).toBeVisible(); + await expect.element(footEditor()).toBeVisible(); +} + +/** + * Playwright clears a contenteditable by writing to the DOM, which races + * CodeMirror's reconciliation. Clear through its own keymap first. + */ +async function typeInto(editor: ReturnType, code: string) { + await editor.click(); + await userEvent.keyboard('{ControlOrMeta>}a{/ControlOrMeta}'); + await userEvent.keyboard('{Backspace}'); + await expect.poll(() => editor.element().textContent).toBe(''); + if (!code) { + return; + } + await editor.fill(code); + await expect.poll(() => (editor.element() as HTMLElement).innerText).toBe(code); +} + +/** + * The sidebar's Code injection pane: the header and footer code this post adds + * to the page it renders on. + */ +describe('Post settings code injection', () => { + it( + 'opens the pane over the section list and comes back from it', + async () => { + fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + // The pane replaces the list it was opened from, in a widened panel. + await expect(editorScreen.settingsExcerpt()).toHaveCount(0); + expect(sidebarWidthPx()).toBe(WIDE_PANEL_WIDTH); + + await editorScreen.settingsSubviewBack(BACK_LABEL).click(); + + await expect(editorScreen.settingsSubviewPane()).toHaveCount(0); + await expect.element(editorScreen.settingsExcerpt()).toBeVisible(); + await expect.element(editorScreen.settingsSubviewRow(ROW_LABEL)).toBeVisible(); + // The panel goes back to the width the section list is shown at. + await expect.poll(sidebarWidthPx).toBe(PANEL_WIDTH); + }, + SLOW, + ); + + it( + 'closes the pane on Escape from outside the editors', + async () => { + fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + // Opening a pane leaves the writer on its back button. + await expect.element(editorScreen.settingsSubviewBack(BACK_LABEL)).toHaveFocus(); + await userEvent.keyboard('{Escape}'); + + await expect(editorScreen.settingsSubviewPane()).toHaveCount(0); + await expect.element(editorScreen.settingsSubviewRow(ROW_LABEL)).toBeVisible(); + }, + SLOW, + ); + + it( + 'keeps the pane open on Escape inside an editor', + async () => { + fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + await headEditor().click(); + await userEvent.keyboard('{Escape}'); + + await expect.element(editorScreen.settingsSubviewPane()).toBeVisible(); + await expect.element(headEditor()).toBeVisible(); + }, + SLOW, + ); + + it( + 'moves between the editors on the Tab that follows Escape', + async () => { + fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + await headEditor().click(); + await expect.element(headEditor()).toHaveFocus(); + + // Without Escape first, Tab indents the code rather than leaving. + await userEvent.keyboard('{Escape}'); + await userEvent.tab(); + await expect.element(footEditor()).toHaveFocus(); + + await userEvent.keyboard('{Escape}'); + await userEvent.tab({ shift: true }); + await expect.element(headEditor()).toHaveFocus(); + await expect.element(editorScreen.settingsSubviewPane()).toBeVisible(); + }, + SLOW, + ); + + it( + 'names a page’s editors for a page', + async () => { + fakeSavablePage(); + await renderAdminApp(`/editor/page/${POST_ID}`, FLAG_ON); + await editorScreen.settingsToggle().click(); + await expect.element(editorScreen.settingsSidebar()).toBeVisible(); + await editorScreen.settingsSubviewRow(ROW_LABEL).click(); + + await expect + .element(editorScreen.settingsCodeInjection(codeInjectionPageHeadLabel)) + .toBeVisible(); + await expect + .element(editorScreen.settingsCodeInjection(codeInjectionPageFootLabel)) + .toBeVisible(); + await expect(editorScreen.settingsCodeInjection(codeInjectionHeadLabel)).toHaveCount(0); + }, + SLOW, + ); + + it( + 'shows the code the post was saved with', + async () => { + const head = ''; + const foot = ''; + fakeSavablePost({ codeinjection_head: head, codeinjection_foot: foot }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + await expect.poll(() => (headEditor().element() as HTMLElement).innerText).toBe(head); + await expect.poll(() => (footEditor().element() as HTMLElement).innerText).toBe(foot); + }, + SLOW, + ); + + it( + 'persists a draft’s header and footer code on the blur that ends each edit', + async () => { + const saveApi = fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + await typeInto(headEditor(), ''); + await footEditor().click(); + + // A field save has no debounce, so it lands well inside the autosave's 3s. + await expect.poll(() => saveApi.requests.length, FIELD_POLL).toBe(1); + expect(submittedPost(saveApi)).toMatchObject({ + codeinjection_head: '', + }); + + await typeInto(footEditor(), ''); + await headEditor().click(); + + await expect.poll(() => saveApi.requests.length, POLL).toBe(2); + expect(submittedPost(saveApi)).toMatchObject({ + codeinjection_foot: '', + }); + // The editors keep what the writer typed once the save is answered. + await expect + .poll(() => (headEditor().element() as HTMLElement).innerText) + .toBe(''); + await expect + .poll(() => (footEditor().element() as HTMLElement).innerText) + .toBe(''); + }, + SLOW, + ); + + it( + 'stages a published post’s header code until Update', + async () => { + const saveApi = fakeSavablePost({ status: 'published', published_at: PUBLISHED_AT }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + await typeInto(headEditor(), ''); + await footEditor().click(); + + await expect.element(editorScreen.updateButton()).toBeEnabled(); + await expect.poll(unsavedChangesGuarded).toBe(true); + expect(saveApi.requests).toHaveLength(0); + + await userEvent.keyboard('{Meta>}s{/Meta}'); + + await expect.poll(() => saveApi.requests.length, POLL).toBe(1); + expect(submittedPost(saveApi)).toMatchObject({ + codeinjection_head: '', + status: 'published', + }); + }, + SLOW, + ); + + it( + 'persists the focused editor when the writer closes the pane', + async () => { + const saveApi = fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + await typeInto(headEditor(), ''); + await expect.element(headEditor()).toHaveFocus(); + await editorScreen.settingsSubviewBack(BACK_LABEL).click(); + + await expect(editorScreen.settingsSubviewPane()).toHaveCount(0); + await expect.poll(() => saveApi.requests.length, FIELD_POLL).toBe(1); + expect(submittedPost(saveApi).codeinjection_head).toBe(''); + + await editorScreen.settingsSubviewRow(ROW_LABEL).click(); + await expect.element(headEditor()).toHaveTextContent(''); + }, + SLOW, + ); + + it.each(['codeinjection_head', 'codeinjection_foot'] as const)( + 'clears saved %s code to null on blur', + async (field) => { + const saveApi = fakeSavablePost({ + codeinjection_head: '', + codeinjection_foot: '', + }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openCodeInjection(); + + const editor = field === 'codeinjection_head' ? headEditor() : footEditor(); + const otherEditor = field === 'codeinjection_head' ? footEditor() : headEditor(); + await typeInto(editor, ''); + await otherEditor.click(); + + await expect.poll(() => saveApi.requests.length, FIELD_POLL).toBe(1); + expect(submittedPost(saveApi)[field]).toBeNull(); + await expect.poll(() => editor.element().textContent).toBe(''); + }, + SLOW, + ); + + it( + 'gives a contributor the pane their role can write', + async () => { + // A contributor may only open a draft they authored. + const saveApi = fakeSavablePost({ authors: [{ id: CURRENT_USER_ID }] }); + await renderAdminApp(`/editor/post/${POST_ID}`, asRole('Contributor')); + await openCodeInjection(); + + await typeInto(headEditor(), ''); + await footEditor().click(); + + await expect + .poll(() => submittedPost(saveApi).codeinjection_head, POLL) + .toBe(''); + }, + SLOW, + ); +}); diff --git a/apps/admin/src/editor/editor.screen.ts b/apps/admin/src/editor/editor.screen.ts index 117a4648e85..3647609fb50 100644 --- a/apps/admin/src/editor/editor.screen.ts +++ b/apps/admin/src/editor/editor.screen.ts @@ -215,6 +215,9 @@ export const editorScreen = { settingsMetaTitle: () => page.getByTestId(settingsMetaTitleInput), settingsMetaDescription: () => page.getByTestId(settingsMetaDescriptionInput), settingsSerpPreview: () => page.getByTestId(settingsSerpPreview), + /** CodeMirror exposes its content as a textbox named by the editor's label. */ + settingsCodeInjection: (label: string) => + page.getByRole('textbox', { name: new RegExp(`^${label}`) }), settingsPostHistory: () => page.getByTestId(settingsPostHistoryButton), postHistoryModal: () => page.getByTestId(postHistoryModal), diff --git a/apps/admin/src/editor/settings/README.md b/apps/admin/src/editor/settings/README.md index d2e72dca654..132d00a33e3 100644 --- a/apps/admin/src/editor/settings/README.md +++ b/apps/admin/src/editor/settings/README.md @@ -392,6 +392,26 @@ left holding the save engine's slug wait. The restore's save carries the slug the post already holds, and the URL section accepts the next manual edit normally. +## Code injection + +The row opens a pane holding the header and footer code this post injects into +the page it renders on, each an HTML editor labelled with the theme helper it +lands in. Every role that can open the panel can write both fields. A page's +editors are named for a page rather than a post. + +The two fields are settings fields like any other: staged as the writer types, +committed on the blur that ends the edit, and persisted or held back by the +panel's save policy. Closing the pane commits the editor the writer was in, and +a field cleared back to empty is stored as no value, as the excerpt is. A post +saved before that convention holds an empty string rather than no value, so +clearing such a field back to empty counts as a change until the next save. + +Escape inside either editor leaves the pane open. An open completion list or a +selection wider than the cursor takes it first; otherwise it frees the editor's +Tab, so the next Tab moves on to the footer editor and out of the pane rather +than indenting. The back button, or Escape from anywhere else in the pane, +still closes the pane. + ## Open and closed The toggle sits in the editor header, and the panel starts closed on every diff --git a/apps/admin/src/editor/settings/code-injection-section.tsx b/apps/admin/src/editor/settings/code-injection-section.tsx new file mode 100644 index 00000000000..aca5f0ccbe1 --- /dev/null +++ b/apps/admin/src/editor/settings/code-injection-section.tsx @@ -0,0 +1,80 @@ +import { CodeEditor } from '@tryghost/shade/components'; +import { LucideIcon } from '@tryghost/shade/utils'; +import type { PostType } from '@/editor/card-config'; +import type { EditorSessionHandle } from '@/editor/session/use-editor-session'; +import { SettingsSubview } from './settings-subview'; + +// An Escape no other binding answers leaves the event unprevented, closing the +// pane. Arm tab-focus mode for the 2s @codemirror/view does, so Tab leaves. +const TAB_FOCUS_ESCAPE = () => + import('@uiw/react-codemirror').then(({ Prec, keymap }) => + Prec.lowest( + keymap.of([ + { + key: 'Escape', + run: (view) => { + view.setTabFocusMode(2_000); + return true; + }, + }, + ]), + ), + ); + +// Loaded on demand so CodeMirror stays out of the editor's main bundle. +const EDITOR_EXTENSIONS = [ + () => import('@codemirror/lang-html').then((module) => module.html()), + TAB_FOCUS_ESCAPE, +]; + +const EDITOR_HEIGHT = '240px'; + +function EditorLabel({ text, helper }: { text: string; helper: string }) { + return ( + <> + {text} {helper} + + ); +} + +export interface CodeInjectionSectionProps { + session: EditorSessionHandle; + postType: PostType; +} + +/** + * The header and footer code this post injects into the page it renders on, + * beside whatever the site already injects. + */ +export function CodeInjectionSection({ session, postType }: CodeInjectionSectionProps) { + const name = postType === 'page' ? 'Page' : 'Post'; + + return ( + } + id="code-injection" + label="Code injection" + title="Code injection" + wide + > + } + value={session.settings.codeinjection_head ?? ''} + onBlur={session.commitSettings} + // A field cleared back to empty is stored as no value, as the excerpt is. + onChange={(value) => session.stageSettings({ codeinjection_head: value || null })} + /> + } + value={session.settings.codeinjection_foot ?? ''} + onBlur={session.commitSettings} + onChange={(value) => session.stageSettings({ codeinjection_foot: value || null })} + /> + + ); +} diff --git a/apps/admin/src/editor/settings/post-settings-sidebar.tsx b/apps/admin/src/editor/settings/post-settings-sidebar.tsx index 6bb8ce74b74..2bae696daf8 100644 --- a/apps/admin/src/editor/settings/post-settings-sidebar.tsx +++ b/apps/admin/src/editor/settings/post-settings-sidebar.tsx @@ -18,6 +18,7 @@ import type { EditorSessionHandle } from '@/editor/session/use-editor-session'; import { AccessSection } from './access-section'; import { PublishDateSection } from './publish-date-section'; import { AuthorsSection } from './authors-section'; +import { CodeInjectionSection } from './code-injection-section'; import { DeleteSection } from './delete-section'; import { MetaDataSection } from './meta-data-section'; import { PostHistorySection } from './post-history-section'; @@ -116,6 +117,7 @@ export function PostSettingsSidebar({ postType === 'page' ? : null, template: , delete: , + 'code-injection': , 'meta-data': , 'post-history': ( Date: Wed, 9 Sep 2026 08:22:00 -0500 Subject: [PATCH 04/17] Added tests and types to LLM discovery middleware (#30616) no ref This change should have no user impact. --- .../{llms-discovery.js => llms-discovery.ts} | 26 +++-- .../web/middleware/llms-discovery.test.ts | 99 +++++++++++++++++++ 2 files changed, 117 insertions(+), 8 deletions(-) rename ghost/core/core/frontend/web/middleware/{llms-discovery.js => llms-discovery.ts} (55%) create mode 100644 ghost/core/test/unit/frontend/web/middleware/llms-discovery.test.ts diff --git a/ghost/core/core/frontend/web/middleware/llms-discovery.js b/ghost/core/core/frontend/web/middleware/llms-discovery.ts similarity index 55% rename from ghost/core/core/frontend/web/middleware/llms-discovery.js rename to ghost/core/core/frontend/web/middleware/llms-discovery.ts index 015865b4f12..d620fb14398 100644 --- a/ghost/core/core/frontend/web/middleware/llms-discovery.js +++ b/ghost/core/core/frontend/web/middleware/llms-discovery.ts @@ -1,12 +1,20 @@ -const onHeaders = require('on-headers'); +import type * as http from 'node:http'; +import onHeaders from 'on-headers'; -function appendHeaderValue(existingValue, newValue) { +type SettingsCache = { + get: (key: 'is_private' | 'llms_enabled') => unknown; +}; + +function appendHeaderValue( + existingValue: http.OutgoingHttpHeader | undefined, + newValue: string, +): string { if (!existingValue) { return newValue; } - const raw = Array.isArray(existingValue) ? existingValue : [existingValue]; - const values = raw.flatMap((v) => v.split(',').map((s) => s.trim())); + const raw = Array.isArray(existingValue) ? existingValue : [String(existingValue)]; + const values = raw.flatMap((value) => value.split(',').map((part) => part.trim())); if (values.includes(newValue)) { return raw.join(', '); @@ -15,12 +23,16 @@ function appendHeaderValue(existingValue, newValue) { return raw.concat(newValue).join(', '); } -function createLlmsDiscovery({ settingsCache }) { +export function createLlmsDiscovery({ settingsCache }: { settingsCache: SettingsCache }) { function isDiscoveryEnabled() { return !settingsCache.get('is_private') && settingsCache.get('llms_enabled') !== false; } - return function llmsDiscovery(req, res, next) { + return function llmsDiscovery( + req: http.IncomingMessage, + res: http.ServerResponse, + next: () => unknown, + ) { if (!isDiscoveryEnabled()) { return next(); } @@ -44,5 +56,3 @@ function createLlmsDiscovery({ settingsCache }) { next(); }; } - -module.exports = { createLlmsDiscovery }; diff --git a/ghost/core/test/unit/frontend/web/middleware/llms-discovery.test.ts b/ghost/core/test/unit/frontend/web/middleware/llms-discovery.test.ts new file mode 100644 index 00000000000..e053d0a1c8f --- /dev/null +++ b/ghost/core/test/unit/frontend/web/middleware/llms-discovery.test.ts @@ -0,0 +1,99 @@ +import assert from 'node:assert/strict'; +import express, { type Express } from 'express'; +import request from 'supertest'; +import { createLlmsDiscovery } from '../../../../../core/frontend/web/middleware/llms-discovery'; + +type Settings = { + is_private?: boolean; + llms_enabled?: boolean; +}; + +describe('LLMs discovery middleware', function () { + function createApp( + settings: Settings = {}, + existingHeaders: Record = {}, + ): Express { + const settingsCache = { + get(key: keyof Settings) { + return settings[key]; + }, + }; + const app = express(); + + app.use(createLlmsDiscovery({ settingsCache })); + app.use((_req, res) => { + for (const [name, value] of Object.entries(existingHeaders)) { + res.setHeader(name, value); + } + res.sendStatus(204); + }); + + return app; + } + + it('adds discovery headers when enabled', async function () { + await request(createApp({ is_private: false, llms_enabled: true })) + .get('/') + .expect(204) + .expect('Link', '; rel="llms-txt", ; rel="llms-full-txt"') + .expect('X-Llms-Txt', '/llms.txt'); + }); + + it('adds discovery headers when llms_enabled is unset', async function () { + await request(createApp({ is_private: false })) + .get('/') + .expect(204) + .expect('X-Llms-Txt', '/llms.txt'); + }); + + it('does not add discovery headers on private sites', async function () { + const response = await request(createApp({ is_private: true, llms_enabled: true })) + .get('/') + .expect(204); + + assert.equal(response.headers.link, undefined); + assert.equal(response.headers['x-llms-txt'], undefined); + }); + + it('does not add discovery headers when disabled', async function () { + const response = await request(createApp({ is_private: false, llms_enabled: false })) + .get('/') + .expect(204); + + assert.equal(response.headers.link, undefined); + assert.equal(response.headers['x-llms-txt'], undefined); + }); + + it('preserves existing header values', async function () { + await request( + createApp( + { is_private: false, llms_enabled: true }, + { + Link: '; rel="alternate"', + 'X-Llms-Txt': '/custom-llms.txt', + }, + ), + ) + .get('/') + .expect(204) + .expect( + 'Link', + '; rel="alternate", ; rel="llms-txt", ; rel="llms-full-txt"', + ) + .expect('X-Llms-Txt', '/custom-llms.txt'); + }); + + it('does not duplicate existing discovery links', async function () { + await request( + createApp( + { is_private: false, llms_enabled: true }, + { + Link: '; rel="llms-txt", ; rel="llms-full-txt"', + }, + ), + ) + .get('/') + .expect(204) + .expect('Link', '; rel="llms-txt", ; rel="llms-full-txt"'); + }); +}); From 13d1d36e1d94a3f7d0110df22aca316b8fa5654c Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 9 Sep 2026 12:02:06 +0100 Subject: [PATCH 05/17] Changed every member read to state who the extra fields are read for ref https://linear.app/ghost/issue/BER-3864/differentiate-public-and-private-member-projections Which of the extra fields a publisher defines about their members come back is about to depend on which side of Ghost is asking, and several callers said nothing and inherited staff as the answer. That is the wrong thing to inherit: a surface that silently loses these fields sends an incomplete payload, which someone notices, while the other default hands a reader fields that were never theirs, which nobody does. Fetching them is now something a caller asks for, and the surfaces that show them say whose view they are showing. Two callers had been quietly relying on the old default. The unsubscribe link, which identifies someone by a signed link rather than a session, was loading every value a member holds onto the request and then discarding all of it behind a fixed list of newsletter preferences. The CSV import and export declared their own narrower view of the fields service with the argument missing altogether, which TypeScript accepts, so it had been arriving as nothing at all. --- ghost/core/content/themes/source | 2 +- .../server/api/endpoints/member-metafields.ts | 13 ++++++---- .../core/core/server/api/endpoints/members.js | 21 ++++++++++++--- .../members-metafields/definitions-service.ts | 6 ++--- .../services/members/import-export/index.ts | 26 ++++++++++++------- .../services/member-bread-service.js | 14 +++++----- .../server/services/members/middleware.js | 8 +++++- .../services/members-bread-service.test.js | 3 ++- 8 files changed, 63 insertions(+), 30 deletions(-) diff --git a/ghost/core/content/themes/source b/ghost/core/content/themes/source index ec174b0fb9c..219dc7dbe62 160000 --- a/ghost/core/content/themes/source +++ b/ghost/core/content/themes/source @@ -1 +1 @@ -Subproject commit ec174b0fb9c4b1dad06e7cf2dfaef63c13fcaabf +Subproject commit 219dc7dbe62bb6dfa8db9fd651528571eae6cfd3 diff --git a/ghost/core/core/server/api/endpoints/member-metafields.ts b/ghost/core/core/server/api/endpoints/member-metafields.ts index d6710fba3dc..508c4ac6a99 100644 --- a/ghost/core/core/server/api/endpoints/member-metafields.ts +++ b/ghost/core/core/server/api/endpoints/member-metafields.ts @@ -1,4 +1,4 @@ -import { actingContext, definitions } from '../../services/members-metafields'; +import { ADMIN, actingContext, definitions } from '../../services/members-metafields'; import { assertDefinable } from '../../services/members-metafields/namespaces'; const permissionsService = require('../../services/permissions'); @@ -46,10 +46,13 @@ const controller = { validation: { options: { namespace: { required: true } } }, permissions: false, query(frame: Frame) { - return definitions!.browse({ - namespace: frame.options.namespace, - filter: frame.options.filter as string | undefined, - }); + return definitions!.browse( + { + namespace: frame.options.namespace, + filter: frame.options.filter as string | undefined, + }, + ADMIN, + ); }, }, diff --git a/ghost/core/core/server/api/endpoints/members.js b/ghost/core/core/server/api/endpoints/members.js index 4951e854294..d02c1e2be6e 100644 --- a/ghost/core/core/server/api/endpoints/members.js +++ b/ghost/core/core/server/api/endpoints/members.js @@ -8,6 +8,7 @@ const membersService = require('../../services/members'); const tpl = require('@tryghost/tpl'); const _ = require('lodash'); const { getCSVExportFileName } = require('./utils/csv-export-filename'); +const { ADMIN } = require('../../services/members-metafields'); // Shape the import service's outcome into the API response envelope: an inline import // reports its stats and label, a deferred one only how much it accepted. @@ -88,7 +89,10 @@ const controller = { }, permissions: true, async query(frame) { - const member = await membersService.api.memberBREADService.read(frame.data, frame.options); + const member = await membersService.api.memberBREADService.read(frame.data, { + ...frame.options, + metafieldsFor: ADMIN, + }); if (!member) { throw new errors.NotFoundError({ @@ -223,7 +227,10 @@ const controller = { }, }); } - let model = await membersService.api.memberBREADService.read({ id: frame.options.id }); + let model = await membersService.api.memberBREADService.read( + { id: frame.options.id }, + { metafieldsFor: ADMIN }, + ); if (!model) { throw new errors.NotFoundError({ message: tpl(messages.memberNotFound), @@ -263,7 +270,10 @@ const controller = { stripe_price_id: frame.data.stripe_price_id, }, }); - let model = await membersService.api.memberBREADService.read({ id: frame.options.id }); + let model = await membersService.api.memberBREADService.read( + { id: frame.options.id }, + { metafieldsFor: ADMIN }, + ); if (!model) { throw new errors.NotFoundError({ message: tpl(messages.memberNotFound), @@ -500,7 +510,10 @@ const controller = { const emailSuppressionList = require('../../services/email-suppression-list'); // Get the member first to retrieve their email - const member = await membersService.api.memberBREADService.read({ id: frame.options.id }, {}); + const member = await membersService.api.memberBREADService.read( + { id: frame.options.id }, + { metafieldsFor: ADMIN }, + ); if (!member) { throw new errors.NotFoundError({ diff --git a/ghost/core/core/server/services/members-metafields/definitions-service.ts b/ghost/core/core/server/services/members-metafields/definitions-service.ts index a8a07569088..a9da940b7b7 100644 --- a/ghost/core/core/server/services/members-metafields/definitions-service.ts +++ b/ghost/core/core/server/services/members-metafields/definitions-service.ts @@ -8,7 +8,7 @@ import { CUSTOM_NAMESPACE } from '@tryghost/metafield-types/identity'; import { metafieldCodec } from './codec'; import { assertDefinable } from './namespaces'; import { FIELD_STATUS, FieldStatusSchema } from './schema'; -import { ADMIN, readableFields, type Audience } from './access'; +import { readableFields, type Audience } from './access'; import { activeFields, fieldByKey, inFieldOrder, type DefinitionQuery } from './queries'; import { KEY_CHARACTERS, mintableKey } from './key'; import { type RecordMetafieldAction, type RequestContext } from './actions'; @@ -135,8 +135,8 @@ export class MetafieldDefinitionsService { } async browse( - options: { namespace?: string; filter?: string } = {}, - audience: Audience = ADMIN, + options: { namespace?: string; filter?: string }, + audience: Audience, ): Promise { if (options.namespace !== undefined && !this.isStored(options.namespace)) { return []; diff --git a/ghost/core/core/server/services/members/import-export/index.ts b/ghost/core/core/server/services/members/import-export/index.ts index cb41420f99d..fda26f2c365 100644 --- a/ghost/core/core/server/services/members/import-export/index.ts +++ b/ghost/core/core/server/services/members/import-export/index.ts @@ -1,6 +1,6 @@ import type { Knex } from 'knex'; import type { CsvField } from '@tryghost/metafield-types/csv'; -import type { WrittenBy } from '../../members-metafields'; +import { INTERNAL, type Audience, type WrittenBy } from '../../members-metafields'; import MembersCSVImporter, { type MembersRepository, type GiftService, @@ -46,9 +46,11 @@ interface ImporterServices { productRepository: unknown; // The metafields services the members service hands the import composition root. metafields: { - definitions: { browse(): Promise }; + definitions: { + browse(options: { namespace?: string }, audience: Audience): Promise; + }; values: { - planWrite(values: Record): Promise; + planWrite(values: Record, audience: Audience): Promise; applyWrite( memberId: string, plan: unknown[], @@ -60,9 +62,14 @@ interface ImporterServices { // The metafields services the members service hands the export composition root. interface MetafieldsServices { - definitions: { browse(): Promise }; + definitions: { + browse(options: { namespace?: string }, audience: Audience): Promise; + }; values: { - getValuesForMembers(memberIds: string[]): Promise>>; + getValuesForMembers( + memberIds: string[], + audience: Audience, + ): Promise>>; }; } @@ -116,8 +123,8 @@ export function makeImporter(deps: ImporterServices) { }; const metafields: MetafieldsImport = { - activeFields: async () => deps.metafields.definitions.browse(), - planWrite: (values) => deps.metafields.values.planWrite(values), + activeFields: async () => deps.metafields.definitions.browse({}, INTERNAL), + planWrite: (values) => deps.metafields.values.planWrite(values, INTERNAL), // Every value the import writes came out of the file, whichever column carried it. // An import has no id to give until runs are tracked, so it names its kind only. applyWrite: (memberId, plan, executor) => @@ -191,8 +198,9 @@ export function makeExporter({ metafields: { // Boot builds the definitions and values services before this one, so they // are always present -- no not-initialised state to guard. - activeDefinitions: async (): Promise => definitions.browse(), - valuesForMembers: (memberIds) => values.getValuesForMembers(memberIds), + activeDefinitions: async (): Promise => + definitions.browse({}, INTERNAL), + valuesForMembers: (memberIds) => values.getValuesForMembers(memberIds, INTERNAL), }, }); diff --git a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js index 2afe7eec208..e849c247144 100644 --- a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js +++ b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js @@ -385,15 +385,17 @@ module.exports = class MemberBREADService { /** * @param {object} data * @param {object} [options] - * @param {import('../../../members-metafields').Audience | null} [options.metafieldsFor] + * @param {import('../../../members-metafields').Audience | null} options.metafieldsFor * Who the extra fields a publisher defined are being read for, or null to leave them * off entirely. Null is not the same as "nobody may see them": it means this caller * never shows them, so fetching them is two database queries whose results are thrown * away. Ghost identifies a signed-in reader on every page view of a themed site * through this method, and that caller renders a member through a fixed list of * fields which has never included these. + * + * Defaults to null, so a caller that does not ask gets none of them. */ - async read(data, { metafieldsFor = ADMIN, ...options } = {}) { + async read(data, { metafieldsFor = null, ...options } = {}) { const defaultWithRelated = [ 'labels', 'stripeSubscriptions', @@ -562,7 +564,7 @@ module.exports = class MemberBREADService { await this.memberRepository.setComplimentarySubscription(model, sharedOptions); } - return this.read({ id: model.id }, options); + return this.read({ id: model.id }, { ...options, metafieldsFor: ADMIN }); } async edit(data, options) { @@ -678,7 +680,7 @@ module.exports = class MemberBREADService { } } - return this.read({ id: model.id }, options); + return this.read({ id: model.id }, { ...options, metafieldsFor: ADMIN }); } /** @@ -710,7 +712,7 @@ module.exports = class MemberBREADService { ); } - return this.read({ id: memberId }); + return this.read({ id: memberId }, { metafieldsFor: ADMIN }); } /** @@ -732,7 +734,7 @@ module.exports = class MemberBREADService { await this.memberRepository.saveCommenting(memberId, updated, 'commenting_enabled', context); - return this.read({ id: memberId }); + return this.read({ id: memberId }, { metafieldsFor: ADMIN }); } async logout(options) { diff --git a/ghost/core/core/server/services/members/middleware.js b/ghost/core/core/server/services/members/middleware.js index 9c23263e9d3..500d5a515d7 100644 --- a/ghost/core/core/server/services/members/middleware.js +++ b/ghost/core/core/server/services/members/middleware.js @@ -243,7 +243,13 @@ const authMemberByUuid = async function authMemberByUuid(req, res, next) { }); } - const member = await membersService.api.memberBREADService.read({ uuid }); + // Nothing reached through this middleware renders a member's extra fields, so + // they are not fetched. It authenticates by a signed link rather than a session, + // which makes anything loaded here readable without being signed in. + const member = await membersService.api.memberBREADService.read( + { uuid }, + { metafieldsFor: null }, + ); if (!member) { throw new errors.UnauthorizedError({ message: tpl(messages.invalidUuid), diff --git a/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js b/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js index 303f7000096..c89fcba90a3 100644 --- a/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js +++ b/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js @@ -3,6 +3,7 @@ const sinon = require('sinon'); const MemberBreadService = require('../../../../../../../core/server/services/members/members-api/services/member-bread-service'); const NextPaymentCalculator = require('../../../../../../../core/server/services/members/members-api/services/next-payment-calculator'); const moment = require('moment'); +const { ADMIN } = require('../../../../../../../core/server/services/members-metafields'); // The custom fields service is a required dependency: boot constructs it before // the members service, so the members service is never without one. Fixtures build @@ -593,7 +594,7 @@ describe('MemberBreadService', function () { const metafieldDefinitions = createMetafieldDefinitionsStub(true); const memberBreadService = getService({ metafieldDefinitions }); - const member = await memberBreadService.read({ id: MEMBER_ID }); + const member = await memberBreadService.read({ id: MEMBER_ID }, { metafieldsFor: ADMIN }); assert.deepEqual(member.metafields, {}); }); From 3f3cd9f3542690cb3fa68b928981c0835c7969ed Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 9 Sep 2026 12:02:06 +0100 Subject: [PATCH 06/17] Added per-field control over what a member may see and change ref https://linear.app/ghost/issue/BER-3864/differentiate-public-and-private-member-projections A publisher collecting a delivery address and an internal note about the same member wants different answers to who may see each, so which of the extra fields they define are open to the member whose record it is now belongs to the field rather than to the feature. A field can be closed, readable, or readable and writable, and it is closed when it is made, including every field that already exists. Nothing infers the setting from a field's name, type or use, and no client that has never heard of it can open one, because the permissive answer here publishes what a site has already collected to the people it was collected about. A field closed to members behaves, to that member, exactly as a field nobody has defined: absent from what they are offered, absent from their own record, and a write naming it refused in the same words a write naming nothing gets. That is not two code paths written to agree. The narrowing happens in the query, so a closed field is never fetched and "unknown field" is the only answer left to give. Fetching everything and dropping what the audience may not have leaves a correct answer one forgotten filter away, and the way that fails is silent. It is also why there is no longer a way to ask for the site's fields without saying who is asking. The filter is unconditional, staff included. They are narrowed to a list that happens to hold every level rather than routed around the clause, so there is no branch to reach by accident and an audience nobody has taught this about narrows to nothing at all. Getting it wrong costs a publisher sight of their own fields, which somebody reports within the hour. The other direction is the one nobody sees. A member is refused differently when they may read a field but not change it, because they are looking at it and telling them it does not exist would be a lie about something on their screen. A site whose every field is closed carries no such key in a member's payload at all, rather than an empty one: an empty bag says a publisher collects something about them without saying what. An access change is recorded in the history with the level it became and the level it was, unlike the other edits, because who could read a member's answers, and from when, gets asked long after the current setting has stopped being the answer. --- .../server/api/endpoints/member-metafields.ts | 12 +- ...add-member-access-to-members-metafields.js | 12 ++ ghost/core/core/server/data/schema/schema.js | 12 ++ .../services/members-metafields/access.ts | 79 ++++---- .../services/members-metafields/actions.ts | 10 +- .../services/members-metafields/codec.ts | 11 +- .../members-metafields/definitions-service.ts | 132 ++++++++----- .../services/members-metafields/index.ts | 10 +- .../services/members-metafields/models.ts | 2 + .../services/members-metafields/queries.ts | 72 +++++-- .../services/members-metafields/schema.ts | 2 + .../members-metafields/serializers.ts | 1 + .../members-metafields/values-service.ts | 50 +++-- .../services/member-bread-service.js | 13 +- .../services/tier-checkout-config/service.ts | 21 +- ghost/core/package.json | 2 +- .../admin/member-custom-fields.test.ts | 88 +++++++++ .../admin/tiers-checkout-config.test.ts | 59 ++++++ .../e2e-api/members/custom-fields.test.ts | 179 +++++++++++++++++- .../unit/server/data/schema/integrity.test.js | 2 +- .../services/members-bread-service.test.js | 6 +- packages/metafield-types/src/index.ts | 18 ++ 22 files changed, 658 insertions(+), 135 deletions(-) create mode 100644 ghost/core/core/server/data/migrations/versions/6.64/2026-09-03-15-40-25-add-member-access-to-members-metafields.js diff --git a/ghost/core/core/server/api/endpoints/member-metafields.ts b/ghost/core/core/server/api/endpoints/member-metafields.ts index 508c4ac6a99..f1ebec6f0ad 100644 --- a/ghost/core/core/server/api/endpoints/member-metafields.ts +++ b/ghost/core/core/server/api/endpoints/member-metafields.ts @@ -8,7 +8,13 @@ interface Frame { // `members_metafields` array before any handler runs, so `edit` can take element 0 without // checking. data: { members_metafields: unknown[] }; - options: { namespace: string; key: string; context: unknown; [key: string]: unknown }; + options: { + namespace: string; + key: string; + filter?: string; + context: unknown; + [key: string]: unknown; + }; } // Reading a definition needs no permission. A definition says only that the site collects @@ -49,7 +55,7 @@ const controller = { return definitions!.browse( { namespace: frame.options.namespace, - filter: frame.options.filter as string | undefined, + filter: frame.options.filter, }, ADMIN, ); @@ -62,7 +68,7 @@ const controller = { validation: { options: { namespace: { required: true }, key: { required: true } } }, permissions: false, query(frame: Frame) { - return definitions!.read(frame.options.namespace, frame.options.key); + return definitions!.read(frame.options.namespace, frame.options.key, ADMIN); }, }, diff --git a/ghost/core/core/server/data/migrations/versions/6.64/2026-09-03-15-40-25-add-member-access-to-members-metafields.js b/ghost/core/core/server/data/migrations/versions/6.64/2026-09-03-15-40-25-add-member-access-to-members-metafields.js new file mode 100644 index 00000000000..5e0471a94fa --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/6.64/2026-09-03-15-40-25-add-member-access-to-members-metafields.js @@ -0,0 +1,12 @@ +const { createAddColumnMigration } = require('../../utils'); + +// Every field that already exists lands closed. A permissive default would publish +// what those sites have already collected to the members it was collected about, on +// deploy, without anyone choosing it. +module.exports = createAddColumnMigration('members_metafields', 'member_access', { + type: 'string', + maxlength: 50, + nullable: false, + defaultTo: 'none', + validations: { isIn: [['none', 'read', 'write']] }, +}); diff --git a/ghost/core/core/server/data/schema/schema.js b/ghost/core/core/server/data/schema/schema.js index d3a7cf78f91..cabfcc2e27c 100644 --- a/ghost/core/core/server/data/schema/schema.js +++ b/ghost/core/core/server/data/schema/schema.js @@ -1054,6 +1054,18 @@ module.exports = { defaultTo: 'active', validations: { isIn: [['active', 'archived']] }, }, + // These validations never run: they are applied by Bookshelf's onValidate hook, + // and this table has no Bookshelf model — the metafields service writes it through + // knex, validating with MEMBER_ACCESS in that service instead. Recorded here to + // describe the column, and duplicated because this static schema cannot import + // TypeScript. + member_access: { + type: 'string', + maxlength: 50, + nullable: false, + defaultTo: 'none', + validations: { isIn: [['none', 'read', 'write']] }, + }, // The publisher's order for the list, rewritten across every row whenever the // list is reordered. Only the relative order carries meaning: creates append past // the highest rank and deletes leave gaps, so the values are not a dense diff --git a/ghost/core/core/server/services/members-metafields/access.ts b/ghost/core/core/server/services/members-metafields/access.ts index e43d5223b32..2d84088f65e 100644 --- a/ghost/core/core/server/services/members-metafields/access.ts +++ b/ghost/core/core/server/services/members-metafields/access.ts @@ -1,44 +1,53 @@ -/** - * Who is asking, in the only terms that decide what they may see or change. - * - * Not which user, but which door they came through. Staff reaching a member - * through the Admin API and a member reaching their own record through Portal are - * asking different questions about the same fields, and the answers will differ - * per field once a publisher can say so. Naming the door gives that decision - * somewhere to live. - * - * `internal` is neither: an importer, a value collected at checkout, a job. Those - * act on nobody's behalf and are bounded by whatever set them off rather than by - * who is looking. - * - * A member's own entry carries no id. Which member is asking is already settled by - * the query, which is scoped to them, so repeating it here would be a second answer - * to a question that is already decided and could disagree with the first. - */ +import { + MEMBER_ACCESS, + MEMBER_ACCESS_LEVELS, + MemberAccessSchema, + type MemberAccess, +} from '@tryghost/metafield-types'; + +// Re-exported so this module stays the one place the rest of the service asks about +// access, whether the answer is the shared vocabulary or the audience rules below. +export { MEMBER_ACCESS, MemberAccessSchema, type MemberAccess }; + +const EVERY_LEVEL: MemberAccess[] = [...MEMBER_ACCESS_LEVELS]; + export type Audience = { entry: 'admin' } | { entry: 'members' } | { entry: 'internal' }; export const ADMIN: Audience = { entry: 'admin' }; export const MEMBERS: Audience = { entry: 'members' }; export const INTERNAL: Audience = { entry: 'internal' }; -/** - * Which of the site's fields this audience may see. - * - * Every field, whichever door they came through. Written as a function rather than - * left unwritten so that when a publisher can mark a field as staff only, one - * function changes and every caller is already asking. - */ -export function readableFields(_audience: Audience, fields: T[]): T[] { - return fields; +// Every audience is spelled out with the levels it may read, staff included, because +// reads narrow on whatever this returns and an audience with no entry gets an empty +// list, which matches no field. Giving staff a way to skip the check instead would +// invert that: an unrecognised audience would then match everything. +const READABLE: Record = { + admin: EVERY_LEVEL, + internal: EVERY_LEVEL, + members: [MEMBER_ACCESS.read, MEMBER_ACCESS.write], +}; + +const WRITABLE: Record = { + admin: EVERY_LEVEL, + internal: EVERY_LEVEL, + members: [MEMBER_ACCESS.write], +}; + +// `Object.hasOwn` rather than a plain lookup: an entry naming a property every object +// inherits (`__proto__`, `constructor`, `toString`) would otherwise return that +// inherited value instead of falling through to the empty list. +function levelsFor( + table: Record, + audience: Audience, +): MemberAccess[] { + const entry = audience?.entry; + return entry !== undefined && Object.hasOwn(table, entry) ? table[entry] : []; +} + +export function readableLevels(audience: Audience): MemberAccess[] { + return levelsFor(READABLE, audience); } -/** - * Whether this audience may write this field. - * - * Also total today. Kept separate from `readableFields` because the asymmetry to - * expect is a field a member may read but not change, which one combined - * permission could not express. - */ -export function canWrite(_audience: Audience, _field: { key: string }): boolean { - return true; +export function canWrite(audience: Audience, field: { memberAccess: MemberAccess }): boolean { + return levelsFor(WRITABLE, audience).includes(field.memberAccess); } diff --git a/ghost/core/core/server/services/members-metafields/actions.ts b/ghost/core/core/server/services/members-metafields/actions.ts index 66049dba0dd..86af2115a2f 100644 --- a/ghost/core/core/server/services/members-metafields/actions.ts +++ b/ghost/core/core/server/services/members-metafields/actions.ts @@ -1,4 +1,5 @@ import logging from '@tryghost/logging'; +import type { MemberAccess } from './access'; export interface Actor { id: string; @@ -39,6 +40,7 @@ const COMMANDS = { create: 'added', rename: 'edited', reorder: 'edited', + changeAccess: 'edited', archive: 'archived', restore: 'restored', delete: 'deleted', @@ -56,7 +58,13 @@ export type MetafieldVerb = keyof typeof COMMANDS; // // A reorder names no field: it carries the count and the word the feed reads it by. export type MetafieldActionDetails = - | { primary_name: string; key: string; previous_name?: string } + | { + primary_name: string; + key: string; + previous_name?: string; + member_access?: MemberAccess; + previous_member_access?: MemberAccess; + } | { action_name: 'reordered'; count: number }; // `subject` is the field's row id, or null for an act that belongs to no single field. diff --git a/ghost/core/core/server/services/members-metafields/codec.ts b/ghost/core/core/server/services/members-metafields/codec.ts index 4018a7a35a0..aed4329149b 100644 --- a/ghost/core/core/server/services/members-metafields/codec.ts +++ b/ghost/core/core/server/services/members-metafields/codec.ts @@ -7,6 +7,13 @@ import { Metafield } from './models'; export const metafieldCodec = z.codec(DbMetafield, Metafield, { // DbMetafield validates `type` as the field-type enum, so the decoded row // already carries a FieldType and camelKeys preserves it — no cast needed. - decode: (row) => ({ ...camelKeys(row), namespace: CUSTOM_NAMESPACE }), - encode: ({ namespace: _namespace, ...field }) => snakeKeys(field), + decode: ({ member_access: memberAccess, ...row }) => ({ + ...camelKeys(row), + namespace: CUSTOM_NAMESPACE, + access: { member: memberAccess }, + }), + encode: ({ namespace: _namespace, access, ...field }) => ({ + ...snakeKeys(field), + member_access: access.member, + }), }); diff --git a/ghost/core/core/server/services/members-metafields/definitions-service.ts b/ghost/core/core/server/services/members-metafields/definitions-service.ts index a9da940b7b7..b70bbd718f6 100644 --- a/ghost/core/core/server/services/members-metafields/definitions-service.ts +++ b/ghost/core/core/server/services/members-metafields/definitions-service.ts @@ -8,8 +8,20 @@ import { CUSTOM_NAMESPACE } from '@tryghost/metafield-types/identity'; import { metafieldCodec } from './codec'; import { assertDefinable } from './namespaces'; import { FIELD_STATUS, FieldStatusSchema } from './schema'; -import { readableFields, type Audience } from './access'; -import { activeFields, fieldByKey, inFieldOrder, type DefinitionQuery } from './queries'; +import { + ADMIN, + MEMBER_ACCESS, + MemberAccessSchema, + type Audience, + type MemberAccess, +} from './access'; +import { + ACTIVE_ONLY, + ANY_STATUS, + definitions, + inFieldOrder, + type DefinitionQuery, +} from './queries'; import { KEY_CHARACTERS, mintableKey } from './key'; import { type RecordMetafieldAction, type RequestContext } from './actions'; @@ -56,10 +68,14 @@ const FieldName = z .min(1, { message: 'Custom field name is required.' }) .max(MAX_NAME_LENGTH, { message: 'Custom field name is too long.' }); -// The backend mints the key from the name, so create takes just a name and type. +const FieldAccess = z.object({ member: MemberAccessSchema }); + +// No key: the backend mints it from the name. + const AddFieldInput = z.object({ name: FieldName, type: FieldTypeSchema, + access: FieldAccess.optional(), }); // A bound on the work one request can ask for, separate from how many definitions @@ -88,11 +104,12 @@ const ReorderInput = z ) .min(1, { message: 'The order must name every custom field.' }); -// Name and status are mutable. `key` and `type` are accepted so the immutability -// rules can reject a change loudly; they are never persisted. +// Name, status and access are mutable. `key` and `type` are accepted so the +// immutability rules can reject a change loudly; they are never persisted. const EditFieldInput = z.object({ name: FieldName.optional(), status: FieldStatusSchema.optional(), + access: FieldAccess.optional(), key: z.string().optional(), type: FieldTypeSchema.optional(), }); @@ -121,8 +138,10 @@ export class MetafieldDefinitionsService { this.getMaxDefinitions = getMaxDefinitions; } - async hasAnyActive(): Promise { - const [field] = await this.list(activeFields(this.knex).limit(1)); + async hasAnyReadable(audience: Audience): Promise { + const [field] = await this.list( + definitions(this.knex, { audience, status: ACTIVE_ONLY, limit: 1 }), + ); return Boolean(field); } @@ -141,32 +160,35 @@ export class MetafieldDefinitionsService { if (options.namespace !== undefined && !this.isStored(options.namespace)) { return []; } - // Archived fields are hidden by default: most surfaces (member details, the - // filter picker, the importer) only ever want active fields. A caller- - // supplied `filter` can widen that — Settings pulls active and archived - // together in one request (`filter=status:[active,archived]`). - // // Whichever set comes back, it comes back in the publisher's order: filtering // narrows the list, it never reorders it. - const query = options.filter - ? applyFilter(this.knex(TABLE), options.filter) - : activeFields(this.knex); - return readableFields(audience, await this.list(query)); + const { filter } = options; + if (filter) { + // A filter naming `status` decides the statuses itself, which is how Settings + // pulls active and archived together in one request + // (`filter=status:[active,archived]`). One that does not gets the same active-only + // scope as an unfiltered read. + const parsed = parseFilter(filter); + return this.list( + definitions(this.knex, { + audience, + status: filterReferencesStatus(parsed) ? ANY_STATUS : ACTIVE_ONLY, + filter: (query) => knexify(query, parsed, { tableName: TABLE }), + }), + ); + } + return this.list(definitions(this.knex, { audience, status: ACTIVE_ONLY })); } - /** - * Decode a definition query into the domain, in the publisher's order. - * - * Typed off `activeFields` so the builder keeps the table's row type: every caller - * hands over a query against the definitions table, whatever it has narrowed. - */ private async list(query: DefinitionQuery): Promise { const rows = await inFieldOrder(query).select('*'); return rows.map((row) => z.decode(metafieldCodec, row)); } - async read(namespace: string, key: string): Promise { - const [field] = this.isStored(namespace) ? await this.list(fieldByKey(this.knex, key)) : []; + async read(namespace: string, key: string, audience: Audience): Promise { + const [field] = this.isStored(namespace) + ? await this.list(definitions(this.knex, { audience, status: ANY_STATUS, key })) + : []; if (!field) { throw new errors.NotFoundError({ message: 'Custom field not found.' }); } @@ -230,6 +252,7 @@ export class MetafieldDefinitionsService { key, name: field.name, type: field.type, + memberAccess: field.access?.member ?? MEMBER_ACCESS.none, sortOrder: firstSortOrder + index, }); keys.push(key); @@ -265,7 +288,7 @@ export class MetafieldDefinitionsService { * which a single-connection pool would deadlock against an open transaction. */ async addOne( - wanted: { key: string; name: string; type: FieldType }, + wanted: { key: string; name: string; type: FieldType; access: z.infer }, { executor = this.knex }: { executor?: Knex } = {}, ): Promise { // Before any database access, the way `add` mints before opening its transaction: @@ -281,6 +304,7 @@ export class MetafieldDefinitionsService { key: wanted.key, name: wanted.name, type: wanted.type, + memberAccess: wanted.access.member, sortOrder: await this.nextSortOrder(db), }); const [created] = await this.readMany(db, [wanted.key]); @@ -307,13 +331,20 @@ export class MetafieldDefinitionsService { private async insertField( db: Knex, - field: { key: string; name: string; type: FieldType; sortOrder: number }, + field: { + key: string; + name: string; + type: FieldType; + memberAccess: MemberAccess; + sortOrder: number; + }, ): Promise { await db(TABLE).insert({ id: new ObjectID().toHexString(), key: field.key, name: field.name, type: field.type, + member_access: field.memberAccess, sort_order: field.sortOrder, created_at: new Date(), }); @@ -416,7 +447,8 @@ export class MetafieldDefinitionsService { .update({ sort_order: ranks.get(key)! }); } - return this.list(trx(TABLE)); + // An order covers the whole list, archived definitions included. + return this.list(definitions(trx, { audience: ADMIN, status: ANY_STATUS })); }); await this.recordAction({ @@ -541,7 +573,7 @@ export class MetafieldDefinitionsService { } const patch = parsed.data; - const existing = await this.read(CUSTOM_NAMESPACE, key); + const existing = await this.read(CUSTOM_NAMESPACE, key, ADMIN); // Key and type are immutable after creation: values are addressed by key // and interpreted by type, so changing either would silently orphan or @@ -583,6 +615,23 @@ export class MetafieldDefinitionsService { }); } + if (patch.access !== undefined && patch.access.member !== existing.access.member) { + await this.knex(TABLE) + .where('key', key) + .update({ member_access: patch.access.member, updated_at: new Date() }); + await this.recordAction({ + context, + verb: 'changeAccess', + subject: existing.id, + details: { + primary_name: patch.name ?? existing.name, + key, + member_access: patch.access.member, + previous_member_access: existing.access.member, + }, + }); + } + // A status change is the archive/restore transition. Only write (and log) // when it actually flips, so re-sending the current status is a no-op. if (patch.status !== undefined && patch.status !== existing.status) { @@ -598,7 +647,7 @@ export class MetafieldDefinitionsService { }); } - return this.read(CUSTOM_NAMESPACE, key); + return this.read(CUSTOM_NAMESPACE, key, ADMIN); } /** @@ -686,12 +735,11 @@ function isUniqueConstraintViolation(error: unknown): boolean { return code === 'ER_DUP_ENTRY' || code === 'SQLITE_CONSTRAINT'; } -// Apply a caller-supplied NQL filter to the definition query. A malformed filter -// is a client error (400), not a 500. The active-only default is preserved unless -// the filter itself constrains status, so a filter on another field (e.g. type) -// can never surface archived fields — this is the invariant queries.ts centralises, -// held here as the per-field default override Bookshelf's filter plugin does. -function applyFilter(query: T, filter: string): T { +// Parse a caller-supplied NQL filter, without applying it. A malformed filter is a +// client error (400), not a 500. Whether the result widens the statuses is decided by +// the caller, which reads it with `filterReferencesStatus` and says so when building +// the query; nothing here narrows anything. +function parseFilter(filter: string): Record { let mongoQuery: Record; try { mongoQuery = nql(filter).toJSON() as Record; @@ -711,19 +759,7 @@ function applyFilter(query: T, filter: string): T { property: 'filter', }); } - try { - knexify(query, mongoQuery, { tableName: TABLE }); - } catch (err) { - throw new errors.BadRequestError({ - message: 'Could not parse the filter parameter.', - property: 'filter', - err: err as Error, - }); - } - if (!filterReferencesStatus(mongoQuery)) { - query.where('status', FIELD_STATUS.active); - } - return query; + return mongoQuery; } // Whether an NQL-parsed filter constrains `status` anywhere, including inside the diff --git a/ghost/core/core/server/services/members-metafields/index.ts b/ghost/core/core/server/services/members-metafields/index.ts index 5145dbd7b75..25974dc21d7 100644 --- a/ghost/core/core/server/services/members-metafields/index.ts +++ b/ghost/core/core/server/services/members-metafields/index.ts @@ -13,7 +13,15 @@ export type { WrittenBy } from './schema'; // Which door a request came through, which is what decides how much of a member's // answers it may see or change. Required wherever that is asked, so a new caller // has to name itself rather than inherit an answer by default. -export { ADMIN, INTERNAL, MEMBERS, canWrite, readableFields, type Audience } from './access'; +export { + ADMIN, + INTERNAL, + MEMBERS, + MEMBER_ACCESS, + canWrite, + type Audience, + type MemberAccess, +} from './access'; // Three services from one module, split along aggregate boundaries rather than // technical layers: `definitions` owns the field definitions, which belong to the diff --git a/ghost/core/core/server/services/members-metafields/models.ts b/ghost/core/core/server/services/members-metafields/models.ts index 71ef4345864..fb77542753d 100644 --- a/ghost/core/core/server/services/members-metafields/models.ts +++ b/ghost/core/core/server/services/members-metafields/models.ts @@ -1,5 +1,6 @@ import { z } from 'zod'; import { FieldTypeSchema } from '@tryghost/metafield-types'; +import { MemberAccessSchema } from './access'; import { FieldStatusSchema } from './schema'; export const Metafield = z.object({ @@ -9,6 +10,7 @@ export const Metafield = z.object({ name: z.string(), type: FieldTypeSchema, status: FieldStatusSchema, + access: z.object({ member: MemberAccessSchema }), createdAt: z.date(), updatedAt: z.date().nullable(), }); diff --git a/ghost/core/core/server/services/members-metafields/queries.ts b/ghost/core/core/server/services/members-metafields/queries.ts index 401ce5ba0f5..9da2d1efb4d 100644 --- a/ghost/core/core/server/services/members-metafields/queries.ts +++ b/ghost/core/core/server/services/members-metafields/queries.ts @@ -1,29 +1,73 @@ import type { Knex } from 'knex'; +import { readableLevels, type Audience } from './access'; import { FIELD_STATUS } from './schema'; const FIELDS_TABLE = 'members_metafields'; -// Archived fields must stay out of every read and write, and nothing in the database -// enforces that — no constraint stops a value row referencing an archived field. A query -// that forgets the filter is a silent bug, so the filter lives in one place. +// Nothing in the database keeps an archived or hidden field out of a read: no +// constraint stops a value row referencing one. Both filters are therefore applied in +// code, and a query that forgets either is a silent bug. -/** Takes the executor so the same query runs standalone or inside a write's transaction. */ -export function activeFields(db: Knex) { - return db(FIELDS_TABLE).where('status', FIELD_STATUS.active); +export function readableBy(query: T, audience: Audience): T { + query.whereIn(`${FIELDS_TABLE}.member_access`, readableLevels(audience)); + return query; +} + +/** + * Whether a read includes definitions the publisher has archived. + * + * A member is never offered an archived field. Staff managing the list are always + * shown one, since an archived field is still theirs to rename, restore or delete. + */ +export const ACTIVE_ONLY = 'active-only'; +export const ANY_STATUS = 'any-status'; +export type StatusScope = typeof ACTIVE_ONLY | typeof ANY_STATUS; + +declare const scoped: unique symbol; + +// Unannotated so the builder keeps the row type knex derives from the table +// registration; naming a type here would both lose that and make the alias below +// refer to itself. +function metafieldsTable(db: Knex) { + return db(FIELDS_TABLE); } /** - * A narrowed query against the definitions table, whatever it has narrowed by. + * A query built by `definitions()`, carrying both filters. * - * Named rather than inferred from `activeFields`, so a caller that narrows some other way — - * one key, a publisher's filter — states the same type instead of asserting its way back to - * it. + * knex's builder methods return an unbranded type, so chaining anything onto one of + * these drops the mark and it stops satisfying this type. Narrow by passing what you + * need to `definitions()` instead. */ -export type DefinitionQuery = ReturnType; +export type DefinitionQuery = ReturnType & { readonly [scoped]: true }; + +export function definitions( + db: Knex, + scope: { + audience: Audience; + status: StatusScope; + key?: string; + /** A publisher's NQL filter, applied under the two filters below rather than over them. */ + filter?: (query: Knex.QueryBuilder) => Knex.QueryBuilder; + limit?: number; + }, +): DefinitionQuery { + let query = metafieldsTable(db); + + if (scope.filter) { + query = scope.filter(query); + } + if (scope.key !== undefined) { + query = query.where(`${FIELDS_TABLE}.key`, scope.key); + } + if (scope.status === ACTIVE_ONLY) { + query = query.where(`${FIELDS_TABLE}.status`, FIELD_STATUS.active); + } + if (scope.limit !== undefined) { + query = query.limit(scope.limit); + } -/** One field by key, as the same kind of query the rest of this reads through. */ -export function fieldByKey(db: Knex, key: string): DefinitionQuery { - return db(FIELDS_TABLE).where(`${FIELDS_TABLE}.key`, key); + return readableBy(query, scope.audience) as DefinitionQuery; } /** diff --git a/ghost/core/core/server/services/members-metafields/schema.ts b/ghost/core/core/server/services/members-metafields/schema.ts index 5f4deab6f42..7f0eb279ec6 100644 --- a/ghost/core/core/server/services/members-metafields/schema.ts +++ b/ghost/core/core/server/services/members-metafields/schema.ts @@ -2,6 +2,7 @@ import { z } from 'zod'; import type { Knex } from 'knex'; import { FieldTypeSchema } from '@tryghost/metafield-types'; import { DbDate } from '../../lib/db-types/date'; +import { MemberAccessSchema } from './access'; // `archived` is soft: the field drops out of the values path but stays in the definition // list so it can be renamed, restored or deleted. Mirrors schema.js's `isIn` on the @@ -18,6 +19,7 @@ export const DbMetafield = z.object({ name: z.string(), type: FieldTypeSchema, status: FieldStatusSchema, + member_access: MemberAccessSchema, created_at: DbDate, updated_at: DbDate.nullable(), }); diff --git a/ghost/core/core/server/services/members-metafields/serializers.ts b/ghost/core/core/server/services/members-metafields/serializers.ts index 3898688aab8..1656c6d3aeb 100644 --- a/ghost/core/core/server/services/members-metafields/serializers.ts +++ b/ghost/core/core/server/services/members-metafields/serializers.ts @@ -8,6 +8,7 @@ const MetafieldResource = z.object({ name: z.string(), type: z.string(), status: z.string(), + access: z.object({ member: z.string() }), created_at: z.date(), updated_at: z.date().nullable(), }); diff --git a/ghost/core/core/server/services/members-metafields/values-service.ts b/ghost/core/core/server/services/members-metafields/values-service.ts index 58f0c560ef1..f32553a0cce 100644 --- a/ghost/core/core/server/services/members-metafields/values-service.ts +++ b/ghost/core/core/server/services/members-metafields/values-service.ts @@ -11,8 +11,8 @@ import { parseIdentity, } from '@tryghost/metafield-types/identity'; import { DbMetafieldLeaf, DbMetafieldValue, FIELD_STATUS, type WrittenBy } from './schema'; -import { activeFields } from './queries'; -import { canWrite, readableFields, type Audience } from './access'; +import { ACTIVE_ONLY, definitions, readableBy } from './queries'; +import { canWrite, type Audience, type MemberAccess } from './access'; import { leavesToWrite, valuesFromLeaves, type StoredLeaf } from './storage'; const FIELDS_TABLE = 'members_metafields'; @@ -43,17 +43,18 @@ const ValuesInput = z.record(z.string().max(MAX_IDENTITY_LENGTH), z.unknown()); const wireProperty = (identity: string): string => [QUALIFIER, identity].join('.'); -interface ActiveField { +interface AllowedField { id: string; namespace: string; key: string; name: string; type: FieldType; + memberAccess: MemberAccess; } /** An absent `value` means clear the field. */ export interface PlannedWrite { - field: ActiveField; + field: AllowedField; value?: unknown; } @@ -72,7 +73,14 @@ export class MetafieldValuesService { this.getMaxDefinitions = getMaxDefinitions; } - private async activeFieldsByIdentity(identities: string[]): Promise> { + // The query is scoped to the audience, so a field they may not see is absent from + // the map and the caller reports it as unknown. Looking up unscoped and rejecting + // afterwards would let a member tell a hidden field from an undefined one by the + // reply they got. + private async allowedFieldsByIdentity( + identities: string[], + audience: Audience, + ): Promise> { const keys = identities .map((identity) => parseIdentity(identity)) .filter((parsed) => parsed !== null && parsed.namespace === CUSTOM_NAMESPACE) @@ -80,13 +88,20 @@ export class MetafieldValuesService { if (keys.length === 0) { return new Map(); } - const fields = await activeFields(this.knex) + const fields = await definitions(this.knex, { audience, status: ACTIVE_ONLY }) .whereIn('key', keys) - .select('id', 'key', 'name', 'type'); + .select('id', 'key', 'name', 'type', 'member_access'); return new Map( fields.map((field) => [ formatIdentity({ namespace: CUSTOM_NAMESPACE, key: field.key, partPath: null }), - { ...field, namespace: CUSTOM_NAMESPACE }, + { + id: field.id, + key: field.key, + name: field.name, + type: field.type, + namespace: CUSTOM_NAMESPACE, + memberAccess: field.member_access, + }, ]), ); } @@ -102,8 +117,14 @@ export class MetafieldValuesService { // Not ordered by field: these rows become an object keyed by field, and an object // cannot carry an order. `path` is ordered so composite parts assemble the same // way every time. - const rows = await this.knex(VALUES_TABLE) - .join(FIELDS_TABLE, `${VALUES_TABLE}.metafield_key`, `${FIELDS_TABLE}.key`) + const rows = await readableBy( + this.knex(VALUES_TABLE).join( + FIELDS_TABLE, + `${VALUES_TABLE}.metafield_key`, + `${FIELDS_TABLE}.key`, + ), + audience, + ) .whereIn(`${VALUES_TABLE}.member_id`, memberIds) .where(`${FIELDS_TABLE}.status`, FIELD_STATUS.active) .orderBy(`${VALUES_TABLE}.path`, 'asc') @@ -132,10 +153,7 @@ export class MetafieldValuesService { } } - // Narrowed before assembly, not after: a composite is one value spread across - // several rows, and dropping some of its parts later would hand back half an - // address rather than no address. - const flat = valuesFromLeaves(readableFields(audience, leaves)); + const flat = valuesFromLeaves(leaves); return new Map( memberIds.map((memberId) => [memberId, { [CUSTOM_NAMESPACE]: flat.get(memberId) ?? {} }]), ); @@ -209,7 +227,7 @@ export class MetafieldValuesService { }); } - const byIdentity = await this.activeFieldsByIdentity(identities); + const byIdentity = await this.allowedFieldsByIdentity(identities, audience); const writes: PlannedWrite[] = []; for (const [identity, raw] of Object.entries(values)) { @@ -221,6 +239,8 @@ export class MetafieldValuesService { }); } + // A different refusal from the unknown-field one above: this audience can + // already see the field, so naming it discloses nothing. if (!canWrite(audience, field)) { throw new errors.ValidationError({ message: `Cannot set custom field: ${identity}`, diff --git a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js index e849c247144..0b899b4246f 100644 --- a/ghost/core/core/server/services/members/members-api/services/member-bread-service.js +++ b/ghost/core/core/server/services/members/members-api/services/member-bread-service.js @@ -95,16 +95,17 @@ module.exports = class MemberBREADService { * size or a delivery address. Their values live in their own table, so they are fetched * here rather than loaded alongside the member. * - * Returns null when the site has defined no fields, which tells the caller to leave the - * `metafields` key off the member payload rather than send an empty object: a key added to - * an API response cannot be withdrawn without breaking whoever started reading it, and - * most sites have never defined a field, so those sites keep the payload they had before - * this feature existed. + * Returns null when this audience has no field to be told about, which tells the caller to + * leave the `metafields` key off the member payload rather than send an empty object: a key + * added to an API response cannot be withdrawn without breaking whoever started reading it, + * and most sites have never defined a field, so those sites keep the payload they had + * before this feature existed. * @param {string[]} memberIds + * @param {import('../../../members-metafields').Audience} audience * @returns {Promise> | null>} */ async fetchMetafieldValues(memberIds, audience) { - if (!(await this.metafieldDefinitions.hasAnyActive())) { + if (!(await this.metafieldDefinitions.hasAnyReadable(audience))) { return null; } diff --git a/ghost/core/core/server/services/tier-checkout-config/service.ts b/ghost/core/core/server/services/tier-checkout-config/service.ts index 4394fdb644a..425493512dc 100644 --- a/ghost/core/core/server/services/tier-checkout-config/service.ts +++ b/ghost/core/core/server/services/tier-checkout-config/service.ts @@ -4,6 +4,7 @@ import { z } from 'zod'; import type { Knex } from 'knex'; import type { FieldType } from '@tryghost/metafield-types'; import { DbMetafield, FIELD_STATUS } from '../members-metafields/schema'; +import { MEMBER_ACCESS, type MemberAccess } from '../members-metafields'; import type { Metafield, RequestContext } from '../members-metafields'; import { MAX_CHECKOUT_LABEL_LENGTH, @@ -40,7 +41,18 @@ import { CheckoutConfigInput } from './serializers'; type FieldRow = Pick, 'key' | 'name' | 'type' | 'status'>; -type NewField = { key: string; name: string; type: FieldType }; +type NewField = { key: string; name: string; type: FieldType; access: { member: MemberAccess } }; + +/** + * What a member may do with a field this service creates for them. + * + * The opposite of the default a publisher-made field gets. These hold what the member + * themselves gave at checkout, and collection can be switched on while the screen for + * opening a field to members is not, since the two sit behind different flags. A closed + * default would then leave a member unable to correct their own address and nobody able + * to open it for them. + */ +const COLLECTED_FIELD_ACCESS = { member: MEMBER_ACCESS.write } as const; /** * A request states its settings in named sections: one for shipping, one for phone. Each @@ -228,7 +240,12 @@ export class TierCheckoutConfigService { }); } if (!alreadyPlanned) { - create.set(key, { key, name: wants.name, type: wants.type }); + create.set(key, { + key, + name: wants.name, + type: wants.type, + access: COLLECTED_FIELD_ACCESS, + }); } } diff --git a/ghost/core/package.json b/ghost/core/package.json index f6e16d7ec72..c4d47b80014 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -1,6 +1,6 @@ { "name": "ghost", - "version": "6.63.1-rc.0", + "version": "6.64.0-rc.0", "description": "The professional publishing platform", "keywords": [ "blog", diff --git a/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts b/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts index d9d495a18cb..3776b5faf4b 100644 --- a/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts +++ b/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts @@ -463,6 +463,76 @@ describe('Member Custom Fields Admin API', function () { }); }); + describe('Member access', function () { + it('creates a field closed to members', async function () { + const created = await createField({ name: 'Internal note' }); + assert.deepEqual(created.access, { member: 'none' }); + }); + + it('creates a field open to members when the publisher says so', async function () { + const { body } = await agent + .post('members/metafields/custom/') + .body({ + members_metafields: [ + { name: 'Shoe size', type: 'short_text', access: { member: 'write' } }, + ], + }) + .expectStatus(201); + assert.deepEqual(body.members_metafields[0].access, { member: 'write' }); + }); + + it('opens and closes a field after the fact', async function () { + const field = await createField({ name: 'Shoe size' }); + + const opened = ( + await agent + .put(`members/metafields/custom/${field.key}/`) + .body({ members_metafields: [{ access: { member: 'read' } }] }) + .expectStatus(200) + ).body.members_metafields[0]; + assert.deepEqual(opened.access, { member: 'read' }); + + const closed = ( + await agent + .put(`members/metafields/custom/${field.key}/`) + .body({ members_metafields: [{ access: { member: 'none' } }] }) + .expectStatus(200) + ).body.members_metafields[0]; + assert.deepEqual(closed.access, { member: 'none' }); + }); + + it('leaves access alone when an edit says nothing about it', async function () { + const field = await createField({ name: 'Shoe size' }); + await agent + .put(`members/metafields/custom/${field.key}/`) + .body({ members_metafields: [{ access: { member: 'write' } }] }) + .expectStatus(200); + + const renamed = ( + await agent + .put(`members/metafields/custom/${field.key}/`) + .body({ members_metafields: [{ name: 'Shoe size (EU)' }] }) + .expectStatus(200) + ).body.members_metafields[0]; + + assert.equal(renamed.name, 'Shoe size (EU)'); + assert.deepEqual(renamed.access, { member: 'write' }); + }); + + it('refuses a level it does not recognise', async function () { + const field = await createField({ name: 'Shoe size' }); + await agent + .put(`members/metafields/custom/${field.key}/`) + .body({ members_metafields: [{ access: { member: 'admin' } }] }) + .expectStatus(422); + + const unchanged = ( + await agent.get(`members/metafields/custom/${field.key}/`).expectStatus(200) + ).body.members_metafields[0]; + assert.deepEqual(unchanged.access, { member: 'none' }); + }); + }); + // Admin sends `?include=` on resources that have relations to load. This one has none, // so the parameter has nothing to act on — which is a request that returns a definition, // not a request that got something wrong. @@ -1958,6 +2028,8 @@ describe('Member Custom Fields Admin API', function () { primary_name?: string; key?: string; previous_name?: string; + member_access?: string; + previous_member_access?: string; action_name?: string; count?: number; }; @@ -1966,6 +2038,22 @@ describe('Member Custom Fields Admin API', function () { actorId = (await agent.get('users/me/').expectStatus(200)).body.users[0].id; }); + it("records what a field's member access became, and what it was", async function () { + const field = await createField({ name: 'Shoe size' }); + await agent + .put(`members/metafields/custom/${field.key}/`) + .body({ members_metafields: [{ access: { member: 'write' } }] }) + .expectStatus(200); + + const actions = await customFieldActions(); + const opened = actions.find((a: { event: string }) => a.event === 'edited') as unknown as { + context: unknown; + }; + + assert.equal(contextOf(opened).member_access, 'write'); + assert.equal(contextOf(opened).previous_member_access, 'none'); + }); + it('records an "added" action when a field is created', async function () { const field = await createField({ name: 'Favourite topic' }); diff --git a/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts b/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts index e33fefc9dd4..4c3bed3147f 100644 --- a/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts +++ b/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts @@ -1125,6 +1125,65 @@ describe('Tier Checkout Admin API', function () { ); }); + it('provisions them as the member’s own to read and change', async function () { + mockManager.mockLabsDisabled('membersCustomFields'); + await agent + .put(`tiers/${tierId}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + name: { custom_field_key: PORT_FIELD[STRIPE_PORT.shippingName].key }, + address: { custom_field_key: PORT_FIELD[STRIPE_PORT.shippingAddress].key }, + }, + }, + ], + }) + .expectStatus(200); + + const { body } = await agent.get('members/metafields/custom/').expectStatus(200); + assert.deepEqual( + body.members_metafields.map((f: { key: string; access: unknown }) => [f.key, f.access]), + [ + ['shipping_name', { member: 'write' }], + ['shipping_address', { member: 'write' }], + ], + ); + }); + + // The other half of the same rule: the exception covers fields this path creates, + // not fields it binds to. Reopening one the publisher had closed would disclose + // what they had deliberately kept back. + it('leaves an existing field’s access alone when it binds to one', async function () { + const existing = await createField({ name: 'Delivery notes' }); + await agent + .put(`members/metafields/custom/${existing.key}/`) + .body({ members_metafields: [{ access: { member: 'none' } }] }) + .expectStatus(200); + + mockManager.mockLabsDisabled('membersCustomFields'); + await agent + .put(`tiers/${tierId}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + name: { custom_field_key: existing.key }, + address: { custom_field_key: PORT_FIELD[STRIPE_PORT.shippingAddress].key }, + }, + }, + ], + }) + .expectStatus(200); + + const { body } = await agent + .get(`members/metafields/custom/${existing.key}/`) + .expectStatus(200); + assert.deepEqual(body.members_metafields[0].access, { member: 'none' }); + }); + // The tier resource is generally available, so this concept must not appear on it. it('adds nothing to the tier itself', async function () { const { body } = await agent.get(`tiers/${tierId}/`).expectStatus(200); diff --git a/ghost/core/test/e2e-api/members/custom-fields.test.ts b/ghost/core/test/e2e-api/members/custom-fields.test.ts index 463f03728e5..dd6ba6d71a4 100644 --- a/ghost/core/test/e2e-api/members/custom-fields.test.ts +++ b/ghost/core/test/e2e-api/members/custom-fields.test.ts @@ -36,20 +36,31 @@ describe('Member Custom Fields Members API', function () { let memberId: string; let fieldKey: string; - /** Define a field, as a publisher does, and hand back the key Ghost minted for it. */ /** Every field this suite defined, so cleanup can undo its own work and no more. */ const defined = new Set(); - async function defineField(name: string, type = 'short_text'): Promise { + // Opens the field to members unless told otherwise. The API creates fields closed, + // so the tests that want a closed one ask for it explicitly. + async function defineField( + name: string, + { type = 'short_text', access = 'write' }: { type?: string; access?: string } = {}, + ): Promise { const { body } = await adminAgent .post('members/metafields/custom/') - .body({ members_metafields: [{ name, type }] }) + .body({ members_metafields: [{ name, type, access: { member: access } }] }) .expectStatus(201); const { key } = body.members_metafields[0]; defined.add(key); return key; } + async function setAccess(key: string, access: string): Promise { + await adminAgent + .put(`members/metafields/custom/${key}/`) + .body({ members_metafields: [{ access: { member: access } }] }) + .expectStatus(200); + } + async function archiveField(key: string): Promise { await adminAgent .put(`members/metafields/custom/${key}/`) @@ -253,4 +264,166 @@ describe('Member Custom Fields Members API', function () { assert.equal(after.name, before.name); assert.equal(after.metafields.custom[fieldKey], '9', 'the defined field kept its value'); }); + + describe('what a publisher has kept to themselves', function () { + it('says nothing at all about a field the member may not see', async function () { + const privateKey = await defineField('Internal note', { access: 'none' }); + await setValuesAsStaff({ [privateKey]: 'Difficult on the phone' }); + + const { body: offered } = await membersAgent + .get('/api/member/metafields/custom/') + .expectStatus(200); + assert.deepEqual( + offered.members_metafields.map((field: { key: string }) => field.key), + [fieldKey], + 'the closed field is not among the fields there are to fill in', + ); + + const { body: account } = await membersAgent.get('/api/member/').expectStatus(200); + assert.deepEqual( + account.metafields, + { custom: { [fieldKey]: '9' } }, + 'nor is what staff put in it', + ); + + const seen = JSON.stringify(offered) + JSON.stringify(account); + assert.ok(!seen.includes(privateKey), 'the key is not named'); + assert.ok(!seen.includes('Internal note'), 'nor the name the publisher gave it'); + assert.ok(!seen.includes('Difficult on the phone'), 'nor what it holds'); + }); + + it('answers a write to a closed field exactly as it answers a write to no field', async function () { + const privateKey = await defineField('Internal note', { access: 'none' }); + + const refusals = []; + for (const key of [privateKey, 'nothing_by_this_name']) { + const { body } = await membersAgent + .put('/api/member/') + .body({ metafields: { custom: { [key]: 'x' } } }) + .expectStatus(422); + refusals.push(body.errors[0]); + } + + const [closed, undefined_] = refusals; + // Any difference between these two is a way to ask a site which fields it holds. + assert.equal(closed.message, `Unknown custom field: custom.${privateKey}`); + assert.equal(undefined_.message, 'Unknown custom field: custom.nothing_by_this_name'); + assert.equal(closed.type, undefined_.type); + assert.equal(closed.property, `metafields.custom.${privateKey}`); + assert.equal(undefined_.property, 'metafields.custom.nothing_by_this_name'); + + const stored = await readMemberAsStaff(); + assert.equal(Object.hasOwn(stored.metafields.custom, privateKey), false); + }); + + // A member's request body keys reach a plain object on the way to being resolved, + // and a key naming something every object inherits reads back as present when it + // was never set. These are refused like any other unknown field today only because + // the namespace makes the key a compound one; if the bare form ever becomes + // addressable, this is what should fail first. + it('refuses a key naming an inherited property, and stores nothing', async function () { + for (const hostile of ['__proto__', 'constructor', 'toString', 'hasOwnProperty']) { + const { body } = await membersAgent + .put('/api/member/') + .body({ metafields: { custom: { [hostile]: 'x' } } }) + .expectStatus(422); + assert.equal(body.errors[0].message, `Unknown custom field: custom.${hostile}`); + } + + // Namespaces are data too, so the same key can arrive one level up. Built by + // parsing rather than as a literal: `{__proto__: …}` in source sets the + // prototype instead of creating a key, so it would serialise to `{}` and this + // would assert nothing. + await membersAgent + .put('/api/member/') + .body({ metafields: JSON.parse('{"__proto__": {"anything": "x"}}') }) + .expectStatus(422); + + const stored = await readMemberAsStaff(); + assert.deepEqual(stored.metafields.custom, { [fieldKey]: '9' }, 'nothing else was written'); + assert.equal( + ({} as Record).anything, + undefined, + 'and nothing leaked onto Object', + ); + }); + + it('will not let a member change a field they may only read', async function () { + const readOnlyKey = await defineField('Membership number', { access: 'read' }); + await setValuesAsStaff({ [readOnlyKey]: 'M-001' }); + + const { body: account } = await membersAgent.get('/api/member/').expectStatus(200); + assert.equal(account.metafields.custom[readOnlyKey], 'M-001', 'they are shown it'); + + const { body: offered } = await membersAgent + .get('/api/member/metafields/custom/') + .expectStatus(200); + const offeredField = offered.members_metafields.find( + (field: { key: string }) => field.key === readOnlyKey, + ); + assert.deepEqual( + offeredField.access, + { member: 'read' }, + 'and told they may not change it, so a client can render it as such', + ); + + const { body } = await membersAgent + .put('/api/member/') + .body({ metafields: { custom: { [readOnlyKey]: 'M-999' } } }) + .expectStatus(422); + + assert.equal(body.errors[0].message, `Cannot set custom field: custom.${readOnlyKey}`); + + const stored = await readMemberAsStaff(); + assert.equal(stored.metafields.custom[readOnlyKey], 'M-001'); + }); + + it('leaves a member no way to tell a closed site from a site with no fields', async function () { + await setAccess(fieldKey, 'none'); + + const { body } = await membersAgent.get('/api/member/').expectStatus(200); + assert.equal(Object.hasOwn(body, 'metafields'), false); + }); + + it('shows a member what was already collected when a field is opened to them', async function () { + const laterKey = await defineField('Delivery address', { access: 'none' }); + await setValuesAsStaff({ [laterKey]: '1 Main St' }); + + const { body: before } = await membersAgent.get('/api/member/').expectStatus(200); + assert.equal(Object.hasOwn(before.metafields.custom, laterKey), false); + + await setAccess(laterKey, 'read'); + + const { body: after } = await membersAgent.get('/api/member/').expectStatus(200); + assert.equal(after.metafields.custom[laterKey], '1 Main St'); + }); + + it('stops showing a member a field that is closed again, and keeps what they wrote', async function () { + await membersAgent + .put('/api/member/') + .body({ metafields: { custom: { [fieldKey]: '11' } } }) + .expectStatus(200); + + await setAccess(fieldKey, 'none'); + + const { body } = await membersAgent.get('/api/member/').expectStatus(200); + assert.equal(Object.hasOwn(body, 'metafields'), false); + + const stored = await readMemberAsStaff(); + assert.equal(stored.metafields.custom[fieldKey], '11'); + }); + + it('shows staff every field, whatever a member may do with it', async function () { + const privateKey = await defineField('Internal note', { access: 'none' }); + await setValuesAsStaff({ [privateKey]: 'Difficult on the phone' }); + + const stored = await readMemberAsStaff(); + assert.equal(stored.metafields.custom[privateKey], 'Difficult on the phone'); + assert.equal(stored.metafields.custom[fieldKey], '9'); + + const { body } = await adminAgent.get('members/metafields/custom/').expectStatus(200); + const keys = body.members_metafields.map((field: { key: string }) => field.key); + assert.ok(keys.includes(privateKey), 'and the definition, in the list they manage'); + }); + }); }); diff --git a/ghost/core/test/unit/server/data/schema/integrity.test.js b/ghost/core/test/unit/server/data/schema/integrity.test.js index 5b881bbb7cd..61d26d2ec87 100644 --- a/ghost/core/test/unit/server/data/schema/integrity.test.js +++ b/ghost/core/test/unit/server/data/schema/integrity.test.js @@ -37,7 +37,7 @@ const parseYaml = require('../../../../../core/server/services/route-settings/ya */ describe('DB version integrity', function () { // Only these variables should need updating - const currentSchemaHash = '29d413d3d965639382e8506d5db877d2'; + const currentSchemaHash = '83816146af992ec6a8df98956c3e6bd9'; const currentFixturesHash = '5718e0d4eb037f159c312369e949829a'; const currentSettingsHash = '6ea42a00cca61a1ba87f66eb6e25a78a'; const currentRoutesHash = 'd8c25fa01bf6d22a2bcb05ba0de70dc1'; diff --git a/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js b/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js index c89fcba90a3..975a9f876a8 100644 --- a/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js +++ b/ghost/core/test/unit/server/services/members/members-api/services/members-bread-service.test.js @@ -17,8 +17,8 @@ const createMetafieldValuesStub = () => ({ applyWrite: sinon.stub().resolves(), }); -const createMetafieldDefinitionsStub = (hasAnyActive = false) => ({ - hasAnyActive: sinon.stub().resolves(hasAnyActive), +const createMetafieldDefinitionsStub = (hasAnyReadable = false) => ({ + hasAnyReadable: sinon.stub().resolves(hasAnyReadable), }); describe('MemberBreadService', function () { @@ -586,7 +586,7 @@ describe('MemberBreadService', function () { const member = await memberBreadService.read({ id: MEMBER_ID }, { metafieldsFor: null }); assert.equal(Object.hasOwn(member, 'metafields'), false); - assert.equal(metafieldDefinitions.hasAnyActive.called, false); + assert.equal(metafieldDefinitions.hasAnyReadable.called, false); assert.equal(metafieldValues.getValuesForMembers.called, false); }); diff --git a/packages/metafield-types/src/index.ts b/packages/metafield-types/src/index.ts index 12659a1a10b..1a2a4076785 100644 --- a/packages/metafield-types/src/index.ts +++ b/packages/metafield-types/src/index.ts @@ -60,6 +60,24 @@ export const FIELD_TYPE_IDS = ['short_text', 'long_text', 'address'] as const; export type FieldType = (typeof FIELD_TYPE_IDS)[number]; export const FieldTypeSchema = z.enum(FIELD_TYPE_IDS); +/** + * What the member whose record it is may do with a field. + * + * A property of a definition rather than of the publisher's namespace, so it has an + * answer for any regime that ever stores definitions, not only the publisher's. + * + * Ordered: `write` includes being able to read. Naming the levels for a publisher is + * presentation and stays with whoever renders them. + */ +export const MEMBER_ACCESS_LEVELS = ['none', 'read', 'write'] as const; +export type MemberAccess = (typeof MEMBER_ACCESS_LEVELS)[number]; +export const MemberAccessSchema = z.enum(MEMBER_ACCESS_LEVELS); +export const MEMBER_ACCESS = { + none: 'none', + read: 'read', + write: 'write', +} as const satisfies Record; + /** * What kind of thing a type's value is, as anything comparing values needs to know. * From 555b290a782293f04945f1e168a52b6b40fa0e18 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 9 Sep 2026 12:02:06 +0100 Subject: [PATCH 07/17] Added the control a publisher opens a custom field to members with ref https://linear.app/ghost/issue/BER-3864/differentiate-public-and-private-member-projections Deciding who a field is for now happens where a publisher makes the field, rather than against the API by hand. The choice is one of three rather than two switches, because the levels are ordered: a member cannot usefully change something they are not shown. Opening a field says, on the spot, that anything already recorded in it becomes visible to the member, which is the part nobody would expect. It reads as a change of display and is a disclosure of everything staff have written there since the field existed. Who each field is for also reads off the list itself, because which of them members can reach is a question asked of the whole list rather than of one field at a time. Admin ships separately from Core, and a Core that predates the setting sends no answer at all; that reads as staff-only, in one place, since guessing the other way would draw a field as open on a Ghost that has no such idea and the publisher would believe it. --- .../src/api/member-custom-fields.ts | 57 +++++++++-- .../custom-fields/mapping-step.tsx | 2 +- ...r-detail-custom-fields.acceptance.test.tsx | 3 + ...-members-custom-fields.acceptance.test.tsx | 8 +- .../members-filtering.acceptance.test.tsx | 1 + .../custom-fields.acceptance.test.tsx | 96 ++++++++++++++++++- .../src/settings/membership/custom-fields.tsx | 5 +- .../custom-fields/custom-field-modal.tsx | 64 ++++++++++++- .../tiers-checkout.acceptance.test.tsx | 5 +- .../custom-field-picker.tsx | 9 +- 10 files changed, 230 insertions(+), 20 deletions(-) diff --git a/apps/admin-x-framework/src/api/member-custom-fields.ts b/apps/admin-x-framework/src/api/member-custom-fields.ts index bc3954c9b91..5a1ddc3598f 100644 --- a/apps/admin-x-framework/src/api/member-custom-fields.ts +++ b/apps/admin-x-framework/src/api/member-custom-fields.ts @@ -1,6 +1,7 @@ import { FIELD_TYPES, FIELD_TYPE_IDS, + type MemberAccess, partTypesOf, subFieldsOf, type FieldKind, @@ -37,10 +38,48 @@ export type MemberCustomField = { // Browse hides archived fields by default (most surfaces only want active // ones); Settings opts in via filter and splits on this. status: 'active' | 'archived'; + access: { member: MemberCustomFieldAccess }; created_at: string; updated_at: string | null; }; +// The levels themselves are the shared vocabulary; what a publisher is told they mean +// is presentation, and stays below. +export type MemberCustomFieldAccess = MemberAccess; + +export const MEMBER_CUSTOM_FIELD_ACCESS_OPTIONS: { + value: MemberCustomFieldAccess; + label: string; + description: string; +}[] = [ + { + value: 'none', + label: 'Only staff', + description: 'Members never see this field or what you record in it', + }, + { + value: 'read', + label: 'Members can view', + description: 'Shown in their account, but only staff can change it', + }, + { + value: 'write', + label: 'Members can edit', + description: 'Members fill this in and keep it up to date themselves', + }, +]; + +/** + * The words a publisher reads for a level. + * + * A level this build does not know shows as itself rather than falling back to the + * closed label: the fallback would tell a publisher a field is staff-only when the + * server may be treating it as open, and a label that reassures is worse than one + * that reads oddly. Reachable only from a Core newer than this Admin. + */ +export const memberAccessLabel = (access: MemberCustomFieldAccess): string => + MEMBER_CUSTOM_FIELD_ACCESS_OPTIONS.find((option) => option.value === access)?.label ?? access; + /** * The user-type catalog: the presentation layer over the shared field types. * @@ -323,10 +362,16 @@ export const useBrowseMemberCustomFieldsIncludingArchived = ( ) => useBrowseMemberCustomFields({ ...options, searchParams: { filter: 'status:[active,archived]' } }); -// The backend mints the key from the name, so create takes just a name and a type. +/** Everything a new field is created from. The backend mints the key from the name. */ +export type NewMemberCustomField = Pick; + +/** A change to one field, addressed by key. Anything omitted is left as it is. */ +export type MemberCustomFieldEdit = Pick & + Partial>; + export const useCreateMemberCustomField = createMutation< MemberCustomFieldsResponseType, - Pick + NewMemberCustomField >({ method: 'POST', path: () => '/members/metafields/custom/', @@ -357,12 +402,12 @@ export const useCreateMemberCustomField = createMutation< }, }); -// Keys are immutable after creation (the API rejects changes); `name` and -// `status` are the editable surface — a status flip to 'active' is how an -// archived field is reactivated. +// Keys are immutable after creation (the API rejects changes); `name`, `status` and +// `access` are the editable surface — a status flip to 'active' is how an archived +// field is reactivated, and an access change is what opens a field to members. export const useEditMemberCustomField = createMutation< MemberCustomFieldsResponseType, - Pick & Partial> + MemberCustomFieldEdit >({ method: 'PUT', path: (field) => `/members/metafields/custom/${field.key}/`, diff --git a/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/mapping-step.tsx b/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/mapping-step.tsx index 917da8fc984..bfc6fac497b 100644 --- a/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/mapping-step.tsx +++ b/apps/admin/src/members/components/bulk-action-modals/import-members/custom-fields/mapping-step.tsx @@ -221,7 +221,7 @@ export function MappingStep({ let field; try { - const response = await createField({ name, type }); + const response = await createField({ name, type, access: { member: 'none' } }); field = response.members_metafields?.[0]; } catch (error) { reportCreateFailure(error); 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 dbe64c6735d..5fbf0f46b31 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 @@ -19,6 +19,7 @@ const FIELDS: MemberCustomField[] = [ name: 'Job title', type: 'short_text', status: 'active', + access: { member: 'none' }, created_at: '2026-07-14T00:00:00.000Z', updated_at: null, }, @@ -28,6 +29,7 @@ const FIELDS: MemberCustomField[] = [ name: 'Company', type: 'long_text', status: 'active', + access: { member: 'none' }, created_at: '2026-07-14T00:00:00.000Z', updated_at: null, }, @@ -37,6 +39,7 @@ const FIELDS: MemberCustomField[] = [ name: 'Home address', type: 'address', status: 'active', + access: { member: 'none' }, created_at: '2026-07-14T00:00:00.000Z', updated_at: null, }, diff --git a/apps/admin/src/members/import-members-custom-fields.acceptance.test.tsx b/apps/admin/src/members/import-members-custom-fields.acceptance.test.tsx index fe424d2edfe..5ad49428776 100644 --- a/apps/admin/src/members/import-members-custom-fields.acceptance.test.tsx +++ b/apps/admin/src/members/import-members-custom-fields.acceptance.test.tsx @@ -58,6 +58,7 @@ function fakeCustomFieldsWorld(definedFields: MemberCustomField[] = []) { name: input.name.trim(), type: input.type, status: 'active', + access: { member: 'none' }, created_at: '2026-08-05T00:00:00.000Z', updated_at: null, }; @@ -74,6 +75,7 @@ const NICKNAME_FIELD: MemberCustomField = { name: 'Nickname', type: 'short_text', status: 'active', + access: { member: 'none' }, created_at: '2026-08-05T00:00:00.000Z', updated_at: null, }; @@ -134,7 +136,7 @@ describe('Import members custom fields', () => { await expect .poll(() => createApi.lastRequest?.body) .toEqual({ - members_metafields: [{ name: 'Nickname', type: 'short_text' }], + members_metafields: [{ name: 'Nickname', type: 'short_text', access: { member: 'none' } }], }); // The row carries the new field immediately, from the create response: the browse query @@ -284,7 +286,9 @@ describe('Import members custom fields', () => { await expect .poll(() => createApi.lastRequest?.body) .toEqual({ - members_metafields: [{ name: 'Shipping address', type: 'address' }], + members_metafields: [ + { name: 'Shipping address', type: 'address', access: { member: 'none' } }, + ], }); // The form is gone and its row's picker is open in its place, showing that field's diff --git a/apps/admin/src/members/members-filtering.acceptance.test.tsx b/apps/admin/src/members/members-filtering.acceptance.test.tsx index f7231d3932c..9e8ff2f6b76 100644 --- a/apps/admin/src/members/members-filtering.acceptance.test.tsx +++ b/apps/admin/src/members/members-filtering.acceptance.test.tsx @@ -93,6 +93,7 @@ describe('Members list', () => { name: 'Employer', type: 'short_text', status: 'active', + access: { member: 'none' }, created_at: '2026-08-05T00:00:00.000Z', updated_at: null, }, diff --git a/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx b/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx index 889e6b393fa..a8375fb33c6 100644 --- a/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx @@ -16,6 +16,7 @@ const companyField: MemberCustomField = { name: 'Company', type: 'short_text', status: 'active', + access: { member: 'none' }, created_at: '2026-07-13T00:00:00.000Z', updated_at: null, }; @@ -26,6 +27,7 @@ const archivedField: MemberCustomField = { name: 'Old hobby', type: 'short_text', status: 'archived', + access: { member: 'none' }, created_at: '2026-07-12T00:00:00.000Z', updated_at: '2026-07-13T00:00:00.000Z', }; @@ -79,6 +81,7 @@ describe('Custom fields', () => { await expect(row).toHaveCount(1); await expect.element(row).toHaveTextContent('Company'); await expect.element(row).toHaveTextContent('Short text'); + await expect.element(row).toHaveTextContent('Only staff'); }); it('validates and creates a short-text field without sending a key', async () => { @@ -99,7 +102,7 @@ describe('Custom fields', () => { await expect(modal).toHaveCount(0); expect(createApi.lastRequest?.body).toEqual({ - members_metafields: [{ name: 'Job Title', type: 'short_text' }], + members_metafields: [{ name: 'Job Title', type: 'short_text', access: { member: 'none' } }], }); }); @@ -120,7 +123,7 @@ describe('Custom fields', () => { await expect(modal).toHaveCount(0); expect(createApi.lastRequest?.body).toEqual({ - members_metafields: [{ name: 'Bio', type: 'long_text' }], + members_metafields: [{ name: 'Bio', type: 'long_text', access: { member: 'none' } }], }); }); @@ -156,6 +159,69 @@ describe('Custom fields', () => { expect(createApi.requests).toHaveLength(1); }); + it('warns that opening a closed field discloses what is already in it', async () => { + fakeSettingsScreens(); + fakeCustomFields(); + fakeAdminEndpoint('PUT', '/members/metafields/custom/company/', { + members_metafields: [{ ...companyField, access: { member: 'write' as const } }], + }); + await renderAdminApp('/settings', flagOn); + + await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); + const modal = settingsScreen.customFieldModal(); + await modal.getByLabelText('Who it’s for').click(); + await page.getByRole('option', { name: 'Members can edit' }).click(); + + await expect.element(modal.getByText(/becomes visible to them/)).toBeVisible(); + }); + + it('does not warn about disclosure when the member could already see the field', async () => { + fakeSettingsScreens(); + fakeCustomFields([{ ...companyField, access: { member: 'read' as const } }]); + fakeAdminEndpoint('PUT', '/members/metafields/custom/company/', { + members_metafields: [{ ...companyField, access: { member: 'write' as const } }], + }); + await renderAdminApp('/settings', flagOn); + + await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); + const modal = settingsScreen.customFieldModal(); + await modal.getByLabelText('Who it’s for').click(); + await page.getByRole('option', { name: 'Members can edit' }).click(); + + await expect(modal.getByText(/becomes visible to them/)).toHaveCount(0); + }); + + // Access does nothing while a field is archived — members never see one — so the + // control is not offered. Otherwise it would change something invisible, and the + // disclosure would arrive later, at reactivation, unannounced. + it('does not offer the access control on an archived field', async () => { + fakeSettingsScreens(); + fakeCustomFields([archivedField]); + await renderAdminApp('/settings', flagOn); + + await settingsScreen.customFields().getByRole('tab', { name: 'Archived' }).click(); + await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); + const modal = settingsScreen.customFieldModal(); + await expect.element(modal.getByTestId('custom-field-access')).toBeDisabled(); + await expect.element(modal.getByText(/Reactivate it to choose who it/)).toBeVisible(); + }); + + // Reactivating is the only way an archived field's values reach a member, so it is + // the moment that has to say so. + it('warns when reactivating a field that is open to members', async () => { + fakeSettingsScreens(); + fakeCustomFields([{ ...archivedField, access: { member: 'write' as const } }]); + await renderAdminApp('/settings', flagOn); + + await settingsScreen.customFields().getByRole('tab', { name: 'Archived' }).click(); + await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); + await settingsScreen.customFieldModal().getByRole('button', { name: 'Reactivate' }).click(); + + await expect + .element(settingsScreen.confirmationModal().getByText(/becomes visible to each of them/)) + .toBeVisible(); + }); + it('renames a field without allowing its type to change', async () => { fakeSettingsScreens(); fakeCustomFields(); @@ -172,7 +238,31 @@ describe('Custom fields', () => { await modal.getByRole('button', { name: 'Save' }).click(); await expect(modal).toHaveCount(0); - expect(editApi.lastRequest?.body).toEqual({ members_metafields: [{ name: 'Employer' }] }); + // Only the name. Echoing the access back would carry whatever this screen loaded, + // so a rename would undo an access change someone else made in the meantime. + expect(editApi.lastRequest?.body).toEqual({ + members_metafields: [{ name: 'Employer' }], + }); + }); + + it('sends only the access when only the access changed', async () => { + fakeSettingsScreens(); + fakeCustomFields(); + const editApi = fakeAdminEndpoint('PUT', '/members/metafields/custom/company/', { + members_metafields: [{ ...companyField, access: { member: 'write' as const } }], + }); + await renderAdminApp('/settings', flagOn); + + await settingsScreen.customFields().getByTestId('custom-field-list-item').click(); + const modal = settingsScreen.customFieldModal(); + await modal.getByLabelText('Who it’s for').click(); + await page.getByRole('option', { name: 'Members can edit' }).click(); + await modal.getByRole('button', { name: 'Save' }).click(); + + await expect(modal).toHaveCount(0); + expect(editApi.lastRequest?.body).toEqual({ + members_metafields: [{ access: { member: 'write' } }], + }); }); it('archives a field only after destructive confirmation', async () => { diff --git a/apps/admin/src/settings/membership/custom-fields.tsx b/apps/admin/src/settings/membership/custom-fields.tsx index 8d1580ceade..0da1058d9c2 100644 --- a/apps/admin/src/settings/membership/custom-fields.tsx +++ b/apps/admin/src/settings/membership/custom-fields.tsx @@ -23,6 +23,7 @@ import { TextCursorInput } from 'lucide-react'; import { arrayMove } from '@dnd-kit/sortable'; import { inOrderOf, + memberAccessLabel, memberCustomFieldsDataType, useBrowseMemberCustomFieldsIncludingArchived, useReorderMemberCustomFields, @@ -64,7 +65,9 @@ const FieldRow: React.FC<{ {field.name} - {userType.label} + + {userType.label} · {memberAccessLabel(field.access.member)} + diff --git a/apps/admin/src/settings/membership/custom-fields/custom-field-modal.tsx b/apps/admin/src/settings/membership/custom-fields/custom-field-modal.tsx index 46b21c1c4f6..342ede7c3d2 100644 --- a/apps/admin/src/settings/membership/custom-fields/custom-field-modal.tsx +++ b/apps/admin/src/settings/membership/custom-fields/custom-field-modal.tsx @@ -22,6 +22,7 @@ import { LucideIcon } from '@tryghost/shade/utils'; import { SettingsModal } from '@tryghost/shade/patterns'; import { ValidationError, getErrorMessage } from '@tryghost/admin-x-framework/errors'; import { + MEMBER_CUSTOM_FIELD_ACCESS_OPTIONS, memberCustomFieldUserTypes, useCreateMemberCustomField, useDeleteMemberCustomField, @@ -31,7 +32,10 @@ import { import { toast } from 'sonner'; import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useForm, useHandleError } from '@tryghost/admin-x-framework/hooks'; -import type { MemberCustomField } from '@tryghost/admin-x-framework/api/member-custom-fields'; +import type { + MemberCustomField, + MemberCustomFieldAccess, +} from '@tryghost/admin-x-framework/api/member-custom-fields'; const userTypeById = (id: string) => memberCustomFieldUserTypes.find((userType) => userType.id === id) || @@ -46,7 +50,7 @@ const CustomFieldModal: React.FC<{ field?: MemberCustomField; onClose: () => voi const { mutateAsync: editField } = useEditMemberCustomField(); const { mutateAsync: deleteField } = useDeleteMemberCustomField(); const handleError = useHandleError(); - const isEdit = Boolean(field); + const isEdit = field !== undefined; const { formState, updateForm, handleSave, errors, clearError, setErrors, okProps } = useForm({ initialState: { @@ -54,6 +58,7 @@ const CustomFieldModal: React.FC<{ field?: MemberCustomField; onClose: () => voi // Form state tracks the user-type id; it maps to the API storage // type on save userTypeId: field ? userTypeForField(field).id : memberCustomFieldUserTypes[0].id, + access: field ? field.access.member : 'none', }, savingDelay: 500, onValidate: (state) => { @@ -68,10 +73,22 @@ const CustomFieldModal: React.FC<{ field?: MemberCustomField; onClose: () => voi }, onSave: async (state) => { if (field) { - await editField({ key: field.key, name: state.name.trim() }); + // Only the properties this form actually changed. Sending the whole field back + // would carry whatever the list held when it was loaded, so saving a rename + // would silently restore the access a colleague had changed in the meantime. + const name = state.name.trim(); + await editField({ + key: field.key, + ...(name === field.name ? {} : { name }), + ...(state.access === field.access.member ? {} : { access: { member: state.access } }), + }); } else { - // Just name and type: the backend mints the immutable key. - await createField({ name: state.name.trim(), type: userTypeById(state.userTypeId).id }); + // No key: the backend mints it from the name. + await createField({ + name: state.name.trim(), + type: userTypeById(state.userTypeId).id, + access: { member: state.access }, + }); } }, onSaveError: (error) => { @@ -151,6 +168,12 @@ const CustomFieldModal: React.FC<{ field?: MemberCustomField; onClose: () => voi on your members, for collecting, and in filters.
Values already collected for this field will remain unchanged.
+ {field!.access.member !== 'none' && ( +
+ This field is open to members, so what you have already collected becomes visible + to each of them on their own record. +
+ )} ), okLabel: 'Reactivate', @@ -286,6 +309,37 @@ const CustomFieldModal: React.FC<{ field?: MemberCustomField; onClose: () => voi {isEdit && Type can’t be changed after creation} + + Who it’s for + + + {isArchived + ? 'Members never see an archived field. Reactivate it to choose who it’s for.' + : MEMBER_CUSTOM_FIELD_ACCESS_OPTIONS.find( + (option) => option.value === formState.access, + )?.description} + {isEdit && formState.access !== 'none' && field.access.member === 'none' && ( + <> Anything already recorded in this field becomes visible to them. + )} + + ); 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 ac2eeb7ddba..59a4b2de115 100644 --- a/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/tiers-checkout.acceptance.test.tsx @@ -26,6 +26,7 @@ const addressField: MemberCustomField = { name: 'Shipping Address', type: 'address', status: 'active', + access: { member: 'none' }, created_at: '2026-07-13T00:00:00.000Z', updated_at: null, }; @@ -301,8 +302,10 @@ describe('Tier checkout collection', () => { await expect .element(modal.getByLabelText('Save recipient name as')) .toHaveTextContent(created.name); + // Closed, like any other field a publisher makes by hand. The picker only appears + // when the setting for opening one to members does too. expect(createApi.lastRequest?.body).toEqual({ - members_metafields: [{ name: created.name, type: 'short_text' }], + members_metafields: [{ name: created.name, type: 'short_text', access: { member: 'none' } }], }); }); diff --git a/apps/admin/src/shared/member-custom-fields/custom-field-picker.tsx b/apps/admin/src/shared/member-custom-fields/custom-field-picker.tsx index 15c84a04d07..d9ccc411b23 100644 --- a/apps/admin/src/shared/member-custom-fields/custom-field-picker.tsx +++ b/apps/admin/src/shared/member-custom-fields/custom-field-picker.tsx @@ -240,7 +240,14 @@ function CreateFieldInline({ } try { - const response = await createField({ name: name.trim(), type: typeId }); + // The ordinary closed default. This picker only renders where the setting for + // opening a field to members is also available, so a publisher who wants members + // to fill this in can say so; nothing here has to guess on their behalf. + const response = await createField({ + name: name.trim(), + type: typeId, + access: { member: 'none' }, + }); const field = response.members_metafields?.[0]; if (!field) { setSaveError('The field was created but could not be selected. Choose it from the list.'); From 11d5eac40b09766e452835e581c24be785b1c846 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 9 Sep 2026 12:02:06 +0100 Subject: [PATCH 08/17] Added a browser test for who a custom field is for ref https://linear.app/ghost/issue/BER-3864/differentiate-public-and-private-member-projections A publisher sets this in Settings and a member lives with the result on the other side of Ghost, through a different API, signed in as themselves. Until now nothing watched both ends at once: the Admin tests drive the control against a faked server, and the API tests prove the rules with no publisher and no browser, so a control that saved the setting somewhere the enforcement never read would leave both green. This defines a field through Settings, fills it in on a member, signs a real member in through the magic link and asks what they are told, then opens and closes the field and asks again. It reads the member's own side over its API rather than through Portal, which does not render these yet. Two of the Admin tests were the weaker half of what this now proves and have gone; what stays there is the disclosure warning, whose condition is the app's own, and the older backend that sends no setting at all, which no real Ghost can be made to serve. Also removes a self-referencing type on the definitions query. The type is read off the function's own signature, so naming it as the return type made it circular, which the main typecheck accepted as an implicit any and the E2E workspace's stricter one refused. --- .../sections/custom-fields-section.ts | 21 ++- .../member-custom-field-access.test.ts | 148 ++++++++++++++++++ .../test-data/src/selectors/settings.ts | 1 + 3 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 e2e/tests/admin/members/member-custom-field-access.test.ts diff --git a/e2e/helpers/pages/admin/settings/sections/custom-fields-section.ts b/e2e/helpers/pages/admin/settings/sections/custom-fields-section.ts index ddbf2dd7ba1..9da9c314174 100644 --- a/e2e/helpers/pages/admin/settings/sections/custom-fields-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/custom-fields-section.ts @@ -1,11 +1,14 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; import { + customFieldAccess, customFieldListItem, customFieldModal, customFields, } from '@tryghost/test-data/selectors/settings'; +export type CustomFieldAudience = 'Only staff' | 'Members can view' | 'Members can edit'; + /** * Settings -> Membership -> Custom fields. Defining fields is behind the * `membersCustomFields` flag, so a test using this section must enable that flag via @@ -29,7 +32,7 @@ export class CustomFieldsSection extends BasePage { } /** Creates a field of the named type. The modal closes itself on success. */ - async createField(name: string, type?: string): Promise { + async createField(name: string, type?: string, audience?: CustomFieldAudience): Promise { await this.addButton.waitFor(); await this.addButton.click(); await this.modal.getByLabel('Name').fill(name); @@ -39,10 +42,26 @@ export class CustomFieldsSection extends BasePage { await this.page.getByRole('option', { name: type, exact: true }).click(); } + if (audience) { + await this.chooseAudience(audience); + } + await this.modal.getByRole('button', { name: 'Save' }).click(); await this.listItem(name).waitFor(); } + async setAudience(name: string, audience: CustomFieldAudience): Promise { + await this.listItem(name).click(); + await this.chooseAudience(audience); + await this.modal.getByRole('button', { name: 'Save' }).click(); + await this.listItem(name).filter({ hasText: audience }).waitFor(); + } + + private async chooseAudience(audience: CustomFieldAudience): Promise { + await this.modal.getByTestId(customFieldAccess).click(); + await this.page.getByRole('option', { name: audience, exact: true }).click(); + } + /** Short text is the default type, and keeps the member detail editor a plain input. */ async createShortTextField(name: string): Promise { await this.createField(name); diff --git a/e2e/tests/admin/members/member-custom-field-access.test.ts b/e2e/tests/admin/members/member-custom-field-access.test.ts new file mode 100644 index 00000000000..d56e2ea8843 --- /dev/null +++ b/e2e/tests/admin/members/member-custom-field-access.test.ts @@ -0,0 +1,148 @@ +import { Browser, BrowserContext, Page } from '@playwright/test'; +import { Member, createMemberFactory } from '@/data-factory'; +import { MemberDetailsPage, SettingsPage } from '@/admin-pages'; +import { expect, test } from '@/helpers/playwright'; +import { signInAsMember } from '@/helpers/playwright/flows/sign-in'; +import { usePerTestIsolation } from '@/helpers/playwright/isolation'; + +// The member's side is read over its API rather than driven through Portal, which +// does not render these fields yet. Swap to real UI when it does. +usePerTestIsolation(); + +interface OfferedField { + key: string; + name: string; + access: { member: string }; +} + +async function asMember( + browser: Browser, + baseURL: string, + member: Member, +): Promise<{ context: BrowserContext; page: Page }> { + const context = await browser.newContext({ baseURL, extraHTTPHeaders: { Origin: baseURL } }); + const page = await context.newPage(); + await signInAsMember(page, member); + return { context, page }; +} + +async function fieldsOfferedTo(page: Page): Promise { + const response = await page.request.get('/members/api/member/metafields/custom/'); + expect(response.status()).toBe(200); + const { members_metafields: fields } = await response.json(); + return fields; +} + +const namesOf = (fields: OfferedField[]): string[] => fields.map((field) => field.name); + +async function offeredFieldNamed(page: Page, name: string): Promise { + const offered = (await fieldsOfferedTo(page)).find((field) => field.name === name); + if (!offered) { + throw new Error(`${name} is not among the fields offered to this member`); + } + return offered; +} + +// Undefined when the member is told nothing at all, which is distinct from being +// told they hold no values. +async function valuesHeldBy(page: Page): Promise | undefined> { + const response = await page.request.get('/members/api/member/'); + expect(response.status()).toBe(200); + const account = await response.json(); + return account.metafields?.custom; +} + +async function memberWrites(page: Page, key: string, value: string) { + return page.request.put('/members/api/member/', { + data: { metafields: { custom: { [key]: value } } }, + }); +} + +test.describe('Ghost Admin - Member custom field access', () => { + test.use({ labs: { membersCustomFields: true } }); + + test('a field kept to staff is invisible until the publisher opens it', async ({ + page, + browser, + baseURL, + }) => { + const fieldName = `Renewal note ${Date.now()}`; + const member = await createMemberFactory(page.request).create({ + name: 'Ada Lovelace', + email: `ada-field-access-${Date.now()}@ghost.org`, + }); + + const settingsPage = new SettingsPage(page); + const memberDetailsPage = new MemberDetailsPage(page); + + await settingsPage.goto(); + await settingsPage.customFieldsSection.createField(fieldName, undefined, 'Only staff'); + + await page.goto(`/ghost/#/members/${member.id}`); + await memberDetailsPage.setCustomFieldValue(fieldName, 'Renewing, do not chase'); + + const { context, page: memberPage } = await asMember(browser, baseURL!, member); + try { + expect(namesOf(await fieldsOfferedTo(memberPage))).not.toContain(fieldName); + expect(await valuesHeldBy(memberPage)).toBeUndefined(); + + await settingsPage.goto(); + await settingsPage.customFieldsSection.setAudience(fieldName, 'Members can view'); + + const offered = await offeredFieldNamed(memberPage, fieldName); + expect(offered.access).toEqual({ member: 'read' }); + expect((await valuesHeldBy(memberPage))?.[offered.key]).toBe('Renewing, do not chase'); + + const refused = await memberWrites(memberPage, offered.key, 'Chase away'); + expect(refused.status()).toBe(422); + expect((await refused.json()).errors[0].message).toMatch(/Cannot set custom field/); + } finally { + await context.close(); + } + }); + + test('a field opened to members is theirs to change, until it is closed again', async ({ + page, + browser, + baseURL, + }) => { + const fieldName = `Job title ${Date.now()}`; + const member = await createMemberFactory(page.request).create({ + name: 'Grace Hopper', + email: `grace-field-access-${Date.now()}@ghost.org`, + }); + + const settingsPage = new SettingsPage(page); + const memberDetailsPage = new MemberDetailsPage(page); + + await settingsPage.goto(); + await settingsPage.customFieldsSection.createField(fieldName, undefined, 'Members can edit'); + + const { context, page: memberPage } = await asMember(browser, baseURL!, member); + try { + const offered = await offeredFieldNamed(memberPage, fieldName); + expect(offered.access).toEqual({ member: 'write' }); + + const written = await memberWrites(memberPage, offered.key, 'Rear Admiral'); + expect(written.status()).toBe(200); + + await page.goto(`/ghost/#/members/${member.id}`); + await expect(memberDetailsPage.customFieldsCard.getByText('Rear Admiral')).toBeVisible(); + + await settingsPage.goto(); + await settingsPage.customFieldsSection.setAudience(fieldName, 'Only staff'); + + expect(namesOf(await fieldsOfferedTo(memberPage))).not.toContain(fieldName); + expect(await valuesHeldBy(memberPage)).toBeUndefined(); + + const refused = await memberWrites(memberPage, offered.key, 'Commodore'); + expect(refused.status()).toBe(422); + expect((await refused.json()).errors[0].message).toMatch(/Unknown custom field/); + + await page.goto(`/ghost/#/members/${member.id}`); + await expect(memberDetailsPage.customFieldsCard.getByText('Rear Admiral')).toBeVisible(); + } finally { + await context.close(); + } + }); +}); diff --git a/packages/testing/test-data/src/selectors/settings.ts b/packages/testing/test-data/src/selectors/settings.ts index 47153bb967f..6e060c99255 100644 --- a/packages/testing/test-data/src/selectors/settings.ts +++ b/packages/testing/test-data/src/selectors/settings.ts @@ -62,6 +62,7 @@ export const tiersSelect = 'tiers-select'; export const customFields = 'custom-fields'; export const customFieldListItem = 'custom-field-list-item'; export const customFieldModal = 'custom-field-modal'; +export const customFieldAccess = 'custom-field-access'; export const stripeModal = 'stripe-modal'; export const tiers = 'tiers'; export const tierDetailModal = 'tier-detail-modal'; From ad9e9c946306a92b9eeae376836217aef3be3a40 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 9 Sep 2026 12:02:07 +0100 Subject: [PATCH 09/17] Changed the browser tests to one timeout everywhere ref https://linear.app/ghost/issue/BER-3864/differentiate-public-and-private-member-projections A test had half as long to finish on a laptop as it had on CI, so a correct test could fail in front of the person writing it and pass once pushed. That is the worst way for a suite to be wrong: it costs an investigation every time, and it teaches everyone to disbelieve a local failure, which is the one signal that arrives early enough to be cheap. Half a minute is also not the difference between a fast iteration loop and a slow one, so the shorter budget was buying nothing to offset that. It was not a decision anybody made. The two were deliberately brought together at thirty seconds once; a later commit raised CI to sixty because CI needed longer, and splitting them again was a side effect of that rather than its intent. The identical branches left behind on the assertion timeout are the fossil of the same tidy-up, and go here too. --- e2e/playwright.config.mjs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/e2e/playwright.config.mjs b/e2e/playwright.config.mjs index a446f431ea9..9a58e833e9e 100644 --- a/e2e/playwright.config.mjs +++ b/e2e/playwright.config.mjs @@ -17,9 +17,11 @@ const getWorkerCount = () => { /** @type {import('@playwright/test').PlaywrightTestConfig} */ const config = { - timeout: process.env.CI ? 60 * 1000 : 30 * 1000, + // One budget for every environment. Splitting it lets a correct test fail on a + // laptop and pass on CI. + timeout: 60 * 1000, expect: { - timeout: process.env.CI ? 10 * 1000 : 10 * 1000, + timeout: 10 * 1000, }, retries: 0, // Retries open the door to flaky tests. If the test needs retries, it's not a good test or the app is broken. maxFailures: process.argv.includes('--ui') ? 0 : 1, From bbc8e8df112cd427561d40c54393e1c48e83ede8 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Wed, 9 Sep 2026 08:56:47 -0500 Subject: [PATCH 10/17] Moved both chip pickers onto one shared shell (#30609) no ref Consolidated the "add Tag" picker in the Posts/Pages list with the Editor sidebar tag picker. --- .../editor-settings-tags.acceptance.test.tsx | 5 +- apps/admin/src/editor/settings/README.md | 2 +- .../editor/settings/authors-options.test.ts | 30 +- .../src/editor/settings/authors-options.ts | 24 +- .../src/editor/settings/authors-picker.tsx | 298 +++---------- .../src/editor/settings/authors-section.tsx | 17 +- .../src/editor/settings/tags-section.tsx | 2 +- ...osts-list-bulk-actions.acceptance.test.tsx | 5 +- .../src/shared/pickers/chip-picker.test.tsx | 78 ++++ apps/admin/src/shared/pickers/chip-picker.tsx | 406 ++++++++++++++++++ apps/admin/src/shared/tags/tag-picker.tsx | 348 +++------------ .../src/shared/tags/tag-selection.test.ts | 33 -- apps/admin/src/shared/tags/tag-selection.ts | 18 - 13 files changed, 637 insertions(+), 629 deletions(-) create mode 100644 apps/admin/src/shared/pickers/chip-picker.test.tsx create mode 100644 apps/admin/src/shared/pickers/chip-picker.tsx diff --git a/apps/admin/src/editor/editor-settings-tags.acceptance.test.tsx b/apps/admin/src/editor/editor-settings-tags.acceptance.test.tsx index 20a203ebfcb..7f79da77c59 100644 --- a/apps/admin/src/editor/editor-settings-tags.acceptance.test.tsx +++ b/apps/admin/src/editor/editor-settings-tags.acceptance.test.tsx @@ -141,8 +141,9 @@ describe('Post settings tags', () => { await openSidebar(); await openTagList(); - // No row shows a post count, so the read does not ask for the join. - await expect.poll(() => tagsApi.lastRequest?.url, POLL).not.toContain('count.posts'); + await expect.poll(() => tagsApi.lastRequest?.url, POLL).toBeDefined(); + // Ghost rejects an explicitly empty include instead of treating it as absent. + expect(new URL(tagsApi.lastRequest!.url).searchParams.get('include')).not.toBe(''); await editorScreen.settingsTagOption('Sport').click(); diff --git a/apps/admin/src/editor/settings/README.md b/apps/admin/src/editor/settings/README.md index 132d00a33e3..521c191f54b 100644 --- a/apps/admin/src/editor/settings/README.md +++ b/apps/admin/src/editor/settings/README.md @@ -251,7 +251,7 @@ ignoring case and accents, and it leaves out anyone already credited. Arrow keys move the highlight, Enter takes the highlighted row and so does Tab once something has been typed, Escape closes the list and keeps the term, and both clicking away and moving focus out of the field close it and discard the term. A -chip goes with its own remove button, and Backspace in an empty field drops the +chip is removed by clicking it, and Backspace in an empty field drops the last one and opens the list on the staff it can offer again. A pick that empties the row under the highlight moves it to the last row rather than losing it. diff --git a/apps/admin/src/editor/settings/authors-options.test.ts b/apps/admin/src/editor/settings/authors-options.test.ts index 666a370dd80..35ee2d111af 100644 --- a/apps/admin/src/editor/settings/authors-options.test.ts +++ b/apps/admin/src/editor/settings/authors-options.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; import type { User } from '@tryghost/admin-x-framework/api/users'; -import { authorSuggestions, matchesAuthor, selectedAuthors } from './authors-options'; +import { matchesAuthor, selectedAuthors, toAuthorOption } from './authors-options'; function user(overrides: Partial): User { return { @@ -36,29 +36,17 @@ describe('matchesAuthor', () => { }); }); -describe('authorSuggestions', () => { - it('offers everyone who is not already an author', () => { - const suggestions = authorSuggestions( - [JANE, JOSE], - [{ id: '1', name: 'Jane Doe', email: '' }], - '', - ); - - expect(suggestions.map(({ id }) => id)).toEqual(['2']); - }); - - it('narrows the offer by the typed term', () => { - expect(authorSuggestions([JANE, JOSE], [], 'jane').map(({ id }) => id)).toEqual(['1']); +describe('toAuthorOption', () => { + it('keeps the staff member’s own name', () => { + expect(toAuthorOption(JANE)).toEqual({ id: '1', name: 'Jane Doe', email: 'jane@example.com' }); }); it('falls back to the email when a staff member has no name', () => { - expect(authorSuggestions([NAMELESS], [], '')).toEqual([ - { id: '3', name: 'ghost@example.com', email: 'ghost@example.com' }, - ]); - }); - - it('answers with nothing before the staff browse has loaded', () => { - expect(authorSuggestions(undefined, [], '')).toEqual([]); + expect(toAuthorOption(NAMELESS)).toEqual({ + id: '3', + name: 'ghost@example.com', + email: 'ghost@example.com', + }); }); }); diff --git a/apps/admin/src/editor/settings/authors-options.ts b/apps/admin/src/editor/settings/authors-options.ts index 5dfe45ffb34..4c94754d72c 100644 --- a/apps/admin/src/editor/settings/authors-options.ts +++ b/apps/admin/src/editor/settings/authors-options.ts @@ -22,8 +22,13 @@ function fold(value: string): string { .toLowerCase(); } -function toOption(user: Pick): AuthorOption { - return { id: user.id, name: user.name || user.email, email: user.email }; +/** What a staff member reads as: their name, or their email when they have none. */ +export function authorName(person: Pick): string { + return person.name || person.email; +} + +export function toAuthorOption(user: Pick): AuthorOption { + return { id: user.id, name: authorName(user), email: user.email }; } /** @@ -35,19 +40,6 @@ export function matchesAuthor(user: Pick, term: return [user.name, user.slug, user.email].some((field) => fold(field ?? '').includes(needle)); } -/** The rows the list offers: everyone not already an author, narrowed by the term. */ -export function authorSuggestions( - users: User[] | undefined, - selected: ReadonlyArray, - term: string, -): AuthorOption[] { - const chosen = new Set(selected.map(({ id }) => id)); - const trimmed = term.trim(); - return (users ?? []) - .filter((user) => !chosen.has(user.id) && (!trimmed || matchesAuthor(user, trimmed))) - .map(toOption); -} - /** * The chips, in the post's own order. A saved post carries its authors' names, * so a chip is named whether or not the staff browse has run. @@ -65,7 +57,7 @@ export function selectedAuthors( const user = known.get(author.id); options.push( user - ? toOption(user) + ? toAuthorOption(user) : { id: author.id, name: author.name || author.email || author.id, diff --git a/apps/admin/src/editor/settings/authors-picker.tsx b/apps/admin/src/editor/settings/authors-picker.tsx index 8bde79d24f5..3c1022a17a6 100644 --- a/apps/admin/src/editor/settings/authors-picker.tsx +++ b/apps/admin/src/editor/settings/authors-picker.tsx @@ -1,14 +1,14 @@ -import { useCallback, useEffect, useId, useRef, useState } from 'react'; -import type { KeyboardEvent } from 'react'; -import { Badge, Button, inputSurface } from '@tryghost/shade/components'; +import { useCallback, useRef } from 'react'; +import { Button } from '@tryghost/shade/components'; import { Stack, Text } from '@tryghost/shade/primitives'; -import { cn, LucideIcon } from '@tryghost/shade/utils'; +import type { User } from '@tryghost/admin-x-framework/api/users'; import { settingsAuthorChip, settingsAuthorsList, settingsAuthorsPicker, } from '@tryghost/test-data/selectors/editor'; -import type { AuthorOption } from './authors-options'; +import { ChipPicker } from '@/shared/pickers/chip-picker'; +import { authorName, matchesAuthor, toAuthorOption, type AuthorOption } from './authors-options'; export interface AuthorsPickerProps { inputId: string; @@ -16,16 +16,14 @@ export interface AuthorsPickerProps { invalid: boolean; /** The post's authors, in order. */ selected: AuthorOption[]; - /** Everyone else, already narrowed by the typed term. */ - suggestions: AuthorOption[]; + /** The site's staff, whoever the browse has answered with so far. */ + staff: User[]; loading: boolean; loadError: boolean; onRetry: () => void; onChange: (next: AuthorOption[]) => void; /** The first open; the staff browse starts here rather than on every editor entry. */ onOpen: () => void; - onSearch: (term: string) => void; - term: string; } /** @@ -38,246 +36,76 @@ export function AuthorsPicker({ describedBy, invalid, selected, - suggestions, + staff, loading, loadError, onRetry, onChange, onOpen, - onSearch, - term, }: AuthorsPickerProps) { - const listId = useId(); - const optionId = (index: number) => `${listId}-${index}`; - const [open, setOpen] = useState(false); - const [highlighted, setHighlighted] = useState(0); const inputRef = useRef(null); - const containerRef = useRef(null); - const listRef = useRef(null); const showLoadError = loadError && !loading; - // The term goes with the list when the writer leaves the field: left behind, - // it reads as an edit that nothing will ever commit. - const closeAndDiscard = useCallback(() => { - setOpen(false); - onSearch(''); - }, [onSearch]); - - // Dismissal on `pointerdown` without preventing the default, so the click - // that follows still reaches whatever the writer aimed at. - useEffect(() => { - if (!open) { - return; - } - - const handlePointerDown = (event: PointerEvent) => { - if (containerRef.current && !containerRef.current.contains(event.target as Node)) { - closeAndDiscard(); - } - }; - - document.addEventListener('pointerdown', handlePointerDown); - - return () => document.removeEventListener('pointerdown', handlePointerDown); - }, [closeAndDiscard, open]); - - // Back to the first row whenever the list narrows, so the highlight never - // points past the end of what is on screen. - useEffect(() => { - setHighlighted(0); - }, [term]); - - // A pick can shrink the list under the highlight, which leaves the stored - // index past the end until the next arrow key moves it. - const highlightedIndex = Math.min(highlighted, Math.max(suggestions.length - 1, 0)); - - useEffect(() => { - listRef.current - ?.querySelector('[data-highlighted="true"]') - ?.scrollIntoView({ block: 'nearest' }); - }, [highlightedIndex, open]); - - const reveal = () => { - setOpen(true); - onOpen(); - }; - - const choose = (option: AuthorOption) => { - onChange([...selected, option]); - // Cleared either way: leaving the term in the field means the next thing - // typed appends to a search already acted on. - onSearch(''); - }; - - const remove = (option: AuthorOption) => { - onChange(selected.filter((author) => author.id !== option.id)); - }; - - const handleKeyDown = (event: KeyboardEvent) => { - // IME confirmation and candidate navigation belong to the input method. - // Safari can end composition before its confirmation keydown, reporting 229. - if (event.nativeEvent.isComposing || event.nativeEvent.keyCode === 229) { - return; - } - - if (event.key === 'Backspace' && !term && selected.length > 0) { - remove(selected[selected.length - 1]); - // The list comes back with the removed author in it (gh-token-input.js). - reveal(); - return; - } - - if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { - event.preventDefault(); - - if (!open) { - reveal(); - return; + // Stable: a new handler each render would re-register the shell's document + // listeners for as long as the list is open. + const handleOpenChange = useCallback( + (open: boolean) => { + if (open) { + onOpen(); } - - if (suggestions.length > 0) { - const step = event.key === 'ArrowDown' ? 1 : -1; - - setHighlighted((highlightedIndex + step + suggestions.length) % suggestions.length); - } - - return; - } - - // Escape keeps the term: the writer closed the list, not the search. - if (event.key === 'Escape' && open) { - event.preventDefault(); - event.stopPropagation(); - setOpen(false); - return; - } - - const commits = event.key === 'Enter' || (event.key === 'Tab' && term.trim().length > 0); - if (commits && open && !showLoadError && suggestions[highlightedIndex]) { - event.preventDefault(); - choose(suggestions[highlightedIndex]); - } - }; + }, + [onOpen], + ); return ( -
{ - if (!event.currentTarget.contains(event.relatedTarget)) { - closeAndDiscard(); - } - }} - > -
{ - inputRef.current?.focus(); - reveal(); - }} - > - {selected.map((author) => ( - - {author.name} - - - ))} - { - onSearch(event.target.value); - reveal(); - }} - onKeyDown={handleKeyDown} - /> - -
- {open && ( -
- {showLoadError && ( - - - Couldn’t load authors. - - - - )} -
- {!showLoadError && suggestions.length === 0 && ( -
- {loading ? 'Loading authors...' : 'No authors found'} -
- )} - {!showLoadError && - suggestions.map((option, index) => { - const isHighlighted = index === highlightedIndex; - - return ( -
choose(option)} - // Keeps focus in the input, which clicking a plain div would - // otherwise drop, so the writer can keep typing after picking. - onMouseDown={(event) => event.preventDefault()} - onMouseEnter={() => setHighlighted(index)} - > - {option.name} - - {option.email} - -
- ); - })} -
-
+ Retry + + + ) : null + } + options={staff} + placeholder="Select authors..." + renderOption={(person) => ( + <> + {authorName(person)} + {person.email} + )} -
+ selected={selected} + testIds={{ + field: settingsAuthorsPicker, + list: settingsAuthorsList, + chip: settingsAuthorChip, + }} + hideSelected + reopenOnRemove + onAdd={(person) => onChange([...selected, toAuthorOption(person)])} + onOpenChange={handleOpenChange} + onRemove={(key) => onChange(selected.filter((author) => author.id !== key))} + /> ); } diff --git a/apps/admin/src/editor/settings/authors-section.tsx b/apps/admin/src/editor/settings/authors-section.tsx index 4d62539c645..b8d57615e98 100644 --- a/apps/admin/src/editor/settings/authors-section.tsx +++ b/apps/admin/src/editor/settings/authors-section.tsx @@ -1,4 +1,4 @@ -import { useId, useState } from 'react'; +import { useCallback, useId, useState } from 'react'; import { Label } from '@tryghost/shade/components'; import { Text } from '@tryghost/shade/primitives'; import { useBrowseUsers, type User } from '@tryghost/admin-x-framework/api/users'; @@ -9,12 +9,7 @@ import { AUTHORS_REQUIRED } from '@/editor/session/settings-fields'; import type { EditorSessionHandle } from '@/editor/session/use-editor-session'; import { SettingsSection } from './settings-section'; import { AuthorsPicker } from './authors-picker'; -import { - AUTHORS_SEARCH_PARAMS, - authorSuggestions, - selectedAuthors, - type AuthorOption, -} from './authors-options'; +import { AUTHORS_SEARCH_PARAMS, selectedAuthors, type AuthorOption } from './authors-options'; export interface AuthorsSectionProps { session: EditorSessionHandle; @@ -30,7 +25,7 @@ export function AuthorsSection({ session, currentUser }: AuthorsSectionProps) { const inputId = useId(); const errorId = useId(); const [browsing, setBrowsing] = useState(false); - const [term, setTerm] = useState(''); + const startBrowsing = useCallback(() => setBrowsing(true), []); const { data, isFetching, isError, refetch } = useBrowseUsers({ defaultErrorHandler: false, @@ -60,12 +55,10 @@ export function AuthorsSection({ session, currentUser }: AuthorsSectionProps) { loadError={isError} loading={isFetching} selected={selected} - suggestions={authorSuggestions(data?.users, selected, term)} - term={term} + staff={data?.users ?? []} onChange={change} - onOpen={() => setBrowsing(true)} + onOpen={startBrowsing} onRetry={() => void refetch()} - onSearch={setTerm} /> {invalid ? ( { await postsListScreen.tagSearchInput().fill('C++'); await expect.poll(() => tags.lastRequest?.filter).toContain("tags.name:~'C++'"); + // An empty include makes the real API refuse the search with withRelated. + expect(new URL(tags.lastRequest!.url).searchParams.get('include')).not.toBe(''); await expect.element(postsListScreen.tagOption('C++')).toBeVisible(); await expect(postsListScreen.tagOption(/Create/)).toHaveCount(0); }); @@ -419,7 +421,8 @@ describe('Posts list bulk actions', () => { await postsListScreen.contextMenuItem('Add a tag').click(); await postsListScreen.tagPickerField().click(); - // First closes the list, second reaches the dialog. + // With nothing typed one Escape does both: the dialog's guard stands + // aside and the list closes under it. The second has nothing left to do. await userEvent.keyboard('{Escape}'); await userEvent.keyboard('{Escape}'); diff --git a/apps/admin/src/shared/pickers/chip-picker.test.tsx b/apps/admin/src/shared/pickers/chip-picker.test.tsx new file mode 100644 index 00000000000..5ee8600931f --- /dev/null +++ b/apps/admin/src/shared/pickers/chip-picker.test.tsx @@ -0,0 +1,78 @@ +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { useEffect, useState } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { ChipPicker } from './chip-picker'; + +interface Option { + id: string; + name: string; +} + +const scrollIntoView = Object.getOwnPropertyDescriptor(Element.prototype, 'scrollIntoView'); + +beforeEach(() => { + Object.defineProperty(Element.prototype, 'scrollIntoView', { + configurable: true, + value: vi.fn(), + }); +}); + +afterEach(() => { + cleanup(); + if (scrollIntoView) { + Object.defineProperty(Element.prototype, 'scrollIntoView', scrollIntoView); + } else { + Reflect.deleteProperty(Element.prototype, 'scrollIntoView'); + } +}); + +/** The settings sidebar's pane, reduced to the Escape listener it dismisses on. */ +function Pane() { + const [open, setOpen] = useState(true); + + useEffect(() => { + if (!open) { + return; + } + + const onKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape' && !event.defaultPrevented) { + setOpen(false); + } + }; + + window.addEventListener('keydown', onKeyDown); + return () => window.removeEventListener('keydown', onKeyDown); + }, [open]); + + if (!open) { + return

Pane closed

; + } + + return ( + + emptyMessage="No options found" + getKey={(option) => option.id} + getLabel={(option) => option.name} + inputLabel="Options" + options={[{ id: 'alpha', name: 'Alpha' }]} + placeholder="Select options..." + selected={[]} + onAdd={vi.fn()} + onRemove={vi.fn()} + /> + ); +} + +describe('ChipPicker Escape', () => { + it('closes the list without dismissing the pane around it', () => { + render(); + fireEvent.click(screen.getByRole('combobox')); + expect(screen.getByRole('listbox')).toBeVisible(); + + fireEvent.keyDown(screen.getByRole('combobox'), { key: 'Escape' }); + + expect(screen.queryByRole('listbox')).not.toBeInTheDocument(); + expect(screen.queryByText('Pane closed')).not.toBeInTheDocument(); + }); +}); diff --git a/apps/admin/src/shared/pickers/chip-picker.tsx b/apps/admin/src/shared/pickers/chip-picker.tsx new file mode 100644 index 00000000000..003decafe3d --- /dev/null +++ b/apps/admin/src/shared/pickers/chip-picker.tsx @@ -0,0 +1,406 @@ +import { badgeVariants, inputSurface } from '@tryghost/shade/components'; +import { cn, LucideIcon } from '@tryghost/shade/utils'; +import { useCallback, useEffect, useId, useRef, useState } from 'react'; +import type { KeyboardEvent, ReactNode, RefObject } from 'react'; + +/** Trailing and leading space is never part of what is picked. */ +const trimTerm = (search: string) => search.trim(); + +/** The offer to commit what was typed as something the list does not hold yet. */ +export interface ChipPickerCreateRow { + /** Whether the term is worth offering, given the rows already on screen. */ + offer: (term: string, offered: readonly TOption[]) => boolean; + render: (term: string) => ReactNode; + onSelect: (term: string) => void; +} + +export interface ChipPickerTestIds { + field?: string; + input?: string; + list?: string; + chip?: string; +} + +export interface ChipPickerProps { + /** Everything the list can offer, in the order it should read. */ + options: ReadonlyArray; + /** What the field already carries, in the order it should read. */ + selected: ReadonlyArray; + getKey: (item: TOption | TChip) => string; + getLabel: (item: TOption | TChip) => string; + /** Whether a row is a chip the field already carries. Keys are the default test. */ + isChosen?: (option: TOption, chip: TChip) => boolean; + renderOption?: (option: TOption, state: { chosen: boolean }) => ReactNode; + chipVariant?: (chip: TChip) => 'default' | 'secondary'; + onAdd: (option: TOption) => void; + onRemove: (key: string) => void; + /** Leaves the chosen rows out of the list rather than listing them ticked. */ + hideSelected?: boolean; + /** Narrows the rows by the typed term, for a list the server has not narrowed. */ + matches?: (option: TOption, term: string) => boolean; + /** Reduces what is typed to the term the rows and the create offer read against. */ + normalizeTerm?: (search: string) => string; + createRow?: ChipPickerCreateRow; + /** Reports what is typed, for a caller that must know there is work to lose. */ + onSearchChange?: (search: string) => void; + onOpenChange?: (open: boolean) => void; + /** Opens the list again on the row a Backspace removal handed back. */ + reopenOnRemove?: boolean; + /** Stands in for the rows, for a list with nothing it can offer. */ + notice?: ReactNode; + /** Hands the caller the search input, for a notice that puts focus back on it. */ + inputRef?: RefObject; + emptyMessage: string; + placeholder: string; + /** The accessible name of the field and of the list it opens. */ + inputLabel: string; + /** Ties the input to a visible label the caller renders. */ + inputId?: string; + describedBy?: string; + invalid?: boolean; + maxLength?: number; + testIds?: ChipPickerTestIds; +} + +type Row = + | { kind: 'option'; key: string; option: TOption; chosen: boolean } + | { kind: 'create'; key: string }; + +/** + * The chips-in-a-field picker: what the field carries drawn as removable chips, + * and a list of what else it could carry under a search input. + * + * The list is built by hand rather than with `cmdk`: cmdk only drives the + * keyboard for an input inside its own tree, and this input sits in the chip + * field above the list. + */ +export function ChipPicker({ + options, + selected, + getKey, + getLabel, + isChosen, + renderOption, + chipVariant, + onAdd, + onRemove, + hideSelected = false, + matches, + normalizeTerm = trimTerm, + createRow, + onSearchChange, + onOpenChange, + reopenOnRemove = false, + notice, + inputRef, + emptyMessage, + placeholder, + inputLabel, + inputId, + describedBy, + invalid, + maxLength, + testIds, +}: ChipPickerProps) { + const listId = useId(); + const [open, setOpen] = useState(false); + const [search, setSearch] = useState(''); + const [highlighted, setHighlighted] = useState(0); + const ownInputRef = useRef(null); + const input = inputRef ?? ownInputRef; + const containerRef = useRef(null); + const listRef = useRef(null); + + // One term behind the rows and the create offer, and behind whatever the + // caller queries with: a caller that normalises differently hands its own in. + const term = normalizeTerm(search); + + const updateSearch = (value: string) => { + setSearch(value); + onSearchChange?.(value); + }; + + const reveal = () => { + setOpen(true); + onOpenChange?.(true); + }; + + // Backs out of the list only: the term stays for the caller that reads it. + const close = useCallback(() => { + setOpen(false); + onOpenChange?.(false); + }, [onOpenChange]); + + // The term goes with the list when the writer leaves the field: left behind, + // it reads as an edit that nothing will ever commit. + const closeAndDiscard = useCallback(() => { + setOpen(false); + onOpenChange?.(false); + setSearch(''); + onSearchChange?.(''); + }, [onOpenChange, onSearchChange]); + + // Not a Radix Popover: portalled, it would sit outside a Dialog's subtree + // where the scroll-lock blocks it. Closing on `pointerdown` without + // preventing the default lets the `click` that follows reach what is under it. + useEffect(() => { + if (!open) { + return; + } + + const handlePointerDown = (event: PointerEvent) => { + if (containerRef.current && !containerRef.current.contains(event.target as Node)) { + closeAndDiscard(); + } + }; + + document.addEventListener('pointerdown', handlePointerDown); + + return () => document.removeEventListener('pointerdown', handlePointerDown); + }, [closeAndDiscard, open]); + + // On the document, not the input: clicking a row with the mouse moves focus + // off the input, and an Escape after that never reached a handler bound there. + useEffect(() => { + if (!open) { + return; + } + + const handleKeyDown = (event: globalThis.KeyboardEvent) => { + if (event.isComposing || event.keyCode === 229) { + return; + } + // Capture runs after any Radix layer and before an enclosing pane's bubble + // listener; claiming the key keeps the pane from closing with the list. + if (event.key === 'Escape') { + event.preventDefault(); + close(); + } + }; + + document.addEventListener('keydown', handleKeyDown, true); + + return () => document.removeEventListener('keydown', handleKeyDown, true); + }, [close, open]); + + // Back to the top whenever the list narrows, so the highlight never points + // past the end of what is on screen. + useEffect(() => { + setHighlighted(0); + }, [term]); + + const chipFor = (option: TOption) => + selected.find((chip) => (isChosen ? isChosen(option, chip) : getKey(option) === getKey(chip))); + + const narrowed = + matches && term !== '' ? options.filter((option) => matches(option, term)) : options; + const offered = hideSelected ? narrowed.filter((option) => !chipFor(option)) : narrowed; + const rows: Row[] = notice + ? [] + : [ + ...offered.map((option) => ({ + kind: 'option' as const, + key: getKey(option), + option, + chosen: Boolean(chipFor(option)), + })), + ...(createRow?.offer(term, offered) ? [{ kind: 'create' as const, key: 'create' }] : []), + ]; + + // A pick can shrink the list under the highlight, which leaves the stored + // index past the end until the next arrow key moves it. + const highlightedIndex = Math.min(highlighted, Math.max(rows.length - 1, 0)); + + // Keeps the highlighted row in view while arrowing through a long list. + useEffect(() => { + listRef.current + ?.querySelector('[data-highlighted="true"]') + ?.scrollIntoView({ block: 'nearest' }); + }, [highlightedIndex, open]); + + const choose = (row: Row) => { + if (row.kind === 'create') { + createRow?.onSelect(term); + } else { + const chip = chipFor(row.option); + + if (chip) { + onRemove(getKey(chip)); + } else { + onAdd(row.option); + } + } + // Cleared either way: leaving the term in the field means the next thing + // typed appends to a search already acted on. + updateSearch(''); + }; + + const handleKeyDown = (event: KeyboardEvent) => { + // IME confirmation and candidate navigation belong to the input method. + // Safari can end composition before its confirmation keydown, reporting 229. + if (event.nativeEvent.isComposing || event.nativeEvent.keyCode === 229) { + return; + } + + // Backspace on an empty field removes the last chip — the chips are + // otherwise only removable by mouse. + if (event.key === 'Backspace' && search === '' && selected.length > 0) { + onRemove(getKey(selected[selected.length - 1])); + + if (reopenOnRemove) { + reveal(); + } + + return; + } + + if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { + event.preventDefault(); + + if (!open) { + reveal(); + return; + } + + if (rows.length > 0) { + const step = event.key === 'ArrowDown' ? 1 : -1; + + setHighlighted((highlightedIndex + step + rows.length) % rows.length); + } + + return; + } + + // Tab commits the highlighted row rather than moving on and dropping what + // was typed. With nothing typed there is nothing to lose, so it keeps its + // own job and moves focus out of the field. + const commits = event.key === 'Enter' || (event.key === 'Tab' && term !== ''); + + if (commits && open && rows[highlightedIndex]) { + event.preventDefault(); + choose(rows[highlightedIndex]); + } + }; + + const optionId = (index: number) => `${listId}-option-${index}`; + + return ( +
{ + if (!event.currentTarget.contains(event.relatedTarget)) { + closeAndDiscard(); + } + }} + > +
{ + input.current?.focus(); + reveal(); + }} + > + {/* The whole chip removes, so it is the button rather than carrying one. */} + {selected.map((chip) => ( + + ))} + { + updateSearch(event.target.value); + reveal(); + }} + onKeyDown={handleKeyDown} + /> + {/* Says the field opens a list; without it a bordered box with a + placeholder reads as a plain text input. */} + +
+ {open && ( +
+ {notice} +
+ {!notice && rows.length === 0 && ( +
{emptyMessage}
+ )} + {rows.map((row, index) => { + const isHighlighted = index === highlightedIndex; + const chosen = row.kind === 'option' && row.chosen; + + return ( +
choose(row)} + // Keeps focus in the input, which clicking a plain div would + // otherwise drop, so typing and Escape both still work after a pick. + onMouseDown={(event) => event.preventDefault()} + onMouseEnter={() => setHighlighted(index)} + > + {row.kind === 'create' + ? createRow?.render(term) + : (renderOption?.(row.option, { chosen }) ?? ( + {getLabel(row.option)} + ))} +
+ ); + })} +
+
+ )} +
+ ); +} diff --git a/apps/admin/src/shared/tags/tag-picker.tsx b/apps/admin/src/shared/tags/tag-picker.tsx index 187b10f8a06..5b474d96160 100644 --- a/apps/admin/src/shared/tags/tag-picker.tsx +++ b/apps/admin/src/shared/tags/tag-picker.tsx @@ -1,17 +1,15 @@ -import { badgeVariants, inputSurface } from '@tryghost/shade/components'; import { cn, LucideIcon } from '@tryghost/shade/utils'; -import { useCallback, useEffect, useId, useRef, useState } from 'react'; -import type { KeyboardEvent } from 'react'; +import { useCallback, useState } from 'react'; import { useDebounce } from 'use-debounce'; import { escapeNqlString } from '@tryghost/nql-string'; import { useBrowseTags, type Tag } from '@tryghost/admin-x-framework/api/tags'; +import { ChipPicker } from '@/shared/pickers/chip-picker'; import { - availableTags, canCreateTag, isInternalTag, - matchingTags, normalizeTagName, sameTag, + sortTagsByName, tagKey, tagName, type PickedTag, @@ -38,19 +36,13 @@ interface TagPickerProps { /** Reports what is typed, for a caller that must know there is work to lose. */ onSearchChange?: (search: string) => void; maxLength?: number; - testIds?: { field?: string; input?: string; list?: string; token?: string }; + testIds?: { field?: string; input?: string; list?: string; chip?: string }; } -/** A row in the list: an existing tag, or the offer to create what was typed. */ -type PickerOption = { kind: 'tag'; tag: Tag } | { kind: 'create'; name: string }; - /** - * The chips-in-a-field tag picker, shared by the editor's settings sidebar and - * the posts list's bulk "Add tags". - * - * The list is built by hand rather than with `cmdk`: cmdk only drives the - * keyboard for an input inside its own tree, and this input sits in the chip - * field above the list. + * The tag picker, shared by the editor's settings sidebar and the posts list's + * bulk "Add tags": the site's tags read from the server as the writer types, + * with what is typed offered as a new tag when nothing already carries it. */ export function TagPicker({ selected, @@ -66,13 +58,8 @@ export function TagPicker({ maxLength, testIds, }: TagPickerProps) { - const listId = useId(); const [open, setOpen] = useState(false); const [search, setSearch] = useState(''); - const [highlighted, setHighlighted] = useState(0); - const inputRef = useRef(null); - const containerRef = useRef(null); - const listRef = useRef(null); const term = normalizeTagName(search); const [debouncedTerm] = useDebounce(term, SEARCH_DEBOUNCE_MS); @@ -85,286 +72,69 @@ export function TagPicker({ searchParams: { limit: TAG_PAGE_LIMIT, order: 'name asc', - // No row shows a post count, so the join behind one is wasted per keystroke. - include: '', ...(debouncedTerm ? { filter: `tags.name:~${escapeNqlString(debouncedTerm)}` } : {}), }, }); const tags = data?.tags ?? []; - - const updateSearch = (value: string) => { - setSearch(value); - onSearchChange?.(value); - }; - - const close = useCallback(() => { - setOpen(false); - }, []); - - // The term goes with the list when the writer leaves the field: left behind, - // it reads as an edit that nothing will ever commit. - const closeAndDiscard = useCallback(() => { - setOpen(false); - setSearch(''); - onSearchChange?.(''); - }, [onSearchChange]); - - // Not a Radix Popover: portalled, it would sit outside a Dialog's subtree - // where the scroll-lock blocks it. Closing on `pointerdown` without - // preventing the default lets the `click` that follows reach what is under - // the list, which is how one click on the bulk dialog's Add both works. - useEffect(() => { - if (!open) { - return; - } - - const handlePointerDown = (event: PointerEvent) => { - if (containerRef.current && !containerRef.current.contains(event.target as Node)) { - closeAndDiscard(); - } - }; - - document.addEventListener('pointerdown', handlePointerDown); - - return () => document.removeEventListener('pointerdown', handlePointerDown); - }, [closeAndDiscard, open]); - - // On the document, not the input: clicking a row with the mouse moves focus - // off the input, and an Escape after that never reached a handler bound there. - // Escape backs out of the list only; the term stays for the caller that reads - // it to decide whether the writer has work to lose. - useEffect(() => { - if (!open) { - return; - } - - const handleKeyDown = (event: globalThis.KeyboardEvent) => { - if (event.isComposing || event.keyCode === 229) { - return; - } - if (event.key === 'Escape') { - close(); - } - }; - - document.addEventListener('keydown', handleKeyDown, true); - - return () => document.removeEventListener('keydown', handleKeyDown, true); - }, [close, open]); - - // Back to the top whenever the list narrows, so the highlight never points - // past the end of what is on screen. - useEffect(() => { - setHighlighted(0); - }, [term]); - - const matches = hideSelected ? availableTags(tags, selected, term) : matchingTags(tags, term); // Held back until the server has answered for what is typed, or a term still // being searched would offer to create a tag that already exists. const searchSettled = !isFetching && term === debouncedTerm; - const options: PickerOption[] = [ - ...matches.map((tag) => ({ kind: 'tag' as const, tag })), - ...(searchSettled && canCreateTag(term, matches, selected) - ? [{ kind: 'create' as const, name: term }] - : []), - ]; - // A pick can shrink the list under the highlight, which leaves the stored - // index past the end until the next arrow key moves it. - const highlightedIndex = Math.min(highlighted, Math.max(options.length - 1, 0)); - - // Keeps the highlighted row in view while arrowing through a long list. - useEffect(() => { - listRef.current - ?.querySelector('[data-highlighted="true"]') - ?.scrollIntoView({ block: 'nearest' }); - }, [highlightedIndex, open]); - const choose = (option: PickerOption) => { - if (option.kind === 'tag') { - const chosen = selected.find((tag) => sameTag(tag, option.tag)); - - if (chosen) { - onRemove(tagKey(chosen)); - } else { - onAdd({ id: option.tag.id, name: option.tag.name, slug: option.tag.slug }); - } - } else { - onAdd({ name: option.name }); - } - // Cleared either way: leaving the term in the field means the next thing - // typed appends to a search already acted on. - updateSearch(''); - }; - - const handleKeyDown = (event: KeyboardEvent) => { - // IME confirmation and candidate navigation belong to the input method. - // Safari can end composition before its confirmation keydown, reporting 229. - if (event.nativeEvent.isComposing || event.nativeEvent.keyCode === 229) { - return; - } - - // Backspace on an empty field removes the last chip, as the members picker - // does — the chips are otherwise only removable by mouse. - if (event.key === 'Backspace' && search === '' && selected.length > 0) { - onRemove(tagKey(selected[selected.length - 1])); - return; - } - - if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { - event.preventDefault(); - - if (!open) { - setOpen(true); - return; - } - - if (options.length > 0) { - const step = event.key === 'ArrowDown' ? 1 : -1; - - setHighlighted((highlightedIndex + step + options.length) % options.length); - } - - return; - } - - // Tab commits the highlighted row rather than moving on and dropping what - // was typed. With nothing typed there is nothing to lose, so it keeps its - // own job and moves focus out of the field. - const commits = event.key === 'Enter' || (event.key === 'Tab' && term !== ''); - - if (commits && open && options[highlightedIndex]) { - event.preventDefault(); - choose(options[highlightedIndex]); - } - }; - - const optionId = (index: number) => `${listId}-option-${index}`; + const handleSearchChange = useCallback( + (value: string) => { + setSearch(value); + onSearchChange?.(value); + }, + [onSearchChange], + ); return ( -
{ - if (!event.currentTarget.contains(event.relatedTarget)) { - closeAndDiscard(); - } + + chipVariant={(tag) => (isInternalTag(tag) ? 'default' : 'secondary')} + createRow={{ + offer: (typed, offered) => searchSettled && canCreateTag(typed, offered, selected), + render: (typed) => ( + <> + + Create “{typed}” + + ), + onSelect: (typed) => onAdd({ name: typed }), }} - > -
{ - inputRef.current?.focus(); - setOpen(true); - }} - > - {/* The whole chip removes, so it is the button rather than carrying one. */} - {selected.map((tag) => ( - - ))} - { - updateSearch(event.target.value); - setOpen(true); - }} - onKeyDown={handleKeyDown} - /> - {/* Says the field opens a list; without it a bordered box with a - placeholder reads as a plain text input. */} - -
- {open && ( -
- {options.length === 0 && ( -
No tags found
- )} - {options.map((option, index) => { - const isHighlighted = index === highlightedIndex; - const isChosen = - option.kind === 'tag' && selected.some((tag) => sameTag(tag, option.tag)); - - return ( -
choose(option)} - // Keeps focus in the input, which clicking a plain div would - // otherwise drop, so typing and Escape both still work after a pick. - onMouseDown={(event) => event.preventDefault()} - onMouseEnter={() => setHighlighted(index)} - > - {option.kind === 'create' ? ( - <> - - Create “{option.name}” - - ) : ( - <> - - {option.tag.name} - - {/* Names are not unique; the slug is what tells two of them apart. */} - - {option.tag.slug} - - {isChosen && } - - )} -
- ); - })} -
+ emptyMessage="No tags found" + getKey={tagKey} + getLabel={tagName} + hideSelected={hideSelected} + inputId={inputId} + inputLabel={inputLabel} + // Names are not unique, so what makes a row a chip is the id when both carry one. + isChosen={(tag, chip) => sameTag(chip, tag)} + matches={(tag, typed) => tagName(tag).toLowerCase().includes(typed.toLowerCase())} + maxLength={maxLength} + normalizeTerm={normalizeTagName} + options={sortTagsByName(tags)} + placeholder="Select or enter tags..." + renderOption={(tag, { chosen }) => ( + <> + {tag.name} + {/* Names are not unique; the slug is what tells two of them apart. */} + + {tag.slug} + + {chosen && } + )} -
+ selected={selected} + testIds={{ + field: testIds?.field ?? 'tag-picker', + input: testIds?.input, + list: testIds?.list, + chip: testIds?.chip, + }} + onAdd={(tag) => onAdd({ id: tag.id, name: tag.name, slug: tag.slug })} + onOpenChange={setOpen} + onRemove={onRemove} + onSearchChange={handleSearchChange} + /> ); } diff --git a/apps/admin/src/shared/tags/tag-selection.test.ts b/apps/admin/src/shared/tags/tag-selection.test.ts index e46233e9a4a..35cd0f925e7 100644 --- a/apps/admin/src/shared/tags/tag-selection.test.ts +++ b/apps/admin/src/shared/tags/tag-selection.test.ts @@ -1,10 +1,8 @@ import { describe, expect, it } from 'vitest'; import { addTag, - availableTags, canCreateTag, isInternalTag, - matchingTags, normalizeTagName, removeTag, sameTag, @@ -100,37 +98,6 @@ describe('sortTagsByName', () => { }); }); -describe('matchingTags', () => { - it('narrows by the term, case-insensitively, and keeps what is selected', () => { - const site = [ - { id: 't1', name: 'News' }, - { id: 't2', name: 'Newsletter' }, - { id: 't3', name: 'Sport' }, - ]; - - expect(matchingTags(site, 'news').map((tag) => tag.name)).toEqual(['News', 'Newsletter']); - }); -}); - -describe('availableTags', () => { - const site = [ - { id: 't1', name: 'News', slug: 'news' }, - { id: 't2', name: 'News', slug: 'news-2' }, - { id: 't3', name: 'Sport' }, - ]; - - it('leaves out the tag the post carries, not every tag sharing its name', () => { - expect(availableTags(site, [{ id: 't1', name: 'News' }], '').map((tag) => tag.slug)).toEqual([ - 'news-2', - undefined, - ]); - }); - - it('narrows by the term', () => { - expect(availableTags(site, [], 'sport').map((tag) => tag.name)).toEqual(['Sport']); - }); -}); - describe('canCreateTag', () => { it('offers a term nothing else carries', () => { expect(canCreateTag('Culture', [{ id: 't1', name: 'News' }], [])).toBe(true); diff --git a/apps/admin/src/shared/tags/tag-selection.ts b/apps/admin/src/shared/tags/tag-selection.ts index 5b6bcb0cb69..f5369b08bc7 100644 --- a/apps/admin/src/shared/tags/tag-selection.ts +++ b/apps/admin/src/shared/tags/tag-selection.ts @@ -82,24 +82,6 @@ export function sortTagsByName(tags: ReadonlyArray): T[] { ); } -/** The tags a search term matches, in name order. */ -export function matchingTags(tags: ReadonlyArray, term: string): T[] { - const needle = term.toLowerCase(); - - return sortTagsByName(tags).filter( - (tag) => needle === '' || tagName(tag).toLowerCase().includes(needle), - ); -} - -/** Those a selection does not already carry. */ -export function availableTags( - tags: ReadonlyArray, - selected: ReadonlyArray, - term: string, -): T[] { - return matchingTags(tags, term).filter((tag) => !selected.some((chosen) => sameTag(chosen, tag))); -} - /** * Whether the term is worth offering as a new tag. Anything already carrying * that name — offered a row below, or selected already — would be a duplicate. From c48b9d75a391b56cd89fb1901420ea10d30e4268 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 9 Sep 2026 11:40:50 +0100 Subject: [PATCH 11/17] Added a host limit that can switch custom member fields off per site Custom member fields are intended for one Ghost(Pro) plan and above, but nothing could express that. A labs flag says a feature does not exist yet, which is the wrong thing to tell someone whose plan simply does not include it, and it is the same answer for every site. This adds limitCustomFields alongside the other flag limits the host already sets, so the decision about which plan carries the feature is configuration on the hosting side rather than a change to Ghost. It stays open unless a host switches it off, so nothing changes for anyone until that configuration exists. The routes that exist only to change field definitions sit behind one guard, and Admin asks the same question before offering any of it, so a publisher whose plan excludes the feature is told so rather than shown a door that refuses them. --- apps/admin-x-framework/src/api/config.ts | 4 + .../custom-fields/import-members-modal.tsx | 3 +- apps/admin/src/settings/layout/sidebar.tsx | 3 +- .../custom-fields.acceptance.test.tsx | 37 ++++ .../src/settings/membership/custom-fields.tsx | 11 +- .../membership/membership-settings.tsx | 3 +- .../tiers/tier-checkout-collection.tsx | 9 +- .../member-custom-fields/use-availability.ts | 24 +++ ghost/core/core/server/services/limits.js | 16 ++ .../server/web/api/endpoints/admin/routes.js | 67 ++++--- .../admin/member-custom-fields.test.ts | 166 ++++++++++++++++++ packages/limit-service/lib/config.js | 1 + 12 files changed, 296 insertions(+), 48 deletions(-) create mode 100644 apps/admin/src/shared/member-custom-fields/use-availability.ts diff --git a/apps/admin-x-framework/src/api/config.ts b/apps/admin-x-framework/src/api/config.ts index cbf14912c41..618352db554 100644 --- a/apps/admin-x-framework/src/api/config.ts +++ b/apps/admin-x-framework/src/api/config.ts @@ -74,6 +74,10 @@ export type Config = { disabled: boolean; error?: string; }; + limitCustomFields?: { + disabled: boolean; + error?: string; + }; publicSiteAccess?: { disabled: boolean; // Copy shown in the pre-launch banner when public site access is disabled. 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 c76086f5d16..b9559ba8ca8 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 @@ -54,6 +54,7 @@ import { memberCustomFieldCsvColumns } from '@tryghost/admin-x-framework/api/mem 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 { useCustomFieldsAvailable } from '@/shared/member-custom-fields/use-availability'; import { useFeatureFlag } from '@tryghost/admin-x-framework/hooks'; import { useLabelPicker } from '@/members/hooks/use-label-picker'; @@ -75,7 +76,7 @@ export function ImportMembersModal({ const { mutateAsync: importMembers } = useImportMembers(); const importMemberTier = useFeatureFlag('importMemberTier'); - const canCreateCustomFields = useFeatureFlag('membersCustomFields'); + const canCreateCustomFields = useCustomFieldsAvailable(); 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 diff --git a/apps/admin/src/settings/layout/sidebar.tsx b/apps/admin/src/settings/layout/sidebar.tsx index 01f3819b984..1c9413e6050 100644 --- a/apps/admin/src/settings/layout/sidebar.tsx +++ b/apps/admin/src/settings/layout/sidebar.tsx @@ -31,6 +31,7 @@ import { searchKeywords as growthSearchKeywords } from '@/settings/growth/search import { searchKeywords as membershipSearchKeywords } from '@/settings/membership/search-keywords'; import { searchKeywords as siteSearchKeywords } from '@/settings/site/search-keywords'; +import { useCustomFieldsAvailable } from '@/shared/member-custom-fields/use-availability'; import { useFeatureFlag } from '@tryghost/admin-x-framework/hooks'; import { useGlobalData } from '@/settings/providers/global-data-context'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; @@ -124,7 +125,7 @@ const Sidebar: React.FC = () => { const paidMembersEnabled = usePaidMembersEnabled(); const hasStripeEnabled = checkStripeEnabled(settings || [], config || {}); const hasAutomations = useFeatureFlag('automations'); - const hasCustomFields = useFeatureFlag('membersCustomFields'); + const hasCustomFields = useCustomFieldsAvailable(); const hasNewslettersEnabled = useNewslettersEnabled() === true; const mailgunIsConfigured = Boolean(config.mailgunIsConfigured); const hasMailgun = hasNewslettersEnabled && !mailgunIsConfigured; diff --git a/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx b/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx index a8375fb33c6..50b8dc42938 100644 --- a/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx +++ b/apps/admin/src/settings/membership/custom-fields.acceptance.test.tsx @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import { page, userEvent } from 'vitest/browser'; import { + configResponse, fakeAdminEndpoint, fakeMemberCustomFields, fakeSettingsScreens, @@ -67,6 +68,42 @@ describe('Custom fields', () => { expect(customFieldsApi.requests).toHaveLength(0); }); + // A host can switch custom fields off for a site separately from the flag, so the + // feature can be sold with a plan. Settings is where a publisher would go to set fields + // up, so a limited site is offered nothing to set up. Reading definitions stays open on + // the server, so this is the check that stops a limited site being shown a section whose + // every save would come back refused. + it('stays hidden when the host limit disables the feature', async () => { + fakeSettingsScreens(); + const customFieldsApi = fakeCustomFields(); + const config = configResponse({ labs: { membersCustomFields: true } }); + config.config.hostSettings = { + limits: { limitCustomFields: { disabled: true } }, + }; + await renderAdminApp('/settings', { + ...flagOn, + boot: { browseConfig: { response: config } }, + }); + + await expect(settingsScreen.customFields()).toHaveCount(0); + expect(customFieldsApi.requests).toHaveLength(0); + }); + + it('stays visible when the host sets the limit but leaves it enabled', async () => { + fakeSettingsScreens(); + fakeCustomFields(); + const config = configResponse({ labs: { membersCustomFields: true } }); + config.config.hostSettings = { + limits: { limitCustomFields: { disabled: false } }, + }; + await renderAdminApp('/settings', { + ...flagOn, + boot: { browseConfig: { response: config } }, + }); + + await expect.element(settingsScreen.customFields()).toBeVisible(); + }); + it('lists each field with its user-facing type, opting into archived fields', async () => { fakeSettingsScreens(); const customFieldsApi = fakeCustomFields(); diff --git a/apps/admin/src/settings/membership/custom-fields.tsx b/apps/admin/src/settings/membership/custom-fields.tsx index 0da1058d9c2..717cf98c29b 100644 --- a/apps/admin/src/settings/membership/custom-fields.tsx +++ b/apps/admin/src/settings/membership/custom-fields.tsx @@ -29,7 +29,8 @@ import { useReorderMemberCustomFields, userTypeForField, } from '@tryghost/admin-x-framework/api/member-custom-fields'; -import { useFeatureFlag, useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useCustomFieldsAvailable } from '@/shared/member-custom-fields/use-availability'; +import { useHandleError } from '@tryghost/admin-x-framework/hooks'; import { useQueryClient } from '@tryghost/admin-x-framework'; import { withErrorBoundary } from '@/settings/components/with-error-boundary'; import type { MemberCustomField } from '@tryghost/admin-x-framework/api/member-custom-fields'; @@ -185,11 +186,11 @@ const FieldList: React.FC<{ }; const CustomFields: React.FC<{ keywords: string[] }> = ({ keywords }) => { - // The endpoint is closed (404s) while the flag is off, so keep the query in - // step with the flag rather than firing it into a wall. Settings is the one - // place that manages archived fields too, so it uses the include-archived + // Nothing to fetch when the site cannot use custom fields at all, so keep the + // query in step with availability rather than firing it into a wall. Settings is + // the one place that manages archived fields too, so it uses the include-archived // variant rather than the default active-only browse. - const hasCustomFields = useFeatureFlag('membersCustomFields'); + const hasCustomFields = useCustomFieldsAvailable(); const { data } = useBrowseMemberCustomFieldsIncludingArchived({ enabled: hasCustomFields, }); diff --git a/apps/admin/src/settings/membership/membership-settings.tsx b/apps/admin/src/settings/membership/membership-settings.tsx index 573d6bc9cc2..22f4ea0e28c 100644 --- a/apps/admin/src/settings/membership/membership-settings.tsx +++ b/apps/admin/src/settings/membership/membership-settings.tsx @@ -14,6 +14,7 @@ import { usePaidMembersEnabled, } from '@tryghost/admin-x-framework/api/settings'; import { searchKeywords } from './search-keywords'; +import { useCustomFieldsAvailable } from '@/shared/member-custom-fields/use-availability'; import { useFeatureFlag } from '@tryghost/admin-x-framework/hooks'; import { useGlobalData } from '@/settings/providers/global-data-context'; @@ -23,7 +24,7 @@ const MembershipSettings: React.FC = () => { const [hasTipsAndDonations] = getSettingValues(settings, ['donations_enabled']) as [boolean]; const hasStripeEnabled = checkStripeEnabled(settings || [], config || {}); const hasAutomations = useFeatureFlag('automations'); - const hasCustomFields = useFeatureFlag('membersCustomFields'); + const hasCustomFields = useCustomFieldsAvailable(); const visibleSearchKeywords = [ searchKeywords.access, searchKeywords.tiers, 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 7feab3c415c..a15677a787e 100644 --- a/apps/admin/src/settings/membership/tiers/tier-checkout-collection.tsx +++ b/apps/admin/src/settings/membership/tiers/tier-checkout-collection.tsx @@ -30,11 +30,8 @@ import { type StripePort, } from '@tryghost/checkout'; import { JSONError, getErrorMessage } from '@tryghost/admin-x-framework/errors'; -import { - type ErrorMessages, - useFeatureFlag, - useHandleError, -} from '@tryghost/admin-x-framework/hooks'; +import { useCustomFieldsAvailable } from '@/shared/member-custom-fields/use-availability'; +import { type ErrorMessages, useHandleError } from '@tryghost/admin-x-framework/hooks'; import { Text } from '@tryghost/shade/primitives'; import { type MemberCustomField, @@ -297,7 +294,7 @@ const TierCheckoutCollection = forwardRef< const { mutateAsync: editCheckoutConfig } = useEditTierCheckoutConfig(); const handleError = useHandleError(); - const canManageFields = useFeatureFlag('membersCustomFields'); + const canManageFields = useCustomFieldsAvailable(); const { data: fieldsData } = useBrowseMemberCustomFields({ enabled: canManageFields }); const allFields = fieldsData ?? []; // What each collected value may be kept in is the server's rule, so it is read from the diff --git a/apps/admin/src/shared/member-custom-fields/use-availability.ts b/apps/admin/src/shared/member-custom-fields/use-availability.ts new file mode 100644 index 00000000000..a02aa6398fb --- /dev/null +++ b/apps/admin/src/shared/member-custom-fields/use-availability.ts @@ -0,0 +1,24 @@ +import { useFeatureFlag, useHostLimits } from '@tryghost/admin-x-framework/hooks'; + +/** + * Whether this site may use custom member fields. + * + * Two separate things can withhold them, and one place answers for both, so a screen + * cannot be left checking one and forgetting the other. The labs flag says whether this + * build of Ghost offers the feature at all; the host limit says whether this site's plan + * includes it. When the flag goes at GA only this function changes. + * + * The limit is unset on every self-hosted site and on any plan that includes the feature, + * which is every site today. + * + * A boolean because that is all any screen needs so far. The server does distinguish the + * two refusals, answering "not found" for the flag and "forbidden" for the limit, so a + * screen that offers an upgrade will want to know which applies. Nothing offers one yet, + * so the reason is not reported until something reads it. + */ +export const useCustomFieldsAvailable = (): boolean => { + const hasFlag = useFeatureFlag('membersCustomFields'); + const limit = useHostLimits()?.limitCustomFields; + + return hasFlag && limit?.disabled !== true; +}; diff --git a/ghost/core/core/server/services/limits.js b/ghost/core/core/server/services/limits.js index 05ce3c6d4f7..99451517c01 100644 --- a/ghost/core/core/server/services/limits.js +++ b/ghost/core/core/server/services/limits.js @@ -47,6 +47,22 @@ const init = () => { } }; +/** + * Route guard for a feature a host can switch off, for the routes that exist only to + * change it. Answers 403 with the host's own wording, which is a different thing to tell a + * caller than the 404 a labs flag gives: the feature exists, this plan does not include it. + */ +const requireFeature = (limitName) => + async function requireFeatureMw(req, res, next) { + try { + await limitService.errorIfWouldGoOverLimit(limitName); + next(); + } catch (err) { + next(err); + } + }; + module.exports = limitService; module.exports.init = init; +module.exports.requireFeature = requireFeature; diff --git a/ghost/core/core/server/web/api/endpoints/admin/routes.js b/ghost/core/core/server/web/api/endpoints/admin/routes.js index 1bdd6b701d0..1d5bb904951 100644 --- a/ghost/core/core/server/web/api/endpoints/admin/routes.js +++ b/ghost/core/core/server/web/api/endpoints/admin/routes.js @@ -5,6 +5,7 @@ const auth = require('../../../../services/auth'); const apiMw = require('../../middleware'); const mw = require('./middleware'); const labs = require('../../../../../shared/labs'); +const limits = require('../../../../services/limits'); const shared = require('../../../shared'); @@ -195,40 +196,38 @@ module.exports = function apiRoutes() { router.get('/members/stripe_connect', mw.authAdminApi, http(api.membersStripeConnect.auth)); - // Reading definitions is deliberately not behind the feature flag: Admin asks every site - // for them, and a site without the flag simply has none, so the answer is an empty list - // rather than a 404. Creating and changing them is flagged. Registered before /members/:id - // so the literal path is not captured as an id. - router.get('/members/metafields/:namespace', mw.authAdminApi, http(api.membersMetafields.browse)); - router.post( - '/members/metafields/:namespace', - mw.authAdminApi, - labs.enabledMiddleware('membersCustomFields'), - http(api.membersMetafields.add), - ); - router.put( - '/members/metafields/:namespace', - mw.authAdminApi, - labs.enabledMiddleware('membersCustomFields'), - http(api.membersMetafields.reorder), - ); - router.get( - '/members/metafields/:namespace/:key', - mw.authAdminApi, - http(api.membersMetafields.read), - ); - router.put( - '/members/metafields/:namespace/:key', - mw.authAdminApi, - labs.enabledMiddleware('membersCustomFields'), - http(api.membersMetafields.edit), - ); - router.delete( - '/members/metafields/:namespace/:key', - mw.authAdminApi, - labs.enabledMiddleware('membersCustomFields'), - http(api.membersMetafields.destroy), - ); + // Custom field definitions. Mounted rather than listed so every route under it is + // reached the same way, and so the guards are stated once each instead of on every + // route that needs them. + // + // Order carries the rule here, the way Express reads it: a request walks this stack + // from the top, so the reads below are answered before the guards are reached, and + // everything registered after them passes through both. A route added at the end is + // guarded by being there, which is the safer way round to forget. + // + // Mounted before /members/:id so the literal path is not captured as an id. + const metafieldsRouter = express.Router('admin api members metafields'); + router.use('/members/metafields', metafieldsRouter); + + metafieldsRouter.use(mw.authAdminApi); + + // Reading is deliberately open: Admin asks every site for its definitions to draw + // screens it renders either way, and a site that has none simply answers with an empty + // list rather than a 404. + metafieldsRouter.get('/:namespace', http(api.membersMetafields.browse)); + metafieldsRouter.get('/:namespace/:key', http(api.membersMetafields.read)); + + // Changing one needs the feature to exist in this build and the site's plan to include + // it. Two separate questions, asked once each: a 404 says the feature is not here, a 403 + // says the plan does not cover it, and only the second is something a publisher can act + // on. + metafieldsRouter.use(labs.enabledMiddleware('membersCustomFields')); + metafieldsRouter.use(limits.requireFeature('limitCustomFields')); + + metafieldsRouter.post('/:namespace', http(api.membersMetafields.add)); + metafieldsRouter.put('/:namespace', http(api.membersMetafields.reorder)); + metafieldsRouter.put('/:namespace/:key', http(api.membersMetafields.edit)); + metafieldsRouter.delete('/:namespace/:key', http(api.membersMetafields.destroy)); router.get('/members/:id', mw.authAdminApi, http(api.members.read)); router.put('/members/:id', mw.authAdminApi, http(api.members.edit)); diff --git a/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts b/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts index 3776b5faf4b..1aaf346296a 100644 --- a/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts +++ b/ghost/core/test/e2e-api/admin/member-custom-fields.test.ts @@ -5,6 +5,7 @@ const { fixtureManager, mockManager, configUtils, + hostLimits, } = require('../../utils/e2e-framework'); const models = require('../../../core/server/models'); const events = require('../../../core/server/lib/common/events'); @@ -2402,4 +2403,169 @@ describe('Member Custom Fields Admin API', function () { await agent.delete('members/metafields/custom/company/').expectStatus(404); }); }); + // Custom fields can be switched off for a site by its host, separately from the flag, + // so the feature can be sold with a plan. The limit is unset everywhere today, which is + // what the first test pins: nothing changes for a site nobody has limited. When it is + // set, changing definitions answers 403 with the host's copy, distinct from the 404 the + // flag gives, because "your plan does not include this" is a different thing to tell a + // caller than "this does not exist here". Reading stays open either way, so a site whose + // plan drops can still see and export the fields it already has. + describe('Host limit', function () { + afterEach(async function () { + await hostLimits.restoreHostLimits(); + }); + + it('leaves every route alone when the limit is unset', async function () { + const field = await createField({ name: 'Unlimited' }); + + await agent.get('members/metafields/custom/').expectStatus(200); + await agent + .put(`members/metafields/custom/${field.key}/`) + .body({ members_metafields: [{ name: 'Still unlimited' }] }) + .expectStatus(200); + // Deleting is only offered on an archived field, so archiving is the step + // that proves the edit route is open, and the delete that follows it too. + await setStatus(field.key, 'archived'); + await agent.delete(`members/metafields/custom/${field.key}/`).expectStatus(204); + }); + + it('leaves every route alone when the limit is present but not disabled', async function () { + await hostLimits.setHostLimits({ limitCustomFields: { disabled: false } }); + + const field = await createField({ name: 'Permitted' }); + await setStatus(field.key, 'archived'); + await agent.delete(`members/metafields/custom/${field.key}/`).expectStatus(204); + }); + + describe('when the host disables the feature', function () { + let existingKey: string; + + beforeEach(async function () { + // Created before the limit goes on, standing in for a site that had the feature + // and then dropped below the plan that includes it. + existingKey = (await createField({ name: 'Bought earlier' })).key; + await hostLimits.setHostLimits({ + limitCustomFields: { + disabled: true, + error: 'Custom fields are available on the Publisher plan and above.', + }, + }); + }); + + it('still lists the fields the site already has', async function () { + const { body } = await agent.get('members/metafields/custom/').expectStatus(200); + assert.equal( + body.members_metafields.some((field: { key: string }) => field.key === existingKey), + true, + ); + }); + + it('still reads a single field', async function () { + await agent.get(`members/metafields/custom/${existingKey}/`).expectStatus(200); + }); + + it('403s the create endpoint, with the host copy', async function () { + const { body } = await agent + .post('members/metafields/custom/') + .body({ members_metafields: [{ name: 'Company', type: 'short_text' }] }) + .expectStatus(403); + + // The guard is middleware, so the refusal reaches the caller as the limit service + // raised it. Endpoints that catch a host limit and re-word it put their own sentence + // in `message` and move the host's to `context`; this route has nowhere doing that, + // so the publisher reads what their host wrote and `context` stays empty. + assert.equal( + body.errors[0].message, + 'Custom fields are available on the Publisher plan and above.', + ); + assert.equal(body.errors[0].context, null); + assert.equal(body.errors[0].details.name, 'limitCustomFields'); + }); + + it('403s the reorder endpoint', async function () { + await agent + .put('members/metafields/custom/') + .body({ members_metafields: [{ key: existingKey }] }) + .expectStatus(403); + }); + + it('403s the edit endpoint', async function () { + await agent + .put(`members/metafields/custom/${existingKey}/`) + .body({ members_metafields: [{ name: 'Employer' }] }) + .expectStatus(403); + }); + + it('403s the delete endpoint', async function () { + await agent.delete(`members/metafields/custom/${existingKey}/`).expectStatus(403); + }); + + // Turning checkout collection on makes the field the collected value lands in, so + // this route creates definitions without going near the routes above. That is + // deliberate and stays allowed: the publisher is not managing custom fields here, + // they are turning on shipping, and the field is the machinery that serves it. + // + // What makes it safe is the packaging, not anything enforced along the way. Only one + // plan limit is involved at all: limitStripeConnect, which withholds Stripe, without + // which there is nothing to charge for and so no reason to have a paid tier. Admin + // then offers a checkout configuration only on a paid tier. The plan that includes + // Stripe is the plan that will include custom fields, so a site that reaches here is + // entitled to what it provisions. + // + // That chain holds by coincidence of pricing, and only its first link is enforced. + // The last one is a convention of the interface: this route accepts a free tier, and + // checks neither the tier's type nor whether Stripe is connected. Nor does the + // stripeCheckoutCollection flag stand in for any of it, being a rollout switch with + // no view on what a site pays for. + // + // So this pins a decision, not a mechanism. If checkout collection is ever sold + // apart from custom fields, this route needs a limit of its own rather than + // borrowing this one, because what it governs is what a checkout may collect. + it('still provisions the field a checkout collection needs', async function () { + const { body: tiers } = await agent + .get('tiers/?limit=1&filter=type:paid') + .expectStatus(200); + + await agent + .put(`tiers/${tiers.tiers[0].id}/checkout_config/`) + .body({ + tiers_checkout_config: [ + { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }, + ], + }) + .expectStatus(200); + + const { body } = await agent.get('members/metafields/custom/').expectStatus(200); + assert.equal( + body.members_metafields.some( + (field: { key: string }) => field.key === 'shipping_address', + ), + true, + ); + }); + + it('falls back to generic copy when the host sets no message', async function () { + await hostLimits.setHostLimits({ limitCustomFields: { disabled: true } }); + + const { body } = await agent + .post('members/metafields/custom/') + .body({ members_metafields: [{ name: 'Company', type: 'short_text' }] }) + .expectStatus(403); + + // The wording the limit service builds when a host supplies none, from the limit's + // own name. + assert.equal( + body.errors[0].message, + 'Your plan does not support custom fields. Please upgrade to enable custom fields.', + ); + }); + }); + }); }); diff --git a/packages/limit-service/lib/config.js b/packages/limit-service/lib/config.js index 979b01cb7f8..4a5aebb5870 100644 --- a/packages/limit-service/lib/config.js +++ b/packages/limit-service/lib/config.js @@ -59,5 +59,6 @@ module.exports = { limitStripeConnect: {}, limitAnalytics: {}, limitSocialWeb: {}, + limitCustomFields: {}, publicSiteAccess: {} }; From c61a5a7bce523f7416568aca6783b1a3f3504764 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Wed, 9 Sep 2026 15:08:02 +0100 Subject: [PATCH 12/17] Fixed gift email analytics site filtering (#30624) ref https://linear.app/ghost/issue/BER-3951/ Gift delivery analytics queried all events on shared Mailgun domains because the filter only included `gift-delivery`. Gift delivery sends now include `bulkEmail:mailgun:tag`, and analytics require both tags. Missing or empty site tags remain supported. Automation analytics were already fixed in [#30430](https://github.com/TryGhost/Ghost/pull/30430); this change completes site scoping for gift delivery emails. --- .../server/services/email-analytics/index.ts | 4 +- .../services/gifts/gift-email-service.ts | 15 +- .../core/core/server/services/gifts/index.ts | 1 + .../services/email-analytics/index.test.ts | 16 +- .../services/gifts/gift-email-service.test.js | 144 ++++++++++-------- 5 files changed, 112 insertions(+), 68 deletions(-) diff --git a/ghost/core/core/server/services/email-analytics/index.ts b/ghost/core/core/server/services/email-analytics/index.ts index a7c4cd8173e..08c951d25f8 100644 --- a/ghost/core/core/server/services/email-analytics/index.ts +++ b/ghost/core/core/server/services/email-analytics/index.ts @@ -97,10 +97,12 @@ export const init = ({ const newsletterMailgunTags = ['bulk-email']; const automationMailgunTags = [AUTOMATION_EMAIL_TAG]; + const giftMailgunTags = [GIFT_DELIVERY_EMAIL_TAG]; const mailgunTagFromConfig = config.get('bulkEmail:mailgun:tag'); if (mailgunTagFromConfig) { newsletterMailgunTags.push(mailgunTagFromConfig); automationMailgunTags.push(mailgunTagFromConfig); + giftMailgunTags.push(mailgunTagFromConfig); } prometheusClient?.registerCounter({ @@ -171,7 +173,7 @@ export const init = ({ domainEvents, event: StartGiftEmailAnalyticsJobEvent, queries, - mailgunTags: [GIFT_DELIVERY_EMAIL_TAG], + mailgunTags: giftMailgunTags, jobNames: { latestNonOpened: 'email-analytics-gifts-latest-others', missing: 'email-analytics-gifts-missing', diff --git a/ghost/core/core/server/services/gifts/gift-email-service.ts b/ghost/core/core/server/services/gifts/gift-email-service.ts index c8822355769..7d726cedd04 100644 --- a/ghost/core/core/server/services/gifts/gift-email-service.ts +++ b/ghost/core/core/server/services/gifts/gift-email-service.ts @@ -5,6 +5,7 @@ import { Color } from '@tryghost/color-utils'; import errors from '@tryghost/errors'; import { getMailgunMessageId } from '../lib/mailgun-message-id'; import { GIFT_DELIVERY_EMAIL_TAG } from './constants'; +import type { ConfigInstance } from '../../../shared/config/loader'; import { formatGiftDate } from './gift-date'; const DEFAULT_ACCENT_COLOR = '#15212A'; @@ -99,6 +100,7 @@ type GiftSentConfirmationData = GiftDeliveryNoticeData; export class GiftEmailService { private readonly transactionalMailer: TransactionalMailer; + private readonly config: Pick; private readonly bulkMailer: BulkMailer; private readonly settingsCache: SettingsCache; private readonly urlUtils: UrlUtils; @@ -109,6 +111,7 @@ export class GiftEmailService { private readonly t: Translate; constructor({ + config, transactionalMailer, bulkMailer, settingsCache, @@ -118,6 +121,7 @@ export class GiftEmailService { blogIcon, t, }: { + config: Pick; transactionalMailer: TransactionalMailer; bulkMailer: BulkMailer; settingsCache: SettingsCache; @@ -127,6 +131,7 @@ export class GiftEmailService { blogIcon: BlogIcon; t: Translate; }) { + this.config = config; this.transactionalMailer = transactionalMailer; this.bulkMailer = bulkMailer; this.settingsCache = settingsCache; @@ -363,6 +368,12 @@ export class GiftEmailService { interpolation: { escapeValue: false }, }); + const tags = [GIFT_DELIVERY_EMAIL_TAG]; + const mailgunTagFromConfig = this.config.get('bulkEmail:mailgun:tag'); + if (typeof mailgunTagFromConfig === 'string' && mailgunTagFromConfig.length > 0) { + tags.push(mailgunTagFromConfig); + } + if (!this.bulkMailer.isConfigured()) { await this.transactionalMailer.send({ to: recipientEmail, @@ -372,7 +383,7 @@ export class GiftEmailService { from: this.getFromAddress(), replyTo: this.getReplyToAddress(), forceTextContent: true, - tags: [GIFT_DELIVERY_EMAIL_TAG], + tags, disableTracking: true, }); @@ -386,7 +397,7 @@ export class GiftEmailService { plaintext: text, from: this.getFromAddress(), replyTo: this.getReplyToAddress(), - tags: [GIFT_DELIVERY_EMAIL_TAG], + tags, disable_tracking: true, }, { [recipientEmail]: {} }, diff --git a/ghost/core/core/server/services/gifts/index.ts b/ghost/core/core/server/services/gifts/index.ts index 3e4aebbf02b..bc22af340dc 100644 --- a/ghost/core/core/server/services/gifts/index.ts +++ b/ghost/core/core/server/services/gifts/index.ts @@ -75,6 +75,7 @@ export function init(options: GiftServiceInitOptions): void { }); const giftEmailService = new GiftEmailService({ + config, transactionalMailer: new GhostMailer(), bulkMailer: new MailgunClient({ config, settings: settingsCache }), settingsCache, diff --git a/ghost/core/test/unit/server/services/email-analytics/index.test.ts b/ghost/core/test/unit/server/services/email-analytics/index.test.ts index dea9e0f42f3..252d047433c 100644 --- a/ghost/core/test/unit/server/services/email-analytics/index.test.ts +++ b/ghost/core/test/unit/server/services/email-analytics/index.test.ts @@ -139,7 +139,7 @@ describe('email analytics service', function () { event: { name: 'StartGiftEmailAnalyticsJobEvent', }, - mailgunTags: [GIFT_DELIVERY_EMAIL_TAG], + mailgunTags: [GIFT_DELIVERY_EMAIL_TAG, 'custom-mailgun-tag'], jobNames: { latestNonOpened: 'email-analytics-gifts-latest-others', missing: 'email-analytics-gifts-missing', @@ -173,6 +173,20 @@ describe('email analytics service', function () { ); }); + it.each([undefined, ''])( + 'does not add a gift analytics site tag when configured as %s', + function (siteTag) { + config.get.withArgs('bulkEmail:mailgun:tag').returns(siteTag); + + init(dependencies); + + sinon.assert.calledOnceWithExactly( + giftsInit, + sinon.match({ mailgunTags: [GIFT_DELIVERY_EMAIL_TAG] }), + ); + }, + ); + it('registers Prometheus metrics for member stat aggregation', function () { const registerCounter = sinon.stub(); diff --git a/ghost/core/test/unit/server/services/gifts/gift-email-service.test.js b/ghost/core/test/unit/server/services/gifts/gift-email-service.test.js index ce3b22e8488..f31c0d9721c 100644 --- a/ghost/core/test/unit/server/services/gifts/gift-email-service.test.js +++ b/ghost/core/test/unit/server/services/gifts/gift-email-service.test.js @@ -8,6 +8,7 @@ describe('GiftEmailService', function () { let transactionalMailer; let bulkMailer; let service; + let config; const settingsCache = { get: (key) => { @@ -70,12 +71,14 @@ describe('GiftEmailService', function () { }; beforeEach(function () { + config = { get: sinon.stub() }; transactionalMailer = { send: sinon.stub().resolves() }; bulkMailer = { isConfigured: sinon.stub().returns(true), send: sinon.stub().resolves({ id: '' }), }; service = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache, @@ -226,6 +229,7 @@ describe('GiftEmailService', function () { }, }; const localizedService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: localizedSettingsCache, @@ -265,6 +269,7 @@ describe('GiftEmailService', function () { }; const noTitleService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: noTitleSettingsCache, @@ -296,6 +301,7 @@ describe('GiftEmailService', function () { }; const hostileService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: hostileSettingsCache, @@ -333,74 +339,79 @@ describe('GiftEmailService', function () { }); describe('sendGiftDelivery', function () { - it('sends the prototype delivery content through bulk Mailgun without open or click tracking', async function () { - const result = await service.sendGiftDelivery({ - recipientEmail: 'recipient@example.com', - recipientName: 'Recipient', - buyerEmail: 'buyer@example.com', - buyerName: 'Buyer', - personalMessage: 'Enjoy this gift', - token: 'abc-123', - tierName: 'Gold', - benefits: ['All stories'], - cadence: 'year', - duration: 1, - expiresAt: new Date('2027-04-07'), - }); + it.each([undefined, '', 'blog-123'])( + 'sends gift delivery with site tag %s and without open or click tracking', + async function (siteTag) { + config.get.withArgs('bulkEmail:mailgun:tag').returns(siteTag); + const result = await service.sendGiftDelivery({ + recipientEmail: 'recipient@example.com', + recipientName: 'Recipient', + buyerEmail: 'buyer@example.com', + buyerName: 'Buyer', + personalMessage: 'Enjoy this gift', + token: 'abc-123', + tierName: 'Gold', + benefits: ['All stories'], + cadence: 'year', + duration: 1, + expiresAt: new Date('2027-04-07'), + }); - assert.deepEqual(result, { providerMessageId: 'provider-123' }); - sinon.assert.notCalled(transactionalMailer.send); - const message = bulkMailer.send.firstCall.firstArg; - sinon.assert.match(message, { - subject: 'Buyer sent you a gift', - replyTo: 'support@example.com', - tags: ['gift-delivery'], - disable_tracking: true, - }); - assert.deepEqual(bulkMailer.send.firstCall.args[1], { 'recipient@example.com': {} }); - for (const field of ['html', 'plaintext']) { - sinon.assert.match(message[field], sinon.match('Recipient')); - sinon.assert.match(message[field], sinon.match('Enjoy this gift')); - sinon.assert.match(message[field], sinon.match('All stories')); - sinon.assert.match(message[field], sinon.match('https://example.com/gift/abc-123')); - } - sinon.assert.match( - message.plaintext, - sinon.match('Buyer has gifted you a 1-year Gold membership to Test Site'), - ); - sinon.assert.match( - message.plaintext, - sinon.match('Redeem your gift:\nhttps://example.com/gift/abc-123'), - ); - sinon.assert.match( - message.plaintext, - sinon.match( - 'This message was sent from example.com to recipient@example.com on behalf of Buyer (buyer@example.com).', - ), - ); - sinon.assert.match(message.html, sinon.match('Redeem your gift')); - sinon.assert.match( - message.html, - sinon.match((value) => !value.includes('Redeem your gift:')), - ); - sinon.assert.match( - message.html, - sinon.match( - 'Buyer has gifted you a 1-year Gold membership to Test Site', - ), - ); - sinon.assert.match( - message.html, - sinon.match( - 'This message was sent from example.com to recipient@example.com on behalf of Buyer (buyer@example.com).', - ), - ); - sinon.assert.match(message.html, sinon.match('background:#fff3ed')); - sinon.assert.match(message.html, sinon.match('color:#bd460c')); - }); + assert.deepEqual(result, { providerMessageId: 'provider-123' }); + sinon.assert.notCalled(transactionalMailer.send); + const message = bulkMailer.send.firstCall.firstArg; + sinon.assert.match(message, { + subject: 'Buyer sent you a gift', + replyTo: 'support@example.com', + tags: ['gift-delivery', ...(siteTag ? [siteTag] : [])], + disable_tracking: true, + }); + assert.deepEqual(bulkMailer.send.firstCall.args[1], { 'recipient@example.com': {} }); + for (const field of ['html', 'plaintext']) { + sinon.assert.match(message[field], sinon.match('Recipient')); + sinon.assert.match(message[field], sinon.match('Enjoy this gift')); + sinon.assert.match(message[field], sinon.match('All stories')); + sinon.assert.match(message[field], sinon.match('https://example.com/gift/abc-123')); + } + sinon.assert.match( + message.plaintext, + sinon.match('Buyer has gifted you a 1-year Gold membership to Test Site'), + ); + sinon.assert.match( + message.plaintext, + sinon.match('Redeem your gift:\nhttps://example.com/gift/abc-123'), + ); + sinon.assert.match( + message.plaintext, + sinon.match( + 'This message was sent from example.com to recipient@example.com on behalf of Buyer (buyer@example.com).', + ), + ); + sinon.assert.match(message.html, sinon.match('Redeem your gift')); + sinon.assert.match( + message.html, + sinon.match((value) => !value.includes('Redeem your gift:')), + ); + sinon.assert.match( + message.html, + sinon.match( + 'Buyer has gifted you a 1-year Gold membership to Test Site', + ), + ); + sinon.assert.match( + message.html, + sinon.match( + 'This message was sent from example.com to recipient@example.com on behalf of Buyer (buyer@example.com).', + ), + ); + sinon.assert.match(message.html, sinon.match('background:#fff3ed')); + sinon.assert.match(message.html, sinon.match('color:#bd460c')); + }, + ); it('shows the publication title when the publication has no icon', async function () { const iconlessService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache, @@ -449,6 +460,7 @@ describe('GiftEmailService', function () { }, }; const siteDateService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: siteSettingsCache, @@ -491,6 +503,7 @@ describe('GiftEmailService', function () { }, }; const invalidLocaleService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: invalidLocaleSettingsCache, @@ -525,6 +538,7 @@ describe('GiftEmailService', function () { get: (key) => (key === 'accent_color' ? 'invalid' : settingsCache.get(key)), }; const invalidColorService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: invalidColorSettingsCache, @@ -652,6 +666,7 @@ describe('GiftEmailService', function () { }, }; const literalService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: literalSettingsCache, @@ -819,6 +834,7 @@ describe('GiftEmailService', function () { }, }; const localizedService = new GiftEmailService({ + config, transactionalMailer, bulkMailer, settingsCache: localizedSettingsCache, From 478a4bf8ef435e6818e63c4ed836f282287fd9ed Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Wed, 9 Sep 2026 09:19:24 -0500 Subject: [PATCH 13/17] Added the Keyboard shortcuts pane to the React editor's settings sidebar (#30610) no ref --- ...ngs-keyboard-shortcuts.acceptance.test.tsx | 210 ++++++++++++++++++ apps/admin/src/editor/editor.screen.ts | 7 + apps/admin/src/editor/settings/README.md | 11 + .../settings/keyboard-shortcuts-section.tsx | 104 +++++++++ .../settings/keyboard-shortcuts.test.ts | 86 +++++++ .../src/editor/settings/keyboard-shortcuts.ts | 123 ++++++++++ .../editor/settings/post-settings-sidebar.tsx | 2 + .../layout/app-sidebar/app-sidebar-header.tsx | 3 +- apps/admin/src/utils/is-mac-platform.test.ts | 19 ++ apps/admin/src/utils/is-mac-platform.ts | 4 + .../testing/test-data/src/selectors/editor.ts | 1 + 11 files changed, 569 insertions(+), 1 deletion(-) create mode 100644 apps/admin/src/editor/editor-settings-keyboard-shortcuts.acceptance.test.tsx create mode 100644 apps/admin/src/editor/settings/keyboard-shortcuts-section.tsx create mode 100644 apps/admin/src/editor/settings/keyboard-shortcuts.test.ts create mode 100644 apps/admin/src/editor/settings/keyboard-shortcuts.ts create mode 100644 apps/admin/src/utils/is-mac-platform.test.ts create mode 100644 apps/admin/src/utils/is-mac-platform.ts diff --git a/apps/admin/src/editor/editor-settings-keyboard-shortcuts.acceptance.test.tsx b/apps/admin/src/editor/editor-settings-keyboard-shortcuts.acceptance.test.tsx new file mode 100644 index 00000000000..7549e129549 --- /dev/null +++ b/apps/admin/src/editor/editor-settings-keyboard-shortcuts.acceptance.test.tsx @@ -0,0 +1,210 @@ +import { describe, expect, it, onTestFinished } from 'vitest'; +import { page, userEvent } from 'vitest/browser'; +import { buildLexicalParagraph } from '@tryghost/test-data'; + +import { + currentUserResponse, + fakeAdminEndpoint, + fakeMembers, + fakeNewsletters, + fakePosts, + fakeSnippets, + fakeTiers, + post, + renderAdminApp, + staffRole, + type StaffRoleName, +} from '@test-utils/acceptance'; +import { editorScreen } from '@/editor/editor.screen'; + +const POST_ID = 'abc123'; +const CURRENT_USER_ID = '1'; +const FLAG_ON = { labs: { editorReact: true } }; +const ROUTE = new RegExp(`^/posts/${POST_ID}/\\?`); +const BACK_LABEL = 'Close keyboard shortcuts panel'; +const ROW_LABEL = 'Keyboard shortcuts'; +const MAC_AGENT = + 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36'; +const WINDOWS_AGENT = + 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36'; + +// A full-app render outlasts the default timeout. +const SLOW = 20_000; + +/** The pane reads the platform as it renders, so the agent has to be in place first. */ +function onPlatform(userAgent: string) { + Object.defineProperty(navigator, 'userAgent', { configurable: true, get: () => userAgent }); + onTestFinished(() => { + Reflect.deleteProperty(navigator, 'userAgent'); + }); +} + +function asRole(name: StaffRoleName) { + const me = currentUserResponse(); + me.users[0].roles = [staffRole({ name })]; + return { ...FLAG_ON, boot: { browseMe: { response: me } } }; +} + +function fakeEditablePost(overrides: Partial> = {}) { + fakeSnippets([]); + fakePosts([]); + // The header's publish inputs and preview read these beyond the boot table. + fakeMembers([]); + fakeNewsletters([]); + fakeTiers([]); + fakeAdminEndpoint('GET', /^\/slugs\/post\//, ({ url }) => ({ + slugs: [{ slug: decodeURIComponent(url.split('/slugs/post/')[1].split('/')[0]) }], + })); + + const current = post({ + id: POST_ID, + title: 'Hello from React', + slug: 'hello-from-react', + status: 'draft', + lexical: buildLexicalParagraph('Hello from React'), + tags: [], + ...overrides, + }); + + fakeAdminEndpoint('GET', ROUTE, () => ({ posts: [current] })); + fakeAdminEndpoint('PUT', ROUTE, () => ({ posts: [current] })); +} + +async function openShortcuts() { + await editorScreen.settingsToggle().click(); + await expect.element(editorScreen.settingsSidebar()).toBeVisible(); + await editorScreen.settingsSubviewRow(ROW_LABEL).click(); + await expect.element(editorScreen.settingsSubviewPane()).toBeVisible(); +} + +/** + * The sidebar's Keyboard shortcuts pane: the reference list of every chord and + * slash command the editor answers to, in the writer's own platform glyphs. + */ +describe('Post settings keyboard shortcuts', () => { + it( + 'opens the pane over the section list and comes back from it', + async () => { + fakeEditablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openShortcuts(); + + // The pane replaces the list it was opened from. + await expect(editorScreen.settingsExcerpt()).toHaveCount(0); + await expect.element(editorScreen.settingsSidebar()).toHaveAttribute('aria-label', ROW_LABEL); + await expect.element(page.getByRole('heading', { level: 2, name: ROW_LABEL })).toBeVisible(); + + await editorScreen.settingsSubviewBack(BACK_LABEL).click(); + + await expect(editorScreen.settingsSubviewPane()).toHaveCount(0); + await expect.element(editorScreen.settingsExcerpt()).toBeVisible(); + await expect.element(editorScreen.settingsSubviewRow(ROW_LABEL)).toBeVisible(); + }, + SLOW, + ); + + it( + 'lists every shortcut under the group it belongs to, without widening the panel', + async () => { + fakeEditablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openShortcuts(); + + const pane = editorScreen.settingsSubviewPane(); + await expect.element(pane).toHaveTextContent('Formatting'); + await expect.element(pane).toHaveTextContent('Editing'); + await expect.element(pane).toHaveTextContent('Application'); + await expect.element(pane).toHaveTextContent('Inserting'); + + expect(editorScreen.settingsShortcutRows()).toHaveLength(50); + expect(editorScreen.settingsSidebar().element().getBoundingClientRect().width).toBe(350); + }, + SLOW, + ); + + it( + 'shows a Mac writer the Mac glyphs', + async () => { + onPlatform(MAC_AGENT); + fakeEditablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openShortcuts(); + + const rows = editorScreen.settingsShortcutRows(); + expect(rows).toContain('Bold⌘B'); + expect(rows).toContain('Strike through⌃⌥U'); + expect(rows).toContain('Inline code⌃⇧K'); + expect(rows).toContain('Toggle card edit mode⌘↩'); + expect(rows).toContain('Publish⌘⇧P'); + expect(rows).toContain('Image/image'); + expect(rows).toContain('Divider---or/hr'); + }, + SLOW, + ); + + it( + 'names the modifier a glyph stands for when the writer hovers it', + async () => { + onPlatform(MAC_AGENT); + fakeEditablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openShortcuts(); + + await userEvent.hover(page.getByRole('img', { name: 'Command', exact: true }).first()); + + // The tooltip opens after Radix's hover delay. + await expect.element(page.getByText('Command'), { timeout: SLOW }).toBeVisible(); + }, + SLOW, + ); + + it( + 'shows everyone else the key names instead', + async () => { + onPlatform(WINDOWS_AGENT); + fakeEditablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openShortcuts(); + + const rows = editorScreen.settingsShortcutRows(); + expect(rows).toContain('BoldCtrlB'); + expect(rows).toContain('Strike throughCtrlAltU'); + expect(rows).toContain('Inline codeCtrlShiftK'); + expect(rows).toContain('Toggle card edit modeCtrlEnter'); + expect(rows).toContain('PublishCtrlShiftP'); + // A slash command is the same text whatever the writer is typing it on. + expect(rows).toContain('Image/image'); + }, + SLOW, + ); + + it( + 'closes the pane on Escape and returns focus to the row', + async () => { + fakeEditablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openShortcuts(); + + await userEvent.keyboard('{Escape}'); + + await expect(editorScreen.settingsSubviewPane()).toHaveCount(0); + await expect.element(editorScreen.settingsSubviewRow(ROW_LABEL)).toHaveFocus(); + }, + SLOW, + ); + + it( + 'gives a contributor the same reference list', + async () => { + onPlatform(MAC_AGENT); + // A contributor may only open a draft they authored. + fakeEditablePost({ authors: [{ id: CURRENT_USER_ID }] }); + await renderAdminApp(`/editor/post/${POST_ID}`, asRole('Contributor')); + await openShortcuts(); + + expect(editorScreen.settingsShortcutRows()).toHaveLength(50); + expect(editorScreen.settingsShortcutRows()).toContain('Bold⌘B'); + }, + SLOW, + ); +}); diff --git a/apps/admin/src/editor/editor.screen.ts b/apps/admin/src/editor/editor.screen.ts index 3647609fb50..86f9815a57f 100644 --- a/apps/admin/src/editor/editor.screen.ts +++ b/apps/admin/src/editor/editor.screen.ts @@ -64,6 +64,7 @@ import { settingsMetaDescriptionInput, settingsMetaTitleInput, settingsSerpPreview, + settingsShortcutRow, restoreRevisionButton, settingsPostHistoryButton, settingsShowTitleToggle, @@ -218,6 +219,12 @@ export const editorScreen = { /** CodeMirror exposes its content as a textbox named by the editor's label. */ settingsCodeInjection: (label: string) => page.getByRole('textbox', { name: new RegExp(`^${label}`) }), + /** Each keyboard-shortcut row as its label followed by the keys shown against it. */ + settingsShortcutRows: (): string[] => + page + .getByTestId(settingsShortcutRow) + .elements() + .map((row) => row.textContent ?? ''), settingsPostHistory: () => page.getByTestId(settingsPostHistoryButton), postHistoryModal: () => page.getByTestId(postHistoryModal), diff --git a/apps/admin/src/editor/settings/README.md b/apps/admin/src/editor/settings/README.md index 521c191f54b..c8aa976482d 100644 --- a/apps/admin/src/editor/settings/README.md +++ b/apps/admin/src/editor/settings/README.md @@ -412,6 +412,17 @@ Tab, so the next Tab moves on to the footer editor and out of the pane rather than indenting. The back button, or Escape from anywhere else in the pane, still closes the pane. +## Keyboard shortcuts + +A pane every role that can open the panel can open, and the one thing in the +sidebar that edits nothing: the chords and slash commands the editor answers to, +grouped as Formatting, Editing, Application and Inserting, with the keys shown +against each. The modifiers are drawn as the writer's own platform draws them — +the Mac glyphs for a Mac writer, the key names for everyone else — read from the +user agent as the pane renders. Hovering a glyph names the key it stands for; +a key already shown as its name carries no tooltip. A slash command reads the +same wherever it is typed. + ## Open and closed The toggle sits in the editor header, and the panel starts closed on every diff --git a/apps/admin/src/editor/settings/keyboard-shortcuts-section.tsx b/apps/admin/src/editor/settings/keyboard-shortcuts-section.tsx new file mode 100644 index 00000000000..2c712e9293a --- /dev/null +++ b/apps/admin/src/editor/settings/keyboard-shortcuts-section.tsx @@ -0,0 +1,104 @@ +import { useMemo } from 'react'; +import { Kbd, KbdGroup, Tooltip, TooltipContent, TooltipTrigger } from '@tryghost/shade/components'; +import { Inline, Stack, Text } from '@tryghost/shade/primitives'; +import { LucideIcon, cn } from '@tryghost/shade/utils'; +import { settingsShortcutRow } from '@tryghost/test-data/selectors/editor'; +import { isMacPlatform } from '@/utils/is-mac-platform'; +import { + keyboardShortcutGroups, + type Shortcut, + type ShortcutKey, + type ShortcutStyle, +} from './keyboard-shortcuts'; +import { SettingsSubview } from './settings-subview'; + +const LABEL_CLASSES: Record = { + bold: 'font-semibold', + italic: 'italic', + underline: 'underline', + strikethrough: 'line-through', + // A sample of the highlight the editor applies, not themed chrome, so the + // pair stays the same in either theme. + highlight: 'bg-yellow-200 text-black', + link: 'text-ghostaccent', + code: 'font-mono', +}; + +function KeyCap({ token }: { token: ShortcutKey }) { + const cap = ( + + {token.text} + + ); + + if (!token.tooltip) { + return cap; + } + + // Kbd forwards no ref, so the tooltip anchors on a wrapper instead. + return ( + + + {cap} + + {token.tooltip} + + ); +} + +function ShortcutRow({ shortcut }: { shortcut: Shortcut }) { + return ( + +
+ + {shortcut.label} + +
+
+ + {shortcut.keys.map((token) => ( + + ))} + +
+ + ); +} + +/** The editor's keyboard shortcuts, as a reference the writer reads rather than edits. */ +export function KeyboardShortcutsSection() { + const groups = useMemo(() => keyboardShortcutGroups(isMacPlatform()), []); + + return ( + } + id="keyboard-shortcuts" + label="Keyboard shortcuts" + title="Keyboard shortcuts" + > + {groups.map((group) => ( + + + {group.title} + +
+ {group.shortcuts.map((shortcut) => ( + + ))} +
+ + ))} + + ); +} diff --git a/apps/admin/src/editor/settings/keyboard-shortcuts.test.ts b/apps/admin/src/editor/settings/keyboard-shortcuts.test.ts new file mode 100644 index 00000000000..22a5d1d7581 --- /dev/null +++ b/apps/admin/src/editor/settings/keyboard-shortcuts.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from 'vitest'; +import { keyboardShortcutGroups, type Shortcut, type ShortcutGroup } from './keyboard-shortcuts'; + +function find(groups: ShortcutGroup[], label: string): Shortcut { + const shortcut = groups.flatMap((group) => group.shortcuts).find((row) => row.label === label); + if (!shortcut) { + throw new Error(`No shortcut labelled ${label}`); + } + return shortcut; +} + +function keys(groups: ShortcutGroup[], label: string): string[] { + return find(groups, label).keys.map((token) => token.text); +} + +describe('keyboardShortcutGroups', () => { + const mac = keyboardShortcutGroups(true); + const windows = keyboardShortcutGroups(false); + + it('lists every group in order', () => { + expect(mac.map((group) => group.title)).toEqual([ + 'Formatting', + 'Editing', + 'Application', + 'Inserting', + ]); + expect(mac.map((group) => group.shortcuts.length)).toEqual([12, 7, 3, 28]); + }); + + it('shows Mac writers the Mac glyphs', () => { + expect(keys(mac, 'Bold')).toEqual(['⌘', 'B']); + expect(keys(mac, 'Strike through')).toEqual(['⌃', '⌥', 'U']); + expect(keys(mac, 'Highlight')).toEqual(['⌘', '⌥', 'H']); + expect(keys(mac, 'Inline code')).toEqual(['⌃', '⇧', 'K']); + expect(keys(mac, 'Toggle card edit mode')).toEqual(['⌘', '↩']); + expect(keys(mac, 'Publish')).toEqual(['⌘', '⇧', 'P']); + }); + + it('shows everyone else the key names', () => { + expect(keys(windows, 'Bold')).toEqual(['Ctrl', 'B']); + expect(keys(windows, 'Strike through')).toEqual(['Ctrl', 'Alt', 'U']); + expect(keys(windows, 'Highlight')).toEqual(['Ctrl', 'Alt', 'H']); + expect(keys(windows, 'Inline code')).toEqual(['Ctrl', 'Shift', 'K']); + expect(keys(windows, 'Toggle card edit mode')).toEqual(['Ctrl', 'Enter']); + expect(keys(windows, 'Publish')).toEqual(['Ctrl', 'Shift', 'P']); + expect(keys(windows, 'Line break')).toEqual(['Shift', 'Enter']); + expect(keys(windows, 'Code block')).toEqual(['```', 'Enter']); + }); + + it('names the modifier a glyph stands for, and only where it is a glyph', () => { + const macCommand = find(mac, 'Bold').keys[0]; + expect(macCommand).toEqual({ text: '⌘', tooltip: 'Command' }); + expect(find(mac, 'Strike through').keys[0].tooltip).toBe('Control'); + expect(find(mac, 'Strike through').keys[1].tooltip).toBe('Option'); + expect(find(mac, 'Inline code').keys[1].tooltip).toBe('Shift'); + expect(find(mac, 'Toggle card edit mode').keys[1].tooltip).toBe('Return'); + + expect(find(windows, 'Bold').keys[0]).toEqual({ text: 'Ctrl', mono: true }); + }); + + it('carries the slash commands unchanged on either platform', () => { + expect(keys(mac, 'Image')).toEqual(['/image']); + expect(keys(windows, 'Image')).toEqual(['/image']); + expect(keys(mac, 'YouTube')).toEqual(['/youtube [url]']); + expect(keys(mac, 'Code block')).toEqual(['```', '↩']); + }); + + it('joins the divider alternatives with a word that is not a key', () => { + expect(find(mac, 'Divider').keys).toEqual([ + { text: '---', mono: true }, + { text: 'or', mono: true, plain: true }, + { text: '/hr', mono: true }, + ]); + }); + + it('shows the formatting labels as the formatting they produce', () => { + expect(find(mac, 'Bold').style).toBe('bold'); + expect(find(mac, 'Emphasize').style).toBe('italic'); + expect(find(mac, 'Underline').style).toBe('underline'); + expect(find(mac, 'Strike through').style).toBe('strikethrough'); + expect(find(mac, 'Highlight').style).toBe('highlight'); + expect(find(mac, 'Link').style).toBe('link'); + expect(find(mac, 'Inline code').style).toBe('code'); + expect(find(mac, 'List').style).toBeUndefined(); + }); +}); diff --git a/apps/admin/src/editor/settings/keyboard-shortcuts.ts b/apps/admin/src/editor/settings/keyboard-shortcuts.ts new file mode 100644 index 00000000000..75ba939ae2c --- /dev/null +++ b/apps/admin/src/editor/settings/keyboard-shortcuts.ts @@ -0,0 +1,123 @@ +/** One token in a shortcut: a modifier glyph, a key cap, or text the writer types. */ +export interface ShortcutKey { + text: string; + /** What a glyph stands for, where the glyph alone does not say. */ + tooltip?: string; + /** Typed text or a letter key rather than a symbol. */ + mono?: boolean; + /** A word joining two alternatives rather than a key of its own. */ + plain?: boolean; +} + +/** The formatting a shortcut produces, which its label is shown in. */ +export type ShortcutStyle = + | 'bold' + | 'italic' + | 'underline' + | 'strikethrough' + | 'highlight' + | 'link' + | 'code'; + +export interface Shortcut { + label: string; + style?: ShortcutStyle; + keys: ShortcutKey[]; +} + +export interface ShortcutGroup { + title: string; + shortcuts: Shortcut[]; +} + +function typed(text: string): ShortcutKey { + return { text, mono: true }; +} + +/** Every shortcut the editor offers, in the order they are shown. */ +export function keyboardShortcutGroups(isMac: boolean): ShortcutGroup[] { + const cmd: ShortcutKey = isMac ? { text: '⌘', tooltip: 'Command' } : { text: 'Ctrl', mono: true }; + const ctrl: ShortcutKey = isMac + ? { text: '⌃', tooltip: 'Control' } + : { text: 'Ctrl', mono: true }; + const alt: ShortcutKey = isMac ? { text: '⌥', tooltip: 'Option' } : { text: 'Alt', mono: true }; + + const shift: ShortcutKey = isMac ? { text: '⇧', tooltip: 'Shift' } : typed('Shift'); + const enter: ShortcutKey = isMac ? { text: '↩', tooltip: 'Return' } : typed('Enter'); + + return [ + { + title: 'Formatting', + shortcuts: [ + { label: 'Bold', style: 'bold', keys: [cmd, typed('B')] }, + { label: 'Emphasize', style: 'italic', keys: [cmd, typed('I')] }, + { label: 'Underline', style: 'underline', keys: [cmd, typed('U')] }, + { label: 'Strike through', style: 'strikethrough', keys: [ctrl, alt, typed('U')] }, + { label: 'Highlight', style: 'highlight', keys: [cmd, alt, typed('H')] }, + { label: 'Link', style: 'link', keys: [cmd, typed('K')] }, + { label: 'Inline code', style: 'code', keys: [ctrl, shift, typed('K')] }, + { label: 'List', keys: [ctrl, typed('L')] }, + { label: 'Ordered list', keys: [ctrl, alt, typed('L')] }, + { label: 'Quote', keys: [ctrl, typed('Q')] }, + { label: 'H2', keys: [ctrl, alt, typed('2')] }, + { label: 'H3', keys: [ctrl, alt, typed('3')] }, + ], + }, + { + title: 'Editing', + shortcuts: [ + { label: 'Toggle card edit mode', keys: [cmd, enter] }, + { label: 'Paste without formatting', keys: [cmd, shift, typed('V')] }, + { label: 'Indent', keys: [typed('tab')] }, + { label: 'Unindent', keys: [shift, typed('tab')] }, + { label: 'Line break', keys: [shift, enter] }, + { label: 'Undo', keys: [cmd, typed('Z')] }, + { label: 'Redo', keys: [cmd, shift, typed('Z')] }, + ], + }, + { + title: 'Application', + shortcuts: [ + { label: 'Save', keys: [cmd, typed('S')] }, + { label: 'Preview', keys: [cmd, typed('P')] }, + { label: 'Publish', keys: [cmd, shift, typed('P')] }, + ], + }, + { + title: 'Inserting', + shortcuts: [ + { label: 'Code block', keys: [typed('```'), enter] }, + { label: 'Language code block', keys: [typed('```html'), enter] }, + { label: 'Emoji', keys: [typed(':emoji_name:')] }, + { label: 'Image', keys: [typed('/image')] }, + { label: 'Markdown', keys: [typed('/md')] }, + { label: 'HTML', keys: [typed('/html')] }, + { label: 'Gallery', keys: [typed('/gallery')] }, + { + label: 'Divider', + keys: [typed('---'), { text: 'or', mono: true, plain: true }, typed('/hr')], + }, + { label: 'Bookmark', keys: [typed('/bookmark [url]')] }, + { label: 'Public preview', keys: [typed('/paywall')] }, + { label: 'Button', keys: [typed('/button')] }, + { label: 'Callout', keys: [typed('/callout')] }, + { label: 'Toggle', keys: [typed('/toggle')] }, + { label: 'Video', keys: [typed('/video')] }, + { label: 'Audio', keys: [typed('/audio')] }, + { label: 'File', keys: [typed('/file')] }, + { label: 'Product', keys: [typed('/product')] }, + { label: 'Header', keys: [typed('/header')] }, + { label: 'GIF', keys: [typed('/gif')] }, + { label: 'Signup', keys: [typed('/signup')] }, + { label: 'YouTube', keys: [typed('/youtube [url]')] }, + { label: 'X (Twitter)', keys: [typed('/twitter [url]')] }, + { label: 'Unsplash', keys: [typed('/unsplash')] }, + { label: 'Vimeo', keys: [typed('/vimeo [url]')] }, + { label: 'CodePen', keys: [typed('/codepen [url]')] }, + { label: 'Spotify', keys: [typed('/spotify [url]')] }, + { label: 'SoundCloud', keys: [typed('/soundcloud [url]')] }, + { label: 'Other embed', keys: [typed('/embed [url]')] }, + ], + }, + ]; +} diff --git a/apps/admin/src/editor/settings/post-settings-sidebar.tsx b/apps/admin/src/editor/settings/post-settings-sidebar.tsx index 2bae696daf8..0bc8d0221f3 100644 --- a/apps/admin/src/editor/settings/post-settings-sidebar.tsx +++ b/apps/admin/src/editor/settings/post-settings-sidebar.tsx @@ -20,6 +20,7 @@ import { PublishDateSection } from './publish-date-section'; import { AuthorsSection } from './authors-section'; import { CodeInjectionSection } from './code-injection-section'; import { DeleteSection } from './delete-section'; +import { KeyboardShortcutsSection } from './keyboard-shortcuts-section'; import { MetaDataSection } from './meta-data-section'; import { PostHistorySection } from './post-history-section'; import { SETTINGS_SECTION_ORDER, type SettingsSectionId } from './sections'; @@ -119,6 +120,7 @@ export function PostSettingsSidebar({ delete: , 'code-injection': , 'meta-data': , + 'keyboard-shortcuts': , 'post-history': ( { + it('reads the platform off the user agent', () => { + expect(isMacPlatform(MAC_AGENT)).toBe(true); + expect(isMacPlatform(WINDOWS_AGENT)).toBe(false); + expect(isMacPlatform('Mozilla/5.0 (X11; Linux x86_64)')).toBe(false); + }); + + it('reads the live user agent when none is given', () => { + expect(isMacPlatform()).toBe(navigator.userAgent.includes('Mac')); + }); +}); diff --git a/apps/admin/src/utils/is-mac-platform.ts b/apps/admin/src/utils/is-mac-platform.ts new file mode 100644 index 00000000000..6e52fe91028 --- /dev/null +++ b/apps/admin/src/utils/is-mac-platform.ts @@ -0,0 +1,4 @@ +/** Whether the writer is on a Mac, which decides the modifier keys shown to them. */ +export function isMacPlatform(userAgent: string = navigator.userAgent): boolean { + return userAgent.indexOf('Mac') !== -1; +} diff --git a/packages/testing/test-data/src/selectors/editor.ts b/packages/testing/test-data/src/selectors/editor.ts index 5fa491cd3a9..2c00ba309d5 100644 --- a/packages/testing/test-data/src/selectors/editor.ts +++ b/packages/testing/test-data/src/selectors/editor.ts @@ -71,6 +71,7 @@ export const settingsMetaTitleInput = 'settings-meta-title-input'; export const settingsMetaDescriptionInput = 'settings-meta-description-input'; export const settingsSerpPreview = 'settings-serp-preview'; export const settingsPostHistoryButton = 'settings-post-history-button'; +export const settingsShortcutRow = 'settings-shortcut-row'; // post history testids export const postHistoryModal = 'post-history-modal'; From 78561ea0a369717bc5353c416c9f0c3b24eef11f Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Wed, 9 Sep 2026 09:43:11 -0500 Subject: [PATCH 14/17] Added the Facebook card pane to the React editor's settings sidebar (#30612) no ref --- apps/admin/src/editor/card-config.test.ts | 50 +++ apps/admin/src/editor/card-config.ts | 11 + apps/admin/src/editor/editor-screen.tsx | 1 + ...settings-facebook-card.acceptance.test.tsx | 352 ++++++++++++++++++ apps/admin/src/editor/editor.screen.ts | 12 + .../src/editor/session/editor-session.ts | 8 +- .../editor/session/settings-fields.test.ts | 44 ++- .../src/editor/session/settings-fields.ts | 29 +- apps/admin/src/editor/settings/README.md | 26 ++ .../settings/facebook-card-fields.test.ts | 118 ++++++ .../editor/settings/facebook-card-fields.ts | 74 ++++ .../editor/settings/facebook-card-section.tsx | 259 +++++++++++++ .../editor/settings/post-settings-sidebar.tsx | 12 + .../testing/test-data/src/selectors/editor.ts | 6 + 14 files changed, 994 insertions(+), 8 deletions(-) create mode 100644 apps/admin/src/editor/editor-settings-facebook-card.acceptance.test.tsx create mode 100644 apps/admin/src/editor/settings/facebook-card-fields.test.ts create mode 100644 apps/admin/src/editor/settings/facebook-card-fields.ts create mode 100644 apps/admin/src/editor/settings/facebook-card-section.tsx diff --git a/apps/admin/src/editor/card-config.test.ts b/apps/admin/src/editor/card-config.test.ts index eadadba450d..46bedde77d6 100644 --- a/apps/admin/src/editor/card-config.test.ts +++ b/apps/admin/src/editor/card-config.test.ts @@ -119,6 +119,56 @@ describe('buildPostCardConfig', () => { expect(cardConfig.deleteSnippet).toBe(ports.deleteSnippet); }); + it('carries the site images the social cards fall back to', () => { + const cardConfig = buildPostCardConfig( + sources({ + settings: settingsFrom({ + ...baseSettings, + og_image: 'site-og.png', + twitter_image: 'site-twitter.png', + cover_image: 'cover.png', + }), + }), + ports, + ); + + expect(cardConfig).toMatchObject({ + siteOgImage: 'site-og.png', + siteTwitterImage: 'site-twitter.png', + siteCoverImage: 'cover.png', + }); + }); + + it('reports a site image the settings do not carry as none', () => { + const cardConfig = buildPostCardConfig(sources(), ports); + + expect(cardConfig).toMatchObject({ + siteOgImage: null, + siteTwitterImage: null, + siteCoverImage: null, + }); + }); + + it.each([123, true])('ignores non-string site images (%s)', (value) => { + const cardConfig = buildPostCardConfig( + sources({ + settings: settingsFrom({ + ...baseSettings, + og_image: value, + twitter_image: value, + cover_image: value, + }), + }), + ports, + ); + + expect(cardConfig).toMatchObject({ + siteOgImage: null, + siteTwitterImage: null, + siteCoverImage: null, + }); + }); + it('drops Unsplash when the integration is off', () => { const cardConfig = buildPostCardConfig( sources({ settings: settingsFrom({ ...baseSettings, unsplash: false }) }), diff --git a/apps/admin/src/editor/card-config.ts b/apps/admin/src/editor/card-config.ts index 0cfb4cabdf0..2b3a79fd113 100644 --- a/apps/admin/src/editor/card-config.ts +++ b/apps/admin/src/editor/card-config.ts @@ -66,6 +66,9 @@ export interface PostCardConfig extends PostCardConfigPorts { membersEnabled: boolean; siteTitle: string; siteDescription: string; + siteOgImage: string | null; + siteTwitterImage: string | null; + siteCoverImage: string | null; siteUrl: string; siteUuid: string; stripeEnabled: boolean; @@ -99,6 +102,11 @@ export function getCardVisibilitySettings( return isPage ? 'web only' : 'web and email'; } +function imageSetting(settings: Setting[], key: string): string | null { + const value = getSettingValue(settings, key); + return typeof value === 'string' ? value : null; +} + export function buildPostCardConfig( sources: PostCardConfigSources, ports: PostCardConfigPorts, @@ -124,6 +132,9 @@ export function buildPostCardConfig( searchLinks: ports.searchLinks, siteTitle: getSettingValue(settings, 'title') ?? '', siteDescription: getSettingValue(settings, 'description') ?? '', + siteOgImage: imageSetting(settings, 'og_image'), + siteTwitterImage: imageSetting(settings, 'twitter_image'), + siteCoverImage: imageSetting(settings, 'cover_image'), siteUrl: getHomepageUrl(site), siteUuid: site.site_uuid, stripeEnabled: checkStripeEnabled(settings, config), diff --git a/apps/admin/src/editor/editor-screen.tsx b/apps/admin/src/editor/editor-screen.tsx index 1d3c19a96f2..e2266b52763 100644 --- a/apps/admin/src/editor/editor-screen.tsx +++ b/apps/admin/src/editor/editor-screen.tsx @@ -214,6 +214,7 @@ function EditorContent({ ; + +function submittedPost(capture: EndpointCapture): Record { + const body = capture.lastRequest?.body as { posts: Record[] } | undefined; + return body?.posts[0] ?? {}; +} + +function asRole(name: StaffRoleName) { + const me = currentUserResponse(); + me.users[0].roles = [staffRole({ name })]; + return { ...FLAG_ON, boot: { browseMe: { response: me } } }; +} + +function editorChrome() { + fakeSnippets([]); + fakePosts([]); + // The header's publish inputs and preview read these beyond the boot table. + fakeMembers([]); + fakeNewsletters([]); + fakeTiers([]); + fakeAdminEndpoint('GET', /^\/slugs\/post\//, ({ url }) => ({ + slugs: [{ slug: decodeURIComponent(url.split('/slugs/post/')[1].split('/')[0]) }], + })); +} + +/** A post that answers saves the way Ghost does: submitted fields back, fresh token. */ +function fakeSavablePost(overrides: Partial = {}) { + editorChrome(); + let current = post({ + id: POST_ID, + title: 'Hello from React', + slug: 'hello-from-react', + status: 'draft', + lexical: buildLexicalParagraph('Hello from React'), + updated_at: LOADED_AT, + published_at: null, + custom_excerpt: null, + excerpt: null, + meta_title: null, + meta_description: null, + og_image: null, + og_title: null, + og_description: null, + feature_image: null, + tags: [], + ...overrides, + }); + let saves = 0; + + fakeAdminEndpoint('GET', ROUTE, () => ({ posts: [current] })); + + return fakeAdminEndpoint('PUT', ROUTE, ({ body }) => { + saves += 1; + const submitted = (body as { posts: Partial[] }).posts[0]; + current = { ...current, ...submitted, updated_at: `2026-01-01T00:00:0${saves}.000Z` }; + return { posts: [current] }; + }); +} + +async function openFacebookCard() { + await editorScreen.settingsToggle().click(); + await expect.element(editorScreen.settingsSidebar()).toBeVisible(); + await editorScreen.settingsSubviewRow('Facebook card').click(); + await expect.element(editorScreen.settingsSubviewPane()).toBeVisible(); +} + +/** + * The sidebar's Facebook card pane: the image, title and description Facebook + * is given instead of the post's own, and the card they produce. + */ +describe('Post settings Facebook card', () => { + it( + 'opens the pane over the section list and comes back from it', + async () => { + fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + // The pane replaces the list it was opened from. + await expect(editorScreen.settingsExcerpt()).toHaveCount(0); + await expect.element(editorScreen.settingsFacebookTitle()).toBeVisible(); + await expect + .element(editorScreen.settingsSidebar()) + .toHaveAttribute('aria-label', 'Facebook card'); + + await editorScreen.settingsSubviewBack(BACK_LABEL).click(); + + await expect(editorScreen.settingsSubviewPane()).toHaveCount(0); + await expect.element(editorScreen.settingsExcerpt()).toBeVisible(); + await expect.element(editorScreen.settingsSubviewRow('Facebook card')).toBeVisible(); + }, + SLOW, + ); + + it( + 'saves an uploaded Facebook image as soon as it lands', + async () => { + const saveApi = fakeSavablePost(); + const uploadApi = fakeAdminEndpoint('POST', '/images/upload/', { + images: [{ url: UPLOADED, ref: null }], + }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + await userEvent.upload( + editorScreen.settingsFacebookImageInput().element(), + new File(['image'], 'hills.png', { type: 'image/png' }), + ); + + await expect.poll(() => uploadApi.requests.length, POLL).toBe(1); + // A field save has no debounce, so it lands well inside the autosave's 3s. + await expect.poll(() => saveApi.requests.length, FIELD_POLL).toBe(1); + expect(submittedPost(saveApi)).toMatchObject({ og_image: UPLOADED }); + await expect.element(editorScreen.removeSettingsFacebookImage()).toBeVisible(); + }, + SLOW, + ); + + it( + 'clears the Facebook image the writer removes', + async () => { + const saveApi = fakeSavablePost({ og_image: UPLOADED }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + await editorScreen.removeSettingsFacebookImage().click(); + + await expect.poll(() => saveApi.requests.length, FIELD_POLL).toBe(1); + expect(submittedPost(saveApi)).toMatchObject({ og_image: null }); + await expect.element(editorScreen.settingsFacebookImageInput()).toBeInTheDocument(); + }, + SLOW, + ); + + it( + 'persists a draft’s Facebook title and description on the blur that ends each edit', + async () => { + const saveApi = fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + await editorScreen.settingsFacebookTitle().fill('A better title for Facebook'); + await editorScreen.settingsFacebookDescription().click(); + + // A field save has no debounce, so it lands well inside the autosave's 3s. + await expect.poll(() => saveApi.requests.length, FIELD_POLL).toBe(1); + expect(submittedPost(saveApi)).toMatchObject({ og_title: 'A better title for Facebook' }); + + await editorScreen.settingsFacebookDescription().fill('What this post is about'); + await editorScreen.settingsFacebookTitle().click(); + + await expect.poll(() => saveApi.requests.length, FIELD_POLL).toBe(2); + expect(submittedPost(saveApi)).toMatchObject({ + og_description: 'What this post is about', + }); + }, + SLOW, + ); + + it( + 'stages a published post’s Facebook title until Update', + async () => { + const saveApi = fakeSavablePost({ status: 'published', published_at: PUBLISHED_AT }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + await editorScreen.settingsFacebookTitle().fill('A better title for Facebook'); + await editorScreen.settingsFacebookDescription().click(); + + await expect.element(editorScreen.updateButton()).toBeEnabled(); + await expect.poll(unsavedChangesGuarded).toBe(true); + expect(saveApi.requests).toHaveLength(0); + + await userEvent.keyboard('{Meta>}s{/Meta}'); + + await expect.poll(() => saveApi.requests.length, POLL).toBe(1); + expect(submittedPost(saveApi)).toMatchObject({ + og_title: 'A better title for Facebook', + status: 'published', + }); + }, + SLOW, + ); + + it( + 'offers the post’s own title and excerpt until the Facebook fields carry their own', + async () => { + fakeSavablePost({ custom_excerpt: 'The excerpt this post already has' }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + await expect + .element(editorScreen.settingsFacebookTitle()) + .toHaveAttribute('placeholder', 'Hello from React'); + await expect + .element(editorScreen.settingsFacebookDescription()) + .toHaveAttribute('placeholder', 'The excerpt this post already has'); + + const preview = editorScreen.settingsFacebookPreview(); + await expect.element(preview).toHaveTextContent('test.com'); + await expect.element(preview).toHaveTextContent('Hello from React'); + await expect.element(preview).toHaveTextContent('The excerpt this post already has'); + + await editorScreen.settingsFacebookTitle().fill('A better title for Facebook'); + await editorScreen.settingsFacebookDescription().fill('What this post is about'); + + await expect.element(preview).toHaveTextContent('A better title for Facebook'); + await expect.element(preview).toHaveTextContent('What this post is about'); + await expect.element(preview).not.toHaveTextContent('The excerpt this post already has'); + }, + SLOW, + ); + + it( + 'previews the feature image the writer is looking at, and follows it as it changes', + async () => { + fakeSavablePost({ feature_image: FEATURE }); + fakeAdminEndpoint('POST', '/images/upload/', { images: [{ url: UPLOADED, ref: null }] }); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + // The card falls back to it while the pane's own dropzone is still empty. + await expect + .element(editorScreen.settingsFacebookPreviewImage()) + .toHaveAttribute('src', FEATURE); + await expect.element(editorScreen.settingsFacebookImageInput()).toBeInTheDocument(); + + const pane = editorScreen.settingsSubviewPane().element(); + + await editorScreen.removeFeatureImage().click(); + + await expect(editorScreen.settingsFacebookPreviewImage()).toHaveCount(0); + + await userEvent.upload( + editorScreen.featureImageInput().element(), + new File(['image'], 'coast.png', { type: 'image/png' }), + ); + + await expect + .element(editorScreen.settingsFacebookPreviewImage()) + .toHaveAttribute('src', UPLOADED); + // The open pane followed the canvas rather than being rebuilt around it. + expect(pane.isConnected).toBe(true); + }, + SLOW, + ); + + it( + 'falls back to the site’s own description for a post that has none', + async () => { + fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + await expect + .element(editorScreen.settingsFacebookDescription()) + .toHaveAttribute('placeholder', SITE_DESCRIPTION); + await expect + .element(editorScreen.settingsFacebookPreview()) + .toHaveTextContent(SITE_DESCRIPTION); + }, + SLOW, + ); + + it( + 'refuses to save a Facebook title longer than the field holds', + async () => { + const saveApi = fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + await openFacebookCard(); + + await editorScreen.settingsFacebookTitle().fill('a'.repeat(301)); + await editorScreen.settingsFacebookDescription().click(); + + await expect + .element(editorScreen.settingsSubviewPane().getByRole('alert')) + .toHaveTextContent('Facebook Title cannot be longer than 300 characters.'); + await expect + .element(editorScreen.settingsFacebookTitle()) + .toHaveAttribute('aria-invalid', 'true'); + // Refused where the writer is typing rather than as a save they did not ask for. + await expect.poll(unsavedChangesGuarded).toBe(true); + await expect(editorScreen.saveErrorBanner()).toHaveCount(0); + expect(saveApi.requests).toHaveLength(0); + + await userEvent.keyboard('{Meta>}s{/Meta}'); + + await expect + .element(editorScreen.saveErrorBanner()) + .toHaveTextContent('Facebook Title cannot be longer than 300 characters.'); + expect(saveApi.requests).toHaveLength(0); + }, + SLOW, + ); + + it( + 'gives a contributor the pane their role can write', + async () => { + // A contributor may only open a draft they authored. + const saveApi = fakeSavablePost({ authors: [{ id: CURRENT_USER_ID }] }); + await renderAdminApp(`/editor/post/${POST_ID}`, asRole('Contributor')); + await openFacebookCard(); + + await editorScreen.settingsFacebookTitle().fill('A contributor’s Facebook title'); + await editorScreen.settingsFacebookDescription().click(); + + await expect + .poll(() => submittedPost(saveApi).og_title, FIELD_POLL) + .toBe('A contributor’s Facebook title'); + }, + SLOW, + ); +}); diff --git a/apps/admin/src/editor/editor.screen.ts b/apps/admin/src/editor/editor.screen.ts index 86f9815a57f..e82009dc741 100644 --- a/apps/admin/src/editor/editor.screen.ts +++ b/apps/admin/src/editor/editor.screen.ts @@ -1,5 +1,6 @@ import { page } from 'vitest/browser'; import { + addFacebookImageLabel, addFeatureImageLabel, conflictCancelReloadButton, conflictCopyContentButton, @@ -44,6 +45,7 @@ import { postHistoryRevisionList, postSettingsSidebar, postsBackLink, + removeFacebookImageButton, removeFeatureImageButton, settingsAuthorChip, settingsAuthorsError, @@ -55,6 +57,10 @@ import { settingsDeleteDialog, settingsDeleteError, settingsExcerptInput, + settingsFacebookDescriptionInput, + settingsFacebookPreview, + settingsFacebookPreviewImage, + settingsFacebookTitleInput, settingsFeaturedToggle, settingsMenuToggle, settingsPublishDate, @@ -225,6 +231,12 @@ export const editorScreen = { .getByTestId(settingsShortcutRow) .elements() .map((row) => row.textContent ?? ''), + settingsFacebookTitle: () => page.getByTestId(settingsFacebookTitleInput), + settingsFacebookDescription: () => page.getByTestId(settingsFacebookDescriptionInput), + settingsFacebookPreview: () => page.getByTestId(settingsFacebookPreview), + settingsFacebookPreviewImage: () => page.getByTestId(settingsFacebookPreviewImage), + settingsFacebookImageInput: () => page.getByLabelText(addFacebookImageLabel), + removeSettingsFacebookImage: () => page.getByRole('button', { name: removeFacebookImageButton }), settingsPostHistory: () => page.getByTestId(settingsPostHistoryButton), postHistoryModal: () => page.getByTestId(postHistoryModal), diff --git a/apps/admin/src/editor/session/editor-session.ts b/apps/admin/src/editor/session/editor-session.ts index 3ceee06d4e5..370f11f2d32 100644 --- a/apps/admin/src/editor/session/editor-session.ts +++ b/apps/admin/src/editor/session/editor-session.ts @@ -35,6 +35,7 @@ import { identityFor, publishedAtInFuture, settingsFieldError, + validatedFieldsOf, type EditorSettingsPatch, type SettingsFieldKey, type ValidatedSettingsFields, @@ -410,12 +411,7 @@ export function createEditorSession({ ...request, projection, authoredFrom: { title: live.title, slug: live.slug }, - validated: { - visibility: live.visibility, - tiers: live.tiers, - meta_title: live.meta_title, - meta_description: live.meta_description, - }, + validated: validatedFieldsOf(live), builtAtVersion: version, payload, options: { diff --git a/apps/admin/src/editor/session/settings-fields.test.ts b/apps/admin/src/editor/session/settings-fields.test.ts index 499aef5d4aa..61a747589ef 100644 --- a/apps/admin/src/editor/session/settings-fields.test.ts +++ b/apps/admin/src/editor/session/settings-fields.test.ts @@ -4,12 +4,25 @@ import { META_DESCRIPTION_TOO_LONG, META_TITLE_MAX, META_TITLE_TOO_LONG, + OG_DESCRIPTION_MAX, + OG_DESCRIPTION_TOO_LONG, + OG_TITLE_MAX, + OG_TITLE_TOO_LONG, TIERS_REQUIRED, + VALIDATED_SETTINGS_FIELD_KEYS, overLength, settingsFieldError, + validatedFieldsOf, } from './settings-fields'; -const VALID = { visibility: 'public', tiers: [], meta_title: null, meta_description: null }; +const VALID = { + visibility: 'public', + tiers: [], + meta_title: null, + meta_description: null, + og_title: null, + og_description: null, +}; describe('overLength', () => { it('counts a multibyte character once', () => { @@ -22,6 +35,15 @@ describe('overLength', () => { }); }); +describe('validatedFieldsOf', () => { + it('takes the keys the validator reads and leaves the rest behind', () => { + const live = { ...VALID, meta_title: 'Meta', custom_excerpt: 'Excerpt', featured: true }; + + expect(validatedFieldsOf(live)).toEqual({ ...VALID, meta_title: 'Meta' }); + expect(Object.keys(validatedFieldsOf(live))).toEqual([...VALIDATED_SETTINGS_FIELD_KEYS]); + }); +}); + describe('settingsFieldError', () => { it('passes fields that break no rule', () => { expect(settingsFieldError(VALID)).toBeNull(); @@ -47,10 +69,30 @@ describe('settingsFieldError', () => { ).toBe(META_DESCRIPTION_TOO_LONG); }); + it('refuses a Facebook title past the column width', () => { + expect(settingsFieldError({ ...VALID, og_title: 'a'.repeat(OG_TITLE_MAX) })).toBeNull(); + expect(settingsFieldError({ ...VALID, og_title: 'a'.repeat(OG_TITLE_MAX + 1) })).toBe( + OG_TITLE_TOO_LONG, + ); + }); + + it('refuses a Facebook description past the column width', () => { + expect( + settingsFieldError({ ...VALID, og_description: 'a'.repeat(OG_DESCRIPTION_MAX) }), + ).toBeNull(); + expect( + settingsFieldError({ ...VALID, og_description: 'a'.repeat(OG_DESCRIPTION_MAX + 1) }), + ).toBe(OG_DESCRIPTION_TOO_LONG); + }); + it('names the field the message is about', () => { expect(META_TITLE_TOO_LONG).toBe('Meta Title cannot be longer than 300 characters.'); expect(META_DESCRIPTION_TOO_LONG).toBe( 'Meta Description cannot be longer than 500 characters.', ); + expect(OG_TITLE_TOO_LONG).toBe('Facebook Title cannot be longer than 300 characters.'); + expect(OG_DESCRIPTION_TOO_LONG).toBe( + 'Facebook Description cannot be longer than 500 characters.', + ); }); }); diff --git a/apps/admin/src/editor/session/settings-fields.ts b/apps/admin/src/editor/session/settings-fields.ts index 886834910b1..ca05f1e8e3a 100644 --- a/apps/admin/src/editor/session/settings-fields.ts +++ b/apps/admin/src/editor/session/settings-fields.ts @@ -59,9 +59,13 @@ export function identityFor(key: SettingsFieldKey, value: unknown): unknown { /** The column widths the schema gives these fields. */ export const META_TITLE_MAX = 300; export const META_DESCRIPTION_MAX = 500; +export const OG_TITLE_MAX = 300; +export const OG_DESCRIPTION_MAX = 500; export const META_TITLE_TOO_LONG = `Meta Title cannot be longer than ${META_TITLE_MAX} characters.`; export const META_DESCRIPTION_TOO_LONG = `Meta Description cannot be longer than ${META_DESCRIPTION_MAX} characters.`; +export const OG_TITLE_TOO_LONG = `Facebook Title cannot be longer than ${OG_TITLE_MAX} characters.`; +export const OG_DESCRIPTION_TOO_LONG = `Facebook Description cannot be longer than ${OG_DESCRIPTION_MAX} characters.`; /** `visibility: 'tiers'` with no tiers: the write contract drops the visibility. */ export function tiersIncomplete( @@ -91,11 +95,28 @@ export function overLength(value: string | null, max: number): boolean { return Array.from(value ?? '').length > max; } +/** The settings keys the validator reads, and all a prepared save carries for it. */ +export const VALIDATED_SETTINGS_FIELD_KEYS = [ + 'visibility', + 'tiers', + 'meta_title', + 'meta_description', + 'og_title', + 'og_description', +] as const; + export type ValidatedSettingsFields = Pick< EditorSettingsFields, - 'visibility' | 'tiers' | 'meta_title' | 'meta_description' + (typeof VALIDATED_SETTINGS_FIELD_KEYS)[number] >; +/** The validator's own view of the live document. */ +export function validatedFieldsOf(fields: ValidatedSettingsFields): ValidatedSettingsFields { + return Object.fromEntries( + VALIDATED_SETTINGS_FIELD_KEYS.map((key) => [key, fields[key]]), + ) as ValidatedSettingsFields; +} + /** The first rule the settings fields break, in the post validator's order. */ export function settingsFieldError(fields: ValidatedSettingsFields): string | null { if (tiersIncomplete(fields)) { @@ -107,5 +128,11 @@ export function settingsFieldError(fields: ValidatedSettingsFields): string | nu if (overLength(fields.meta_description, META_DESCRIPTION_MAX)) { return META_DESCRIPTION_TOO_LONG; } + if (overLength(fields.og_title, OG_TITLE_MAX)) { + return OG_TITLE_TOO_LONG; + } + if (overLength(fields.og_description, OG_DESCRIPTION_MAX)) { + return OG_DESCRIPTION_TOO_LONG; + } return null; } diff --git a/apps/admin/src/editor/settings/README.md b/apps/admin/src/editor/settings/README.md index c8aa976482d..93be44735e5 100644 --- a/apps/admin/src/editor/settings/README.md +++ b/apps/admin/src/editor/settings/README.md @@ -423,6 +423,32 @@ user agent as the pane renders. Hovering a glyph names the key it stands for; a key already shown as its name carries no tooltip. A slash command reads the same wherever it is typed. +## Facebook card + +The card Facebook shows for the post is a pane, and every role that can open the +panel can open it. Its image, title and description are the post's `og_` fields: +the title and description are staged as the writer types and committed on the +blur that ends the edit, and an uploaded or removed image is committed as it +lands rather than waiting for a blur. Committing is not saving, so the save +policy above still decides: a draft persists all three, and every other status +stages them until Update. A field cleared back to empty is stored as no value. +The image comes from the file picker or a drop; there is no Unsplash picker here. + +Nothing here is required, and each line falls back rather than emptying. The +title is the Facebook title, else the meta title, else the title the writer is +looking at, else `(Untitled)`. The description is the Facebook description, else +the post's excerpt, else its meta description, else the excerpt the server +generated for it, else the site's own description. The image is the Facebook +image, else the post's feature image, else the site's social image and cover +image. Those fallbacks are what the two inputs show as placeholders, truncated to +40 and 150 characters, and what the card under them previews, truncated to 140 +and shown against the site's address without its scheme. + +The lengths that are limits are the column widths, 300 for the title and 500 for +the description. Past one of those the field says so where the writer is typing +and nothing is saved — not the field itself, and not a save the writer asks for, +which is refused with the same message. + ## Open and closed The toggle sits in the editor header, and the panel starts closed on every diff --git a/apps/admin/src/editor/settings/facebook-card-fields.test.ts b/apps/admin/src/editor/settings/facebook-card-fields.test.ts new file mode 100644 index 00000000000..d90aed86e57 --- /dev/null +++ b/apps/admin/src/editor/settings/facebook-card-fields.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it } from 'vitest'; +import { + facebookDescription, + facebookDescriptionPlaceholder, + facebookImage, + facebookPreviewText, + facebookTitle, + facebookTitlePlaceholder, + siteDomain, +} from './facebook-card-fields'; + +const NO_TITLE = { ogTitle: '', metaTitle: '', title: '' }; +const NO_DESCRIPTION = { + ogDescription: '', + customExcerpt: '', + metaDescription: '', + postExcerpt: '', + siteDescription: '', +}; +const NO_IMAGE = { ogImage: '', featureImage: '', siteOgImage: '', siteCoverImage: '' }; + +describe('facebookTitle', () => { + it('prefers the Facebook title, then the meta title, then the post title', () => { + expect(facebookTitle({ ogTitle: 'Facebook', metaTitle: 'Meta', title: 'Post' })).toBe( + 'Facebook', + ); + expect(facebookTitle({ ...NO_TITLE, metaTitle: 'Meta', title: 'Post' })).toBe('Meta'); + expect(facebookTitle({ ...NO_TITLE, title: 'Post' })).toBe('Post'); + }); + + it('falls back to the untitled placeholder', () => { + expect(facebookTitle(NO_TITLE)).toBe('(Untitled)'); + }); +}); + +describe('facebookDescription', () => { + it('prefers the Facebook description, then the excerpt, then the meta description', () => { + expect( + facebookDescription({ + ogDescription: 'Facebook', + customExcerpt: 'Excerpt', + metaDescription: 'Meta', + postExcerpt: 'Generated', + siteDescription: 'Site', + }), + ).toBe('Facebook'); + expect( + facebookDescription({ + ...NO_DESCRIPTION, + customExcerpt: 'Excerpt', + metaDescription: 'Meta', + postExcerpt: 'Generated', + }), + ).toBe('Excerpt'); + expect( + facebookDescription({ ...NO_DESCRIPTION, metaDescription: 'Meta', postExcerpt: 'Generated' }), + ).toBe('Meta'); + }); + + it('falls back to the generated excerpt, then the site description, then nothing', () => { + expect( + facebookDescription({ + ...NO_DESCRIPTION, + postExcerpt: 'Generated', + siteDescription: 'Site', + }), + ).toBe('Generated'); + expect(facebookDescription({ ...NO_DESCRIPTION, siteDescription: 'Site' })).toBe('Site'); + expect(facebookDescription(NO_DESCRIPTION)).toBe(''); + }); +}); + +describe('facebookImage', () => { + it('prefers the Facebook image, then the feature image, then the site images', () => { + expect( + facebookImage({ + ogImage: 'og.png', + featureImage: 'feature.png', + siteOgImage: 'site-og.png', + siteCoverImage: 'cover.png', + }), + ).toBe('og.png'); + expect( + facebookImage({ ...NO_IMAGE, featureImage: 'feature.png', siteOgImage: 'site-og.png' }), + ).toBe('feature.png'); + expect( + facebookImage({ ...NO_IMAGE, siteOgImage: 'site-og.png', siteCoverImage: 'c.png' }), + ).toBe('site-og.png'); + expect(facebookImage({ ...NO_IMAGE, siteCoverImage: 'c.png' })).toBe('c.png'); + expect(facebookImage(NO_IMAGE)).toBe(''); + }); +}); + +describe('the lengths the pane truncates to', () => { + const long = 'a'.repeat(400); + + it('cuts the title placeholder to 40 characters, ellipsis included', () => { + expect(facebookTitlePlaceholder(long)).toBe(`${'a'.repeat(37)}...`); + expect(facebookTitlePlaceholder(long)).toHaveLength(40); + }); + + it('cuts the description placeholder to 150 characters', () => { + expect(facebookDescriptionPlaceholder(long)).toBe(`${'a'.repeat(147)}...`); + expect(facebookDescriptionPlaceholder(long)).toHaveLength(150); + }); + + it('cuts the preview to 140 characters, shorter than either placeholder', () => { + expect(facebookPreviewText(long)).toBe(`${'a'.repeat(137)}...`); + expect(facebookPreviewText(long)).toHaveLength(140); + }); +}); + +describe('siteDomain', () => { + it('drops the scheme and the trailing slash', () => { + expect(siteDomain('https://example.com/')).toBe('example.com'); + expect(siteDomain('http://example.com/blog/')).toBe('example.com/blog'); + }); +}); diff --git a/apps/admin/src/editor/settings/facebook-card-fields.ts b/apps/admin/src/editor/settings/facebook-card-fields.ts new file mode 100644 index 00000000000..7b9193f5776 --- /dev/null +++ b/apps/admin/src/editor/settings/facebook-card-fields.ts @@ -0,0 +1,74 @@ +import { seoTitle, truncate } from './meta-data-fields'; + +/** The lengths the pane truncates to: the two placeholders, then the preview. */ +export const FACEBOOK_TITLE_PLACEHOLDER_LENGTH = 40; +export const FACEBOOK_DESCRIPTION_PLACEHOLDER_LENGTH = 150; +export const FACEBOOK_PREVIEW_LENGTH = 140; + +export interface FacebookTitleSources { + ogTitle: string; + metaTitle: string; + title: string; +} + +export interface FacebookDescriptionSources { + ogDescription: string; + customExcerpt: string; + metaDescription: string; + /** The excerpt the post was read with, generated from its body where it has no custom one. */ + postExcerpt: string; + siteDescription: string; +} + +export interface FacebookImageSources { + ogImage: string; + featureImage: string; + siteOgImage: string; + siteCoverImage: string; +} + +/** The Facebook title, else the post's search title. */ +export function facebookTitle({ ogTitle, metaTitle, title }: FacebookTitleSources): string { + return ogTitle || seoTitle(metaTitle, title); +} + +/** + * The Facebook description, else the post's excerpt, its search description, + * the excerpt the server generated, and finally the site's own description. + */ +export function facebookDescription({ + ogDescription, + customExcerpt, + metaDescription, + postExcerpt, + siteDescription, +}: FacebookDescriptionSources): string { + return ogDescription || customExcerpt || metaDescription || postExcerpt || siteDescription || ''; +} + +/** The Facebook image, else the feature image and the site's own images. */ +export function facebookImage({ + ogImage, + featureImage, + siteOgImage, + siteCoverImage, +}: FacebookImageSources): string { + return ogImage || featureImage || siteOgImage || siteCoverImage || ''; +} + +/** The site's address as the card shows it: no scheme, no trailing slash. */ +export function siteDomain(siteUrl: string): string { + return siteUrl.replace(/^https?:\/\//, '').replace(/\/$/, ''); +} + +export function facebookTitlePlaceholder(title: string): string { + return truncate(title, FACEBOOK_TITLE_PLACEHOLDER_LENGTH); +} + +export function facebookDescriptionPlaceholder(description: string): string { + return truncate(description, FACEBOOK_DESCRIPTION_PLACEHOLDER_LENGTH); +} + +export function facebookPreviewText(value: string): string { + return truncate(value, FACEBOOK_PREVIEW_LENGTH); +} diff --git a/apps/admin/src/editor/settings/facebook-card-section.tsx b/apps/admin/src/editor/settings/facebook-card-section.tsx new file mode 100644 index 00000000000..bae9aece27d --- /dev/null +++ b/apps/admin/src/editor/settings/facebook-card-section.tsx @@ -0,0 +1,259 @@ +import { useCallback, useId } from 'react'; +import { toast } from 'sonner'; +import { Input, Label, LoadingIndicator, Textarea } from '@tryghost/shade/components'; +import { + ImageUpload, + ImageUploadAction, + ImageUploadActions, + ImageUploadDropzone, + ImageUploadImage, + ImageUploadPreview, +} from '@tryghost/shade/patterns'; +import { Inline, Stack, Text } from '@tryghost/shade/primitives'; +import { LucideIcon } from '@tryghost/shade/utils'; +import { getImageUrl, useUploadImage } from '@tryghost/admin-x-framework/api/images'; +import { + JSONError, + RequestEntityTooLargeError, + UnsupportedMediaTypeError, +} from '@tryghost/admin-x-framework/errors'; +import { + settingsFacebookDescriptionInput, + settingsFacebookPreview, + settingsFacebookPreviewImage, + settingsFacebookTitleInput, +} from '@tryghost/test-data/selectors/editor'; +import BrandIcon from '@/shared/brand-icon/brand-icon'; +import type { PostCardConfig } from '@/editor/card-config'; +import { + OG_DESCRIPTION_MAX, + OG_DESCRIPTION_TOO_LONG, + OG_TITLE_MAX, + OG_TITLE_TOO_LONG, + overLength, +} from '@/editor/session/settings-fields'; +import type { EditorSessionHandle } from '@/editor/session/use-editor-session'; +import { + facebookDescription, + facebookDescriptionPlaceholder, + facebookImage, + facebookPreviewText, + facebookTitle, + facebookTitlePlaceholder, + siteDomain, +} from './facebook-card-fields'; +import { SettingsSubview } from './settings-subview'; + +const ACCEPTED_IMAGE_TYPES = { + 'image/gif': ['.gif'], + 'image/jpeg': ['.jpg', '.jpeg'], + 'image/png': ['.png'], + 'image/svg+xml': ['.svg', '.svgz'], + 'image/webp': ['.webp'], +}; + +const UNSUPPORTED_IMAGE_MESSAGE = + 'The image type you uploaded is not supported. Please use .GIF, .JPG, .JPEG, .PNG, .SVG, .SVGZ, .WEBP'; + +const ADD_IMAGE_LABEL = 'Add Facebook image'; +const REMOVE_IMAGE_LABEL = 'Remove Facebook image'; + +function uploadErrorMessage(error: unknown): string { + if (error instanceof UnsupportedMediaTypeError) { + return UNSUPPORTED_IMAGE_MESSAGE; + } + if (error instanceof RequestEntityTooLargeError) { + return 'The image you uploaded was larger than the maximum file size your server allows.'; + } + if (error instanceof JSONError && error.data?.errors[0]?.message) { + return error.data.errors[0].message; + } + return 'Couldn’t upload the Facebook image.'; +} + +function FieldError({ id, message }: { id: string; message: string }) { + return ( + + {message} + + ); +} + +export interface FacebookCardSectionProps { + session: EditorSessionHandle; + /** The site's homepage URL, which the card previews the post under. */ + siteUrl: string; + /** The feature image the writer is looking at, which the card falls back to. */ + featureImage: string | null; + /** Carries the site's own description and images, which the card falls back to last. */ + cardConfig: PostCardConfig; +} + +/** + * The card Facebook shows for the post: an image, title and description that + * stand in for the post's own, and the result they produce. + */ +export function FacebookCardSection({ + session, + siteUrl, + featureImage, + cardConfig, +}: FacebookCardSectionProps) { + const titleId = useId(); + const titleErrorId = useId(); + const descriptionId = useId(); + const descriptionErrorId = useId(); + const { mutateAsync: uploadImage, isPending } = useUploadImage(); + + const ogImage = session.settings.og_image ?? ''; + const ogTitle = session.settings.og_title ?? ''; + const ogDescription = session.settings.og_description ?? ''; + const titleError = overLength(ogTitle, OG_TITLE_MAX) ? OG_TITLE_TOO_LONG : null; + const descriptionError = overLength(ogDescription, OG_DESCRIPTION_MAX) + ? OG_DESCRIPTION_TOO_LONG + : null; + + const previewTitle = facebookTitle({ + ogTitle, + metaTitle: session.settings.meta_title ?? '', + title: session.bind.title, + }); + const previewDescription = facebookDescription({ + ogDescription, + customExcerpt: session.settings.custom_excerpt ?? '', + metaDescription: session.settings.meta_description ?? '', + postExcerpt: session.loadedRecord?.excerpt ?? '', + siteDescription: cardConfig.siteDescription, + }); + const previewImage = facebookImage({ + ogImage, + featureImage: featureImage ?? '', + siteOgImage: cardConfig.siteOgImage ?? '', + siteCoverImage: cardConfig.siteCoverImage ?? '', + }); + + const handleUpload = useCallback( + async (file: File) => { + try { + session.editSettings({ og_image: getImageUrl(await uploadImage({ file })) }); + } catch (error) { + toast.error(uploadErrorMessage(error)); + } + }, + [session, uploadImage], + ); + + return ( + } + id="facebook-card" + label="Facebook card" + title="Facebook card" + wide + > + {ogImage ? ( + + + + + session.editSettings({ og_image: null })} + > + + + + + + ) : ( + + files[0] && void handleUpload(files[0])} + onDropRejected={() => toast.error(UNSUPPORTED_IMAGE_MESSAGE)} + > + {isPending ? ( + + ) : ( + + + )} + + + )} + + + + session.stageSettings({ og_title: event.target.value || null })} + /> + {titleError ? : null} + + + + +