Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 Fixed members import unsubscribing members when subscribe_to_emails is blank #19658

Merged
merged 6 commits into from
Feb 6, 2024
Merged

🐛 Fixed members import unsubscribing members when subscribe_to_emails is blank #19658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0dd1432
Added a failing test for members being unsubscribed when `subscribed_…
cmraible Feb 6, 2024
ce272da
Added invalid cases
cmraible Feb 6, 2024
4d206f3
Fixed the bug
cmraible Feb 6, 2024
3ec72aa
Renamed mappedMembers to rows, and member to row
cmraible Feb 6, 2024
4d6c796
Re-added backwards compatibility while allowing for undefined
cmraible Feb 6, 2024
0270228
Fixed linter error and updated an unparse test
cmraible Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions ghost/members-csv/lib/unparse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,53 +14,53 @@ const DEFAULT_COLUMNS = [
'tiers'
];

const unparse = (members, columns = DEFAULT_COLUMNS.slice()) => {
const unparse = (rows, columns = DEFAULT_COLUMNS.slice()) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but at this point these are just rows in a CSV, not members

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,
Copy link
Contributor Author

@cmraible cmraible Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 99% sure there's no way currently to get a value in row.subscribed, BUT on the off chance I'm missing something here, I changed this up to more explicitly check if the key exists in the row (even if the value is undefined) vs using the || operator.

This allows the expected value (row.subscribed_to_emails) to be undefined instead of coercing it to a boolean.

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
});
};
Expand Down
2 changes: 1 addition & 1 deletion ghost/members-csv/test/unparse.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down
218 changes: 217 additions & 1 deletion ghost/members-importer/test/MembersCSVImporter.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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 () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit verbose and cumbersome, but this one has come up a couple times so I want to just squash it dead.

/**
* @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 () {
Expand Down
Loading
Loading