From 90768e9985e756a2706508beeb5c50e34516a170 Mon Sep 17 00:00:00 2001 From: Naz Date: Wed, 19 Oct 2022 18:09:34 +0800 Subject: [PATCH] Added strict field mapping to member CSV importer closes https://github.com/TryGhost/Toolbox/issues/430 - The members importer used to import all fields present in the uploaded CSV if the headers match, even if they're not mapped in the UI. This behavior has lead to have misleading consequences and "hidden" features. For example, if the field was present but intentionally left as "Not imported" in the UI the field would still get imported. - Having a strict list of supported import fields also allows for manageable long-term maintenance of the CSV Import API and detect/communicate changes when they happen. - The list of the current default field mapping is: email: 'email', name: 'name', note: 'note', subscribed_to_emails: 'subscribed', created_at: 'created_at', complimentary_plan: 'complimentary_plan', stripe_customer_id: 'stripe_customer_id', labels: 'labels', products: 'products' --- ghost/members-importer/lib/importer.js | 42 +++++-- .../test/fixtures/member-csv-export.csv | 3 + ghost/members-importer/test/importer.test.js | 110 ++++++++++++++++-- 3 files changed, 136 insertions(+), 19 deletions(-) create mode 100644 ghost/members-importer/test/fixtures/member-csv-export.csv diff --git a/ghost/members-importer/lib/importer.js b/ghost/members-importer/lib/importer.js index 4d61711aa399..e681186c422b 100644 --- a/ghost/members-importer/lib/importer.js +++ b/ghost/members-importer/lib/importer.js @@ -11,10 +11,19 @@ const messages = { filenameCollision: 'Filename already exists, please try again.' }; -const defaultInputCSVHeaderMappings = { - subscribed_to_emails: 'subscribed' +// The key should correspond to a member model field (unless it's a special purpose field like 'complimentary_plan') +// the value should represent an allowed field name coming from user input +const DEFAULT_CSV_HEADER_MAPPING = { + email: 'email', + name: 'name', + note: 'note', + subscribed_to_emails: 'subscribed', + created_at: 'created_at', + complimentary_plan: 'complimentary_plan', + stripe_customer_id: 'stripe_customer_id', + labels: 'labels', + products: 'products' }; - module.exports = class MembersCSVImporter { /** * @param {Object} options @@ -48,12 +57,13 @@ module.exports = class MembersCSVImporter { * - Creates a MemberImport Job and associated MemberImportBatch's * * @param {string} inputFilePath - The path to the CSV to prepare - * @param {Object.} headerMapping - An object whose keys are headers in the input CSV and values are the header to replace it with - * @param {Array} defaultLabels - A list of labels to apply to every member + * @param {Object.} [headerMapping] - An object whose keys are headers in the input CSV and values are the header to replace it with + * @param {Array} [defaultLabels] - A list of labels to apply to every member * * @returns {Promise<{filePath: string, batches: number, metadata: Object.}>} - A promise resolving to the data including filePath of "prepared" CSV */ async prepare(inputFilePath, headerMapping, defaultLabels) { + headerMapping = headerMapping || DEFAULT_CSV_HEADER_MAPPING; // @NOTE: investigate why is it "1" and do we even need this concept anymore? const batchSize = 1; @@ -68,8 +78,8 @@ module.exports = class MembersCSVImporter { throw new errors.DataImportError({message: tpl(messages.filenameCollision)}); } - const inputMapping = Object.assign({}, defaultInputCSVHeaderMappings, headerMapping); - const rows = await membersCSV.parse(inputFilePath, inputMapping, defaultLabels); + // completely rely on explicit user input for header mappings + const rows = await membersCSV.parse(inputFilePath, headerMapping, defaultLabels); const columns = Object.keys(rows[0]); const numberOfBatches = Math.ceil(rows.length / batchSize); const mappedCSV = membersCSV.unparse(rows, columns); @@ -95,7 +105,7 @@ module.exports = class MembersCSVImporter { * @param {string} filePath - the path to a "prepared" CSV file */ async perform(filePath) { - const rows = membersCSV.parse(filePath, defaultInputCSVHeaderMappings); + const rows = membersCSV.parse(filePath, DEFAULT_CSV_HEADER_MAPPING); const membersApi = await this._getMembersApi(); @@ -118,7 +128,15 @@ module.exports = class MembersCSVImporter { }; try { - const existingMember = await membersApi.members.get({email: row.email}, { + const memberValues = { + email: row.email, + name: row.name, + note: row.note, + subscribed: row.subscribed, + created_at: row.created_at, + labels: row.labels + }; + const existingMember = await membersApi.members.get({email: memberValues.email}, { ...options, withRelated: ['labels'] }); @@ -126,14 +144,14 @@ module.exports = class MembersCSVImporter { if (existingMember) { const existingLabels = existingMember.related('labels') ? existingMember.related('labels').toJSON() : []; member = await membersApi.members.update({ - ...row, - labels: existingLabels.concat(row.labels) + ...memberValues, + labels: existingLabels.concat(memberValues.labels) }, { ...options, id: existingMember.id }); } else { - member = await membersApi.members.create(row, Object.assign({}, options, { + member = await membersApi.members.create(memberValues, Object.assign({}, options, { context: { import: true } diff --git a/ghost/members-importer/test/fixtures/member-csv-export.csv b/ghost/members-importer/test/fixtures/member-csv-export.csv new file mode 100644 index 000000000000..3b009d6dc35f --- /dev/null +++ b/ghost/members-importer/test/fixtures/member-csv-export.csv @@ -0,0 +1,3 @@ +id,email,name,note,subscribed_to_emails,complimentary_plan,stripe_customer_id,created_at,deleted_at,labels,products +634e48e056ef99c6a7af5850,member_complimentary_test@example.com,"bobby tables","a note",true,true,,2022-10-18T06:34:08.000Z,,"user import label",This is a Bronze Tier +634e566dde68acf282cd311e,member_stripe_test@example.com,"stirpey beaver","testing notes",false,,cus_MdR9tqW6bAreiq,2022-10-18T07:31:57.000Z,,,This is a Premium Tier diff --git a/ghost/members-importer/test/importer.test.js b/ghost/members-importer/test/importer.test.js index e7a22084ddea..85340ee4966b 100644 --- a/ghost/members-importer/test/importer.test.js +++ b/ghost/members-importer/test/importer.test.js @@ -17,6 +17,18 @@ describe('Importer', function () { let sendEmailStub; let membersApiStub; + const defaultAllowedFields = { + email: 'email', + name: 'name', + note: 'note', + subscribed_to_emails: 'subscribed', + created_at: 'created_at', + complimentary_plan: 'complimentary_plan', + stripe_customer_id: 'stripe_customer_id', + labels: 'labels', + products: 'products' + }; + beforeEach(function () { fsWriteSpy = sinon.spy(fs, 'writeFile'); }); @@ -37,7 +49,9 @@ describe('Importer', function () { id: 'default_product_id' }; - memberCreateStub = sinon.stub().resolves(null); + memberCreateStub = sinon.stub().resolves({ + id: `test_member_id` + }); membersApiStub = { productRepository: { list: async () => { @@ -50,7 +64,9 @@ describe('Importer', function () { get: async () => { return null; }, - create: memberCreateStub + create: memberCreateStub, + update: sinon.stub().resolves(null), + linkStripeCustomer: sinon.stub().resolves(null) } }; @@ -86,7 +102,7 @@ describe('Importer', function () { const result = await importer.process({ pathToCSV: `${csvPath}/single-column-with-header.csv`, - headerMapping: {}, + headerMapping: defaultAllowedFields, importLabel: { name: 'test import' }, @@ -113,6 +129,86 @@ describe('Importer', function () { memberCreateStub.notCalled.should.be.false(); memberCreateStub.firstCall.lastArg.context.import.should.be.true(); }); + + it('should import a CSV in the default Members export format', async function () { + const internalLabel = { + name: 'Test Import' + }; + const LabelModelStub = { + findOne: sinon.stub() + .withArgs({ + name: 'Test Import' + }) + .resolves({ + name: 'Test Import' + }) + }; + + const importer = buildMockImporterInstance(); + const result = await importer.process({ + pathToCSV: `${csvPath}/member-csv-export.csv`, + headerMapping: defaultAllowedFields, + importLabel: { + name: 'Test Import' + }, + user: { + email: 'test@example.com' + }, + LabelModel: LabelModelStub + }); + + should.exist(result.meta); + should.exist(result.meta.stats); + should.exist(result.meta.stats.imported); + result.meta.stats.imported.should.equal(2); + + should.exist(result.meta.stats.invalid); + should.deepEqual(result.meta.import_label, internalLabel); + + should.exist(result.meta.originalImportSize); + result.meta.originalImportSize.should.equal(2); + + fsWriteSpy.calledOnce.should.be.true(); + + // member records get inserted + membersApiStub.members.create.calledTwice.should.be.true(); + + should.equal(membersApiStub.members.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, [{ + 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'); + + // 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'); + + // products import + membersApiStub.members.update.calledOnce.should.be.true(); + should.deepEqual(membersApiStub.members.update.args[0][0].products, [{ + id: 'default_product_id' + }]); + should.deepEqual(membersApiStub.members.update.args[0][1].id, 'test_member_id'); + }); }); describe('sendErrorEmail', function () { @@ -148,7 +244,7 @@ describe('Importer', function () { it('processes a basic valid import file for members', async function () { const membersImporter = buildMockImporterInstance(); - const result = await membersImporter.prepare(`${csvPath}/single-column-with-header.csv`); + const result = await membersImporter.prepare(`${csvPath}/single-column-with-header.csv`, defaultAllowedFields); should.exist(result.filePath); result.filePath.should.match(/\/members-importer\/test\/fixtures\/Members Import/); @@ -162,7 +258,7 @@ describe('Importer', function () { it('Does not include columns not in the original CSV or mapped', async function () { const membersImporter = buildMockImporterInstance(); - await membersImporter.prepare(`${csvPath}/single-column-with-header.csv`); + await membersImporter.prepare(`${csvPath}/single-column-with-header.csv`, defaultAllowedFields); const fileContents = fsWriteSpy.firstCall.args[1]; @@ -172,7 +268,7 @@ describe('Importer', function () { it('It supports "subscribed_to_emails" column header ouf of the box', async function (){ const membersImporter = buildMockImporterInstance(); - await membersImporter.prepare(`${csvPath}/subscribed-to-emails-header.csv`); + await membersImporter.prepare(`${csvPath}/subscribed-to-emails-header.csv`, defaultAllowedFields); const fileContents = fsWriteSpy.firstCall.args[1]; @@ -184,7 +280,7 @@ describe('Importer', function () { it('performs import on a single csv file', async function () { const importer = buildMockImporterInstance(); - const result = await importer.perform(`${csvPath}/single-column-with-header.csv`); + 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, []);