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>