opendev/gerrit
Code Issues Proposed changes

Modify reply dialog to wait for comment save

Browse Source 
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
This commit is contained in:
Kasper Nilsson
2017-08-01 16:39:48 -07:00
parent c3e4ec1b37
commit 6873549786
8 changed files with 121 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

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

16
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
View File

@@ -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]]"></gr-comment-list>
<span
id="savingLabel"
class$="[[_computeSavingLabelClass(_savingComments)]]">
Saving comments...
</span>
</section>
<section>
<gr-button

19
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
View File

@@ -66,6 +66,13 @@
* @event show-alert
*/
/**
* Fires when the reply dialog believes that the server side diff drafts
* have been updated and need to be refreshed.
*
* @event comment-refresh
*/
properties: {
change: Object,
patchNum: String,
@@ -150,6 +157,7 @@
type: Boolean,
computed: '_computeCCsEnabled(serverConfig)',
},
_savingComments: Boolean,
},
FocusTarget,
@@ -198,6 +206,13 @@
if (!this.draft || !this.draft.length) {
this.draft = this._loadStoredDraft();
}
if (this.$.restAPI.hasPendingDiffDrafts()) {
this._savingComments = true;
this.$.restAPI.awaitPendingDiffDrafts().then(() => {
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' : '';
},
});
})();

28
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
View File

@@ -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);
});
});
});
});
</script>

7
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
View File

@@ -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();

30
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@@ -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) {

30
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
View File

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

11
polygerrit-ui/app/test/common-test-setup.html
View File

@@ -33,6 +33,17 @@ limitations under the License.
},
});
</script>
<script>
// eslint-disable-next-line no-unused-vars
const mockPromise = () => {
let res;
const promise = new Promise(resolve => {
res = resolve;
});
promise.resolve = res;
return promise;
};
</script>
<link rel="import"
href="../bower_components/iron-test-helpers/iron-test-helpers.html" />
<link rel="import" href="test-router.html" />