From 0825a2d7f454b6c2182a94874d1f5a35921b0571 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Thu, 15 Dec 2022 17:45:11 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20importing=20existing=20m?= =?UTF-8?q?ember=20resetting=20newsletters=20(#16017)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://github.com/TryGhost/Team/issues/2386 **Issue:** - When trying to import a member that already exists, and has 'subscribed' set to 'true' in the CSV, the newsletters the member is subscribed to are reset to the default newsletters. - When ediging a member with the API and setting `subscribed` to true, the same happens. **Cause:** A faulty check for the `status` property of a newsletter. Fixed and added a new E2E test. --- .../admin/__snapshots__/members.test.js.snap | 219 ++++++++++++++++++ ghost/core/test/e2e-api/admin/members.test.js | 69 ++++++ ghost/members-api/lib/repositories/member.js | 2 +- 3 files changed, 289 insertions(+), 1 deletion(-) diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap index 4a4715f961..37f06d67b9 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap @@ -5342,6 +5342,225 @@ Object { } `; +exports[`Members API Setting subscribed when editing a member won't reset to default newsletters 1: [body] 1`] = ` +Object { + "members": Array [ + Object { + "attribution": Object { + "id": null, + "referrer_medium": "Ghost Admin", + "referrer_source": "Created manually", + "referrer_url": null, + "title": null, + "type": "url", + "url": null, + }, + "avatar_image": null, + "comped": false, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "email": "memberTestChangeSubscribedAttribute@test.com", + "email_count": 0, + "email_open_rate": null, + "email_opened_count": 0, + "email_suppression": Object { + "info": null, + "suppressed": false, + }, + "geolocation": null, + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "labels": Any, + "last_seen_at": null, + "name": "test newsletter", + "newsletters": Array [ + Object { + "body_font_category": "serif", + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "description": null, + "feedback_enabled": false, + "footer_content": null, + "header_image": "http://127.0.0.1:2369/content/images/2022/05/test.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": "Daily newsletter", + "sender_email": "jamie@example.com", + "sender_name": "Jamie", + "sender_reply_to": "newsletter", + "show_badge": true, + "show_feature_image": true, + "show_header_icon": true, + "show_header_name": true, + "show_header_title": true, + "slug": "daily-newsletter", + "sort_order": 1, + "status": "active", + "subscribe_on_signup": false, + "title_alignment": "center", + "title_font_category": "serif", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "visibility": "members", + }, + Object { + "body_font_category": "serif", + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "description": null, + "feedback_enabled": false, + "footer_content": null, + "header_image": "http://127.0.0.1:2369/content/images/2022/05/test.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": "Weekly newsletter", + "sender_email": "jamie@example.com", + "sender_name": "Jamie", + "sender_reply_to": "newsletter", + "show_badge": true, + "show_feature_image": true, + "show_header_icon": true, + "show_header_name": true, + "show_header_title": true, + "slug": "weekly-newsletter", + "sort_order": 2, + "status": "active", + "subscribe_on_signup": true, + "title_alignment": "center", + "title_font_category": "serif", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "visibility": "members", + }, + ], + "note": null, + "status": "free", + "subscribed": true, + "subscriptions": Any, + "tiers": Array [], + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + ], +} +`; + +exports[`Members API Setting subscribed when editing a member won't reset to default newsletters 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "2147", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "location": StringMatching /https\\?:\\\\/\\\\/\\.\\*\\?\\\\/members\\\\/\\[a-f0-9\\]\\{24\\}\\\\//, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Members API Setting subscribed when editing a member won't reset to default newsletters 3: [body] 1`] = ` +Object { + "members": Array [ + Object { + "attribution": Object { + "id": null, + "referrer_medium": "Ghost Admin", + "referrer_source": "Created manually", + "referrer_url": null, + "title": null, + "type": "url", + "url": null, + }, + "avatar_image": null, + "comped": false, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "email": "memberTestChangeSubscribedAttribute@test.com", + "email_count": 0, + "email_open_rate": null, + "email_opened_count": 0, + "email_suppression": Object { + "info": null, + "suppressed": false, + }, + "geolocation": null, + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "labels": Any, + "last_seen_at": null, + "name": "test newsletter", + "newsletters": Array [ + Object { + "body_font_category": "serif", + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "description": null, + "feedback_enabled": false, + "footer_content": null, + "header_image": "http://127.0.0.1:2369/content/images/2022/05/test.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": "Daily newsletter", + "sender_email": "jamie@example.com", + "sender_name": "Jamie", + "sender_reply_to": "newsletter", + "show_badge": true, + "show_feature_image": true, + "show_header_icon": true, + "show_header_name": true, + "show_header_title": true, + "slug": "daily-newsletter", + "sort_order": 1, + "status": "active", + "subscribe_on_signup": false, + "title_alignment": "center", + "title_font_category": "serif", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "visibility": "members", + }, + Object { + "body_font_category": "serif", + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "description": null, + "feedback_enabled": false, + "footer_content": null, + "header_image": "http://127.0.0.1:2369/content/images/2022/05/test.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": "Weekly newsletter", + "sender_email": "jamie@example.com", + "sender_name": "Jamie", + "sender_reply_to": "newsletter", + "show_badge": true, + "show_feature_image": true, + "show_header_icon": true, + "show_header_name": true, + "show_header_title": true, + "slug": "weekly-newsletter", + "sort_order": 2, + "status": "active", + "subscribe_on_signup": true, + "title_alignment": "center", + "title_font_category": "serif", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "visibility": "members", + }, + ], + "note": null, + "status": "free", + "subscribed": true, + "subscriptions": Any, + "tiers": Array [], + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + ], +} +`; + +exports[`Members API Setting subscribed when editing a member won't reset to default newsletters 4: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "2147", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Members API Subscribes to default newsletters 1: [body] 1`] = ` Object { "members": Array [ diff --git a/ghost/core/test/e2e-api/admin/members.test.js b/ghost/core/test/e2e-api/admin/members.test.js index 94cb3c2260..0213573460 100644 --- a/ghost/core/test/e2e-api/admin/members.test.js +++ b/ghost/core/test/e2e-api/admin/members.test.js @@ -2332,6 +2332,75 @@ describe('Members API', function () { .expectStatus(422); }); + it('Setting subscribed when editing a member won\'t reset to default newsletters', async function () { + // First check that this newsletter is off by default, or this test would not make sense + const newsletter = await models.Newsletter.findOne({id: testUtils.DataGenerator.Content.newsletters[0].id}, {require: true}); + assert.equal(newsletter.get('subscribe_on_signup'), false, 'This test expects the newsletter to be off by default'); + + // Add custom newsletter list to new member + const member = { + name: 'test newsletter', + email: 'memberTestChangeSubscribedAttribute@test.com', + newsletters: [ + { + id: testUtils.DataGenerator.Content.newsletters[0].id // This is off by default + }, + { + id: testUtils.DataGenerator.Content.newsletters[1].id + } + ] + }; + + const {body} = await agent + .post(`/members/`) + .body({members: [member]}) + .expectStatus(201) + .matchBodySnapshot({ + members: [{ + id: anyObjectId, + uuid: anyUuid, + created_at: anyISODateTime, + updated_at: anyISODateTime, + subscriptions: anyArray, + labels: anyArray, + newsletters: Array(2).fill(newsletterSnapshot) + }] + }) + .matchHeaderSnapshot({ + etag: anyEtag, + location: anyLocationFor('members') + }); + + const memberId = body.members[0].id; + const editedMember = { + subscribed: true // no change + }; + + // Edit member + const {body: body2} = await agent + .put(`/members/${memberId}`) + .body({members: [editedMember]}) + .expectStatus(200) + .matchBodySnapshot({ + members: [{ + id: anyObjectId, + uuid: anyUuid, + created_at: anyISODateTime, + updated_at: anyISODateTime, + subscriptions: anyArray, + labels: anyArray, + newsletters: Array(2).fill(newsletterSnapshot) + }] + }) + .matchHeaderSnapshot({ + etag: anyEtag + }); + const changedMember = body2.members[0]; + assert.equal(changedMember.newsletters.length, 2); + assert.ok(changedMember.newsletters.find(n => n.id === testUtils.DataGenerator.Content.newsletters[0].id), 'The member is still subscribed for a newsletter that is off by default'); + assert.ok(changedMember.newsletters.find(n => n.id === testUtils.DataGenerator.Content.newsletters[1].id), 'The member is still subscribed for the newsletter it subscribed to'); + }); + it('Can add and send a signup confirmation email (old)', async function () { const filteredNewsletters = newsletters.filter(n => n.get('subscribe_on_signup')); filteredNewsletters.length.should.be.greaterThan(0, 'For this test to work, we need at least one newsletter fixture with subscribe_on_signup = true'); diff --git a/ghost/members-api/lib/repositories/member.js b/ghost/members-api/lib/repositories/member.js index ea2fab6810..862146bb25 100644 --- a/ghost/members-api/lib/repositories/member.js +++ b/ghost/members-api/lib/repositories/member.js @@ -522,7 +522,7 @@ module.exports = class MemberRepository { if (!memberData.newsletters) { if (memberData.subscribed === false) { memberData.newsletters = []; - } else if (memberData.subscribed === true && !existingNewsletters.find(n => n.status === 'active')) { + } else if (memberData.subscribed === true && !existingNewsletters.find(n => n.get('status') === 'active')) { const browseOptions = _.pick(options, 'transacting'); memberData.newsletters = await this.getSubscribeOnSignupNewsletters(browseOptions); }