opendev/gerrit
Code Issues Proposed changes

Merge "Show revert change and revert submission options instead of 2 buttons"

Browse Source
This commit is contained in:
Tao Zhou
2020-01-16 15:54:02 +00:00
committed by Gerrit Code Review
parent 69271a54f7 22a86edc7f
commit 4b33d9e27f
8 changed files with 341 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
View File

@@ -205,6 +205,9 @@ limitations under the License.
<gr-confirm-revert-dialog id="confirmRevertDialog"
class="confirmDialog"
on-confirm="_handleRevertDialogConfirm"
commit-message="[[commitMessage]]"
change="[[change]]"
changes="[[_revertChanges]]"
on-cancel="_handleConfirmDialogCancel"
hidden></gr-confirm-revert-dialog>
<gr-confirm-revert-submission-dialog id="confirmRevertSubmissionDialog"

49
polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
View File

@@ -192,6 +192,11 @@
const AWAIT_CHANGE_ATTEMPTS = 5;
const AWAIT_CHANGE_TIMEOUT_MS = 1000;
const REVERT_TYPES = {
REVERT_SINGLE_CHANGE: 1,
REVERT_SUBMISSION: 2,
};
/**
* @appliesMixin Gerrit.FireMixin
* @appliesMixin Gerrit.PatchSetMixin
@@ -421,6 +426,7 @@
type: Boolean,
value: true,
},
_revertChanges: Array,
};
}
@@ -915,16 +921,13 @@
return null;
}
_modifyRevertMsg() {
return this.$.jsAPI.modifyRevertMsg(this.change,
this.$.confirmRevertDialog.message, this.commitMessage);
}
showRevertDialog() {
this.$.confirmRevertDialog.populateRevertMessage(
this.commitMessage, this.change.current_revision);
this.$.confirmRevertDialog.message = this._modifyRevertMsg();
this._showActionDialog(this.$.confirmRevertDialog);
const query = 'submissionid:' + this.change.submission_id;
this.$.restAPI.getChanges('', query)
.then(changes => {
this._revertChanges = changes;
this._showActionDialog(this.$.confirmRevertDialog);
});
}
showRevertSubmissionDialog() {
@@ -932,7 +935,7 @@
this.$.restAPI.getChanges('', query)
.then(changes => {
this.$.confirmRevertSubmissionDialog.
populateRevertSubmissionMessage(
_populateRevertSubmissionMessage(
this.commitMessage, this.change, changes);
this._showActionDialog(this.$.confirmRevertSubmissionDialog);
});
@@ -1143,20 +1146,24 @@
);
}
_handleRevertDialogConfirm() {
_handleRevertDialogConfirm(e) {
const revertType = e.detail.revertType;
const message = e.detail.message;
const el = this.$.confirmRevertDialog;
this.$.overlay.close();
el.hidden = true;
this._fireAction('/revert', this.actions.revert, false,
{message: el.message});
}
_handleRevertSubmissionDialogConfirm() {
const el = this.$.confirmRevertSubmissionDialog;
this.$.overlay.close();
el.hidden = true;
this._fireAction('/revert_submission', this.actions.revert_submission,
false, {message: el.message});
switch (revertType) {
case REVERT_TYPES.REVERT_SINGLE_CHANGE:
this._fireAction('/revert', this.actions.revert, false,
{message});
break;
case REVERT_TYPES.REVERT_SUBMISSION:
this._fireAction('/revert_submission', this.actions.revert_submission,
false, {message});
break;
default:
console.error('invalid revert type');
}
}
_handleAbandonDialogConfirm() {

172
polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
View File

@@ -37,6 +37,7 @@ limitations under the License.
</test-fixture>
<script>
// TODO(dhruvsri): remove use of _populateRevertMessage as it's private
suite('gr-change-actions tests', () => {
let element;
let sandbox;
@@ -795,12 +796,12 @@ limitations under the License.
});
suite('revert change', () => {
let alertStub;
let fireActionStub;
setup(() => {
fireActionStub = sandbox.stub(element, '_fireAction');
alertStub = sandbox.stub(window, 'alert');
element.commitMessage = 'random commit message';
element.change.current_revision = 'abcdef';
element.actions = {
revert: {
method: 'POST',
@@ -813,50 +814,149 @@ limitations under the License.
});
test('revert change with plugin hook', done => {
const newRevertMsg = 'Modified revert msg';
sandbox.stub(element.$.confirmRevertDialog, '_modifyRevertMsg',
() => newRevertMsg);
element.change = {
current_revision: 'abc1234',
};
const newRevertMsg = 'Modified revert msg';
sandbox.stub(element, '_modifyRevertMsg',
() => newRevertMsg);
sandbox.stub(element.$.confirmRevertDialog, 'populateRevertMessage',
() => 'original msg');
sandbox.stub(element.$.confirmRevertDialog,
'_populateRevertSubmissionMessage', () => 'original msg');
flush(() => {
const revertButton =
element.$$('gr-button[data-action-key="revert"]');
const revertButton = element.shadowRoot
.querySelector('gr-button[data-action-key="revert"]');
MockInteractions.tap(revertButton);
assert.equal(element.$.confirmRevertDialog.message, newRevertMsg);
done();
flush(() => {
assert.equal(element.$.confirmRevertDialog.message, newRevertMsg);
done();
});
});
});
test('works', () => {
element.change = {
current_revision: 'abc1234',
};
sandbox.stub(element.$.confirmRevertDialog, 'populateRevertMessage',
() => 'original msg');
const revertButton = element.$$('gr-button[data-action-key="revert"]');
MockInteractions.tap(revertButton);
suite('revert change submitted together', () => {
setup(() => {
element.change = {
submission_id: '199',
current_revision: '2000',
};
sandbox.stub(element.$.restAPI, 'getChanges')
.returns(Promise.resolve([
{change_id: '12345678901234', topic: 'T', subject: 'random'},
{change_id: '23456', topic: 'T', subject: 'a'.repeat(100)},
]));
});
element.$.confirmRevertDialog.message = 'foo message';
element._handleRevertDialogConfirm();
assert.notOk(alertStub.called);
test('confirm revert dialog shows both options', done => {
const revertButton = element.shadowRoot
.querySelector('gr-button[data-action-key="revert"]');
MockInteractions.tap(revertButton);
flush(() => {
const confirmRevertDialog = element.$.confirmRevertDialog;
const revertSingleChangeLabel = confirmRevertDialog
.shadowRoot.querySelector('.revertSingleChange');
const revertSubmissionLabel = confirmRevertDialog.
shadowRoot.querySelector('.revertSubmission');
assert(revertSingleChangeLabel.innerText.trim() ===
'Revert single change');
assert(revertSubmissionLabel.innerText.trim() ===
'Revert entire submission (2 Changes)');
let expectedMsg = 'Revert submission 199' + '\n\n' +
'Reason for revert: <INSERT REASONING HERE>' + '\n' +
'Reverted Changes:' + '\n' +
'1234567890:random' + '\n' +
'23456:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...' +
'\n';
assert.equal(confirmRevertDialog.message, expectedMsg);
const radioInputs = confirmRevertDialog.shadowRoot
.querySelectorAll('input[name="revertOptions"]');
MockInteractions.tap(radioInputs[0]);
flush(() => {
expectedMsg = 'Revert "random commit message"\n\nThis reverts '
+ 'commit 2000.\n\nReason'
+ ' for revert: <INSERT REASONING HERE>\n';
assert.equal(confirmRevertDialog.message, expectedMsg);
done();
});
});
});
const action = {
__key: 'revert',
__type: 'change',
__primary: false,
enabled: true,
label: 'Revert',
method: 'POST',
title: 'Revert the change',
};
assert.deepEqual(fireActionStub.lastCall.args, [
'/revert', action, false, {
message: 'foo message',
}]);
test('message modification is retained on switching', done => {
const revertButton = element.shadowRoot
.querySelector('gr-button[data-action-key="revert"]');
const confirmRevertDialog = element.$.confirmRevertDialog;
MockInteractions.tap(revertButton);
flush(() => {
const radioInputs = confirmRevertDialog.shadowRoot
.querySelectorAll('input[name="revertOptions"]');
const revertSubmissionMsg = 'Revert submission 199' + '\n\n' +
'Reason for revert: <INSERT REASONING HERE>' + '\n' +
'Reverted Changes:' + '\n' +
'1234567890:random' + '\n' +
'23456:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...' +
'\n';
const singleChangeMsg =
'Revert "random commit message"\n\nThis reverts '
+ 'commit 2000.\n\nReason'
+ ' for revert: <INSERT REASONING HERE>\n';
assert.equal(confirmRevertDialog.message, revertSubmissionMsg);
const newRevertMsg = revertSubmissionMsg + 'random';
const newSingleChangeMsg = singleChangeMsg + 'random';
confirmRevertDialog.message = newRevertMsg;
MockInteractions.tap(radioInputs[0]);
flush(() => {
assert.equal(confirmRevertDialog.message, singleChangeMsg);
confirmRevertDialog.message = newSingleChangeMsg;
MockInteractions.tap(radioInputs[1]);
flush(() => {
assert.equal(confirmRevertDialog.message, newRevertMsg);
MockInteractions.tap(radioInputs[0]);
flush(() => {
assert.equal(confirmRevertDialog.message, newSingleChangeMsg);
done();
});
});
});
});
});
});
suite('revert single change', () => {
setup(() => {
element.change = {
submission_id: '199',
current_revision: '2000',
};
sandbox.stub(element.$.restAPI, 'getChanges')
.returns(Promise.resolve([
{change_id: '12345678901234', topic: 'T', subject: 'random'},
]));
});
test('confirm revert dialog shows one radio button', done => {
const revertButton = element.shadowRoot
.querySelector('gr-button[data-action-key="revert"]');
MockInteractions.tap(revertButton);
flush(() => {
const confirmRevertDialog = element.$.confirmRevertDialog;
const radioInputs = confirmRevertDialog.shadowRoot
.querySelectorAll('input[name="revertOptions"]');
assert.equal(radioInputs.length, 1);
const msg = 'Revert "random commit message"\n\n'
+ 'This reverts commit 2000.\n\nReason '
+ 'for revert: <INSERT REASONING HERE>\n';
assert.equal(confirmRevertDialog.message, msg);
const confirmButton = element.$.confirmRevertDialog.shadowRoot
.querySelector('gr-dialog')
.shadowRoot.querySelector('#confirm');
MockInteractions.tap(confirmButton);
flush(() => {
assert.equal(fireActionStub.getCall(0).args[0], '/revert');
assert.equal(fireActionStub.getCall(0).args[1].__key, 'revert');
assert.equal(fireActionStub.getCall(0).args[3].message, msg);
done();
});
});
});
});
});

49
polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html
View File

@@ -21,6 +21,7 @@ limitations under the License.
<link rel="import" href="../../shared/gr-dialog/gr-dialog.html">
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.html">
<link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
<dom-module id="gr-confirm-revert-dialog">
<template>
@@ -37,6 +38,13 @@ limitations under the License.
display: block;
width: 100%;
}
.revertSubmissionLayout {
display: flex;
}
.label {
margin-left: var(--spacing-m);
margin-bottom: var(--spacing-m);
}
iron-autogrow-textarea {
font-family: var(--monospace-font-family);
font-size: var(--font-size-mono);
@@ -50,7 +58,45 @@ limitations under the License.
on-cancel="_handleCancelTap">
<div class="header" slot="header">Revert Merged Change</div>
<div class="main" slot="main">
<div class="revertSubmissionLayout">
<input
name="revertOptions"
type="radio"
id="revertSingleChange"
on-change="_handleRevertSingleChangeClicked"
checked="[[_computeIfSingleRevert(_revertType)]]">
<label for="revertSingleChange" class="label revertSingleChange">
Revert single change
</label>
</div>
<template is="dom-if" if="[[_showRevertSubmission]]">
<div on-click="_handleRevertSubmissionClicked" class="revertSubmissionLayout">
<input
name="revertOptions"
type="radio"
id="revertSubmission"
checked="[[_computeIfRevertSubmission(_revertType)]]">
<label for="revertSubmission" class="label revertSubmission">
Revert entire submission ([[changes.length]] Changes)
</label>
</template>
<gr-endpoint-decorator name="confirm-revert-change">
<!-- Duplicating the text-area as a plugin in the case of a single
revert will override the entire textarea which should not happen
for multiple revert -->
<template is="dom-if" if="[[_computeIfSingleRevert(_revertType)]]">
<label for="messageInput">
Revert Commit Message
</label>
<iron-autogrow-textarea
id="messageInput"
class="message"
autocomplete="on"
max-rows="15"
bind-value="{{message}}"></iron-autogrow-textarea>
</template>
</gr-endpoint-decorator>
<template is="dom-if" if="[[_computeIfRevertSubmission(_revertType)]]">
<label for="messageInput">
Revert Commit Message
</label>
@@ -60,9 +106,10 @@ limitations under the License.
autocomplete="on"
max-rows="15"
bind-value="{{message}}"></iron-autogrow-textarea>
</gr-endpoint-decorator>
</template>
</div>
</gr-dialog>
<gr-js-api-interface id="jsAPI"></gr-js-api-interface>
</template>
<script src="gr-confirm-revert-dialog.js"></script>
</dom-module>

117
polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
View File

@@ -19,6 +19,13 @@
const ERR_COMMIT_NOT_FOUND =
'Unable to find the commit hash of this change.';
const CHANGE_SUBJECT_LIMIT = 50;
// TODO(dhruvsri): clean up repeated definitions after moving to js modules
const REVERT_TYPES = {
REVERT_SINGLE_CHANGE: 1,
REVERT_SUBMISSION: 2,
};
/**
* @appliesMixin Gerrit.FireMixin
@@ -45,12 +52,55 @@
static get properties() {
return {
message: String,
_revertType: {
type: Number,
value: REVERT_TYPES.REVERT_SINGLE_CHANGE,
},
_showRevertSubmission: {
type: Boolean,
value: false,
},
changes: {
type: Array,
value() { return []; },
},
change: Object,
commitMessage: String,
};
}
populateRevertMessage(message, commitHash) {
static get observers() {
return [
'onInputUpdate(change, commitMessage, changes)',
];
}
_computeIfSingleRevert(revertType) {
return revertType === REVERT_TYPES.REVERT_SINGLE_CHANGE;
}
_computeIfRevertSubmission(revertType) {
return revertType === REVERT_TYPES.REVERT_SUBMISSION;
}
_modifyRevertMsg(change, commitMessage, message) {
return this.$.jsAPI.modifyRevertMsg(change,
message, commitMessage);
}
onInputUpdate(change, commitMessage, changes) {
if (!change || !changes) return;
this._populateRevertSingleChangeMessage(
change, commitMessage, change.current_revision);
if (changes.length > 1) {
this._populateRevertSubmissionMessage(
change, changes);
}
}
_populateRevertSingleChangeMessage(change, commitMessage, commitHash) {
// Figure out what the revert title should be.
const originalTitle = message.split('\n')[0];
const originalTitle = (commitMessage || '').split('\n')[0];
const revertTitle = `Revert "${originalTitle}"`;
if (!commitHash) {
this.fire('show-alert', {message: ERR_COMMIT_NOT_FOUND});
@@ -58,20 +108,77 @@
}
const revertCommitText = `This reverts commit ${commitHash}.`;
this.message = `${revertTitle}\n\n${revertCommitText}\n\n` +
this.revertSingleChangeMessage =
`${revertTitle}\n\n${revertCommitText}\n\n` +
`Reason for revert: <INSERT REASONING HERE>\n`;
// This is to give plugins a chance to update message
this.revertSingleChangeMessage =
this._modifyRevertMsg(change, commitMessage,
this.revertSingleChangeMessage);
this.message = this.revertSingleChangeMessage;
}
_getTrimmedChangeSubject(subject) {
if (!subject) return '';
if (subject.length < CHANGE_SUBJECT_LIMIT) return subject;
return subject.substring(0, CHANGE_SUBJECT_LIMIT) + '...';
}
_modifyRevertSubmissionMsg(change) {
return this.$.jsAPI.modifyRevertSubmissionMsg(change,
this.revertSubmissionMessage, this.commitMessage);
}
_populateRevertSubmissionMessage(change, changes) {
// Follow the same convention of the revert
const commitHash = change.current_revision;
if (!commitHash) {
this.fire('show-alert', {message: ERR_COMMIT_NOT_FOUND});
return;
}
if (!changes || changes.length <= 1) return;
const submissionId = change.submission_id;
const revertTitle = 'Revert submission ' + submissionId;
this.changes = changes;
this.revertSubmissionMessage = revertTitle + '\n\n' +
'Reason for revert: <INSERT REASONING HERE>\n';
this.revertSubmissionMessage += 'Reverted Changes:\n';
changes.forEach(change => {
this.revertSubmissionMessage += change.change_id.substring(0, 10) + ':'
+ this._getTrimmedChangeSubject(change.subject) + '\n';
});
this.revertSubmissionMessage = this._modifyRevertSubmissionMsg(change);
this.message = this.revertSubmissionMessage;
this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
this._showRevertSubmission = true;
}
_handleRevertSingleChangeClicked() {
if (this._revertType === REVERT_TYPES.REVERT_SINGLE_CHANGE) return;
this.revertSubmissionMessage = this.message;
this.message = this.revertSingleChangeMessage;
this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
}
_handleRevertSubmissionClicked() {
if (this._revertType === REVERT_TYPES.REVERT_SUBMISSION) return;
this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
this.revertSingleChangeMessage = this.message;
this.message = this.revertSubmissionMessage;
}
_handleConfirmTap(e) {
e.preventDefault();
e.stopPropagation();
this.fire('confirm', null, {bubbles: false});
this.fire('confirm', {revertType: this._revertType,
message: this.message}, {bubbles: false});
}
_handleCancelTap(e) {
e.preventDefault();
e.stopPropagation();
this.fire('cancel', null, {bubbles: false});
this.fire('cancel', {revertType: this._revertType},
{bubbles: false});
}
}

11
polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog_test.html
View File

@@ -50,13 +50,14 @@ limitations under the License.
assert.isNotOk(element.message);
const alertStub = sandbox.stub();
element.addEventListener('show-alert', alertStub);
element.populateRevertMessage('not a commitHash in sight', undefined);
element._populateRevertSingleChangeMessage({},
'not a commitHash in sight', undefined);
assert.isTrue(alertStub.calledOnce);
});
test('single line', () => {
assert.isNotOk(element.message);
element.populateRevertMessage(
element._populateRevertSingleChangeMessage({},
'one line commit\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "one line commit"\n\n' +
@@ -67,7 +68,7 @@ limitations under the License.
test('multi line', () => {
assert.isNotOk(element.message);
element.populateRevertMessage(
element._populateRevertSingleChangeMessage({},
'many lines\ncommit\n\nmessage\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "many lines"\n\n' +
@@ -78,7 +79,7 @@ limitations under the License.
test('issue above change id', () => {
assert.isNotOk(element.message);
element.populateRevertMessage(
element._populateRevertSingleChangeMessage({},
'much lines\nvery\n\ncommit\n\nBug: Issue 42\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "much lines"\n\n' +
@@ -89,7 +90,7 @@ limitations under the License.
test('revert a revert', () => {
assert.isNotOk(element.message);
element.populateRevertMessage(
element._populateRevertSingleChangeMessage({},
'Revert "one line commit"\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "Revert "one line commit""\n\n' +

6
polygerrit-ui/app/elements/change/gr-confirm-revert-submission-dialog/gr-confirm-revert-submission-dialog.js
View File

@@ -50,7 +50,7 @@
};
}
getTrimmedChangeSubject(subject) {
_getTrimmedChangeSubject(subject) {
if (!subject) return '';
if (subject.length < CHANGE_SUBJECT_LIMIT) return subject;
return subject.substring(0, CHANGE_SUBJECT_LIMIT) + '...';
@@ -61,7 +61,7 @@
this.message, this.commitMessage);
}
populateRevertSubmissionMessage(message, change, changes) {
_populateRevertSubmissionMessage(message, change, changes) {
// Follow the same convention of the revert
const commitHash = change.current_revision;
if (!commitHash) {
@@ -77,7 +77,7 @@
changes = changes || [];
changes.forEach(change => {
this.message += change.change_id.substring(0, 10) + ': ' +
this.getTrimmedChangeSubject(change.subject) + '\n';
this._getTrimmedChangeSubject(change.subject) + '\n';
});
this.message = this._modifyRevertSubmissionMsg(change);
}

10
polygerrit-ui/app/elements/change/gr-confirm-revert-submission-dialog/gr-confirm-revert-submission-dialog_test.html
View File

@@ -51,7 +51,7 @@ limitations under the License.
assert.isNotOk(element.message);
const alertStub = sandbox.stub();
element.addEventListener('show-alert', alertStub);
element.populateRevertSubmissionMessage(
element._populateRevertSubmissionMessage(
'not a commitHash in sight'
);
assert.isTrue(alertStub.calledOnce);
@@ -59,7 +59,7 @@ limitations under the License.
test('single line', () => {
assert.isNotOk(element.message);
element.populateRevertSubmissionMessage(
element._populateRevertSubmissionMessage(
'one line commit\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert submission\n\n' +
@@ -69,7 +69,7 @@ limitations under the License.
test('multi line', () => {
assert.isNotOk(element.message);
element.populateRevertSubmissionMessage(
element._populateRevertSubmissionMessage(
'many lines\ncommit\n\nmessage\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert submission\n\n' +
@@ -79,7 +79,7 @@ limitations under the License.
test('issue above change id', () => {
assert.isNotOk(element.message);
element.populateRevertSubmissionMessage(
element._populateRevertSubmissionMessage(
'test \nvery\n\ncommit\n\nBug: Issue 42\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert submission\n\n' +
@@ -89,7 +89,7 @@ limitations under the License.
test('revert a revert', () => {
assert.isNotOk(element.message);
element.populateRevertSubmissionMessage(
element._populateRevertSubmissionMessage(
'Revert "one line commit"\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert submission\n\n' +