Merge "Only erase unsaved drafts after save request succeeds"

polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js

@@ -116,7 +116,7 @@
         observer: '_toggleResolved',
       },
 
-      _numPendingDiffRequests: {
+      _numPendingDraftRequests: {
         type: Object,
         value: {number: 0}, // Intentional to share the object across instances.
       },
@@ -180,12 +180,11 @@
 
       this.disabled = true;
 
-      this._eraseDraftComment();
-
       this._xhrPromise = this._saveDraft(this.comment).then(response => {
         this.disabled = false;
         if (!response.ok) { return response; }
 
+        this._eraseDraftComment();
         return this.$.restAPI.getResponseObject(response).then(obj => {
           const comment = obj;
           comment.__draft = true;
@@ -204,6 +203,8 @@
         this.disabled = false;
         throw err;
       });
+
+      return this._xhrPromise;
     },
 
     _eraseDraftComment() {
@@ -467,15 +468,23 @@
     },
 
     _showStartRequest() {
-      const numPending = ++this._numPendingDiffRequests.number;
+      const numPending = ++this._numPendingDraftRequests.number;
       this._updateRequestToast(numPending);
     },
 
     _showEndRequest() {
-      const numPending = --this._numPendingDiffRequests.number;
+      const numPending = --this._numPendingDraftRequests.number;
       this._updateRequestToast(numPending);
     },
 
+    _handleFailedDraftRequest() {
+      this._numPendingDraftRequests.number--;
+
+      // Cancel the debouncer so that error toasts from the error-manager will
+      // not be overridden.
+      this.cancelDebouncer('draft-toast');
+    },
+
     _updateRequestToast(numPending) {
       const message = this._getSavingMessage(numPending);
       this.debounce('draft-toast', () => {
@@ -491,7 +500,11 @@
       this._showStartRequest();
       return this.$.restAPI.saveDiffDraft(this.changeNum, this.patchNum, draft)
           .then(result => {
-            this._showEndRequest();
+            if (result.ok) {
+              this._showEndRequest();
+            } else {
+              this._handleFailedDraftRequest();
+            }
             return result;
           });
     },
@@ -500,7 +513,11 @@
       this._showStartRequest();
       return this.$.restAPI.deleteDiffDraft(this.changeNum, this.patchNum,
           draft).then(result => {
-            this._showEndRequest();
+            if (result.ok) {
+              this._showEndRequest();
+            } else {
+              this._handleFailedDraftRequest();
+            }
             return result;
           });
     },

polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html

@@ -493,6 +493,27 @@ limitations under the License.
       element._handleConfirmDiscard({preventDefault: sinon.stub()});
     });
 
+    test('storage is cleared only after save success', () => {
+      const eraseStub = sandbox.stub(element, '_eraseDraftComment');
+      sandbox.stub(element.$.restAPI, 'getResponseObject')
+          .returns(Promise.resolve({}));
+
+      sandbox.stub(element, '_saveDraft').returns(Promise.resolve({ok: false}));
+
+      const savePromise = element.save();
+      assert.isFalse(eraseStub.called);
+      return savePromise.then(() => {
+        assert.isFalse(eraseStub.called);
+
+        element._saveDraft.restore();
+        sandbox.stub(element, '_saveDraft')
+            .returns(Promise.resolve({ok: true}));
+        return element.save().then(() => {
+          assert.isTrue(eraseStub.called);
+        });
+      });
+    });
+
     suite('confirm discard', () => {
       let saveDisabled;
       let discardStub;
@@ -659,22 +680,22 @@ limitations under the License.
 
       test('_show{Start,End}Request', () => {
         const updateStub = sandbox.stub(element, '_updateRequestToast');
-        element._numPendingDiffRequests.number = 1;
+        element._numPendingDraftRequests.number = 1;
 
         element._showStartRequest();
         assert.isTrue(updateStub.calledOnce);
         assert.equal(updateStub.lastCall.args[0], 2);
-        assert.equal(element._numPendingDiffRequests.number, 2);
+        assert.equal(element._numPendingDraftRequests.number, 2);
 
         element._showEndRequest();
         assert.isTrue(updateStub.calledTwice);
         assert.equal(updateStub.lastCall.args[0], 1);
-        assert.equal(element._numPendingDiffRequests.number, 1);
+        assert.equal(element._numPendingDraftRequests.number, 1);
 
         element._showEndRequest();
         assert.isTrue(updateStub.calledThrice);
         assert.equal(updateStub.lastCall.args[0], 0);
-        assert.equal(element._numPendingDiffRequests.number, 0);
+        assert.equal(element._numPendingDraftRequests.number, 0);
       });
     });
   });