🐛 Fixed retrying failed emails when rescheduling them (#16383)

fixes https://github.com/TryGhost/Team/issues/2560

When an email fails, and you reschedule the post, the error dialog was
shown (from the previous try). The retry button on that page allowed you
to retry sending the email immediately, which could be very confusing.

- The email error dialog is no longer shown for scheduled emails
- The email status is no longer polled for scheduled emails
- Retrying an email is not possible via the API if the post status is
not published or sent
- Added some extra snapshot tests
- When retrying an email, we immediately update the email status to
'pending' to have a better API response (instead of still returning
failed).
- Disabled email sending retrying in development (otherwise very hard to
test failed emails if it takes 10 mins before it gives up automatic
retrying)

ghost/admin/app/components/editor/publish-management.js

@@ -166,7 +166,7 @@ export default class PublishManagement extends Component {
 
     @task
     *publishTask({taskName = 'saveTask'} = {}) {
-        const willEmail = this.publishOptions.willEmail;
+        const willEmailImmediately = this.publishOptions.willEmailImmediately;
 
         // clean up blank editor cards
         // apply cloned mobiledoc
@@ -182,7 +182,7 @@ export default class PublishManagement extends Component {
         yield this.args.afterPublish(result);
 
         // if emailed, wait until it has been submitted so we can show a failure message if needed
-        if (willEmail && this.publishOptions.post.email) {
+        if (willEmailImmediately && this.publishOptions.post.email) {
             yield this.confirmEmailTask.perform();
         }
 
@@ -216,6 +216,11 @@ export default class PublishManagement extends Component {
 
                 yield post.reload();
 
+                if (!post.isSent && !post.isPublished) {
+                    // A post that is not published doesn't try to send or retry an email
+                    break;
+                }
+
                 if (post.email.status === 'submitted') {
                     break;
                 }

ghost/admin/app/utils/publish-options.js

@@ -33,6 +33,10 @@ export default class PublishOptions {
         );
     }
 
+    get willEmailImmediately() {
+        return this.willEmail && !this.isScheduled;
+    }
+
     get willPublish() {
         return this.publishType !== 'send';
     }

ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/emails.js

@@ -24,5 +24,8 @@ module.exports = (model, frame) => {
         }
     }
 
+    // Removed loaded post relation if set
+    delete jsonModel.post;
+
     return jsonModel;
 };

ghost/core/core/server/models/base/plugins/relations.js

@@ -7,7 +7,7 @@ module.exports = function (Bookshelf) {
          * Return a relation, and load it if it hasn't been loaded already (or force a refresh with the forceRefresh option).
          * refs https://github.com/TryGhost/Team/issues/1626
          * @param {string} name Name of the relation to load
-         * @param {Object} [options] Options to pass to the fetch when not yet loaded (or when force refreshing) 
+         * @param {Object} [options] Options to pass to the fetch when not yet loaded (or when force refreshing)
          * @param {boolean} [options.forceRefresh] If true, the relation will be fetched again even if it has already been loaded.
          * @returns {Promise<import('bookshelf').Model|import('bookshelf').Collection|null>}
          */
@@ -23,8 +23,10 @@ module.exports = function (Bookshelf) {
             // Not yet loaded, or force refresh
             // Note that we don't use .refresh on the relation on options.forceRefresh
             // Because the relation can also be a collection, which doesn't have a refresh method
-            this.relations[name] = this[name]();
-            return this.relations[name].fetch(options);
+            const instance = this[name]();
+            await instance.fetch(options);
+            this.relations[name] = instance;
+            return instance;
         }
     });
 };

ghost/core/test/e2e-api/admin/__snapshots__/emails.test.js.snap

@@ -753,7 +753,7 @@ Object {
       "reply_to": null,
       "source": null,
       "source_type": "html",
-      "status": "failed",
+      "status": "pending",
       "subject": "You got mailed! Again!",
       "submitted_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
       "track_clicks": false,
@@ -769,7 +769,7 @@ exports[`Emails API Can retry a failed email 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": "687",
+  "content-length": "688",
   "content-type": "application/json; charset=utf-8",
   "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
   "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,

ghost/core/test/e2e-api/admin/email-previews.test.js

@@ -61,6 +61,7 @@ describe('Email Preview API', function () {
         });
 
         it('can read post email preview with fields', async function () {
+            const defaultNewsletter = await models.Newsletter.getDefaultNewsletter();
             await agent
                 .get(`email_previews/posts/${fixtureManager.get('posts', 0).id}/`)
                 .expectStatus(200)
@@ -68,10 +69,19 @@ describe('Email Preview API', function () {
                     'content-version': anyContentVersion,
                     etag: anyEtag
                 })
-                .matchBodySnapshot(matchEmailPreviewBody);
+                .matchBodySnapshot(matchEmailPreviewBody)
+                .expect(({body}) => {
+                    testCleanedSnapshot(body.email_previews[0].html, {
+                        [defaultNewsletter.get('uuid')]: 'requested-newsletter-uuid'
+                    });
+                    testCleanedSnapshot(body.email_previews[0].plaintext, {
+                        [defaultNewsletter.get('uuid')]: 'requested-newsletter-uuid'
+                    });
+                });
         });
 
         it('can read post email preview with email card and replacements', async function () {
+            const defaultNewsletter = await models.Newsletter.getDefaultNewsletter();
             const post = testUtils.DataGenerator.forKnex.createPost({
                 id: ObjectId().toHexString(),
                 title: 'Post with email-only card',
@@ -93,7 +103,15 @@ describe('Email Preview API', function () {
                     'content-version': anyContentVersion,
                     etag: anyEtag
                 })
-                .matchBodySnapshot(matchEmailPreviewBody);
+                .matchBodySnapshot(matchEmailPreviewBody)
+                .expect(({body}) => {
+                    testCleanedSnapshot(body.email_previews[0].html, {
+                        [defaultNewsletter.get('uuid')]: 'requested-newsletter-uuid'
+                    });
+                    testCleanedSnapshot(body.email_previews[0].plaintext, {
+                        [defaultNewsletter.get('uuid')]: 'requested-newsletter-uuid'
+                    });
+                });
         });
 
         it('has custom content transformations for email compatibility', async function () {
@@ -125,6 +143,9 @@ describe('Email Preview API', function () {
                     assert.doesNotMatch(body.email_previews[0].html, /Testing links in email excerpt and apostrophes '/);
                     assert.match(body.email_previews[0].html, /Testing links in email excerpt and apostrophes '/);
 
+                    testCleanedSnapshot(body.email_previews[0].html, {
+                        [defaultNewsletter.get('uuid')]: 'requested-newsletter-uuid'
+                    });
                     testCleanedSnapshot(body.email_previews[0].plaintext, {
                         [defaultNewsletter.get('uuid')]: 'requested-newsletter-uuid'
                     });

ghost/email-service/lib/batch-sending-service.js

@@ -76,14 +76,14 @@ class BatchSendingService {
         if (BEFORE_RETRY_CONFIG) {
             this.#BEFORE_RETRY_CONFIG = BEFORE_RETRY_CONFIG;
         } else {
-            if (process.env.NODE_ENV.startsWith('test')) {
+            if (process.env.NODE_ENV.startsWith('test') || process.env.NODE_ENV === 'development') {
                 this.#BEFORE_RETRY_CONFIG = {maxRetries: 0};
             }
         }
         if (AFTER_RETRY_CONFIG) {
             this.#AFTER_RETRY_CONFIG = AFTER_RETRY_CONFIG;
         } else {
-            if (process.env.NODE_ENV.startsWith('test')) {
+            if (process.env.NODE_ENV.startsWith('test') || process.env.NODE_ENV === 'development') {
                 this.#AFTER_RETRY_CONFIG = {maxRetries: 0};
             }
         }
@@ -91,7 +91,7 @@ class BatchSendingService {
         if (MAILGUN_API_RETRY_CONFIG) {
             this.#MAILGUN_API_RETRY_CONFIG = MAILGUN_API_RETRY_CONFIG;
         } else {
-            if (process.env.NODE_ENV.startsWith('test')) {
+            if (process.env.NODE_ENV.startsWith('test') || process.env.NODE_ENV === 'development') {
                 this.#MAILGUN_API_RETRY_CONFIG = {maxRetries: 0};
             }
         }

ghost/email-service/lib/email-service.js

@@ -18,7 +18,8 @@ const logging = require('@tryghost/logging');
 const messages = {
     archivedNewsletterError: 'Cannot send email to archived newsletters',
     missingNewsletterError: 'The post does not have a newsletter relation',
-    emailSendingDisabled: `Email sending is temporarily disabled because your account is currently in review. You should have an email about this from us already, but you can also reach us any time at support@ghost.org`
+    emailSendingDisabled: `Email sending is temporarily disabled because your account is currently in review. You should have an email about this from us already, but you can also reach us any time at support@ghost.org`,
+    retryEmailStatusError: 'Can only retry emails for published posts'
 };
 
 class EmailService {
@@ -156,7 +157,22 @@ class EmailService {
         return email;
     }
     async retryEmail(email) {
+        // Block accidentaly retrying non-published posts (can happen due to bugs in frontend)
+        const post = await email.getLazyRelation('post');
+        if (post.get('status') !== 'published' && post.get('status') !== 'sent') {
+            throw new errors.IncorrectUsageError({
+                message: tpl(messages.retryEmailStatusError)
+            });
+        }
+
         await this.checkLimits();
+
+        // Change email status back to 'pending' before scheduling
+        // so we have a immediate response when retrying an email (schedule can take a while to kick off sometimes)
+        if (email.get('status') === 'failed') {
+            await email.save({status: 'pending'}, {patch: true});
+        }
+
         this.#batchSendingService.scheduleEmail(email);
         return email;
     }

ghost/email-service/test/batch-sending-service.test.js

@@ -17,6 +17,18 @@ describe('Batch Sending Service', function () {
         sinon.restore();
     });
 
+    describe('constructor', function () {
+        it('works in development mode', async function () {
+            const env = process.env.NODE_ENV;
+            process.env.NODE_ENV = 'development';
+            try {
+                new BatchSendingService({});
+            } finally {
+                process.env.NODE_ENV = env;
+            }
+        });
+    });
+
     describe('scheduleEmail', function () {
         it('schedules email', async function () {
             const jobsService = {

ghost/email-service/test/email-service.test.js

@@ -253,13 +253,29 @@ describe('Email Service', function () {
         it('Schedules email again', async function () {
             const email = createModel({
                 status: 'failed',
-                error: 'Test error'
+                error: 'Test error',
+                post: createModel({
+                    status: 'published'
+                })
             });
 
             await service.retryEmail(email);
             sinon.assert.calledOnce(scheduleEmail);
         });
 
+        it('Does not schedule email again if draft', async function () {
+            const email = createModel({
+                status: 'failed',
+                error: 'Test error',
+                post: createModel({
+                    status: 'draft'
+                })
+            });
+
+            await assert.rejects(service.retryEmail(email));
+            sinon.assert.notCalled(scheduleEmail);
+        });
+
         it('Checks limits before scheduling', async function () {
             const email = createModel({
                 status: 'failed',