FIX: do not save draft while it is loading

FIX: do not save draft while it is loading

When editing a post we were incorrectly saving a draft prior to user typing

This caused a bloat in the amount of drafts saved per user and inconsistency around behavior of “escape” button.

It also lead to lots of warnings about draft conflicts when copying stuff between posts.

The code is improved to use promises more appropriately, however further changes are needed to clean up internals so methods consistently return promises.

Too many methods in the controller sometimes return a promise and sometimes an object. Long term the methods will become async and all of this will be corrected.

diff --git a/app/assets/javascripts/discourse/controllers/composer.js b/app/assets/javascripts/discourse/controllers/composer.js
index e7d02fb..297d635 100644
--- a/app/assets/javascripts/discourse/controllers/composer.js
+++ b/app/assets/javascripts/discourse/controllers/composer.js
@@ -27,6 +27,8 @@ import EmberObject, { computed } from "@ember/object";
 import deprecated from "discourse-common/lib/deprecated";
 
 function loadDraft(store, opts) {
+  let promise = Promise.resolve();
+
   opts = opts || {};
 
   let draft = opts.draft;
@@ -59,10 +61,10 @@ function loadDraft(store, opts) {
       attrs[f] = draft[f] || opts[f];
     });
 
-    composer.open(attrs);
-
-    return composer;
+    promise = promise.then(() => composer.open(attrs)).then(() => composer);
   }
+
+  return promise;
 }
 
 const _popupMenuOptionsCallbacks = [];
