🎨 Removed member bulk deletion safeguard from safe queries (#20747)

fixes https://linear.app/tryghost/issue/ENG-1484

- in Ghost release
[v5.89.0](https://github.com/TryGhost/Ghost/releases/tag/v5.89.0), we
have added a safeguard around bulk member deletion, due to a limitation
in NQL for member filters (commit: 2484a77)
- with this change, we limit the safeguard to only the cases we know are
problematic, and remove it for other useful and safe queries
- more precisely, the safeguard is in place only when:
    - Multiple newsletters exist, and the filter contains 2 or more
newsletter filters
    - 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
This commit is contained in:
Sag 2024-08-14 17:48:54 +02:00 committed by GitHub
parent 04b600b0b8
commit e6254bbb93
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
21 changed files with 222 additions and 90 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

@ -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() {

View File

@ -104,14 +104,14 @@
</button>
</li>
{{/if}}
{{#unless this.isMultiFiltered}}
{{#if this.isBulkDeletePermitted}}
<li class="divider"></li>
<li>
<button class="mr2" data-test-button="delete-selected" type="button" {{on "click" this.bulkDelete}}>
<span class="red">Delete selected members ({{this.members.length}})</span>
</button>
</li>
{{/unless}}
{{/if}}
{{/if}}
</GhDropdown>
</span>

View File

@ -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(); }
});

View File

@ -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 () {