Only erase unsaved drafts after save request succeeds

Unsaved diff drafts are kept in localstorage so they are not lost if
they do not get saved (for example, if the browser crashes). However,
this stored copy of the comment is cleared when the save request is
started, not after it succeeds. As such, if the save request fails (for
example, if the user credentials are invalid), the comment can be truly
lost.

With this change, the localstorage copy is only cleared after save
success. If the request fails, it allows the error message to appear
rather than showing a misleading "All comments saved" toast.

Bug: issue 7289
Change-Id: Id5c8144ecccec50de35f83af616b4e4915b62b5c

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);
       });
     });
   });