@@ -799,7 +801,8 @@ export default Controller.extend({
     this.setProperties({
       showEditReason: false,
       editReason: null,
-      scopedCategoryId: null
+      scopedCategoryId: null,
+      skipAutoSave: true
     });
 
     // Scope the categories drop down to the category we opened the composer with.
@@ -820,7 +823,7 @@ export default Controller.extend({
       composerModel = null;
     }
 
-    return new Promise((resolve, reject) => {
+    let promise = new Promise((resolve, reject) => {
       if (composerModel && composerModel.replyDirty) {
         // If we're already open, we don't have to do anything
         if (
@@ -870,7 +873,7 @@ export default Controller.extend({
               opts.draft = data.draft;
             }
             opts.draftSequence = data.draft_sequence;
-            this._setModel(composerModel, opts);
+            return this._setModel(composerModel, opts);
           })
           .then(resolve, reject);
       }
@@ -884,72 +887,88 @@ export default Controller.extend({
             if (data.draft) {
               opts.draft = data.draft;
               opts.draftSequence = data.draft_sequence;
-              this.open(opts);
+              return this.open(opts);
             }
           });
       }
 
-      this._setModel(composerModel, opts);
-      resolve();
+      this._setModel(composerModel, opts).then(resolve, reject);
+    });
+
+    promise = promise.finally(() => {
+      this.skipAutoSave = false;
     });
+    return promise;
   },
 
   // Given a potential instance and options, set the model for this composer.
-  _setModel(composerModel, opts) {
+  _setModel(optionalComposerModel, opts) {
+    let promise = Promise.resolve();
+
     this.set("linkLookup", null);
 
-    if (opts.draft) {
-      composerModel = loadDraft(this.store, opts);
-      if (composerModel) {
-        composerModel.set("topic", opts.topic);
+    promise = promise.then(() => {
+      if (opts.draft) {
+        return loadDraft(this.store, opts).then(model => {
+          if (!model) {
+            throw new Error("draft was not found");
+          }
+          return model;
+        });
+      } else {
+        let model =
+          optionalComposerModel || this.store.createRecord("composer");
+        return model.open(opts).then(() => model);
       }
-    } else {
-      composerModel = composerModel || this.store.createRecord("composer");
-      composerModel.open(opts);
-    }
-
-    this.set("model", composerModel);
-    composerModel.setProperties({
-      composeState: Composer.OPEN,
-      isWarning: false
     });
 
-    if (!this.model.targetRecipients) {
-      if (opts.usernames) {
-        deprecated("`usernames` is deprecated, use `recipients` instead.");
-        this.model.set("targetRecipients", opts.usernames);
-      } else if (opts.recipients) {
-        this.model.set("targetRecipients", opts.recipients);
+    promise.then(composerModel => {
+      this.set("model", composerModel);
+
+      composerModel.setProperties({
+        composeState: Composer.OPEN,
+        isWarning: false
+      });
+
+      if (!this.model.targetRecipients) {
+        if (opts.usernames) {
+          deprecated("`usernames` is deprecated, use `recipients` instead.");
+          this.model.set("targetRecipients", opts.usernames);
+        } else if (opts.recipients) {
+          this.model.set("targetRecipients", opts.recipients);
+        }
       }
-    }
 
-    if (
-      opts.topicTitle &&
-      opts.topicTitle.length <= this.siteSettings.max_topic_title_length
-    ) {
-      this.model.set("title", opts.topicTitle);
-    }
+      if (
+        opts.topicTitle &&
+        opts.topicTitle.length <= this.siteSettings.max_topic_title_length
+      ) {
+        this.model.set("title", opts.topicTitle);
+      }
 
-    if (opts.topicCategoryId) {
-      this.model.set("categoryId", opts.topicCategoryId);
-    }
+      if (opts.topicCategoryId) {
+        this.model.set("categoryId", opts.topicCategoryId);
+      }
 
-    if (opts.topicTags && !this.site.mobileView && this.site.can_tag_topics) {
-      let tags = escapeExpression(opts.topicTags)
-        .split(",")
-        .slice(0, this.siteSettings.max_tags_per_topic);
+      if (opts.topicTags && !this.site.mobileView && this.site.can_tag_topics) {
+        let tags = escapeExpression(opts.topicTags)
+          .split(",")
+          .slice(0, this.siteSettings.max_tags_per_topic);
 
-      tags.forEach(
-        (tag, index, array) =>
-          (array[index] = tag.substring(0, this.siteSettings.max_tag_length))
-      );
+        tags.forEach(
+          (tag, index, array) =>
+            (array[index] = tag.substring(0, this.siteSettings.max_tag_length))
+        );
 
-      this.model.set("tags", tags);
-    }
+        this.model.set("tags", tags);
+      }
 
-    if (opts.topicBody) {
-      this.model.set("reply", opts.topicBody);
-    }
+      if (opts.topicBody) {
+        this.model.set("reply", opts.topicBody);
+      }
+    });
+
+    return promise;
   },
 
   viewNewReply() {
@@ -1009,10 +1028,12 @@ export default Controller.extend({
   },
 
   cancelComposer(differentDraft = false) {
+    this.skipAutoSave = true;
+
     const keyPrefix =
       this.model.action === "edit" ? "post.abandon_edit" : "post.abandon";
 
-    return new Promise(resolve => {
+    let promise = new Promise(resolve => {
       if (this.get("model.hasMetaData") || this.get("model.replyDirty")) {
         bootbox.dialog(I18n.t(keyPrefix + ".confirm"), [
           {
@@ -1051,6 +1072,10 @@ export default Controller.extend({
         });
       }
     });
+
+    return promise.finally(() => {
+      this.skipAutoSave = false;
+    });
   },
 
   shrink() {
@@ -1073,7 +1098,14 @@ export default Controller.extend({
 
   @observes("model.reply", "model.title")
   _shouldSaveDraft() {
-    debounce(this, this._saveDraft, 2000);
+    if (
+      this.model &&
+      !this.model.loading &&
+      !this.skipAutoSave &&
+      !this.model.disableDrafts
+    ) {
+      debounce(this, this._saveDraft, 2000);
+    }
   },
 
   @discourseComputed("model.categoryId", "lastValidatedAt")
diff --git a/app/assets/javascripts/discourse/models/composer.js b/app/assets/javascripts/discourse/models/composer.js
index 1b53ed8..9d0dcb8 100644
--- a/app/assets/javascripts/discourse/models/composer.js
+++ b/app/assets/javascripts/discourse/models/composer.js
@@ -673,14 +673,16 @@ const Composer = RestModel.extend({
        quote    - If we're opening a reply from a quote, the quote we're making
   */
   open(opts) {
+    let promise = Promise.resolve();
+
     if (!opts) opts = {};
-    this.set("loading", false);
+    this.set("loading", true);
 
     const replyBlank = isEmpty(this.reply);
 
     const composer = this;
     if (!replyBlank && (opts.reply || isEdit(opts.action)) && this.replyDirty) {
-      return;
+      return promise;
     }
 
     if (opts.action === REPLY && isEdit(this.action)) {
@@ -741,11 +743,11 @@ const Composer = RestModel.extend({
     }
 
     if (opts.postId) {
-      this.set("loading", true);
-
-      this.store

[... diff too long, it was truncated ...]

GitHub sha: a34711c2

This commit has been mentioned on Discourse Meta. There might be relevant details there: