diff --git a/apps/admin-x-framework/src/api/stats.ts b/apps/admin-x-framework/src/api/stats.ts index 9989821bd65..5946659bb47 100644 --- a/apps/admin-x-framework/src/api/stats.ts +++ b/apps/admin-x-framework/src/api/stats.ts @@ -33,6 +33,7 @@ export type MemberCountHistoryResponseType = { paid: number; free: number; comped: number; + gift: number; } }; } diff --git a/apps/admin-x-framework/src/test/msw-utils.ts b/apps/admin-x-framework/src/test/msw-utils.ts index 4724d6ed1e8..169cb3cdf3a 100644 --- a/apps/admin-x-framework/src/test/msw-utils.ts +++ b/apps/admin-x-framework/src/test/msw-utils.ts @@ -85,7 +85,7 @@ export const fixtures = { {date: '2024-01-01', paid: 100, free: 500, comped: 10, paid_subscribed: 5, paid_canceled: 2}, {date: '2024-01-02', paid: 102, free: 505, comped: 10, paid_subscribed: 3, paid_canceled: 1} ], - meta: {totals: {paid: 102, free: 505, comped: 10}} + meta: {totals: {paid: 102, free: 505, comped: 10, gift: 8}} }, mrrHistory: { diff --git a/apps/admin-x-framework/src/test/responses/member_count_history.json b/apps/admin-x-framework/src/test/responses/member_count_history.json index 0bd9a3970a2..6569cfdcf38 100644 --- a/apps/admin-x-framework/src/test/responses/member_count_history.json +++ b/apps/admin-x-framework/src/test/responses/member_count_history.json @@ -45,7 +45,8 @@ "totals": { "paid": 110, "free": 530, - "comped": 12 + "comped": 12, + "gift": 8 } } } \ No newline at end of file diff --git a/apps/admin-x-settings/src/components/settings/membership/portal/portal-links.tsx b/apps/admin-x-settings/src/components/settings/membership/portal/portal-links.tsx index 54b22a3ad3c..c1619779c6b 100644 --- a/apps/admin-x-settings/src/components/settings/membership/portal/portal-links.tsx +++ b/apps/admin-x-settings/src/components/settings/membership/portal/portal-links.tsx @@ -1,4 +1,5 @@ import React, {useEffect, useId, useState} from 'react'; +import useFeatureFlag from '../../../../hooks/use-feature-flag'; import {Button, List, ListItem, ModalPage, Select, TextField} from '@tryghost/admin-x-design-system'; import {getHomepageUrl} from '@tryghost/admin-x-framework/api/site'; import {getPaidActiveTiers, useBrowseTiers} from '@tryghost/admin-x-framework/api/tiers'; @@ -38,6 +39,7 @@ const PortalLinks: React.FC = () => { const {siteData} = useGlobalData(); const {data: {tiers: allTiers} = {}} = useBrowseTiers(); const tiers = getPaidActiveTiers(allTiers || []); + const hasGiftSubscriptions = useFeatureFlag('giftSubscriptions'); const toggleIsDataAttributes = () => { setIsDataAttributes(!isDataAttributes); @@ -66,6 +68,7 @@ const PortalLinks: React.FC = () => { + {hasGiftSubscriptions && } diff --git a/apps/portal/package.json b/apps/portal/package.json index 98768e5d30a..9d45fcfac78 100644 --- a/apps/portal/package.json +++ b/apps/portal/package.json @@ -1,6 +1,6 @@ { "name": "@tryghost/portal", - "version": "2.68.10", + "version": "2.68.11", "license": "MIT", "repository": "https://github.com/TryGhost/Ghost", "author": "Ghost Foundation", diff --git a/apps/portal/src/components/pages/share/share-modal.js b/apps/portal/src/components/pages/share/share-modal.js index 41f9b71bbc3..fb81b00d53d 100644 --- a/apps/portal/src/components/pages/share/share-modal.js +++ b/apps/portal/src/components/pages/share/share-modal.js @@ -38,8 +38,10 @@ const ShareModal = () => { // correct document context where the click events actually fire. const doc = moreMenuRef.current?.ownerDocument || document; - const onDocumentMouseDown = (event) => { + const onDocumentClickCapture = (event) => { if (moreMenuRef.current && !moreMenuRef.current.contains(event.target)) { + event.stopPropagation(); + event.preventDefault(); setIsMoreMenuOpen(false); } }; @@ -50,11 +52,11 @@ const ShareModal = () => { } }; - doc.addEventListener('mousedown', onDocumentMouseDown); + doc.addEventListener('click', onDocumentClickCapture, true); doc.addEventListener('keydown', onDocumentKeyDown); return () => { - doc.removeEventListener('mousedown', onDocumentMouseDown); + doc.removeEventListener('click', onDocumentClickCapture, true); doc.removeEventListener('keydown', onDocumentKeyDown); }; }, [isMoreMenuOpen]); diff --git a/apps/portal/src/components/pages/share/share-modal.styles.js b/apps/portal/src/components/pages/share/share-modal.styles.js index 131884e562d..ca221894990 100644 --- a/apps/portal/src/components/pages/share/share-modal.styles.js +++ b/apps/portal/src/components/pages/share/share-modal.styles.js @@ -263,6 +263,13 @@ export const ShareModalStyles = ` top: 20px; } + @media (max-width: 480px) { + .gh-portal-popup-container.share { + flex: 1 0 auto; + margin-bottom: 0; + } + } + @media (max-width: 420px) { .gh-portal-share-actions { flex-direction: column; @@ -292,11 +299,8 @@ export const ShareModalStyles = ` order: 4; } - .gh-portal-share-action.more { - order: 5; - } - .gh-portal-share-more { + order: 5; width: 100%; } diff --git a/apps/portal/test/unit/components/pages/share-modal.test.js b/apps/portal/test/unit/components/pages/share-modal.test.js index 3f3c91fd953..bf547558e0e 100644 --- a/apps/portal/test/unit/components/pages/share-modal.test.js +++ b/apps/portal/test/unit/components/pages/share-modal.test.js @@ -216,7 +216,7 @@ describe('ShareModal', () => { expect(getByRole('menu')).toBeInTheDocument(); expect(moreButton).toHaveAttribute('aria-expanded', 'true'); - fireEvent.mouseDown(getByText('Share')); + fireEvent.click(getByText('Share')); expect(queryByRole('menu')).not.toBeInTheDocument(); expect(moreButton).toHaveAttribute('aria-expanded', 'false'); diff --git a/ghost/admin/app/components/posts-list/selection-list.js b/ghost/admin/app/components/posts-list/selection-list.js index c55f51d3a18..4371e9a03b9 100644 --- a/ghost/admin/app/components/posts-list/selection-list.js +++ b/ghost/admin/app/components/posts-list/selection-list.js @@ -123,11 +123,11 @@ export default class SelectionList { } const models = this.infinityModel; - let total; + let total = 0; for (const key in models) { - total += models[key].meta?.pagination?.total; - } - return Math.max((total ?? 0) - this.selectedIds.size, 1); + total += models[key].meta?.pagination?.total ?? 0; + } + return Math.max(total - this.selectedIds.size, 1); } isSelected(id) { diff --git a/ghost/core/core/frontend/utils/member-count.js b/ghost/core/core/frontend/utils/member-count.js index 70565712d86..1e8ad309f39 100644 --- a/ghost/core/core/frontend/utils/member-count.js +++ b/ghost/core/core/frontend/utils/member-count.js @@ -6,14 +6,15 @@ const {api} = require('../services/proxy'); * free: number; * paid: number; * comped: number; + * gift: number; * total: number; * }>} */ async function getMemberStats() { let memberStats = this.data || await api.stats.memberCountHistory.query(); - const {free, paid, comped} = memberStats.meta.totals; - let total = free + paid + comped; - return {free, paid, comped, total}; + const {free, paid, comped, gift} = memberStats.meta.totals; + let total = free + paid + comped + gift; + return {free, paid, comped, gift, total}; } /** diff --git a/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js b/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js index 6a1c0cd8e73..c7a04a3a09f 100644 --- a/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js +++ b/ghost/core/core/server/adapters/lib/redis/AdapterCacheRedis.js @@ -1,13 +1,16 @@ +const crypto = require('crypto'); const BaseCacheAdapter = require('@tryghost/adapter-base-cache'); const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); -const metrics = require('@tryghost/metrics'); const debug = require('@tryghost/debug')('redis-cache'); const cacheManager = require('cache-manager'); const redisStoreFactory = require('./redis-store-factory'); -const calculateSlot = require('cluster-key-slot'); + +const PREFIX_HASH_KEY = 'prefix_hash'; class AdapterCacheRedis extends BaseCacheAdapter { + #prefixHashInitInFlight = null; + /** * * @param {Object} config @@ -64,55 +67,62 @@ class AdapterCacheRedis extends BaseCacheAdapter { this.refreshAheadFactor = config.refreshAheadFactor || 0; this.getTimeoutMilliseconds = config.getTimeoutMilliseconds || null; this.currentlyExecutingBackgroundRefreshes = new Set(); - this.keyPrefix = config.keyPrefix; - this._keysPattern = config.keyPrefix ? `${config.keyPrefix}*` : ''; + this._keyPrefix = config.keyPrefix || ''; this.redisClient = this.cache.store.getClient(); this.redisClient.on('error', this.handleRedisError); } - handleRedisError(error) { - logging.error(error); + /** + * NOTE: this adds an extra round-trip to every cache operation. The + * trade-off is intentional for now (reset becomes O(1) instead of an + * O(n) SCAN). + * + * NOTE: the prefix_hash key is written without a TTL, but redis can + * still evict it under allkeys-lru / allkeys-random eviction policies. + * Eviction will silently invalidate the entire cache as a side effect. + * Operators who need stricter invalidation guarantees should configure + * a noeviction policy or pin this key out-of-band. + */ + async prefixHash() { + const currentPrefixHash = await this.redisClient.get(this._keyPrefix + PREFIX_HASH_KEY); + if (currentPrefixHash) { + return currentPrefixHash; + } + return this.#initPrefixHash(); } - #getPrimaryRedisNode() { - debug('getPrimaryRedisNode'); - if (this.redisClient.constructor.name !== 'Cluster') { - return this.redisClient; - } - const slot = calculateSlot(this.keyPrefix); - const [ip, port] = this.redisClient.slots[slot][0].split(':'); - for (const node of this.redisClient.nodes()) { - if (node.options.host === ip && node.options.port === parseInt(port)) { - return node; - } + /** + * Lazily creates the prefix_hash. Concurrent callers in this process + * share one in-flight init via #prefixHashInitInFlight; SET NX ensures + * only one writer wins across processes. + */ + #initPrefixHash() { + if (this.#prefixHashInitInFlight) { + return this.#prefixHashInitInFlight; } - return null; + const value = crypto.randomBytes(12).toString('hex'); + this.#prefixHashInitInFlight = this.redisClient + .set(this._keyPrefix + PREFIX_HASH_KEY, value, 'NX') + .then(async (result) => { + if (result === 'OK') { + return value; + } + // someone else wrote it first; return their value + return await this.redisClient.get(this._keyPrefix + PREFIX_HASH_KEY); + }) + .finally(() => { + this.#prefixHashInitInFlight = null; + }); + return this.#prefixHashInitInFlight; } - #scanNodeForKeys(node) { - debug(`scanNodeForKeys matching ${this._keysPattern}`); - return new Promise((resolve, reject) => { - const stream = node.scanStream({match: this._keysPattern, count: 100}); - let keys = []; - stream.on('data', (resultKeys) => { - keys = keys.concat(resultKeys); - }); - stream.on('error', (e) => { - reject(e); - }); - stream.on('end', () => { - resolve(keys); - }); - }); + async keyPrefix() { + const prefixHash = await this.prefixHash(); + return this._keyPrefix + prefixHash; } - async #getKeys() { - debug('#getKeys'); - const primaryNode = this.#getPrimaryRedisNode(); - if (primaryNode === null) { - return []; - } - return await this.#scanNodeForKeys(primaryNode); + handleRedisError(error) { + logging.error(error); } /** @@ -120,23 +130,11 @@ class AdapterCacheRedis extends BaseCacheAdapter { * the cache-manager package. Might be a good contribution to make * in the package itself (https://github.com/node-cache-manager/node-cache-manager/issues/158) * @param {string} key - * @returns {string} - */ - _buildKey(key) { - if (this.keyPrefix) { - return `${this.keyPrefix}${key}`; - } - - return key; - } - - /** - * This is a method to remove the key prefix from any raw key returned from redis. - * @param {string} key - * @returns {string} + * @returns {Promise} */ - _removeKeyPrefix(key) { - return key.slice(this.keyPrefix.length); + async _buildKey(key) { + const keyPrefix = await this.keyPrefix(); + return `${keyPrefix}${key}`; } /** @@ -164,27 +162,38 @@ class AdapterCacheRedis extends BaseCacheAdapter { } /** - * Returns the specified key from the cache if it exists, otherwise returns null - * - If getTimeoutMilliseconds is set, the method will return a promise that resolves with the value or null if the timeout is exceeded - * - * @param {string} key + * Performs the buildKey + cache.get sequence, bounded by + * getTimeoutMilliseconds if configured. On timeout resolves with + * {internalKey: null, result: null} so the caller treats it as a MISS. + * + * @param {string} key + * @returns {Promise<{internalKey: string|null, result: any}>} */ - async _get(key) { + #lookupWithTimeout(key) { + const lookup = (async () => { + const internalKey = await this._buildKey(key); + const result = await this.cache.get(internalKey); + return {internalKey, result}; + })(); + if (typeof this.getTimeoutMilliseconds !== 'number') { - return this.cache.get(key); - } else { - return new Promise((resolve) => { - const timer = setTimeout(() => { - debug('get', key, 'timeout'); - resolve(null); - }, this.getTimeoutMilliseconds); - - this.cache.get(key).then((result) => { - clearTimeout(timer); - resolve(result); - }); - }); + return lookup; } + + return new Promise((resolve) => { + const timer = setTimeout(() => { + debug('get', key, 'timeout'); + resolve({internalKey: null, result: null}); + }, this.getTimeoutMilliseconds); + lookup.then((value) => { + clearTimeout(timer); + resolve(value); + }, () => { + // redis failure during lookup - treat as MISS, same as the timeout path + clearTimeout(timer); + resolve({internalKey: null, result: null}); + }); + }); } /** @@ -193,10 +202,9 @@ class AdapterCacheRedis extends BaseCacheAdapter { * @param {() => Promise} [fetchData] An optional function to fetch the data, which will be used in the case of a cache MISS or a background refresh */ async get(key, fetchData) { - const internalKey = this._buildKey(key); try { - const result = await this._get(internalKey); - debug(`get ${internalKey}: Cache ${result ? 'HIT' : 'MISS'}`); + const {internalKey, result} = await this.#lookupWithTimeout(key); + debug(`get ${key}: Cache ${result ? 'HIT' : 'MISS'}`); if (!fetchData) { return result; } @@ -204,10 +212,10 @@ class AdapterCacheRedis extends BaseCacheAdapter { const shouldRefresh = await this.shouldRefresh(internalKey); const isRefreshing = this.currentlyExecutingBackgroundRefreshes.has(internalKey); if (isRefreshing) { - debug(`Background refresh already happening for ${internalKey}`); + debug(`Background refresh already happening for ${key}`); } if (!isRefreshing && shouldRefresh) { - debug(`Doing background refresh for ${internalKey}`); + debug(`Doing background refresh for ${key}`); this.currentlyExecutingBackgroundRefreshes.add(internalKey); fetchData().then(async (data) => { await this.set(key, data); // We don't use `internalKey` here because `set` handles it @@ -237,34 +245,26 @@ class AdapterCacheRedis extends BaseCacheAdapter { * @param {*} value */ async set(key, value) { - const internalKey = this._buildKey(key); - debug('set', internalKey); try { + const internalKey = await this._buildKey(key); + debug('set', internalKey); return await this.cache.set(internalKey, value); } catch (err) { logging.error(err); } } - /** - * Reset the cache by deleting everything from redis - */ + async cyclePrefixHash() { + const value = crypto.randomBytes(12).toString('hex'); + // Raw client: cache-manager would JSON-wrap, and reset needs an unconditional overwrite (no NX). + await this.redisClient.set(this._keyPrefix + PREFIX_HASH_KEY, value); + return value; + } + async reset() { debug('reset'); try { - const t0 = performance.now(); - logging.debug(`[RedisAdapter] Clearing cache: scanning for keys matching ${this._keysPattern}`); - const keys = await this.#getKeys(); - logging.debug(`[RedisAdapter] Clearing cache: found ${keys.length} keys matching ${this._keysPattern} in ${(performance.now() - t0).toFixed(1)}ms`); - metrics.metric('cache-reset-scan', (performance.now() - t0).toFixed(1)); - const t1 = performance.now(); - for (const key of keys) { - await this.cache.del(key); - } - logging.debug(`[RedisAdapter] Clearing cache: deleted ${keys.length} keys matching ${this._keysPattern} in ${(performance.now() - t1).toFixed(1)}ms`); - metrics.metric('cache-reset-delete', (performance.now() - t1).toFixed(1)); - metrics.metric('cache-reset', (performance.now() - t0).toFixed(1)); - metrics.metric('cache-reset-key-count', keys.length); + await this.cyclePrefixHash(); } catch (err) { logging.error(err); } diff --git a/ghost/core/core/server/services/gifts/gift-service.ts b/ghost/core/core/server/services/gifts/gift-service.ts index d3ebb78038f..973c44e9beb 100644 --- a/ghost/core/core/server/services/gifts/gift-service.ts +++ b/ghost/core/core/server/services/gifts/gift-service.ts @@ -85,6 +85,8 @@ interface StaffServiceEmails { memberEmail: string; memberName: string | null; tierName: string; + cadence: 'month' | 'year'; + duration: number; buyerEmail: string; }): Promise; } @@ -313,6 +315,8 @@ export class GiftService { memberEmail: member.get('email'), memberName: member.get('name'), tierName: tier.name, + cadence: redeemed.cadence, + duration: redeemed.duration, buyerEmail: redeemed.buyerEmail }); } catch (err) { diff --git a/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.hbs b/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.hbs index 1e413e098f8..7e3e34f221a 100644 --- a/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.hbs +++ b/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.hbs @@ -3,7 +3,7 @@ - 🎁 New paid subscriber: {{memberData.name}} + 🎁 Paid subscription started: {{memberData.name}} {{> styles}} @@ -44,9 +44,7 @@

Name

Tier

- -

Source

-

Gift subscription

+

Gifted by

{{giftedByEmail}}

diff --git a/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.txt.js b/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.txt.js index fa7e50ccc55..7ba62f9c46c 100644 --- a/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.txt.js +++ b/ghost/core/core/server/services/staff/email-templates/new-gift-subscription.txt.js @@ -5,8 +5,7 @@ Congratulations! You have a new paid member: ${data.memberData.name} -Tier: ${data.tierData.name} -Source: Gift subscription +Tier: ${data.tierData.name}${data.tierData.details ? ` • ${data.tierData.details}` : ''} Gifted by: ${data.giftedByEmail} --- diff --git a/ghost/core/core/server/services/staff/staff-service-emails.js b/ghost/core/core/server/services/staff/staff-service-emails.js index bfe4264ff78..bfb2c4ef4f6 100644 --- a/ghost/core/core/server/services/staff/staff-service-emails.js +++ b/ghost/core/core/server/services/staff/staff-service-emails.js @@ -337,14 +337,15 @@ class StaffServiceEmails { }); } - async notifyGiftSubscriptionStarted({memberId, memberName, memberEmail, tierName, buyerEmail}, options = {}) { + async notifyGiftSubscriptionStarted({memberId, memberName, memberEmail, tierName, cadence, duration, buyerEmail}, options = {}) { const users = await this.models.User.getEmailAlertUsers('paid-started', options); const memberData = this.getMemberData({ id: memberId, name: memberName ?? null, email: memberEmail }); - const subject = `🎁 New paid subscriber: ${memberData.name}`; + const subject = `🎁 Paid subscription started: ${memberData.name}`; + const cadenceLabel = duration === 1 ? `1 ${cadence}` : `${duration} ${cadence}s`; await this.sendToStaff({ users, @@ -353,7 +354,8 @@ class StaffServiceEmails { memberData, templateData: { tierData: { - name: tierName + name: tierName, + details: cadenceLabel }, giftedByEmail: buyerEmail } diff --git a/ghost/core/core/server/services/stats/members-stats-service.js b/ghost/core/core/server/services/stats/members-stats-service.js index 42896ed159c..2dc2b5edf33 100644 --- a/ghost/core/core/server/services/stats/members-stats-service.js +++ b/ghost/core/core/server/services/stats/members-stats-service.js @@ -22,11 +22,13 @@ class MembersStatsService { const paidEvent = rows.find(c => c.status === 'paid'); const freeEvent = rows.find(c => c.status === 'free'); const compedEvent = rows.find(c => c.status === 'comped'); + const giftEvent = rows.find(c => c.status === 'gift'); return { paid: paidEvent ? paidEvent.total : 0, free: freeEvent ? freeEvent.total : 0, - comped: compedEvent ? compedEvent.total : 0 + comped: compedEvent ? compedEvent.total : 0, + gift: giftEvent ? giftEvent.total : 0 }; } @@ -279,6 +281,7 @@ module.exports = MembersStatsService; * @property {number} paid Total paid members * @property {number} free Total free members * @property {number} comped Total comped members + * @property {number} gift Total gift members */ /** diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/stats.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/stats.test.js.snap index 0b93c43a1e5..ecd76e984d0 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/stats.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/stats.test.js.snap @@ -44,6 +44,7 @@ Object { "totals": Object { "comped": 0, "free": 3, + "gift": 0, "paid": 5, }, }, @@ -64,7 +65,7 @@ exports[`Stats API Can fetch member count history 2: [headers] 1`] = ` Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "149", + "content-length": "158", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, 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 6b689c687f9..8386c90d339 100644 --- a/ghost/core/test/e2e-api/members/gift-subscriptions.test.js +++ b/ghost/core/test/e2e-api/members/gift-subscriptions.test.js @@ -671,7 +671,7 @@ describe('Gift Subscriptions', function () { // Verify staff notification email was sent mockManager.assert.sentEmail({ - subject: /new paid subscriber/i, + subject: /paid subscription started/i, to: 'jbloggs@example.com' }); }); @@ -849,7 +849,7 @@ describe('Gift Subscriptions', function () { // Verify gift subscription started staff notification was sent, // and that no other unwanted staff notifications were sent (i.e. no "Free member signup" email) mockManager.assert.sentEmail({ - subject: /new paid subscriber/i, + subject: /paid subscription started/i, to: 'jbloggs@example.com' }); mockManager.assert.sentEmailCount(1); @@ -920,7 +920,7 @@ describe('Gift Subscriptions', function () { // Verify gift subscription started staff notification was sent mockManager.assert.sentEmail({ - subject: /new paid subscriber/i, + subject: /paid subscription started/i, to: 'jbloggs@example.com' }); 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 d7acfb443f3..0fd8c65f6c2 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 @@ -132,6 +132,44 @@ describe('Integration: AdapterCacheRedis', function () { }); }); + describe('get with fetchData (error paths)', function () { + it('does not cache errors — a subsequent call retries fetchData', async function () { + const cache = createCache(); + const fetcher = sinon.stub(); + fetcher.onFirstCall().rejects(new Error('transient DB error')); + fetcher.onSecondCall().resolves('recovered'); + + await cache.get('retry-key', fetcher); + + const value = await cache.get('retry-key', fetcher); + + assert.equal(fetcher.callCount, 2); + assert.equal(value, 'recovered'); + }); + + it('stores and retrieves complex nested objects', async function () { + const cache = createCache(); + const complexValue = { + posts: [ + { + id: 'abc123', + title: 'Test Post', + tags: [{id: 't1', name: 'News'}], + authors: [{id: 'a1', name: 'Jane'}] + } + ], + meta: { + pagination: {page: 1, limit: 15, pages: 3, total: 42, next: 2, prev: null} + } + }; + + await cache.set('complex', complexValue); + const retrieved = await cache.get('complex'); + + assert.deepEqual(retrieved, complexValue); + }); + }); + describe('without a keyPrefix', function () { it('still stores and retrieves values', async function () { const cache = buildCache(undefined); @@ -150,7 +188,7 @@ describe('Integration: AdapterCacheRedis', function () { assert.equal(await cache.get('fast'), 'value'); }); - it('returns null when the underlying get exceeds the timeout', async function () { + it('returns null when the data fetch exceeds the timeout', async function () { const cache = createCache({getTimeoutMilliseconds: 1}); await cache.set('slow', 'value'); @@ -161,5 +199,18 @@ describe('Integration: AdapterCacheRedis', function () { assert.equal(await cache.get('slow'), null); }); + + it('returns null when the prefix_hash fetch exceeds the timeout', async function () { + const cache = createCache({getTimeoutMilliseconds: 1}); + // Prime the cache so prefix_hash exists before we slow reads down. + await cache.set('foo', 'value'); + + const original = cache.redisClient.get.bind(cache.redisClient); + cache.redisClient.get = k => new Promise((resolve) => { + setTimeout(() => original(k).then(resolve), 50); + }); + + assert.equal(await cache.get('foo'), null); + }); }); }); diff --git a/ghost/core/test/unit/frontend/utils/member-count.test.js b/ghost/core/test/unit/frontend/utils/member-count.test.js index f6000428b3d..e424546b427 100644 --- a/ghost/core/test/unit/frontend/utils/member-count.test.js +++ b/ghost/core/test/unit/frontend/utils/member-count.test.js @@ -35,10 +35,10 @@ const getMemberStatsMock = [ describe('Member Count', function () { it('should return total members', async function () { const meta = {data: { - meta: {totals: {paid: 1000, free: 500, comped: 500}} + meta: {totals: {paid: 1000, free: 500, comped: 500, gift: 100}} }}; const members = await getMemberStats.call(meta); - assert.equal(members.total, 2000); + assert.equal(members.total, 2100); }); it('should return rounded numbers in correct format', function () { 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 7302220b1ab..d7cb760159b 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 @@ -4,6 +4,33 @@ const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); const RedisCache = require('../../../../../../core/server/adapters/lib/redis/AdapterCacheRedis'); +const PREFIX_HASH = 'mock-prefix-hash'; + +/** + * Build a stub for the cache-manager instance, including a stubbed + * underlying redis client. The redis client's get() returns PREFIX_HASH + * for the prefix_hash key by default so the prefix-rotation plumbing + * doesn't get in the way of the behaviour under test. + */ +function createCacheStub({keyPrefix = ''} = {}) { + const redisGet = sinon.stub(); + redisGet.withArgs(`${keyPrefix}prefix_hash`).resolves(PREFIX_HASH); + + const cacheStub = { + get: sinon.stub(), + set: sinon.stub().resolvesArg(1), + ttl: sinon.stub(), + store: { + getClient: sinon.stub().returns({ + on: sinon.stub(), + get: redisGet, + set: sinon.stub().resolves('OK') + }) + } + }; + return cacheStub; +} + describe('Adapter Cache Redis', function () { beforeEach(function () { sinon.stub(logging, 'error'); @@ -14,17 +41,7 @@ describe('Adapter Cache Redis', function () { }); it('can initialize Redis cache instance directly', async function () { - const redisCacheInstanceStub = { - store: { - getClient: sinon.stub().returns({ - on: sinon.stub() - }) - } - }; - const cache = new RedisCache({ - cache: redisCacheInstanceStub - }); - + const cache = new RedisCache({cache: createCacheStub()}); assert.ok(cache); }); @@ -44,8 +61,60 @@ describe('Adapter Cache Redis', function () { describe('get', function () { it('can get a value from the cache', async function () { + const cacheStub = createCacheStub(); + cacheStub.get.resolves('value from cache'); + const cache = new RedisCache({cache: cacheStub}); + + const value = await cache.get('key'); + + assert.equal(value, 'value from cache'); + }); + + it('returns null if getTimeoutMilliseconds is exceeded', async function () { + const cacheStub = createCacheStub(); + cacheStub.get.callsFake(() => new Promise((resolve) => { + setTimeout(() => resolve('value from cache'), 200); + })); + const cache = new RedisCache({ + cache: cacheStub, + getTimeoutMilliseconds: 100 + }); + + const value = await cache.get('key'); + assert.equal(value, null); + }); + + it('can update the cache in the case of a cache miss', async function () { + const KEY = 'update-cache-on-miss'; + let cachedValue = null; + const cacheStub = createCacheStub(); + cacheStub.get.callsFake(async (key) => { + if (key === PREFIX_HASH + KEY) { + return cachedValue; + } + }); + cacheStub.set.callsFake(async (key, value) => { + if (key === PREFIX_HASH + KEY) { + cachedValue = value; + } + }); + const cache = new RedisCache({cache: cacheStub}); + + const fetchData = sinon.stub().resolves('Da Value'); + + const firstRead = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 1); + assert.equal(firstRead, 'Da Value'); + + const secondRead = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 1); + assert.equal(secondRead, 'Da Value'); + }); + + it('returns undefined and logs error when fetchData rejects on a cache miss', async function () { const redisCacheInstanceStub = { - get: sinon.stub().resolves('value from cache'), + get: sinon.stub().resolves(null), + set: sinon.stub().resolves(), store: { getClient: sinon.stub().returns({ on: sinon.stub() @@ -56,19 +125,21 @@ describe('Adapter Cache Redis', function () { cache: redisCacheInstanceStub }); - const value = await cache.get('key'); + const fetchData = sinon.stub().rejects(new Error('DB is down')); - assert.equal(value, 'value from cache'); + const value = await cache.get('key', fetchData); + + assert.equal(value, undefined); + sinon.assert.calledOnce(fetchData); + sinon.assert.calledOnce(logging.error); }); - it('returns null if getTimeoutMilliseconds is exceeded', async function () { + it('retries fetchData on next call after a previous fetchData rejection', async function () { + let cachedValue = null; const redisCacheInstanceStub = { - get: sinon.stub().callsFake(async () => { - return new Promise((resolve) => { - setTimeout(() => { - resolve('value from cache'); - }, 200); - }); + get: sinon.stub().callsFake(() => cachedValue), + set: sinon.stub().callsFake((_key, value) => { + cachedValue = value; }), store: { getClient: sinon.stub().returns({ @@ -77,26 +148,25 @@ describe('Adapter Cache Redis', function () { } }; const cache = new RedisCache({ - cache: redisCacheInstanceStub, - getTimeoutMilliseconds: 100 + cache: redisCacheInstanceStub }); - const value = await cache.get('key'); - assert.equal(value, null); + const fetchData = sinon.stub(); + fetchData.onFirstCall().rejects(new Error('transient failure')); + fetchData.onSecondCall().resolves('recovered value'); + + await cache.get('key', fetchData); + + const value = await cache.get('key', fetchData); + + assert.equal(fetchData.callCount, 2); + assert.equal(value, 'recovered value'); }); - it('can update the cache in the case of a cache miss', async function () { - const KEY = 'update-cache-on-miss'; - let cachedValue = null; + it('does not call fetchData when the underlying Redis get throws', async function () { const redisCacheInstanceStub = { - get: function (key) { - assert(key === KEY); - return cachedValue; - }, - set: function (key, value) { - assert(key === KEY); - cachedValue = value; - }, + get: sinon.stub().rejects(new Error('Redis connection lost')), + set: sinon.stub().resolves(), store: { getClient: sinon.stub().returns({ on: sinon.stub() @@ -107,27 +177,18 @@ describe('Adapter Cache Redis', function () { cache: redisCacheInstanceStub }); - const fetchData = sinon.stub().resolves('Da Value'); + const fetchData = sinon.stub().resolves('fallback value'); - checkFirstRead: { - const value = await cache.get(KEY, fetchData); - sinon.assert.calledOnce(fetchData); - assert.equal(value, 'Da Value'); - break checkFirstRead; - } - - checkSecondRead: { - const value = await cache.get(KEY, fetchData); - sinon.assert.calledOnce(fetchData); - assert.equal(value, 'Da Value'); - break checkSecondRead; - } + const value = await cache.get('key', fetchData); + + assert.equal(value, undefined); + assert.equal(fetchData.callCount, 0); + sinon.assert.calledOnce(logging.error); }); - it('Can do a background update of the cache', async function () { - const KEY = 'update-cache-in-background'; + it('returns the cached value when background refresh fails', async function () { + const KEY = 'bg-refresh-error'; let cachedValue = null; - let remainingTTL = 100; const redisCacheInstanceStub = { get: function (key) { @@ -138,9 +199,8 @@ describe('Adapter Cache Redis', function () { assert(key === KEY); cachedValue = value; }, - ttl: function (key) { - assert(key === KEY); - return remainingTTL; + ttl: function () { + return 5; }, store: { getClient: sinon.stub().returns({ @@ -154,64 +214,107 @@ describe('Adapter Cache Redis', function () { refreshAheadFactor: 0.2 }); + const fetchData = sinon.stub(); + fetchData.onFirstCall().resolves('Original Value'); + fetchData.onSecondCall().rejects(new Error('refresh failed')); + + const first = await cache.get(KEY, fetchData); + assert.equal(first, 'Original Value'); + sinon.assert.calledOnce(fetchData); + + const second = await cache.get(KEY, fetchData); + assert.equal(second, 'Original Value'); + sinon.assert.calledTwice(fetchData); + + // The .catch() handler fires asynchronously — wait for it to settle + await new Promise((resolve) => { + setTimeout(resolve, 10); + }); + sinon.assert.calledOnce(logging.error); + assert.equal(logging.error.firstCall.args[0].message, 'There was an error refreshing cache data in the background'); + assert.equal(logging.error.firstCall.args[0].error.message, 'refresh failed'); + }); + + it('Can do a background update of the cache', async function () { + const KEY = 'update-cache-in-background'; + let cachedValue = null; + let remainingTTL = 100; + + const cacheStub = createCacheStub(); + cacheStub.get.callsFake(async (key) => { + if (key === PREFIX_HASH + KEY) { + return cachedValue; + } + }); + cacheStub.set.callsFake(async (key, value) => { + if (key === PREFIX_HASH + KEY) { + cachedValue = value; + } + }); + cacheStub.ttl.callsFake(async (key) => { + if (key === PREFIX_HASH + KEY) { + return remainingTTL; + } + }); + const cache = new RedisCache({ + cache: cacheStub, + ttl: 100, + refreshAheadFactor: 0.2 + }); + const fetchData = sinon.stub(); fetchData.onFirstCall().resolves('First Value'); fetchData.onSecondCall().resolves('Second Value'); - checkFirstRead: { - const value = await cache.get(KEY, fetchData); - sinon.assert.calledOnce(fetchData); - assert.equal(value, 'First Value'); - break checkFirstRead; - } + const first = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 1); + assert.equal(first, 'First Value'); // We simulate having been in the cache for 15 seconds remainingTTL = 85; - checkSecondRead: { - const value = await cache.get(KEY, fetchData); - sinon.assert.calledOnce(fetchData); - assert.equal(value, 'First Value'); - break checkSecondRead; - } + const second = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 1); + assert.equal(second, 'First Value'); // We simulate having been in the cache for 30 seconds remainingTTL = 70; - checkThirdRead: { - const value = await cache.get(KEY, fetchData); - sinon.assert.calledOnce(fetchData); - assert.equal(value, 'First Value'); - break checkThirdRead; - } + const third = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 1); + assert.equal(third, 'First Value'); // We simulate having been in the cache for 85 seconds // This should trigger a background refresh remainingTTL = 15; - checkFourthRead: { - const value = await cache.get(KEY, fetchData); - sinon.assert.calledTwice(fetchData); - assert.equal(value, 'First Value'); - break checkFourthRead; - } + const fourth = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 2); + assert.equal(fourth, 'First Value'); // We reset the TTL to 100 for the most recent write remainingTTL = 100; - checkFifthRead: { - const value = await cache.get(KEY, fetchData); - sinon.assert.calledTwice(fetchData); - assert.equal(value, 'Second Value'); - break checkFifthRead; - } + const fifth = await cache.get(KEY, fetchData); + assert.equal(fetchData.callCount, 2); + assert.equal(fifth, 'Second Value'); }); }); describe('set', function () { it('can set a value in the cache', async function () { + const cacheStub = createCacheStub(); + const cache = new RedisCache({cache: cacheStub}); + + const value = await cache.set('key-here', 'new value'); + + assert.equal(value, 'new value'); + assert.equal(cacheStub.set.args[0][0], `${PREFIX_HASH}key-here`); + }); + + it('logs error and does not throw when the underlying Redis set throws', async function () { const redisCacheInstanceStub = { - set: sinon.stub().resolvesArg(1), + set: sinon.stub().rejects(new Error('Redis write failed')), store: { getClient: sinon.stub().returns({ on: sinon.stub() @@ -224,48 +327,36 @@ describe('Adapter Cache Redis', function () { const value = await cache.set('key-here', 'new value'); - assert.equal(value, 'new value'); - assert.equal(redisCacheInstanceStub.set.args[0][0], 'key-here'); + assert.equal(value, undefined); + sinon.assert.calledOnce(logging.error); }); it('sets a key based on keyPrefix', async function () { - const redisCacheInstanceStub = { - set: sinon.stub().resolvesArg(1), - store: { - getClient: sinon.stub().returns({ - on: sinon.stub() - }) - } - }; + const cacheStub = createCacheStub({keyPrefix: 'testing-prefix:'}); const cache = new RedisCache({ - cache: redisCacheInstanceStub, + cache: cacheStub, keyPrefix: 'testing-prefix:' }); const value = await cache.set('key-here', 'new value'); assert.equal(value, 'new value'); - assert.equal(redisCacheInstanceStub.set.args[0][0], 'testing-prefix:key-here'); + assert.equal(cacheStub.set.args[0][0], `testing-prefix:${PREFIX_HASH}key-here`); }); }); describe('reset', function () { - it('catches an error when thrown during the reset', async function () { - const redisCacheInstanceStub = { - get: sinon.stub().resolves('value from cache'), - store: { - getClient: sinon.stub().returns({ - on: sinon.stub() - }) - } - }; - const cache = new RedisCache({ - cache: redisCacheInstanceStub - }); + it('writes a new prefix_hash directly via the redis client (no TTL)', async function () { + const cacheStub = createCacheStub(); + const cache = new RedisCache({cache: cacheStub}); + const redisSet = cacheStub.store.getClient().set; await cache.reset(); - sinon.assert.calledOnce(logging.error); + const [key, value, ...extra] = redisSet.lastCall.args; + assert.equal(key, 'prefix_hash'); + assert.match(value, /^[0-9a-f]{24}$/); + assert.deepEqual(extra, []); }); }); 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 ef18fca9bd6..4287b5ccbdb 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 @@ -1007,6 +1007,8 @@ describe('GiftService', function () { memberEmail: 'member@example.com', memberName: 'Member Name', tierName: 'Bronze', + cadence: 'year', + duration: 1, buyerEmail: 'buyer@example.com' }); assert.equal(redeemed.status, 'redeemed'); diff --git a/ghost/core/test/unit/server/services/staff/staff-service.test.js b/ghost/core/test/unit/server/services/staff/staff-service.test.js index 0a807b6d8bf..389d316908b 100644 --- a/ghost/core/test/unit/server/services/staff/staff-service.test.js +++ b/ghost/core/test/unit/server/services/staff/staff-service.test.js @@ -1097,54 +1097,63 @@ describe('StaffService', function () { memberEmail: 'jamie@example.com', memberId: 'abc', tierName: 'Premium', + cadence: 'year', + duration: 1, buyerEmail: 'gifter@example.com' }); sinon.assert.calledWith(getEmailAlertUsersStub, 'paid-started'); sinon.assert.calledOnce(mailStub); - sinon.assert.calledWith(mailStub, sinon.match.has('subject', sinon.match('New paid subscriber: Jamie'))); + sinon.assert.calledWith(mailStub, sinon.match.has('subject', sinon.match('🎁 Paid subscription started: Jamie'))); }); - it('includes the tier in HTML and plain text', async function () { + it('includes the tier and cadence in HTML and plain text', async function () { await service.emails.notifyGiftSubscriptionStarted({ memberName: 'Jamie', memberEmail: 'jamie@example.com', memberId: 'abc', tierName: 'Premium', + cadence: 'year', + duration: 1, buyerEmail: 'gifter@example.com' }); sinon.assert.calledOnce(mailStub); sinon.assert.calledWith(mailStub, sinon.match.has('html', sinon.match('Premium'))); - sinon.assert.calledWith(mailStub, sinon.match.has('text', sinon.match('Tier: Premium'))); + sinon.assert.calledWith(mailStub, sinon.match.has('html', sinon.match('1 year'))); + sinon.assert.calledWith(mailStub, sinon.match.has('text', sinon.match('Tier: Premium • 1 year'))); }); - it('includes the member name in HTML and plain text', async function () { + it('pluralises the cadence when duration is greater than 1', async function () { await service.emails.notifyGiftSubscriptionStarted({ memberName: 'Jamie', memberEmail: 'jamie@example.com', memberId: 'abc', tierName: 'Premium', + cadence: 'month', + duration: 3, buyerEmail: 'gifter@example.com' }); sinon.assert.calledOnce(mailStub); - sinon.assert.calledWith(mailStub, sinon.match.has('html', sinon.match('Jamie'))); - sinon.assert.calledWith(mailStub, sinon.match.has('text', sinon.match('Jamie'))); + sinon.assert.calledWith(mailStub, sinon.match.has('html', sinon.match('3 months'))); + sinon.assert.calledWith(mailStub, sinon.match.has('text', sinon.match('Tier: Premium • 3 months'))); }); - it('includes the source in HTML and plain text', async function () { + it('includes the member name in HTML and plain text', async function () { await service.emails.notifyGiftSubscriptionStarted({ memberName: 'Jamie', memberEmail: 'jamie@example.com', memberId: 'abc', tierName: 'Premium', + cadence: 'year', + duration: 1, buyerEmail: 'gifter@example.com' }); sinon.assert.calledOnce(mailStub); - sinon.assert.calledWith(mailStub, sinon.match.has('html', sinon.match('Gift subscription'))); - sinon.assert.calledWith(mailStub, sinon.match.has('text', sinon.match('Source: Gift subscription'))); + sinon.assert.calledWith(mailStub, sinon.match.has('html', sinon.match('Jamie'))); + sinon.assert.calledWith(mailStub, sinon.match.has('text', sinon.match('Jamie'))); }); it('includes the buyer email in HTML and plain text', async function () { @@ -1153,6 +1162,8 @@ describe('StaffService', function () { memberEmail: 'jamie@example.com', memberId: 'abc', tierName: 'Premium', + cadence: 'year', + duration: 1, buyerEmail: 'gifter@example.com' }); diff --git a/ghost/core/test/unit/server/services/stats/members.test.js b/ghost/core/test/unit/server/services/stats/members.test.js index 691f92e2401..9a51ab20740 100644 --- a/ghost/core/test/unit/server/services/stats/members.test.js +++ b/ghost/core/test/unit/server/services/stats/members.test.js @@ -12,7 +12,7 @@ describe('MembersStatsService', function () { /** * @type {MembersStatsService.TotalMembersByStatus} */ - const currentCounts = {paid: 0, free: 0, comped: 0}; + const currentCounts = {paid: 0, free: 0, comped: 0, gift: 0}; /** * @type {MembersStatsService.MemberStatusDelta[]} */ @@ -49,6 +49,11 @@ describe('MembersStatsService', function () { }); beforeEach(async function () { + currentCounts.paid = 0; + currentCounts.free = 0; + currentCounts.comped = 0; + currentCounts.gift = 0; + db = knex({client: 'sqlite3', connection: {filename: ':memory:'}, useNullAsDefault: true}); membersStatsService = new MembersStatsService({knex: db}); @@ -81,8 +86,12 @@ describe('MembersStatsService', function () { id: 'id', status: 'comped' })); + const giftMembers = Array.from({length: currentCounts.gift}).map(() => ({ + id: 'id', + status: 'gift' + })); - await db('members').insert(paidMembers.concat(freeMembers, compedMembers)); + await db('members').insert(paidMembers.concat(freeMembers, compedMembers, giftMembers)); /** * @typedef {object} StatusEvent @@ -125,6 +134,7 @@ describe('MembersStatsService', function () { currentCounts.paid = 1; currentCounts.free = 2; currentCounts.comped = 3; + currentCounts.gift = 4; await setupDB();