Skip to content

Commit

Permalink
Added strict field mapping to member CSV importer
Browse files Browse the repository at this point in the history 
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'
  • Loading branch information
@naz
naz committed Oct 19, 2022
1 parent 748ef87 commit 90768e9
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 19 deletions.
42 changes: 30 additions & 12 deletions ghost/members-importer/lib/importer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.<string, string>} headerMapping - An object whose keys are headers in the input CSV and values are the header to replace it with
* @param {Array<string>} defaultLabels - A list of labels to apply to every member
* @param {Object.<string, string>} [headerMapping] - An object whose keys are headers in the input CSV and values are the header to replace it with
* @param {Array<string>} [defaultLabels] - A list of labels to apply to every member
*
* @returns {Promise<{filePath: string, batches: number, metadata: Object.<string, any>}>} - 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;

Expand All @@ -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);
Expand All @@ -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();

Expand All @@ -118,22 +128,30 @@ 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']
});
let member;
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
}
Expand Down
3 changes: 3 additions & 0 deletions ghost/members-importer/test/fixtures/member-csv-export.csv
View file
Original file line number Diff line number Diff line change
@@ -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
110 changes: 103 additions & 7 deletions ghost/members-importer/test/importer.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
Expand All @@ -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 () => {
Expand All @@ -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)
}
};

Expand Down Expand Up @@ -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'
},
Expand All @@ -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 () {
Expand Down Expand Up @@ -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/);
Expand All @@ -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];

Expand All @@ -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];

Expand All @@ -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, []);
Expand Down

0 comments on commit 90768e9

Please sign in to comment.