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.

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

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

Conversation

cmraible
Copy link
Contributor

@cmraible cmraible commented Feb 6, 2024

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.

@@ -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

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

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.

@@ -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.

@cmraible cmraible marked this pull request as ready for review February 6, 2024 03:16
Copy link
Contributor

@9larsons 9larsons left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for putting in some notes. I think my only comments here are limited to the confusing duplicative/overlapping columns and data model which is something we should revisit down the road.

@cmraible
Copy link
Contributor Author

cmraible commented Feb 6, 2024

Hard agree!

@cmraible cmraible force-pushed the chris-eng-611-importer-issue-blank-subscribed-to-emails-column-is-setting branch from e5e3f72 to 0270228 Compare February 6, 2024 17:22
@cmraible cmraible merged commit c2fd22a into TryGhost:main Feb 6, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants