Merge "Block revert submission if message is not edited"

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

@@ -827,7 +827,7 @@ limitations under the License.
               .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 @@ limitations under the License.
               '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 @@ limitations under the License.
               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 @@ limitations under the License.
             '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);
+              assert.equal(confirmRevertDialog._message, singleChangeMsg);
-              confirmRevertDialog.message = newSingleChangeMsg;
+              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 @@ limitations under the License.
               ]));
         });
 
+        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 @@ limitations under the License.
             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 @@ limitations under the License.
             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();
             });
           });

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

@@ -51,13 +51,22 @@ limitations under the License.
         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 @@ limitations under the License.
           </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 @@ limitations under the License.
               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 @@ limitations under the License.
               class="message"
               autocomplete="on"
               max-rows="15"
-              bind-value="{{message}}"></iron-autogrow-textarea>
+              bind-value="{{_message}}"></iron-autogrow-textarea>
         </template>
       </div>
     </gr-dialog>

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 =
+      this._message = `${revertTitle}\n\n${revertCommitText}\n\n` +
-          `${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._message = this._modifyRevertMsg(change, commitMessage,
-          this._modifyRevertMsg(change, commitMessage,
+          this._message);
-              this.revertSingleChangeMessage);
+      this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
-      this.message = this.revertSingleChangeMessage;
+      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) {
+    _modifyRevertSubmissionMsg(change, msg) {
-      return this.$.jsAPI.modifyRevertSubmissionMsg(change,
+      return this.$.jsAPI.modifyRevertSubmissionMsg(change, msg,
-          this.revertSubmissionMessage, this.commitMessage);
+          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._message = revertTitle + '\n\n' + 'Reason for revert: <INSERT ' +
-      this.revertSubmissionMessage = revertTitle + '\n\n' +
+        'REASONING HERE>\n';
-          'Reason for revert: <INSERT REASONING HERE>\n';
+      this._message += 'Reverted Changes:\n';
-      this.revertSubmissionMessage += '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._modifyRevertSubmissionMsg(change, this._message);
-      this.message = this.revertSubmissionMessage;
       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._showErrorMessage = false;
-      this.revertSubmissionMessage = this.message;
+      this._revertMessages[REVERT_TYPES.REVERT_SUBMISSION] = this._message;
-      this.message = this.revertSingleChangeMessage;
+      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._revertMessages[REVERT_TYPES.REVERT_SINGLE_CHANGE] = this._message;
-      this.message = this.revertSubmissionMessage;
+      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) {

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

@@ -47,7 +47,7 @@ limitations under the License.
     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 @@ limitations under the License.
     });
 
     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>