From 8b45af345869c33695d671e14f9d76dea7d91070 Mon Sep 17 00:00:00 2001 From: Sag Date: Tue, 9 Jul 2024 12:11:26 +0200 Subject: [PATCH] Cleaned up 'Filter by email disabled' GA feature flag (#20554) no issue - "Filter by email disabled" feature has been released to GA in [Ghost v5.74.0](https://github.com/TryGhost/Ghost/releases/tag/v5.74.0) (commit: 32d0d2b293ecba1bb1943a4efe9e8405cc0a977e) - cf. [Project details](https://www.notion.so/ghost/Filter-by-email-disabled-2a73f5da5e8b46bcaacb944bd98e0674?pvs=4) --- .../components/members/filters/subscribed.js | 185 ++++++------------ ghost/admin/app/services/feature.js | 1 - .../tests/acceptance/members/filter-test.js | 154 ++++++++++----- ghost/core/core/shared/labs.js | 1 - .../admin/__snapshots__/settings.test.js.snap | 2 +- 5 files changed, 167 insertions(+), 176 deletions(-) diff --git a/ghost/admin/app/components/members/filters/subscribed.js b/ghost/admin/app/components/members/filters/subscribed.js index f825b362316..9a4d143e841 100644 --- a/ghost/admin/app/components/members/filters/subscribed.js +++ b/ghost/admin/app/components/members/filters/subscribed.js @@ -1,126 +1,54 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; -export const SUBSCRIBED_FILTER = ({newsletters, feature, group}) => { - if (feature.filterEmailDisabled) { - return { - label: newsletters.length > 1 ? 'All newsletters' : 'Newsletter subscription', - name: 'subscribed', - columnLabel: 'Subscribed', - relationOptions: MATCH_RELATION_OPTIONS, - valueType: 'options', - group: newsletters.length > 1 ? 'Newsletters' : group, - // Only show the filter for multiple newsletters if feature flag is enabled - feature: newsletters.length > 1 ? 'filterEmailDisabled' : undefined, - buildNqlFilter: (flt) => { - const relation = flt.relation; - const value = flt.value; - - if (value === 'email-disabled') { - if (relation === 'is') { - return '(email_disabled:1)'; - } - return '(email_disabled:0)'; - } +export const SUBSCRIBED_FILTER = ({newsletters, group}) => { + return { + label: newsletters.length > 1 ? 'All newsletters' : 'Newsletter subscription', + name: 'subscribed', + columnLabel: 'Subscribed', + relationOptions: MATCH_RELATION_OPTIONS, + valueType: 'options', + group: newsletters.length > 1 ? 'Newsletters' : group, + buildNqlFilter: (flt) => { + const relation = flt.relation; + const value = flt.value; + if (value === 'email-disabled') { if (relation === 'is') { - if (value === 'subscribed') { - return '(subscribed:true+email_disabled:0)'; - } - return '(subscribed:false+email_disabled:0)'; + return '(email_disabled:1)'; } + return '(email_disabled:0)'; + } - // relation === 'is-not' + if (relation === 'is') { if (value === 'subscribed') { - return '(subscribed:false,email_disabled:1)'; + return '(subscribed:true+email_disabled:0)'; } - return '(subscribed:true,email_disabled:1)'; - }, - parseNqlFilter: (flt) => { - const comparator = flt.$and || flt.$or; // $or for legacy filter backwards compatibility + return '(subscribed:false+email_disabled:0)'; + } - if (!comparator || comparator.length !== 2) { - const filter = flt; - if (filter && filter.email_disabled !== undefined) { - if (filter.email_disabled) { - return { - value: 'email-disabled', - relation: 'is' - }; - } + // relation === 'is-not' + if (value === 'subscribed') { + return '(subscribed:false,email_disabled:1)'; + } + return '(subscribed:true,email_disabled:1)'; + }, + parseNqlFilter: (flt) => { + const comparator = flt.$and || flt.$or; // $or for legacy filter backwards compatibility + + if (!comparator || comparator.length !== 2) { + const filter = flt; + if (filter && filter.email_disabled !== undefined) { + if (filter.email_disabled) { return { value: 'email-disabled', - relation: 'is-not' + relation: 'is' }; } - return; - } - - if (comparator[0].subscribed === undefined || comparator[1].email_disabled === undefined) { - return; - } - - const usedOr = flt.$or !== undefined; - const subscribed = comparator[0].subscribed; - - if (usedOr) { - // Is not return { - value: !subscribed ? 'subscribed' : 'unsubscribed', + value: 'email-disabled', relation: 'is-not' }; } - - return { - value: subscribed ? 'subscribed' : 'unsubscribed', - relation: 'is' - }; - }, - options: [ - {label: newsletters.length > 1 ? 'Subscribed to at least one' : 'Subscribed', name: 'subscribed'}, - {label: newsletters.length > 1 ? 'Unsubscribed from all' : 'Unsubscribed', name: 'unsubscribed'}, - {label: 'Email disabled', name: 'email-disabled'} - ], - getColumnValue: (member) => { - if (member.emailSuppression && member.emailSuppression.suppressed) { - return { - text: 'Email disabled' - }; - } - - return member.newsletters.length > 0 ? { - text: 'Subscribed' - } : { - text: 'Unsubscribed' - }; - } - }; - } - - if (newsletters.length > 1) { - // Disable - // Only show the filter for multiple newsletters if feature flag is enabled - return []; - } - - return { - label: 'Newsletter subscription', - name: 'subscribed', - columnLabel: 'Subscribed', - relationOptions: MATCH_RELATION_OPTIONS, - valueType: 'options', - group: group, - buildNqlFilter: (flt) => { - const relation = flt.relation; - const value = flt.value; - - return (relation === 'is' && value === 'true') || (relation === 'is-not' && value === 'false') - ? '(subscribed:true+email_disabled:0)' - : '(subscribed:false,email_disabled:1)'; - }, - parseNqlFilter: (flt) => { - const comparator = flt.$and || flt.$or; - - if (!comparator || comparator.length !== 2) { return; } @@ -128,31 +56,44 @@ export const SUBSCRIBED_FILTER = ({newsletters, feature, group}) => { return; } + const usedOr = flt.$or !== undefined; const subscribed = comparator[0].subscribed; + if (usedOr) { + // Is not + return { + value: !subscribed ? 'subscribed' : 'unsubscribed', + relation: 'is-not' + }; + } + return { - value: subscribed ? 'true' : 'false', + value: subscribed ? 'subscribed' : 'unsubscribed', relation: 'is' }; }, options: [ - {label: 'Subscribed', name: 'true'}, - {label: 'Unsubscribed', name: 'false'} + {label: newsletters.length > 1 ? 'Subscribed to at least one' : 'Subscribed', name: 'subscribed'}, + {label: newsletters.length > 1 ? 'Unsubscribed from all' : 'Unsubscribed', name: 'unsubscribed'}, + {label: 'Email disabled', name: 'email-disabled'} ], - getColumnValue: (member, flt) => { - const relation = flt.relation; - const value = flt.value; + getColumnValue: (member) => { + if (member.emailSuppression && member.emailSuppression.suppressed) { + return { + text: 'Email disabled' + }; + } - return { - text: (relation === 'is' && value === 'true') || (relation === 'is-not' && value === 'false') - ? 'Subscribed' - : 'Unsubscribed' + return member.newsletters.length > 0 ? { + text: 'Subscribed' + } : { + text: 'Unsubscribed' }; } }; }; -export const NEWSLETTERS_FILTERS = ({newsletters, group, feature}) => { +export const NEWSLETTERS_FILTERS = ({newsletters, group}) => { if (newsletters.length <= 1) { return []; } @@ -210,12 +151,10 @@ export const NEWSLETTERS_FILTERS = ({newsletters, group, feature}) => { const relation = flt.relation; const value = flt.value; - if (feature.filterEmailDisabled) { - if (member.emailSuppression && member.emailSuppression.suppressed) { - return { - text: 'Email disabled' - }; - } + if (member.emailSuppression && member.emailSuppression.suppressed) { + return { + text: 'Email disabled' + }; } return { diff --git a/ghost/admin/app/services/feature.js b/ghost/admin/app/services/feature.js index d16cdff79b9..31a411a7633 100644 --- a/ghost/admin/app/services/feature.js +++ b/ghost/admin/app/services/feature.js @@ -76,7 +76,6 @@ export default class FeatureService extends Service { @feature('tipsAndDonations') tipsAndDonations; @feature('recommendations') recommendations; @feature('lexicalIndicators') lexicalIndicators; - @feature('filterEmailDisabled') filterEmailDisabled; @feature('adminXDemo') adminXDemo; @feature('ActivityPub') ActivityPub; @feature('internalLinking') internalLinking; diff --git a/ghost/admin/tests/acceptance/members/filter-test.js b/ghost/admin/tests/acceptance/members/filter-test.js index 5bf2585b7eb..cf9f3a1e49b 100644 --- a/ghost/admin/tests/acceptance/members/filter-test.js +++ b/ghost/admin/tests/acceptance/members/filter-test.js @@ -197,99 +197,153 @@ describe('Acceptance: Members filtering', function () { expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows').to.equal(1); }); - it('can filter by specific newsletter subscription', async function () { - // add some members to filters - const newsletter = this.server.create('newsletter', {status: 'active', slug: 'test-newsletter'}); - this.server.createList('newsletter', 4); - this.server.createList('tier', 4); - this.server.createList('member', 4, {subscribed: false}); + it('can filter by newsletter subscription when there is only one newsletter', async function () { + // Create a single newsletter + this.server.createList('newsletter', 1); + // Add some members to filter + this.server.createList('member', 3, {subscribed: true, email_disabled: 0}); + this.server.createList('member', 4, {subscribed: false, email_disabled: 0}); + this.server.createList('member', 1, {subscribed: true, email_disabled: 1}); await visit('/members'); expect(findAll('[data-test-list="members-list-item"]').length, '# of initial member rows') + .to.equal(8); + + await click('[data-test-button="members-filter-actions"]'); + + const filterSelector = `[data-test-members-filter="0"]`; + + await fillIn(`${filterSelector} [data-test-select="members-filter"]`, 'subscribed'); + + // has the right operators + const operatorOptions = findAll(`${filterSelector} [data-test-select="members-filter-operator"] option`); + expect(operatorOptions).to.have.length(2); + expect(operatorOptions[0]).to.have.value('is'); + expect(operatorOptions[1]).to.have.value('is-not'); + + // has the right values + const valueOptions = findAll(`${filterSelector} [data-test-select="members-filter-value"] option`); + expect(valueOptions).to.have.length(3); + expect(valueOptions[0]).to.have.value('subscribed'); + expect(valueOptions[1]).to.have.value('unsubscribed'); + expect(valueOptions[2]).to.have.value('email-disabled'); + + // applies default filter subscribed immediately + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - subscribed') + .to.equal(3); + + // can change filter to unsubscribed + await fillIn(`${filterSelector} [data-test-select="members-filter-value"]`, 'unsubscribed'); + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - unsubscribed') .to.equal(4); + expect(find('[data-test-table-column="subscribed"]')).to.exist; + + // can change filter to email-disabled + await fillIn(`${filterSelector} [data-test-select="members-filter-value"]`, 'email-disabled'); + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - email-disabled') + .to.equal(1); + expect(find('[data-test-table-column="subscribed"]')).to.exist; + + // can delete filter + await click('[data-test-delete-members-filter="0"]'); + + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows after delete') + .to.equal(8); + // Can set filter to 'subscribed' by path + await visit('/'); + await visit('/members?filter=' + encodeURIComponent('(subscribed:true+email_disabled:0)')); + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - subscribed - from URL') + .to.equal(3); await click('[data-test-button="members-filter-actions"]'); - // make sure newsletters are in the filter dropdown - const newslettersCount = this.server.schema.newsletters.all().models.length; - let options = this.element.querySelectorAll('option'); - let matchingOptions = [...options].filter(option => option.value.includes('newsletters.slug')); - expect(matchingOptions).to.have.length(newslettersCount); + expect(find(`${filterSelector} [data-test-select="members-filter-value"]`)).to.have.value('subscribed'); + // Can set filter to 'unsubscribed' by path await visit('/'); - await visit('/members'); - // add some members with tiers - const tier = this.server.create('tier'); - const member = this.server.create('member', {tiers: [tier], subscribed: true}); - member.update({newsletters: [newsletter]}); - this.server.createList('member', 4, {subscribed: false}); + await visit('/members?filter=' + encodeURIComponent('(subscribed:false+email_disabled:0)')); + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - unsubscribed - from URL') + .to.equal(4); + await click('[data-test-button="members-filter-actions"]'); + expect(find(`${filterSelector} [data-test-select="members-filter-value"]`)).to.have.value('unsubscribed'); - await visit('/members?filter=' + encodeURIComponent(`newsletters.slug:${newsletter.slug}`)); - // only 1 member is subscribed so we should only see 1 row - expect(findAll('[data-test-list="members-list-item"]').length, '# of initial member rows') + // Can set filter to 'email-disabled' by path + await visit('/'); + await visit('/members?filter=' + encodeURIComponent('(email_disabled:1)')); + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - email-disabled - from URL') .to.equal(1); + await click('[data-test-button="members-filter-actions"]'); + expect(find(`${filterSelector} [data-test-select="members-filter-value"]`)).to.have.value('email-disabled'); }); - it('can filter by newsletter subscription', async function () { - // add some members to filter - this.server.createList('member', 3, {subscribed: true, email_disabled: 0}); + it('can filter by specific newsletter subscription when there are multiple newsletters', async function () { + // Create: + // - 1 subscribed member to newsletter + // - 1 subscribed member to newsletter with email disabled + // - 4 unsubscribed members + const newsletter = this.server.create('newsletter', {status: 'active', slug: 'test-newsletter'}); + const tier = this.server.create('tier'); + + const subscribedMember = this.server.create('member', {tiers: [tier], subscribed: true, email_disabled: 0}); + subscribedMember.update({newsletters: [newsletter]}); + + const emailDisabledMember = this.server.create('member', {tiers: [tier], subscribed: true, email_disabled: 1}); + emailDisabledMember.update({newsletters: [newsletter]}); + this.server.createList('member', 4, {subscribed: false, email_disabled: 0}); - this.server.createList('member', 1, {subscribed: true, email_disabled: 1}); + // Test initial member count await visit('/members'); - expect(findAll('[data-test-list="members-list-item"]').length, '# of initial member rows') - .to.equal(8); + .to.equal(6); + // Test newsletters options are in the filter dropdown await click('[data-test-button="members-filter-actions"]'); + const newslettersCount = this.server.schema.newsletters.all().models.length; + let options = this.element.querySelectorAll('option'); + let matchingOptions = [...options].filter(option => option.value.includes('newsletters.slug')); + expect(matchingOptions).to.have.length(newslettersCount); const filterSelector = `[data-test-members-filter="0"]`; - await fillIn(`${filterSelector} [data-test-select="members-filter"]`, 'subscribed'); + // Select first newsletter + await fillIn(`${filterSelector} [data-test-select="members-filter"]`, `newsletters.slug:${newsletter.slug}`); - // has the right operators + // Test that the filter has the right operators const operatorOptions = findAll(`${filterSelector} [data-test-select="members-filter-operator"] option`); - expect(operatorOptions).to.have.length(2); expect(operatorOptions[0]).to.have.value('is'); expect(operatorOptions[1]).to.have.value('is-not'); - // has the right values + // Test that the filter has the right operators const valueOptions = findAll(`${filterSelector} [data-test-select="members-filter-value"] option`); - expect(valueOptions).to.have.length(2); expect(valueOptions[0]).to.have.value('true'); expect(valueOptions[1]).to.have.value('false'); - // applies default filter immediately - expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - true') - .to.equal(3); + // applies default filter subscribed immediately, and only count subscribed members without email disabled + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - subscribed') + .to.equal(1); - // can change filter + // can change filter to unsubscribed await fillIn(`${filterSelector} [data-test-select="members-filter-value"]`, 'false'); - expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - false') + expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - unsubscribed') .to.equal(5); - expect(find('[data-test-table-column="subscribed"]')).to.exist; // can delete filter await click('[data-test-delete-members-filter="0"]'); - expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows after delete') - .to.equal(8); + .to.equal(6); - // Can set filter by path + // Can filter members subscribed to that newsletter by path await visit('/'); - await visit('/members?filter=' + encodeURIComponent('(subscribed:true+email_disabled:0)')); - expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - true - from URL') - .to.equal(3); - await click('[data-test-button="members-filter-actions"]'); - expect(find(`${filterSelector} [data-test-select="members-filter-value"]`)).to.have.value('true'); + await visit('/members?filter=' + encodeURIComponent(`newsletters.slug:${newsletter.slug}+email_disabled:0`)); + expect(findAll('[data-test-list="members-list-item"]').length, '# of initial member rows') + .to.equal(1); - // Can set filter by path + // Can filter members unsubscribed to that newsletter by path await visit('/'); - await visit('/members?filter=' + encodeURIComponent('(subscribed:false,email_disabled:1)')); - expect(findAll('[data-test-list="members-list-item"]').length, '# of filtered member rows - false - from URL') + await visit('/members?filter=' + encodeURIComponent(`newsletters.slug:-${newsletter.slug},email_disabled:1`)); + expect(findAll('[data-test-list="members-list-item"]').length, '# of initial member rows') .to.equal(5); - await click('[data-test-button="members-filter-actions"]'); - expect(find(`${filterSelector} [data-test-select="members-filter-value"]`)).to.have.value('false'); }); it('can filter by member status', async function () { diff --git a/ghost/core/core/shared/labs.js b/ghost/core/core/shared/labs.js index a439506ee7f..c35874d3faf 100644 --- a/ghost/core/core/shared/labs.js +++ b/ghost/core/core/shared/labs.js @@ -22,7 +22,6 @@ const GA_FEATURES = [ 'signupForm', 'recommendations', 'listUnsubscribeHeader', - 'filterEmailDisabled', 'newEmailAddresses', 'internalLinking' ]; diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap index b1073bc2aa1..8e8e09661d8 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap @@ -1155,7 +1155,7 @@ exports[`Settings API Edit Can edit a setting 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": "4559", + "content-length": "4530", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,