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 --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 23cf5cf..e8717b5 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 824a175..8b6ec5e 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 @@
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 @@
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);
});
});
});
