From 81fc5f8edac7eae214a1a6346d74523957e81bc9 Mon Sep 17 00:00:00 2001 From: Nazar Gargol Date: Tue, 9 Jun 2020 23:02:38 +1200 Subject: [PATCH] Added special handling for member import with Stripe connection no issue - When imported member contains stripe_customer_id data but there is no Stripe configured on the Ghost instance such import should faiil. The logic is consistent with one where import fails after not being able to find customer in linked Stripe account - Fixed import stats to show import failures instead of "duplicate" when the validation error is of "Stripe" origin --- core/server/api/canary/members.js | 22 ++++++++++++++----- core/server/services/members/config.js | 6 +++++ core/server/translations/en.json | 5 +++++ .../api/canary/admin/members_spec.js | 12 +++++----- ...pe-ids.csv => members-with-stripe-ids.csv} | 1 - 5 files changed, 34 insertions(+), 12 deletions(-) rename test/utils/fixtures/csv/{members-with-duplicate-stripe-ids.csv => members-with-stripe-ids.csv} (71%) diff --git a/core/server/api/canary/members.js b/core/server/api/canary/members.js index 66e66fb017..2a83eda88f 100644 --- a/core/server/api/canary/members.js +++ b/core/server/api/canary/members.js @@ -190,6 +190,15 @@ const members = { const member = model.toJSON(frame.options); if (frame.data.members[0].stripe_customer_id) { + // let stripeEnabled = membersSubscriptionSettings && !!(membersSubscriptionSettings.paymentProcessors[0].config.secret_token) && !!(membersSubscriptionSettings.paymentProcessors[0].config.public_token); + if (!membersService.config.isStripeConnected()) { + throw new errors.ValidationError({ + message: i18n.t('errors.api.members.stripeNotConnected.message'), + context: i18n.t('errors.api.members.stripeNotConnected.context'), + help: i18n.t('errors.api.members.stripeNotConnected.help') + }); + } + await membersService.api.members.linkStripeCustomer(frame.data.members[0].stripe_customer_id, member); } @@ -208,8 +217,10 @@ const members = { } // NOTE: failed to link Stripe customer/plan/subscription - if (model && error.message && (error.message.indexOf('customer') || error.message.indexOf('plan') || error.message.indexOf('subscription'))) { + const isStripeLinkingError = error.message && error.message.match(/customer|plan|subscription|Stripe account/g); + if (model && isStripeLinkingError) { if (error.message.indexOf('customer') && error.code === 'resource_missing') { + error.message = `Member not imported, ${error.message}`; error.context = i18n.t('errors.api.members.stripeCustomerNotFound.context'); error.help = i18n.t('errors.api.members.stripeCustomerNotFound.help'); } @@ -421,15 +432,16 @@ const members = { if (inspection.isFulfilled()) { fulfilled = fulfilled + 1; } else { - if (inspection.reason() instanceof errors.ValidationError) { + const error = inspection.reason(); + if (error instanceof errors.ValidationError && !(error.message.match(/Stripe/g))) { duplicates = duplicates + 1; } else { // NOTE: if the error happens as a result of pure API call it doesn't get logged anywhere // for this reason we have to make sure any unexpected errors are logged here - if (Array.isArray(inspection.reason())) { - logging.error(inspection.reason()[0]); + if (Array.isArray(error)) { + logging.error(error[0]); } else { - logging.error(inspection.reason()); + logging.error(error); } invalid = invalid + 1; diff --git a/core/server/services/members/config.js b/core/server/services/members/config.js index 071e2145fa..1fabb87a76 100644 --- a/core/server/services/members/config.js +++ b/core/server/services/members/config.js @@ -114,6 +114,12 @@ class MembersConfigProvider { }; } + isStripeConnected() { + const paymentConfig = this.getStripePaymentConfig(); + + return (paymentConfig && paymentConfig.publicKey && paymentConfig.secretKey); + } + getStripePaymentConfig() { const subscriptionSettings = this._settingsCache.get('members_subscription_settings'); diff --git a/core/server/translations/en.json b/core/server/translations/en.json index a52e25707c..554b9ecfa3 100644 --- a/core/server/translations/en.json +++ b/core/server/translations/en.json @@ -369,6 +369,11 @@ "members": { "memberNotFound": "Member not found.", "memberAlreadyExists": "Email address is already member.", + "stripeNotConnected": { + "message": "Member not imported, Stripe account missing", + "context": "Attempting to import members with Stripe data when there is no Stripe account connected", + "help": "Connect the correct Stripe account, and re-run the import" + }, "stripeCustomerNotFound": { "context": "The customer does not exist in currently linked Stripe account.", "help": "Connect the correct Stripe account, and re-run the import." diff --git a/test/regression/api/canary/admin/members_spec.js b/test/regression/api/canary/admin/members_spec.js index 51f1b96d5f..465fba96eb 100644 --- a/test/regression/api/canary/admin/members_spec.js +++ b/test/regression/api/canary/admin/members_spec.js @@ -235,10 +235,10 @@ describe('Members API', function () { }); }); - it('Can import file with duplicate stripe customer ids', function () { + it('Fails to import members with stripe_customer_id', function () { return request .post(localUtils.API.getApiQuery(`members/csv/`)) - .attach('membersfile', path.join(__dirname, '/../../../../utils/fixtures/csv/members-with-duplicate-stripe-ids.csv')) + .attach('membersfile', path.join(__dirname, '/../../../../utils/fixtures/csv/members-with-stripe-ids.csv')) .set('Origin', config.get('url')) .expect('Content-Type', /json/) .expect('Cache-Control', testUtils.cacheRules.private) @@ -251,7 +251,7 @@ describe('Members API', function () { should.exist(jsonResponse.meta); should.exist(jsonResponse.meta.stats); - jsonResponse.meta.stats.imported.should.equal(1); + jsonResponse.meta.stats.imported.should.equal(0); jsonResponse.meta.stats.duplicates.should.equal(0); jsonResponse.meta.stats.invalid.should.equal(2); }); @@ -277,7 +277,7 @@ describe('Members API', function () { should.exist(jsonResponse.new_today); // 2 from fixtures and 5 imported in previous tests - jsonResponse.total.should.equal(7); + jsonResponse.total.should.equal(6); }); }); @@ -301,7 +301,7 @@ describe('Members API', function () { should.exist(jsonResponse.new_today); // 2 from fixtures and 5 imported in previous tests - jsonResponse.total.should.equal(7); + jsonResponse.total.should.equal(6); }); }); @@ -325,7 +325,7 @@ describe('Members API', function () { should.exist(jsonResponse.new_today); // 2 from fixtures and 5 imported in previous tests - jsonResponse.total.should.equal(7); + jsonResponse.total.should.equal(6); }); }); diff --git a/test/utils/fixtures/csv/members-with-duplicate-stripe-ids.csv b/test/utils/fixtures/csv/members-with-stripe-ids.csv similarity index 71% rename from test/utils/fixtures/csv/members-with-duplicate-stripe-ids.csv rename to test/utils/fixtures/csv/members-with-stripe-ids.csv index 61fce2511d..5f8e974d5b 100644 --- a/test/utils/fixtures/csv/members-with-duplicate-stripe-ids.csv +++ b/test/utils/fixtures/csv/members-with-stripe-ids.csv @@ -1,4 +1,3 @@ email,subscribed,stripe_customer_id member+duplicate_stripe_1@example.com,true,cus_GbbIQRd8TnFqHq -member+duplicate_stripe_2@example.com,false,cus_GbbIQRd8TnFqHq member+unique_stripe_1@example.com,true,cus_GbbIQRd8TnFqHA