From e3db911108a1d0ead300532339d048cc72702bc0 Mon Sep 17 00:00:00 2001
From: Kevin Ansfield <kevin@ghost.org>
Date: Mon, 26 Sep 2022 17:37:35 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20confusing=20error=20stat?=
=?UTF-8?q?e=20when=20publishing=20if=20member=20count=20is=20over=20hosti?=
=?UTF-8?q?ng=20plan=20limit=20(#15473)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
closes https://github.com/TryGhost/Team/issues/1885
- adds limit check for members to the `PublishOptions` class when it's constructed to set a `publishDisabledError` property if the limit check fails
- if `publishOptions.publishDisabledCheck` is present, all publish options in the publish flow are disabled, the underlying error message is shown, and the continue button is removed to prevent filling in everything only to find at the end of the process that publishing fails
- added handling for a `HostLimitError` error from the API when confirming publishing so the proper underlying message is displayed instead of the confusing "Host limit error, cannot edit post" error
- this is a backup measure for any instances where you're under the max members limit when starting the publish flow but are over the limit when you reach the end of the publish flow
---
.../editor/modals/publish-flow/confirm.hbs | 1 +
.../editor/modals/publish-flow/confirm.js | 6 +-
.../editor/modals/publish-flow/options.hbs | 58 +++++++++------
ghost/admin/app/services/members-stats.js | 45 +++--------
ghost/admin/app/utils/publish-options.js | 17 ++++-
.../acceptance/editor/publish-flow-test.js | 74 ++++++++++++++++++-
6 files changed, 140 insertions(+), 61 deletions(-)
diff --git a/ghost/admin/app/components/editor/modals/publish-flow/confirm.hbs b/ghost/admin/app/components/editor/modals/publish-flow/confirm.hbs
index 4006d594d6..e216ebc21d 100644
--- a/ghost/admin/app/components/editor/modals/publish-flow/confirm.hbs
+++ b/ghost/admin/app/components/editor/modals/publish-flow/confirm.hbs
@@ -67,6 +67,7 @@
@class="gh-btn gh-btn-large"
@idleClass="gh-btn-pulse"
@runningClass="gh-btn-green gh-btn-icon"
+ @failureClass="gh-btn-red gh-btn-icon"
data-test-button="confirm-publish"
/>
<button type="button" class="gh-btn gh-btn-link gh-btn-large gh-publish-cta-secondary" {{on "click" @cancel}} data-test-button="back-to-options">Back to settings</button>
diff --git a/ghost/admin/app/components/editor/modals/publish-flow/confirm.js b/ghost/admin/app/components/editor/modals/publish-flow/confirm.js
index 6247e07415..b6e2a68185 100644
--- a/ghost/admin/app/components/editor/modals/publish-flow/confirm.js
+++ b/ghost/admin/app/components/editor/modals/publish-flow/confirm.js
@@ -101,15 +101,19 @@ export default class PublishFlowOptions extends Component {
let errorMessage = '';
+ const payloadError = e?.payload?.errors?.[0];
+
if (isServerUnreachableError(e)) {
errorMessage = 'Unable to connect, please check your internet connection and try again.';
+ } else if (payloadError?.type === 'HostLimitError') {
+ errorMessage = htmlSafe(payloadError.context.replace(/please upgrade/i, '<a href="#/pro">$&</a>'));
} else if (e && isString(e)) {
errorMessage = e;
} else if (e && isArray(e)) {
// This is here because validation errors are returned as an array
// TODO: remove this once validations are fixed
errorMessage = e[0];
- } else if (e?.payload?.errors?.[0].message) {
+ } else if (payloadError?.message) {
errorMessage = e.payload.errors[0].message;
} else {
errorMessage = 'Unknown Error';
diff --git a/ghost/admin/app/components/editor/modals/publish-flow/options.hbs b/ghost/admin/app/components/editor/modals/publish-flow/options.hbs
index 59ad2f737c..40cb88be4e 100644
--- a/ghost/admin/app/components/editor/modals/publish-flow/options.hbs
+++ b/ghost/admin/app/components/editor/modals/publish-flow/options.hbs
@@ -2,13 +2,23 @@
<div class="green">Ready, set, publish.</div>
<div>Share it with the world.</div>
</div>
+
<div class="gh-publish-settings">
<div class="gh-publish-setting" data-test-setting="publish-type">
- {{#if @publishOptions.emailUnavailable}}
- <div class="gh-publish-setting-title" data-test-setting-title>
+ {{#if (or @publishOptions.emailUnavailable @publishOptions.publishDisabledError)}}
+ <div class="gh-publish-setting-title {{if @publishOptions.publishDisabledError "disabled"}}" data-test-setting-title>
{{svg-jar "send-email"}}
<div class="gh-publish-setting-trigger">Publish on site</div>
+
+ {{#if @publishOptions.publishDisabledError}}
+ <span>{{svg-jar "arrow-down" class="icon-expand"}}</span>
+ {{/if}}
</div>
+ {{#if @publishOptions.publishDisabledError}}
+ <p class="gh-box gh-content-box" data-test-publish-type-error="publish-disabled">
+ {{@publishOptions.publishDisabledError}}
+ </p>
+ {{/if}}
{{else}}
<button type="button" class="gh-publish-setting-title" {{on "click" (fn this.toggleSection "publishType")}} data-test-setting-title>
{{svg-jar "send-email"}}
@@ -31,7 +41,21 @@
{{#unless @publishOptions.emailUnavailable}}
<div class="gh-publish-setting" data-test-setting="email-recipients">
- {{#if (not-eq @publishOptions.publishType "publish")}}
+ {{#if (or (eq @publishOptions.publishType "publish") @publishOptions.publishDisabledError)}}
+ <button
+ type="button"
+ class="gh-publish-setting-title disabled"
+ data-test-setting-title
+ >
+ {{svg-jar "member"}}
+ <div class="gh-publish-setting-trigger">
+ Not sent as newsletter
+ </div>
+ <span>
+ {{svg-jar "arrow-down" class="icon-expand"}}
+ </span>
+ </button>
+ {{else}}
<button
type="button"
class="gh-publish-setting-title"
@@ -67,20 +91,6 @@
{{svg-jar "arrow-down" class="icon-expand"}}
</span>
</button>
- {{else}}
- <button
- type="button"
- class="gh-publish-setting-title disabled"
- data-test-setting-title
- >
- {{svg-jar "member"}}
- <div class="gh-publish-setting-trigger">
- Not sent as newsletter
- </div>
- <span>
- {{svg-jar "arrow-down" class="icon-expand"}}
- </span>
- </button>
{{/if}}
{{#liquid-if (eq this.openSection "emailRecipients")}}
<div class="gh-publish-setting-form">
@@ -110,7 +120,7 @@
{{/if}}
<div class="gh-publish-setting last" data-test-setting="publish-at">
- <button type="button" class="gh-publish-setting-title" {{on "click" (fn this.toggleSection "publishAt")}} data-test-setting-title>
+ <button type="button" class="gh-publish-setting-title {{if @publishOptions.publishDisabledError "disabled"}}" {{on "click" (if @publishOptions.publishDisabledError (optional) (fn this.toggleSection "publishAt"))}} data-test-setting-title>
{{svg-jar "clock"}}
<div class="gh-publish-setting-trigger">
<span>
@@ -136,8 +146,10 @@
</div>
-<div class="gh-publish-cta">
- <button type="button" class="gh-btn gh-btn-black gh-btn-large" {{on "click" @confirm}} data-test-button="continue">
- <span>Continue, final review →</span>
- </button>
-</div>
+{{#unless @publishOptions.publishDisabledError}}
+ <div class="gh-publish-cta">
+ <button type="button" class="gh-btn gh-btn-black gh-btn-large" {{on "click" @confirm}} data-test-button="continue">
+ <span>Continue, final review →</span>
+ </button>
+ </div>
+{{/unless}}
\ No newline at end of file
diff --git a/ghost/admin/app/services/members-stats.js b/ghost/admin/app/services/members-stats.js
index 65975a9192..8e546d2d8d 100644
--- a/ghost/admin/app/services/members-stats.js
+++ b/ghost/admin/app/services/members-stats.js
@@ -32,30 +32,20 @@ export default class MembersStatsService extends Service {
let daysChanged = this._lastFetchedDays !== this.days;
let staleData = this._lastFetched && (new Date() - this._lastFetched) > ONE_MINUTE;
- // return an already in-progress promise unless params have changed
- if (this._fetchTask.isRunning && !this._forceRefresh && !daysChanged) {
+ // return existing stats unless data is > 1 min old or days param has changed
+ if (this.stats && !this._forceRefresh && !daysChanged && !staleData && this._fetchTask.last) {
return this._fetchTask.last;
}
- // return existing stats unless data is > 1 min old
- if (this.stats && !this._forceRefresh && !daysChanged && !staleData) {
- return Promise.resolve(this.stats);
- }
-
return this._fetchTask.perform();
}
fetchCounts() {
let staleData = this._lastFetchedCounts && (new Date() - this._lastFetchedCounts) > ONE_MINUTE;
- // return an already in-progress promise unless params have changed
- if (this._fetchCountsTask.isRunning) {
- return this._fetchCountsTask.last;
- }
-
// return existing stats unless data is > 1 min old
- if (this.countStats && !this._forceRefresh && !staleData) {
- return Promise.resolve(this.countStats);
+ if (this.countStats && !this._forceRefresh && !staleData && this._fetchCountsTask.last) {
+ return this._fetchCountsTask.last;
}
return this._fetchCountsTask.perform();
@@ -64,14 +54,9 @@ export default class MembersStatsService extends Service {
fetchMemberCount() {
let staleData = this._lastFetchedMemberCounts && (new Date() - this._lastFetchedMemberCounts) > ONE_MINUTE;
- // return an already in-progress promise unless params have changed
- if (this._fetchMemberCountsTask.isRunning) {
- return this._fetchMemberCountsTask.last;
- }
-
// return existing stats unless data is > 1 min old
- if (this.totalMemberCount && !this._forceRefresh && !staleData) {
- return Promise.resolve(this.totalMemberCount);
+ if (this.totalMemberCount && !this._forceRefresh && !staleData && this._fetchMemberCountsTask.last) {
+ return this._fetchMemberCountsTask.last;
}
return this._fetchMemberCountsTask.perform();
@@ -80,14 +65,9 @@ export default class MembersStatsService extends Service {
fetchNewsletterStats() {
let staleData = this._lastFetchedNewsletterStats && (new Date() - this._lastFetchedNewsletterStats) > ONE_MINUTE;
- // return an already in-progress promise unless params have changed
- if (this._fetchNewsletterStatsTask.isRunning) {
- return this._fetchNewsletterStatsTask.last;
- }
-
// return existing stats unless data is > 1 min old
- if (this.newsletterStats && !this._forceRefresh && !staleData) {
- return Promise.resolve(this.newsletterStats);
+ if (this.newsletterStats && !this._forceRefresh && !staleData && this._fetchNewsletterStatsTask.last) {
+ return this._fetchNewsletterStatsTask.last;
}
return this._fetchNewsletterStatsTask.perform();
@@ -160,14 +140,9 @@ export default class MembersStatsService extends Service {
fetchMRR() {
let staleData = this._lastFetchedMRR && (new Date() - this._lastFetchedMRR) > ONE_MINUTE;
- // return an already in-progress promise unless params have changed
- if (this._fetchMRRTask.isRunning) {
- return this._fetchMRRTask.last;
- }
-
// return existing stats unless data is > 1 min old
- if (this.mrrStats && !this._forceRefresh && !staleData) {
- return Promise.resolve(this.mrrStats);
+ if (this.mrrStats && !this._forceRefresh && !staleData && this._fetchMRRTask) {
+ return this._fetchMRRTask.last;
}
return this._fetchMRRTask.perform();
diff --git a/ghost/admin/app/utils/publish-options.js b/ghost/admin/app/utils/publish-options.js
index d7593ea459..ea79000466 100644
--- a/ghost/admin/app/utils/publish-options.js
+++ b/ghost/admin/app/utils/publish-options.js
@@ -1,5 +1,6 @@
import moment from 'moment';
import {action, get} from '@ember/object';
+import {htmlSafe} from '@ember/template';
import {task} from 'ember-concurrency';
import {tracked} from '@glimmer/tracking';
@@ -14,6 +15,7 @@ export default class PublishOptions {
post = null;
user = null;
+ @tracked publishDisabledError = null;
@tracked totalMemberCount = 0;
get isLoading() {
@@ -270,8 +272,10 @@ export default class PublishOptions {
this.totalMemberCount = 1;
}
- // email limits
+ // limits
promises.push(this._checkSendingLimit());
+ promises.push(this._checkPublishingLimit());
+
// newsletters
promises.push(this.store.query('newsletter', {status: 'active', limit: 'all', include: 'count.members'}));
@@ -377,4 +381,15 @@ export default class PublishOptions {
this.emailDisabledError = e.message;
}
}
+
+ async _checkPublishingLimit() {
+ try {
+ if (this.limit.limiter?.isLimited('members')) {
+ await this.limit.limiter.errorIfIsOverLimit('members');
+ }
+ } catch (e) {
+ const linkedMessage = htmlSafe(e.message.replace(/please upgrade/i, '<a href="#/pro">$&</a>'));
+ this.publishDisabledError = linkedMessage;
+ }
+ }
}
diff --git a/ghost/admin/tests/acceptance/editor/publish-flow-test.js b/ghost/admin/tests/acceptance/editor/publish-flow-test.js
index b845468d36..aa76d68cc4 100644
--- a/ghost/admin/tests/acceptance/editor/publish-flow-test.js
+++ b/ghost/admin/tests/acceptance/editor/publish-flow-test.js
@@ -444,7 +444,79 @@ describe('Acceptance: Publish flow', function () {
expect(find('[data-test-publish-type="send"]')).to.have.attribute('disabled');
});
- it('handles member limits');
+ it('handles over-member limit before publish', async function () {
+ // set members limit
+ const config = this.server.db.configs.find(1);
+ config.hostSettings = {
+ limits: {
+ members: {
+ max: 9,
+ error: 'Your plan supports up to {{max}} members. Please upgrade to reenable publishing.'
+ }
+ }
+ };
+ this.server.db.configs.update(1, config);
+
+ // go over limit (7 created by default in beforeEach)
+ this.server.createList('member', 3);
+
+ // simulate /members/stats/count/ endpoint that's used to get total member count
+ // TODO: can the default endpoint mock handle this?
+ this.server.get('/members/stats/count', function () {
+ return {
+ total: 10,
+ resource: 'members',
+ data: []
+ };
+ });
+
+ // try to publish post
+ const post = this.server.create('post', {status: 'draft'});
+ await visit(`/editor/post/${post.id}`);
+ await click('[data-test-button="publish-flow"]');
+
+ expect(find('[data-test-publish-type-error]'), 'publish disabled error').to.exist;
+ expect(find('[data-test-publish-type-error="publish-disabled"]'), 'publish disabled error')
+ .to.have.trimmed.text('Your plan supports up to 9 members. Please upgrade to reenable publishing.');
+
+ expect(find('[data-test-button="continue"]'), 'continue button').to.not.exist;
+ });
+
+ it('handles over-member limit when confirming', async function () {
+ const post = this.server.create('post', {status: 'draft'});
+ await visit(`/editor/post/${post.id}`);
+ await click('[data-test-button="publish-flow"]');
+ await click('[data-test-button="continue"]');
+
+ this.server.put('/posts/:id/', function () {
+ return {
+ errors: [
+ {
+ message: 'Host Limit error, cannot edit post.',
+ context: 'Your plan supports up to 1,000 members. Please upgrade to reenable publishing.',
+ type: 'HostLimitError',
+ details: {
+ name: 'members',
+ limit: 1000,
+ total: 37406
+ },
+ property: null,
+ help: 'https://ghost.org/help/',
+ code: null,
+ id: '212d9110-3db6-11ed-9651-e9a82ad49a7a',
+ ghostErrorCode: null
+ }
+ ]
+ };
+ });
+
+ await click('[data-test-button="confirm-publish"]');
+
+ expect(find('[data-test-confirm-error]'), 'confirm error').to.exist;
+ expect(find('[data-test-confirm-error]'), 'confirm error')
+ .to.have.trimmed.text('Your plan supports up to 1,000 members. Please upgrade to reenable publishing.');
+ });
+
it('handles server error when confirming');
it('handles email sending error');
});