From 8c9085b63dd4482ac131b304c26df2dcb0f1bdc6 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Wed, 26 Aug 2026 18:00:35 +0100 Subject: [PATCH 1/6] Added gift recipient email pre-fill (#30289) no issue Email-delivered gifts already know the intended recipient address, so the customized redemption flow can use it as an editable prefill instead of asking the recipient to enter it again. - Added nullable recipient email to the shared GET and POST redemption DTO - Read delivery details through GiftDelivery without changing the Gift model - Prefilled only the customized Portal redemption page - Updated the bearer-link domain contract and regression coverage --- .../pages/beta-gift-redemption-page.jsx | 6 ++--- .../pages/gift-redemption-page.test.jsx | 22 +++++++++++----- .../core/server/services/gifts/CONTEXT.md | 2 +- .../gift-delivery-bookshelf-repository.ts | 18 +++++++++++++ .../services/gifts/gift-delivery-service.ts | 9 +++++++ .../server/services/gifts/gift-service.ts | 20 +++++++++++--- .../members/gift-subscriptions.test.js | 15 ++++++++++- ...gift-delivery-bookshelf-repository.test.ts | 10 +++++++ .../gifts/gift-delivery-service.test.ts | 26 +++++++++++++++++++ .../gifts/gift-service-interface.test.ts | 2 ++ .../services/gifts/gift-service.test.ts | 16 ++++++++++++ 11 files changed, 132 insertions(+), 14 deletions(-) diff --git a/apps/portal/src/components/pages/beta-gift-redemption-page.jsx b/apps/portal/src/components/pages/beta-gift-redemption-page.jsx index 59f36c59f36..c9c7111e1e2 100644 --- a/apps/portal/src/components/pages/beta-gift-redemption-page.jsx +++ b/apps/portal/src/components/pages/beta-gift-redemption-page.jsx @@ -60,7 +60,7 @@ const BetaGiftRedemptionPage = () => { const gift = pageData?.gift; const isLoggedIn = !!member; const [name, setName] = useState(gift?.recipient_name || member?.name || ''); - const [email, setEmail] = useState(member?.email || ''); + const [email, setEmail] = useState(member?.email || gift?.recipient_email || ''); const [errors, setErrors] = useState({}); const [showDetails, setShowDetails] = useState(false); const { cardRef, containerProps: cardTiltProps } = useCardTilt(); @@ -69,9 +69,9 @@ const BetaGiftRedemptionPage = () => { // Prefill with the recipient name the buyer entered, so the gift card // is personal before the recipient types anything. setName(gift?.recipient_name || member?.name || ''); - setEmail(member?.email || ''); + setEmail(member?.email || gift?.recipient_email || ''); setErrors({}); - }, [member?.email, member?.name, gift?.recipient_name]); + }, [member?.email, member?.name, gift?.recipient_email, gift?.recipient_name]); useEffect(() => { if (gift) { diff --git a/apps/portal/test/unit/components/pages/gift-redemption-page.test.jsx b/apps/portal/test/unit/components/pages/gift-redemption-page.test.jsx index 96091f2bb9a..fc03032e6c8 100644 --- a/apps/portal/test/unit/components/pages/gift-redemption-page.test.jsx +++ b/apps/portal/test/unit/components/pages/gift-redemption-page.test.jsx @@ -141,17 +141,17 @@ describe('BetaGiftRedemptionPage', () => { expect(queryByText(member.free.name)).not.toBeInTheDocument(); }); - test('presents the buyer details and prefills the intended recipient name', () => { + test('presents the buyer details and prefills the intended recipient details', async () => { const personalizedGift = { ...gift, buyer_name: 'Jamie', recipient_name: 'Taylor', + recipient_email: 'taylor@example.com', message: 'Enjoy this!', expires_at: '2030-01-01T00:00:00.000Z', }; - const { container, getByLabelText, getByTestId, getByText } = renderGiftRedemptionPage( - BetaGiftRedemptionPage, - { + const { container, getByLabelText, getByRole, getByTestId, getByText, mockDoActionFn } = + renderGiftRedemptionPage(BetaGiftRedemptionPage, { site: { ...testSite, url: 'https://example.com/', @@ -162,10 +162,10 @@ describe('BetaGiftRedemptionPage', () => { token: 'gift-token-123', gift: personalizedGift, }, - }, - ); + }); expect(getByLabelText(/your name/i)).toHaveValue('Taylor'); + expect(getByLabelText(/your email/i)).toHaveValue('taylor@example.com'); expect(getByLabelText(/your email/i)).toHaveFocus(); const subtitle = container.querySelector('.gh-portal-gift-checkout-subtitle'); expect(subtitle).toHaveTextContent( @@ -175,6 +175,16 @@ describe('BetaGiftRedemptionPage', () => { expect(getByTestId('gift-message')).toHaveTextContent('Enjoy this!'); expect(getByTestId('gift-message')).toHaveTextContent('Jamie'); expect(getByText(/This gift can only be redeemed once and expires on/i)).toBeInTheDocument(); + + fireEvent.click(getByRole('button', { name: 'Redeem your gift' })); + + await waitFor(() => { + expect(mockDoActionFn).toHaveBeenCalledWith('redeemGift', { + email: 'taylor@example.com', + name: 'Taylor', + giftToken: 'gift-token-123', + }); + }); }); test('presents the claim deadline in the publication locale and timezone', () => { diff --git a/ghost/core/core/server/services/gifts/CONTEXT.md b/ghost/core/core/server/services/gifts/CONTEXT.md index 02f8c0b50fd..25fe19fef0f 100644 --- a/ghost/core/core/server/services/gifts/CONTEXT.md +++ b/ghost/core/core/server/services/gifts/CONTEXT.md @@ -37,7 +37,7 @@ The buyer's chosen way of handing over a gift subscription: the publication emai _Avoid_: Delivery mode **Redemption link**: -A single-use, time-limited link through which a purchased gift subscription can be viewed and claimed. Its bearer may see the gift's buyer name, intended recipient name, and personal message, but not email-routing or delivery details. +A single-use, time-limited link through which a purchased gift subscription can be viewed and claimed. Its bearer may see the gift's buyer name, intended recipient name, recipient email, and personal message; the intended recipient does not reserve redemption. _Avoid_: Gift Link **Gift redemption**: diff --git a/ghost/core/core/server/services/gifts/gift-delivery-bookshelf-repository.ts b/ghost/core/core/server/services/gifts/gift-delivery-bookshelf-repository.ts index 331e2ea2ae2..a248a9fae5d 100644 --- a/ghost/core/core/server/services/gifts/gift-delivery-bookshelf-repository.ts +++ b/ghost/core/core/server/services/gifts/gift-delivery-bookshelf-repository.ts @@ -24,6 +24,10 @@ export interface GiftDeliveryRepository { giftId: string, options?: RepositoryTransactionOptions, ): Promise; + getByGiftToken( + giftToken: string, + options?: RepositoryTransactionOptions, + ): Promise; getByProviderMessageId(providerMessageId: string): Promise; findRecoverableForPurchasedGifts( now: Date, @@ -124,6 +128,20 @@ export class GiftDeliveryBookshelfRepository implements GiftDeliveryRepository { return model ? decodeGiftDeliveryRow(model.toJSON()) : null; } + async getByGiftToken( + giftToken: string, + options: RepositoryTransactionOptions = {}, + ): Promise { + const db = options.transacting ?? this.knex; + const row = await db('gift_deliveries') + .select('gift_deliveries.*') + .join('gifts', 'gifts.id', 'gift_deliveries.gift_id') + .where('gifts.token', giftToken) + .first(); + + return row ? decodeGiftDeliveryRow(row) : null; + } + async getByProviderMessageId(providerMessageId: string): Promise { const model = await this.model.findOne( { email_provider_message_id: providerMessageId }, diff --git a/ghost/core/core/server/services/gifts/gift-delivery-service.ts b/ghost/core/core/server/services/gifts/gift-delivery-service.ts index 5bbc7bd6e33..1d5640eefe1 100644 --- a/ghost/core/core/server/services/gifts/gift-delivery-service.ts +++ b/ghost/core/core/server/services/gifts/gift-delivery-service.ts @@ -124,6 +124,15 @@ export class GiftDeliveryService { return delivery.id; } + async getRecipientEmailForGift( + giftToken: string, + options: RepositoryTransactionOptions = {}, + ): Promise { + const delivery = await this.deps.giftDeliveryRepository.getByGiftToken(giftToken, options); + + return delivery?.recipientEmail ?? null; + } + async dispatchForGift({ giftId, }: { diff --git a/ghost/core/core/server/services/gifts/gift-service.ts b/ghost/core/core/server/services/gifts/gift-service.ts index 05e17912245..d9607258282 100644 --- a/ghost/core/core/server/services/gifts/gift-service.ts +++ b/ghost/core/core/server/services/gifts/gift-service.ts @@ -9,6 +9,7 @@ import type { GiftEventBrowseOptions, GiftEventPage, GiftRepository, + RepositoryTransactionOptions, } from './gift-bookshelf-repository'; import type { GiftDeliveryDispatchResult, GiftDeliveryService } from './gift-delivery-service'; import type { SignedFlushScheduler } from '../../adapters/scheduling/signed-flush-scheduler'; @@ -175,7 +176,11 @@ interface GiftServiceDeps { giftRepository: GiftRepository; giftDeliveryService: Pick< GiftDeliveryService, - 'createForCheckout' | 'dispatchForGift' | 'cancelPendingForGift' | 'recoverPending' + | 'createForCheckout' + | 'getRecipientEmailForGift' + | 'dispatchForGift' + | 'cancelPendingForGift' + | 'recoverPending' >; memberRepository: MemberRepository; tiersService: TiersService; @@ -286,6 +291,7 @@ export interface GiftRedemption { amount: number; buyer_name: string | null; recipient_name: string | null; + recipient_email: string | null; message: string | null; expires_at: Date; consumes_at: Date | null; @@ -893,7 +899,7 @@ export class GiftService { transacting, newMember: input.newMember, }); - const redemption = await this.serializeRedemption(redeemed); + const redemption = await this.serializeRedemption(redeemed, { transacting }); return { redeemed, member, redemption }; }; @@ -1567,7 +1573,14 @@ export class GiftService { return true; } - private async serializeRedemption(gift: Gift): Promise { + private async serializeRedemption( + gift: Gift, + options: RepositoryTransactionOptions = {}, + ): Promise { + const recipientEmail = await this.deps.giftDeliveryService.getRecipientEmailForGift( + gift.token, + options, + ); const tier = await this.deps.tiersService.api.read(gift.tierId); if (!tier) { @@ -1586,6 +1599,7 @@ export class GiftService { amount: gift.amount, buyer_name: gift.buyerName, recipient_name: gift.recipientName, + recipient_email: recipientEmail, message: gift.personalMessage, expires_at: gift.expiresAt!, consumes_at: gift.consumesAt, diff --git a/ghost/core/test/e2e-api/members/gift-subscriptions.test.js b/ghost/core/test/e2e-api/members/gift-subscriptions.test.js index cfe13fe471e..35ed0b70e23 100644 --- a/ghost/core/test/e2e-api/members/gift-subscriptions.test.js +++ b/ghost/core/test/e2e-api/members/gift-subscriptions.test.js @@ -856,6 +856,12 @@ describe('Gift Subscriptions', function () { recipient_name: 'Taylor', personal_message: 'Enjoy!', }); + await models.GiftDelivery.add({ + gift_id: gift.id, + recipient_email: 'taylor@example.com', + status: 'failed', + outcome: 'permanent_failed', + }); const { body } = await membersAgent .get(`/api/gifts/${gift.get('token')}/redeem/`) @@ -869,6 +875,7 @@ describe('Gift Subscriptions', function () { assert.equal(body.gifts[0].amount, 5000); assert.equal(body.gifts[0].buyer_name, 'Jamie'); assert.equal(body.gifts[0].recipient_name, 'Taylor'); + assert.equal(body.gifts[0].recipient_email, 'taylor@example.com'); assert.equal(body.gifts[0].message, 'Enjoy!'); assert.equal(body.gifts[0].expires_at, new Date(gift.get('expires_at')).toISOString()); assert.deepEqual(body.gifts[0].tier, { @@ -881,7 +888,6 @@ describe('Gift Subscriptions', function () { .map((item) => item.name), }); assert.equal(body.gifts[0].buyer_email, undefined); - assert.equal(body.gifts[0].recipient_email, undefined); assert.equal(body.gifts[0].delivery_status, undefined); assert.equal(body.gifts[0].redeemed_at, undefined); assert.equal(body.gifts[0].status, undefined); @@ -897,6 +903,7 @@ describe('Gift Subscriptions', function () { const { body } = await agent.get(`/api/gifts/${gift.get('token')}/redeem/`).expectStatus(200); assert.equal(body.gifts[0].token, gift.get('token')); + assert.equal(body.gifts[0].recipient_email, null); }); it('returns 404 when the gift token does not exist', async function () { @@ -975,6 +982,11 @@ describe('Gift Subscriptions', function () { const agent = membersAgent.duplicate(); const email = `gift-post-free-${giftSequence + 1}@example.com`; const gift = await createGift(); + await models.GiftDelivery.add({ + gift_id: gift.id, + recipient_email: 'recipient@example.com', + status: 'pending', + }); await agent.loginAs(email); @@ -990,6 +1002,7 @@ describe('Gift Subscriptions', function () { const member = await models.Member.findOne({ email }, { require: true }); assert.equal(body.gifts[0].token, gift.get('token')); + assert.equal(body.gifts[0].recipient_email, 'recipient@example.com'); assert.equal(body.gifts[0].status, undefined); assert.ok(body.gifts[0].consumes_at); assert.equal(member.get('status'), 'gift'); diff --git a/ghost/core/test/integration/services/gifts/gift-delivery-bookshelf-repository.test.ts b/ghost/core/test/integration/services/gifts/gift-delivery-bookshelf-repository.test.ts index c68445dea3d..be4741dd030 100644 --- a/ghost/core/test/integration/services/gifts/gift-delivery-bookshelf-repository.test.ts +++ b/ghost/core/test/integration/services/gifts/gift-delivery-bookshelf-repository.test.ts @@ -117,6 +117,16 @@ describe('GiftDeliveryBookshelfRepository (integration)', function () { return { gift, delivery }; } + it('finds an email delivery by gift token', async function () { + const { gift, delivery } = await createPendingEmailGift({ deliveryStatus: 'failed' }); + + assert.equal( + (await deliveryRepository.getByGiftToken(gift.get('token')))?.recipientEmail, + delivery.get('recipient_email'), + ); + assert.equal(await deliveryRepository.getByGiftToken('missing-token'), null); + }); + it('allows exactly one concurrent caller to start a pending delivery', async function () { const startedAt = new Date(); startedAt.setMilliseconds(0); diff --git a/ghost/core/test/unit/server/services/gifts/gift-delivery-service.test.ts b/ghost/core/test/unit/server/services/gifts/gift-delivery-service.test.ts index 929f9227ef8..13e26e57432 100644 --- a/ghost/core/test/unit/server/services/gifts/gift-delivery-service.test.ts +++ b/ghost/core/test/unit/server/services/gifts/gift-delivery-service.test.ts @@ -50,6 +50,7 @@ describe('GiftDeliveryService', function () { giftDeliveryRepository = { getById: sinon.stub().resolves(null), getByGiftId: sinon.stub().resolves(null), + getByGiftToken: sinon.stub().resolves(null), getByProviderMessageId: sinon.stub().resolves(null), findRecoverableForPurchasedGifts: sinon.stub().resolves([]), findScheduledTimesForPurchasedGifts: sinon.stub().resolves([]), @@ -111,6 +112,31 @@ describe('GiftDeliveryService', function () { sinon.restore(); }); + it('returns the recipient email for a gift regardless of delivery state', async function () { + giftDeliveryRepository.getByGiftToken.resolves( + buildGiftDelivery({ + recipientEmail: 'recipient@example.com', + status: 'failed', + outcome: 'permanent_failed', + }), + ); + const service = createService(); + + assert.equal( + await service.getRecipientEmailForGift('gift-token', { transacting }), + 'recipient@example.com', + ); + sinon.assert.calledOnceWithExactly(giftDeliveryRepository.getByGiftToken, 'gift-token', { + transacting, + }); + }); + + it('returns null when a gift has no email delivery', async function () { + const service = createService(); + + assert.equal(await service.getRecipientEmailForGift('gift-token'), null); + }); + it('dispatches an immediate send for a pending delivery after purchase', async function () { giftDeliveryRepository.getByGiftId.resolves( buildGiftDelivery({ id: 'delivery_1', recipientEmail: 'recipient@example.com' }), diff --git a/ghost/core/test/unit/server/services/gifts/gift-service-interface.test.ts b/ghost/core/test/unit/server/services/gifts/gift-service-interface.test.ts index 248daeec8c4..a54ed0b59fa 100644 --- a/ghost/core/test/unit/server/services/gifts/gift-service-interface.test.ts +++ b/ghost/core/test/unit/server/services/gifts/gift-service-interface.test.ts @@ -65,6 +65,7 @@ describe('GiftService interface', function () { }; const giftDeliveryService = { createForCheckout: sinon.stub().resolves(), + getRecipientEmailForGift: sinon.stub().resolves(null), dispatchForGift: sinon.stub().resolves(null), cancelPendingForGift: sinon.stub().resolves(false), }; @@ -652,6 +653,7 @@ describe('GiftService interface', function () { amount: 5000, buyer_name: 'Jamie', recipient_name: 'Taylor', + recipient_email: null, message: 'Enjoy!', expires_at: new Date('2030-01-01T00:00:00.000Z'), consumes_at: null, diff --git a/ghost/core/test/unit/server/services/gifts/gift-service.test.ts b/ghost/core/test/unit/server/services/gifts/gift-service.test.ts index 8a2cbe3e804..f48472391ba 100644 --- a/ghost/core/test/unit/server/services/gifts/gift-service.test.ts +++ b/ghost/core/test/unit/server/services/gifts/gift-service.test.ts @@ -87,6 +87,7 @@ describe('GiftService', function () { let giftRepository: GiftRepositoryStub; let giftDeliveryService: { createForCheckout: sinon.SinonStub; + getRecipientEmailForGift: sinon.SinonStub; dispatchForGift: sinon.SinonStub; cancelPendingForGift: sinon.SinonStub; recoverPending: sinon.SinonStub; @@ -177,6 +178,7 @@ describe('GiftService', function () { }; giftDeliveryService = { createForCheckout: sinon.stub().resolves(undefined), + getRecipientEmailForGift: sinon.stub().resolves(null), dispatchForGift: sinon.stub().resolves(null), cancelPendingForGift: sinon.stub().resolves(false), recoverPending: sinon.stub().resolves({ sentCount: 0, skippedCount: 0, failedCount: 0 }), @@ -838,11 +840,18 @@ describe('GiftService', function () { const service = createService(); giftRepository.getByToken.resolves(gift); + giftDeliveryService.getRecipientEmailForGift.resolves('recipient@example.com'); const result = await service.getRedeemable({ token: 'gift-token', memberStatus: 'free' }); sinon.assert.calledOnceWithExactly(giftRepository.getByToken, 'gift-token'); + sinon.assert.calledOnceWithExactly( + giftDeliveryService.getRecipientEmailForGift, + 'gift-token', + {}, + ); assert.equal(result.token, gift.token); + assert.equal(result.recipient_email, 'recipient@example.com'); assert.equal('buyerEmail' in result, false); }); @@ -1668,6 +1677,7 @@ describe('GiftService', function () { memberGet.withArgs('email').returns('member@example.com'); giftRepository.getByToken.resolves(gift); + giftDeliveryService.getRecipientEmailForGift.resolves('recipient@example.com'); memberRepository.get.resolves({ id: 'member_1', get: memberGet, @@ -1707,6 +1717,11 @@ describe('GiftService', function () { sinon.assert.calledOnceWithExactly(giftDeliveryService.cancelPendingForGift, redeemed.token, { transacting, }); + sinon.assert.calledOnceWithExactly( + giftDeliveryService.getRecipientEmailForGift, + redeemed.token, + { transacting }, + ); sinon.assert.calledTwice(tiersService.api.read); sinon.assert.alwaysCalledWithExactly(tiersService.api.read, 'tier_1'); sinon.assert.calledOnceWithExactly(staffServiceEmails.notifyGiftSubscriptionStarted, { @@ -1722,6 +1737,7 @@ describe('GiftService', function () { assert.equal(redeemed.redeemerMemberId, 'member_1'); assert.notEqual(redeemed.consumesAt, null); assert.equal(redemption.token, 'gift-token'); + assert.equal(redemption.recipient_email, 'recipient@example.com'); assert.deepEqual(redemption.consumes_at, redeemed.consumesAt); }); From 493f590e689cc54d9c28c2d742e5148090c00bb7 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Wed, 26 Aug 2026 13:18:51 -0500 Subject: [PATCH 2/6] Fixed the format-number skill's import example (#30325) no ref --- .agents/skills/format-number/SKILL.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.agents/skills/format-number/SKILL.md b/.agents/skills/format-number/SKILL.md index 81c86358f8c..ab94c9db5de 100644 --- a/.agents/skills/format-number/SKILL.md +++ b/.agents/skills/format-number/SKILL.md @@ -7,12 +7,12 @@ autoTrigger: # Format Numbers -When editing `.tsx` files, ensure all user-facing numbers are formatted using the `formatNumber` utility from `@tryghost/shade`. +When editing `.tsx` files, ensure all user-facing numbers are formatted using the `formatNumber` utility from `@tryghost/shade/utils`. ## Import ```typescript -import {formatNumber} from '@tryghost/shade'; +import { formatNumber } from '@tryghost/shade/utils'; ``` ## When to use formatNumber @@ -58,4 +58,4 @@ Do NOT use any of these patterns for formatting numbers in TSX files: - `abbreviateNumber()` - for compact notation (e.g., 1.2M, 50k) - `centsToDollars()` - convert cents to dollars before passing to `formatNumber` -All are imported from `@tryghost/shade`. +All are imported from `@tryghost/shade/utils`. From 5e460e95ac9871aaa6d5ce0a1c98a42ee2d1d3c9 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Wed, 26 Aug 2026 13:29:54 -0500 Subject: [PATCH 3/6] Moved UI-only e2e cases to the acceptance tier (#30316) no erf Moved various admin UI tests from e2e into the admin acceptance tier. --- .../analytics/analytics.acceptance.test.tsx | 175 +++++++++++++++ apps/admin/src/analytics/analytics.screen.ts | 17 ++ apps/admin/src/layout/sidebar.screen.ts | 4 +- .../post-analytics.acceptance.test.tsx | 117 ++++++++++ .../posts/analytics/post-analytics.screen.ts | 12 +- .../advanced/integrations.acceptance.test.tsx | 42 +++- .../detail/tag-detail.acceptance.test.tsx | 32 +++ .../src/tags/detail/tag-detail.screen.ts | 2 + .../admin/analytics/post-analytics/index.ts | 1 - .../post-analytics-growth-page.ts | 18 -- .../admin/members/member-details-page.ts | 29 ++- .../admin/settings/sections/access-section.ts | 11 +- .../sections/announcement-bar-section.ts | 5 +- .../sections/custom-fields-section.ts | 11 +- .../settings/sections/danger-zone-section.ts | 5 +- .../admin/settings/sections/design-section.ts | 20 +- .../settings/sections/integration-modal.ts | 3 +- .../settings/sections/integrations-section.ts | 3 +- .../sections/member-welcome-emails-section.ts | 42 ++-- .../admin/settings/sections/portal-section.ts | 5 +- .../admin/settings/sections/staff-section.ts | 7 +- .../admin/settings/sections/tiers-section.ts | 29 ++- .../pages/admin/settings/settings-page.ts | 3 +- .../admin/settings/staff-details-page.ts | 17 +- e2e/tests/admin/analytics/growth.test.ts | 44 +--- e2e/tests/admin/analytics/newsletters.test.ts | 22 -- e2e/tests/admin/analytics/overview.test.ts | 50 +---- .../analytics/post-analytics/growth.test.ts | 49 ----- .../analytics/post-analytics/overview.test.ts | 56 +---- e2e/tests/admin/analytics/web-traffic.test.ts | 40 ---- e2e/tests/admin/settings/integrations.test.ts | 200 +----------------- e2e/tests/admin/tags/editor.test.ts | 37 +--- .../test-data/src/selectors/members.ts | 7 + .../test-data/src/selectors/settings.ts | 17 ++ 34 files changed, 573 insertions(+), 559 deletions(-) delete mode 100644 e2e/helpers/pages/admin/analytics/post-analytics/post-analytics-growth-page.ts delete mode 100644 e2e/tests/admin/analytics/post-analytics/growth.test.ts delete mode 100644 e2e/tests/admin/analytics/web-traffic.test.ts diff --git a/apps/admin/src/analytics/analytics.acceptance.test.tsx b/apps/admin/src/analytics/analytics.acceptance.test.tsx index 7e3ae2d8738..6be77376850 100644 --- a/apps/admin/src/analytics/analytics.acceptance.test.tsx +++ b/apps/admin/src/analytics/analytics.acceptance.test.tsx @@ -3,6 +3,7 @@ import { describe, expect, it } from 'vitest'; import { TINYBIRD_SITE_UUID, currentRoute, + fakeAdminEndpoint, fakeAdminStats, fakeAnalyticsOverview, fakeNewsletters, @@ -88,6 +89,40 @@ function seedTopPostsViews() { ]); } +/** + * The stats views' world with web analytics on but nothing recorded yet: a + * site with no members, no MRR, no posts and no visits — the empty states. + */ +function seedEmptyAnalyticsWorld() { + fakeAdminStats.memberCount(); + fakeAdminStats.mrr(); + fakeAdminStats.subscriptions(); + fakeAdminStats.topPostViews(); + fakePosts([]); + fakeTinybirdToken(); + fakeTinybirdPipe('api_active_visitors', []); + fakeTinybirdPipe('api_kpis', []); +} + +/** The web traffic view's own cards, all empty. */ +function seedEmptyWebTraffic() { + fakeAdminStats.topContent([]); + fakeTinybirdPipe('api_top_sources', []); + fakeTinybirdPipe('api_top_locations', []); +} + +/** + * A member-count history of zero rows, the shape the server serves for a + * site with no members — a fully empty history would collapse the growth + * view into its no-stats state instead of rendering the empty cards. + */ +function seedZeroMemberHistory() { + fakeAdminStats.memberCount({ + stats: [{ date: daysAgo(1), free: 0, paid: 0, comped: 0 }], + totals: { free: 0, paid: 0, comped: 0, gift: 0 }, + }); +} + describe('Analytics overview', () => { it('renders zero KPIs when growth history is empty', async () => { fakeAnalyticsOverview(); @@ -138,6 +173,66 @@ describe('Analytics overview', () => { await expect.element(analyticsScreen.dateRangeSelect()).toHaveTextContent('Last 7 days'); await expect.poll(() => kpisApi.lastRequest?.params.get('date_from')).not.toBe(initialDateFrom); }); + + it('renders the latest and top posts with zeroed stats when nothing is recorded', async () => { + seedEmptyAnalyticsWorld(); + fakePosts([ + post({ + id: LATEST_POST_ID, + title: 'Attack of the Clones', + status: 'published', + published_at: `${daysAgo(3)}T10:00:00.000Z`, + url: 'https://example.com/attack-of-the-clones/', + }), + ]); + fakeAdminStats.post(LATEST_POST_ID); + // The server lists recently published posts in top posts even before + // they record any views; the row must render its zeros as zeros. + fakeAdminStats.topPostViews([ + { + post_id: LATEST_POST_ID, + title: 'Attack of the Clones', + published_at: `${daysAgo(3)}T10:00:00.000Z`, + }, + ]); + await renderAdminApp('/analytics', { boot: webAnalyticsBootOverrides() }); + + await expect.element(analyticsScreen.latestPost()).toHaveTextContent('Attack of the Clones'); + await expect.element(analyticsScreen.latestPostVisitors()).toHaveTextContent('0'); + await expect.element(analyticsScreen.latestPostMembers()).toHaveTextContent('0'); + + await expect.element(analyticsScreen.topPostsCard()).toHaveTextContent('Attack of the Clones'); + const visitorsStatistics = analyticsScreen.topPostsVisitorsStatistics(); + await expect.element(visitorsStatistics).toHaveTextContent('Unique visitors'); + await expect.element(visitorsStatistics).toHaveTextContent('0'); + const membersStatistics = analyticsScreen.topPostsMembersStatistics(); + await expect.element(membersStatistics).toHaveTextContent('New members'); + await expect.element(membersStatistics).toHaveTextContent('Free'); + await expect.element(membersStatistics).toHaveTextContent('0'); + }); + + it('navigates to the web traffic view from the visitors KPI', async () => { + seedEmptyAnalyticsWorld(); + seedEmptyWebTraffic(); + await renderAdminApp('/analytics', { boot: webAnalyticsBootOverrides() }); + + await analyticsScreen.uniqueVisitorsViewMoreButton().click(); + + await expect.poll(currentRoute).toMatch(/^\/analytics\/web\/?$/); + await expect.element(analyticsScreen.uniqueVisitorsTab()).toBeVisible(); + }); + + it('navigates to the growth view from the members KPI', async () => { + seedEmptyAnalyticsWorld(); + seedZeroMemberHistory(); + fakeAdminStats.topPosts(); + await renderAdminApp('/analytics', { boot: webAnalyticsBootOverrides() }); + + await analyticsScreen.membersViewMoreButton().click(); + + await expect.poll(currentRoute).toMatch(/^\/analytics\/growth\//); + await expect.element(analyticsScreen.totalMembersCard()).toBeVisible(); + }); }); describe('Analytics web traffic', () => { @@ -184,6 +279,30 @@ describe('Analytics web traffic', () => { await expect.element(analyticsScreen.sourceRow('google.com')).toHaveTextContent('170'); await expect.element(analyticsScreen.locationRow('US')).toHaveTextContent('United States'); }); + + it('renders zeroed KPIs and empty cards when there are no visits', async () => { + seedEmptyAnalyticsWorld(); + seedEmptyWebTraffic(); + await renderAdminApp('/analytics/web', { boot: webAnalyticsBootOverrides() }); + + await expect.element(analyticsScreen.uniqueVisitorsTab()).toHaveTextContent('0'); + await expect.element(analyticsScreen.totalViewsTab()).toHaveTextContent('0'); + await expect.element(analyticsScreen.topContentCard()).toHaveTextContent('No visitors'); + await expect.element(analyticsScreen.topSourcesCard()).toHaveTextContent('No visitors'); + await expect.element(analyticsScreen.locationsCard()).toHaveTextContent('No visitors'); + }); + + it('keeps the empty state across the top content tabs', async () => { + seedEmptyAnalyticsWorld(); + seedEmptyWebTraffic(); + await renderAdminApp('/analytics/web', { boot: webAnalyticsBootOverrides() }); + + await analyticsScreen.topContentTab('Posts').click(); + await expect.element(analyticsScreen.topContentCard()).toHaveTextContent('No visitors'); + + await analyticsScreen.topContentTab('Pages').click(); + await expect.element(analyticsScreen.topContentCard()).toHaveTextContent('No visitors'); + }); }); describe('Analytics growth', () => { @@ -212,6 +331,38 @@ describe('Analytics growth', () => { .toHaveTextContent('Attack of the Clones'); await expect.element(analyticsScreen.topContentCard()).toHaveTextContent('+30'); }); + + it('shows No conversions across the top content tabs when nothing converted', async () => { + seedEmptyAnalyticsWorld(); + seedZeroMemberHistory(); + fakeAdminStats.topPosts(); + fakeAdminEndpoint('GET', /^\/stats\/top-sources-growth/, { stats: [], meta: {} }); + await renderAdminApp('/analytics/growth', { boot: webAnalyticsBootOverrides() }); + + const contentCard = analyticsScreen.topContentCard(); + await expect + .element(contentCard) + .toHaveTextContent('Which posts or pages drove the most growth in the last 30 days'); + await expect.element(contentCard).toHaveTextContent('No conversions'); + + await analyticsScreen.topContentTab('Posts').click(); + await expect + .element(contentCard) + .toHaveTextContent('Which posts drove the most growth in the last 30 days'); + await expect.element(contentCard).toHaveTextContent('No conversions'); + + await analyticsScreen.topContentTab('Pages').click(); + await expect + .element(contentCard) + .toHaveTextContent('Which pages drove the most growth in the last 30 days'); + await expect.element(contentCard).toHaveTextContent('No conversions'); + + await analyticsScreen.topContentTab('Sources').click(); + await expect + .element(contentCard) + .toHaveTextContent('Which sources drove the most growth in the last 30 days'); + await expect.element(contentCard).toHaveTextContent('No conversions'); + }); }); describe('Analytics newsletters', () => { @@ -255,4 +406,28 @@ describe('Analytics newsletters', () => { .element(analyticsScreen.topNewslettersCard()) .toHaveTextContent('Weekly Digest Issue #1'); }); + + it('shows the empty state on every newsletter card when none were sent', async () => { + seedEmptyAnalyticsWorld(); + fakeNewsletters([newsletter({ name: 'Weekly Digest', status: 'active', sort_order: 0 })]); + fakeAdminStats.newsletterSubscribers(); + fakeAdminStats.newsletterBasic(); + fakeAdminStats.newsletterClicks(); + await renderAdminApp('/analytics/newsletters', { boot: webAnalyticsBootOverrides() }); + + await expect.element(analyticsScreen.newslettersCard()).toBeVisible(); + await expect + .element(analyticsScreen.topNewslettersCard()) + .toHaveTextContent('newsletters in the last 30 days'); + + await analyticsScreen.newslettersCardTab('Avg. open rate').click(); + await expect + .element(analyticsScreen.newslettersCard()) + .toHaveTextContent('No newsletters in the last 30 days'); + + await analyticsScreen.newslettersCardTab('Avg. click rate').click(); + await expect + .element(analyticsScreen.newslettersCard()) + .toHaveTextContent('No newsletters in the last 30 days'); + }); }); diff --git a/apps/admin/src/analytics/analytics.screen.ts b/apps/admin/src/analytics/analytics.screen.ts index 4105c804370..32e7734ea16 100644 --- a/apps/admin/src/analytics/analytics.screen.ts +++ b/apps/admin/src/analytics/analytics.screen.ts @@ -9,13 +9,28 @@ export const analyticsScreen = { page.getByTestId(sel.uniqueVisitors).getByTestId(sel.kpiCardHeaderValue), membersCard: () => page.getByTestId(sel.members), membersValue: () => page.getByTestId(sel.members).getByTestId(sel.kpiCardHeaderValue), + membersViewMoreButton: () => + page.getByTestId(sel.members).getByRole('button', { name: 'View more' }), mrrValue: () => page.getByTestId(sel.mrr).getByTestId(sel.kpiCardHeaderValue), + uniqueVisitorsViewMoreButton: () => + page.getByTestId(sel.uniqueVisitors).getByRole('button', { name: 'View more' }), latestPost: () => page.getByTestId(sel.latestPost), + latestPostVisitors: () => page.getByTestId(sel.latestPost).getByTestId(sel.latestPostVisitors), + latestPostMembers: () => page.getByTestId(sel.latestPost).getByTestId(sel.latestPostMembers), topPostsCard: () => page.getByTestId(sel.topPostsCard), + topPostsVisitorsStatistics: () => + page.getByTestId(sel.topPostsCard).getByTestId(sel.statisticsVisitors), + topPostsMembersStatistics: () => + page.getByTestId(sel.topPostsCard).getByTestId(sel.statisticsMembers), // Web webGraph: () => page.getByTestId(sel.webGraph), + uniqueVisitorsTab: () => + page.getByTestId(sel.webGraph).getByRole('tab', { name: 'Unique visitors' }), + totalViewsTab: () => page.getByTestId(sel.webGraph).getByRole('tab', { name: 'Total views' }), topContentCard: () => page.getByTestId(sel.topContentCard), + topContentTab: (name: string) => + page.getByTestId(sel.topContentCard).getByRole('tab', { name, exact: true }), topSourcesCard: () => page.getByTestId(sel.topSourcesCard), sourceRow: (source: string) => page.getByTestId(`${sel.sourceRowPrefix}${source}`), locationsCard: () => page.getByTestId(sel.visitorsCard), @@ -26,6 +41,8 @@ export const analyticsScreen = { // Newsletters newslettersCard: () => page.getByTestId(sel.newslettersCard), + newslettersCardTab: (name: string) => + page.getByTestId(sel.newslettersCard).getByRole('tab', { name }), totalSubscribersValue: () => page.getByTestId(sel.totalSubscribersValue), topNewslettersCard: () => page.getByTestId(sel.topNewslettersCard), diff --git a/apps/admin/src/layout/sidebar.screen.ts b/apps/admin/src/layout/sidebar.screen.ts index 3dfec8f9b89..9abf21e5922 100644 --- a/apps/admin/src/layout/sidebar.screen.ts +++ b/apps/admin/src/layout/sidebar.screen.ts @@ -26,7 +26,9 @@ export const sidebarScreen = { shellNav: () => page.getByTestId(adminSidebar), /** The shell's content area, rendered by AdminLayout alongside the sidebar. */ shellMain: () => page.getByRole('main').first(), - navLink: (name: string) => page.getByRole('navigation').getByRole('link', { name, exact: true }), + // Scoped to the shell sidebar: in-content navigation (e.g. the tag + // detail breadcrumb) repeats the same link names. + navLink: (name: string) => sidebarScreen.shellNav().getByRole('link', { name, exact: true }), postsToggle: () => page.getByRole('button', { name: postsToggle }), networkBadge: () => page.getByTestId(networkNotificationBadge), userMenuTrigger: () => page.getByRole('button', { name: userMenuTrigger }), 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 e3fbcba54ff..5853b681935 100644 --- a/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx +++ b/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx @@ -5,13 +5,16 @@ import { currentRoute, fakeAdminStats, fakeAdminEndpoint, + fakeMembers, fakePosts, fakeTinybirdPipe, fakeTinybirdToken, post, renderAdminApp, + settingsResponse, webAnalyticsBootOverrides, } from '@test-utils/acceptance'; +import { membersScreen } from '@/members/members.screen'; import { postAnalyticsScreen } from './post-analytics.screen'; const POST_ID = '64d623b64676110001e897d9'; @@ -94,6 +97,35 @@ function seedPostAnalyticsWorld() { }; } +/** + * The world for a freshly published post with no activity at all: no visits, + * no attributed members, no link clicks — the empty states every tab shows. + * The post never went out as an email, so no newsletter endpoints fire. + */ +function seedEmptyPostAnalyticsWorld() { + fakePosts([ + post({ + id: POST_ID, + uuid: POST_UUID, + title: 'Attack of the Clones', + slug: 'attack-of-the-clones', + status: 'published', + visibility: 'public', + published_at: `${daysAgo(1)}T10:00:00.000Z`, + url: 'https://example.com/attack-of-the-clones/', + }), + ]); + fakeAdminStats.postReferrers(POST_ID); + fakeAdminStats.postGrowth(POST_ID); + fakeAdminStats.mrr(); + fakeAdminEndpoint('GET', /^\/links\//, { links: [], meta: {} }); + fakeTinybirdToken(); + fakeTinybirdPipe('api_active_visitors', []); + fakeTinybirdPipe('api_kpis', []); + fakeTinybirdPipe('api_top_sources', []); + fakeTinybirdPipe('api_top_locations', []); +} + describe('Post analytics overview', () => { it('renders the seeded post with web and growth sections', async () => { const { postsApi } = seedPostAnalyticsWorld(); @@ -128,6 +160,57 @@ describe('Post analytics overview', () => { await expect.poll(() => kpisApi.requests.length).toBeGreaterThan(overviewKpiRequestCount); await expect.poll(() => kpisApi.lastRequest?.params.get('post_uuid')).toBe(POST_UUID); }); + + it('renders every tab and zeroed sections for a post with no activity', async () => { + seedEmptyPostAnalyticsWorld(); + await renderAdminApp(`/posts/analytics/${POST_ID}`, { boot: webAnalyticsBootOverrides() }); + + await expect.element(postAnalyticsScreen.overviewTab()).toBeVisible(); + await expect.element(postAnalyticsScreen.webTrafficTab()).toBeVisible(); + await expect.element(postAnalyticsScreen.growthTab()).toBeVisible(); + + await expect.element(postAnalyticsScreen.growthCard()).toHaveTextContent('Free members'); + await expect.element(postAnalyticsScreen.growthCard()).toHaveTextContent('0'); + }); + + it('reaches the empty web traffic view through web performance view more', async () => { + seedEmptyPostAnalyticsWorld(); + await renderAdminApp(`/posts/analytics/${POST_ID}`, { boot: webAnalyticsBootOverrides() }); + + await postAnalyticsScreen.webPerformanceViewMoreButton().click(); + + await expect.poll(currentRoute).toBe(`/posts/analytics/${POST_ID}/web`); + // No visits at all: the web view renders its whole-view empty state. + await expect.element(page.getByText('No visitors in the last 30 days').first()).toBeVisible(); + }); + + it('reaches the empty growth view through growth view more', async () => { + seedEmptyPostAnalyticsWorld(); + await renderAdminApp(`/posts/analytics/${POST_ID}`, { boot: webAnalyticsBootOverrides() }); + + await postAnalyticsScreen.growthViewMoreButton().click(); + + await expect.poll(currentRoute).toBe(`/posts/analytics/${POST_ID}/growth`); + await expect + .element(postAnalyticsScreen.topSourcesCard()) + .toHaveTextContent('No sources data available'); + }); + + it('hides the growth tab and section when member source tracking is off', async () => { + seedEmptyPostAnalyticsWorld(); + const boot = webAnalyticsBootOverrides(); + boot.browseSettings = { + response: settingsResponse({ + settings: { web_analytics_enabled: true, members_track_sources: false }, + }), + }; + await renderAdminApp(`/posts/analytics/${POST_ID}`, { boot }); + + await expect.element(postAnalyticsScreen.overviewTab()).toBeVisible(); + await expect.element(postAnalyticsScreen.webTrafficTab()).toBeVisible(); + await expect.element(postAnalyticsScreen.growthTab()).not.toBeInTheDocument(); + await expect.element(postAnalyticsScreen.growthCard()).not.toBeInTheDocument(); + }); }); describe('Post analytics web', () => { @@ -183,6 +266,40 @@ describe('Post analytics growth', () => { await expect.element(page.getByText('Top sources')).toBeVisible(); await expect.element(page.getByText('Google')).toBeVisible(); }); + + it('renders the zeroed members card and empty sources when nothing converted', async () => { + seedEmptyPostAnalyticsWorld(); + await renderAdminApp(`/posts/analytics/${POST_ID}/growth`, { + boot: webAnalyticsBootOverrides(), + }); + + await expect.element(postAnalyticsScreen.membersCard()).toHaveTextContent('Free members'); + await expect.element(postAnalyticsScreen.membersCard()).toHaveTextContent('0'); + await expect + .element(postAnalyticsScreen.topSourcesCard()) + .toHaveTextContent('No sources data available'); + }); + + it('links the members KPI to the members list filtered to this post', async () => { + seedEmptyPostAnalyticsWorld(); + const membersApi = fakeMembers([]); + // The filter bar resolves attribution ids to post/page titles. + fakeAdminEndpoint('GET', /^\/pages\//, { + pages: [], + meta: { pagination: { page: 1, limit: 25, pages: 1, total: 0, next: null, prev: null } }, + }); + await renderAdminApp(`/posts/analytics/${POST_ID}/growth`, { + boot: webAnalyticsBootOverrides(), + }); + + await expect.element(postAnalyticsScreen.membersCard()).toHaveTextContent('Free members'); + await postAnalyticsScreen.freeMembersViewMembersButton().click(); + + // The members screen re-serializes the handed-over filter clauses. + await expect.poll(currentRoute).toMatch(/^\/members\?/); + await expect(membersApi).toHaveSentFilter(`conversion:-'${POST_ID}'+signup:'${POST_ID}'`); + await expect.element(membersScreen.noResults()).toBeVisible(); + }); }); describe('Post analytics newsletter', () => { diff --git a/apps/admin/src/posts/analytics/post-analytics.screen.ts b/apps/admin/src/posts/analytics/post-analytics.screen.ts index b76451824bc..82583f607b0 100644 --- a/apps/admin/src/posts/analytics/post-analytics.screen.ts +++ b/apps/admin/src/posts/analytics/post-analytics.screen.ts @@ -13,8 +13,12 @@ export const postAnalyticsScreen = { // Overview webPerformanceCard: () => page.getByTestId(sel.webPerformance), + webPerformanceViewMoreButton: () => + page.getByTestId(sel.webPerformance).getByRole('button', { name: 'View more' }), uniqueVisitors: () => page.getByTestId(sel.uniqueVisitors), growthCard: () => page.getByTestId(sel.growth), + growthViewMoreButton: () => + page.getByTestId(sel.growth).getByRole('button', { name: 'View more' }), // Web — the post view's row testids carry the lowercased country code and // the source with non-alphanumerics dashed ("google.com" → "google-com"). @@ -26,6 +30,12 @@ export const postAnalyticsScreen = { page.getByTestId(`${sel.sourceRowPrefix}${source.toLowerCase().replace(/[^a-z0-9]/g, '-')}`), filterContainer: () => page.getByTestId(sel.statsFilterContainer), - // Growth + // Growth — every KPI in the members card carries its own "View members" + // button; the free-members KPI renders first. membersCard: () => page.getByTestId(sel.membersCard), + freeMembersViewMembersButton: () => + page + .getByTestId(sel.membersCard) + .getByRole('button', { name: /View members/ }) + .first(), }; diff --git a/apps/admin/src/settings/advanced/integrations.acceptance.test.tsx b/apps/admin/src/settings/advanced/integrations.acceptance.test.tsx index b59f0e7f0cb..717dc4f03d2 100644 --- a/apps/admin/src/settings/advanced/integrations.acceptance.test.tsx +++ b/apps/admin/src/settings/advanced/integrations.acceptance.test.tsx @@ -51,7 +51,6 @@ const customIntegrationsLimit = { function limitedConfig(upgradeUrl?: string) { const response = configResponse(); - response.config.labs = { ...response.config.labs, transistor: true }; response.config.hostSettings = upgradeUrl ? { limits: customIntegrationsLimit, billing: { upgradeUrl } } : { limits: customIntegrationsLimit }; @@ -283,12 +282,17 @@ describe('Advanced integrations', () => { await modal.getByRole('switch').click(); await modal.getByRole('button', { name: 'Save' }).click(); await expect(settingsApi).toHaveEditedSettings([{ key: 'pintura', value: true }]); + + await modal.getByRole('switch').click(); + await modal.getByRole('button', { name: 'Save' }).click(); + await expect(settingsApi).toHaveEditedSettings([{ key: 'pintura', value: false }]); + expect(settingsApi.requests).toHaveLength(2); }); it('shows the Active badge after enabling the Transistor integration', async () => { fakeSettingsScreens(); const settingsApi = fakeEditSettings(); - await renderAdminApp('/settings/integrations', { labs: { transistor: true } }); + await renderAdminApp('/settings/integrations'); const item = settingsScreen.section('integrations').getByTestId('transistor-integration'); const badge = item.getByText('Active', { exact: true }); @@ -297,6 +301,7 @@ describe('Advanced integrations', () => { await item.hover(); await item.getByRole('button', { name: 'Configure' }).click(); const modal = settingsScreen.section('transistor-modal'); + await expect.element(modal.getByRole('switch')).toBeVisible(); await modal.getByRole('switch').click(); await modal.getByRole('button', { name: 'Save' }).click(); await expect(settingsApi).toHaveEditedSettings([{ key: 'transistor', value: true }]); @@ -307,6 +312,38 @@ describe('Advanced integrations', () => { await expect.element(badge).toBeVisible(); }); + it('clears the Active badge after disabling the Transistor integration', async () => { + fakeSettingsScreens(); + const settingsApi = fakeEditSettings(); + await renderAdminApp('/settings/integrations'); + + const item = settingsScreen.section('integrations').getByTestId('transistor-integration'); + const badge = item.getByText('Active', { exact: true }); + await item.hover(); + await item.getByRole('button', { name: 'Configure' }).click(); + const modal = settingsScreen.section('transistor-modal'); + await modal.getByRole('switch').click(); + await modal.getByRole('button', { name: 'Save' }).click(); + await expect(settingsApi).toHaveEditedSettings([{ key: 'transistor', value: true }]); + await expect(badge).toHaveCount(1); + + await modal.getByRole('switch').click(); + await modal.getByRole('button', { name: 'Save' }).click(); + await expect(settingsApi).toHaveEditedSettings([{ key: 'transistor', value: false }]); + await expect(badge).toHaveCount(0); + expect(settingsApi.requests).toHaveLength(2); + }); + + it('shows the Transistor integration in the built-in list', async () => { + // The transistor labs flag was removed (#27082); the card is not gated. + fakeSettingsScreens(); + await renderAdminApp('/settings/integrations'); + + const section = settingsScreen.section('integrations'); + await expect.element(section.getByTestId('unsplash-integration')).toBeVisible(); + await expect.element(section.getByTestId('transistor-integration')).toBeVisible(); + }); + it('shows the upgrade CTA on host-limited integration cards', async () => { fakeSettingsScreens(); await renderAdminApp('/settings/integrations', { @@ -502,7 +539,6 @@ describe('Advanced integrations', () => { it('moves host-limited integrations to the bottom without disturbing relative order', async () => { fakeSettingsScreens(); await renderAdminApp('/settings/integrations', { - labs: { transistor: true }, boot: { browseConfig: { response: limitedConfig() } }, }); diff --git a/apps/admin/src/tags/detail/tag-detail.acceptance.test.tsx b/apps/admin/src/tags/detail/tag-detail.acceptance.test.tsx index 291b0d8341e..9e600ceeebb 100644 --- a/apps/admin/src/tags/detail/tag-detail.acceptance.test.tsx +++ b/apps/admin/src/tags/detail/tag-detail.acceptance.test.tsx @@ -619,6 +619,38 @@ describe('Tag detail (tagDetailsReact on)', () => { .toBeVisible(); }); + it('shows the not-found page when the tag does not exist', async () => { + fakeAdminEndpoint( + 'GET', + new RegExp('^/tags/slug/unknown/'), + { errors: [{ type: 'NotFoundError', message: 'Tag not found.' }] }, + { status: 404 }, + ); + await renderAdminApp('/tags/unknown', FLAGS); + + await expect.element(tagDetailScreen.notFound()).toBeVisible(); + }); + + it('keeps the Tags nav item active on the new tag route', async () => { + fakeTags([]); + await renderAdminApp('/tags/new', FLAGS); + + await expect.element(tagDetailScreen.title()).toHaveTextContent('New tag'); + await expect.element(sidebarScreen.navLink('Tags')).toHaveAttribute('aria-current', 'page'); + }); + + it('keeps the Tags nav item active while editing a tag from the list', async () => { + const t = tag({ name: 'News', slug: 'news' }); + fakeTags([t]); + fakeTagWorld(t); + await renderAdminApp('/tags', FLAGS); + + await tagsScreen.tagRows().getByRole('link', { name: 'News' }).click(); + + await expect.poll(currentRoute).toBe('/tags/news'); + await expect.element(sidebarScreen.navLink('Tags')).toHaveAttribute('aria-current', 'page'); + }); + it('guards leaving with unsaved edits via the breadcrumb', async () => { const t = tag({ name: 'News', slug: 'news' }); fakeTags([t]); diff --git a/apps/admin/src/tags/detail/tag-detail.screen.ts b/apps/admin/src/tags/detail/tag-detail.screen.ts index adb04d8b5d4..64e16c6160b 100644 --- a/apps/admin/src/tags/detail/tag-detail.screen.ts +++ b/apps/admin/src/tags/detail/tag-detail.screen.ts @@ -46,6 +46,8 @@ export const tagDetailScreen = { footerEditor: () => page.getByRole('textbox', { name: new RegExp(`^${sel.tagFooterEditorLabel}`) }), + notFound: () => page.getByText('Page not found', { exact: true }), + actionsButton: () => page.getByRole('button', { name: sel.tagActionsButton }), viewPostsMenuItem: () => page.getByRole('menuitem', { name: sel.viewPostsMenuItem }), deleteTagMenuItem: () => page.getByRole('menuitem', { name: sel.deleteTagMenuItem, exact: true }), diff --git a/e2e/helpers/pages/admin/analytics/post-analytics/index.ts b/e2e/helpers/pages/admin/analytics/post-analytics/index.ts index 3265723594c..75e11cee8e7 100644 --- a/e2e/helpers/pages/admin/analytics/post-analytics/index.ts +++ b/e2e/helpers/pages/admin/analytics/post-analytics/index.ts @@ -1,4 +1,3 @@ export * from './post-analytics-page'; -export * from './post-analytics-growth-page'; export * from './post-analytics-overview-page'; export * from './post-analytics-web-traffic-page'; diff --git a/e2e/helpers/pages/admin/analytics/post-analytics/post-analytics-growth-page.ts b/e2e/helpers/pages/admin/analytics/post-analytics/post-analytics-growth-page.ts deleted file mode 100644 index a1e4cb6b425..00000000000 --- a/e2e/helpers/pages/admin/analytics/post-analytics/post-analytics-growth-page.ts +++ /dev/null @@ -1,18 +0,0 @@ -import * as postAnalyticsSel from '@tryghost/test-data/selectors/post-analytics'; -import { AdminPage } from '@/admin-pages'; -import { Locator, Page } from '@playwright/test'; - -export class PostAnalyticsGrowthPage extends AdminPage { - readonly membersCard: Locator; - readonly viewMemberButton: Locator; - readonly topSourcesCard: Locator; - - constructor(page: Page) { - super(page); - - this.membersCard = this.page.getByTestId(postAnalyticsSel.membersCard); - this.viewMemberButton = this.membersCard.getByRole('button', { name: 'View member' }); - - this.topSourcesCard = this.page.getByTestId(postAnalyticsSel.topSourcesCard); - } -} diff --git a/e2e/helpers/pages/admin/members/member-details-page.ts b/e2e/helpers/pages/admin/members/member-details-page.ts index cd506d739fa..7cf368f231d 100644 --- a/e2e/helpers/pages/admin/members/member-details-page.ts +++ b/e2e/helpers/pages/admin/members/member-details-page.ts @@ -1,6 +1,17 @@ import { AdminPage } from '@/admin-pages'; import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { + cancelDeleteMember, + confirmDeleteMember, + memberActions, + memberCustomFieldEditModal, + memberCustomFieldsField, + memberDetailEngagement, + memberDetailTitle, + memberSigninUrl, + memberSubscriptionToggle, +} from '@tryghost/test-data/selectors/members'; /** * Page object for the member detail screen. @@ -26,7 +37,7 @@ class SettingsSection extends BasePage { constructor(page: Page) { super(page); - this.memberActionsButton = page.getByTestId('member-actions').filter({ visible: true }); + this.memberActionsButton = page.getByTestId(memberActions).filter({ visible: true }); this.impersonateButton = menuAction(page, 'Impersonate'); this.signOutOfAllDevices = menuAction(page, 'Sign out of all devices'); @@ -34,8 +45,8 @@ class SettingsSection extends BasePage { this.enableCommentingButton = menuAction(page, 'Enable commenting'); this.deleteButton = menuAction(page, 'Delete member'); - this.confirmDeleteButton = page.getByTestId('confirm-delete-member').filter({ visible: true }); - this.cancelDeleteButton = page.getByTestId('cancel-delete-member').filter({ visible: true }); + this.confirmDeleteButton = page.getByTestId(confirmDeleteMember).filter({ visible: true }); + this.cancelDeleteButton = page.getByTestId(cancelDeleteMember).filter({ visible: true }); } } @@ -89,7 +100,7 @@ export class MemberDetailsPage extends AdminPage { this.labelsInput = page.getByText('Labels').locator('+ div'); this.labels = this.labelsInput.getByRole('listitem'); this.newsletterSubscriptionToggles = page - .getByTestId('member-subscription-toggle') + .getByTestId(memberSubscriptionToggle) .filter({ visible: true }); this.saveButton = page.getByRole('button', { name: 'Save' }); @@ -98,7 +109,7 @@ export class MemberDetailsPage extends AdminPage { .locator('[data-test-link="members-back"]') .filter({ visible: true }); this.copyLinkButton = page.getByRole('button', { name: 'Copy link' }); - this.magicLinkInput = page.getByTestId('member-signin-url').filter({ visible: true }); + this.magicLinkInput = page.getByTestId(memberSigninUrl).filter({ visible: true }); this.confirmLeaveButton = page.getByRole('button', { name: 'Leave' }); this.settingsSection = new SettingsSection(page); @@ -116,7 +127,7 @@ export class MemberDetailsPage extends AdminPage { }); this.commentingDisabledIndicator = page.getByText('Comments disabled'); - this.screenTitle = page.getByTestId('member-detail-title'); + this.screenTitle = page.getByTestId(memberDetailTitle); this.logoutConfirmModal = page.getByRole('alertdialog', { name: 'Sign out member from all devices?', }); @@ -125,7 +136,7 @@ export class MemberDetailsPage extends AdminPage { this.newsletterSubscriptionCheckboxes = this.newsletterSubscriptionToggles.and( page.getByRole('switch'), ); - this.engagementSection = page.getByTestId('member-detail-engagement').filter({ visible: true }); + this.engagementSection = page.getByTestId(memberDetailEngagement).filter({ visible: true }); this.subscriptionActionsButton = page.getByRole('button', { name: 'Subscription menu' }); this.cancelSubscriptionButton = menuAction(page, 'Cancel subscription'); @@ -133,8 +144,8 @@ export class MemberDetailsPage extends AdminPage { this.removeComplimentaryButton = menuAction(page, 'Remove complimentary subscription'); this.compTierOptions = page.getByRole('option'); - this.customFieldsCard = page.getByTestId('member-custom-fields-field'); - this.customFieldModal = page.getByTestId('member-custom-field-edit-modal'); + this.customFieldsCard = page.getByTestId(memberCustomFieldsField); + this.customFieldModal = page.getByTestId(memberCustomFieldEditModal); } // The row's accessible name is "Edit {field}" (plus ": {value}" once set), so diff --git a/e2e/helpers/pages/admin/settings/sections/access-section.ts b/e2e/helpers/pages/admin/settings/sections/access-section.ts index 5c594d783e8..6b2415a6655 100644 --- a/e2e/helpers/pages/admin/settings/sections/access-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/access-section.ts @@ -1,5 +1,10 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { + access, + siteAccessCode, + siteVisibilitySelect, +} from '@tryghost/test-data/selectors/settings'; export class AccessSection extends BasePage { readonly section: Locator; @@ -10,10 +15,10 @@ export class AccessSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings'); - this.section = page.getByTestId('access'); + this.section = page.getByTestId(access); this.saveButton = this.section.getByRole('button', { name: 'Save' }); - this.visibilitySelect = this.section.getByTestId('site-visibility-select'); - this.passwordInput = this.section.getByTestId('site-access-code'); + this.visibilitySelect = this.section.getByTestId(siteVisibilitySelect); + this.passwordInput = this.section.getByTestId(siteAccessCode); } async enablePrivateMode(password: string): Promise { diff --git a/e2e/helpers/pages/admin/settings/sections/announcement-bar-section.ts b/e2e/helpers/pages/admin/settings/sections/announcement-bar-section.ts index 172299d452d..bdfdb58e8c9 100644 --- a/e2e/helpers/pages/admin/settings/sections/announcement-bar-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/announcement-bar-section.ts @@ -1,5 +1,6 @@ import { BasePage } from '@/helpers/pages'; import { FrameLocator, Locator, Page } from '@playwright/test'; +import { announcementBar, announcementBarModal } from '@tryghost/test-data/selectors/settings'; export class AnnouncementBarSection extends BasePage { readonly section: Locator; @@ -15,9 +16,9 @@ export class AnnouncementBarSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings'); - this.section = page.getByTestId('announcement-bar'); + this.section = page.getByTestId(announcementBar); this.customizeButton = this.section.getByRole('button', { name: 'Customize' }); - this.modal = page.getByTestId('announcement-bar-modal'); + this.modal = page.getByTestId(announcementBarModal); this.freeMembersCheckbox = this.modal.getByLabel('Free members'); this.editor = this.modal.locator('.koenig-react-editor'); this.contentEditable = this.modal.locator('[contenteditable="true"]'); 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 664da2754f4..2f1c276ba67 100644 --- a/e2e/helpers/pages/admin/settings/sections/custom-fields-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/custom-fields-section.ts @@ -1,5 +1,10 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { + customFieldListItem, + customFieldModal, + customFields, +} from '@tryghost/test-data/selectors/settings'; /** * Settings -> Membership -> Custom fields. The whole section is behind the @@ -14,13 +19,13 @@ export class CustomFieldsSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings'); - this.section = page.getByTestId('custom-fields'); + this.section = page.getByTestId(customFields); this.addButton = this.section.getByRole('button', { name: 'Add custom field' }); - this.modal = page.getByTestId('custom-field-modal'); + this.modal = page.getByTestId(customFieldModal); } listItem(name: string): Locator { - return this.section.getByTestId('custom-field-list-item').filter({ hasText: name }); + return this.section.getByTestId(customFieldListItem).filter({ hasText: name }); } /** diff --git a/e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts b/e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts index 83519e91cf3..3ca684727b6 100644 --- a/e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/danger-zone-section.ts @@ -1,5 +1,6 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { confirmationModal, dangerZone } from '@tryghost/test-data/selectors/settings'; export class DangerZoneSection extends BasePage { readonly section: Locator; @@ -15,13 +16,13 @@ export class DangerZoneSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings/advanced'); - this.section = page.getByTestId('dangerzone'); + this.section = page.getByTestId(dangerZone); this.heading = page.getByRole('heading', { level: 5, name: 'Danger zone' }); this.deleteAllContentButton = this.section.getByRole('button', { name: 'Delete all content' }); this.resetAuthButton = this.section.getByRole('button', { name: 'Reset all authentication' }); - this.confirmationModal = page.getByTestId('confirmation-modal'); + this.confirmationModal = page.getByTestId(confirmationModal); this.deleteAllContentOkButton = this.confirmationModal.getByRole('button', { name: 'Delete', exact: true, diff --git a/e2e/helpers/pages/admin/settings/sections/design-section.ts b/e2e/helpers/pages/admin/settings/sections/design-section.ts index 69a6adb9b75..26fcc00dc74 100644 --- a/e2e/helpers/pages/admin/settings/sections/design-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/design-section.ts @@ -1,5 +1,13 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { + design, + designModal, + imageDeleteButton, + imageUploadContainer, + publicationCover, + toggleUnsplashButton, +} from '@tryghost/test-data/selectors/settings'; export class DesignSection extends BasePage { readonly section: Locator; @@ -13,13 +21,13 @@ export class DesignSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings'); - this.section = page.getByTestId('design'); + this.section = page.getByTestId(design); this.customizeButton = this.section.getByRole('button', { name: 'Customize' }); - this.designModal = page.getByTestId('design-modal'); - this.unsplashButton = page.getByTestId('toggle-unsplash-button'); + this.designModal = page.getByTestId(designModal); + this.unsplashButton = page.getByTestId(toggleUnsplashButton); this.unsplashHeading = page.getByRole('heading', { name: 'Unsplash', level: 1 }); this.unsplashPhotos = page.locator('[data-kg-unsplash-gallery-img]'); - this.coverImage = page.getByTestId('publication-cover'); + this.coverImage = page.getByTestId(publicationCover); } async openDesignModal(): Promise { @@ -28,9 +36,9 @@ export class DesignSection extends BasePage { } async deleteCoverImage(): Promise { - const imageContainer = this.coverImage.getByTestId('image-upload-container'); + const imageContainer = this.coverImage.getByTestId(imageUploadContainer); await imageContainer.hover(); - await imageContainer.getByTestId('image-delete-button').click(); + await imageContainer.getByTestId(imageDeleteButton).click(); } async openUnsplashSelector(): Promise { diff --git a/e2e/helpers/pages/admin/settings/sections/integration-modal.ts b/e2e/helpers/pages/admin/settings/sections/integration-modal.ts index b6f6ab25013..1135cdb5c20 100644 --- a/e2e/helpers/pages/admin/settings/sections/integration-modal.ts +++ b/e2e/helpers/pages/admin/settings/sections/integration-modal.ts @@ -1,5 +1,6 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { integrations } from '@tryghost/test-data/selectors/settings'; export interface IntegrationConfig { name: string; @@ -53,7 +54,7 @@ export class IntegrationModal extends BasePage { this.config = typeof integration === 'string' ? INTEGRATIONS[integration] : integration; - this.integrationsSection = page.getByTestId('integrations'); + this.integrationsSection = page.getByTestId(integrations); this.integrationItem = page.getByTestId(this.config.testId); this.modal = page.getByTestId(this.config.modalTestId); this.enableToggle = this.modal.getByLabel(this.config.toggleLabel); diff --git a/e2e/helpers/pages/admin/settings/sections/integrations-section.ts b/e2e/helpers/pages/admin/settings/sections/integrations-section.ts index b25ddbb0dca..4687ec844bb 100644 --- a/e2e/helpers/pages/admin/settings/sections/integrations-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/integrations-section.ts @@ -1,5 +1,6 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { integrations } from '@tryghost/test-data/selectors/settings'; export class IntegrationsSection extends BasePage { readonly integrationsSection: Locator; @@ -10,7 +11,7 @@ export class IntegrationsSection extends BasePage { constructor(page: Page) { super(page, 'ghost/#/settings/integrations'); - this.integrationsSection = page.getByTestId('integrations'); + this.integrationsSection = page.getByTestId(integrations); this.integrationsHeading = page.getByRole('heading', { level: 5, name: 'Integrations' }); this.integrationsDescription = page.getByText('Make Ghost work with apps and tools'); this.integrationsAddButton = page.getByRole('button', { name: 'Add custom integration' }); diff --git a/e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts b/e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts index 50a7dac320f..6140d11517f 100644 --- a/e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts @@ -1,5 +1,17 @@ import { BasePage } from '@/helpers/pages'; import { FrameLocator, Locator, Page } from '@playwright/test'; +import { + freeWelcomeEmailRow, + headerImageField, + memberEmails, + welcomeEmailCustomizeModal, + welcomeEmailEditor, + welcomeEmailModal, + welcomeEmailModeEdit, + welcomeEmailModePreview, + welcomeEmailPreviewIframe, + welcomeEmailPreviewSubject, +} from '@tryghost/test-data/selectors/settings'; export class MemberWelcomeEmailsSection extends BasePage { readonly section: Locator; @@ -47,17 +59,15 @@ export class MemberWelcomeEmailsSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings/memberemails'); - this.section = page.getByTestId('memberemails'); - this.freeWelcomeEmailToggle = this.section - .getByTestId('free-welcome-email-row') - .getByRole('switch'); + this.section = page.getByTestId(memberEmails); + this.freeWelcomeEmailToggle = this.section.getByTestId(freeWelcomeEmailRow).getByRole('switch'); this.freeWelcomeEmailEditButton = this.section - .getByTestId('free-welcome-email-row') + .getByTestId(freeWelcomeEmailRow) .getByRole('button', { name: 'Edit' }); // Customize button and modal this.customizeButton = this.section.getByRole('button', { name: 'Customize' }); - this.customizeModal = page.getByTestId('welcome-email-customize-modal'); + this.customizeModal = page.getByTestId(welcomeEmailCustomizeModal); this.customizeModalSaveButton = this.customizeModal.getByRole('button', { name: 'Save' }); this.customizeModalCloseButton = this.customizeModal.getByRole('button', { name: 'Close' }); this.customizeModalUnsavedChangesDialog = page.getByRole('alertdialog', { @@ -72,7 +82,7 @@ export class MemberWelcomeEmailsSection extends BasePage { .locator('..') .getByRole('switch'); this.customizeModalFooterTextarea = this.customizeModal.getByLabel('Email footer'); - this.customizeModalHeaderImageUpload = this.customizeModal.getByTestId('header-image-field'); + this.customizeModalHeaderImageUpload = this.customizeModal.getByTestId(headerImageField); this.customizeModalBadgeToggle = this.customizeModal .getByText('Promote independent publishing') .locator('../..') @@ -105,18 +115,16 @@ export class MemberWelcomeEmailsSection extends BasePage { this.customizeModalColorPickerPopover = page.locator('[data-radix-popper-content-wrapper]'); // Modal locators - this.welcomeEmailModal = page.getByTestId('welcome-email-modal'); - this.modalEditor = this.welcomeEmailModal.getByTestId('welcome-email-editor'); - this.modalEditTab = this.welcomeEmailModal.getByTestId('welcome-email-mode-edit'); - this.modalPreviewTab = this.welcomeEmailModal.getByTestId('welcome-email-mode-preview'); - this.modalPreviewSubjectInput = this.welcomeEmailModal.getByTestId( - 'welcome-email-preview-subject', - ); + this.welcomeEmailModal = page.getByTestId(welcomeEmailModal); + this.modalEditor = this.welcomeEmailModal.getByTestId(welcomeEmailEditor); + this.modalEditTab = this.welcomeEmailModal.getByTestId(welcomeEmailModeEdit); + this.modalPreviewTab = this.welcomeEmailModal.getByTestId(welcomeEmailModePreview); + this.modalPreviewSubjectInput = this.welcomeEmailModal.getByTestId(welcomeEmailPreviewSubject); this.modalSubjectInput = this.modalPreviewSubjectInput; this.modalSaveButton = this.welcomeEmailModal.getByRole('button', { name: 'Save' }); this.modalSavedButton = this.welcomeEmailModal.getByRole('button', { name: 'Saved' }); this.modalLexicalEditor = this.modalEditor.getByRole('textbox').first(); - this.modalPreviewIframe = this.welcomeEmailModal.getByTestId('welcome-email-preview-iframe'); + this.modalPreviewIframe = this.welcomeEmailModal.getByTestId(welcomeEmailPreviewIframe); this.modalPreviewFrame = page.frameLocator('iframe[title="Welcome email preview"]'); } @@ -140,9 +148,7 @@ export class MemberWelcomeEmailsSection extends BasePage { } private async waitForFreeToggle(checked: boolean): Promise { - const toggle = this.section - .getByTestId('free-welcome-email-row') - .getByRole('switch', { checked }); + const toggle = this.section.getByTestId(freeWelcomeEmailRow).getByRole('switch', { checked }); await toggle.waitFor({ state: 'visible' }); } diff --git a/e2e/helpers/pages/admin/settings/sections/portal-section.ts b/e2e/helpers/pages/admin/settings/sections/portal-section.ts index 94be9490573..be9334182bc 100644 --- a/e2e/helpers/pages/admin/settings/sections/portal-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/portal-section.ts @@ -1,5 +1,6 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { portal, portalModal } from '@tryghost/test-data/selectors/settings'; type PaidSignupCadence = 'monthly' | 'yearly'; @@ -14,9 +15,9 @@ export class PortalSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings'); - this.section = page.getByTestId('portal'); + this.section = page.getByTestId(portal); this.customizeButton = this.section.getByRole('button', { name: 'Customize' }); - this.portalModal = page.getByTestId('portal-modal'); + this.portalModal = page.getByTestId(portalModal); this.linksTab = this.portalModal.getByRole('tab', { name: 'Links' }); this.linksTierSelectControl = this.portalModal.getByRole('combobox', { name: 'Tier' }); this.freeTierToggleLabel = this.portalModal diff --git a/e2e/helpers/pages/admin/settings/sections/staff-section.ts b/e2e/helpers/pages/admin/settings/sections/staff-section.ts index d4e3d00191d..5dab59d8ef6 100644 --- a/e2e/helpers/pages/admin/settings/sections/staff-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/staff-section.ts @@ -1,5 +1,6 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { ownerUser, users } from '@tryghost/test-data/selectors/settings'; export class StaffSection extends BasePage { readonly requireTwoFaButton: Locator; @@ -8,8 +9,8 @@ export class StaffSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings/staff'); - this.ownerUser = this.page.getByTestId('owner-user'); - this.requireTwoFaButton = page.getByTestId('users').getByRole('switch'); + this.ownerUser = this.page.getByTestId(ownerUser); + this.requireTwoFaButton = page.getByTestId(users).getByRole('switch'); this.inviteStaffButton = page.getByRole('button', { name: 'Invite people' }); } @@ -60,7 +61,7 @@ export class StaffSection extends BasePage { } private async waitForSwitch(checked: boolean): Promise { - const switchState = this.page.getByTestId('users').getByRole('switch', { checked: checked }); + const switchState = this.page.getByTestId(users).getByRole('switch', { checked: checked }); await switchState.waitFor({ state: 'visible' }); } } diff --git a/e2e/helpers/pages/admin/settings/sections/tiers-section.ts b/e2e/helpers/pages/admin/settings/sections/tiers-section.ts index 583fbee250f..285dcb2e486 100644 --- a/e2e/helpers/pages/admin/settings/sections/tiers-section.ts +++ b/e2e/helpers/pages/admin/settings/sections/tiers-section.ts @@ -1,5 +1,12 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { + confirmationModal, + portal, + portalModal, + tierDetailModal, + tiers, +} from '@tryghost/test-data/selectors/settings'; export interface TierFormData { name: string; @@ -19,10 +26,10 @@ export class TiersSection extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings'); - this.section = page.getByTestId('tiers'); + this.section = page.getByTestId(tiers); this.addTierButton = this.section.getByRole('button', { name: 'Add tier' }); - this.tierDetailModal = page.getByTestId('tier-detail-modal'); - this.confirmationModal = page.getByTestId('confirmation-modal'); + this.tierDetailModal = page.getByTestId(tierDetailModal); + this.confirmationModal = page.getByTestId(confirmationModal); this.activeTab = this.section.getByRole('tab', { name: 'Active' }); this.archivedTab = this.section.getByRole('tab', { name: 'Archived' }); } @@ -89,27 +96,27 @@ export class TiersSection extends BasePage { } async enableTierInPortal(tierName: string): Promise { - const portalSection = this.page.getByTestId('portal'); + const portalSection = this.page.getByTestId(portal); await portalSection.getByRole('button', { name: 'Customize' }).click(); - const portalModal = this.page.getByTestId('portal-modal'); - await portalModal.waitFor({ state: 'visible' }); + const modal = this.page.getByTestId(portalModal); + await modal.waitFor({ state: 'visible' }); - const tierCheckbox = portalModal.getByLabel(tierName).first(); + const tierCheckbox = modal.getByLabel(tierName).first(); if (!(await tierCheckbox.isChecked())) { await tierCheckbox.check(); } - const monthlyCheckbox = portalModal.getByLabel('Monthly').first(); + const monthlyCheckbox = modal.getByLabel('Monthly').first(); if (!(await monthlyCheckbox.isChecked())) { await monthlyCheckbox.check(); } - const yearlyCheckbox = portalModal.getByLabel('Yearly').first(); + const yearlyCheckbox = modal.getByLabel('Yearly').first(); if (!(await yearlyCheckbox.isChecked())) { await yearlyCheckbox.check(); } - await portalModal.getByRole('button', { name: 'Save' }).click(); - await portalModal.getByRole('button', { name: 'Close' }).click(); + await modal.getByRole('button', { name: 'Save' }).click(); + await modal.getByRole('button', { name: 'Close' }).click(); } } diff --git a/e2e/helpers/pages/admin/settings/settings-page.ts b/e2e/helpers/pages/admin/settings/settings-page.ts index 8e8eee799ea..9d2de4a8fca 100644 --- a/e2e/helpers/pages/admin/settings/settings-page.ts +++ b/e2e/helpers/pages/admin/settings/settings-page.ts @@ -8,6 +8,7 @@ import { } from './sections'; import { Locator, Page } from '@playwright/test'; import { StaffSection } from './sections/staff-section'; +import { settingsSidebar } from '@tryghost/test-data/selectors/settings'; export class SettingsPage extends BasePage { readonly integrationsSection: IntegrationsSection; @@ -22,7 +23,7 @@ export class SettingsPage extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings'); - this.sidebar = page.getByTestId('sidebar'); + this.sidebar = page.getByTestId(settingsSidebar); this.portalSection = new PortalSection(page); this.integrationsSection = new IntegrationsSection(page); diff --git a/e2e/helpers/pages/admin/settings/staff-details-page.ts b/e2e/helpers/pages/admin/settings/staff-details-page.ts index c647c86d7a6..49ae54ae3a3 100644 --- a/e2e/helpers/pages/admin/settings/staff-details-page.ts +++ b/e2e/helpers/pages/admin/settings/staff-details-page.ts @@ -1,5 +1,12 @@ import { BasePage } from '@/helpers/pages'; import { Locator, Page } from '@playwright/test'; +import { + coverImagePreview, + coverImageUpload, + profileImagePreview, + profileImageUpload, + userDetailModal, +} from '@tryghost/test-data/selectors/settings'; type FilePayload = { name: string; @@ -20,13 +27,13 @@ export class AdminStaffDetailsPage extends BasePage { constructor(page: Page) { super(page, '/ghost/#/settings/staff'); - this.userDetailModal = page.getByTestId('user-detail-modal'); + this.userDetailModal = page.getByTestId(userDetailModal); this.emailInput = this.userDetailModal.getByRole('textbox', { name: /Email/i }); this.slugInput = this.userDetailModal.getByRole('textbox', { name: 'Slug' }); - this.profileImageInput = this.userDetailModal.getByTestId('profile-image-upload'); - this.coverImageInput = this.userDetailModal.getByTestId('cover-image-upload'); - this.profileImagePreview = this.userDetailModal.getByTestId('profile-image-preview'); - this.coverImagePreview = this.userDetailModal.getByTestId('cover-image-preview'); + this.profileImageInput = this.userDetailModal.getByTestId(profileImageUpload); + this.coverImageInput = this.userDetailModal.getByTestId(coverImageUpload); + this.profileImagePreview = this.userDetailModal.getByTestId(profileImagePreview); + this.coverImagePreview = this.userDetailModal.getByTestId(coverImagePreview); this.savedButton = this.userDetailModal.getByRole('button', { name: 'Saved' }); } diff --git a/e2e/tests/admin/analytics/growth.test.ts b/e2e/tests/admin/analytics/growth.test.ts index 2adbb14d640..ffb82e257d0 100644 --- a/e2e/tests/admin/analytics/growth.test.ts +++ b/e2e/tests/admin/analytics/growth.test.ts @@ -1,45 +1,17 @@ import { AnalyticsGrowthPage } from '@/admin-pages'; import { expect, test } from '@/helpers/playwright'; +// The growth family's server journey: a fresh site must return a zero-filled +// member count history (members-stats zero-fill contract) so the growth view +// renders instead of collapsing. The empty-state UI variations live in the +// admin acceptance tier, which fakes this contract and depends on it holding. test.describe('Ghost Admin - Growth', () => { - let growthPage: AnalyticsGrowthPage; - - test.beforeEach(async ({ page }) => { - growthPage = new AnalyticsGrowthPage(page); + test('renders the growth view with zeroed KPIs on a fresh site', async ({ page }) => { + const growthPage = new AnalyticsGrowthPage(page); await growthPage.goto(); - }); - - test('empty top content card - posts and pages', async () => { - await expect(growthPage.topContent.contentCard).toContainText( - 'Which posts or pages drove the most growth in the last 30 days', - ); - await expect(growthPage.topContent.contentCard).toContainText('No conversions'); - }); - - test('empty top content card - posts', async () => { - await growthPage.topContent.postsButton.click(); - - await expect(growthPage.topContent.contentCard).toContainText( - 'Which posts drove the most growth in the last 30 days', - ); - await expect(growthPage.topContent.contentCard).toContainText('No conversions'); - }); - - test('empty top content card - pages', async () => { - await growthPage.topContent.pagesButton.click(); - - await expect(growthPage.topContent.contentCard).toContainText( - 'Which pages drove the most growth in the last 30 days', - ); - await expect(growthPage.topContent.contentCard).toContainText('No conversions'); - }); - - test('empty top content card - sources', async () => { - await growthPage.topContent.sourcesButton.click(); - await expect(growthPage.topContent.contentCard).toContainText( - 'Which sources drove the most growth in the last 30 days', - ); + await expect(growthPage.totalMembersCard).toBeVisible(); + await expect(growthPage.totalMembersCard).toContainText('0'); await expect(growthPage.topContent.contentCard).toContainText('No conversions'); }); }); diff --git a/e2e/tests/admin/analytics/newsletters.test.ts b/e2e/tests/admin/analytics/newsletters.test.ts index e11bb05cf51..14803222299 100644 --- a/e2e/tests/admin/analytics/newsletters.test.ts +++ b/e2e/tests/admin/analytics/newsletters.test.ts @@ -16,28 +16,6 @@ test.describe('Ghost Admin - Newsletters', () => { await newslettersPage.goto(); }); - test('empty newsletters card', async () => { - await expect(newslettersPage.newslettersCard).toBeVisible(); - }); - - test('empty average open rate and click rate card', async () => { - await newslettersPage.averageOpenRateTab.click(); - await expect(newslettersPage.newslettersCard).toContainText( - 'No newsletters in the last 30 days', - ); - - await newslettersPage.averageClickRateTab.click(); - await expect(newslettersPage.newslettersCard).toContainText( - 'No newsletters in the last 30 days', - ); - }); - - test('empty top newsletters card', async () => { - await expect(newslettersPage.topNewslettersCard).toContainText( - 'newsletters in the last 30 days', - ); - }); - test('total subscribers percent change calculation', async ({ page }) => { const membersService = new MembersImportService(page.request); diff --git a/e2e/tests/admin/analytics/overview.test.ts b/e2e/tests/admin/analytics/overview.test.ts index 2e5f9fc52d9..7317280558b 100644 --- a/e2e/tests/admin/analytics/overview.test.ts +++ b/e2e/tests/admin/analytics/overview.test.ts @@ -1,4 +1,4 @@ -import { AnalyticsGrowthPage, AnalyticsOverviewPage, AnalyticsWebTrafficPage } from '@/admin-pages'; +import { AnalyticsOverviewPage } from '@/admin-pages'; import { HomePage } from '@/public-pages'; import { createPostFactory } from '@/data-factory'; import { expect, test, withIsolatedPage } from '@/helpers/playwright'; @@ -24,52 +24,4 @@ test.describe('Ghost Admin - Analytics Overview', () => { expect(await analyticsOverviewPage.uniqueVisitors.count()).toBe(1); }); - - test('latest post', async ({ page }) => { - const analyticsOverviewPage = new AnalyticsOverviewPage(page); - await analyticsOverviewPage.goto(); - - const membersCount = await analyticsOverviewPage.latestPost.membersCount(); - const visitorsCount = await analyticsOverviewPage.latestPost.visitorsCount(); - - await expect(analyticsOverviewPage.latestPost.post).toBeVisible(); - expect(visitorsCount).toContain('0'); - expect(membersCount).toContain('0'); - }); - - test('top posts', async ({ page }) => { - const analyticsOverviewPage = new AnalyticsOverviewPage(page); - await analyticsOverviewPage.goto(); - - await expect(analyticsOverviewPage.topPosts.post).toBeVisible(); - - const visitorsStatistics = await analyticsOverviewPage.topPosts.uniqueVisitorsStatistics(); - const membersStatistics = await analyticsOverviewPage.topPosts.membersStatistics(); - - expect(visitorsStatistics).toContain('Unique visitors'); - expect(visitorsStatistics).toContain('0'); - expect(membersStatistics).toContain('New members'); - expect(membersStatistics).toContain('Free'); - expect(membersStatistics).toContain('0'); - }); - - test('view more unique visitors details', async ({ page }) => { - const analyticsOverviewPage = new AnalyticsOverviewPage(page); - await analyticsOverviewPage.goto(); - - await analyticsOverviewPage.viewMoreUniqueVisitorDetails(); - - const analyticsWebTrafficPage = new AnalyticsWebTrafficPage(page); - await expect(analyticsWebTrafficPage.totalUniqueVisitorsTab).toBeVisible(); - }); - - test('view more members details', async ({ page }) => { - const analyticsOverviewPage = new AnalyticsOverviewPage(page); - await analyticsOverviewPage.goto(); - - await analyticsOverviewPage.viewMoreMembersDetails(); - - const analyticsGrowthPage = new AnalyticsGrowthPage(page); - await expect(analyticsGrowthPage.totalMembersCard).toBeVisible(); - }); }); diff --git a/e2e/tests/admin/analytics/post-analytics/growth.test.ts b/e2e/tests/admin/analytics/post-analytics/growth.test.ts deleted file mode 100644 index 354f5a2cdde..00000000000 --- a/e2e/tests/admin/analytics/post-analytics/growth.test.ts +++ /dev/null @@ -1,49 +0,0 @@ -import { - MembersPage, - PostAnalyticsGrowthPage, - PostAnalyticsOverviewPage, - PostAnalyticsPage, -} from '@/admin-pages'; -import { createPostFactory } from '@/data-factory'; -import { expect, test } from '@/helpers/playwright'; - -test.describe('Ghost Admin - Post Analytics - Growth', () => { - test.beforeEach(async ({ page }) => { - const postFactory = createPostFactory(page.request); - const post = await postFactory.create({ - title: 'Post analytics growth test', - status: 'published', - }); - - const postAnalyticsOverviewPage = new PostAnalyticsOverviewPage(page); - await postAnalyticsOverviewPage.gotoForPost(post.id); - - // TODO: check post analytics component, we shouldn't need to wait on page load to be able to click growth link - const postAnalyticsPage = new PostAnalyticsPage(page); - await postAnalyticsPage.waitForPageLoad(); - await postAnalyticsPage.growthButton.click(); - }); - - test('empty members card', async ({ page }) => { - const postAnalyticsPageGrowthPage = new PostAnalyticsGrowthPage(page); - - await expect(postAnalyticsPageGrowthPage.membersCard).toContainText('Free members'); - await expect(postAnalyticsPageGrowthPage.membersCard).toContainText('0'); - }); - - test('empty members card - view member', async ({ page }) => { - const postAnalyticsPageGrowthPage = new PostAnalyticsGrowthPage(page); - await postAnalyticsPageGrowthPage.viewMemberButton.click(); - - const membersPage = new MembersPage(page); - await expect(membersPage.body).toContainText('No matching members found.'); - }); - - test('empty top sources card', async ({ page }) => { - const postAnalyticsPageGrowthPage = new PostAnalyticsGrowthPage(page); - - await expect(postAnalyticsPageGrowthPage.topSourcesCard).toContainText( - 'No sources data available', - ); - }); -}); diff --git a/e2e/tests/admin/analytics/post-analytics/overview.test.ts b/e2e/tests/admin/analytics/post-analytics/overview.test.ts index 9d87d0929d9..d9a14af081a 100644 --- a/e2e/tests/admin/analytics/post-analytics/overview.test.ts +++ b/e2e/tests/admin/analytics/post-analytics/overview.test.ts @@ -1,76 +1,38 @@ -import { - PostAnalyticsGrowthPage, - PostAnalyticsOverviewPage, - PostAnalyticsPage, - PostAnalyticsWebTrafficPage, -} from '@/admin-pages'; +import { PostAnalyticsOverviewPage, PostAnalyticsPage } from '@/admin-pages'; import { SettingsService } from '@/helpers/services/settings/settings-service'; import { createPostFactory } from '@/data-factory'; import { expect, test } from '@/helpers/playwright'; +// The family's server journey: the members_track_sources setting round-trips +// through the real settings API and the analytics chrome reacts after reload. +// The pure-rendering variants of these screens live in the admin acceptance +// tier against faked data. test.describe('Ghost Admin - Post Analytics - Overview', () => { - test.beforeEach(async ({ page }) => { + test('hides growth after member source tracking is disabled and persisted', async ({ page }) => { const postFactory = createPostFactory(page.request); const post = await postFactory.create({ - title: 'Post analytics overview test', + title: 'Post analytics settings journey', status: 'published', }); const postAnalyticsOverviewPage = new PostAnalyticsOverviewPage(page); await postAnalyticsOverviewPage.gotoForPost(post.id); - }); - - test('empty page with all tabs', async ({ page }) => { - const postAnalyticsPage = new PostAnalyticsPage(page); - - await expect(postAnalyticsPage.overviewButton).toBeVisible(); - await expect(postAnalyticsPage.webTrafficButton).toBeVisible(); - await expect(postAnalyticsPage.growthButton).toBeVisible(); - }); - - test('empty page - overview - web performance - view more', async ({ page }) => { - const postAnalyticsPage = new PostAnalyticsPage(page); - await postAnalyticsPage.webPerformanceSection.viewMoreButton.click(); - - const postAnalyticsWebTrafficPage = new PostAnalyticsWebTrafficPage(page); - await expect(postAnalyticsWebTrafficPage.body).toContainText('No visitors in the last 30 days'); - }); - test('empty page - overview - growth', async ({ page }) => { - const postAnalyticsPage = new PostAnalyticsPage(page); - - await expect(postAnalyticsPage.growthSection.card).toContainText('Free members'); - await expect(postAnalyticsPage.growthSection.card).toContainText('0'); - }); - - test('empty page - overview - growth - view more', async ({ page }) => { - const postAnalyticsPage = new PostAnalyticsPage(page); - await postAnalyticsPage.growthSection.viewMoreButton.click(); - - const postAnalyticsGrowthPage = new PostAnalyticsGrowthPage(page); - await expect(postAnalyticsGrowthPage.topSourcesCard).toContainText('No sources data available'); - }); - - test('growth tab and section - hidden when member sources tracking disabled', async ({ - page, - }) => { const settingsService = new SettingsService(page.request); const postAnalyticsPage = new PostAnalyticsPage(page); await expect(postAnalyticsPage.growthButton).toBeVisible(); - await expect(postAnalyticsPage.growthSection.card).toBeVisible(); try { await settingsService.setMembersTrackSources(false); await page.reload(); - await expect(postAnalyticsPage.growthButton).toBeHidden(); - await expect(postAnalyticsPage.growthSection.card).toBeHidden(); await expect(postAnalyticsPage.overviewButton).toBeVisible(); - await expect(postAnalyticsPage.webTrafficButton).toBeVisible(); + await expect(postAnalyticsPage.growthButton).toBeHidden(); } finally { await settingsService.setMembersTrackSources(true); await page.reload(); + await expect(postAnalyticsPage.growthButton).toBeVisible(); } }); }); diff --git a/e2e/tests/admin/analytics/web-traffic.test.ts b/e2e/tests/admin/analytics/web-traffic.test.ts deleted file mode 100644 index 0d7eb22a182..00000000000 --- a/e2e/tests/admin/analytics/web-traffic.test.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { AnalyticsWebTrafficPage } from '@/admin-pages'; -import { expect, test } from '@/helpers/playwright'; - -test.describe('Ghost Admin - Analytics Web Traffic', () => { - let analyticsWebTrafficPage: AnalyticsWebTrafficPage; - - test.beforeEach(async ({ page }) => { - analyticsWebTrafficPage = new AnalyticsWebTrafficPage(page); - await analyticsWebTrafficPage.goto(); - }); - - test('empty web traffic general graph', async () => { - await expect(analyticsWebTrafficPage.totalUniqueVisitorsTab).toContainText('0'); - await expect(analyticsWebTrafficPage.totalViewsTab).toContainText('0'); - }); - - test('empty top content card', async () => { - await expect(analyticsWebTrafficPage.topContentCard).toContainText('No visitors'); - }); - - test('empty top content card - posts', async () => { - await analyticsWebTrafficPage.postsButton.click(); - - await expect(analyticsWebTrafficPage.topContentCard).toContainText('No visitors'); - }); - - test('empty top content card - pages', async () => { - await analyticsWebTrafficPage.pagesButton.click(); - - await expect(analyticsWebTrafficPage.topContentCard).toContainText('No visitors'); - }); - - test('empty top sources card', async () => { - await expect(analyticsWebTrafficPage.topSourcesCard).toContainText('No visitors'); - }); - - test('empty locations card', async () => { - await expect(analyticsWebTrafficPage.locationsCard).toContainText('No visitors'); - }); -}); diff --git a/e2e/tests/admin/settings/integrations.test.ts b/e2e/tests/admin/settings/integrations.test.ts index de36f592645..5791daf536b 100644 --- a/e2e/tests/admin/settings/integrations.test.ts +++ b/e2e/tests/admin/settings/integrations.test.ts @@ -1,199 +1,21 @@ import { IntegrationModal } from '@/admin-pages'; import { expect, test } from '@/helpers/playwright'; -interface Setting { - key: string; - value: string | boolean | null; -} - -interface SettingsResponse { - settings: Setting[]; -} - test.describe('Ghost Admin - Integrations', () => { - test.describe('Transistor Integration', () => { - test.describe('with transistor labs flag enabled', () => { - test.use({ labs: { transistor: true } }); - - test('transistor integration is visible when labs flag is enabled', async ({ page }) => { - const transistor = new IntegrationModal(page, 'transistor'); - - await transistor.goto(); - - await expect(transistor.integrationItem).toBeVisible(); - }); - - test('can open transistor modal', async ({ page }) => { - const transistor = new IntegrationModal(page, 'transistor'); - - await transistor.goto(); - await transistor.openModal(); - - await expect(transistor.modal).toBeVisible(); - await expect(transistor.enableToggle).toBeVisible(); - }); - - test('can enable transistor integration', async ({ page }) => { - const transistor = new IntegrationModal(page, 'transistor'); - - await transistor.goto(); - await transistor.openModal(); - await transistor.enable(); - await transistor.save(); - - const response = await page.request.get('/ghost/api/admin/settings/'); - expect(response.ok()).toBe(true); - - const data = (await response.json()) as SettingsResponse; - const transistorSetting = data.settings.find((s) => s.key === 'transistor'); - expect(transistorSetting).toBeDefined(); - expect(transistorSetting?.value).toBe(true); - }); - - test('can disable transistor integration', async ({ page }) => { - const transistor = new IntegrationModal(page, 'transistor'); - - await transistor.goto(); - await transistor.openModal(); - await transistor.enable(); - await transistor.save(); - - await transistor.closeModal(); - await transistor.openModal(); - await transistor.disable(); - await transistor.save(); - - const response = await page.request.get('/ghost/api/admin/settings/'); - expect(response.ok()).toBe(true); - - const data = (await response.json()) as SettingsResponse; - const transistorSetting = data.settings.find((s) => s.key === 'transistor'); - expect(transistorSetting).toBeDefined(); - expect(transistorSetting?.value).toBe(false); - }); - - test('transistor enabled state persists after page reload', async ({ page }) => { - const transistor = new IntegrationModal(page, 'transistor'); - - await transistor.goto(); - await transistor.openModal(); - await transistor.enable(); - await transistor.save(); - await transistor.closeModal(); - - await page.reload(); - await transistor.integrationsSection.waitFor({ state: 'visible' }); - - await transistor.openModal(); - - await expect(transistor.enableToggle).toHaveAttribute('aria-checked', 'true'); - }); - - test('transistor shows active badge when enabled', async ({ page }) => { - const transistor = new IntegrationModal(page, 'transistor'); - - await transistor.goto(); - await transistor.openModal(); - await transistor.enable(); - await transistor.save(); - await transistor.closeModal(); - - await expect(transistor.integrationItem.getByText('Active')).toBeVisible(); - }); - }); - - test.describe('with transistor labs flag disabled', () => { - test.use({ labs: { transistor: false } }); - - test('transistor integration is hidden when labs flag is disabled', async ({ page }) => { - const transistor = new IntegrationModal(page, 'transistor'); - - await transistor.goto(); - - await expect(transistor.integrationItem).toBeHidden(); - }); - }); - }); - - test.describe('Unsplash Integration', () => { - test('can enable unsplash integration', async ({ page }) => { - const unsplash = new IntegrationModal(page, 'unsplash'); - - await unsplash.goto(); - await unsplash.openModal(); - await unsplash.enable(); - await unsplash.save(); - - const response = await page.request.get('/ghost/api/admin/settings/'); - expect(response.ok()).toBe(true); - - const data = (await response.json()) as SettingsResponse; - const unsplashSetting = data.settings.find((s) => s.key === 'unsplash'); - expect(unsplashSetting).toBeDefined(); - expect(unsplashSetting?.value).toBe(true); - }); - - test('can disable unsplash integration', async ({ page }) => { - const unsplash = new IntegrationModal(page, 'unsplash'); - - await unsplash.goto(); - await unsplash.openModal(); - await unsplash.enable(); - await unsplash.save(); - - await unsplash.closeModal(); - await unsplash.openModal(); - await unsplash.disable(); - await unsplash.save(); - - const response = await page.request.get('/ghost/api/admin/settings/'); - expect(response.ok()).toBe(true); - - const data = (await response.json()) as SettingsResponse; - const unsplashSetting = data.settings.find((s) => s.key === 'unsplash'); - expect(unsplashSetting).toBeDefined(); - expect(unsplashSetting?.value).toBe(false); - }); - }); - - test.describe('Pintura Integration', () => { - test('can enable pintura integration', async ({ page }) => { - const pintura = new IntegrationModal(page, 'pintura'); - - await pintura.goto(); - await pintura.openModal(); - await pintura.enable(); - await pintura.save(); - - const response = await page.request.get('/ghost/api/admin/settings/'); - expect(response.ok()).toBe(true); - - const data = (await response.json()) as SettingsResponse; - const pinturaSetting = data.settings.find((s) => s.key === 'pintura'); - expect(pinturaSetting).toBeDefined(); - expect(pinturaSetting?.value).toBe(true); - }); - - test('can disable pintura integration', async ({ page }) => { - const pintura = new IntegrationModal(page, 'pintura'); + test('transistor enabled state persists after page reload', async ({ page }) => { + const transistor = new IntegrationModal(page, 'transistor'); - await pintura.goto(); - await pintura.openModal(); - await pintura.enable(); - await pintura.save(); + await transistor.goto(); + await transistor.openModal(); + await transistor.enable(); + await transistor.save(); + await transistor.closeModal(); - await pintura.closeModal(); - await pintura.openModal(); - await pintura.disable(); - await pintura.save(); + await page.reload(); + await transistor.integrationsSection.waitFor({ state: 'visible' }); - const response = await page.request.get('/ghost/api/admin/settings/'); - expect(response.ok()).toBe(true); + await transistor.openModal(); - const data = (await response.json()) as SettingsResponse; - const pinturaSetting = data.settings.find((s) => s.key === 'pintura'); - expect(pinturaSetting).toBeDefined(); - expect(pinturaSetting?.value).toBe(false); - }); + await expect(transistor.enableToggle).toHaveAttribute('aria-checked', 'true'); }); }); diff --git a/e2e/tests/admin/tags/editor.test.ts b/e2e/tests/admin/tags/editor.test.ts index 41560728156..3e7661befbf 100644 --- a/e2e/tests/admin/tags/editor.test.ts +++ b/e2e/tests/admin/tags/editor.test.ts @@ -1,4 +1,4 @@ -import { NewTagsPage, SidebarPage, TagEditorPage, TagsPage } from '@/admin-pages'; +import { NewTagsPage, TagEditorPage, TagsPage } from '@/admin-pages'; import { expect, test } from '@/helpers/playwright'; test.describe('Ghost Admin - Tags Editor', () => { @@ -97,39 +97,4 @@ test.describe('Ghost Admin - Tags Editor', () => { await expect(page).toHaveURL(tagsPage.pageUrl); await expect(tagsPage.getTagLinkByName('News')).toBeHidden(); }); - - test('can load tag via slug in url', async ({ page }) => { - const tagEditor = new TagEditorPage(page); - await tagEditor.gotoTagBySlug('news'); - - await expect(page).toHaveURL('/ghost/#/tags/news'); - await expect(tagEditor.nameInput).toHaveValue('News'); - await expect(tagEditor.slugInput).toHaveValue('news'); - }); - - test('redirects to 404 when tag does not exist', async ({ page }) => { - const tagEditor = new TagEditorPage(page); - await tagEditor.gotoTagBySlug('unknown'); - - await expect(page.getByText('Page not found')).toBeVisible(); - }); - - test('maintains active state in nav menu when creating a new tag', async ({ page }) => { - const newTagsPage = new NewTagsPage(page); - const sidebar = new SidebarPage(page); - await newTagsPage.goto(); - - await expect(page).toHaveURL(newTagsPage.pageUrl); - await expect(sidebar.getNavLink('Tags')).toHaveAttribute('aria-current', 'page'); - }); - - test('maintains active state in nav menu when editing a tag', async ({ page }) => { - const tagsPage = new TagsPage(page); - const sidebar = new SidebarPage(page); - - await tagsPage.goto(); - await tagsPage.getTagLinkByName('News').click(); - - await expect(sidebar.getNavLink('Tags')).toHaveAttribute('aria-current', 'page'); - }); }); diff --git a/packages/testing/test-data/src/selectors/members.ts b/packages/testing/test-data/src/selectors/members.ts index 45b93083121..a68fd38f48e 100644 --- a/packages/testing/test-data/src/selectors/members.ts +++ b/packages/testing/test-data/src/selectors/members.ts @@ -8,6 +8,13 @@ export const membersListItem = 'members-list-item'; export const membersSearchInput = 'members-search-input'; export const membersActions = 'members-actions'; export const memberDetail = 'member-detail'; +export const memberDetailTitle = 'member-detail-title'; +export const memberDetailEngagement = 'member-detail-engagement'; +export const memberActions = 'member-actions'; +export const memberSubscriptionToggle = 'member-subscription-toggle'; +export const memberSigninUrl = 'member-signin-url'; +export const confirmDeleteMember = 'confirm-delete-member'; +export const cancelDeleteMember = 'cancel-delete-member'; export const memberCustomFieldsField = 'member-custom-fields-field'; export const memberCustomFieldEditModal = 'member-custom-field-edit-modal'; export const importCreateCustomField = 'import-create-custom-field'; diff --git a/packages/testing/test-data/src/selectors/settings.ts b/packages/testing/test-data/src/selectors/settings.ts index 02edd53920e..47153bb967f 100644 --- a/packages/testing/test-data/src/selectors/settings.ts +++ b/packages/testing/test-data/src/selectors/settings.ts @@ -22,6 +22,9 @@ export const copyShareableLink = 'copy-shareable-link'; export const titleAndDescription = 'title-and-description'; export const design = 'design'; export const users = 'users'; +export const ownerUser = 'owner-user'; +export const integrations = 'integrations'; +export const dangerZone = 'dangerzone'; export const portal = 'portal'; export const explore = 'explore'; export const network = 'network'; @@ -49,6 +52,7 @@ export const addNewsletterModal = 'add-newsletter-modal'; export const newsletterModal = 'newsletter-modal'; export const toastInfo = 'toast-info'; export const access = 'access'; +export const siteVisibilitySelect = 'site-visibility-select'; export const siteAccessCode = 'site-access-code'; export const regenerateAccessCode = 'regenerate-access-code'; export const subscriptionAccessSelect = 'subscription-access-select'; @@ -56,6 +60,7 @@ export const defaultPostAccessSelect = 'default-post-access-select'; export const commentingSelect = 'commenting-select'; 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 stripeModal = 'stripe-modal'; export const tiers = 'tiers'; @@ -87,9 +92,12 @@ export const welcomeEmailPreviewSubject = 'welcome-email-preview-subject'; export const welcomeEmailPreviewIframe = 'welcome-email-preview-iframe'; export const welcomeEmailPreviewError = 'welcome-email-preview-error'; export const welcomeEmailPreviewLoading = 'welcome-email-preview-loading'; +export const welcomeEmailEditor = 'welcome-email-editor'; export const freeWelcomeEmailPreview = 'free-welcome-email-preview'; export const freeWelcomeEmailTitle = 'free-welcome-email-title'; +export const freeWelcomeEmailRow = 'free-welcome-email-row'; export const paidWelcomeEmailRow = 'paid-welcome-email-row'; +export const headerImageField = 'header-image-field'; export const testEmailDropdown = 'test-email-dropdown'; export const automationsTransactionalRow = 'automations-transactional-row'; export const embedIframe = 'embed-iframe'; @@ -101,6 +109,15 @@ export const themeListItem = 'theme-list-item'; export const themeCodeEditorModal = 'theme-code-editor-modal'; export const themeEditorConfirmModal = 'theme-editor-confirm-modal'; export const themeEditorInputModal = 'theme-editor-input-modal'; +export const publicationCover = 'publication-cover'; +export const profileImageUpload = 'profile-image-upload'; +export const profileImagePreview = 'profile-image-preview'; +export const coverImageUpload = 'cover-image-upload'; +export const coverImagePreview = 'cover-image-preview'; + +// testids rendered by the Shade image-upload pattern (apps/shade) +export const imageUploadContainer = 'image-upload-container'; +export const imageDeleteButton = 'image-delete-button'; // accessible names export const settingsSearchLabel = 'Search settings'; From 482a86d92b42d58bae067f8a8f84b22a49f1e595 Mon Sep 17 00:00:00 2001 From: Aileen Booker Date: Wed, 26 Aug 2026 22:42:23 +0400 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20multi-domain=20email?= =?UTF-8?q?=20analytics=20cursors=20(#30294)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes [ONC-1965](https://linear.app/ghost/issue/ONC-1965/email-analytics-prevent-a-shared-cursor-from-skipping-capped-sending) During custom sending-domain warming, the custom and fallback Mailgun domains share one analytics cursor. A domain with newer events could advance it even when the other domain stopped at its event limit, leaving unprocessed pages behind on the capped domain. Use the earliest cursor from any capped domain. Domains that reach the end no longer hold the cursor back, and Mailgun request order, filters, pagination, and per-domain limits stay unchanged. Co-authored-by: Steve Larson <9larsons@gmail.com> --- .../email-analytics-service.ts | 20 +- .../email-analytics/fetch-mailgun-events.ts | 2 +- .../server/services/lib/mailgun-client.js | 52 ++++- .../email-analytics-service.test.ts | 95 +++++++++ .../fetch-mailgun-events.test.ts | 15 ++ .../services/lib/mailgun-client.test.js | 187 +++++++++++++++++- 6 files changed, 359 insertions(+), 12 deletions(-) diff --git a/ghost/core/core/server/services/email-analytics/email-analytics-service.ts b/ghost/core/core/server/services/email-analytics/email-analytics-service.ts index ad0560940e4..fcbb7870dc3 100644 --- a/ghost/core/core/server/services/email-analytics/email-analytics-service.ts +++ b/ghost/core/core/server/services/email-analytics/email-analytics-service.ts @@ -24,6 +24,11 @@ type FetchDataScheduled = FetchData & { schedule?: { begin: Date; end: Date } }; type EmailAnalyticsEvent = 'delivered' | 'opened' | 'failed' | 'unsubscribed' | 'complained'; +type FetchEventsResult = { + /** Earliest processed timestamp for a sending domain that stopped at its event limit. */ + safeCursor?: Date; +}; + /** * Names of the jobs this service runs. Each pipeline needs its own set so their * cursors don't overwrite each other in the jobs table. @@ -68,7 +73,7 @@ type FetchEvents = (options: { end: Date; maxEvents: number; events?: EmailAnalyticsEvent[]; -}) => Promise; +}) => Promise; const TRUST_THRESHOLD_MS = 30 * 60 * 1000; // 30 minutes const FETCH_LATEST_END_MARGIN_MS = 1 * 60 * 1000; // Do not fetch events newer than 1 minute (yet). Reduces the chance of having missed events in fetchLatest. @@ -539,14 +544,25 @@ export class EmailAnalyticsService { }; try { - await this.#fetchEvents({ + const fetchResult = await this.#fetchEvents({ batchHandler: processBatch, begin, end, maxEvents, events: eventTypes, }); + + if ( + fetchResult?.safeCursor && + (!fetchData.lastEventTimestamp || fetchResult.safeCursor < fetchData.lastEventTimestamp) + ) { + fetchData.lastEventTimestamp = fetchResult.safeCursor; + } } catch (err) { + // A fetch can process events from one domain before another domain fails. Keep the + // in-memory cursor at the start of this run so the next attempt retries every domain. + fetchData.lastEventTimestamp = begin; + if (!(err instanceof Error) || err.message !== 'Fetching canceled') { logging.error('[EmailAnalytics] Error while fetching'); logging.error(err); diff --git a/ghost/core/core/server/services/email-analytics/fetch-mailgun-events.ts b/ghost/core/core/server/services/email-analytics/fetch-mailgun-events.ts index 9af38e6e660..cda02b5c678 100644 --- a/ghost/core/core/server/services/email-analytics/fetch-mailgun-events.ts +++ b/ghost/core/core/server/services/email-analytics/fetch-mailgun-events.ts @@ -9,7 +9,7 @@ type FetchMailgunEventsOptions = { settings: { get: (key: string) => unknown }; tags: string[]; batchHandler: Function; - /** Not a strict maximum. We stop fetching after we reached the maximum AND received at least one event after begin (not equal) to prevent deadlocks. */ + /** Per-domain soft maximum. We stop fetching a domain after we reached the maximum AND received at least one event after begin (not equal) to prevent deadlocks. */ maxEvents?: number; begin?: Date; end?: Date; diff --git a/ghost/core/core/server/services/lib/mailgun-client.js b/ghost/core/core/server/services/lib/mailgun-client.js index c533edb899a..618f9a5a50e 100644 --- a/ghost/core/core/server/services/lib/mailgun-client.js +++ b/ghost/core/core/server/services/lib/mailgun-client.js @@ -165,8 +165,8 @@ module.exports = class MailgunClient { * @param {Object} mailgunOptions * @param {Function} batchHandler * @param {Object} options - * @param {number} [options.maxEvents] Not a strict maximum. We stop fetching after we reached the maximum AND received at least one event after begin (not equal) to prevent deadlocks. - * @returns {Promise} + * @param {number} [options.maxEvents] Per-domain soft maximum. We stop fetching a domain after we reached the maximum AND received at least one event after begin (not equal) to prevent deadlocks. + * @returns {Promise<{safeCursor?: Date} | void>} */ async fetchEvents(mailgunOptions, batchHandler, { maxEvents = Infinity } = {}) { const mailgunInstance = this.getInstance(); @@ -184,12 +184,33 @@ module.exports = class MailgunClient { // Determine which domains to fetch from const domains = this.#getDomainsToFetch(mailgunConfig); + const cappedDomainCursors = []; + // Fetch events from each domain for (const domain of domains) { - await this.#fetchEventsFromDomain(domain, mailgunInstance, mailgunOptions, batchHandler, { - maxEvents, - }); + const result = await this.#fetchEventsFromDomain( + domain, + mailgunInstance, + mailgunOptions, + batchHandler, + { maxEvents }, + ); + + if ( + result.capped && + result.lastEventTimestamp && + Number.isFinite(result.lastEventTimestamp.getTime()) + ) { + cappedDomainCursors.push(result.lastEventTimestamp); + } } + + const safeCursor = cappedDomainCursors.reduce( + (earliest, cursor) => (!earliest || cursor < earliest ? cursor : earliest), + undefined, + ); + + return { safeCursor }; } /** @@ -220,7 +241,7 @@ module.exports = class MailgunClient { * @param {Function} batchHandler * @param {Object} options * @param {number} options.maxEvents - * @returns {Promise} + * @returns {Promise<{capped: boolean, eventCount: number, lastEventTimestamp?: Date}>} */ async #fetchEventsFromDomain( domain, @@ -235,6 +256,9 @@ module.exports = class MailgunClient { let batchCount = 0; let totalBatchTime = 0; + let eventCount = 0; + let lastEventTimestamp; + let capped = false; try { let page = await this.getEventsFromMailgun(mailgunInstance, domain, mailgunOptions); @@ -248,7 +272,6 @@ module.exports = class MailgunClient { `[MailgunClient fetchEventsFromDomain ${domain}]: finished fetching first page with ${events.length} events`, ); - let eventCount = 0; const beginTimestamp = mailgunOptions.begin ? Math.ceil(mailgunOptions.begin * 1000) : undefined; // ceil here if we have rounding errors @@ -263,6 +286,13 @@ module.exports = class MailgunClient { totalBatchTime += batchDuration; eventCount += events.length; + const batchLastEventTimestamp = events[events.length - 1].timestamp; + if ( + batchLastEventTimestamp && + (!lastEventTimestamp || batchLastEventTimestamp > lastEventTimestamp) + ) { + lastEventTimestamp = batchLastEventTimestamp; + } if ( eventCount >= maxEvents && @@ -270,6 +300,7 @@ module.exports = class MailgunClient { !events[events.length - 1].timestamp || events[events.length - 1].timestamp.getTime() > beginTimestamp) ) { + capped = true; break; } @@ -294,10 +325,15 @@ module.exports = class MailgunClient { const overallEndTime = Date.now(); const totalDuration = overallEndTime - overallStartTime; const averageBatchTime = batchCount > 0 ? totalBatchTime / batchCount : 0; + const cursor = Number.isFinite(lastEventTimestamp?.getTime()) + ? lastEventTimestamp.toISOString() + : 'none'; logging.info( - `[MailgunClient fetchEventsFromDomain ${domain}]: Processed ${batchCount} batches in ${(totalDuration / 1000).toFixed(2)}s. Average batch time: ${(averageBatchTime / 1000).toFixed(2)}s`, + `[MailgunClient fetchEventsFromDomain ${domain}]: Processed ${eventCount} events in ${batchCount} batches in ${(totalDuration / 1000).toFixed(2)}s. Average batch time: ${(averageBatchTime / 1000).toFixed(2)}s. Status: ${capped ? 'capped' : 'exhausted'}. Cursor: ${cursor}`, ); + + return { capped, eventCount, lastEventTimestamp }; } catch (error) { logging.error(`[MailgunClient fetchEventsFromDomain ${domain}]: Error fetching events`); logging.error(error); diff --git a/ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.ts b/ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.ts index b3fe7d8bc1a..cddb0d92e38 100644 --- a/ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.ts +++ b/ghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.ts @@ -740,6 +740,35 @@ describe('EmailAnalyticsService', function () { ); }); + it('retries from the original cursor after a fetch error', async function () { + const initialCursor = new Date(Date.now() - 10 * 60 * 1000); + const fetchBegins: Date[] = []; + const eventProcessor = createStubEventProcessor(); + eventProcessor.processBatch.callsFake(async (_events, _result, fetchData) => { + fetchData.lastEventTimestamp = new Date(initialCursor.getTime() + 1000); + }); + const fetchEvents = sinon.stub().callsFake(async ({ batchHandler, begin }) => { + fetchBegins.push(begin); + await batchHandler([{ timestamp: new Date(initialCursor.getTime() + 1000) }]); + throw new Error('fallback fetch failed'); + }); + const service = createService({ + queries: { + getLastEventTimestamp: sinon.stub().resolves(initialCursor), + setJobTimestamp: sinon.stub().resolves(), + setJobStatus: sinon.stub().resolves(), + }, + fetchEvents, + createEventProcessor: () => eventProcessor, + }); + + await assert.rejects(service.fetchLatestNonOpenedEvents(), /fallback fetch failed/); + await assert.rejects(service.fetchLatestNonOpenedEvents(), /fallback fetch failed/); + + assert.deepEqual(fetchBegins, [initialCursor, initialCursor]); + assert.deepEqual(service.getStatus().latest.lastEventTimestamp, initialCursor); + }); + it('persists and advances the last processed event timestamp', async function () { const lastEventTimestamp = new Date(Date.now() - 10_000); const setJobTimestamp = sinon.stub().resolves(); @@ -772,6 +801,72 @@ describe('EmailAnalyticsService', function () { new Date(lastEventTimestamp.getTime() + 1000), ); }); + + it('resumes from a capped sending domain until all of its events are processed', async function () { + const newerDomainTimestamp = new Date(Date.now() - 70_000); + const fallbackTimestamps = [ + new Date(Date.now() - 120_000), + new Date(Date.now() - 119_000), + new Date(Date.now() - 118_000), + new Date(Date.now() - 117_000), + ]; + const setJobTimestamp = sinon.stub().resolves(); + const processedEventIds = new Set(); + const eventProcessor = createStubEventProcessor(); + eventProcessor.processBatch.callsFake(async (events, _result, fetchData) => { + for (const event of events) { + processedEventIds.add(event.id); + if (!fetchData.lastEventTimestamp || event.timestamp > fetchData.lastEventTimestamp) { + fetchData.lastEventTimestamp = event.timestamp; + } + } + }); + const service = createService({ + queries: { + getLastEventTimestamp: sinon.stub().resolves(), + setJobTimestamp, + setJobStatus: sinon.stub().resolves(), + }, + fetchEvents: async ({ batchHandler, begin }) => { + const fetchIndex = fetchBegins.push(begin) - 1; + const firstFallbackIndex = fetchIndex; + const safeCursor = fallbackTimestamps[firstFallbackIndex + 1]; + + await batchHandler([{ id: 'primary-10', timestamp: newerDomainTimestamp }]); + await batchHandler([ + { + id: `fallback-${firstFallbackIndex + 1}`, + timestamp: fallbackTimestamps[firstFallbackIndex], + }, + { + id: `fallback-${firstFallbackIndex + 2}`, + timestamp: safeCursor, + }, + ]); + return { safeCursor }; + }, + createEventProcessor: () => eventProcessor, + }); + const fetchBegins: Date[] = []; + + await service.fetchLatestNonOpenedEvents({ maxEvents: 2 }); + await service.fetchLatestNonOpenedEvents({ maxEvents: 2 }); + await service.fetchLatestNonOpenedEvents({ maxEvents: 2 }); + + assert.deepEqual( + setJobTimestamp + .getCalls() + .filter((call) => call.args[1] === 'finished') + .map((call) => call.args[2]), + fallbackTimestamps.slice(1), + ); + assert.deepEqual(fetchBegins.slice(1), fallbackTimestamps.slice(1, 3)); + assert.deepEqual( + [...processedEventIds], + ['primary-10', 'fallback-1', 'fallback-2', 'fallback-3', 'fallback-4'], + ); + assert.deepEqual(service.getStatus().latest.lastEventTimestamp, fallbackTimestamps[3]); + }); }); describe('restoreScheduled', function () { diff --git a/ghost/core/test/unit/server/services/email-analytics/fetch-mailgun-events.test.ts b/ghost/core/test/unit/server/services/email-analytics/fetch-mailgun-events.test.ts index 416e106e5ef..ac25bca45d0 100644 --- a/ghost/core/test/unit/server/services/email-analytics/fetch-mailgun-events.test.ts +++ b/ghost/core/test/unit/server/services/email-analytics/fetch-mailgun-events.test.ts @@ -1,3 +1,4 @@ +import assert from 'node:assert/strict'; import sinon from 'sinon'; // @ts-expect-error This module lacks type definitions. @@ -58,6 +59,20 @@ describe('fetchMailgunEvents', function () { }); }); + it('returns the domain-safe cursor from the Mailgun client', async function () { + const safeCursor = new Date('Thu Feb 25 2021 13:00:00 GMT+0000'); + sinon.stub(MailgunClient.prototype, 'fetchEvents').resolves({ safeCursor }); + + const result = await fetchMailgunEvents({ + config, + settings, + tags: DEFAULT_TAGS, + batchHandler: sinon.spy(), + }); + + assert.deepEqual(result, { safeCursor }); + }); + it('uses supplied end timestamp and max events', async function () { const { batchHandler, mailgunFetchEventsStub } = await fetchEvents({ begin: LATEST_TIMESTAMP, diff --git a/ghost/core/test/unit/server/services/lib/mailgun-client.test.js b/ghost/core/test/unit/server/services/lib/mailgun-client.test.js index dc4d2d0d6e9..4a435caac2a 100644 --- a/ghost/core/test/unit/server/services/lib/mailgun-client.test.js +++ b/ghost/core/test/unit/server/services/lib/mailgun-client.test.js @@ -30,6 +30,31 @@ const createBatchCounter = (customHandler) => { return batchCounter; }; +const createEventsPage = ({ domain, nextPage, timestamps }) => ({ + items: timestamps.map((timestamp, index) => ({ + event: 'delivered', + recipient: `recipient${index}@example.com`, + 'user-variables': { + 'email-id': '5fbe5d9607bdfa3765dc3819', + }, + message: { + headers: { + 'message-id': `message-${index}@${domain}`, + }, + }, + timestamp, + })), + paging: { + next: `https://api.mailgun.net/v3/${domain}/events/${nextPage}`, + }, +}); + +const mockEventsPage = ({ domain, mailgunOptions, nextPage, page, timestamps }) => + nock('https://api.mailgun.net') + .get(`/v3/${domain}/events${page ? `/${page}` : ''}`) + .query(mailgunOptions) + .reply(200, createEventsPage({ domain, nextPage, timestamps })); + describe('MailgunClient', function () { let config, settings; @@ -1119,6 +1144,165 @@ describe('MailgunClient', function () { nock.cleanAll(); }); + it('returns the capped fallback cursor when the primary domain is exhausted further ahead', async function () { + setupDomainWarmingConfig(true); + + const begin = 1606399300; + const mailgunOptions = { ...MAILGUN_OPTIONS, begin }; + + const primaryPageMock = mockEventsPage({ + domain: 'primary.com', + mailgunOptions, + nextPage: 'primary-next', + timestamps: [begin + 10], + }); + const primaryEmptyPageMock = mockEventsPage({ + domain: 'primary.com', + mailgunOptions, + nextPage: 'primary-empty', + page: 'primary-next', + timestamps: [], + }); + const fallbackPageMock = mockEventsPage({ + domain: 'fallback.com', + mailgunOptions, + nextPage: 'fallback-unconsumed', + timestamps: [begin + 1, begin + 2], + }); + const fallbackNextPageMock = mockEventsPage({ + domain: 'fallback.com', + mailgunOptions, + nextPage: 'fallback-empty', + page: 'fallback-unconsumed', + timestamps: [begin + 3], + }); + + const processedTimestamps = []; + + const result = await new MailgunClient({ config, settings }).fetchEvents( + mailgunOptions, + (events) => { + processedTimestamps.push(...events.map((event) => event.timestamp)); + }, + { maxEvents: 2 }, + ); + + assert.equal(primaryPageMock.isDone(), true); + assert.equal(primaryEmptyPageMock.isDone(), true); + assert.equal(fallbackPageMock.isDone(), true); + assert.equal(fallbackNextPageMock.isDone(), false); + assert.deepEqual(processedTimestamps, [ + new Date((begin + 10) * 1000), + new Date((begin + 1) * 1000), + new Date((begin + 2) * 1000), + ]); + assert.deepEqual(result, { + safeCursor: new Date((begin + 2) * 1000), + }); + }); + + it('returns the capped primary cursor when the fallback domain is exhausted further ahead', async function () { + setupDomainWarmingConfig(true); + + const begin = 1606399300; + const mailgunOptions = { ...MAILGUN_OPTIONS, begin }; + + const primaryPageMock = mockEventsPage({ + domain: 'primary.com', + mailgunOptions, + nextPage: 'primary-unconsumed', + timestamps: [begin + 1, begin + 2], + }); + const primaryNextPageMock = mockEventsPage({ + domain: 'primary.com', + mailgunOptions, + nextPage: 'primary-empty', + page: 'primary-unconsumed', + timestamps: [begin + 3], + }); + const fallbackPageMock = mockEventsPage({ + domain: 'fallback.com', + mailgunOptions, + nextPage: 'fallback-next', + timestamps: [begin + 10], + }); + const fallbackEmptyPageMock = mockEventsPage({ + domain: 'fallback.com', + mailgunOptions, + nextPage: 'fallback-empty', + page: 'fallback-next', + timestamps: [], + }); + + const processedTimestamps = []; + const result = await new MailgunClient({ config, settings }).fetchEvents( + mailgunOptions, + (events) => { + processedTimestamps.push(...events.map((event) => event.timestamp)); + }, + { maxEvents: 2 }, + ); + + assert.equal(primaryPageMock.isDone(), true); + assert.equal(primaryNextPageMock.isDone(), false); + assert.equal(fallbackPageMock.isDone(), true); + assert.equal(fallbackEmptyPageMock.isDone(), true); + assert.deepEqual(processedTimestamps, [ + new Date((begin + 1) * 1000), + new Date((begin + 2) * 1000), + new Date((begin + 10) * 1000), + ]); + assert.deepEqual(result, { + safeCursor: new Date((begin + 2) * 1000), + }); + }); + + it('returns the earliest capped cursor when both domains reach their limits', async function () { + setupDomainWarmingConfig(true); + + const begin = 1606399300; + const mailgunOptions = { ...MAILGUN_OPTIONS, begin }; + + mockEventsPage({ + domain: 'primary.com', + mailgunOptions, + nextPage: 'primary-unconsumed', + timestamps: [begin + 5, begin + 6], + }); + const primaryNextPageMock = mockEventsPage({ + domain: 'primary.com', + mailgunOptions, + nextPage: 'primary-empty', + page: 'primary-unconsumed', + timestamps: [begin + 7], + }); + mockEventsPage({ + domain: 'fallback.com', + mailgunOptions, + nextPage: 'fallback-unconsumed', + timestamps: [begin + 1, begin + 2], + }); + const fallbackNextPageMock = mockEventsPage({ + domain: 'fallback.com', + mailgunOptions, + nextPage: 'fallback-empty', + page: 'fallback-unconsumed', + timestamps: [begin + 3], + }); + + const result = await new MailgunClient({ config, settings }).fetchEvents( + mailgunOptions, + () => {}, + { maxEvents: 2 }, + ); + + assert.equal(primaryNextPageMock.isDone(), false); + assert.equal(fallbackNextPageMock.isDone(), false); + assert.deepEqual(result, { + safeCursor: new Date((begin + 2) * 1000), + }); + }); + it('fetches from both primary and fallback domains when enabled', async function () { setupDomainWarmingConfig(true); @@ -1141,12 +1325,13 @@ describe('MailgunClient', function () { const counter = createBatchCounter(); const mailgunClient = new MailgunClient({ config, settings }); - await mailgunClient.fetchEvents(MAILGUN_OPTIONS, counter.batchHandler); + const result = await mailgunClient.fetchEvents(MAILGUN_OPTIONS, counter.batchHandler); assert.equal(primaryMock.isDone(), true); assert.equal(fallbackMock.isDone(), true); assert.equal(counter.batches, 2); assert.equal(counter.events, 6); + assert.deepEqual(result, { safeCursor: undefined }); }); it('only fetches from primary when disabled', async function () { From ff9685ad05d8fe6f84bcc0d29d3f58a28e86da44 Mon Sep 17 00:00:00 2001 From: Rob Lester Date: Wed, 26 Aug 2026 11:38:52 +0100 Subject: [PATCH 5/6] Added a tier's checkout configuration and the bindings it writes through A publisher can now say what a tier's checkout asks for beyond the payment, and where each answer is kept. The answer to where is a binding: it names the source that collects, the source's own word for the thing, and the field the value lands in. Everything that writes into a member's custom fields goes through one, so what writes into a field is a single join rather than a different one per writer. Where a value lands and whether it is being collected are one row, because holding them apart in two tables let them disagree, with no composite foreign key or check constraint able to stop it: a tier could ask for something with nowhere to put it, or go on receiving what a publisher had turned off. The row is the collecting. There is one and the source writes through it, or there is none and it does not. Where each value lands is stated by the request rather than worked out on the publisher's behalf. A field the site already keeps under that key has to be active and hold the kind of thing the port supplies; a key it does not keep yet is made into a field, listed under the name the port carries. Anything else is refused, because a checkout quietly collecting into somewhere other than the field a publisher named is worse than one that says it cannot. Nothing sends any of this to a payment processor yet, and every route that changes it is behind the members custom fields flag. ref https://linear.app/ghost/issue/BER-3872 --- ghost/core/core/server/api/endpoints/index.js | 4 + .../api/endpoints/tiers-checkout-config.ts | 76 ++ .../utils/serializers/output/index.js | 4 + .../output/tiers-checkout-config.ts | 17 + .../core/server/data/exporter/table-lists.js | 3 + ...add-members-custom-field-bindings-table.js | 26 + ...14-48-06-add-tier-checkout-field-tables.js | 35 + ghost/core/core/server/data/schema/schema.js | 73 ++ .../members-custom-fields/bindings-service.ts | 151 +++ .../definitions-service.ts | 145 ++- .../services/members-custom-fields/index.ts | 26 +- .../services/members-custom-fields/queries.ts | 14 + .../services/members-custom-fields/schema.ts | 25 + .../stripe/services/checkout/field-ports.ts | 38 + .../services/tier-checkout-config/codec.ts | 112 ++ .../tier-checkout-config/destinations.ts | 17 + .../services/tier-checkout-config/index.ts | 24 + .../services/tier-checkout-config/models.ts | 71 ++ .../services/tier-checkout-config/queries.ts | 98 ++ .../services/tier-checkout-config/schema.ts | 53 + .../tier-checkout-config/serializers.ts | 135 +++ .../services/tier-checkout-config/service.ts | 420 +++++++ .../core/server/services/tiers/service.js | 21 + .../server/web/api/endpoints/admin/routes.js | 37 +- .../admin/member-custom-fields.test.ts | 45 +- .../admin/tiers-checkout-config.test.ts | 1063 +++++++++++++++++ .../integration/exporter/exporter.test.js | 3 + .../unit/server/data/schema/integrity.test.js | 2 +- 28 files changed, 2695 insertions(+), 43 deletions(-) create mode 100644 ghost/core/core/server/api/endpoints/tiers-checkout-config.ts create mode 100644 ghost/core/core/server/api/endpoints/utils/serializers/output/tiers-checkout-config.ts create mode 100644 ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-38-53-add-members-custom-field-bindings-table.js create mode 100644 ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-48-06-add-tier-checkout-field-tables.js create mode 100644 ghost/core/core/server/services/members-custom-fields/bindings-service.ts create mode 100644 ghost/core/core/server/services/stripe/services/checkout/field-ports.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/codec.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/destinations.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/index.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/models.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/queries.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/schema.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/serializers.ts create mode 100644 ghost/core/core/server/services/tier-checkout-config/service.ts create mode 100644 ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts diff --git a/ghost/core/core/server/api/endpoints/index.js b/ghost/core/core/server/api/endpoints/index.js index 94db512857e..430bedcc601 100644 --- a/ghost/core/core/server/api/endpoints/index.js +++ b/ghost/core/core/server/api/endpoints/index.js @@ -116,6 +116,10 @@ module.exports = { return apiFramework.pipeline(require('./member-custom-fields'), localUtils); }, + get tiersCheckoutConfig() { + return apiFramework.pipeline(require('./tiers-checkout-config'), localUtils); + }, + get memberCommenting() { return apiFramework.pipeline(require('./member-commenting'), localUtils); }, diff --git a/ghost/core/core/server/api/endpoints/tiers-checkout-config.ts b/ghost/core/core/server/api/endpoints/tiers-checkout-config.ts new file mode 100644 index 00000000000..98d960a2a4c --- /dev/null +++ b/ghost/core/core/server/api/endpoints/tiers-checkout-config.ts @@ -0,0 +1,76 @@ +import { actingContext } from '../../services/members-custom-fields'; +import { emptyCheckoutConfig } from '../../services/tier-checkout-config'; +import type { TierCheckoutConfig } from '../../services/tier-checkout-config'; + +const tiersService = require('../../services/tiers'); + +interface Frame { + data: { tiers_checkout_config?: unknown[] }; + options: { id: string; context: unknown; [key: string]: unknown }; +} + +export type TierCheckoutResult = TierCheckoutConfig[]; + +/** + * Reads one tier's checkout settings for the API to return. + * + * The service answers with null when nobody has ever set up checkout for the tier, because + * there is genuinely nothing stored for it. The API still answers with a resource: the tier + * itself exists, so a client asking what it collects should be told "nothing" in the same + * shape as any other answer, rather than getting a 404 or an empty body to puzzle over. + */ +async function forTier(id: string): Promise { + return (await tiersService.checkout.read(id)) ?? emptyCheckoutConfig(id); +} + +/** + * A tier's checkout configuration, as a sub-resource of the tier rather than an attribute + * of it. + * + * The tier resource is generally available and this concept is not, so putting it on the + * tier payload would add a key to every tier response on every site whether or not the + * feature is on. A route of its own can carry the flag, and be removed with it. + * + * Every operation here is one call. A tier's configuration is one shape the service hands + * out and takes back, and that it spans tables, that destinations are site-wide, and that a + * binding exists at all are facts about the inside of that domain. + */ +const controller = { + docName: 'tiers_checkout_config', + + browse: { + headers: { cacheInvalidate: false }, + permissions: { docName: 'products', method: 'browse' }, + query(): Promise { + return tiersService.checkout.browse(); + }, + }, + + read: { + headers: { cacheInvalidate: false }, + options: ['id'], + validation: { options: { id: { required: true } } }, + permissions: { docName: 'products', method: 'read' }, + async query(frame: Frame): Promise { + return [await forTier(frame.options.id)]; + }, + }, + + edit: { + headers: { cacheInvalidate: true }, + options: ['id'], + validation: { options: { id: { required: true } } }, + permissions: { docName: 'products', method: 'edit' }, + async query(frame: Frame): Promise { + await tiersService.checkout.edit( + actingContext(frame.options.context), + frame.options.id, + frame.data.tiers_checkout_config?.[0] ?? {}, + ); + return [await forTier(frame.options.id)]; + }, + }, +}; + +// module.exports (not export): the API framework loads controllers via require(). +module.exports = controller; diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js b/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js index 9645b314584..ce83186c76d 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/index.js @@ -84,6 +84,10 @@ module.exports = { return require('./tiers'); }, + get tiers_checkout_config() { + return require('./tiers-checkout-config'); + }, + get images() { return require('./images'); }, diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/tiers-checkout-config.ts b/ghost/core/core/server/api/endpoints/utils/serializers/output/tiers-checkout-config.ts new file mode 100644 index 00000000000..c252654e5a6 --- /dev/null +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/tiers-checkout-config.ts @@ -0,0 +1,17 @@ +import { toCheckoutConfigResponse } from '../../../../../services/tier-checkout-config'; +import type { TierCheckoutConfig } from '../../../../../services/tier-checkout-config'; + +interface Frame { + response?: unknown; +} + +const serialize = (configs: TierCheckoutConfig[], _apiConfig: unknown, frame: Frame): void => { + frame.response = toCheckoutConfigResponse.parse(configs); +}; + +// module.exports (not export): the API framework loads serializers via require(). +module.exports = { + browse: serialize, + read: serialize, + edit: serialize, +}; diff --git a/ghost/core/core/server/data/exporter/table-lists.js b/ghost/core/core/server/data/exporter/table-lists.js index 5f9141c3c84..a570b54ba2f 100644 --- a/ghost/core/core/server/data/exporter/table-lists.js +++ b/ghost/core/core/server/data/exporter/table-lists.js @@ -44,6 +44,9 @@ const BACKUP_TABLES = [ 'members_newsletters', 'members_custom_fields', 'members_custom_field_values', + 'members_custom_field_bindings', + 'products_checkout_fields', + 'products_checkout_config', 'mentions', 'comments', 'comment_likes', diff --git a/ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-38-53-add-members-custom-field-bindings-table.js b/ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-38-53-add-members-custom-field-bindings-table.js new file mode 100644 index 00000000000..013d0894ad7 --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-38-53-add-members-custom-field-bindings-table.js @@ -0,0 +1,26 @@ +const { addTable } = require('../../utils'); + +module.exports = addTable('members_custom_field_bindings', { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + product_id: { + type: 'string', + maxlength: 24, + nullable: false, + references: 'products.id', + cascadeDelete: true, + }, + port: { type: 'string', maxlength: 191, nullable: false }, + custom_field_key: { + type: 'string', + maxlength: 191, + nullable: false, + references: 'members_custom_fields.key', + cascadeDelete: true, + }, + created_at: { type: 'dateTime', nullable: false }, + updated_at: { type: 'dateTime', nullable: true }, + '@@UNIQUE_CONSTRAINTS@@': [ + { columns: ['product_id', 'port'], indexName: 'members_custom_field_bindings_unique' }, + ], + '@@INDEXES@@': [['custom_field_key']], +}); diff --git a/ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-48-06-add-tier-checkout-field-tables.js b/ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-48-06-add-tier-checkout-field-tables.js new file mode 100644 index 00000000000..f3c6ad787c2 --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/6.61/2026-08-19-14-48-06-add-tier-checkout-field-tables.js @@ -0,0 +1,35 @@ +const { combineNonTransactionalMigrations, addTable } = require('../../utils'); + +module.exports = combineNonTransactionalMigrations( + addTable('products_checkout_fields', { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + binding_id: { + type: 'string', + maxlength: 24, + nullable: false, + unique: true, + references: 'members_custom_field_bindings.id', + cascadeDelete: true, + }, + sort_order: { type: 'integer', nullable: false, unsigned: true, defaultTo: 0 }, + label: { type: 'string', maxlength: 191, nullable: true }, + optional: { type: 'boolean', nullable: false, defaultTo: true }, + created_at: { type: 'dateTime', nullable: false }, + updated_at: { type: 'dateTime', nullable: true }, + }), + addTable('products_checkout_config', { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + product_id: { + type: 'string', + maxlength: 24, + nullable: false, + unique: true, + references: 'products.id', + cascadeDelete: true, + }, + shipping_allowed_countries: { type: 'string', maxlength: 2000, nullable: true }, + tax_number_collect: { type: 'boolean', nullable: false, defaultTo: false }, + created_at: { type: 'dateTime', nullable: false }, + updated_at: { type: 'dateTime', nullable: true }, + }), +); diff --git a/ghost/core/core/server/data/schema/schema.js b/ghost/core/core/server/data/schema/schema.js index ade08d0f8e9..c9e4234ef83 100644 --- a/ghost/core/core/server/data/schema/schema.js +++ b/ghost/core/core/server/data/schema/schema.js @@ -1064,6 +1064,79 @@ module.exports = { created_at: { type: 'dateTime', nullable: false }, updated_at: { type: 'dateTime', nullable: true }, }, + // Where a source sends what it collected. The source is who collects, the port is that + // source's own name for the thing, and the destination is the publisher's field, which + // they can repoint without the source knowing. The row is the collecting: there is one + // and the source writes through it, or there is none and it does not. + // + // A second kind of source becomes a `source_type` beside a widened id. What it must not + // become is a second table holding destinations. + members_custom_field_bindings: { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + product_id: { + type: 'string', + maxlength: 24, + nullable: false, + references: 'products.id', + cascadeDelete: true, + }, + port: { type: 'string', maxlength: 191, nullable: false }, + // Indexed rather than unique: several sources landing in one field is expected. + custom_field_key: { + type: 'string', + maxlength: 191, + nullable: false, + references: 'members_custom_fields.key', + cascadeDelete: true, + }, + created_at: { type: 'dateTime', nullable: false }, + updated_at: { type: 'dateTime', nullable: true }, + '@@UNIQUE_CONSTRAINTS@@': [ + { columns: ['product_id', 'port'], indexName: 'members_custom_field_bindings_unique' }, + ], + '@@INDEXES@@': [['custom_field_key']], + }, + // How a tier's checkout question is asked. Where the answer lands is the binding it + // hangs off. + products_checkout_fields: { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + binding_id: { + type: 'string', + maxlength: 24, + nullable: false, + unique: true, + references: 'members_custom_field_bindings.id', + cascadeDelete: true, + }, + sort_order: { type: 'integer', nullable: false, unsigned: true, defaultTo: 0 }, + // Processors cap a label far shorter than a field name may be. Null asks under the + // field's own name. + label: { type: 'string', maxlength: 191, nullable: true }, + optional: { type: 'boolean', nullable: false, defaultTo: true }, + created_at: { type: 'dateTime', nullable: false }, + updated_at: { type: 'dateTime', nullable: true }, + }, + // The options a tier's collection needs, and the one thing it collects without keeping. + // Whether it collects anything it *does* keep is the binding above. + products_checkout_config: { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + product_id: { + type: 'string', + maxlength: 24, + nullable: false, + unique: true, + references: 'products.id', + cascadeDelete: true, + }, + // ISO 3166-1 alpha-2, comma-joined. A processor will not render an address form + // without them, and a wrong code fails the session create. + shipping_allowed_countries: { type: 'string', maxlength: 2000, nullable: true }, + // Stripe keeps a tax number against the customer it invoices, so there is no + // destination to bind and nothing to record but whether to ask. + tax_number_collect: { type: 'boolean', nullable: false, defaultTo: false }, + created_at: { type: 'dateTime', nullable: false }, + updated_at: { type: 'dateTime', nullable: true }, + }, members_custom_field_values: { id: { type: 'string', maxlength: 24, nullable: false, primary: true }, // The field's stable key, not its id: a value is addressed by key everywhere it diff --git a/ghost/core/core/server/services/members-custom-fields/bindings-service.ts b/ghost/core/core/server/services/members-custom-fields/bindings-service.ts new file mode 100644 index 00000000000..afe5b66d899 --- /dev/null +++ b/ghost/core/core/server/services/members-custom-fields/bindings-service.ts @@ -0,0 +1,151 @@ +import ObjectID from 'bson-objectid'; +import logging from '@tryghost/logging'; +import type { Knex } from 'knex'; +import type { FieldType } from '@tryghost/custom-field-types'; +import { DbBoundField, FIELD_STATUS } from './schema'; +import type { CustomFieldValuesService, PlannedWrite } from './values-service'; + +const FIELDS_TABLE = 'members_custom_fields'; +const BINDINGS_TABLE = 'members_custom_field_bindings'; + +export interface BoundField { + bindingId: string; + key: string; + type: FieldType; +} + +/** + * Where a source sends what it collected: a `port` is the name that source uses for a + * thing, and the binding resolves it to one of the publisher's fields. + */ +export class CustomFieldBindingsService { + private knex: Knex; + private values: CustomFieldValuesService; + + constructor({ knex, values }: { knex: Knex; values: CustomFieldValuesService }) { + this.knex = knex; + this.values = values; + } + + async bind( + db: Knex, + productId: string, + port: string, + customFieldKey: string, + now: Date, + ): Promise { + const existing = await db(BINDINGS_TABLE).where({ product_id: productId, port }).first(); + if (existing?.custom_field_key === customFieldKey) { + await db(BINDINGS_TABLE).where('id', existing.id).update({ updated_at: now }); + return existing.id; + } + if (existing) { + await db(BINDINGS_TABLE).where('id', existing.id).del(); + } + + const bindingId = new ObjectID().toHexString(); + await db(BINDINGS_TABLE).insert({ + id: bindingId, + product_id: productId, + port, + custom_field_key: customFieldKey, + created_at: now, + updated_at: now, + }); + return bindingId; + } + + /** Stops the writing. Whatever hangs off the binding cascades with it. */ + async remove(db: Knex, productId: string, port: string): Promise { + await db(BINDINGS_TABLE).where({ product_id: productId, port }).del(); + } + + /** + * Values arrive in the order they are to be applied: where two land in one field, the + * last of them is what the field holds. + */ + async writeCollected( + memberId: string, + productId: string, + collected: Array<{ port: string; value: unknown }>, + ): Promise { + for (const { port, value } of collected) { + const destination = await this.resolve(productId, port); + if (!destination) { + continue; + } + await this.writeOne(memberId, destination, value); + } + } + + private async writeOne(memberId: string, into: BoundField, value: unknown): Promise { + let planned: PlannedWrite[]; + try { + planned = await this.values.planWrite({ [into.key]: value }); + } catch (err) { + logging.warn( + { + event: { name: 'members.custom_fields.collected_value_rejected' }, + err, + memberId, + customFieldKey: into.key, + }, + 'A collected value could not be saved', + ); + return; + } + + try { + await this.values.applyWrite(memberId, planned, { + writtenBy: { type: 'binding', id: into.bindingId }, + }); + } catch (err) { + logging.error( + { + event: { name: 'members.custom_fields.collected_value_write_failed' }, + err, + memberId, + customFieldKey: into.key, + bindingId: into.bindingId, + }, + 'Failed to store a collected custom field value', + ); + } + } + + private async resolve(productId: string, port: string): Promise { + const row = await this.knex(BINDINGS_TABLE) + .join(FIELDS_TABLE, `${BINDINGS_TABLE}.custom_field_key`, `${FIELDS_TABLE}.key`) + .where(`${BINDINGS_TABLE}.product_id`, productId) + .where(`${BINDINGS_TABLE}.port`, port) + // An archived destination is still where this goes, and still not somewhere a value + // can land, so the write drops rather than waiting. + .where(`${FIELDS_TABLE}.status`, FIELD_STATUS.active) + .select(`${BINDINGS_TABLE}.id as binding_id`, `${FIELDS_TABLE}.key`, `${FIELDS_TABLE}.type`) + .first(); + + if (!row) { + return null; + } + + // Decoded rather than trusted: a join is a read boundary, and `type` is what decides + // how the collected value is read. Unreadable counts as unresolved rather than + // throwing, so one bad row skips its value the way an unbound port does instead of + // failing everything else the same checkout collected. + const bound = DbBoundField.safeParse(row); + if (!bound.success) { + logging.warn( + { + event: { name: 'members.custom_fields.binding_unreadable' }, + err: bound.error, + productId, + port, + }, + 'A binding could not be read', + ); + return null; + } + + return { bindingId: bound.data.binding_id, key: bound.data.key, type: bound.data.type }; + } +} diff --git a/ghost/core/core/server/services/members-custom-fields/definitions-service.ts b/ghost/core/core/server/services/members-custom-fields/definitions-service.ts index fc926b7dea4..aba034b8c0f 100644 --- a/ghost/core/core/server/services/members-custom-fields/definitions-service.ts +++ b/ghost/core/core/server/services/members-custom-fields/definitions-service.ts @@ -3,11 +3,11 @@ import errors from '@tryghost/errors'; import type { Knex } from 'knex'; import { z } from 'zod'; import { CustomField } from './models'; -import { FieldTypeSchema } from '@tryghost/custom-field-types'; +import { FieldTypeSchema, type FieldType } from '@tryghost/custom-field-types'; import { customFieldCodec } from './codec'; import { FIELD_STATUS, FieldStatusSchema } from './schema'; -import { activeFields, inFieldOrder } from './queries'; -import { mintableKey } from './key'; +import { activeFields, fieldByKey, inFieldOrder, type DefinitionQuery } from './queries'; +import { KEY_CHARACTERS, mintableKey } from './key'; import { type RecordCustomFieldAction, type RequestContext } from './actions'; // The same NQL -> knex bridge Bookshelf's filter plugin uses, applied directly to @@ -38,11 +38,13 @@ const MAX_KEY_BASE_LENGTH = MAX_KEY_LENGTH - (String(MAX_KEY_ITERATIONS).length // object from JSON.parse. A key naming a member of Object.prototype reads back as // inherited rather than absent wherever one of those objects is indexed. // -// Derived rather than listed, because the set is a consequence of how keys are -// minted: minting lowercases and trims leading underscores, so a prototype name -// survives only if it has neither. `constructor` is the only one. -const RESERVED_KEYS = Object.getOwnPropertyNames(Object.prototype).filter( - (name) => mintableKey(name) === name, +// Derived rather than listed, from the format a key may take rather than from how one +// is minted: a caller can state a key instead of deriving it from a name, so what has +// to be reserved is every prototype name a well-formed key could spell. The ones that +// carry a capital cannot be spelled at all and need no reserving; `constructor` and +// `__proto__` can. +const RESERVED_KEYS = Object.getOwnPropertyNames(Object.prototype).filter((name) => + KEY_CHARACTERS.test(name), ); const FieldName = z @@ -136,17 +138,17 @@ export class CustomFieldDefinitionsService { * 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: ReturnType): Promise { + private async list(query: DefinitionQuery): Promise { const rows = await inFieldOrder(query).select('*'); return rows.map((row) => z.decode(customFieldCodec, row)); } async read(key: string): Promise { - const row = await this.knex(TABLE).where('key', key).first(); - if (!row) { + const [field] = await this.list(fieldByKey(this.knex, key)); + if (!field) { throw new errors.NotFoundError({ message: 'Custom field not found.' }); } - return z.decode(customFieldCodec, row); + return field; } /** @@ -201,13 +203,11 @@ export class CustomFieldDefinitionsService { for (const [index, field] of fields.entries()) { await this.assertNameAvailable(trx, field.name); const key = await this.mintKey(trx, bases[index]); - await trx(TABLE).insert({ - id: new ObjectID().toHexString(), + await this.insertField(trx, { key, name: field.name, type: field.type, - sort_order: firstSortOrder + index, - created_at: new Date(), + sortOrder: firstSortOrder + index, }); keys.push(key); } @@ -228,7 +228,86 @@ export class CustomFieldDefinitionsService { // Logged after the commit: the action log is a separate Bookshelf write // outside this transaction, so recording inside it would leave orphaned // "added" entries for fields a rollback never created. - for (const field of created) { + await this.recordCreated(context, created); + return created; + } + + /** + * Given an executor it joins that transaction; given none it opens its own. Unlike + * `add`, the key is stated rather than minted from the name, and nothing about it is + * worked around: a caller states a key because something else already names that key, + * so a variant of it would be a field nothing points at. + * + * The field is returned rather than logged: the history writes on its own connection, + * which a single-connection pool would deadlock against an open transaction. + */ + async addOne( + wanted: { key: string; name: string; type: FieldType }, + { executor = this.knex }: { executor?: Knex } = {}, + ): Promise { + // Before any database access, the way `add` mints before opening its transaction: + // an unusable key is a payload problem worth reporting on its own terms. + assertKeyUsable(wanted.key); + + const write = async (db: Knex) => { + await this.assertWithinLimit(db, 1); + await this.assertKeyAvailable(db, wanted.key); + await this.assertNameAvailable(db, wanted.name); + + await this.insertField(db, { + key: wanted.key, + name: wanted.name, + type: wanted.type, + sortOrder: await this.nextSortOrder(db), + }); + const [created] = await this.readMany(db, [wanted.key]); + return created; + }; + + // knex's marker for a transactor: join it rather than nesting a savepoint under it. + return executor.isTransaction ? write(executor) : executor.transaction(write); + } + + /** + * Only for a stated key. Minting picks a free one instead, so this reads as a clean 422 + * where the unique index would read as a 500. + */ + private async assertKeyAvailable(db: Knex, key: string): Promise { + const taken = await db(TABLE).where('key', key).first(); + if (taken) { + throw new errors.ValidationError({ + message: 'A custom field with this key already exists.', + property: 'key', + }); + } + } + + private async insertField( + db: Knex, + field: { key: string; name: string; type: FieldType; sortOrder: number }, + ): Promise { + await db(TABLE).insert({ + id: new ObjectID().toHexString(), + key: field.key, + name: field.name, + type: field.type, + sort_order: field.sortOrder, + created_at: new Date(), + }); + } + + /** `read` for a caller that is deciding rather than serving: absent is an answer. */ + async findByKey( + key: string, + { executor = this.knex }: { executor?: Knex } = {}, + ): Promise { + const row = await executor(TABLE).where('key', key).first(); + return row ? z.decode(customFieldCodec, row) : null; + } + + /** The same entry `add` writes. Only the caller knows its transaction committed. */ + async recordCreated(context: RequestContext, fields: CustomField[]): Promise { + for (const field of fields) { await this.recordAction({ context, verb: 'create', @@ -236,7 +315,6 @@ export class CustomFieldDefinitionsService { details: { primary_name: field.name, key: field.key }, }); } - return created; } /** @@ -518,6 +596,37 @@ export class CustomFieldDefinitionsService { } } +/** + * The shape a stated key has to have. Minting derives one that is usable by + * construction, so this is the check that path never needed: a caller stating its own + * key has said nothing about the format, and guarding here covers every route in + * rather than whichever one arrived first. + * + * The length bound is the column's own, not `mintKey`'s: that one holds back room for a + * `_` collision suffix, and a stated key is written exactly as given and never + * suffixed, so the whole column is available to it. + */ +function assertKeyUsable(key: string): void { + if (!KEY_CHARACTERS.test(key)) { + throw new errors.ValidationError({ + message: 'A custom field key can only contain lowercase letters, numbers and underscores.', + property: 'key', + }); + } + if (key.length > MAX_KEY_LENGTH) { + throw new errors.ValidationError({ + message: `A custom field key can be at most ${MAX_KEY_LENGTH} characters.`, + property: 'key', + }); + } + if (RESERVED_KEYS.includes(key)) { + throw new errors.ValidationError({ + message: `${key} cannot be used as a custom field key.`, + property: 'key', + }); + } +} + // The field a zod issue points at. Create validates an array, so an issue's path // is prefixed with the item's index (`[0, 'name']`); `property` names the field // that is wrong, so the numeric prefix is dropped. Which item it was is reported diff --git a/ghost/core/core/server/services/members-custom-fields/index.ts b/ghost/core/core/server/services/members-custom-fields/index.ts index 546cd4d3d25..ff9f5d84fd9 100644 --- a/ghost/core/core/server/services/members-custom-fields/index.ts +++ b/ghost/core/core/server/services/members-custom-fields/index.ts @@ -1,28 +1,31 @@ import { CustomFieldDefinitionsService } from './definitions-service'; import { CustomFieldValuesService } from './values-service'; +import { CustomFieldBindingsService } from './bindings-service'; import { recordCustomFieldAction, type RecordCustomFieldAction } from './actions'; import { resolveMaxDefinitions } from './config'; export type { CustomField } from './models'; export type { RequestContext } from './actions'; export { actingContext } from './actions'; +export type { BoundField } from './bindings-service'; export type { WrittenBy } from './schema'; -// Two services from one module, split along an aggregate boundary rather than a -// technical layer: `definitions` owns the field definitions, which belong to the -// site's settings, and `values` owns the per-member values, which belong to the -// member. The values service reads the definitions table directly for the -// reference data it needs — a value referencing its definition, not a boundary -// crossing. +// Three services from one module, split along aggregate boundaries rather than +// technical layers: `definitions` owns the field definitions, which belong to the +// site's settings, `values` owns the per-member values, which belong to the +// member, and `bindings` owns which of a source's ports writes into which field. +// The values service reads the definitions table directly for the reference data +// it needs — a value referencing its definition, not a boundary crossing. // // Constructed by init() at boot, not at import: knex is only available once the DB has connected. export let definitions: CustomFieldDefinitionsService | undefined; export let values: CustomFieldValuesService | undefined; +export let bindings: CustomFieldBindingsService | undefined; export function init(): void { - // The two are constructed together below, so checking both keeps the "both or - // neither" invariant explicit rather than trusting one to stand in for the pair. - if (definitions && values) { + // The three are constructed together below, so checking all of them keeps the "all or + // none" invariant explicit rather than trusting one to stand in for the rest. + if (definitions && values && bindings) { return; } @@ -51,4 +54,9 @@ export function init(): void { getMaxDefinitions: () => resolveMaxDefinitions(config.get('members:customFields:maxDefinitions')), }); + + // Built after the values, which is what a binding routes into. It has no handle on the + // definitions: making a field is not part of binding to one, so a caller that needs both + // asks for both. + bindings = new CustomFieldBindingsService({ knex, values }); } diff --git a/ghost/core/core/server/services/members-custom-fields/queries.ts b/ghost/core/core/server/services/members-custom-fields/queries.ts index 40a8a51b7b4..1dc8ddd60ee 100644 --- a/ghost/core/core/server/services/members-custom-fields/queries.ts +++ b/ghost/core/core/server/services/members-custom-fields/queries.ts @@ -12,6 +12,20 @@ export function activeFields(db: Knex) { return db(FIELDS_TABLE).where('status', FIELD_STATUS.active); } +/** + * A narrowed query against the definitions table, whatever it has narrowed by. + * + * 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. + */ +export type DefinitionQuery = ReturnType; + +/** 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); +} + /** * The publisher's order, applied to every read of the list. Here for the same reason the * status filter is: a read that forgets it comes back in whatever order the engine chose. diff --git a/ghost/core/core/server/services/members-custom-fields/schema.ts b/ghost/core/core/server/services/members-custom-fields/schema.ts index 1e401b3308c..7d24f509231 100644 --- a/ghost/core/core/server/services/members-custom-fields/schema.ts +++ b/ghost/core/core/server/services/members-custom-fields/schema.ts @@ -75,6 +75,24 @@ export const DbCustomFieldLeaf = z.object({ value_text: z.string(), }); +export const DbCustomFieldBinding = z.object({ + id: z.string(), + product_id: z.string(), + port: z.string(), + custom_field_key: z.string(), + created_at: DbDate, + updated_at: DbDate.nullable(), +}); + +type CustomFieldBindingRow = z.infer; + +/** A binding joined to the field it points at, which is how a collected value is routed. */ +export const DbBoundField = z.object({ + binding_id: z.string(), + key: z.string(), + type: FieldTypeSchema, +}); + declare module 'knex/types/tables' { interface Tables { members_custom_fields: Knex.CompositeTableType< @@ -89,5 +107,12 @@ declare module 'knex/types/tables' { Omit, 'updated_at'>, Partial >; + members_custom_field_bindings: Knex.CompositeTableType< + CustomFieldBindingRow, + // `updated_at` is set on insert as well as update: a binding is a setting, and + // "when was this last stated" is the same question whichever way it got there. + z.input, + Partial + >; } } diff --git a/ghost/core/core/server/services/stripe/services/checkout/field-ports.ts b/ghost/core/core/server/services/stripe/services/checkout/field-ports.ts new file mode 100644 index 00000000000..7d88d6f135d --- /dev/null +++ b/ghost/core/core/server/services/stripe/services/checkout/field-ports.ts @@ -0,0 +1,38 @@ +/** + * What Stripe Checkout will render, measured against the live API at Ghost's pinned + * version rather than read from the reference — which disagreed with the API in three of + * five probes, missing the field cap and the key format entirely. A wrong bound here fails + * the session create, which fails the checkout. + */ + +/** The names Stripe returns values under, and the only ports a binding for it may use. */ +export const STRIPE_PORTS = ['shipping_name', 'shipping_address', 'phone'] as const; +export type StripePort = (typeof STRIPE_PORTS)[number]; + +export function isStripePort(key: string): key is StripePort { + return (STRIPE_PORTS as readonly string[]).includes(key); +} + +export const STRIPE_PORT = { + shippingName: 'shipping_name', + shippingAddress: 'shipping_address', + phone: 'phone', +} as const satisfies Record; + +/** Stripe rejects a fourth. */ +export const MAX_CHECKOUT_CUSTOM_FIELDS = 3; + +/** Stripe caps a custom label at 50, where a field name may be 191 — hence a question label. */ +export const MAX_CHECKOUT_LABEL_LENGTH = 50; + +/** + * What Stripe Checkout can ask for. No `long_text`: its text input caps shorter than that + * type allows. No `address`: Stripe has no custom-field equivalent, so an address is + * collected through its own parameter instead. + */ +export const CHECKOUT_ELIGIBLE_FIELD_TYPES = ['short_text'] as const; +export type CheckoutEligibleFieldType = (typeof CHECKOUT_ELIGIBLE_FIELD_TYPES)[number]; + +export function isCheckoutEligible(type: string): type is CheckoutEligibleFieldType { + return (CHECKOUT_ELIGIBLE_FIELD_TYPES as readonly string[]).includes(type); +} diff --git a/ghost/core/core/server/services/tier-checkout-config/codec.ts b/ghost/core/core/server/services/tier-checkout-config/codec.ts new file mode 100644 index 00000000000..2113c4b4cec --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/codec.ts @@ -0,0 +1,112 @@ +import { z } from 'zod'; +import { FieldTypeSchema } from '@tryghost/custom-field-types'; +import { DbBoolean } from '../../lib/db-types/boolean'; +import { DbCheckoutOptions, DbCheckoutQuestion } from './schema'; +import { CheckoutOptions } from './models'; + +const SEPARATOR = ','; + +export const optionsCodec = z.codec(DbCheckoutOptions, CheckoutOptions, { + decode: (columns) => ({ + shippingAllowedCountries: splitList(columns.shipping_allowed_countries), + taxNumber: columns.tax_number_collect, + }), + encode: (options) => ({ + shipping_allowed_countries: options.shippingAllowedCountries.length + ? joinList(options.shippingAllowedCountries) + : null, + tax_number_collect: options.taxNumber, + }), +}); + +function splitList(stored: string | null): string[] { + return stored ? stored.split(SEPARATOR).filter(Boolean) : []; +} + +function joinList(values: string[]): string { + return [...new Set(values)].join(SEPARATOR); +} + +export const CollectionRow = z.object({ + product_id: z.string(), + config_id: z.string().nullable(), + + shipping_allowed_countries: z.string().nullable(), + tax_number_collect: DbBoolean.nullable(), + + shipping_name_key: z.string().nullable(), + shipping_name_collectable: z.string().nullable(), + shipping_address_key: z.string().nullable(), + shipping_address_collectable: z.string().nullable(), + phone_key: z.string().nullable(), + phone_collectable: z.string().nullable(), +}); +export type CollectionRow = z.input; + +export const collectionRowCodec = CollectionRow.transform((row) => { + const collection = collectedInto(row); + + return { + tierId: row.product_id, + configured: row.config_id !== null, + collection, + collecting: { + // Stripe collects the recipient's name and their address under one parameter, so it + // asks for both or neither. Archiving one of the two destinations is the publisher + // saying they no longer want that half: the step is still worth asking for while the + // other half can land, and whatever arrives for the archived one is dropped. Only + // archiving both leaves nothing worth asking for. + shipping: + row.shipping_name_collectable === null && row.shipping_address_collectable === null + ? null + : collection.shipping, + taxNumber: collection.taxNumber, + phone: row.phone_collectable === null ? null : collection.phone, + }, + }; +}); +export type CollectionParts = z.infer; + +export const QuestionRow = z.object({ + product_id: z.string(), + port: z.string(), + label: DbCheckoutQuestion.shape.label, + optional: DbCheckoutQuestion.shape.optional, + question_name: z.string().nullable(), + question_type: FieldTypeSchema.nullable(), +}); +export type QuestionRow = z.input; + +export const questionRowCodec = QuestionRow.transform((row) => ({ + tierId: row.product_id, + question: { + key: row.port, + label: row.label, + optional: row.optional, + }, + askable: + row.question_type === null + ? null + : { prompt: row.label ?? row.question_name ?? row.port, type: row.question_type }, +})); +export type QuestionParts = z.infer; + +function collectedInto(row: z.output) { + const { shippingAllowedCountries, taxNumber } = z.decode(optionsCodec, { + shipping_allowed_countries: row.shipping_allowed_countries, + tax_number_collect: row.tax_number_collect ?? false, + }); + + return { + shipping: + row.shipping_name_key !== null && row.shipping_address_key !== null + ? { + allowedCountries: shippingAllowedCountries, + nameCustomFieldKey: row.shipping_name_key, + addressCustomFieldKey: row.shipping_address_key, + } + : null, + taxNumber, + phone: row.phone_key === null ? null : { customFieldKey: row.phone_key }, + }; +} diff --git a/ghost/core/core/server/services/tier-checkout-config/destinations.ts b/ghost/core/core/server/services/tier-checkout-config/destinations.ts new file mode 100644 index 00000000000..877cb094c5e --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/destinations.ts @@ -0,0 +1,17 @@ +import type { FieldType } from '@tryghost/custom-field-types'; +import type { StripePort } from '../stripe/services/checkout/field-ports'; + +/** + * What a port supplies, and what to call a field made to hold it. The type is a rule about + * any destination: a port that returns an address can only be collected into a field that + * keeps one. The name is used only when the request names a key the site does not keep + * yet, which is the one moment a publisher has not chosen a label themselves. + * + * A key is not a port. `phone` is what Stripe calls what it returns; where it lands is + * whatever the request said, and `Shipping Phone` is only how that field is listed. + */ +export const PORT_FIELD = { + shipping_name: { name: 'Shipping Name', type: 'short_text' }, + shipping_address: { name: 'Shipping Address', type: 'address' }, + phone: { name: 'Shipping Phone', type: 'short_text' }, +} as const satisfies Record; diff --git a/ghost/core/core/server/services/tier-checkout-config/index.ts b/ghost/core/core/server/services/tier-checkout-config/index.ts new file mode 100644 index 00000000000..38135ca2653 --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/index.ts @@ -0,0 +1,24 @@ +/** + * What a tier's checkout asks and collects. + * + * A domain of its own rather than part of the Tier aggregate: a tier is loaded into memory + * once at boot, and these rows are read live because deleting a custom field cascades a + * question away without that repository ever seeing it. + * + * Constructed by the tiers service wrapper at boot, which already holds the collaborators + * this needs, rather than by an init() here — the custom field services are built before + * it, so both are ready by the time it runs. + */ +export { TierCheckoutConfigService } from './service'; + +export { + CheckoutQuestion, + emptyCheckoutConfig, + PhoneCollection, + ResolvedCheckout, + ResolvedQuestion, + ShippingCollection, + TierCheckoutConfig, +} from './models'; + +export { toCheckoutConfigResponse } from './serializers'; diff --git a/ghost/core/core/server/services/tier-checkout-config/models.ts b/ghost/core/core/server/services/tier-checkout-config/models.ts new file mode 100644 index 00000000000..22248675904 --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/models.ts @@ -0,0 +1,71 @@ +import { z } from 'zod'; +import { FieldTypeSchema } from '@tryghost/custom-field-types'; + +export const CheckoutQuestion = z.object({ + key: z.string(), + label: z.string().nullable(), + optional: z.boolean(), +}); +export type CheckoutQuestion = z.infer; + +/** + * One toggle and two destinations: a processor returns the recipient and the address + * under one parameter, but a publisher keeps a name and an address in different fields. + */ +export const ShippingCollection = z.object({ + /** ISO 3166-1 alpha-2. A processor will not render an address form without them. */ + allowedCountries: z.array(z.string()), + nameCustomFieldKey: z.string(), + addressCustomFieldKey: z.string(), +}); +export type ShippingCollection = z.infer; + +export const PhoneCollection = z.object({ + customFieldKey: z.string(), +}); +export type PhoneCollection = z.infer; + +export const TierCheckoutConfig = z.object({ + tierId: z.string(), + customFields: z.array(CheckoutQuestion), + shipping: ShippingCollection.nullable(), + /** Stripe keeps a tax number against the customer it invoices; Ghost never stores one. */ + taxNumber: z.boolean(), + phone: PhoneCollection.nullable(), +}); +export type TierCheckoutConfig = z.infer; + +export const emptyCollection = (): Pick< + TierCheckoutConfig, + 'shipping' | 'taxNumber' | 'phone' +> => ({ + shipping: null, + taxNumber: false, + phone: null, +}); + +export const emptyCheckoutConfig = (tierId: string): TierCheckoutConfig => ({ + tierId, + customFields: [], + ...emptyCollection(), +}); + +export const CheckoutOptions = z.object({ + shippingAllowedCountries: z.array(z.string()), + taxNumber: z.boolean(), +}); +export type CheckoutOptions = z.infer; + +export const ResolvedQuestion = CheckoutQuestion.extend({ + prompt: z.string(), + type: FieldTypeSchema, +}); +export type ResolvedQuestion = z.infer; + +export const ResolvedCheckout = z.object({ + customFields: z.array(ResolvedQuestion), + shipping: ShippingCollection.nullable(), + taxNumber: z.boolean(), + phone: PhoneCollection.nullable(), +}); +export type ResolvedCheckout = z.infer; diff --git a/ghost/core/core/server/services/tier-checkout-config/queries.ts b/ghost/core/core/server/services/tier-checkout-config/queries.ts new file mode 100644 index 00000000000..521a5f82855 --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/queries.ts @@ -0,0 +1,98 @@ +import type { Knex } from 'knex'; +import { FIELD_STATUS } from '../members-custom-fields/schema'; +import { STRIPE_PORT } from '../stripe/services/checkout/field-ports'; +import { DbCheckoutOptions } from './schema'; +import type { CollectionRow, QuestionRow } from './codec'; + +export const QUESTIONS_TABLE = 'products_checkout_fields'; +export const CONFIG_TABLE = 'products_checkout_config'; +export const BINDINGS_TABLE = 'members_custom_field_bindings'; +export const FIELDS_TABLE = 'members_custom_fields'; + +const ACTIVE = FIELD_STATUS.active; + +const optionColumns = Object.keys(DbCheckoutOptions.shape).map( + (column) => `${CONFIG_TABLE}.${column}`, +); + +function collectionQuery(db: Knex) { + const bindTo = (alias: string, port: string) => + function (this: Knex.JoinClause) { + this.on(`${alias}.product_id`, 'products.id').andOn(db.raw(`${alias}.port = ?`, [port])); + }; + + const landsIn = (alias: string, binding: string) => + function (this: Knex.JoinClause) { + this.on(`${alias}.key`, `${binding}.custom_field_key`).andOn( + db.raw(`${alias}.status = ?`, [ACTIVE]), + ); + }; + + const query = db('products') + .leftJoin(CONFIG_TABLE, `${CONFIG_TABLE}.product_id`, 'products.id') + .leftJoin({ name_binding: BINDINGS_TABLE }, bindTo('name_binding', STRIPE_PORT.shippingName)) + .leftJoin({ name_field: FIELDS_TABLE }, landsIn('name_field', 'name_binding')) + .leftJoin( + { shipping_binding: BINDINGS_TABLE }, + bindTo('shipping_binding', STRIPE_PORT.shippingAddress), + ) + .leftJoin({ shipping_field: FIELDS_TABLE }, landsIn('shipping_field', 'shipping_binding')) + .leftJoin({ phone_binding: BINDINGS_TABLE }, bindTo('phone_binding', STRIPE_PORT.phone)) + .leftJoin({ phone_field: FIELDS_TABLE }, landsIn('phone_field', 'phone_binding')) + .orderBy('products.id', 'asc') + .select([ + 'products.id as product_id', + `${CONFIG_TABLE}.id as config_id`, + ...optionColumns, + 'name_binding.custom_field_key as shipping_name_key', + 'name_field.key as shipping_name_collectable', + 'shipping_binding.custom_field_key as shipping_address_key', + 'shipping_field.key as shipping_address_collectable', + 'phone_binding.custom_field_key as phone_key', + 'phone_field.key as phone_collectable', + ]); + + return query; +} + +/** One row, whether or not that tier has ever been configured. */ +export function collectionRowsForTier(db: Knex, productId: string) { + return collectionQuery(db).where('products.id', productId); +} + +/** + * This query starts from products, so without a filter it would return every tier on the + * site, configured or not. A tier only gets a row in products_checkout_config when someone + * saves checkout settings for it, so requiring that row narrows the list to the tiers + * somebody has actually set up. A tier that was set up and then had everything switched off + * keeps its row and stays in the list, reporting that it collects nothing. + */ +export function configuredCollectionRows(db: Knex) { + return collectionQuery(db).whereNotNull(`${CONFIG_TABLE}.id`); +} + +export function questionRows(db: Knex, productId?: string) { + const query = db(QUESTIONS_TABLE) + .join(BINDINGS_TABLE, `${BINDINGS_TABLE}.id`, `${QUESTIONS_TABLE}.binding_id`) + .leftJoin({ question_field: FIELDS_TABLE }, function () { + this.on('question_field.key', `${BINDINGS_TABLE}.custom_field_key`).andOn( + db.raw('question_field.status = ?', [ACTIVE]), + ); + }) + .orderBy(`${BINDINGS_TABLE}.product_id`, 'asc') + .orderBy(`${QUESTIONS_TABLE}.sort_order`, 'asc') + .orderBy(`${QUESTIONS_TABLE}.id`, 'asc') + .select([ + `${BINDINGS_TABLE}.product_id`, + `${BINDINGS_TABLE}.port`, + `${QUESTIONS_TABLE}.label`, + `${QUESTIONS_TABLE}.optional`, + 'question_field.name as question_name', + 'question_field.type as question_type', + ]); + + if (productId) { + return query.where(`${BINDINGS_TABLE}.product_id`, productId); + } + return query; +} diff --git a/ghost/core/core/server/services/tier-checkout-config/schema.ts b/ghost/core/core/server/services/tier-checkout-config/schema.ts new file mode 100644 index 00000000000..0ef3d44c1dd --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/schema.ts @@ -0,0 +1,53 @@ +import { z } from 'zod'; +import type { Knex } from 'knex'; +import { DbBoolean } from '../../lib/db-types/boolean'; +import { DbDate } from '../../lib/db-types/date'; + +export const DbCheckoutQuestion = z.object({ + id: z.string(), + binding_id: z.string(), + sort_order: z.number(), + label: z.string().nullable(), + optional: DbBoolean, + created_at: DbDate, + updated_at: DbDate.nullable(), +}); + +/** + * Options only. Whether a tier collects something it keeps is the binding; a tax number is + * the exception, since Stripe keeps it and Ghost never does, so there is nothing to bind. + */ +export const DbCheckoutConfig = z.object({ + id: z.string(), + product_id: z.string(), + shipping_allowed_countries: z.string().nullable(), + tax_number_collect: DbBoolean, + created_at: DbDate, + updated_at: DbDate.nullable(), +}); + +type CheckoutConfigRow = z.infer; + +export const DbCheckoutOptions = DbCheckoutConfig.omit({ + id: true, + product_id: true, + created_at: true, + updated_at: true, +}); +export type DbCheckoutOptions = z.infer; + +declare module 'knex/types/tables' { + interface Tables { + products_checkout_fields: Knex.CompositeTableType< + z.infer, + Omit, 'updated_at'>, + Partial> + >; + products_checkout_config: Knex.CompositeTableType< + CheckoutConfigRow, + Omit, keyof DbCheckoutOptions> & + Partial>, + Partial> + >; + } +} diff --git a/ghost/core/core/server/services/tier-checkout-config/serializers.ts b/ghost/core/core/server/services/tier-checkout-config/serializers.ts new file mode 100644 index 00000000000..48106509e72 --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/serializers.ts @@ -0,0 +1,135 @@ +import { z } from 'zod'; +import { MAX_CHECKOUT_CUSTOM_FIELDS } from '../stripe/services/checkout/field-ports'; +import { TierCheckoutConfig } from './models'; + +/** Each code costs three characters once comma-joined, against a 2000-character column. */ +const MAX_ALLOWED_COUNTRIES = 600; + +const QuestionInput = z.object({ + key: z.string().min(1, { error: 'Every checkout question needs a custom field key.' }), + label: z.string().trim().min(1).nullish(), + optional: z.boolean().optional(), +}); +export type QuestionInput = z.infer; + +// Not checked against a list of countries: membership of that list is contested, and Ghost +// is not its arbiter. +const CountryCode = z + .string() + .trim() + .regex(/^[A-Za-z]{2}$/, { error: 'Enter a 2-letter country code, like US.' }) + .toUpperCase(); + +/** + * Where a collected value lands is the request's to state. Ghost keeps no convention about + * it, so a block that collects names its destination and one that does not carries nothing + * to name. + */ +const DESTINATION_REQUIRED = 'Say which custom field this is collected into.'; +const CustomFieldKey = z + .string({ error: DESTINATION_REQUIRED }) + .min(1, { error: DESTINATION_REQUIRED }); +const Destination = z.strictObject( + { custom_field_key: CustomFieldKey }, + { error: DESTINATION_REQUIRED }, +); + +export const CheckoutConfigInput = z.strictObject({ + custom_fields: z + .array(QuestionInput) + .max(MAX_CHECKOUT_CUSTOM_FIELDS, { + error: `A checkout can ask at most ${MAX_CHECKOUT_CUSTOM_FIELDS} questions.`, + }) + .refine( + (questions) => new Set(questions.map((question) => question.key)).size === questions.length, + { error: 'This checkout already asks for that field.' }, + ) + .optional(), + + shipping: z + .discriminatedUnion('collect', [ + z.strictObject({ collect: z.literal(false) }), + z.strictObject({ + collect: z.literal(true), + allowed_countries: z + .array(CountryCode, { error: 'Choose at least one country you deliver to.' }) + .min(1, { error: 'Choose at least one country you deliver to.' }) + .max(MAX_ALLOWED_COUNTRIES), + name: Destination, + address: Destination, + }), + ]) + .optional(), + + tax_number: z.strictObject({ collect: z.boolean() }).optional(), + + phone: z + .discriminatedUnion('collect', [ + z.strictObject({ collect: z.literal(false) }), + z.strictObject({ collect: z.literal(true), custom_field_key: CustomFieldKey }), + ]) + .optional(), +}); +export type CheckoutConfigInput = z.infer; + +const QuestionResource = z.object({ + key: z.string(), + label: z.string().nullable(), + optional: z.boolean(), +}); + +const CollectionResource = z.object({ + collect: z.literal(true), + custom_field_key: z.string(), +}); + +const ShippingResource = z.object({ + collect: z.literal(true), + allowed_countries: z.array(z.string()), + name: z.object({ custom_field_key: z.string() }), + address: z.object({ custom_field_key: z.string() }), +}); + +const CheckoutConfigResource = z.object({ + tier_id: z.string(), + custom_fields: z.array(QuestionResource), + shipping: ShippingResource.optional(), + tax_number: z.object({ collect: z.literal(true) }).optional(), + phone: CollectionResource.optional(), +}); + +const CheckoutConfigResponse = z.object({ + tiers_checkout_config: z.array(CheckoutConfigResource), +}); + +/** One resource per tier, so a browse and a read differ only in how many come back. */ +export const toCheckoutConfigResponse = z + .array(TierCheckoutConfig) + .transform((configs): z.input => ({ + tiers_checkout_config: configs.map((config) => ({ + tier_id: config.tierId, + custom_fields: config.customFields, + // A block appears only when the tier collects that thing, so a client reads + // presence rather than a flag it would have to check. + ...(config.shipping + ? { + shipping: { + collect: true as const, + allowed_countries: config.shipping.allowedCountries, + name: { custom_field_key: config.shipping.nameCustomFieldKey }, + address: { custom_field_key: config.shipping.addressCustomFieldKey }, + }, + } + : {}), + ...(config.taxNumber ? { tax_number: { collect: true as const } } : {}), + ...(config.phone + ? { + phone: { + collect: true as const, + custom_field_key: config.phone.customFieldKey, + }, + } + : {}), + })), + })) + .pipe(CheckoutConfigResponse); diff --git a/ghost/core/core/server/services/tier-checkout-config/service.ts b/ghost/core/core/server/services/tier-checkout-config/service.ts new file mode 100644 index 00000000000..45eaea32754 --- /dev/null +++ b/ghost/core/core/server/services/tier-checkout-config/service.ts @@ -0,0 +1,420 @@ +import ObjectID from 'bson-objectid'; +import errors from '@tryghost/errors'; +import { z } from 'zod'; +import type { Knex } from 'knex'; +import type { FieldType } from '@tryghost/custom-field-types'; +import { DbCustomField, FIELD_STATUS } from '../members-custom-fields/schema'; +import type { CustomField, RequestContext } from '../members-custom-fields'; +import { + MAX_CHECKOUT_LABEL_LENGTH, + STRIPE_PORT, + isCheckoutEligible, + isStripePort, + type StripePort, +} from '../stripe/services/checkout/field-ports'; +import { + collectionRowCodec, + optionsCodec, + questionRowCodec, + type CollectionParts, + type CollectionRow, + type QuestionParts, +} from './codec'; +import { + BINDINGS_TABLE, + CONFIG_TABLE, + FIELDS_TABLE, + QUESTIONS_TABLE, + collectionRowsForTier, + configuredCollectionRows, + questionRows, +} from './queries'; +import { + emptyCollection, + type ResolvedCheckout, + type ResolvedQuestion, + type TierCheckoutConfig, +} from './models'; +import { PORT_FIELD } from './destinations'; +import { CheckoutConfigInput } from './serializers'; + +type FieldRow = Pick, 'key' | 'name' | 'type' | 'status'>; + +type NewField = { key: string; name: string; type: FieldType }; + +/** + * A request states its settings in named sections: one for shipping, one for phone. Each + * section stands for one or more of the values Stripe hands back when a checkout is + * completed — shipping covers both the recipient's name and their address, phone covers + * only the phone number. + * + * Naming those values here means that a request which includes a section settles every + * value in it: each one is either given a destination or has its old one removed. Without + * this list, a value the request never mentioned could keep a destination from an earlier + * save that the publisher believes they have turned off. + */ +const BLOCK_PORTS = { + shipping: [STRIPE_PORT.shippingName, STRIPE_PORT.shippingAddress], + phone: [STRIPE_PORT.phone], +} as const satisfies Record; + +interface CollectionPlan { + clear: StripePort[]; + create: NewField[]; + bind: Array<{ port: StripePort; key: string }>; +} + +export interface PortBinder { + bind( + db: Knex, + productId: string, + port: string, + customFieldKey: string, + now: Date, + ): Promise; + remove(db: Knex, productId: string, port: string): Promise; +} + +export interface FieldMaker { + findByKey(key: string, options?: { executor?: Knex }): Promise; + addOne(wanted: NewField, options?: { executor?: Knex }): Promise; + recordCreated(context: RequestContext, fields: CustomField[]): Promise; +} + +export class TierCheckoutConfigService { + private knex: Knex; + private bindings: PortBinder; + private fields: FieldMaker; + + constructor({ + knex, + bindings, + fields, + }: { + knex: Knex; + bindings: PortBinder; + fields: FieldMaker; + }) { + this.knex = knex; + this.bindings = bindings; + this.fields = fields; + } + + async browse(): Promise { + const rows = decodeCollection(await configuredCollectionRows(this.knex)); + if (rows.length === 0) { + return []; + } + + const asked = await this.questions(); + return rows.map((row) => assemble(row, asked.get(row.tierId) ?? [])); + } + + async read(productId: string): Promise { + const [row] = decodeCollection(await collectionRowsForTier(this.knex, productId)); + if (!row) { + throw new errors.NotFoundError({ message: 'Tier not found.' }); + } + if (!row.configured) { + return null; + } + + const asked = await this.questions(productId); + return assemble(row, asked.get(productId) ?? []); + } + + async resolve(productId: string): Promise { + const [row] = decodeCollection(await collectionRowsForTier(this.knex, productId)); + if (!row?.configured) { + return { customFields: [], ...emptyCollection() }; + } + + const asked = await this.questions(productId); + const customFields: ResolvedQuestion[] = (asked.get(productId) ?? []).flatMap((entry) => + entry.askable ? [{ ...entry.question, ...entry.askable }] : [], + ); + return { customFields, ...row.collecting }; + } + + /** + * Saves the checkout settings a request states, and leaves the rest alone. + * + * A request only has to include the sections it wants to change. Say nothing about + * shipping and the shipping settings stay exactly as they were, so a client that only + * knows how to edit the questions cannot wipe out the shipping settings by omitting + * them. + */ + async edit(context: RequestContext, productId: string, input: unknown): Promise { + const stated = parseInput(input); + const now = new Date(); + + if (stated.custom_fields) { + await assertQuestionsAskable(this.knex, stated.custom_fields); + } + const plan = await this.planCollection(stated); + + const created = await this.knex.transaction(async (trx) => { + await assertTierExists(trx, productId); + await writeOptions(trx, productId, stated, now); + + for (const port of plan.clear) { + await this.bindings.remove(trx, productId, port); + } + + const made: CustomField[] = []; + for (const wanted of plan.create) { + made.push(await this.fields.addOne(wanted, { executor: trx })); + } + for (const { port, key } of plan.bind) { + await this.bindings.bind(trx, productId, port, key, now); + } + + if (stated.custom_fields) { + await this.writeQuestions(trx, productId, stated.custom_fields, now); + } + return made; + }); + + await this.fields.recordCreated(context, created); + } + + private async questions(productId?: string): Promise> { + const rows = await questionRows(this.knex, productId); + const byTier = new Map(); + for (const row of rows) { + const parts = z.decode(questionRowCodec, row); + const forTier = byTier.get(parts.tierId) ?? []; + forTier.push(parts); + byTier.set(parts.tierId, forTier); + } + return byTier; + } + + private async planCollection(stated: CheckoutConfigInput): Promise { + const wanted: Array<{ port: StripePort; key: string }> = []; + + if (stated.shipping?.collect) { + wanted.push( + { port: STRIPE_PORT.shippingName, key: stated.shipping.name.custom_field_key }, + { port: STRIPE_PORT.shippingAddress, key: stated.shipping.address.custom_field_key }, + ); + } + if (stated.phone?.collect) { + wanted.push({ port: STRIPE_PORT.phone, key: stated.phone.custom_field_key }); + } + + const bound = new Set(wanted.map(({ port }) => port)); + const clear: StripePort[] = []; + for (const block of ['shipping', 'phone'] as const) { + if (stated[block]) { + clear.push(...BLOCK_PORTS[block].filter((port) => !bound.has(port))); + } + } + + const create = new Map(); + for (const { port, key } of wanted) { + const wants = PORT_FIELD[port]; + const existing = await this.fields.findByKey(key); + if (existing) { + assertCollectableInto(port, existing, wants.type); + continue; + } + + const alreadyPlanned = create.get(key); + if (alreadyPlanned && alreadyPlanned.type !== wants.type) { + throw new errors.ValidationError({ + message: `This can only be collected into a ${wants.type} field.`, + property: `checkout.${port}.custom_field_key`, + }); + } + if (!alreadyPlanned) { + create.set(key, { key, name: wants.name, type: wants.type }); + } + } + + return { clear, create: [...create.values()], bind: wanted }; + } + + /** + * Saves the questions this tier asks its buyers during checkout. + * + * A binding records that one value coming back from Stripe belongs in one custom field, + * and it identifies the value by the name Stripe uses for it. For a question that name + * is the custom field's own key: Ghost sends the key to Stripe as the question's + * identifier, and Stripe returns the buyer's answer labelled with that same key. That is + * why the key is passed twice below — once as the name Stripe will answer under, and + * once as the field the answer is stored in. + */ + private async writeQuestions( + trx: Knex.Transaction, + productId: string, + questions: NonNullable, + now: Date, + ): Promise { + const asked = new Set(questions.map((question) => question.key)); + const alreadyAsked: Array<{ port: string }> = await trx(QUESTIONS_TABLE) + .join(BINDINGS_TABLE, `${BINDINGS_TABLE}.id`, `${QUESTIONS_TABLE}.binding_id`) + .where(`${BINDINGS_TABLE}.product_id`, productId) + .select(`${BINDINGS_TABLE}.port`); + for (const { port } of alreadyAsked) { + if (!asked.has(port)) { + await this.bindings.remove(trx, productId, port); + } + } + + for (const [index, question] of questions.entries()) { + const bindingId = await this.bindings.bind(trx, productId, question.key, question.key, now); + await trx(QUESTIONS_TABLE).where('binding_id', bindingId).del(); + await trx(QUESTIONS_TABLE).insert({ + id: new ObjectID().toHexString(), + binding_id: bindingId, + sort_order: index, + label: question.label ?? null, + optional: question.optional ?? true, + created_at: now, + }); + } + } +} + +function parseInput(input: unknown): CheckoutConfigInput { + const parsed = CheckoutConfigInput.safeParse(input); + if (parsed.success) { + return parsed.data; + } + const issue = parsed.error.issues[0]; + throw new errors.ValidationError({ + message: issue.message, + property: issue.path.join('.') || 'checkout', + }); +} + +function decodeCollection(rows: CollectionRow[]): CollectionParts[] { + return rows.map((row) => z.decode(collectionRowCodec, row)); +} + +function assemble(row: CollectionParts, asked: QuestionParts[]): TierCheckoutConfig { + return { + tierId: row.tierId, + customFields: asked.map((entry) => entry.question), + ...row.collection, + }; +} + +async function assertTierExists(db: Knex, productId: string): Promise { + const tier = await db('products').where('id', productId).first(); + if (!tier) { + throw new errors.NotFoundError({ message: 'Tier not found.' }); + } +} + +/** + * Refuses a custom field that cannot hold what Stripe will send back. + * + * A request names the custom field each collected value should be saved into. That field + * has to be active, because an archived field accepts no new values, and it has to store + * the right kind of data: Stripe returns a structured address for the shipping address, + * and plain text for a name or a phone number. + * + * Both refusals are deliberate rather than defensive. Ghost could save the value into some + * other field, or accept the request and quietly collect nothing, but a publisher who named + * a field meant that field. Being told now is better than finding out weeks later that + * nothing was ever recorded. + */ +function assertCollectableInto(port: StripePort, field: CustomField, valueType: FieldType): void { + if (field.status !== FIELD_STATUS.active) { + throw new errors.ValidationError({ + message: 'An archived custom field cannot receive collected data. Restore it first.', + property: `checkout.${port}.custom_field_key`, + }); + } + if (field.type !== valueType) { + throw new errors.ValidationError({ + message: `This can only be collected into a ${valueType} field.`, + property: `checkout.${port}.custom_field_key`, + }); + } +} + +async function writeOptions( + trx: Knex.Transaction, + productId: string, + stated: CheckoutConfigInput, + now: Date, +): Promise { + const all = z.encode(optionsCodec, { + shippingAllowedCountries: stated.shipping?.collect ? stated.shipping.allowed_countries : [], + taxNumber: stated.tax_number?.collect ?? false, + }); + + // Only the columns this request spoke about are written, so two requests changing + // different parts of the same tier cannot undo each other, and a request that mentions + // neither still leaves the row behind as the record that this tier has been set up. + const columns = { + ...(stated.shipping ? { shipping_allowed_countries: all.shipping_allowed_countries } : {}), + ...(stated.tax_number ? { tax_number_collect: all.tax_number_collect } : {}), + }; + + await trx(CONFIG_TABLE) + .insert({ + id: new ObjectID().toHexString(), + product_id: productId, + created_at: now, + updated_at: now, + ...columns, + }) + .onConflict('product_id') + .merge({ ...columns, updated_at: now }); +} + +async function assertQuestionsAskable( + db: Knex, + questions: NonNullable, +): Promise { + const collides = questions.find((question) => isStripePort(question.key)); + if (collides) { + throw new errors.ValidationError({ + message: `A field keyed ${collides.key} cannot be asked at checkout, because that is what this checkout calls something it collects for itself.`, + property: 'checkout.custom_fields', + }); + } + + const keys = questions.map((question) => question.key); + if (keys.length === 0) { + return; + } + + const rows: FieldRow[] = await db(FIELDS_TABLE) + .whereIn(`${FIELDS_TABLE}.key`, keys) + .where(`${FIELDS_TABLE}.status`, FIELD_STATUS.active) + .select( + `${FIELDS_TABLE}.key`, + `${FIELDS_TABLE}.name`, + `${FIELDS_TABLE}.type`, + `${FIELDS_TABLE}.status`, + ); + const byKey = new Map(rows.map((row) => [row.key, row])); + + for (const question of questions) { + const field = byKey.get(question.key); + if (!field) { + throw new errors.ValidationError({ + message: `Unknown custom field: ${question.key}`, + property: 'checkout.custom_fields', + }); + } + if (!isCheckoutEligible(field.type)) { + throw new errors.ValidationError({ + message: `A ${field.type} field cannot be asked for at checkout.`, + property: 'checkout.custom_fields', + }); + } + const prompt = question.label ?? field.name; + if (prompt.length > MAX_CHECKOUT_LABEL_LENGTH) { + throw new errors.ValidationError({ + message: `A checkout question can be at most ${MAX_CHECKOUT_LABEL_LENGTH} characters. Give this one a shorter label.`, + property: 'checkout.custom_fields', + }); + } + } +} diff --git a/ghost/core/core/server/services/tiers/service.js b/ghost/core/core/server/services/tiers/service.js index b03a7ca6b8a..e119d341090 100644 --- a/ghost/core/core/server/services/tiers/service.js +++ b/ghost/core/core/server/services/tiers/service.js @@ -30,6 +30,27 @@ class TiersServiceWrapper { repository, slugService, }); + + // What a tier's checkout asks, kept beside the tier rather than inside it: these + // rows are read live on every request, because deleting a custom field cascades a + // question away without the repository above ever seeing it, and a cached copy + // would go on naming a field the site no longer has. + // + // Boot builds the custom fields services before this one, so both collaborators + // are ready by the time this runs. + const { TierCheckoutConfigService } = require('../tier-checkout-config'); + const { bindings, definitions } = require('../members-custom-fields'); + this.checkout = new TierCheckoutConfigService({ + knex: models.Base.knex, + // A binding is where a collected value lands, and the same rows are what a completed + // checkout is routed through later. Handed over rather than reached for, so the + // checkout domain states what it needs of them and nothing more. + bindings, + // Turning a collection on has to leave it somewhere to land, and making a field is + // the definitions' to do. A separate collaborator because it is a separate act, and + // because deciding whether one is needed belongs to the checkout rather than to them. + fields: definitions, + }); } } 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 6c6023c1d87..263584c260a 100644 --- a/ghost/core/core/server/web/api/endpoints/admin/routes.js +++ b/ghost/core/core/server/web/api/endpoints/admin/routes.js @@ -146,6 +146,25 @@ module.exports = function apiRoutes() { // Tiers router.get('/tiers', mw.authAdminApi, http(api.tiers.browse)); router.post('/tiers', mw.authAdminApi, http(api.tiers.add)); + // What a tier's checkout asks for, read when a Stripe checkout session is built. + // Registered before /tiers/:id so the literal path isn't captured by :id. + // + // A sub-resource rather than a key on the tier, because the tier payload is a public + // projection: `tiers-public` shares this docName's serializer, so anything on a tier is + // rendered by themes. This is admin-only configuration that no client renders, since + // the questions are drawn by Stripe's own checkout page rather than by Portal. + // + // Named for the configuration rather than the checkout, so it cannot be mistaken for + // the session that `create-stripe-checkout-session` creates from it. + router.get('/tiers/checkout_config', mw.authAdminApi, http(api.tiersCheckoutConfig.browse)); + router.get('/tiers/:id/checkout_config', mw.authAdminApi, http(api.tiersCheckoutConfig.read)); + router.put( + '/tiers/:id/checkout_config', + mw.authAdminApi, + labs.enabledMiddleware('membersCustomFields'), + http(api.tiersCheckoutConfig.edit), + ); + router.get('/tiers/:id', mw.authAdminApi, http(api.tiers.read)); router.put('/tiers/:id', mw.authAdminApi, http(api.tiers.edit)); @@ -176,14 +195,11 @@ module.exports = function apiRoutes() { router.get('/members/stripe_connect', mw.authAdminApi, http(api.membersStripeConnect.auth)); - // Member custom field definitions — gated by the members_custom_fields flag. + // Member custom field definitions. Reading them is open: a site that has never + // turned the flag on has none, so the answer is an empty list rather than a 404, and + // Admin can ask without first knowing whether it may. Changing them is gated. // Registered before /members/:id so the literal path isn't captured by :id. - router.get( - '/members/custom_fields', - mw.authAdminApi, - labs.enabledMiddleware('membersCustomFields'), - http(api.membersCustomFields.browse), - ); + router.get('/members/custom_fields', mw.authAdminApi, http(api.membersCustomFields.browse)); router.post( '/members/custom_fields', mw.authAdminApi, @@ -197,12 +213,7 @@ module.exports = function apiRoutes() { labs.enabledMiddleware('membersCustomFields'), http(api.membersCustomFields.reorder), ); - router.get( - '/members/custom_fields/:key', - mw.authAdminApi, - labs.enabledMiddleware('membersCustomFields'), - http(api.membersCustomFields.read), - ); + router.get('/members/custom_fields/:key', mw.authAdminApi, http(api.membersCustomFields.read)); router.put( '/members/custom_fields/:key', mw.authAdminApi, 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 b3e71216d5c..a03ce7d6707 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 @@ -435,6 +435,20 @@ describe('Member Custom Fields Admin API', function () { }); }); + // 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. + describe('Asking for relations', function () { + it('ignores an include, rather than failing on it', async function () { + const field = await createField({ name: 'T-shirt size' }); + + const { body } = await agent.get('members/custom_fields/?include=bindings').expectStatus(200); + + assert.equal(body.members_custom_fields[0].key, field.key); + assert.equal('bindings' in body.members_custom_fields[0], false); + }); + }); + describe('Creating several definitions at once', function () { it('creates every definition in the request, in order', async function () { const { body } = await agent @@ -2254,14 +2268,30 @@ describe('Member Custom Fields Admin API', function () { }); }); + // The flag governs whether a publisher can set custom fields up, not whether the rest of + // Ghost may ask what they are. Admin reads this list to draw screens it renders either + // way, and a site that has never enabled the flag has no fields, so the honest answer is + // an empty list. Every route that changes something stays behind the flag. describe('Flag disabled', function () { beforeEach(function () { mockManager.restore(); mockManager.mockLabsDisabled('membersCustomFields'); }); - it('404s the definitions endpoint', async function () { - await agent.get('members/custom_fields/').expectStatus(404); + it('serves the definitions endpoint, with nothing on it', async function () { + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual(body.members_custom_fields, []); + }); + + it('404s a read of a field that does not exist, rather than hiding the route', async function () { + await agent.get('members/custom_fields/company/').expectStatus(404); + }); + + it('404s the create endpoint', async function () { + await agent + .post('members/custom_fields/') + .body({ members_custom_fields: [{ name: 'Company', type: 'short_text' }] }) + .expectStatus(404); }); it('404s the reorder endpoint', async function () { @@ -2270,5 +2300,16 @@ describe('Member Custom Fields Admin API', function () { .body({ members_custom_fields: [{ key: 'company' }] }) .expectStatus(404); }); + + it('404s the edit endpoint', async function () { + await agent + .put('members/custom_fields/company/') + .body({ members_custom_fields: [{ name: 'Employer' }] }) + .expectStatus(404); + }); + + it('404s the delete endpoint', async function () { + await agent.delete('members/custom_fields/company/').expectStatus(404); + }); }); }); 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 new file mode 100644 index 00000000000..a4f3e99f54b --- /dev/null +++ b/ghost/core/test/e2e-api/admin/tiers-checkout-config.test.ts @@ -0,0 +1,1063 @@ +import assert from 'node:assert/strict'; + +const { agentProvider, fixtureManager, mockManager } = require('../../utils/e2e-framework'); +const models = require('../../../core/server/models'); + +describe('Tier Checkout Admin API', function () { + let agent: { + get: (_url: string) => any; + put: (_url: string) => any; + post: (_url: string) => any; + delete: (_url: string) => any; + loginAsOwner: () => Promise; + }; + let tierId: string; + + async function createField(field: { name: string; type?: string }) { + const { body } = await agent + .post('members/custom_fields/') + .body({ members_custom_fields: [{ type: 'short_text', ...field }] }) + .expectStatus(201); + return body.members_custom_fields[0]; + } + + async function setStatus(key: string, status: 'active' | 'archived') { + await agent + .put(`members/custom_fields/${key}/`) + .body({ members_custom_fields: [{ status }] }) + .expectStatus(200); + } + + async function setCheckout(config: Record, status = 200) { + const { body } = await agent + .put(`tiers/${tierId}/checkout_config/`) + .body({ tiers_checkout_config: [config] }) + .expectStatus(status); + return body; + } + + async function readCheckout() { + const { body } = await agent.get(`tiers/${tierId}/checkout_config/`).expectStatus(200); + return body.tiers_checkout_config[0]; + } + + const shipping = (over: Record = {}) => ({ + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'delivery_address' }, + ...over, + }, + }); + + beforeAll(async function () { + agent = await agentProvider.getAdminAPIAgent(); + await fixtureManager.init('users'); + await agent.loginAsOwner(); + + const { body } = await agent.get('tiers/?limit=1&filter=type:paid').expectStatus(200); + tierId = body.tiers[0].id; + }); + + beforeEach(function () { + mockManager.mockLabsEnabled('membersCustomFields'); + }); + + afterEach(async function () { + mockManager.restore(); + await models.Base.knex('products_checkout_fields').del(); + await models.Base.knex('products_checkout_config').del(); + await models.Base.knex('members_custom_field_bindings').del(); + await models.Base.knex('members_custom_fields').del(); + // The history outlives the fields it describes, which is the point of it — but across + // tests in one file it would leave each one reading the ones before. + await models.Base.knex('actions').where('resource_type', 'member_custom_field').del(); + await models.Base.knex('products').where('id', 'ffffffffffffffffffffffff').del(); + }); + + describe('Questions the checkout asks', function () { + it('starts with nothing configured', async function () { + assert.deepEqual(await readCheckout(), { tier_id: tierId, custom_fields: [] }); + }); + + it('keeps the questions in the order they were given', async function () { + const size = await createField({ name: 'T-shirt size' }); + const diet = await createField({ name: 'Dietary requirements' }); + + await setCheckout({ custom_fields: [{ key: diet.key }, { key: size.key }] }); + + const { custom_fields: questions } = await readCheckout(); + assert.deepEqual( + questions.map((question: { key: string }) => question.key), + ['dietary_requirements', 't_shirt_size'], + ); + }); + + // A question is optional unless a publisher chooses otherwise: a required question + // at the payment step costs conversion. + it("asks optionally, and under the field's own name, unless told otherwise", async function () { + const size = await createField({ name: 'T-shirt size' }); + + await setCheckout({ custom_fields: [{ key: size.key }] }); + assert.deepEqual(await readCheckout(), { + tier_id: tierId, + custom_fields: [{ key: 't_shirt_size', label: null, optional: true }], + }); + + await setCheckout({ + custom_fields: [{ key: size.key, label: 'Which size?', optional: false }], + }); + assert.deepEqual((await readCheckout()).custom_fields, [ + { key: 't_shirt_size', label: 'Which size?', optional: false }, + ]); + }); + + it('states the whole list, so a question left out is no longer asked', async function () { + const size = await createField({ name: 'T-shirt size' }); + const diet = await createField({ name: 'Dietary requirements' }); + await setCheckout({ custom_fields: [{ key: size.key }, { key: diet.key }] }); + + await setCheckout({ custom_fields: [{ key: diet.key }] }); + assert.deepEqual( + (await readCheckout()).custom_fields.map((question: { key: string }) => question.key), + ['dietary_requirements'], + ); + }); + + it('refuses more questions than the processor will render', async function () { + const keys = []; + for (const name of ['One', 'Two', 'Three', 'Four']) { + keys.push((await createField({ name })).key); + } + + const body = await setCheckout({ custom_fields: keys.map((key) => ({ key })) }, 422); + assert.match(body.errors[0].context, /at most 3 questions/); + }); + + // A publisher can name a field something no processor will render as a label, so + // the label exists to be shorter than the name. + it('refuses a question the processor could not label', async function () { + const long = await createField({ name: `Tell us ${'x'.repeat(60)}` }); + + const body = await setCheckout({ custom_fields: [{ key: long.key }] }, 422); + assert.match(body.errors[0].context, /at most 50 characters/); + + await setCheckout({ custom_fields: [{ key: long.key, label: 'Tell us more' }] }); + assert.equal((await readCheckout()).custom_fields[0].label, 'Tell us more'); + }); + + it('refuses a field type the processor cannot ask for', async function () { + // Named so its key is not one of the things this checkout collects for itself, + // which is refused earlier and for a different reason. + const address = await createField({ name: 'Postal address', type: 'address' }); + + const body = await setCheckout({ custom_fields: [{ key: address.key }] }, 422); + assert.match(body.errors[0].context, /cannot be asked for at checkout/); + }); + + it('refuses the same field twice', async function () { + const size = await createField({ name: 'T-shirt size' }); + + const body = await setCheckout( + { custom_fields: [{ key: size.key }, { key: size.key }] }, + 422, + ); + assert.match(body.errors[0].context, /already asks/); + }); + + it('refuses a field the site does not have', async function () { + const body = await setCheckout({ custom_fields: [{ key: 'no_such_field' }] }, 422); + assert.match(body.errors[0].context, /Unknown custom field/); + }); + + // A question's port is the field's own key, so a field keyed like something this + // checkout already collects would want a port that is taken. Refused in its own + // words, rather than left to the unique index to report unreadably. + it('refuses a field keyed like something the checkout collects itself', async function () { + const phone = await createField({ name: 'Phone' }); + assert.equal(phone.key, 'phone', 'the name mints the key the phone port uses'); + + const body = await setCheckout({ custom_fields: [{ key: phone.key }] }, 422); + assert.match(body.errors[0].context, /cannot be asked at checkout/); + }); + }); + + describe('What the checkout collects for itself', function () { + // Collecting and choosing where it lands are one statement, because a publisher + // makes them as one choice: a checkbox and the field beside it. + it('collects a port and binds its destination in one write', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + await setCheckout(shipping({ allowed_countries: ['GB', 'ie'] })); + assert.deepEqual((await readCheckout()).shipping, { + collect: true, + // Uppercased on the way in, so one country is one value. + allowed_countries: ['GB', 'IE'], + // Nothing kept the recipient's name yet, so a field was made under that key. + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'delivery_address' }, + }); + }); + + // Turning collection on is a publisher saying they want the data. Making them build a + // field for it first turns one checkbox into an errand, so naming a key the site does + // not keep yet makes it. + it('makes a field for a key the site does not keep yet', async function () { + await setCheckout({ + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }); + + assert.deepEqual((await readCheckout()).shipping, { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields.map((field: { name: string; type: string }) => [ + field.name, + field.type, + ]), + [ + ['Shipping Name', 'short_text'], + ['Shipping Address', 'address'], + ], + 'and it is listed under the name the port supplies', + ); + }); + + // Turning collection off and on again is the ordinary case. A second "Shipping Address" + // beside the first would split one thing across two columns of every export. + it('binds the field it made rather than making another', async function () { + const collect = { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }; + await setCheckout(collect); + await setCheckout({ shipping: { collect: false } }); + await setCheckout(collect); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields.map((field: { key: string }) => field.key), + ['shipping_name', 'shipping_address'], + ); + assert.equal( + (await readCheckout()).shipping.address.custom_field_key, + 'shipping_address', + 'and bound to it again', + ); + }); + + // Names are unique across the whole list, so the label a made field would be listed + // under can be taken by something else. Numbering past it would leave a publisher with + // two things called nearly the same, neither of them obviously the one collected into. + it('refuses to make a field whose label is already taken', async function () { + await createField({ name: 'Shipping Address', type: 'short_text' }); + + await setCheckout(shipping(), 422); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields.map((field: { key: string }) => field.key), + ['shipping_address'], + 'and nothing was made under a numbered label instead', + ); + }); + + // The key is the field's identity and the name is not, so renaming what Ghost made + // does not cost a publisher a second copy of it: the request names the same key and + // finds the same field. + it('binds the same field after a publisher renames it', async function () { + const collect = { + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }; + await setCheckout(collect); + await agent + .put('members/custom_fields/shipping_address/') + .body({ members_custom_fields: [{ name: 'Delivery address' }] }) + .expectStatus(200); + + await setCheckout({ shipping: { collect: false } }); + await setCheckout(collect); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields + .filter((field: { type: string }) => field.type === 'address') + .map((field: { key: string; name: string }) => [field.key, field.name]), + [['shipping_address', 'Delivery address']], + 'the renamed field is still the one, and there is only one', + ); + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'shipping_address'); + }); + + // A field of the right type under the key that was named is that field, whoever made it. + it('collects into a field the publisher already keeps under that key', async function () { + const theirs = await createField({ name: 'Shipping Address', type: 'address' }); + assert.equal(theirs.key, 'shipping_address'); + + await setCheckout(shipping({ address: { custom_field_key: theirs.key } })); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.equal( + body.members_custom_fields.filter((field: { type: string }) => field.type === 'address') + .length, + 1, + 'nothing was made beside it', + ); + assert.equal((await readCheckout()).shipping.address.custom_field_key, theirs.key); + }); + + // Binding to it would succeed and collect nothing until someone restored it, which the + // publisher who asked for the collection has no way to see. Restoring the field is one + // step; working out why a checkout stopped asking is not. + it('refuses a destination the publisher has archived', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + await setStatus('delivery_address', 'archived'); + + const body = await setCheckout(shipping(), 422); + assert.match(body.errors[0].context, /archived/); + + assert.deepEqual( + await models.Base.knex('members_custom_field_bindings').select(), + [], + 'and nothing was bound', + ); + }); + + // A field appearing in a publisher's list without them creating it is exactly the case + // the history is for, and it is theirs from that point on: the same "added" entry a + // field they typed gets, attributed to whoever turned the collection on. + it('records the fields it made in the history', async function () { + await setCheckout({ + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }); + + const { body } = await agent + .get('actions/?filter=resource_type:member_custom_field&limit=all') + .expectStatus(200); + // The name rides in the action's context; resource_id holds the row id. Sorted + // because both were written in the same statement, so their order is not meaningful. + const provisioned = body.actions + .map((action: { event: string; actor_type: string; context: unknown }) => ({ + event: action.event, + actorType: action.actor_type, + ...(typeof action.context === 'string' ? JSON.parse(action.context) : action.context), + })) + .sort((a: { key: string }, b: { key: string }) => a.key.localeCompare(b.key)); + + assert.deepEqual( + provisioned.map((action: { event: string; primary_name: string; actorType: string }) => [ + action.event, + action.primary_name, + action.actorType, + ]), + [ + ['added', 'Shipping Address', 'user'], + ['added', 'Shipping Name', 'user'], + ], + ); + }); + + // The phone number is the port whose name is not the publisher's word for it: Stripe + // calls what it returns `phone`, and a field made for it is listed as Shipping Phone. + // So the two are asserted together, because a declaration that muddled them would list + // a publisher's field under the processor's own vocabulary. + it('makes a field for the phone number under its own name', async function () { + await setCheckout({ phone: { collect: true, custom_field_key: 'shipping_phone' } }); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields.map((field: { key: string; name: string; type: string }) => [ + field.key, + field.name, + field.type, + ]), + [['shipping_phone', 'Shipping Phone', 'short_text']], + ); + assert.equal((await readCheckout()).phone.custom_field_key, 'shipping_phone'); + }); + + // A publisher stated one thing, so it either happened or it did not. Every custom field + // a request names is now checked before anything at all is written, so a request that + // will be refused never gets as far as creating one. This test holds that line: were + // the checking ever to move back inside the writing, a refused request could leave a + // field behind that nobody asked for, sitting in the publisher's list, made by a change + // they were told did not happen. + it('leaves nothing behind when a later part of the same request is refused', async function () { + // An address cannot be collected into short_text, so naming this one is refused — + // but only after the recipient's name has already been given a field of its own. + const theirs = await createField({ name: 'Delivery address', type: 'short_text' }); + + await setCheckout(shipping({ address: { custom_field_key: theirs.key } }), 422); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields.map((field: { key: string }) => field.key), + [theirs.key], + 'the field made for the recipient went back with the request', + ); + assert.deepEqual( + await models.Base.knex('members_custom_field_bindings').select(), + [], + 'and so did the binding made before it', + ); + }); + + it('makes nothing when both destinations already exist', async function () { + await createField({ name: 'Recipient', type: 'short_text' }); + await createField({ name: 'Delivery address', type: 'address' }); + + await setCheckout(shipping({ name: { custom_field_key: 'recipient' } })); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields.map((field: { key: string }) => field.key), + ['recipient', 'delivery_address'], + ); + }); + + // Turning it on is one decision; where a parcel goes is not something Ghost can guess. + it('still refuses to collect an address without a country to deliver to', async function () { + const body = await setCheckout({ shipping: { collect: true } }, 422); + assert.match(body.errors[0].context, /at least one country/); + }); + + // Ghost keeps no convention about where a collected value belongs, so a request that + // collects without saying where is missing half the decision rather than deferring it. + it('refuses to collect without saying where the value lands', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + const body = await setCheckout(shipping({ name: undefined }), 422); + assert.match(body.errors[0].context, /which custom field this is collected into/); + + await setCheckout(shipping({ address: { custom_field_key: '' } }), 422); + await setCheckout({ phone: { collect: true } }, 422); + + assert.deepEqual(await models.Base.knex('members_custom_field_bindings').select(), []); + }); + + // The destination is site-wide, so it is one answer however many tiers collect it. + it('reports the destination against the thing collected', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout(shipping()); + + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'delivery_address'); + }); + + // An address form cannot be rendered without a country list. Stripe's reference + // calls it optional, but an empty collection object form-encodes to nothing, so a + // request built that way never asks Stripe to collect anything at all. + it('refuses to collect an address without countries to collect it in', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + const body = await setCheckout(shipping({ allowed_countries: undefined }), 422); + assert.match(body.errors[0].context, /at least one country/); + }); + + it('refuses a country code that is not one', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + const body = await setCheckout(shipping({ allowed_countries: ['IRL'] }), 422); + assert.match(body.errors[0].context, /2-letter country code/); + }); + + it('refuses a destination whose type is not what the port supplies', async function () { + await createField({ name: 'Delivery notes', type: 'short_text' }); + + const body = await setCheckout( + shipping({ address: { custom_field_key: 'delivery_notes' } }), + 422, + ); + assert.match(body.errors[0].context, /address field/); + }); + + // A key is stated rather than derived from a name now, so nothing has put it into + // the shape Ghost's own keys take, and a request that gets it wrong is refused + // rather than written into the column as given. + it('makes one field when two ports name the same key that does not exist yet', async function () { + await setCheckout({ + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'contact' }, + address: { custom_field_key: 'delivery' }, + }, + phone: { collect: true, custom_field_key: 'contact' }, + }); + + const { body } = await agent + .get('members/custom_fields/?filter=status:[active,archived]') + .expectStatus(200); + const named = body.members_custom_fields.filter( + (field: { key: string }) => field.key === 'contact', + ); + assert.equal(named.length, 1, 'the key both ports named was made once'); + + const { body: read } = await agent.get(`tiers/${tierId}/checkout_config/`).expectStatus(200); + const [config] = read.tiers_checkout_config; + assert.equal(config.shipping.name.custom_field_key, 'contact'); + assert.equal(config.phone.custom_field_key, 'contact'); + }); + + it('refuses a key of a shape nothing could be keyed', async function () { + const body = await setCheckout( + { phone: { collect: true, custom_field_key: 'Shipping Phone' } }, + 422, + ); + assert.match(body.errors[0].context, /lowercase letters, numbers and underscores/); + + assert.deepEqual(await models.Base.knex('members_custom_fields').select(), []); + }); + + // A key is a property name on the plain object a member's values travel as, so one + // naming something every object already has reads back as inherited rather than + // absent. Both of these are well-formed keys, which is exactly why the format check + // is not enough on its own. + it('refuses a key that names something every object already has', async function () { + for (const key of ['constructor', '__proto__']) { + const body = await setCheckout({ phone: { collect: true, custom_field_key: key } }, 422); + assert.match(body.errors[0].context, /cannot be used as a custom field key/); + } + + assert.deepEqual(await models.Base.knex('members_custom_fields').select(), []); + }); + + // Minting holds back room for a numbering suffix; a stated key is written as given, + // so what it has to fit in is the whole column and nothing less. + it('refuses a key longer than the column that holds it', async function () { + const body = await setCheckout( + { phone: { collect: true, custom_field_key: 'a'.repeat(192) } }, + 422, + ); + assert.match(body.errors[0].context, /at most 191 characters/); + + assert.deepEqual(await models.Base.knex('members_custom_fields').select(), []); + }); + + // Which kinds of thing exist is the request schema's to state, so naming one that + // does not is a malformed body rather than a lookup that came back empty. + it('refuses a kind of thing nothing can collect', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout( + { inside_leg: { collect: true, custom_field_key: 'delivery_address' } }, + 422, + ); + }); + }); + + describe('Naming one list and not the other', function () { + // A client that knows about the questions must not erase the collection by staying + // silent about it. + it('leaves a list the request does not name alone', async function () { + const size = await createField({ name: 'T-shirt size' }); + await createField({ name: 'Delivery address', type: 'address' }); + + await setCheckout({ custom_fields: [{ key: size.key }] }); + await setCheckout(shipping()); + + const config = await readCheckout(); + assert.deepEqual( + config.custom_fields.map((question: { key: string }) => question.key), + ['t_shirt_size'], + ); + assert.deepEqual(config.shipping, { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'delivery_address' }, + }); + }); + + // Every collectable thing shares one row, so leaving one alone is a property of + // the write rather than of the storage. + it('leaves a collectable thing the request does not name alone', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + await setCheckout(shipping()); + await setCheckout({ tax_number: { collect: true } }); + + const config = await readCheckout(); + assert.equal(config.shipping.collect, true); + assert.deepEqual(config.shipping.allowed_countries, ['GB']); + assert.equal(config.tax_number.collect, true); + }); + + // A tax number is collected and not kept, so it is stated as an option beside the + // countries rather than as a binding, and nothing is made to put it in. + it('collects a tax number without keeping it anywhere', async function () { + await setCheckout({ tax_number: { collect: true } }); + + assert.deepEqual((await readCheckout()).tax_number, { collect: true }); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual(body.members_custom_fields, [], 'no field was made for it'); + + assert.deepEqual( + await models.Base.knex('members_custom_field_bindings').select(), + [], + 'and nothing was bound', + ); + }); + + it('names nowhere to keep a tax number', async function () { + await setCheckout({ tax_number: { collect: true, custom_field_key: 'anything' } }, 422); + }); + + // Turning collection back on is a fresh statement of where a publisher delivers, not a + // resumption of the last one: the countries have to be given again, so a tier cannot + // quietly start delivering somewhere the publisher has since stopped. + it('stops collecting, and asks where to deliver again before it will resume', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + await setCheckout(shipping()); + await setCheckout({ shipping: { collect: false } }); + assert.equal((await readCheckout()).shipping, undefined); + + const body = await setCheckout(shipping({ allowed_countries: undefined }), 422); + assert.match(body.errors[0].context, /at least one country/); + }); + }); + + describe('When a field stops being usable', function () { + // Refused at write, tolerated at read. Archiving is reversible, so the question + // waits for the field to come back rather than being torn out. + it('keeps a question whose field was archived', async function () { + const size = await createField({ name: 'T-shirt size' }); + await setCheckout({ custom_fields: [{ key: size.key }] }); + + await setStatus(size.key, 'archived'); + assert.deepEqual( + (await readCheckout()).custom_fields.map((question: { key: string }) => question.key), + ['t_shirt_size'], + ); + + await setStatus(size.key, 'active'); + assert.deepEqual( + (await readCheckout()).custom_fields.map((question: { key: string }) => question.key), + ['t_shirt_size'], + ); + }); + + // Deleting is irreversible and already gated behind archiving, so the question goes + // with the field rather than pointing at nothing. + it('drops a question whose field was permanently deleted', async function () { + const size = await createField({ name: 'T-shirt size' }); + await setCheckout({ custom_fields: [{ key: size.key }] }); + + await setStatus(size.key, 'archived'); + await agent.delete(`members/custom_fields/${size.key}/`).expectStatus(204); + + assert.deepEqual((await readCheckout()).custom_fields, []); + }); + }); + + describe('Browsing every tier', function () { + it('returns the tiers that ask for something, so a list needs one request', async function () { + const size = await createField({ name: 'T-shirt size' }); + await setCheckout({ custom_fields: [{ key: size.key }] }); + + const { body } = await agent.get('tiers/checkout_config/').expectStatus(200); + assert.deepEqual(body.tiers_checkout_config, [ + { + tier_id: tierId, + custom_fields: [{ key: 't_shirt_size', label: null, optional: true }], + }, + ]); + }); + + it('still lists a tier that turned everything back off, collecting nothing', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout(shipping()); + await setCheckout({ shipping: { collect: false } }); + + const { body } = await agent.get('tiers/checkout_config/').expectStatus(200); + assert.deepEqual(body.tiers_checkout_config, [{ tier_id: tierId, custom_fields: [] }]); + }); + }); + + // The litmus test for the data model: if someone administers this with plain SQL, what + // states can they reach, and does anything break when they do. Each of these reaches a + // state the API refuses to create, and asserts the system stays sane in it. + describe('When the database is edited by hand', function () { + async function collectShipping() { + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout(shipping()); + } + + // The options row is the other half that could disagree. It holds no opinion about + // whether anything is collected, so one left behind changes nothing. + it('collects nothing when only the options row is left', async function () { + await collectShipping(); + await models.Base.knex('members_custom_field_bindings').del(); + + const [options] = await models.Base.knex('products_checkout_config').select(); + assert.ok(options, 'the options row outlived the collection'); + assert.deepEqual(await readCheckout(), { tier_id: tierId, custom_fields: [] }); + }); + + it('takes the binding with the field when a definition is deleted', async function () { + await collectShipping(); + // Deleting takes archiving first, which is the API's own rule; the cascade below + // is the database's, and it is what a hand-written DELETE would hit too. + await setStatus('delivery_address', 'archived'); + await agent.delete('members/custom_fields/delivery_address/').expectStatus(204); + + // Cascade, so no binding can name a field that is gone. The recipient's name is + // kept elsewhere, so its binding survives — but a recipient with nowhere to send + // the parcel is not a delivery, so the tier reports collecting nothing. + assert.deepEqual( + (await models.Base.knex('members_custom_field_bindings').select()).map( + (row: { port: string }) => row.port, + ), + ['shipping_name'], + ); + assert.equal((await readCheckout()).shipping, undefined); + }); + + it('keeps the binding when a definition is archived, and resumes on restore', async function () { + await collectShipping(); + await setStatus('delivery_address', 'archived'); + + // Archiving is reversible, so the binding waits rather than being torn out, and + // the tier goes on reporting where the value goes. + const [binding] = await models.Base.knex('members_custom_field_bindings') + .where('port', 'shipping_address') + .select(); + assert.equal(binding.custom_field_key, 'delivery_address'); + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'delivery_address'); + + await setStatus('delivery_address', 'active'); + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'delivery_address'); + }); + }); + + // Asking for a field and collecting into it are two writers aimed at one destination: + // the checkout shows the question and the widget together, and whichever the webhook + // writes second is what the field holds. Allowed rather than refused, because a + // destination is not exclusive — another tier can already be writing into the same + // field — so refusing it here would only forbid the one arrangement Ghost can see. + describe('One field, asked for and collected into', function () { + it('accepts both, in one statement or in two', async function () { + const field = await createField({ name: 'Contact' }); + + await setCheckout({ + custom_fields: [{ key: field.key }], + phone: { collect: true, custom_field_key: field.key }, + }); + + const config = await readCheckout(); + assert.deepEqual( + config.custom_fields.map((question: { key: string }) => question.key), + [field.key], + ); + assert.equal(config.phone.custom_field_key, field.key); + }); + }); + + // A destination is stated, not remembered. The request says where every value it + // collects lands, so the same statement made twice reaches the same field and two tiers + // making different statements keep them apart. + describe('A destination the publisher chose', function () { + it('is still theirs after turning collection off and on', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout(shipping()); + + await setCheckout({ shipping: { collect: false } }); + await setCheckout(shipping()); + + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'delivery_address'); + }); + + // Another tier binding the same port first says nothing about this one: each states + // its own destination, and neither is worked out from what the other settled on. + it('is not decided by another tier that bound the port first', async function () { + const [existing] = await models.Base.knex('products').where('id', tierId); + const secondId = 'ffffffffffffffffffffffff'; + await models.Base.knex('products').insert({ + ...existing, + id: secondId, + name: 'Digital', + slug: 'digital-second', + }); + + await createField({ name: 'Delivery address', type: 'address' }); + await createField({ name: 'Digital address', type: 'address' }); + + // The other tier goes first, so its binding is the older one. + await agent + .put(`tiers/${secondId}/checkout_config/`) + .body({ + tiers_checkout_config: [shipping({ address: { custom_field_key: 'digital_address' } })], + }) + .expectStatus(200); + + await setCheckout(shipping()); + + assert.equal( + (await readCheckout()).shipping.address.custom_field_key, + 'delivery_address', + 'this tier kept its own destination', + ); + const { body } = await agent.get(`tiers/${secondId}/checkout_config/`).expectStatus(200); + assert.equal( + body.tiers_checkout_config[0].shipping.address.custom_field_key, + 'digital_address', + 'and the other kept its own', + ); + }); + }); + + // A binding is the collecting: there is one and the tier collects, or there is none and + // it does not. What the database does to a row nobody is looking at is the whole of the + // rest of it. + describe('When collection stops', function () { + const bindingsFor = (port: string) => + models.Base.knex('members_custom_field_bindings').where('port', port).select(); + + it('forgets the binding rather than keeping it switched off', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout(shipping()); + + await setCheckout({ shipping: { collect: false } }); + + assert.deepEqual(await models.Base.knex('members_custom_field_bindings').select(), []); + assert.deepEqual(await readCheckout(), { tier_id: tierId, custom_fields: [] }); + }); + + // Nothing is left pointing at it, so the same request that made it once makes it again. + it('makes the field again once it has been deleted', async function () { + await setCheckout({ + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }); + await setCheckout({ shipping: { collect: false } }); + await setStatus('shipping_address', 'archived'); + await agent.delete('members/custom_fields/shipping_address/').expectStatus(204); + + await setCheckout({ + shipping: { + collect: true, + allowed_countries: ['GB'], + name: { custom_field_key: 'shipping_name' }, + address: { custom_field_key: 'shipping_address' }, + }, + }); + + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'shipping_address'); + }); + + // Deleting the tier takes its bindings, so nothing it configured outlives it. + it('takes a binding with its tier', async function () { + const [existing] = await models.Base.knex('products').where('id', tierId); + const secondId = 'ffffffffffffffffffffffff'; + await models.Base.knex('products').insert({ + ...existing, + id: secondId, + name: 'Digital', + slug: 'digital-second', + }); + + await agent + .put(`tiers/${secondId}/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); + + await models.Base.knex('products').where('id', secondId).del(); + + assert.deepEqual(await bindingsFor('shipping_address'), []); + }); + }); + + // Archiving a bound field stops the tier that bound it, silently and by design. A second + // tier asking to collect into the same field is a publisher walking into that state + // rather than out of it, so it is refused where the first tier's collection is left + // alone: one of them chose this before the archiving, and the other is choosing it after. + describe('A destination archived before another tier wants it', function () { + it('is refused, rather than bound to collect nothing', async function () { + const [existing] = await models.Base.knex('products').where('id', tierId); + const secondId = 'ffffffffffffffffffffffff'; + await models.Base.knex('products').insert({ + ...existing, + id: secondId, + name: 'Digital', + slug: 'digital-second', + }); + + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout(shipping()); + await setStatus('delivery_address', 'archived'); + + await agent + .put(`tiers/${secondId}/checkout_config/`) + .body({ tiers_checkout_config: [shipping()] }) + .expectStatus(422); + + assert.deepEqual( + await models.Base.knex('members_custom_field_bindings') + .where('product_id', secondId) + .select(), + [], + ); + + // The tier that bound it first is untouched: it still reports where the address + // goes, and restoring the field is what starts it collecting again. + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'delivery_address'); + + await setStatus('delivery_address', 'active'); + await agent + .put(`tiers/${secondId}/checkout_config/`) + .body({ tiers_checkout_config: [shipping()] }) + .expectStatus(200); + + const { body: fields } = await agent + .get('members/custom_fields/?filter=status:[active,archived]') + .expectStatus(200); + assert.deepEqual( + fields.members_custom_fields + .filter((field: { type: string }) => field.type === 'address') + .map((field: { key: string }) => field.key), + ['delivery_address'], + 'and no second address field was made along the way', + ); + }); + }); + + // A destination belongs to a tier, not to the site: the binding is keyed by tier and + // port, so each tier names its own and one turning collection off says nothing about + // any other. What these pin is that the two stay independent in both directions. + describe('Two tiers, one destination', function () { + it('leaves another tier collecting when this one stops', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + + // Copied from the tier the suite already has, because what is being tested is + // what happens between two of them rather than how one is made. + const [existing] = await models.Base.knex('products').where('id', tierId); + const secondId = 'ffffffffffffffffffffffff'; + await models.Base.knex('products').insert({ + ...existing, + id: secondId, + name: 'Digital', + slug: 'digital-second', + }); + + await setCheckout(shipping()); + + // The digital tier does not ship anything, so a publisher turns it off there. + await agent + .put(`tiers/${secondId}/checkout_config/`) + .body({ tiers_checkout_config: [{ shipping: { collect: false } }] }) + .expectStatus(200); + + // The print tier never changed, so it must still be collecting into its field. + const config = await readCheckout(); + assert.equal(config.shipping.collect, true); + assert.equal(config.shipping.address.custom_field_key, 'delivery_address'); + }); + + // The other half of the same rule. The field outlives every binding into it, because + // it holds everything collected so far, and stating it again reaches the same one. + it('keeps the field once no tier collects into it', async function () { + await createField({ name: 'Delivery address', type: 'address' }); + await setCheckout(shipping()); + + await setCheckout({ shipping: { collect: false } }); + + // Off, so the tier reports nothing and the checkout asks for nothing. + assert.deepEqual(await models.Base.knex('members_custom_field_bindings').select(), []); + assert.deepEqual(await readCheckout(), { tier_id: tierId, custom_fields: [] }); + + const { body } = await agent.get('members/custom_fields/').expectStatus(200); + assert.deepEqual( + body.members_custom_fields.map((field: { key: string }) => field.key), + ['delivery_address', 'shipping_name'], + 'the fields it collected into are still there', + ); + + // And on again, it is the same field rather than a second one beside it. + await setCheckout(shipping()); + assert.equal((await readCheckout()).shipping.address.custom_field_key, 'delivery_address'); + }); + }); + + // A tier that asks for nothing and a tier that does not exist read the same off the + // join, and answering an empty configuration for the second would tell a client the + // tier is there. + describe('A tier that does not exist', function () { + it('404s rather than reading as unconfigured', async function () { + await agent.get('tiers/6a8dbb6a2668becb3f92f000/checkout_config/').expectStatus(404); + }); + + // Every table a write touches references the tier, so the tier is checked before any + // of them is written. Otherwise the write reaches a foreign key and answers with a + // database error, where reading the same tier answers 404. + it('404s a write, rather than failing against a foreign key', async function () { + await agent + .put('tiers/6a8dbb6a2668becb3f92f000/checkout_config/') + .body({ tiers_checkout_config: [{ tax_number: { collect: true } }] }) + .expectStatus(404); + }); + }); + + describe('Flag', function () { + // Reading is open, because a tier that collects nothing reads the same either way and + // the checkout has to build its session whatever the flag says. Configuring is not. + it('still reads with the flag off', async function () { + mockManager.mockLabsDisabled('membersCustomFields'); + const { body } = await agent.get(`tiers/${tierId}/checkout_config/`).expectStatus(200); + assert.deepEqual(body.tiers_checkout_config[0].custom_fields, []); + }); + + it('cannot be configured with the flag off', async function () { + mockManager.mockLabsDisabled('membersCustomFields'); + await agent + .put(`tiers/${tierId}/checkout_config/`) + .body({ tiers_checkout_config: [{ custom_fields: [] }] }) + .expectStatus(404); + }); + + // 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); + assert.equal(body.tiers[0].checkout, undefined); + }); + }); +}); diff --git a/ghost/core/test/integration/exporter/exporter.test.js b/ghost/core/test/integration/exporter/exporter.test.js index bb7bf345138..03295b5715e 100644 --- a/ghost/core/test/integration/exporter/exporter.test.js +++ b/ghost/core/test/integration/exporter/exporter.test.js @@ -60,6 +60,7 @@ describe('Exporter', function () { 'members_click_events', 'members_created_events', 'members_current_subscription', + 'members_custom_field_bindings', 'members_custom_field_values', 'members_custom_fields', 'members_email_change_events', @@ -97,6 +98,8 @@ describe('Exporter', function () { 'posts_tags', 'products', 'products_benefits', + 'products_checkout_config', + 'products_checkout_fields', 'recommendation_click_events', 'recommendation_subscribe_events', 'recommendations', 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 9acbc4aa6dc..0cf5224feaf 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 = 'dff334a3ab0ee69919d330d3c46752a2'; + const currentSchemaHash = '0d83dfdf9142d606e0660871c5adcf1a'; const currentFixturesHash = '1727a789194847da33d68dd95301b416'; const currentSettingsHash = '6ea42a00cca61a1ba87f66eb6e25a78a'; const currentRoutesHash = 'd8c25fa01bf6d22a2bcb05ba0de70dc1'; From e239f719010f3a3f35764d950effacae569b6e3f Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Wed, 26 Aug 2026 17:52:41 -0400 Subject: [PATCH 6/6] Added local TrafficAnalytics development command (#30328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/ghost/issue/NY-1550 This is a development only change that should have no user impact. _Note: this PR depends on [this PR](https://github.com/TryGhost/TrafficAnalytics/pull/859) in the Traffic Analytics repository._ ## Summary When developing features that touch both Ghost and the Traffic Analytics service, it can be useful to run your local version of both services locally. Currently, `pnpm dev:analytics` runs Ghost against the latest published version of the Traffic Analytics service, but running against your own locally running instance of the service was not possible out of the box — this makes it as simple as running `pnpm dev:analytics:local` in Ghost, and `yarn dev:ghost` in Traffic Analytics --- compose.dev.yaml | 2 ++ docs/contributing/development-setup.md | 19 ++++++++++--------- package.json | 1 + 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/compose.dev.yaml b/compose.dev.yaml index 9749999ab95..af8d62ee90a 100644 --- a/compose.dev.yaml +++ b/compose.dev.yaml @@ -144,6 +144,8 @@ services: context: ./docker/dev-gateway dockerfile: Dockerfile container_name: ghost-dev-gateway + environment: + ANALYTICS_PROXY_TARGET: ${ANALYTICS_PROXY_TARGET:-analytics:3000} ports: - '2368:80' - '80:80' diff --git a/docs/contributing/development-setup.md b/docs/contributing/development-setup.md index e5173f34c3c..2380cbfbb6f 100644 --- a/docs/contributing/development-setup.md +++ b/docs/contributing/development-setup.md @@ -105,15 +105,16 @@ runs. Run one root command at a time. Each variant includes the standard development environment and adds the listed tooling: -| Command | Use it when working on | -| -------------------- | --------------------------------------------------------------------------------------- | -| `pnpm dev` | Ghost Core, Admin, or Portal | -| `pnpm dev:public` | Comments UI, Signup Form, Search, Announcement Bar, or Admin Toolbar | -| `pnpm dev:lexical` | Koenig's Lexical editor inside Ghost Admin | -| `pnpm dev:analytics` | Tinybird-backed analytics; also exposes Tinybird on port `7181` | -| `pnpm dev:storage` | S3-compatible storage through MinIO on ports `9000` and `9001` | -| `pnpm dev:stripe` | Stripe webhooks; requires `STRIPE_SECRET_KEY` in the environment or a local `.env` file | -| `pnpm dev:full` | Public app watchers plus analytics, storage, and Stripe | +| Command | Use it when working on | +| -------------------------- | --------------------------------------------------------------------------------------------- | +| `pnpm dev` | Ghost Core, Admin, or Portal | +| `pnpm dev:public` | Comments UI, Signup Form, Search, Announcement Bar, or Admin Toolbar | +| `pnpm dev:lexical` | Koenig's Lexical editor inside Ghost Admin | +| `pnpm dev:analytics` | Tinybird-backed analytics with the latest published version of the Traffic Analytics service | +| `pnpm dev:analytics:local` | Tinybird-backed analytics with your locally running instance of the Traffic Analytics service | +| `pnpm dev:storage` | S3-compatible storage through MinIO on ports `9000` and `9001` | +| `pnpm dev:stripe` | Stripe webhooks; requires `STRIPE_SECRET_KEY` in the environment or a local `.env` file | +| `pnpm dev:full` | Public app watchers plus analytics, storage, and Stripe | Copy [`.env.example`](../../.env.example) to `.env` only when you need an optional integration. Never commit credentials or the local `.env` file. diff --git a/package.json b/package.json index 2993c7c281f..4c5dcf5f9a3 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "dev:public": "pnpm nx run ghost-monorepo:docker:dev:public", "dev:full": "DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml -f compose.dev.storage.yaml' ./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev:public", "dev:analytics": "DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml' pnpm nx run ghost-monorepo:docker:dev", + "dev:analytics:local": "ANALYTICS_PROXY_TARGET=traffic-analytics-local:3000 DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml' pnpm nx run ghost-monorepo:docker:dev", "dev:storage": "DEV_COMPOSE_FILES='-f compose.dev.storage.yaml' pnpm nx run ghost-monorepo:docker:dev", "dev:stripe": "./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev", "dev:all": "DEV_COMPOSE_FILES='-f compose.dev.analytics.yaml -f compose.dev.storage.yaml' ./docker/stripe/with-stripe.sh pnpm nx run ghost-monorepo:docker:dev",