opendev/gerrit
Code Issues Proposed changes

Allow replies on patches known not to be latest

Browse Source 
The reply dialog checks whether there are newer patches than the
currently known latest patch so that users can avoid voting on
non-latest patches, which would typically be a no-op and have no effect.
Formerly, if the patch was known to be non-latest, the [Send] button
would be disabled completely and a red warning with a [Reload] button
shown.

With this change, the [Send] button is no longer disabled, and replies
to non-latest patches are allowed, but the warning still appears as an
advisory message, and additional wording appears when users attempt to
vote.

Bug: Issue 6612
Change-Id: Ib8f117dcc96b3fcba255e05a3cc72abb5fc1b14d
This commit is contained in:
Wyatt Allen
2017-12-18 17:18:31 -08:00
parent 0c523cb80e
commit 69dbc5f244
3 changed files with 19 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -266,7 +266,7 @@ limitations under the License.
<span
id="notLatestLabel"
hidden$="[[!_isState(knownLatestState, 'not-latest')]]">
Patch [[patchNum]] is not latest.
[[_computePatchSetWarning(patchNum, _labelsChanged)]]
<gr-button link on-tap="_reload">Reload</gr-button>
</span>
</div>
@@ -279,7 +279,7 @@ limitations under the License.
<gr-button
link
primary
disabled="[[_computeSendButtonDisabled(knownLatestState, _sendButtonLabel, diffDrafts, draft, _reviewersMutated, _labelsChanged, _includeComments)]]"
disabled="[[_computeSendButtonDisabled(_sendButtonLabel, diffDrafts, draft, _reviewersMutated, _labelsChanged, _includeComments)]]"
class="action send"
has-tooltip
title$="[[_computeSendButtonTooltip(canBeStarted)]]"

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

@@ -406,12 +406,6 @@
},
send(includeComments, startReview) {
if (this.knownLatestState === 'not-latest') {
this.fire('show-alert',
{message: 'Cannot reply to non-latest patch.'});
return Promise.resolve({});
}
const labels = this.$.labelScores.getLabelValues();
const obj = {
@@ -835,16 +829,21 @@
return savingComments ? 'saving' : '';
},
_computeSendButtonDisabled(knownLatestState, buttonLabel, drafts, text,
reviewersMutated, labelsChanged, includeComments) {
if (this._isState(knownLatestState, LatestPatchState.NOT_LATEST)) {
return true;
}
_computeSendButtonDisabled(buttonLabel, drafts, text, reviewersMutated,
labelsChanged, includeComments) {
if (buttonLabel === ButtonLabels.START_REVIEW) {
return false;
}
const hasDrafts = includeComments && Object.keys(drafts).length;
return !hasDrafts && !text.length && !reviewersMutated && !labelsChanged;
},
_computePatchSetWarning(patchNum, labelsChanged) {
let str = `Patch ${patchNum} is not latest.`;
if (labelsChanged) {
str += ' Voting on a non-latest patch will have no effect.';
}
return str;
},
});
})();

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

@@ -1083,21 +1083,20 @@ limitations under the License.
test('_computeSendButtonDisabled', () => {
const fn = element._computeSendButtonDisabled.bind(element);
assert.isTrue(fn('not-latest'));
assert.isFalse(fn('latest', 'Start review'));
assert.isTrue(fn('latest', 'Send', {}, '', false, false, false));
assert.isFalse(fn('Start review'));
assert.isTrue(fn('Send', {}, '', false, false, false));
// Mock nonempty comment draft array, with seding comments.
assert.isFalse(fn('latest', 'Send', {file: ['draft']}, '', false, false,
assert.isFalse(fn('Send', {file: ['draft']}, '', false, false,
true));
// Mock nonempty comment draft array, without seding comments.
assert.isTrue(fn('latest', 'Send', {file: ['draft']}, '', false, false,
assert.isTrue(fn('Send', {file: ['draft']}, '', false, false,
false));
// Mock nonempty change message.
assert.isFalse(fn('latest', 'Send', {}, 'test', false, false, false));
assert.isFalse(fn('Send', {}, 'test', false, false, false));
// Mock reviewers mutated.
assert.isFalse(fn('latest', 'Send', {}, '', true, false, false));
assert.isFalse(fn('Send', {}, '', true, false, false));
// Mock labels changed.
assert.isFalse(fn('latest', 'Send', {}, '', false, true, false));
assert.isFalse(fn('Send', {}, '', false, true, false));
});
});
</script>