From 013041304e495cb091734a0604fa642954a70eac Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Tue, 3 Sep 2024 20:49:03 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20Tips=20&=20Donations=20c?= =?UTF-8?q?heckout=20error=20for=20sites=20with=20long=20titles?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/tryghost/issue/ONC-296 Our `stripe_prices.nickname` field had a length of 50 chars which meant we could error out trying to save a donation Stripe price with a generated product nickname containing a long site title. - updated db schema and added a migration to change column length to 255 - added truncation to nickname generation to enforce a limit of 250 chars to match Stripe's limit --- ...01-update-stripe-prices-nickname-length.js | 19 ++++++++ ghost/core/core/server/data/schema/schema.js | 2 +- .../donation-checkout-session.test.js.snap | 6 +++ .../members/donation-checkout-session.test.js | 48 +++++++++++++++++++ .../unit/server/data/schema/integrity.test.js | 2 +- ghost/payments/lib/PaymentsService.js | 11 ++++- 6 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 ghost/core/core/server/data/migrations/versions/5.93/2024-09-03-18-51-01-update-stripe-prices-nickname-length.js diff --git a/ghost/core/core/server/data/migrations/versions/5.93/2024-09-03-18-51-01-update-stripe-prices-nickname-length.js b/ghost/core/core/server/data/migrations/versions/5.93/2024-09-03-18-51-01-update-stripe-prices-nickname-length.js new file mode 100644 index 0000000000..050f7844ab --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/5.93/2024-09-03-18-51-01-update-stripe-prices-nickname-length.js @@ -0,0 +1,19 @@ +const logging = require('@tryghost/logging'); +const {createNonTransactionalMigration} = require('../../utils'); +const DatabaseInfo = require('@tryghost/database-info'); + +module.exports = createNonTransactionalMigration( + async function up(knex) { + if (DatabaseInfo.isSQLite(knex)) { + logging.warn('Skipping migration for SQLite3'); + return; + } + logging.info('Changing stripe_prices.nickname column from VARCHAR(50) to VARCHAR(255)'); + await knex.schema.alterTable('stripe_prices', function (table) { + table.string('nickname', 255).alter(); + }); + }, + async function down() { + logging.warn('Not changing stripe_prices.nickname column'); + } +); diff --git a/ghost/core/core/server/data/schema/schema.js b/ghost/core/core/server/data/schema/schema.js index ee0cb42464..7d1b113618 100644 --- a/ghost/core/core/server/data/schema/schema.js +++ b/ghost/core/core/server/data/schema/schema.js @@ -781,7 +781,7 @@ module.exports = { stripe_price_id: {type: 'string', maxlength: 255, nullable: false, unique: true}, stripe_product_id: {type: 'string', maxlength: 255, nullable: false, unique: false, references: 'stripe_products.stripe_product_id'}, active: {type: 'boolean', nullable: false}, - nickname: {type: 'string', maxlength: 50, nullable: true}, + nickname: {type: 'string', maxlength: 255, nullable: true}, // @note: this is longer than originally intended due to a bug - https://github.com/TryGhost/Ghost/pull/15606 // so we should decide whether we should reduce it down in the future currency: {type: 'string', maxlength: 191, nullable: false}, diff --git a/ghost/core/test/e2e-api/members/__snapshots__/donation-checkout-session.test.js.snap b/ghost/core/test/e2e-api/members/__snapshots__/donation-checkout-session.test.js.snap index b6feb03d6b..2882f387c0 100644 --- a/ghost/core/test/e2e-api/members/__snapshots__/donation-checkout-session.test.js.snap +++ b/ghost/core/test/e2e-api/members/__snapshots__/donation-checkout-session.test.js.snap @@ -12,6 +12,12 @@ Object { } `; +exports[`Create Stripe Checkout Session for Donations can create a checkout session for a site with a long title 1: [body] 1`] = ` +Object { + "url": "https://checkout.stripe.com/c/pay/fake-data", +} +`; + exports[`Create Stripe Checkout Session for Donations check if donation message is in email 1: [body] 1`] = ` Object { "url": "https://checkout.stripe.com/c/pay/fake-data", diff --git a/ghost/core/test/e2e-api/members/donation-checkout-session.test.js b/ghost/core/test/e2e-api/members/donation-checkout-session.test.js index 30394914e5..8e6489a5a5 100644 --- a/ghost/core/test/e2e-api/members/donation-checkout-session.test.js +++ b/ghost/core/test/e2e-api/members/donation-checkout-session.test.js @@ -211,6 +211,7 @@ describe('Create Stripe Checkout Session for Donations', function () { assert.equal(lastDonation.get('attribution_type'), 'post'); assert.equal(lastDonation.get('attribution_url'), url); }); + it('check if donation message is in email', async function () { const post = await getPost(fixtureManager.get('posts', 0).id); const url = urlService.getUrlByResourceId(post.id, {absolute: false}); @@ -281,4 +282,51 @@ describe('Create Stripe Checkout Session for Donations', function () { text: /You are the best! Have a lovely day!/ }); }); + + // We had a bug where the stripe_prices.nickname column was too short for the site title + // Stripe is also limited to 250 chars so we need to truncate the nickname + it('can create a checkout session for a site with a long title', async function () { + // Ensure site title is longer than 250 characters + mockManager.mockSetting('title', 'a'.repeat(251)); + + // clear out existing prices to guarantee we're creating a new one + await models.StripePrice.where('type', 'donation').destroy().catch((e) => { + if (e.message !== 'No Rows Deleted') { + throw e; + } + }); + + // Fake a visit to a post + const post = await getPost(fixtureManager.get('posts', 0).id); + const url = urlService.getUrlByResourceId(post.id, {absolute: false}); + + await membersAgent.post('/api/create-stripe-checkout-session/') + .body({ + customerEmail: 'paid@test.com', + type: 'donation', + successUrl: 'https://example.com/?type=success', + cancelUrl: 'https://example.com/?type=cancel', + metadata: { + test: 'hello', + urlHistory: [ + { + path: url, + time: Date.now(), + referrerMedium: null, + referrerSource: 'ghost-explore', + referrerUrl: 'https://example.com/blog/' + } + ] + } + }) + .expectStatus(200) + .matchBodySnapshot(); + + const latestStripePrice = await models.StripePrice + .where('type', 'donation') + .orderBy('created_at', 'DESC') + .fetch({require: true}); + + latestStripePrice.get('nickname').should.have.length(250); + }); }); 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 0a8600a728..8242d198c2 100644 --- a/ghost/core/test/unit/server/data/schema/integrity.test.js +++ b/ghost/core/test/unit/server/data/schema/integrity.test.js @@ -35,7 +35,7 @@ const validateRouteSettings = require('../../../../../core/server/services/route */ describe('DB version integrity', function () { // Only these variables should need updating - const currentSchemaHash = 'b59d502d0e7965a837bb1dfb5c583562'; + const currentSchemaHash = 'a4f016480ff73c6f52ee4c86482b45a7'; const currentFixturesHash = 'a489d615989eab1023d4b8af0ecee7fd'; const currentSettingsHash = '051ef2a50e2edb8723e89461448313cb'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01'; diff --git a/ghost/payments/lib/PaymentsService.js b/ghost/payments/lib/PaymentsService.js index 6b8fe8db7f..8d80dcf09a 100644 --- a/ghost/payments/lib/PaymentsService.js +++ b/ghost/payments/lib/PaymentsService.js @@ -276,11 +276,20 @@ class PaymentsService { }; } + /** + * Stripe's nickname field is limited to 250 characters + * @returns {string} + */ + getDonationPriceNickname() { + const nickname = 'Support ' + this.settingsCache.get('title'); + return nickname.substring(0, 250); + } + /** * @returns {Promise<{id: string}>} */ async getPriceForDonations() { - const nickname = 'Support ' + this.settingsCache.get('title'); + const nickname = this.getDonationPriceNickname(); const currency = this.settingsCache.get('donations_currency'); const suggestedAmount = this.settingsCache.get('donations_suggested_amount');