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 d770dddc80..968f56fd1c 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 @@ -310,7 +310,7 @@ class GrChangeView extends mixinBehaviors( [ _replyButtonLabel: { type: String, value: 'Reply', - computed: '_computeReplyButtonLabel(_diffDrafts.*, _canStartReview)', + computed: '_computeReplyButtonLabel(_diffDrafts.*)', }, _selectedPatchSet: String, _shownFileCount: Number, @@ -922,6 +922,23 @@ class GrChangeView extends mixinBehaviors( [ this._openReplyDialog(this.$.replyDialog.FocusTarget.ANY); } + _handleReadyTap(e) { + e.preventDefault(); + const button = e && e.target; + if (button) { + button.loading = true; + } + return this.$.restAPI.startReview(this._changeNum) + .then(result => { + this._reload(result); + }) + .finally(() => { + if (button) { + button.loading = false; + } + }); + } + _handleOpenDiffPrefs() { this.$.fileList.openDiffPrefs(); } @@ -1361,16 +1378,12 @@ class GrChangeView extends mixinBehaviors( [ return result; } - _computeReplyButtonLabel(changeRecord, canStartReview) { + _computeReplyButtonLabel(changeRecord) { // Polymer 2: check for undefined - if ([changeRecord, canStartReview].some(arg => arg === undefined)) { + if ([changeRecord].some(arg => arg === undefined)) { return 'Reply'; } - if (canStartReview) { - return 'Start review'; - } - const drafts = (changeRecord && changeRecord.base) || {}; const draftCount = Object.keys(drafts) .reduce((count, file) => count + drafts[file].length, 0); @@ -1879,7 +1892,7 @@ class GrChangeView extends mixinBehaviors( [ _computeCanStartReview(change) { return !!(change.actions && change.actions.ready && - change.actions.ready.enabled); + change.actions.ready.enabled); } _computeReplyDisabled() { return false; } diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.js index 5521eb4c3c..e8f8a68a6d 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.js @@ -446,11 +446,20 @@ export const htmlTemplate = html` title="[[createTitle(Shortcut.OPEN_REPLY_DIALOG, ShortcutSection.ACTIONS)]]" hidden$="[[!_loggedIn]]" - primary="" + primary$="[[!_canStartReview]]" disabled="[[_replyDisabled]]" on-click="_handleReplyTap" >[[_replyButtonLabel]] +
{ const getLabel = element._computeReplyButtonLabel; assert.equal(getLabel(null, false), 'Reply'); - assert.equal(getLabel(null, true), 'Start review'); + assert.equal(getLabel(null, true), 'Reply'); const changeRecord = {base: null}; assert.equal(getLabel(changeRecord, false), 'Reply'); @@ -1078,9 +1078,7 @@ suite('gr-change-view tests', () => { }); test('start review button when owner of WIP change', () => { - assert.equal( - element._computeReplyButtonLabel(null, true), - 'Start review'); + assert.equal(element.$.startReviewBtn.innerHTML, 'Start review'); }); test('comment events properly update diff drafts', () => { diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js index 0b92d784f0..419e95bdf9 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js @@ -250,7 +250,7 @@ class GrReplyDialog extends mixinBehaviors( [ }, _sendDisabled: { type: Boolean, - computed: '_computeSendButtonDisabled(_sendButtonLabel, ' + + computed: '_computeSendButtonDisabled(canBeStarted, ' + 'draftCommentThreads, draft, _reviewersMutated, _labelsChanged, ' + '_includeComments, disabled, _commentEditing)', observer: '_sendDisabledChanged', @@ -871,7 +871,8 @@ class GrReplyDialog extends mixinBehaviors( [ } _computeSendButtonLabel(canBeStarted) { - return canBeStarted ? ButtonLabels.START_REVIEW : ButtonLabels.SEND; + return canBeStarted ? ButtonLabels.SEND + ' and ' + + ButtonLabels.START_REVIEW : ButtonLabels.SEND; } _computeSendButtonTooltip(canBeStarted) { @@ -883,11 +884,11 @@ class GrReplyDialog extends mixinBehaviors( [ } _computeSendButtonDisabled( - buttonLabel, draftCommentThreads, text, reviewersMutated, + canBeStarted, draftCommentThreads, text, reviewersMutated, labelsChanged, includeComments, disabled, commentEditing) { // Polymer 2: check for undefined if ([ - buttonLabel, + canBeStarted, draftCommentThreads, text, reviewersMutated, @@ -900,7 +901,7 @@ class GrReplyDialog extends mixinBehaviors( [ } if (commentEditing || disabled) { return true; } - if (buttonLabel === ButtonLabels.START_REVIEW) { return false; } + if (canBeStarted === true) { return false; } const hasDrafts = includeComments && draftCommentThreads.length; return !hasDrafts && !text.length && !reviewersMutated && !labelsChanged; } 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 91b4db2c1c..5a61864f79 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 @@ -955,7 +955,7 @@ suite('gr-reply-dialog tests', () => { 'Send'); assert.equal( element._computeSendButtonLabel(true), - 'Start review'); + 'Send and Start review'); }); test('_handle400Error reviewrs and CCs', done => { @@ -1138,10 +1138,11 @@ suite('gr-reply-dialog tests', () => { }); }); - test('_computeSendButtonDisabled', () => { + test('_computeSendButtonDisabled_canBeStarted', () => { const fn = element._computeSendButtonDisabled.bind(element); + // Mock canBeStarted assert.isFalse(fn( - /* buttonLabel= */ 'Start review', + /* canBeStarted= */ true, /* draftCommentThreads= */ [], /* text= */ '', /* reviewersMutated= */ false, @@ -1150,8 +1151,13 @@ suite('gr-reply-dialog tests', () => { /* disabled= */ false, /* commentEditing= */ false )); + }); + + test('_computeSendButtonDisabled_allFalse', () => { + const fn = element._computeSendButtonDisabled.bind(element); + // Mock everything false assert.isTrue(fn( - /* buttonLabel= */ 'Send', + /* canBeStarted= */ false, /* draftCommentThreads= */ [], /* text= */ '', /* reviewersMutated= */ false, @@ -1160,9 +1166,13 @@ suite('gr-reply-dialog tests', () => { /* disabled= */ false, /* commentEditing= */ false )); - // Mock nonempty comment draft array, with seding comments. + }); + + test('_computeSendButtonDisabled_draftCommentsSend', () => { + const fn = element._computeSendButtonDisabled.bind(element); + // Mock nonempty comment draft array, with sending comments. assert.isFalse(fn( - /* buttonLabel= */ 'Send', + /* canBeStarted= */ false, /* draftCommentThreads= */ [{comments: [{__draft: true}]}], /* text= */ '', /* reviewersMutated= */ false, @@ -1171,9 +1181,13 @@ suite('gr-reply-dialog tests', () => { /* disabled= */ false, /* commentEditing= */ false )); - // Mock nonempty comment draft array, without seding comments. + }); + + test('_computeSendButtonDisabled_draftCommentsDoNotSend', () => { + const fn = element._computeSendButtonDisabled.bind(element); + // Mock nonempty comment draft array, without sending comments. assert.isTrue(fn( - /* buttonLabel= */ 'Send', + /* canBeStarted= */ false, /* draftCommentThreads= */ [{comments: [{__draft: true}]}], /* text= */ '', /* reviewersMutated= */ false, @@ -1182,9 +1196,13 @@ suite('gr-reply-dialog tests', () => { /* disabled= */ false, /* commentEditing= */ false )); + }); + + test('_computeSendButtonDisabled_changeMessage', () => { + const fn = element._computeSendButtonDisabled.bind(element); // Mock nonempty change message. assert.isFalse(fn( - /* buttonLabel= */ 'Send', + /* canBeStarted= */ false, /* draftCommentThreads= */ {}, /* text= */ 'test', /* reviewersMutated= */ false, @@ -1193,9 +1211,13 @@ suite('gr-reply-dialog tests', () => { /* disabled= */ false, /* commentEditing= */ false )); + }); + + test('_computeSendButtonDisabled_reviewersChanged', () => { + const fn = element._computeSendButtonDisabled.bind(element); // Mock reviewers mutated. assert.isFalse(fn( - /* buttonLabel= */ 'Send', + /* canBeStarted= */ false, /* draftCommentThreads= */ {}, /* text= */ '', /* reviewersMutated= */ true, @@ -1204,9 +1226,13 @@ suite('gr-reply-dialog tests', () => { /* disabled= */ false, /* commentEditing= */ false )); + }); + + test('_computeSendButtonDisabled_labelsChanged', () => { + const fn = element._computeSendButtonDisabled.bind(element); // Mock labels changed. assert.isFalse(fn( - /* buttonLabel= */ 'Send', + /* canBeStarted= */ false, /* draftCommentThreads= */ {}, /* text= */ '', /* reviewersMutated= */ false, @@ -1215,9 +1241,13 @@ suite('gr-reply-dialog tests', () => { /* disabled= */ false, /* commentEditing= */ false )); + }); + + test('_computeSendButtonDisabled_dialogDisabled', () => { + const fn = element._computeSendButtonDisabled.bind(element); // Whole dialog is disabled. assert.isTrue(fn( - /* buttonLabel= */ 'Send', + /* canBeStarted= */ false, /* draftCommentThreads= */ {}, /* text= */ '', /* reviewersMutated= */ false,