From 2447335ab16e4e2595a2007fd1aa30217a23fbef Mon Sep 17 00:00:00 2001
From: Katharina Irrgang <katharina.irrgang@googlemail.com>
Date: Mon, 6 May 2019 11:11:43 +0200
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20post=20scheduling=20on?=
 =?UTF-8?q?=20restart=20(#10726)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

no issue

- case: restart Ghost and while having a scheduled post
- caused by https://github.com/TryGhost/Ghost/commit/4acc375fb600bf9b46041e26e3d06b71a5b141b1#diff-4726ce3c4d18d41afad4b46cb0aa7dd3
  - the bug exists since 2.12
  - Bookshelf added support (or better said fixed a bug) for accessing previous attributes
  - `object.updated('published_at')` always returned "undefined", because the self-implementation < 2.12 only remembered previous attributes after update (see https://github.com/TryGhost/Ghost/blob/2.11.0/core/server/models/base/index.js#L234)
  - but `object.previous('published_at')` returns the current value (object.get('published_at') === object.previous('published_at') -> and that's why rescheduling on bootstrap never worked
- might fix https://forum.ghost.org/t/scheduled-posts-never-publish/6873/10
- reduced timeouts on scheduling unit tests
---
 .../adapters/scheduling/SchedulingDefault.js  | 19 ++++++-
 .../scheduling/post-scheduling/index.js       |  4 +-
 .../scheduling/SchedulingDefault_spec.js      | 49 +++++++++++++++++++
 3 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/core/server/adapters/scheduling/SchedulingDefault.js b/core/server/adapters/scheduling/SchedulingDefault.js
index 992a9ce8a4..c2727a2f2f 100644
--- a/core/server/adapters/scheduling/SchedulingDefault.js
+++ b/core/server/adapters/scheduling/SchedulingDefault.js
@@ -74,9 +74,24 @@ SchedulingDefault.prototype.schedule = function (object) {
  *                              oldTime:    [Number] The previous published time.
  *                          }
  *                       }
+ * @param {Object} options
+ *                      {
+ *                          bootstrap: [Boolean]
+ *                      }
  */
-SchedulingDefault.prototype.reschedule = function (object) {
-    this._deleteJob({time: object.extra.oldTime, url: object.url});
+SchedulingDefault.prototype.reschedule = function (object, options = {bootstrap: false}) {
+    /**
+     * CASE:
+     * The post scheduling unit calls "reschedule" on bootstrap, because other custom scheduling implementations
+     * could use a database and we need to give the chance to update the job (delete + re-add).
+     *
+     * We receive a "bootstrap" variable to ensure that jobs are scheduled correctly for this scheduler implementation,
+     * because "object.extra.oldTime" === "object.time". If we mark the job as deleted, it won't get scheduled.
+     */
+    if (!options.bootstrap) {
+        this._deleteJob({time: object.extra.oldTime, url: object.url});
+    }
+
     this._addJob(object);
 };
 
diff --git a/core/server/adapters/scheduling/post-scheduling/index.js b/core/server/adapters/scheduling/post-scheduling/index.js
index e4ce448aa8..59c9d2fbff 100644
--- a/core/server/adapters/scheduling/post-scheduling/index.js
+++ b/core/server/adapters/scheduling/post-scheduling/index.js
@@ -86,7 +86,9 @@ exports.init = function init(options = {}) {
             }
 
             scheduledPosts.forEach((model) => {
-                adapter.reschedule(_private.normalize({model, apiUrl, client}));
+                // NOTE: We are using reschedule, because custom scheduling adapter could use a database, which needs to be updated
+                //       and not an in-process implementation!
+                adapter.reschedule(_private.normalize({model, apiUrl, client}), {bootstrap: true});
             });
         })
         .then(() => {
diff --git a/core/test/unit/adapters/scheduling/SchedulingDefault_spec.js b/core/test/unit/adapters/scheduling/SchedulingDefault_spec.js
index 1de0d5e818..72ede5309b 100644
--- a/core/test/unit/adapters/scheduling/SchedulingDefault_spec.js
+++ b/core/test/unit/adapters/scheduling/SchedulingDefault_spec.js
@@ -60,6 +60,55 @@ describe('Scheduling Default Adapter', function () {
             ]);
         });
 
+        it('reschedule: default', function (done) {
+            sinon.stub(scope.adapter, '_pingUrl');
+
+            const time = moment().add(20, 'milliseconds').valueOf();
+
+            scope.adapter.schedule({
+                time: time,
+                url: 'something',
+                extra: {
+                    oldTime: null,
+                    method: 'PUT'
+                }
+            });
+
+            scope.adapter.reschedule({
+                time: time,
+                url: 'something',
+                extra: {
+                    oldTime: time,
+                    method: 'PUT'
+                }
+            });
+
+            setTimeout(() => {
+                scope.adapter._pingUrl.calledOnce.should.eql(true);
+                done();
+            }, 50);
+        });
+
+        it('reschedule: simulate restart', function (done) {
+            sinon.stub(scope.adapter, '_pingUrl');
+
+            const time = moment().add(20, 'milliseconds').valueOf();
+
+            scope.adapter.reschedule({
+               time: time,
+                url: 'something',
+                extra: {
+                   oldTime: time,
+                    method: 'PUT'
+                }
+            }, {bootstrap: true});
+
+            setTimeout(() => {
+                scope.adapter._pingUrl.calledOnce.should.eql(true);
+                done();
+            }, 50);
+        });
+
         it('run', function (done) {
             // 1000 jobs, but only the number x are under 1 minute
             var timestamps = _.map(_.range(1000), function (i) {