diff --git a/apps/portal/package.json b/apps/portal/package.json index a6718b6e10e..55f15c926d5 100644 --- a/apps/portal/package.json +++ b/apps/portal/package.json @@ -1,6 +1,6 @@ { "name": "@tryghost/portal", - "version": "2.68.13", + "version": "2.68.14", "license": "MIT", "repository": "https://github.com/TryGhost/Ghost", "author": "Ghost Foundation", diff --git a/apps/portal/src/actions.js b/apps/portal/src/actions.js index 662afa61dbc..35063506f32 100644 --- a/apps/portal/src/actions.js +++ b/apps/portal/src/actions.js @@ -1,5 +1,6 @@ import setupGhostApi from './utils/api'; import {chooseBestErrorMessage} from './utils/errors'; +import {getGiftRedemptionSuccessMessage} from './utils/gift-redemption-notification'; import {createNotification, createPopupNotification, getMemberEmail, getMemberName, getProductCadenceFromPrice, removePortalLinkFromUrl, getRefDomain} from './utils/helpers'; import {t} from './utils/i18n'; @@ -229,13 +230,18 @@ async function redeemGift({data, state, api}) { status: 'success', autoHide: true, closeable: true, - state + state, + message: getGiftRedemptionSuccessMessage({member}) }); + removePortalLinkFromUrl(); return { action: 'redeemGift:success', member, - page: 'accountHome', + showPopup: false, + lastPage: null, + pageQuery: '', + popupNotification: null, notification, notificationSequence: notification.count }; @@ -243,10 +249,10 @@ async function redeemGift({data, state, api}) { const integrityToken = await api.member.getIntegrityToken(); const redirectUrl = new URL(state?.site?.url || window.location.href); - const hashParams = new URLSearchParams({ + redirectUrl.search = new URLSearchParams({ giftRedemption: 'true' - }); - redirectUrl.hash = `/portal/account?${hashParams.toString()}`; + }).toString(); + redirectUrl.hash = ''; const {otc_ref: otcRef, inboxLinks} = await api.member.sendMagicLink({ email: (email || '').trim(), diff --git a/apps/portal/src/components/notification.js b/apps/portal/src/components/notification.js index aabf708c2e9..c427f1b13c9 100644 --- a/apps/portal/src/components/notification.js +++ b/apps/portal/src/components/notification.js @@ -7,6 +7,7 @@ import {ReactComponent as CloseIcon} from '../images/icons/close.svg'; import {ReactComponent as CheckmarkIcon} from '../images/icons/checkmark-fill.svg'; import {ReactComponent as WarningIcon} from '../images/icons/warning-fill.svg'; import NotificationParser, {clearURLParams} from '../utils/notifications'; +import {getGiftRedemptionSuccessMessage} from '../utils/gift-redemption-notification'; import {getPortalLink} from '../utils/helpers'; import {t} from '../utils/i18n'; @@ -108,12 +109,16 @@ const NotificationText = ({type, status, message, context}) => { ); } else if (type === 'giftRedeem' && status === 'success') { // TODO: Add translation strings once copy has been finalised + const successMessage = getGiftRedemptionSuccessMessage({member: context.member}) + || 'Gift redeemed! You\'re all set.'; + return (

- {'Gift redeemed! You\'re all set.'} + {successMessage}

); } else if (type === 'giftRedeem' && status === 'error') { + // TODO: Add translation strings once copy has been finalised return (

{'We couldn\'t redeem this gift for your account.'} diff --git a/apps/portal/src/utils/gift-redemption-notification.js b/apps/portal/src/utils/gift-redemption-notification.js index f14dfd64918..bc8510c6809 100644 --- a/apps/portal/src/utils/gift-redemption-notification.js +++ b/apps/portal/src/utils/gift-redemption-notification.js @@ -1,3 +1,4 @@ +import {getMemberTierName, getSubscriptionExpiry} from './helpers'; import {t} from './i18n'; export function getGiftDurationLabel({cadence, duration} = {}) { @@ -12,6 +13,16 @@ export function getGiftDurationLabel({cadence, duration} = {}) { : t('{months} months', {months: duration}); } +export function getGiftRedemptionSuccessMessage({member} = {}) { + const tierName = getMemberTierName({member}); + const expiryDate = getSubscriptionExpiry({member}); + if (!tierName || !expiryDate) { + return null; + } + // TODO: Add translation strings once copy has been finalised + return `You now have access to ${tierName} until ${expiryDate}. Enjoy!`; +} + export function getGiftRedemptionErrorMessage(error) { const subtitle = error?.message && error.message !== 'Failed to load gift data' ? error.message diff --git a/apps/portal/src/utils/notifications.js b/apps/portal/src/utils/notifications.js index 4ca670ff681..57d416a5ee4 100644 --- a/apps/portal/src/utils/notifications.js +++ b/apps/portal/src/utils/notifications.js @@ -12,17 +12,12 @@ const getURLParam = ({searchParams, hashParams}, name) => { return searchParams.get(name) ?? hashParams.get(name); }; -export const handleGiftRedemptionAction = ({status}) => { - const successStatus = JSON.parse(status); - +export const handleGiftRedemptionAction = ({success}) => { return { type: 'giftRedeem', - status: successStatus ? 'success' : 'error', - duration: successStatus ? 5000 : 3000, - autoHide: successStatus, - ...(successStatus ? { - message: 'Gift redeemed! You\'re all set.' // TODO: Add translation strings once copy has been finalised - } : {}) + status: success ? 'success' : 'error', + duration: success ? 5000 : 3000, + autoHide: success }; }; @@ -100,8 +95,9 @@ export default function NotificationParser({billingOnly = false} = {}) { return handleStripeActions({status: stripeStatus, billingOnly}); } - if ((giftRedemption || action === 'giftRedeem') && successStatus && !billingOnly) { - return handleGiftRedemptionAction({status: successStatus}); + if (giftRedemption && successStatus) { + const success = successStatus === 'true'; + return handleGiftRedemptionAction({success}); } if (action && successStatus && !billingOnly) { diff --git a/apps/portal/test/actions.test.ts b/apps/portal/test/actions.test.ts index 8ff8d17d1d0..17dfb0f6d8c 100644 --- a/apps/portal/test/actions.test.ts +++ b/apps/portal/test/actions.test.ts @@ -48,6 +48,8 @@ describe('signup action', () => { describe('redeemGift action', () => { test('redeems a gift directly for a logged-in member and refreshes member data', async () => { + window.history.replaceState({}, '', '/#/portal/gift/redeem/gift-token-123'); + const mockApi = { gift: { redeem: vi.fn(() => Promise.resolve({ @@ -62,7 +64,14 @@ describe('redeemGift action', () => { name: 'Jamie Larson', email: 'jamie@example.com', paid: true, - status: 'gift' + status: 'gift', + subscriptions: [{ + status: 'active', + tier: { + name: 'Premium', + expiry_at: '2027-05-29T12:00:00.000Z' + } + }] })), getIntegrityToken: vi.fn(), sendMagicLink: vi.fn() @@ -101,15 +110,23 @@ describe('redeemGift action', () => { expect(mockApi.member.sendMagicLink).not.toHaveBeenCalled(); expect(result).toMatchObject({ action: 'redeemGift:success', - page: 'accountHome', + showPopup: false, + lastPage: null, + pageQuery: '', + popupNotification: null, member: { status: 'gift' }, notification: { type: 'giftRedeem', - status: 'success' + status: 'success', + message: 'You now have access to Premium until 29 May 2027. Enjoy!' } }); + // Ensure the account page is no longer rendered after redemption. + expect(result).not.toHaveProperty('page'); + // Redemption hash is cleared so a refresh doesn't re-trigger the redeemed-token flow. + expect(window.location.hash).toBe(''); }); test('sends a subscribe magic link with the gift token and redirects back to Portal account', async () => { @@ -124,7 +141,14 @@ describe('redeemGift action', () => { url: 'https://example.com/' }, pageData: { - token: 'gift-token-123' + token: 'gift-token-123', + gift: { + cadence: 'month', + duration: 3, + tier: { + name: 'Ultra' + } + } } }; @@ -139,12 +163,14 @@ describe('redeemGift action', () => { api: mockApi }); + const expectedRedirect = 'https://example.com/?giftRedemption=true'; + expect(mockApi.member.sendMagicLink).toHaveBeenCalledWith({ email: 'jamie@example.com', emailType: 'subscribe', integrityToken: 'token-123', includeOTC: true, - redirect: 'https://example.com/#/portal/account?giftRedemption=true', + redirect: expectedRedirect, giftToken: 'gift-token-123', name: 'Jamie Larson' }); @@ -156,7 +182,7 @@ describe('redeemGift action', () => { pageData: { token: 'gift-token-123', email: 'jamie@example.com', - redirect: 'https://example.com/#/portal/account?giftRedemption=true' + redirect: expectedRedirect } }); }); diff --git a/apps/portal/test/unit/components/notification.test.js b/apps/portal/test/unit/components/notification.test.js index 25a2f2654d0..8971bdfab10 100644 --- a/apps/portal/test/unit/components/notification.test.js +++ b/apps/portal/test/unit/components/notification.test.js @@ -91,7 +91,6 @@ describe('Notification', () => { NotificationParser.mockReturnValue({ type: 'giftRedeem', status: 'success', - message: 'Gift redeemed! You\'re all set.', autoHide: true, duration: 5000 }); @@ -130,7 +129,6 @@ describe('Notification', () => { NotificationParser.mockReturnValue({ type: 'giftRedeem', status: 'success', - message: 'Gift redeemed! You\'re all set.', autoHide: true, duration: 5000 }); @@ -165,4 +163,46 @@ describe('Notification', () => { expect(container.querySelector('.gh-portal-notification')).not.toHaveClass('slideout'); }); + + test('derives gift redemption success message from member tier in context', async () => { + NotificationParser.mockReturnValue({ + type: 'giftRedeem', + status: 'success', + autoHide: true, + duration: 5000 + }); + + const doAction = vi.fn(); + const site = { + url: 'https://example.com', + title: 'Example Site' + }; + + const {getByText} = render( + + + + ); + + await waitFor(() => { + expect(getByText('You now have access to Ultra until 29 May 2027. Enjoy!')).toBeInTheDocument(); + }); + }); }); diff --git a/apps/portal/test/unit/notifications.test.js b/apps/portal/test/unit/notifications.test.js index a3901a2be85..fa95b1a78c5 100644 --- a/apps/portal/test/unit/notifications.test.js +++ b/apps/portal/test/unit/notifications.test.js @@ -9,24 +9,11 @@ describe('notifications utils', () => { test('reads gift redemption params from action subscribe plus portal hash query', () => { window.history.replaceState({}, '', '/?action=subscribe&success=true#/portal/account?giftRedemption=true'); - expect(NotificationParser()).toMatchObject({ - type: 'giftRedeem', - status: 'success', - autoHide: true, - duration: 5000, - message: 'Gift redeemed! You\'re all set.' - }); - }); - - test('reads gift redemption params from legacy giftRedeem action', () => { - window.history.replaceState({}, '', '/#/portal/account?action=giftRedeem&success=true'); - - expect(NotificationParser()).toMatchObject({ + expect(NotificationParser()).toEqual({ type: 'giftRedeem', status: 'success', autoHide: true, - duration: 5000, - message: 'Gift redeemed! You\'re all set.' + duration: 5000 }); }); diff --git a/ghost/admin/app/helpers/parse-member-event.js b/ghost/admin/app/helpers/parse-member-event.js index eab5b5fd940..623bef5cbfd 100644 --- a/ghost/admin/app/helpers/parse-member-event.js +++ b/ghost/admin/app/helpers/parse-member-event.js @@ -270,12 +270,11 @@ export default class ParseMemberEventHelper extends Helper { const duration = event.data.duration; const cadenceLabel = duration === 1 ? event.data.cadence : event.data.cadence + 's'; - return `Purchased a gift subscription for ${formattedAmount} (${tierName}, ${duration} ${cadenceLabel})`; + return `Purchased gift subscription for ${formattedAmount} (${tierName}, ${duration} ${cadenceLabel})`; } if (event.type === 'gift_redemption_event') { - const tierName = event.data.tier_name; - return `Started paid subscription (${tierName}) via gift`; + return 'started paid subscription via gift'; } } @@ -358,6 +357,10 @@ export default class ParseMemberEventHelper extends Helper { return formattedAmount; } + if (event.type === 'gift_redemption_event') { + return event.data.tier_name; + } + return; } diff --git a/ghost/admin/public/assets/icons/event-gift.svg b/ghost/admin/public/assets/icons/event-gift.svg index bcbaae2d703..348b52d797c 100644 --- a/ghost/admin/public/assets/icons/event-gift.svg +++ b/ghost/admin/public/assets/icons/event-gift.svg @@ -1,3 +1,3 @@ - + diff --git a/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js b/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js index f60d9bc546b..be7809e87a6 100644 --- a/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js +++ b/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js @@ -65,6 +65,7 @@ class AdapterCacheRedis extends BaseCacheAdapter { this.refreshAheadFactor = config.refreshAheadFactor || 0; this.getTimeoutMilliseconds = config.getTimeoutMilliseconds || null; this.currentlyExecutingBackgroundRefreshes = new Set(); + this.currentlyExecutingReads = new Map(); this._keyPrefix = config.keyPrefix || ''; this._prefixHashInitInFlight = null; this.redisClient.on('error', this.handleRedisError); @@ -240,9 +241,30 @@ class AdapterCacheRedis extends BaseCacheAdapter { } return result; } else { - const data = await fetchData(); - await this.set(key, data); // We don't use `internalKey` here because `set` handles it - return data; + if (!internalKey) { + return fetchData(); + } + if (this.currentlyExecutingReads.has(internalKey)) { + return this.currentlyExecutingReads.get(internalKey); + } + const fetchPromise = fetchData(); + const resultPromise = fetchPromise.catch((err) => { + logging.error(err); + }); + fetchPromise.then(async (data) => { + try { + debug('set', internalKey); + await this.cache.set(internalKey, data); + } catch (err) { + logging.error(err); + } + }).catch(() => { + // fetchData rejection — already logged by resultPromise + }).finally(() => { + this.currentlyExecutingReads.delete(internalKey); + }); + this.currentlyExecutingReads.set(internalKey, resultPromise); + return resultPromise; } } catch (err) { logging.error(err); diff --git a/ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js b/ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js index 5f53c496a80..223b29f0efd 100644 --- a/ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js +++ b/ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js @@ -143,6 +143,28 @@ SCENARIOS.forEach(({label, wrap}) => { }); }); + describe('concurrent cache miss coalescing', function () { + it('calls fetchData only once when multiple callers miss simultaneously', async function () { + const cache = createCache(); + const fetcher = sinon.stub().resolves('shared'); + + const [v1, v2, v3] = await Promise.all([ + cache.get('same-key', fetcher), + cache.get('same-key', fetcher), + cache.get('same-key', fetcher) + ]); + + assert.equal(fetcher.callCount, 1); + assert.equal(v1, 'shared'); + assert.equal(v2, 'shared'); + assert.equal(v3, 'shared'); + + const cached = await cache.get('same-key', fetcher); + assert.equal(fetcher.callCount, 1); + assert.equal(cached, 'shared'); + }); + }); + describe('get with fetchData (error paths)', function () { it('does not cache errors — a subsequent call retries fetchData', async function () { const cache = createCache(); diff --git a/ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js b/ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js index 7b3fc9f53df..1e9b177da61 100644 --- a/ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js +++ b/ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js @@ -170,6 +170,145 @@ describe('Adapter Cache Redis', function () { sinon.assert.calledOnce(logging.error); }); + describe('concurrent cache miss coalescing', function () { + it('calls fetchData only once when multiple callers miss simultaneously', async function () { + const KEY = 'concurrent-miss'; + const cacheStub = createCacheStub(); + cacheStub.get.resolves(null); + cacheStub.set.resolves(); + const cache = new RedisCache({cache: cacheStub}); + + const fetchData = sinon.stub().resolves('shared value'); + + const [v1, v2, v3] = await Promise.all([ + cache.get(KEY, fetchData), + cache.get(KEY, fetchData), + cache.get(KEY, fetchData) + ]); + + assert.equal(fetchData.callCount, 1); + assert.equal(v1, 'shared value'); + assert.equal(v2, 'shared value'); + assert.equal(v3, 'shared value'); + }); + + it('propagates fetchData rejection to all concurrent callers', async function () { + const KEY = 'concurrent-miss-error'; + const cacheStub = createCacheStub(); + cacheStub.get.resolves(null); + const cache = new RedisCache({cache: cacheStub}); + + const fetchData = sinon.stub().rejects(new Error('upstream down')); + + const [v1, v2] = await Promise.all([ + cache.get(KEY, fetchData), + cache.get(KEY, fetchData) + ]); + + assert.equal(fetchData.callCount, 1); + assert.equal(v1, undefined); + assert.equal(v2, undefined); + sinon.assert.calledOnce(logging.error); + }); + + it('allows retry after a coalesced fetch rejection', async function () { + const KEY = 'concurrent-miss-retry'; + let cachedValue = null; + const cacheStub = createCacheStub(); + cacheStub.get.callsFake(async () => cachedValue); + cacheStub.set.callsFake(async (_key, value) => { + cachedValue = value; + }); + const cache = new RedisCache({cache: cacheStub}); + + const fetchData = sinon.stub(); + fetchData.onFirstCall().rejects(new Error('transient')); + fetchData.onSecondCall().resolves('recovered'); + + await Promise.all([ + cache.get(KEY, fetchData), + cache.get(KEY, fetchData) + ]); + assert.equal(fetchData.callCount, 1); + + const value = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 2); + assert.equal(value, 'recovered'); + }); + + it('does not coalesce fetches across a prefix_hash cycle (reset)', async function () { + let prefixHash = 'gen1'; + const cacheStore = new Map(); + const redisClient = { + on: sinon.stub(), + get: sinon.stub().callsFake(async (k) => { + if (k === 'prefix_hash') { + return prefixHash; + } + return null; + }), + set: sinon.stub().resolves('OK') + }; + const cacheInstance = { + get: sinon.stub().callsFake(async k => cacheStore.get(k) ?? null), + set: sinon.stub().callsFake(async (k, v) => { + cacheStore.set(k, v); + }), + ttl: sinon.stub(), + store: {getClient: () => redisClient} + }; + const cache = new RedisCache({cache: cacheInstance}); + + let resolveSlow; + const slowFetch = sinon.stub().returns(new Promise((resolve) => { + resolveSlow = resolve; + })); + const fastFetch = sinon.stub().resolves('post-reset'); + + const pA = cache.get('k', slowFetch); + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + assert.equal(slowFetch.callCount, 1); + + prefixHash = 'gen2'; + + const pB = cache.get('k', fastFetch); + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + assert.equal(fastFetch.callCount, 1, 'post-reset caller must not join pre-reset in-flight fetch'); + + resolveSlow('pre-reset'); + const [vA, vB] = await Promise.all([pA, pB]); + + assert.equal(vA, 'pre-reset'); + assert.equal(vB, 'post-reset'); + assert.equal(cacheStore.get('gen1k'), 'pre-reset'); + assert.equal(cacheStore.get('gen2k'), 'post-reset'); + }); + + it('fetches independently for different keys', async function () { + const cacheStub = createCacheStub(); + cacheStub.get.resolves(null); + cacheStub.set.resolves(); + const cache = new RedisCache({cache: cacheStub}); + + const fetchA = sinon.stub().resolves('value-a'); + const fetchB = sinon.stub().resolves('value-b'); + + const [v1, v2] = await Promise.all([ + cache.get('key-a', fetchA), + cache.get('key-b', fetchB) + ]); + + assert.equal(fetchA.callCount, 1); + assert.equal(fetchB.callCount, 1); + assert.equal(v1, 'value-a'); + assert.equal(v2, 'value-b'); + }); + }); + it('returns the cached value when background refresh fails', async function () { const KEY = 'bg-refresh-error'; let cachedValue = null;