From d563a713d68b58113d2e53e32e5b29d3014401fc Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Thu, 26 Oct 2017 14:11:24 -0700 Subject: [PATCH] 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 --- .../diff/gr-diff-comment/gr-diff-comment.js | 31 ++++++++++++++----- .../gr-diff-comment/gr-diff-comment_test.html | 29 ++++++++++++++--- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js index 23cf5cfbfc..e8717b566a 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js +++ b/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; }); }, diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html index 824a1752ff..8b6ec5ed04 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html +++ b/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); }); }); });