From c2fd22a246c30795b75085c2704b49d0f9792ea5 Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Tue, 6 Feb 2024 10:19:16 -0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20members=20import=20unsub?= =?UTF-8?q?scribing=20members=20when=20`subscribe=5Fto=5Femails`=20is=20em?= =?UTF-8?q?pty=20(#19658)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes ENG-611 - Previously, if an existing member with newsletter subscriptions was imported, and `subscribe_to_emails` was blank/empty, the member would be unsubscribed from all newsletters, which is not the expected behavior. - This PR changes the behavior so if `subscribe_to_emails` is blank, it will not unsubscribe existing members. --- ghost/members-csv/lib/unparse.js | 42 ++-- ghost/members-csv/test/unparse.test.js | 2 +- .../test/MembersCSVImporter.test.js | 218 +++++++++++++++++- .../fixtures/subscribed-to-emails-cases.csv | 16 ++ 4 files changed, 255 insertions(+), 23 deletions(-) create mode 100644 ghost/members-importer/test/fixtures/subscribed-to-emails-cases.csv diff --git a/ghost/members-csv/lib/unparse.js b/ghost/members-csv/lib/unparse.js index 58abb231c2..5cf4f1f046 100644 --- a/ghost/members-csv/lib/unparse.js +++ b/ghost/members-csv/lib/unparse.js @@ -14,53 +14,53 @@ const DEFAULT_COLUMNS = [ 'tiers' ]; -const unparse = (members, columns = DEFAULT_COLUMNS.slice()) => { +const unparse = (rows, columns = DEFAULT_COLUMNS.slice()) => { columns = columns.map((column) => { if (column === 'subscribed') { return 'subscribed_to_emails'; } return column; }); - const mappedMembers = members.map((member) => { - if (member.error && !columns.includes('error')) { + const mappedRows = rows.map((row) => { + if (row.error && !columns.includes('error')) { columns.push('error'); } let labels = ''; - if (typeof member.labels === 'string') { - labels = member.labels; - } else if (Array.isArray(member.labels)) { - labels = member.labels.map((l) => { + if (typeof row.labels === 'string') { + labels = row.labels; + } else if (Array.isArray(row.labels)) { + labels = row.labels.map((l) => { return typeof l === 'string' ? l : l.name; }).join(','); } let tiers = ''; - if (Array.isArray(member.tiers)) { - tiers = member.tiers.map((tier) => { + if (Array.isArray(row.tiers)) { + tiers = row.tiers.map((tier) => { return tier.name; }).join(','); } return { - id: member.id, - email: member.email, - name: member.name, - note: member.note, - subscribed_to_emails: member.subscribed || member.subscribed_to_emails ? true : false, - complimentary_plan: member.comped || member.complimentary_plan, - stripe_customer_id: _.get(member, 'subscriptions[0].customer.id') || member.stripe_customer_id, - created_at: member.created_at, - deleted_at: member.deleted_at, + id: row.id, + email: row.email, + name: row.name, + note: row.note, + subscribed_to_emails: 'subscribed' in row ? row.subscribed : row.subscribed_to_emails, + complimentary_plan: row.comped || row.complimentary_plan, + stripe_customer_id: _.get(row, 'subscriptions[0].customer.id') || row.stripe_customer_id, + created_at: row.created_at, + deleted_at: row.deleted_at, labels: labels, tiers: tiers, - import_tier: member.import_tier || null, - error: member.error || null + import_tier: row.import_tier || null, + error: row.error || null }; }); - return papaparse.unparse(mappedMembers, { + return papaparse.unparse(mappedRows, { columns }); }; diff --git a/ghost/members-csv/test/unparse.test.js b/ghost/members-csv/test/unparse.test.js index 47a48ccaf1..f139f10998 100644 --- a/ghost/members-csv/test/unparse.test.js +++ b/ghost/members-csv/test/unparse.test.js @@ -13,7 +13,7 @@ describe('unparse', function () { assert.ok(result); - const expected = `id,email,name,note,subscribed_to_emails,complimentary_plan,stripe_customer_id,created_at,deleted_at,labels,tiers\r\n,email@example.com,Sam Memberino,Early supporter,false,,,,,,`; + const expected = `id,email,name,note,subscribed_to_emails,complimentary_plan,stripe_customer_id,created_at,deleted_at,labels,tiers\r\n,email@example.com,Sam Memberino,Early supporter,,,,,,,`; assert.equal(result, expected); }); diff --git a/ghost/members-importer/test/MembersCSVImporter.test.js b/ghost/members-importer/test/MembersCSVImporter.test.js index 06ff3ebe30..e7775a6c31 100644 --- a/ghost/members-importer/test/MembersCSVImporter.test.js +++ b/ghost/members-importer/test/MembersCSVImporter.test.js @@ -25,7 +25,7 @@ describe('MembersCSVImporter', function () { email: 'email', name: 'name', note: 'note', - subscribed_to_emails: 'subscribed', + subscribed_to_emails: 'subscribed_to_emails', created_at: 'created_at', complimentary_plan: 'complimentary_plan', stripe_customer_id: 'stripe_customer_id', @@ -221,6 +221,222 @@ describe('MembersCSVImporter', function () { }]); should.deepEqual(membersRepositoryStub.update.args[0][1].id, 'test_member_id'); }); + + it('should subscribe or unsubscribe members as per the `subscribe_to_emails` column', async function () { + /** + * @NOTE This tests all the different scenarios for the `subscribed_to_emails` column for existing and new members + * For existing members with at least one newsletter subscription: + * CASE 1: If `subscribe_to_emails` is `true`, the member should remain subscribed (but not added to any additional newsletters) + * CASE 2: If `subscribe_to_emails` is `false`, the member should be unsubscribed from all newsletters + * CASE 3: If `subscribe_to_emails` is NULL, the member should remain subscribed (but not added to any additional newsletters) + * CASE 4: If `subscribe_to_emails` is empty, the member should remain subscribed (but not added to any additional newsletters) + * CASE 5: If `subscribe_to_emails` is invalid, the member should remain subscribed (but not added to any additional newsletters) + * + * + * For existing members with no newsletter subscriptions: + * CASE 6: If `subscribe_to_emails` is `true`, the member should remain unsubscribed (as they likely have previously unsubscribed) + * CASE 7: If `subscribe_to_emails` is `false`, the member should remain unsubscribed + * CASE 8: If `subscribe_to_emails` is NULL, the member should remain unsubscribed + * CASE 9: If `subscribe_to_emails` is empty, the member should remain unsubscribed + * CASE 10: If `subscribe_to_emails` is invalid, the member should remain unsubscribed + * + * - In summary, an existing member with no pre-existing newsletter subscriptions should _never_ be subscribed to newsletters upon import + * + * For new members: + * CASE 11: If `subscribe_to_emails` is `true`, the member should be created and subscribed + * CASE 12: If `subscribe_to_emails` is `false`, the member should be created but not subscribed + * CASE 13: If `subscribe_to_emails` is NULL, the member should be created and subscribed + * CASE 14: If `subscribe_to_emails` is empty, the member should be created and subscribed + * CASE 15: If `subscribe_to_emails` is invalid, the member should be created and subscribed + */ + + const internalLabel = { + name: 'Test Subscription Import' + }; + const LabelModelStub = { + findOne: sinon.stub() + .withArgs({ + name: 'Test Subscription Import' + }) + .resolves({ + name: 'Test Subscription Import' + }) + }; + + const importer = buildMockImporterInstance(); + + const defaultNewsletters = [ + {id: 'newsletter_1'}, + {id: 'newsletter_2'} + ]; + + const existingMembers = [ + {email: 'member_subscribed_true@example.com', newsletters: defaultNewsletters}, + {email: 'member_subscribed_null@example.com', newsletters: defaultNewsletters}, + {email: 'member_subscribed_false@example.com', newsletters: defaultNewsletters}, + {email: 'member_subscribed_empty@example.com', newsletters: defaultNewsletters}, + {email: 'member_subscribed_invalid@example.com', newsletters: defaultNewsletters}, + {email: 'member_not_subscribed_true@example.com', newsletters: []}, + {email: 'member_not_subscribed_null@example.com', newsletters: []}, + {email: 'member_not_subscribed_false@example.com', newsletters: []}, + {email: 'member_not_subscribed_empty@example.com', newsletters: []}, + {email: 'member_not_subscribed_invalid@example.com', newsletters: []} + ]; + + membersRepositoryStub.get = sinon.stub(); + + for (const existingMember of existingMembers) { + const newslettersCollection = { + length: existingMember.newsletters.length, + toJSON: sinon.stub().returns(existingMember.newsletters) + }; + const memberRecord = { + related: sinon.stub() + }; + memberRecord.related.withArgs('labels').returns(null); + memberRecord.related.withArgs('newsletters').returns(newslettersCollection); + membersRepositoryStub.get.withArgs({email: existingMember.email}).resolves(memberRecord); + } + + const result = await importer.process({ + pathToCSV: `${csvPath}/subscribed-to-emails-cases.csv`, + headerMapping: defaultAllowedFields, + importLabel: { + name: 'Test Subscription Import' + }, + user: { + email: 'test@example.com' + }, + LabelModel: LabelModelStub, + forceInline: true, + verificationTrigger: { + testImportThreshold: () => {} + } + }); + + should.exist(result.meta); + should.exist(result.meta.stats); + should.exist(result.meta.stats.imported); + result.meta.stats.imported.should.equal(5); + + should.exist(result.meta.stats.invalid); + should.deepEqual(result.meta.import_label, internalLabel); + + should.exist(result.meta.originalImportSize); + result.meta.originalImportSize.should.equal(15); + + fsWriteSpy.calledOnce.should.be.true(); + + // member records get inserted + should.equal(membersRepositoryStub.create.callCount, 5); + + should.equal(membersRepositoryStub.create.args[0][1].context.import, true, 'inserts are done in the "import" context'); + + // CASE 1: Existing member with at least one newsletter subscription, `subscribed_to_emails` column is true + // Member's newsletter subscriptions should not change + should.deepEqual(Object.keys(membersRepositoryStub.update.args[0][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels', 'newsletters']); + should.equal(membersRepositoryStub.update.args[0][0].email, 'member_subscribed_true@example.com'); + should.equal(membersRepositoryStub.update.args[0][0].subscribed, true); + should.deepEqual(membersRepositoryStub.update.args[0][0].newsletters, defaultNewsletters); + + // CASE 2: Existing member with at least one newsletter subscription, `subscribed_to_emails` column is false + // Member's newsletter subscriptions should be removed + should.deepEqual(Object.keys(membersRepositoryStub.update.args[1][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.update.args[1][0].email, 'member_subscribed_false@example.com'); + should.equal(membersRepositoryStub.update.args[1][0].subscribed, false); + should.equal(membersRepositoryStub.update.args[1][0].newsletters, undefined); + + // CASE 3: Existing member with at least one newsletter subscription, `subscribed_to_emails` column is NULL + // Member's newsletter subscriptions should not change + should.deepEqual(Object.keys(membersRepositoryStub.update.args[2][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels', 'newsletters']); + should.equal(membersRepositoryStub.update.args[2][0].email, 'member_subscribed_null@example.com'); + should.equal(membersRepositoryStub.update.args[2][0].subscribed, true); + should.deepEqual(membersRepositoryStub.update.args[2][0].newsletters, defaultNewsletters); + + // CASE 4: Existing member with at least one newsletter subscription, `subscribed_to_emails` column is empty + // Member's newsletter subscriptions should not change + should.deepEqual(Object.keys(membersRepositoryStub.update.args[3][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels', 'newsletters']); + should.equal(membersRepositoryStub.update.args[3][0].email, 'member_subscribed_empty@example.com'); + should.equal(membersRepositoryStub.update.args[3][0].subscribed, true); + should.deepEqual(membersRepositoryStub.update.args[3][0].newsletters, defaultNewsletters); + + // CASE 5: Existing member with at least one newsletter subscription, `subscribed_to_emails` column is invalid + // Member's newsletter subscriptions should not change + should.deepEqual(Object.keys(membersRepositoryStub.update.args[4][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels', 'newsletters']); + should.equal(membersRepositoryStub.update.args[4][0].email, 'member_subscribed_invalid@example.com'); + should.equal(membersRepositoryStub.update.args[4][0].subscribed, true); + should.deepEqual(membersRepositoryStub.update.args[4][0].newsletters, defaultNewsletters); + + // CASE 6: Existing member with no newsletter subscriptions, `subscribed_to_emails` column is true + // Member should remain unsubscribed and not added to any newsletters + should.deepEqual(Object.keys(membersRepositoryStub.update.args[5][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.update.args[5][0].email, 'member_not_subscribed_true@example.com'); + should.equal(membersRepositoryStub.update.args[5][0].subscribed, false); + should.equal(membersRepositoryStub.update.args[5][0].newsletters, undefined); + + // CASE 7: Existing member with no newsletter subscriptions, `subscribed_to_emails` column is false + // Member should remain unsubscribed and not added to any newsletters + should.deepEqual(Object.keys(membersRepositoryStub.update.args[6][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.update.args[6][0].email, 'member_not_subscribed_false@example.com'); + should.equal(membersRepositoryStub.update.args[6][0].subscribed, false); + should.equal(membersRepositoryStub.update.args[6][0].newsletters, undefined); + + // CASE 8: Existing member with no newsletter subscriptions, `subscribed_to_emails` column is NULL + // Member should remain unsubscribed and not added to any newsletters + should.deepEqual(Object.keys(membersRepositoryStub.update.args[7][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.update.args[7][0].email, 'member_not_subscribed_null@example.com'); + should.equal(membersRepositoryStub.update.args[7][0].subscribed, false); + should.equal(membersRepositoryStub.update.args[7][0].newsletters, undefined); + + // CASE 9: Existing member with no newsletter subscriptions, `subscribed_to_emails` column is empty + // Member should remain unsubscribed and not added to any newsletters + should.deepEqual(Object.keys(membersRepositoryStub.update.args[8][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.update.args[8][0].email, 'member_not_subscribed_empty@example.com'); + should.equal(membersRepositoryStub.update.args[8][0].subscribed, false); + should.equal(membersRepositoryStub.update.args[8][0].newsletters, undefined); + + // CASE 10: Existing member with no newsletter subscriptions, `subscribed_to_emails` column is invalid + // Member should remain unsubscribed and not added to any newsletters + should.deepEqual(Object.keys(membersRepositoryStub.update.args[9][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.update.args[9][0].email, 'member_not_subscribed_invalid@example.com'); + should.equal(membersRepositoryStub.update.args[9][0].subscribed, false); + should.equal(membersRepositoryStub.update.args[9][0].newsletters, undefined); + + // CASE 11: New member, `subscribed_to_emails` column is true + // Member should be created and subscribed + should.deepEqual(Object.keys(membersRepositoryStub.create.args[0][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.create.args[0][0].email, 'new_member_true@example.com'); + should.equal(membersRepositoryStub.create.args[0][0].subscribed, true); + should.equal(membersRepositoryStub.create.args[0][0].newsletters, undefined); + + // CASE 12: New member, `subscribed_to_emails` column is false + // Member should be created but not subscribed + should.deepEqual(Object.keys(membersRepositoryStub.create.args[1][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.create.args[1][0].email, 'new_member_false@example.com'); + should.equal(membersRepositoryStub.create.args[1][0].subscribed, false); + should.equal(membersRepositoryStub.create.args[1][0].newsletters, undefined); + + // CASE 13: New member, `subscribed_to_emails` column is NULL + // Member should be created but not subscribed + should.deepEqual(Object.keys(membersRepositoryStub.create.args[2][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.create.args[2][0].email, 'new_member_null@example.com'); + should.equal(membersRepositoryStub.create.args[2][0].subscribed, true); + should.equal(membersRepositoryStub.create.args[2][0].newsletters, undefined); + + // CASE 14: New member, `subscribed_to_emails` column is empty + // Member should be created but not subscribed + should.deepEqual(Object.keys(membersRepositoryStub.create.args[3][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.create.args[3][0].email, 'new_member_empty@example.com'); + should.equal(membersRepositoryStub.create.args[3][0].subscribed, true); + should.equal(membersRepositoryStub.create.args[3][0].newsletters, undefined); + + // CASE 15: New member, `subscribed_to_emails` column is invalid + // Member should be created but not subscribed + should.deepEqual(Object.keys(membersRepositoryStub.create.args[4][0]), ['email', 'name', 'note', 'subscribed', 'created_at', 'labels']); + should.equal(membersRepositoryStub.create.args[4][0].email, 'new_member_invalid@example.com'); + should.equal(membersRepositoryStub.create.args[4][0].subscribed, true); + should.equal(membersRepositoryStub.create.args[4][0].newsletters, undefined); + }); }); describe('sendErrorEmail', function () { diff --git a/ghost/members-importer/test/fixtures/subscribed-to-emails-cases.csv b/ghost/members-importer/test/fixtures/subscribed-to-emails-cases.csv new file mode 100644 index 0000000000..4af2abd400 --- /dev/null +++ b/ghost/members-importer/test/fixtures/subscribed-to-emails-cases.csv @@ -0,0 +1,16 @@ +email,name,note,subscribed_to_emails,stripe_customer_id,complimentary_plan,labels,created_at +member_subscribed_true@example.com,Test Email,This is a test template for importing your members list to Ghost,true,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_subscribed_false@example.com,Test Email,This is a test template for importing your members list to Ghost,false,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_subscribed_null@example.com,Test Email,This is a test template for importing your members list to Ghost,null,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_subscribed_empty@example.com,Test Email,This is a test template for importing your members list to Ghost,,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_subscribed_invalid@example.com,Test Email,This is a test template for importing your members list to Ghost,asdfasdf,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_not_subscribed_true@example.com,Test Email,This is a test template for importing your members list to Ghost,true,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_not_subscribed_false@example.com,Test Email,This is a test template for importing your members list to Ghost,false,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_not_subscribed_null@example.com,Test Email,This is a test template for importing your members list to Ghost,null,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_not_subscribed_empty@example.com,Test Email,This is a test template for importing your members list to Ghost,,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +member_not_subscribed_invalid@example.com,Test Email,This is a test template for importing your members list to Ghost,asdfasdf,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +new_member_true@example.com,Test Email,This is a test template for importing your members list to Ghost,true,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +new_member_false@example.com,Test Email,This is a test template for importing your members list to Ghost,false,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +new_member_null@example.com,Test Email,This is a test template for importing your members list to Ghost,null,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +new_member_empty@example.com,Test Email,This is a test template for importing your members list to Ghost,,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z +new_member_invalid@example.com,Test Email,This is a test template for importing your members list to Ghost,asdfasdf,Example: cus_GdsYH4fZbHx9hF,false,"vip,promotion",2019-10-30T14:52:08.000Z \ No newline at end of file