From 6873549786d35230c3ab6f2cccf6dff71029beea Mon Sep 17 00:00:00 2001 From: Kasper Nilsson Date: Tue, 1 Aug 2017 16:39:48 -0700 Subject: [PATCH] Modify reply dialog to wait for comment save Previously, opening the reply dialog was disabled when draft comments were pending save. Now, the reply dialog is fully enabled, and will silently wait for pending comments to be saved before proceeding with any save/send operations. Moreover, when diff drafts are in the process of saving, a label is displayed below the comments list. When they finish saving, the list is updated with the diff comments. Change-Id: I2777f9e15b052e606aa210b5372dc7a89a076345 --- .../change/gr-change-view/gr-change-view.js | 24 +++++---------- .../gr-reply-dialog/gr-reply-dialog.html | 16 ++++++++-- .../change/gr-reply-dialog/gr-reply-dialog.js | 19 ++++++++++++ .../gr-reply-dialog/gr-reply-dialog_test.html | 28 +++++++++++++++++ .../diff/gr-diff-view/gr-diff-view.js | 7 ----- .../gr-rest-api-interface.js | 30 ++++++++++++++----- .../gr-rest-api-interface_test.html | 30 ++++++++++++------- polygerrit-ui/app/test/common-test-setup.html | 11 +++++++ 8 files changed, 121 insertions(+), 44 deletions(-) diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js index 9088070850..1a20e7c74e 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js @@ -19,7 +19,6 @@ MISSING: 'missing', }; const CHANGE_ID_REGEX_PATTERN = /^Change-Id\:\s(I[0-9a-f]{8,40})/gm; - const COMMENT_SAVE = 'Saving... Try again after all comments are saved.'; const MIN_LINES_FOR_COMMIT_COLLAPSE = 30; const DEFAULT_NUM_FILES_SHOWN = 200; @@ -224,6 +223,7 @@ }); this.addEventListener('comment-save', this._handleCommentSave.bind(this)); + this.addEventListener('comment-refresh', this._getDiffDrafts.bind(this)); this.addEventListener('comment-discard', this._handleCommentDiscard.bind(this)); this.addEventListener('editable-content-save', @@ -841,11 +841,6 @@ }, _openReplyDialog(opt_section) { - if (this.$.restAPI.hasPendingDiffDrafts()) { - this.dispatchEvent(new CustomEvent('show-alert', - {detail: {message: COMMENT_SAVE}, bubbles: true})); - return; - } this.$.replyOverlay.open().then(() => { this.$.replyOverlay.setFocusStops(this.$.replyDialog.getFocusStops()); this.$.replyDialog.open(opt_section); @@ -869,10 +864,9 @@ }, _getDiffDrafts() { - return this.$.restAPI.getDiffDrafts(this._changeNum).then( - drafts => { - return this._diffDrafts = drafts; - }); + return this.$.restAPI.getDiffDrafts(this._changeNum).then(drafts => { + this._diffDrafts = drafts; + }); }, _getLoggedIn() { @@ -953,16 +947,14 @@ }, _getComments() { - return this.$.restAPI.getDiffComments(this._changeNum).then( - comments => { - this._comments = comments; - }); + return this.$.restAPI.getDiffComments(this._changeNum).then(comments => { + this._comments = comments; + }); }, _getLatestCommitMessage() { return this.$.restAPI.getChangeCommitInfo(this._changeNum, - this.computeLatestPatchNum(this._allPatchSets)).then( - commitInfo => { + this.computeLatestPatchNum(this._allPatchSets)).then(commitInfo => { this._latestCommitMessage = this._prepareCommitMsgForLinkify(commitInfo.message); }); diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html index 57c9f7d2a1..04a1b16189 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html @@ -127,11 +127,18 @@ limitations under the License. color: #444; font-style: italic; } - #notLatestLabel { + #notLatestLabel, + #savingLabel { color: red; } + #savingLabel { + display: none; + } + #savingLabel.saving { + display: inline; + } #cancelButton { - float:right; + float: right; } @media screen and (max-width: 50em) { :host { @@ -247,6 +254,11 @@ limitations under the License. project-config="[[projectConfig]]" patch-num="[[patchNum]]" hidden$="[[!_includeComments]]"> + + Saving comments... +
{ + this.fire('comment-refresh'); + this._savingComments = false; + }); + } }, focus() { @@ -749,5 +764,9 @@ _computeCCsEnabled(serverConfig) { return serverConfig && serverConfig.note_db_enabled; }, + + _computeSavingLabelClass(savingComments) { + return savingComments ? 'saving' : ''; + }, }); })(); diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html index 535dc40b16..f4b18419a6 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html @@ -964,6 +964,34 @@ limitations under the License. assertDialogClosed(); }); }); + + suite('pending diff drafts?', () => { + test('yes', () => { + const promise = mockPromise(); + const refreshHandler = sandbox.stub(); + + element.addEventListener('comment-refresh', refreshHandler); + sandbox.stub(element.$.restAPI, 'hasPendingDiffDrafts').returns(true); + element.$.restAPI._pendingRequests.sendDiffDraft = [promise]; + element.open(); + + assert.isFalse(refreshHandler.called); + assert.isTrue(element._savingComments); + + promise.resolve(); + + return element.$.restAPI.awaitPendingDiffDrafts().then(() => { + assert.isTrue(refreshHandler.called); + assert.isFalse(element._savingComments); + }); + }); + + test('no', () => { + sandbox.stub(element.$.restAPI, 'hasPendingDiffDrafts').returns(false); + element.open(); + assert.notOk(element._savingComments); + }); + }); }); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index e4901f5621..df7e3ab687 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js @@ -19,8 +19,6 @@ const ERR_REVIEW_STATUS = 'Couldn’t change file review status.'; - const COMMENT_SAVE = 'Try again when all comments have saved.'; - const PARENT = 'PARENT'; const DiffSides = { @@ -345,11 +343,6 @@ if (this.modifierPressed(e)) { return; } if (!this._loggedIn) { return; } - if (this.$.restAPI.hasPendingDiffDrafts()) { - this.dispatchEvent(new CustomEvent('show-alert', - {detail: {message: COMMENT_SAVE}, bubbles: true})); - return; - } this.set('changeViewState.showReplyDialog', true); e.preventDefault(); diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js index 536bdb2ab1..ab1e583536 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js @@ -803,7 +803,8 @@ saveChangeReview(changeNum, patchNum, review, opt_errFn, opt_ctx) { const url = this.getChangeActionURL(changeNum, patchNum, '/review'); - return this.send('POST', url, review, opt_errFn, opt_ctx); + return this.awaitPendingDiffDrafts() + .then(() => this.send('POST', url, review, opt_errFn, opt_ctx)); }, getFileInChangeEdit(changeNum, path) { @@ -1035,8 +1036,23 @@ return this._sendDiffDraftRequest('DELETE', changeNum, patchNum, draft); }, + /** + * @returns {boolean} Whether there are pending diff draft sends. + */ hasPendingDiffDrafts() { - return !!this._pendingRequests[Requests.SEND_DIFF_DRAFT]; + const promises = this._pendingRequests[Requests.SEND_DIFF_DRAFT]; + return promises && promises.length; + }, + + /** + * @returns {Promise} A promise that resolves when all pending diff draft + * sends have resolved. + */ + awaitPendingDiffDrafts() { + return Promise.all(this._pendingRequests[Requests.SEND_DIFF_DRAFT] || []) + .then(() => { + this._pendingRequests[Requests.SEND_DIFF_DRAFT] = []; + }); }, _sendDiffDraftRequest(method, changeNum, patchNum, draft) { @@ -1050,14 +1066,12 @@ } if (!this._pendingRequests[Requests.SEND_DIFF_DRAFT]) { - this._pendingRequests[Requests.SEND_DIFF_DRAFT] = 0; + this._pendingRequests[Requests.SEND_DIFF_DRAFT] = []; } - this._pendingRequests[Requests.SEND_DIFF_DRAFT]++; - return this.send(method, url, body).then(res => { - this._pendingRequests[Requests.SEND_DIFF_DRAFT]--; - return res; - }); + const promise = this.send(method, url, body); + this._pendingRequests[Requests.SEND_DIFF_DRAFT].push(promise); + return promise; }, getCommitInfo(project, commit) { diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html index a45ec04b6b..2cf1fa6c65 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html @@ -602,17 +602,25 @@ limitations under the License. }); }); - test('_sendDiffDraft pending requests tracked', done => { - sandbox.stub(element, 'send', () => { - assert.equal(element._pendingRequests.sendDiffDraft, 1); - return Promise.resolve([]); - }); - element.saveDiffDraft('', 1, 1).then(() => { - assert.equal(element._pendingRequests.sendDiffDraft, 0); - element.deleteDiffDraft('', 1, 1).then(() => { - assert.equal(element._pendingRequests.sendDiffDraft, 0); - done(); - }); + test('_sendDiffDraft pending requests tracked', () => { + const obj = element._pendingRequests; + sandbox.stub(element, 'send', () => mockPromise()); + sandbox.stub(element, 'getChangeActionURL'); + assert.notOk(element.hasPendingDiffDrafts()); + + element._sendDiffDraftRequest(null, null, null, {}); + assert.equal(obj.sendDiffDraft.length, 1); + assert.isTrue(!!element.hasPendingDiffDrafts()); + + element._sendDiffDraftRequest(null, null, null, {}); + assert.equal(obj.sendDiffDraft.length, 2); + assert.isTrue(!!element.hasPendingDiffDrafts()); + + for (const promise of obj.sendDiffDraft) { promise.resolve(); } + + return element.awaitPendingDiffDrafts().then(() => { + assert.equal(obj.sendDiffDraft.length, 0); + assert.isFalse(!!element.hasPendingDiffDrafts()); }); }); diff --git a/polygerrit-ui/app/test/common-test-setup.html b/polygerrit-ui/app/test/common-test-setup.html index 7c894b6d6f..57df8a6d2d 100644 --- a/polygerrit-ui/app/test/common-test-setup.html +++ b/polygerrit-ui/app/test/common-test-setup.html @@ -33,6 +33,17 @@ limitations under the License. }, }); +