Block revert submission if message is not edited
Stop users from reverting a submission without editing the message.
The check is also valid for reverting a single submission.
The check is only added to frontend as a first step. The final check
if needed must be done at the backend.
Change-Id: Ida3f9e07efef1c5ef08a3b5285ff6f8ad0ef40b2
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
index 9c1a527..6495adb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
@@ -827,7 +827,7 @@
.querySelector('gr-button[data-action-key="revert"]');
MockInteractions.tap(revertButton);
flush(() => {
- assert.equal(element.$.confirmRevertDialog.message, newRevertMsg);
+ assert.equal(element.$.confirmRevertDialog._message, newRevertMsg);
done();
});
});
@@ -866,7 +866,7 @@
'1234567890:random' + '\n' +
'23456:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...' +
'\n';
- assert.equal(confirmRevertDialog.message, expectedMsg);
+ assert.equal(confirmRevertDialog._message, expectedMsg);
const radioInputs = confirmRevertDialog.shadowRoot
.querySelectorAll('input[name="revertOptions"]');
MockInteractions.tap(radioInputs[0]);
@@ -874,7 +874,26 @@
expectedMsg = 'Revert "random commit message"\n\nThis reverts '
+ 'commit 2000.\n\nReason'
+ ' for revert: <INSERT REASONING HERE>\n';
- assert.equal(confirmRevertDialog.message, expectedMsg);
+ assert.equal(confirmRevertDialog._message, expectedMsg);
+ done();
+ });
+ });
+ });
+
+ test('submit fails if message is not edited', done => {
+ const revertButton = element.shadowRoot
+ .querySelector('gr-button[data-action-key="revert"]');
+ const confirmRevertDialog = element.$.confirmRevertDialog;
+ MockInteractions.tap(revertButton);
+ const fireStub = sandbox.stub(confirmRevertDialog, 'fire');
+ flush(() => {
+ const confirmButton = element.$.confirmRevertDialog.shadowRoot
+ .querySelector('gr-dialog')
+ .shadowRoot.querySelector('#confirm');
+ MockInteractions.tap(confirmButton);
+ flush(() => {
+ assert.isTrue(confirmRevertDialog._showErrorMessage);
+ assert.isFalse(fireStub.called);
done();
});
});
@@ -898,20 +917,20 @@
'Revert "random commit message"\n\nThis reverts '
+ 'commit 2000.\n\nReason'
+ ' for revert: <INSERT REASONING HERE>\n';
- assert.equal(confirmRevertDialog.message, revertSubmissionMsg);
+ assert.equal(confirmRevertDialog._message, revertSubmissionMsg);
const newRevertMsg = revertSubmissionMsg + 'random';
const newSingleChangeMsg = singleChangeMsg + 'random';
- confirmRevertDialog.message = newRevertMsg;
+ confirmRevertDialog._message = newRevertMsg;
MockInteractions.tap(radioInputs[0]);
flush(() => {
- assert.equal(confirmRevertDialog.message, singleChangeMsg);
- confirmRevertDialog.message = newSingleChangeMsg;
+ assert.equal(confirmRevertDialog._message, singleChangeMsg);
+ confirmRevertDialog._message = newSingleChangeMsg;
MockInteractions.tap(radioInputs[1]);
flush(() => {
- assert.equal(confirmRevertDialog.message, newRevertMsg);
+ assert.equal(confirmRevertDialog._message, newRevertMsg);
MockInteractions.tap(radioInputs[0]);
flush(() => {
- assert.equal(confirmRevertDialog.message, newSingleChangeMsg);
+ assert.equal(confirmRevertDialog._message, newSingleChangeMsg);
done();
});
});
@@ -932,6 +951,25 @@
]));
});
+ test('submit fails if message is not edited', done => {
+ const revertButton = element.shadowRoot
+ .querySelector('gr-button[data-action-key="revert"]');
+ const confirmRevertDialog = element.$.confirmRevertDialog;
+ MockInteractions.tap(revertButton);
+ const fireStub = sandbox.stub(confirmRevertDialog, 'fire');
+ flush(() => {
+ const confirmButton = element.$.confirmRevertDialog.shadowRoot
+ .querySelector('gr-dialog')
+ .shadowRoot.querySelector('#confirm');
+ MockInteractions.tap(confirmButton);
+ flush(() => {
+ assert.isTrue(confirmRevertDialog._showErrorMessage);
+ assert.isFalse(fireStub.called);
+ done();
+ });
+ });
+ });
+
test('confirm revert dialog shows one radio button', done => {
const revertButton = element.shadowRoot
.querySelector('gr-button[data-action-key="revert"]');
@@ -944,7 +982,9 @@
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);
+ assert.equal(confirmRevertDialog._message, msg);
+ const editedMsg = msg + 'hello';
+ confirmRevertDialog._message += 'hello';
const confirmButton = element.$.confirmRevertDialog.shadowRoot
.querySelector('gr-dialog')
.shadowRoot.querySelector('#confirm');
@@ -952,7 +992,8 @@
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);
+ assert.equal(fireActionStub.getCall(0).args[3].message,
+ editedMsg);
done();
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html
index 7bffe8a..0ad3ed3 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html
@@ -51,13 +51,22 @@
line-height: var(--line-height-mono);
width: 73ch; /* Add a char to account for the border. */
}
+ .error {
+ color: var(--error-text-color);
+ margin-bottom: var(--spacing-m);
+ }
</style>
<gr-dialog
confirm-label="Revert"
on-confirm="_handleConfirmTap"
on-cancel="_handleCancelTap">
- <div class="header" slot="header">Revert Merged Change</div>
+ <div class="header" slot="header">
+ Revert Merged Change
+ </div>
<div class="main" slot="main">
+ <div class="error" hidden$="[[!_showErrorMessage]]">
+ <span> A reason is required </span>
+ </div>
<div class="revertSubmissionLayout">
<input
name="revertOptions"
@@ -70,11 +79,12 @@
</label>
</div>
<template is="dom-if" if="[[_showRevertSubmission]]">
- <div on-click="_handleRevertSubmissionClicked" class="revertSubmissionLayout">
+ <div class="revertSubmissionLayout">
<input
name="revertOptions"
type="radio"
id="revertSubmission"
+ on-change="_handleRevertSubmissionClicked"
checked="[[_computeIfRevertSubmission(_revertType)]]">
<label for="revertSubmission" class="label revertSubmission">
Revert entire submission ([[changes.length]] Changes)
@@ -93,7 +103,7 @@
class="message"
autocomplete="on"
max-rows="15"
- bind-value="{{message}}"></iron-autogrow-textarea>
+ bind-value="{{_message}}"></iron-autogrow-textarea>
</template>
</gr-endpoint-decorator>
<template is="dom-if" if="[[_computeIfRevertSubmission(_revertType)]]">
@@ -105,7 +115,7 @@
class="message"
autocomplete="on"
max-rows="15"
- bind-value="{{message}}"></iron-autogrow-textarea>
+ bind-value="{{_message}}"></iron-autogrow-textarea>
</template>
</div>
</gr-dialog>
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
index 438440e..b07738a 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
@@ -51,7 +51,9 @@
static get properties() {
return {
- message: String,
+ /* The revert message updated by the user
+ The default value is set by the dialog */
+ _message: String,
_revertType: {
type: Number,
value: REVERT_TYPES.REVERT_SINGLE_CHANGE,
@@ -60,12 +62,31 @@
type: Boolean,
value: false,
},
+ /* List of changes with the same topic
+ value is empty if only a single change is being reverted */
changes: {
type: Array,
value() { return []; },
},
change: Object,
+ /* commit message is _latestCommitMessage coming from gr-change-view
+ read only and is not meant to be modified */
commitMessage: String,
+ _showErrorMessage: {
+ type: Boolean,
+ value: false,
+ },
+ /* store the default revert messages per revert type so that we can
+ check if user has edited the revert message or not */
+ _originalRevertMessages: {
+ type: Array,
+ value() { return []; },
+ },
+ // Store the actual messages that the user has edited
+ _revertMessages: {
+ type: Array,
+ value() { return []; },
+ },
};
}
@@ -90,12 +111,10 @@
onInputUpdate(change, commitMessage, changes) {
if (!change || !changes) return;
+ // The option to revert a single change is always available
this._populateRevertSingleChangeMessage(
change, commitMessage, change.current_revision);
- if (changes.length > 1) {
- this._populateRevertSubmissionMessage(
- change, changes);
- }
+ this._populateRevertSubmissionMessage(change, changes);
}
_populateRevertSingleChangeMessage(change, commitMessage, commitHash) {
@@ -108,14 +127,15 @@
}
const revertCommitText = `This reverts commit ${commitHash}.`;
- this.revertSingleChangeMessage =
- `${revertTitle}\n\n${revertCommitText}\n\n` +
+ this._message = `${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;
+ this._message = this._modifyRevertMsg(change, commitMessage,
+ this._message);
+ this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
+ this._showRevertSubmission = false;
+ this._revertMessages[this._revertType] = this._message;
+ this._originalRevertMessages[this._revertType] = this._message;
}
_getTrimmedChangeSubject(subject) {
@@ -124,9 +144,9 @@
return subject.substring(0, CHANGE_SUBJECT_LIMIT) + '...';
}
- _modifyRevertSubmissionMsg(change) {
- return this.$.jsAPI.modifyRevertSubmissionMsg(change,
- this.revertSubmissionMessage, this.commitMessage);
+ _modifyRevertSubmissionMsg(change, msg) {
+ return this.$.jsAPI.modifyRevertSubmissionMsg(change, msg,
+ this.commitMessage);
}
_populateRevertSubmissionMessage(change, changes) {
@@ -139,39 +159,43 @@
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';
+ this._message = revertTitle + '\n\n' + 'Reason for revert: <INSERT ' +
+ 'REASONING HERE>\n';
+ this._message += 'Reverted Changes:\n';
changes.forEach(change => {
- this.revertSubmissionMessage += change.change_id.substring(0, 10) + ':'
+ this._message += change.change_id.substring(0, 10) + ':'
+ this._getTrimmedChangeSubject(change.subject) + '\n';
});
- this.revertSubmissionMessage = this._modifyRevertSubmissionMsg(change);
- this.message = this.revertSubmissionMessage;
+ this._message = this._modifyRevertSubmissionMsg(change, this._message);
this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
+ this._revertMessages[this._revertType] = this._message;
+ this._originalRevertMessages[this._revertType] = this._message;
this._showRevertSubmission = true;
}
_handleRevertSingleChangeClicked() {
- if (this._revertType === REVERT_TYPES.REVERT_SINGLE_CHANGE) return;
- this.revertSubmissionMessage = this.message;
- this.message = this.revertSingleChangeMessage;
+ this._showErrorMessage = false;
+ this._revertMessages[REVERT_TYPES.REVERT_SUBMISSION] = this._message;
+ this._message = this._revertMessages[REVERT_TYPES.REVERT_SINGLE_CHANGE];
this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
}
_handleRevertSubmissionClicked() {
- if (this._revertType === REVERT_TYPES.REVERT_SUBMISSION) return;
+ this._showErrorMessage = false;
this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
- this.revertSingleChangeMessage = this.message;
- this.message = this.revertSubmissionMessage;
+ this._revertMessages[REVERT_TYPES.REVERT_SINGLE_CHANGE] = this._message;
+ this._message = this._revertMessages[REVERT_TYPES.REVERT_SUBMISSION];
}
_handleConfirmTap(e) {
e.preventDefault();
e.stopPropagation();
+ if (this._message === this._originalRevertMessages[this._revertType]) {
+ this._showErrorMessage = true;
+ return;
+ }
this.fire('confirm', {revertType: this._revertType,
- message: this.message}, {bubbles: false});
+ message: this._message}, {bubbles: false});
}
_handleCancelTap(e) {
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog_test.html b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog_test.html
index 1d28d32..76f5ecd 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog_test.html
@@ -47,7 +47,7 @@
teardown(() => sandbox.restore());
test('no match', () => {
- assert.isNotOk(element.message);
+ assert.isNotOk(element._message);
const alertStub = sandbox.stub();
element.addEventListener('show-alert', alertStub);
element._populateRevertSingleChangeMessage({},
@@ -56,47 +56,47 @@
});
test('single line', () => {
- assert.isNotOk(element.message);
+ assert.isNotOk(element._message);
element._populateRevertSingleChangeMessage({},
'one line commit\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "one line commit"\n\n' +
'This reverts commit abcd123.\n\n' +
'Reason for revert: <INSERT REASONING HERE>\n';
- assert.equal(element.message, expected);
+ assert.equal(element._message, expected);
});
test('multi line', () => {
- assert.isNotOk(element.message);
+ assert.isNotOk(element._message);
element._populateRevertSingleChangeMessage({},
'many lines\ncommit\n\nmessage\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "many lines"\n\n' +
'This reverts commit abcd123.\n\n' +
'Reason for revert: <INSERT REASONING HERE>\n';
- assert.equal(element.message, expected);
+ assert.equal(element._message, expected);
});
test('issue above change id', () => {
- assert.isNotOk(element.message);
+ assert.isNotOk(element._message);
element._populateRevertSingleChangeMessage({},
'much lines\nvery\n\ncommit\n\nBug: Issue 42\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "much lines"\n\n' +
'This reverts commit abcd123.\n\n' +
'Reason for revert: <INSERT REASONING HERE>\n';
- assert.equal(element.message, expected);
+ assert.equal(element._message, expected);
});
test('revert a revert', () => {
- assert.isNotOk(element.message);
+ assert.isNotOk(element._message);
element._populateRevertSingleChangeMessage({},
'Revert "one line commit"\n\nChange-Id: abcdefg\n',
'abcd123');
const expected = 'Revert "Revert "one line commit""\n\n' +
'This reverts commit abcd123.\n\n' +
'Reason for revert: <INSERT REASONING HERE>\n';
- assert.equal(element.message, expected);
+ assert.equal(element._message, expected);
});
});
</script>
