diff --git a/ghost/admin/README.md b/ghost/admin/README.md index 73a49b566b..04fa03e8ed 100644 --- a/ghost/admin/README.md +++ b/ghost/admin/README.md @@ -12,7 +12,7 @@ Run all tests in the browser by running `yarn dev` in the Ghost monorepo and vis --- -Tip: You can use `await this.pauseTest()` in your tests to temporarily pause the execution of browser tests. Use the browser console to inspect and debug the DOM, then resume tests by running `resumeTest()` directly in the browser console ([docs](https://guides.emberjs.com/v3.28.0/testing/testing-application/#toc_debugging-your-tests)) +Tip: You can use `this.timeout(0); await this.pauseTest();` in your tests to temporarily pause the execution of browser tests. Use the browser console to inspect and debug the DOM, then resume tests by running `resumeTest()` directly in the browser console ([docs](https://guides.emberjs.com/v3.28.0/testing/testing-application/#toc_debugging-your-tests)) ### Running tests in the CLI diff --git a/ghost/admin/app/components/members/filter.js b/ghost/admin/app/components/members/filter.js index 6993d1c4e7..46da839176 100644 --- a/ghost/admin/app/components/members/filter.js +++ b/ghost/admin/app/components/members/filter.js @@ -137,6 +137,10 @@ class Filter { return this.properties.options ?? []; } + get group() { + return this.properties.group; + } + get isValid() { if (Array.isArray(this.value)) { return !!this.value.length; diff --git a/ghost/admin/app/components/members/filters/created-at.js b/ghost/admin/app/components/members/filters/created-at.js index 9ded43af78..f857771115 100644 --- a/ghost/admin/app/components/members/filters/created-at.js +++ b/ghost/admin/app/components/members/filters/created-at.js @@ -1,8 +1,8 @@ import {DATE_RELATION_OPTIONS} from './relation-options'; export const CREATED_AT_FILTER = { - label: 'Created', - name: 'created_at', - valueType: 'date', + label: 'Created', + name: 'created_at', + valueType: 'date', relationOptions: DATE_RELATION_OPTIONS }; diff --git a/ghost/admin/app/components/members/filters/email-clicked.js b/ghost/admin/app/components/members/filters/email-clicked.js index d24798b197..c78dae0c41 100644 --- a/ghost/admin/app/components/members/filters/email-clicked.js +++ b/ghost/admin/app/components/members/filters/email-clicked.js @@ -1,10 +1,10 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const EMAIL_CLICKED_FILTER = { - label: 'Clicked email', - name: 'clicked_links.post_id', - valueType: 'string', - resource: 'email', + label: 'Clicked email', + name: 'clicked_links.post_id', + valueType: 'string', + resource: 'email', relationOptions: MATCH_RELATION_OPTIONS, columnLabel: 'Clicked email', setting: 'emailTrackClicks', diff --git a/ghost/admin/app/components/members/filters/email-count.js b/ghost/admin/app/components/members/filters/email-count.js index ef8396c574..03a0a4242f 100644 --- a/ghost/admin/app/components/members/filters/email-count.js +++ b/ghost/admin/app/components/members/filters/email-count.js @@ -2,10 +2,10 @@ import {NUMBER_RELATION_OPTIONS} from './relation-options'; import {formatNumber} from 'ghost-admin/helpers/format-number'; export const EMAIL_COUNT_FILTER = { - label: 'Emails sent (all time)', - name: 'email_count', - columnLabel: 'Email count', - valueType: 'number', + label: 'Emails sent (all time)', + name: 'email_count', + columnLabel: 'Email count', + valueType: 'number', relationOptions: NUMBER_RELATION_OPTIONS, getColumnValue: (member) => { return { diff --git a/ghost/admin/app/components/members/filters/email-open-rate.js b/ghost/admin/app/components/members/filters/email-open-rate.js index 5990199a4d..6361239b05 100644 --- a/ghost/admin/app/components/members/filters/email-open-rate.js +++ b/ghost/admin/app/components/members/filters/email-open-rate.js @@ -1,9 +1,9 @@ import {NUMBER_RELATION_OPTIONS} from './relation-options'; export const EMAIL_OPEN_RATE_FILTER = { - label: 'Open rate (all time)', - name: 'email_open_rate', - valueType: 'number', + label: 'Open rate (all time)', + name: 'email_open_rate', + valueType: 'number', setting: 'emailTrackOpens', relationOptions: NUMBER_RELATION_OPTIONS }; diff --git a/ghost/admin/app/components/members/filters/email-opened-count.js b/ghost/admin/app/components/members/filters/email-opened-count.js index ff4d2e3363..dece4042ad 100644 --- a/ghost/admin/app/components/members/filters/email-opened-count.js +++ b/ghost/admin/app/components/members/filters/email-opened-count.js @@ -2,10 +2,10 @@ import {NUMBER_RELATION_OPTIONS} from './relation-options'; import {formatNumber} from 'ghost-admin/helpers/format-number'; export const EMAIL_OPENED_COUNT_FILTER = { - label: 'Emails opened (all time)', - name: 'email_opened_count', - columnLabel: 'Email opened count', - valueType: 'number', + label: 'Emails opened (all time)', + name: 'email_opened_count', + columnLabel: 'Email opened count', + valueType: 'number', relationOptions: NUMBER_RELATION_OPTIONS, getColumnValue: (member) => { return { diff --git a/ghost/admin/app/components/members/filters/email-opened.js b/ghost/admin/app/components/members/filters/email-opened.js index 5f284e16a5..d9535523aa 100644 --- a/ghost/admin/app/components/members/filters/email-opened.js +++ b/ghost/admin/app/components/members/filters/email-opened.js @@ -1,10 +1,10 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const EMAIL_OPENED_FILTER = { - label: 'Opened email', - name: 'opened_emails.post_id', - valueType: 'string', - resource: 'email', + label: 'Opened email', + name: 'opened_emails.post_id', + valueType: 'string', + resource: 'email', relationOptions: MATCH_RELATION_OPTIONS, columnLabel: 'Opened email', setting: 'emailTrackOpens', diff --git a/ghost/admin/app/components/members/filters/email.js b/ghost/admin/app/components/members/filters/email.js index 3645374c34..1aa094a395 100644 --- a/ghost/admin/app/components/members/filters/email.js +++ b/ghost/admin/app/components/members/filters/email.js @@ -1,8 +1,8 @@ import {CONTAINS_RELATION_OPTIONS} from './relation-options'; export const EMAIL_FILTER = { - label: 'Email', + label: 'Email', name: 'email', - valueType: 'string', + valueType: 'string', relationOptions: CONTAINS_RELATION_OPTIONS }; diff --git a/ghost/admin/app/components/members/filters/label.js b/ghost/admin/app/components/members/filters/label.js index 7538d60df5..857845a2db 100644 --- a/ghost/admin/app/components/members/filters/label.js +++ b/ghost/admin/app/components/members/filters/label.js @@ -1,10 +1,10 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const LABEL_FILTER = { - label: 'Label', - name: 'label', - valueType: 'array', - columnLabel: 'Label', + label: 'Label', + name: 'label', + valueType: 'array', + columnLabel: 'Label', relationOptions: MATCH_RELATION_OPTIONS, getColumnValue: (member) => { return { diff --git a/ghost/admin/app/components/members/filters/last-seen.js b/ghost/admin/app/components/members/filters/last-seen.js index 22eb950daf..d0667dbda3 100644 --- a/ghost/admin/app/components/members/filters/last-seen.js +++ b/ghost/admin/app/components/members/filters/last-seen.js @@ -2,10 +2,10 @@ import {DATE_RELATION_OPTIONS} from './relation-options'; import {getDateColumnValue} from './columns/date-column'; export const LAST_SEEN_FILTER = { - label: 'Last seen', - name: 'last_seen_at', - valueType: 'date', - columnLabel: 'Last seen at', + label: 'Last seen', + name: 'last_seen_at', + valueType: 'date', + columnLabel: 'Last seen at', relationOptions: DATE_RELATION_OPTIONS, getColumnValue: (member, filter) => { return getDateColumnValue(member.lastSeenAtUTC, filter); diff --git a/ghost/admin/app/components/members/filters/name.js b/ghost/admin/app/components/members/filters/name.js index 875d50e5fa..c5571d5567 100644 --- a/ghost/admin/app/components/members/filters/name.js +++ b/ghost/admin/app/components/members/filters/name.js @@ -1,8 +1,8 @@ import {CONTAINS_RELATION_OPTIONS} from './relation-options'; export const NAME_FILTER = { - label: 'Name', - name: 'name', - valueType: 'string', + label: 'Name', + name: 'name', + valueType: 'string', relationOptions: CONTAINS_RELATION_OPTIONS }; diff --git a/ghost/admin/app/components/members/filters/offers.js b/ghost/admin/app/components/members/filters/offers.js index 0039f04c19..854c46e4e7 100644 --- a/ghost/admin/app/components/members/filters/offers.js +++ b/ghost/admin/app/components/members/filters/offers.js @@ -1,7 +1,7 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const OFFERS_FILTER = { - label: 'Offers', + label: 'Offers', name: 'offer_redemptions', group: 'Subscription', relationOptions: MATCH_RELATION_OPTIONS, diff --git a/ghost/admin/app/components/members/filters/signup-attribution.js b/ghost/admin/app/components/members/filters/signup-attribution.js index 49a396e071..6bcf2b2762 100644 --- a/ghost/admin/app/components/members/filters/signup-attribution.js +++ b/ghost/admin/app/components/members/filters/signup-attribution.js @@ -1,10 +1,10 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const SIGNUP_ATTRIBUTION_FILTER = { - label: 'Signed up on post/page', - name: 'signup', - valueType: 'string', - resource: 'post', + label: 'Signed up on post/page', + name: 'signup', + valueType: 'string', + resource: 'post', relationOptions: MATCH_RELATION_OPTIONS, columnLabel: 'Signed up on', setting: 'membersTrackSources', diff --git a/ghost/admin/app/components/members/filters/status.js b/ghost/admin/app/components/members/filters/status.js index e6d31dd697..08cbbef1a9 100644 --- a/ghost/admin/app/components/members/filters/status.js +++ b/ghost/admin/app/components/members/filters/status.js @@ -1,8 +1,8 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const STATUS_FILTER = { - label: 'Member status', - name: 'status', + label: 'Member status', + name: 'status', relationOptions: MATCH_RELATION_OPTIONS, valueType: 'options', options: [ diff --git a/ghost/admin/app/components/members/filters/subscription-attribution.js b/ghost/admin/app/components/members/filters/subscription-attribution.js index 84688dac5d..373b51c0b3 100644 --- a/ghost/admin/app/components/members/filters/subscription-attribution.js +++ b/ghost/admin/app/components/members/filters/subscription-attribution.js @@ -1,10 +1,10 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const SUBSCRIPTION_ATTRIBUTION_FILTER = { - label: 'Subscription started on post/page', - name: 'conversion', - valueType: 'string', - resource: 'post', + label: 'Subscription started on post/page', + name: 'conversion', + valueType: 'string', + resource: 'post', relationOptions: MATCH_RELATION_OPTIONS, columnLabel: 'Subscription started on', setting: 'membersTrackSources', diff --git a/ghost/admin/app/components/members/filters/tier.js b/ghost/admin/app/components/members/filters/tier.js index b219d0fc4f..bbd1c00e2d 100644 --- a/ghost/admin/app/components/members/filters/tier.js +++ b/ghost/admin/app/components/members/filters/tier.js @@ -1,10 +1,10 @@ import {MATCH_RELATION_OPTIONS} from './relation-options'; export const TIER_FILTER = { - label: 'Membership tier', - name: 'tier_id', - valueType: 'array', - columnLabel: 'Membership tier', + label: 'Membership tier', + name: 'tier_id', + valueType: 'array', + columnLabel: 'Membership tier', relationOptions: MATCH_RELATION_OPTIONS, getColumnValue: (member) => { return { diff --git a/ghost/admin/app/controllers/members.js b/ghost/admin/app/controllers/members.js index 1270b5a06a..9279418d23 100644 --- a/ghost/admin/app/controllers/members.js +++ b/ghost/admin/app/controllers/members.js @@ -209,8 +209,44 @@ export default class MembersController extends Controller { return uniqueColumns.splice(0, 2); // Maximum 2 columns } - get isMultiFiltered() { - return this.isFiltered && this.filters.length >= 2; + /* Due to a limitation with NQL when multiple member filters are used in combination, we currently have a safeguard around member bulk deletion. + * Member bulk deletion is not permitted when: + * 1) Multiple newsletters exist, and 2 or more newsletter filters are in use + * 2) If any of the following Stripe filters are used, even once: + * - Billing period + * - Stripe subscription status + * - Paid start date + * - Next billing date + * - Subscription started on post/page + * - Offers + * + * See issue https://linear.app/tryghost/issue/ENG-1484 for more context + */ + get isBulkDeletePermitted() { + if (!this.isFiltered) { + return false; + } + + const newsletterFilters = this.filters.filter(f => f.group === 'Newsletters'); + + if (newsletterFilters && newsletterFilters.length >= 2) { + return false; + } + + const stripeFilters = this.filters.filter(f => [ + 'subscriptions.plan_interval', + 'subscriptions.status', + 'subscriptions.start_date', + 'subscriptions.current_period_end', + 'conversion', + 'offer_redemptions' + ].includes(f.type)); + + if (stripeFilters && stripeFilters.length >= 1) { + return false; + } + + return true; } includeTierQuery() { diff --git a/ghost/admin/app/templates/members.hbs b/ghost/admin/app/templates/members.hbs index d72c91a904..c965f8389b 100644 --- a/ghost/admin/app/templates/members.hbs +++ b/ghost/admin/app/templates/members.hbs @@ -104,14 +104,14 @@ {{/if}} - {{#unless this.isMultiFiltered}} + {{#if this.isBulkDeletePermitted}}
    • - {{/unless}} + {{/if}} {{/if}} diff --git a/ghost/admin/mirage/factories/newsletter.js b/ghost/admin/mirage/factories/newsletter.js new file mode 100644 index 0000000000..b6b0ac2ad4 --- /dev/null +++ b/ghost/admin/mirage/factories/newsletter.js @@ -0,0 +1,10 @@ +import moment from 'moment-timezone'; +import {Factory} from 'miragejs'; + +export default Factory.extend({ + name(i) { return `Newsletter ${i}`; }, + slug(i) { return `newsletter-${i}`; }, + status() { return 'active'; }, + createdAt() { return moment.utc().toISOString(); }, + updatedAt() { return moment.utc().toISOString(); } +}); diff --git a/ghost/admin/tests/acceptance/members-test.js b/ghost/admin/tests/acceptance/members-test.js index b59f9514ca..ba9f0c9ef8 100644 --- a/ghost/admin/tests/acceptance/members-test.js +++ b/ghost/admin/tests/acceptance/members-test.js @@ -143,51 +143,133 @@ describe('Acceptance: Members', function () { .to.equal('example@domain.com'); }); - /* NOTE: Bulk deletion is disabled temporarily when multiple filters are applied, due to a NQL limitation. - * Delete this test once we have fixed the root NQL limitation. - * See https://linear.app/tryghost/issue/ONC-203 + /* Due to a limitation with NQL when multiple member filters are used in combination, we currently have a safeguard around member bulk deletion. + * Member bulk deletion is not permitted when: + * 1) Multiple newsletters exist, and 2 or more newsletter filters are in use + * 2) If any of the following Stripe filters are used, even once: + * - Billing period + * - Stripe subscription status + * - Paid start date + * - Next billing date + * - Subscription started on post/page + * - Offers + * + * See code: ghost/admin/app/controllers/members.js:isBulkDeletePermitted + * See issue https://linear.app/tryghost/issue/ENG-1484 for more context + * + * TODO: delete this block of tests once the guardrail has been removed */ - it('cannot bulk delete members if more than 1 filter is selected', async function () { - // Members with label - const labelOne = this.server.create('label'); - const labelTwo = this.server.create('label'); - this.server.createList('member', 2, {labels: [labelOne]}); - this.server.createList('member', 2, {labels: [labelOne, labelTwo]}); + describe('[Temp] Guardrail against bulk deletion', function () { + it('cannot bulk delete members if more than 1 newsletter filter is used', async function () { + // Create two newsletters and members subscribed to 1 or 2 newsletters + const newsletterOne = this.server.create('newsletter'); + const newsletterTwo = this.server.create('newsletter'); + this.server.createList('member', 2).forEach(member => member.update({newsletters: [newsletterOne], email_disabled: 0})); + this.server.createList('member', 2).forEach(member => member.update({newsletters: [newsletterOne, newsletterTwo], email_disabled: 0})); - await visit('/members'); - expect(findAll('[data-test-member]').length).to.equal(4); + await visit('/members'); + expect(findAll('[data-test-member]').length).to.equal(4); - // The delete button should not be visible by default - await click('[data-test-button="members-actions"]'); - expect(find('[data-test-button="delete-selected"]')).to.not.exist; + // The delete button should not be visible by default + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; - // Apply a single filter - await click('[data-test-button="members-filter-actions"]'); - await fillIn('[data-test-members-filter="0"] [data-test-select="members-filter"]', 'label'); - await click('.gh-member-label-input input'); - await click(`[data-test-label-filter="${labelOne.name}"]`); - await click(`[data-test-button="members-apply-filter"]`); + // Apply a first filter + await click('[data-test-button="members-filter-actions"]'); + await fillIn('[data-test-members-filter="0"] [data-test-select="members-filter"]', `newsletters.slug:${newsletterOne.slug}`); + await click(`[data-test-button="members-apply-filter"]`); - expect(findAll('[data-test-member]').length).to.equal(4); - expect(currentURL()).to.equal(`/members?filter=label%3A%5B${labelOne.slug}%5D`); + expect(findAll('[data-test-member]').length).to.equal(4); + expect(currentURL()).to.equal(`/members?filter=(newsletters.slug%3A${newsletterOne.slug}%2Bemail_disabled%3A0)`); - await click('[data-test-button="members-actions"]'); - expect(find('[data-test-button="delete-selected"]')).to.exist; + // Bulk deletion is permitted + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.exist; - // Apply a second filter - await click('[data-test-button="members-filter-actions"]'); - await click('[data-test-button="add-members-filter"]'); + // Apply a second filter + await click('[data-test-button="members-filter-actions"]'); + await click('[data-test-button="add-members-filter"]'); + await fillIn('[data-test-members-filter="1"] [data-test-select="members-filter"]', `newsletters.slug:${newsletterTwo.slug}`); + await click(`[data-test-button="members-apply-filter"]`); - await fillIn('[data-test-members-filter="1"] [data-test-select="members-filter"]', 'label'); - await click('[data-test-members-filter="1"] .gh-member-label-input input'); - await click(`[data-test-members-filter="1"] [data-test-label-filter="${labelTwo.name}"]`); - await click(`[data-test-button="members-apply-filter"]`); + expect(findAll('[data-test-member]').length).to.equal(2); + expect(currentURL()).to.equal(`/members?filter=(newsletters.slug%3A${newsletterOne.slug}%2Bemail_disabled%3A0)%2B(newsletters.slug%3A${newsletterTwo.slug}%2Bemail_disabled%3A0)`); - expect(findAll('[data-test-member]').length).to.equal(2); - expect(currentURL()).to.equal(`/members?filter=label%3A%5B${labelOne.slug}%5D%2Blabel%3A%5B${labelTwo.slug}%5D`); + // Bulk deletion is not permitted anymore + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + }); - await click('[data-test-button="members-actions"]'); - expect(find('[data-test-button="delete-selected"]')).to.not.exist; + it('can bulk delete members if a non-Stripe subscription filter is in use (member tier, status)', async function () { + const tier = this.server.create('tier', {id: 'qwerty123456789'}); + this.server.createList('member', 2, {status: 'free'}); + this.server.createList('member', 2, {status: 'paid', tiers: [tier]}); + + await visit('/members'); + expect(findAll('[data-test-member]').length).to.equal(4); + + // The delete button should not be visible by default + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + + // 1) Membership tier filter: permitted + await visit(`/members?filter=tier_id:[${tier.id}]`); + expect(findAll('[data-test-member]').length).to.equal(2); + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.exist; + + // 2) Member status filter: permitted + await visit('/members?filter=status%3Afree'); + expect(findAll('[data-test-member]').length).to.equal(2); + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.exist; + }); + + it('cannot bulk delete members if a Stripe subscription filter is in use', async function () { + // Create free and paid members + const tier = this.server.create('tier'); + const offer = this.server.create('offer', {tier: {id: tier.id}, createdAt: moment.utc().subtract(1, 'day').valueOf()}); + this.server.createList('member', 2, {status: 'free'}); + this.server.createList('member', 2, {status: 'paid'}).forEach(member => this.server.create('subscription', {member, planInterval: 'month', status: 'active', start_date: '2000-01-01T00:00:00.000Z', current_period_end: '2000-02-01T00:00:00.000Z', offer: offer, tier: tier})); + this.server.createList('member', 2, {status: 'paid'}).forEach(member => this.server.create('subscription', {member, planInterval: 'year', status: 'active'})); + + await visit('/members'); + expect(findAll('[data-test-member]').length).to.equal(6); + + // The delete button should not be visible by default + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + + // 1) Stripe billing period filter: not permitted + await visit('/members?filter=subscriptions.plan_interval%3Amonth'); + expect(findAll('[data-test-member]').length).to.equal(2); + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + + // 2) Stripe subscription status filter: not permitted + await visit('/members?filter=subscriptions.status%3Aactive'); + expect(findAll('[data-test-member]').length).to.equal(4); + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + + // 3) Stripe paid start date filter: not permitted + await visit(`/members?filter=subscriptions.start_date%3A>'1999-01-01%2005%3A59%3A59'`); + expect(findAll('[data-test-member]').length).to.equal(2); + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + + // 4) Next billing date filter: not permitted + await visit(`/members?filter=subscriptions.current_period_end%3A>'2000-01-01%2005%3A59%3A59'`); + expect(findAll('[data-test-member]').length).to.equal(2); + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + + // 5) Offers redeemed filter: not permitted + await visit('/members?filter=' + encodeURIComponent(`offer_redemptions:'${offer.id}'`)); + expect(findAll('[data-test-member]').length).to.equal(2); + await click('[data-test-button="members-actions"]'); + expect(find('[data-test-button="delete-selected"]')).to.not.exist; + }); }); it('can bulk delete members', async function () {