From a7f5ee0ad51f673156e661e9de8bf946e3029ee5 Mon Sep 17 00:00:00 2001 From: Naz Date: Tue, 25 Oct 2022 16:40:28 +0800 Subject: [PATCH] Simplified members CSV importer constructor refs https://github.com/TryGhost/Team/issues/2077 - Passing in the whole "getMembersApi" is just too much state to know about for the importer - it only uses a concept of default tier and members repository, the rest is distracting fluff making it hard to reason about what the importer **has to** know to function - Passing in two functions breaking up the above state simplifies the constructor API. - This is also a groundwork before substituting productsRepository for tiersRepository (refed issue objective) --- .../core/server/services/members/service.js | 9 +- ghost/members-importer/lib/importer.js | 23 ++-- ghost/members-importer/test/importer.test.js | 103 +++++++++--------- 3 files changed, 70 insertions(+), 65 deletions(-) diff --git a/ghost/core/core/server/services/members/service.js b/ghost/core/core/server/services/members/service.js index 89290803d6..9b66c07473 100644 --- a/ghost/core/core/server/services/members/service.js +++ b/ghost/core/core/server/services/members/service.js @@ -47,7 +47,14 @@ let verificationTrigger; const membersImporter = new MembersCSVImporter({ storagePath: config.getContentPath('data'), getTimezone: () => settingsCache.get('timezone'), - getMembersApi: () => module.exports.api, + getMembersRepository: async () => { + const api = await module.exports.api; + return api.members; + }, + getDefaultTier: async () => { + const api = await module.exports.api; + return api.productRepository.getDefaultProduct; + }, sendEmail: ghostMailer.send.bind(ghostMailer), isSet: labsService.isSet.bind(labsService), addJob: jobsService.addJob.bind(jobsService), diff --git a/ghost/members-importer/lib/importer.js b/ghost/members-importer/lib/importer.js index 2c5694ce58..66c9e1c856 100644 --- a/ghost/members-importer/lib/importer.js +++ b/ghost/members-importer/lib/importer.js @@ -29,7 +29,8 @@ module.exports = class MembersCSVImporter { * @param {Object} options * @param {string} options.storagePath - The path to store CSV's in before importing * @param {Function} options.getTimezone - function returning currently configured timezone - * @param {() => Object} options.getMembersApi + * @param {() => Object} options.getMembersRepository - member model access instance for data access and manipulation + * @param {() => Object} options.getDefaultTier - async function returning default Member Tier * @param {Function} options.sendEmail - function sending an email * @param {(string) => boolean} options.isSet - Method checking if specific feature is enabled * @param {({job, offloaded}) => void} options.addJob - Method registering an async job @@ -37,10 +38,11 @@ module.exports = class MembersCSVImporter { * @param {Function} options.urlFor - function generating urls * @param {Object} options.context */ - constructor({storagePath, getTimezone, getMembersApi, sendEmail, isSet, addJob, knex, urlFor, context}) { + constructor({storagePath, getTimezone, getMembersRepository, getDefaultTier, sendEmail, isSet, addJob, knex, urlFor, context}) { this._storagePath = storagePath; this._getTimezone = getTimezone; - this._getMembersApi = getMembersApi; + this._getMembersRepository = getMembersRepository; + this._getDefaultTier = getDefaultTier; this._sendEmail = sendEmail; this._isSet = isSet; this._addJob = addJob; @@ -107,9 +109,8 @@ module.exports = class MembersCSVImporter { async perform(filePath) { const rows = membersCSV.parse(filePath, DEFAULT_CSV_HEADER_MAPPING); - const membersApi = await this._getMembersApi(); - - const defaultTier = await membersApi.productRepository.getDefaultProduct(); + const defaultTier = await this._getDefaultTier(); + const membersRepository = await this._getMembersRepository(); const result = await rows.reduce(async (resultPromise, row) => { const resultAccumulator = await resultPromise; @@ -131,14 +132,14 @@ module.exports = class MembersCSVImporter { created_at: row.created_at, labels: row.labels }; - const existingMember = await membersApi.members.get({email: memberValues.email}, { + const existingMember = await membersRepository.get({email: memberValues.email}, { ...options, withRelated: ['labels'] }); let member; if (existingMember) { const existingLabels = existingMember.related('labels') ? existingMember.related('labels').toJSON() : []; - member = await membersApi.members.update({ + member = await membersRepository.update({ ...memberValues, labels: existingLabels.concat(memberValues.labels) }, { @@ -146,7 +147,7 @@ module.exports = class MembersCSVImporter { id: existingMember.id }); } else { - member = await membersApi.members.create(memberValues, Object.assign({}, options, { + member = await membersRepository.create(memberValues, Object.assign({}, options, { context: { import: true } @@ -154,12 +155,12 @@ module.exports = class MembersCSVImporter { } if (row.stripe_customer_id && typeof row.stripe_customer_id === 'string') { - await membersApi.members.linkStripeCustomer({ + await membersRepository.linkStripeCustomer({ customer_id: row.stripe_customer_id, member_id: member.id }, options); } else if (row.complimentary_plan) { - await membersApi.members.update({ + await membersRepository.update({ products: [{id: defaultTier.id}] }, { ...options, diff --git a/ghost/members-importer/test/importer.test.js b/ghost/members-importer/test/importer.test.js index 2be5c53f46..fbd08c41ba 100644 --- a/ghost/members-importer/test/importer.test.js +++ b/ghost/members-importer/test/importer.test.js @@ -15,7 +15,7 @@ describe('Importer', function () { let memberCreateStub; let knexStub; let sendEmailStub; - let membersApiStub; + let membersRepositoryStub; const defaultAllowedFields = { email: 'email', @@ -42,29 +42,21 @@ describe('Importer', function () { sinon.restore(); }); - // @NOTE: this is way too much mocking! the MembersCSVImporter constructor API should be simplified const buildMockImporterInstance = () => { - const defaultTier = { + const defaultTierDummy = { id: 'default_tier_id' }; memberCreateStub = sinon.stub().resolves({ id: `test_member_id` }); - membersApiStub = { - productRepository: { - getDefaultProduct: async () => { - return defaultTier; - } + membersRepositoryStub = { + get: async () => { + return null; }, - members: { - get: async () => { - return null; - }, - create: memberCreateStub, - update: sinon.stub().resolves(null), - linkStripeCustomer: sinon.stub().resolves(null) - } + create: memberCreateStub, + update: sinon.stub().resolves(null), + linkStripeCustomer: sinon.stub().resolves(null) }; knexStub = { @@ -79,7 +71,12 @@ describe('Importer', function () { return new MembersCSVImporter({ storagePath: csvPath, getTimezone: sinon.stub().returns('UTC'), - getMembersApi: () => membersApiStub, + getMembersRepository: () => { + return membersRepositoryStub; + }, + getDefaultTier: () => { + return defaultTierDummy; + }, sendEmail: sendEmailStub, isSet: sinon.stub(), addJob: sinon.stub(), @@ -169,43 +166,43 @@ describe('Importer', function () { fsWriteSpy.calledOnce.should.be.true(); // member records get inserted - membersApiStub.members.create.calledTwice.should.be.true(); + membersRepositoryStub.create.calledTwice.should.be.true(); - should.equal(membersApiStub.members.create.args[0][1].context.import, true, 'inserts are done in the "import" context'); + should.equal(membersRepositoryStub.create.args[0][1].context.import, true, 'inserts are done in the "import" context'); - should.deepEqual(Object.keys(membersApiStub.members.create.args[0][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); - should.equal(membersApiStub.members.create.args[0][0].id, undefined, 'id field should not be taken from the user input'); - should.equal(membersApiStub.members.create.args[0][0].email, 'member_complimentary_test@example.com'); - should.equal(membersApiStub.members.create.args[0][0].name, 'bobby tables'); - should.equal(membersApiStub.members.create.args[0][0].note, 'a note'); - should.equal(membersApiStub.members.create.args[0][0].subscribed, true); - should.equal(membersApiStub.members.create.args[0][0].created_at, '2022-10-18T06:34:08.000Z'); - should.equal(membersApiStub.members.create.args[0][0].deleted_at, undefined, 'deleted_at field should not be taken from the user input'); - should.deepEqual(membersApiStub.members.create.args[0][0].labels, [{ + should.deepEqual(Object.keys(membersRepositoryStub.create.args[0][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.create.args[0][0].id, undefined, 'id field should not be taken from the user input'); + should.equal(membersRepositoryStub.create.args[0][0].email, 'member_complimentary_test@example.com'); + should.equal(membersRepositoryStub.create.args[0][0].name, 'bobby tables'); + should.equal(membersRepositoryStub.create.args[0][0].note, 'a note'); + should.equal(membersRepositoryStub.create.args[0][0].subscribed, true); + should.equal(membersRepositoryStub.create.args[0][0].created_at, '2022-10-18T06:34:08.000Z'); + should.equal(membersRepositoryStub.create.args[0][0].deleted_at, undefined, 'deleted_at field should not be taken from the user input'); + should.deepEqual(membersRepositoryStub.create.args[0][0].labels, [{ name: 'user import label' }]); - should.deepEqual(Object.keys(membersApiStub.members.create.args[1][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); - should.equal(membersApiStub.members.create.args[1][0].id, undefined, 'id field should not be taken from the user input'); - should.equal(membersApiStub.members.create.args[1][0].email, 'member_stripe_test@example.com'); - should.equal(membersApiStub.members.create.args[1][0].name, 'stirpey beaver'); - should.equal(membersApiStub.members.create.args[1][0].note, 'testing notes'); - should.equal(membersApiStub.members.create.args[1][0].subscribed, false); - should.equal(membersApiStub.members.create.args[1][0].created_at, '2022-10-18T07:31:57.000Z'); - should.equal(membersApiStub.members.create.args[1][0].deleted_at, undefined, 'deleted_at field should not be taken from the user input'); - should.deepEqual(membersApiStub.members.create.args[1][0].labels, [], 'no labels should be assigned'); + should.deepEqual(Object.keys(membersRepositoryStub.create.args[1][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.create.args[1][0].id, undefined, 'id field should not be taken from the user input'); + should.equal(membersRepositoryStub.create.args[1][0].email, 'member_stripe_test@example.com'); + should.equal(membersRepositoryStub.create.args[1][0].name, 'stirpey beaver'); + should.equal(membersRepositoryStub.create.args[1][0].note, 'testing notes'); + should.equal(membersRepositoryStub.create.args[1][0].subscribed, false); + should.equal(membersRepositoryStub.create.args[1][0].created_at, '2022-10-18T07:31:57.000Z'); + should.equal(membersRepositoryStub.create.args[1][0].deleted_at, undefined, 'deleted_at field should not be taken from the user input'); + should.deepEqual(membersRepositoryStub.create.args[1][0].labels, [], 'no labels should be assigned'); // stripe customer import - membersApiStub.members.linkStripeCustomer.calledOnce.should.be.true(); - should.equal(membersApiStub.members.linkStripeCustomer.args[0][0].customer_id, 'cus_MdR9tqW6bAreiq'); - should.equal(membersApiStub.members.linkStripeCustomer.args[0][0].member_id, 'test_member_id'); + membersRepositoryStub.linkStripeCustomer.calledOnce.should.be.true(); + should.equal(membersRepositoryStub.linkStripeCustomer.args[0][0].customer_id, 'cus_MdR9tqW6bAreiq'); + should.equal(membersRepositoryStub.linkStripeCustomer.args[0][0].member_id, 'test_member_id'); // complimentary_plan import - membersApiStub.members.update.calledOnce.should.be.true(); - should.deepEqual(membersApiStub.members.update.args[0][0].products, [{ + membersRepositoryStub.update.calledOnce.should.be.true(); + should.deepEqual(membersRepositoryStub.update.args[0][0].products, [{ id: 'default_tier_id' }]); - should.deepEqual(membersApiStub.members.update.args[0][1].id, 'test_member_id'); + should.deepEqual(membersRepositoryStub.update.args[0][1].id, 'test_member_id'); }); }); @@ -280,11 +277,11 @@ describe('Importer', function () { const result = await importer.perform(`${csvPath}/single-column-with-header.csv`, defaultAllowedFields); - assert.equal(membersApiStub.members.create.args[0][0].email, 'jbloggs@example.com'); - assert.deepEqual(membersApiStub.members.create.args[0][0].labels, []); + assert.equal(membersRepositoryStub.create.args[0][0].email, 'jbloggs@example.com'); + assert.deepEqual(membersRepositoryStub.create.args[0][0].labels, []); - assert.equal(membersApiStub.members.create.args[1][0].email, 'test@example.com'); - assert.deepEqual(membersApiStub.members.create.args[1][0].labels, []); + assert.equal(membersRepositoryStub.create.args[1][0].email, 'test@example.com'); + assert.deepEqual(membersRepositoryStub.create.args[1][0].labels, []); assert.equal(result.total, 2); assert.equal(result.imported, 2); @@ -296,13 +293,13 @@ describe('Importer', function () { const result = await importer.perform(`${csvPath}/subscribed-to-emails-header.csv`); - assert.equal(membersApiStub.members.create.args[0][0].email, 'jbloggs@example.com'); - assert.equal(membersApiStub.members.create.args[0][0].subscribed, true); - assert.deepEqual(membersApiStub.members.create.args[0][0].labels, []); + assert.equal(membersRepositoryStub.create.args[0][0].email, 'jbloggs@example.com'); + assert.equal(membersRepositoryStub.create.args[0][0].subscribed, true); + assert.deepEqual(membersRepositoryStub.create.args[0][0].labels, []); - assert.equal(membersApiStub.members.create.args[1][0].email, 'test@example.com'); - assert.equal(membersApiStub.members.create.args[1][0].subscribed, false); - assert.deepEqual(membersApiStub.members.create.args[1][0].labels, []); + assert.equal(membersRepositoryStub.create.args[1][0].email, 'test@example.com'); + assert.equal(membersRepositoryStub.create.args[1][0].subscribed, false); + assert.deepEqual(membersRepositoryStub.create.args[1][0].labels, []); assert.equal(result.total, 2); assert.equal(result.imported, 2);