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.

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,31 +887,44 @@ 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);
 
+    promise = promise.then(() => {
       if (opts.draft) {
-      composerModel = loadDraft(this.store, opts);
-      if (composerModel) {
-        composerModel.set("topic", opts.topic);
+        return loadDraft(this.store, opts).then(model => {
+          if (!model) {
+            throw new Error("draft was not found");
           }
+          return model;
+        });
       } else {
-      composerModel = composerModel || this.store.createRecord("composer");
-      composerModel.open(opts);
+        let model =
+          optionalComposerModel || this.store.createRecord("composer");
+        return model.open(opts).then(() => model);
       }
+    });
 
+    promise.then(composerModel => {
       this.set("model", composerModel);
+
       composerModel.setProperties({
         composeState: Composer.OPEN,
         isWarning: false
@@ -950,6 +966,9 @@ export default Controller.extend({
       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() {
+    if (
+      this.model &&
+      !this.model.loading &&
+      !this.skipAutoSave &&
+      !this.model.disableDrafts
+    ) {
       debounce(this, this._saveDraft, 2000);
+    }
   },
 
   @discourseComputed("model.categoryId", "lastValidatedAt")

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);
-
+      promise = promise.then(() =>
         this.store
           .find("post", opts.postId)
-        .then(post => composer.setProperties({ post, loading: false }));
+          .then(post => composer.setProperties({ post }))
+      );
     }
 
     // If we are editing a post, load it.
@@ -759,15 +761,16 @@ const Composer = RestModel.extend({
       }
       this.setProperties(topicProps);
 
+      promise = promise.then(() =>
         this.store.find("post", opts.post.id).then(post => {
           composer.setProperties({
             reply: post.raw,
-          originalText: post.raw,
-          loading: false
+            originalText: post.raw
           });
 
           composer.appEvents.trigger("composer:reply-reloaded", composer);
-      });
+        })
+      );
     } else if (opts.action === REPLY && opts.quote) {
       this.setProperties({
         reply: opts.quote,
@@ -785,7 +788,9 @@ const Composer = RestModel.extend({
     }
 
     if (!isEdit(opts.action) || !opts.post) {
-      composer.appEvents.trigger("composer:reply-reloaded", composer);
+      promise = promise.then(() =>
+        composer.appEvents.trigger("composer:reply-reloaded", composer)
+      );
     }
 
     // Ensure additional draft fields are set
@@ -793,7 +798,9 @@ const Composer = RestModel.extend({
       this.set(_add_draft_fields[f], opts[f]);
     });
 
-    return false;
+    return promise.finally(() => {
+      this.set("loading", false);
+    });
   },
 
   // Overwrite to implement custom logic