Control auto-publishing comments on push with preference

Change-Id: Ibf6c61d8c34ee790ca7ed29a0fad0b5e81910f09

Documentation/intro-user.txt

@@ -773,6 +773,12 @@ changes that are created from the web UI (e.g. by the `Create Change`
 and `Edit Config` buttons on the project screen, and the `Follow-Up`
 button on the change screen).
 
+- [[publish-comments-on-push]]`Publish Draft Comments When a Change Is Updated by Push`:
++
+Whether to publish any outstanding draft comments by default when pushing
+updates to open changes. This preference just sets the default; the behavior can
+still be overridden using a link:user-upload.html#publish-comments[push option].
+
 - [[use-flash]]`Use Flash Clipboard Widget`:
 +
 Whether the Flash clipboard widget should be used. If enabled and the Flash

Documentation/rest-api-accounts.txt

@@ -1248,6 +1248,7 @@ any account.
     "size_bar_in_change_table": true,
     "review_category_strategy": "ABBREV",
     "mute_common_path_prefixes": true,
+    "publish_comments_on_push": true,
     "default_base_for_merges": "FIRST_PARENT",
     "my": [
       {
@@ -1361,6 +1362,7 @@ link:#preferences-info[PreferencesInfo] entity.
     "size_bar_in_change_table": true,
     "review_category_strategy": "NAME",
     "diff_view": "SIDE_BY_SIDE",
+    "publish_comments_on_push": true,
     "mute_common_path_prefixes": true,
     "my": [
       {
@@ -2643,6 +2645,9 @@ Allowed values are `ENABLED`, `CC_ON_OWN_COMMENTS`, `DISABLED`.
 The base which should be pre-selected in the 'Diff Against' drop-down
 list when the change screen is opened for a merge commit.
 Allowed values are `AUTO_MERGE` and `FIRST_PARENT`.
+|`publish_comments_on_push`     |not set if `false`|
+Whether to link:user-upload.html#publish-comments[publish draft comments] on
+push by default.
 |============================================
 
 [[preferences-input]]

Documentation/rest-api-config.txt

@@ -1065,6 +1065,7 @@ PreferencesInfo] is returned.
     "size_bar_in_change_table": true,
     "review_category_strategy": "NONE",
     "mute_common_path_prefixes": true,
+    "publish_comments_on_push": true,
     "my": [
       {
         "url": "#/dashboard/self",
@@ -1147,6 +1148,7 @@ PreferencesInfo] is returned.
     "size_bar_in_change_table": true,
     "review_category_strategy": "NONE",
     "mute_common_path_prefixes": true,
+    "publish_comments_on_push": true,
     "my": [
       {
         "url": "#/dashboard/self",

Documentation/user-upload.txt

@@ -272,6 +272,11 @@ If you have draft comments on the change(s) that are updated by the push, the
   git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%publish-comments
 ----
 
+The default for this option can be set as a
+link:intro-user.html#publish-comments-on-push[user preference]. If the
+preference is set so the default behavior is to publish, this can be overridden
+with the `no-publish-comments` (or `np`) option.
+
 [[review_labels]]
 ==== Review Labels
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -46,6 +46,7 @@ import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.ProjectWatchInfo;
@@ -82,6 +83,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.transport.PushResult;
 import org.eclipse.jgit.transport.RefSpec;
 import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.junit.After;
 import org.junit.AfterClass;
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -118,6 +120,14 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     grant(Permission.LABEL + "Patch-Set-Lock", project, "refs/heads/*");
   }
 
+  @After
+  public void tearDown() throws Exception {
+    setApiUser(admin);
+    GeneralPreferencesInfo prefs = gApi.accounts().id(admin.id.get()).getPreferences();
+    prefs.publishCommentsOnPush = false;
+    gApi.accounts().id(admin.id.get()).setPreferences(prefs);
+  }
+
   protected void selectProtocol(Protocol p) throws Exception {
     String url;
     switch (p) {
@@ -1572,6 +1582,37 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertThat(getLastMessage(id2)).isEqualTo("Uploaded patch set 2.\n\n(1 comment)");
   }
 
+  @Test
+  public void publishCommentsOnPushWithPreference() throws Exception {
+    PushOneCommit.Result r = createChange();
+    addDraft(r.getChangeId(), r.getCommit().name(), newDraft(FILE_NAME, 1, "comment1"));
+    r = amendChange(r.getChangeId());
+
+    assertThat(getPublishedComments(r.getChangeId())).isEmpty();
+
+    GeneralPreferencesInfo prefs = gApi.accounts().id(admin.id.get()).getPreferences();
+    prefs.publishCommentsOnPush = true;
+    gApi.accounts().id(admin.id.get()).setPreferences(prefs);
+
+    r = amendChange(r.getChangeId());
+    assertThat(getPublishedComments(r.getChangeId()).stream().map(c -> c.message))
+        .containsExactly("comment1");
+  }
+
+  @Test
+  public void publishCommentsOnPushOverridingPreference() throws Exception {
+    PushOneCommit.Result r = createChange();
+    addDraft(r.getChangeId(), r.getCommit().name(), newDraft(FILE_NAME, 1, "comment1"));
+
+    GeneralPreferencesInfo prefs = gApi.accounts().id(admin.id.get()).getPreferences();
+    prefs.publishCommentsOnPush = true;
+    gApi.accounts().id(admin.id.get()).setPreferences(prefs);
+
+    r = amendChange(r.getChangeId(), "refs/for/master%no-publish-comments");
+
+    assertThat(getPublishedComments(r.getChangeId())).isEmpty();
+  }
+
   private DraftInput newDraft(String path, int line, String message) {
     DraftInput d = new DraftInput();
     d.path = path;

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java

@@ -157,6 +157,7 @@ public class GeneralPreferencesInfo {
   public EmailStrategy emailStrategy;
   public EmailFormat emailFormat;
   public DefaultBase defaultBaseForMerges;
+  public Boolean publishCommentsOnPush;
 
   public boolean isShowInfoInReviewCategory() {
     return getReviewCategoryStrategy() != ReviewCategoryStrategy.NONE;
@@ -225,6 +226,7 @@ public class GeneralPreferencesInfo {
     p.muteCommonPathPrefixes = true;
     p.signedOffBy = false;
     p.defaultBaseForMerges = DefaultBase.FIRST_PARENT;
+    p.publishCommentsOnPush = false;
     return p;
   }
 }

gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java

@@ -148,6 +148,9 @@ public class GeneralPreferences extends JavaScriptObject {
 
   private native String defaultBaseForMergesRaw() /*-{ return this.default_base_for_merges }-*/;
 
+  public final native boolean
+      publishCommentsOnPush() /*-{ return this.publish_comments_on_push || false }-*/;
+
   public final native JsArray<TopMenuItem> my() /*-{ return this.my; }-*/;
 
   public final native void changesPerPage(int n) /*-{ this.changes_per_page = n }-*/;
@@ -224,6 +227,9 @@ public class GeneralPreferences extends JavaScriptObject {
 
   private native void defaultBaseForMergesRaw(String b) /*-{ this.default_base_for_merges = b }-*/;
 
+  public final native void publishCommentsOnPush(
+      boolean p) /*-{ this.publish_comments_on_push = p }-*/;
+
   public final void setMyMenus(List<TopMenuItem> myMenus) {
     initMy();
     for (TopMenuItem n : myMenus) {

gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java

@@ -67,6 +67,8 @@ public interface AccountConstants extends Constants {
 
   String signedOffBy();
 
+  String publishCommentsOnPush();
+
   String myMenu();
 
   String myMenuInfo();

gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties

@@ -38,6 +38,7 @@ showSizeBarInChangeTable = Show Change Sizes As Colored Bars
 showLegacycidInChangeTable = Show Change Number In Changes Table
 muteCommonPathPrefixes = Mute Common Path Prefixes In File List
 signedOffBy = Insert Signed-off-by Footer For Inline Edit Changes
+publishCommentsOnPush = Publish Draft Comments When a Change Is Updated by Push
 myMenu = My Menu
 myMenuInfo = \
   Menu items for the 'My' top level menu. \

gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java

@@ -55,6 +55,7 @@ public class MyPreferencesScreen extends SettingsScreen {
   private CheckBox legacycidInChangeTable;
   private CheckBox muteCommonPathPrefixes;
   private CheckBox signedOffBy;
+  private CheckBox publishCommentsOnPush;
   private ListBox maximumPageSize;
   private ListBox dateFormat;
   private ListBox timeFormat;
@@ -161,9 +162,10 @@ public class MyPreferencesScreen extends SettingsScreen {
     legacycidInChangeTable = new CheckBox(Util.C.showLegacycidInChangeTable());
     muteCommonPathPrefixes = new CheckBox(Util.C.muteCommonPathPrefixes());
     signedOffBy = new CheckBox(Util.C.signedOffBy());
+    publishCommentsOnPush = new CheckBox(Util.C.publishCommentsOnPush());
 
     boolean flashClippy = !UserAgent.hasJavaScriptClipboard() && UserAgent.Flash.isInstalled();
-    final Grid formGrid = new Grid(14 + (flashClippy ? 1 : 0), 2);
+    final Grid formGrid = new Grid(15 + (flashClippy ? 1 : 0), 2);
 
     int row = 0;
 
@@ -223,6 +225,10 @@ public class MyPreferencesScreen extends SettingsScreen {
     formGrid.setWidget(row, fieldIdx, signedOffBy);
     row++;
 
+    formGrid.setText(row, labelIdx, "");
+    formGrid.setWidget(row, fieldIdx, publishCommentsOnPush);
+    row++;
+
     if (flashClippy) {
       formGrid.setText(row, labelIdx, "");
       formGrid.setWidget(row, fieldIdx, useFlashClipboard);
@@ -257,6 +263,7 @@ public class MyPreferencesScreen extends SettingsScreen {
     e.listenTo(legacycidInChangeTable);
     e.listenTo(muteCommonPathPrefixes);
     e.listenTo(signedOffBy);
+    e.listenTo(publishCommentsOnPush);
     e.listenTo(diffView);
     e.listenTo(reviewCategoryStrategy);
     e.listenTo(emailStrategy);
@@ -295,6 +302,7 @@ public class MyPreferencesScreen extends SettingsScreen {
     legacycidInChangeTable.setEnabled(on);
     muteCommonPathPrefixes.setEnabled(on);
     signedOffBy.setEnabled(on);
+    publishCommentsOnPush.setEnabled(on);
     reviewCategoryStrategy.setEnabled(on);
     diffView.setEnabled(on);
     emailStrategy.setEnabled(on);
@@ -320,6 +328,7 @@ public class MyPreferencesScreen extends SettingsScreen {
     legacycidInChangeTable.setValue(p.legacycidInChangeTable());
     muteCommonPathPrefixes.setValue(p.muteCommonPathPrefixes());
     signedOffBy.setValue(p.signedOffBy());
+    publishCommentsOnPush.setValue(p.publishCommentsOnPush());
     setListBox(
         reviewCategoryStrategy,
         GeneralPreferencesInfo.ReviewCategoryStrategy.NONE,
@@ -412,6 +421,7 @@ public class MyPreferencesScreen extends SettingsScreen {
     p.legacycidInChangeTable(legacycidInChangeTable.getValue());
     p.muteCommonPathPrefixes(muteCommonPathPrefixes.getValue());
     p.signedOffBy(signedOffBy.getValue());
+    p.publishCommentsOnPush(publishCommentsOnPush.getValue());
     p.reviewCategoryStrategy(
         getListBox(
             reviewCategoryStrategy, ReviewCategoryStrategy.NONE, ReviewCategoryStrategy.values()));

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.git;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
@@ -1194,6 +1195,7 @@ public class ReceiveCommits {
     final ReceiveCommand cmd;
     final LabelTypes labelTypes;
     final NotesMigration notesMigration;
+    private final boolean defaultPublishComments;
     Branch.NameKey dest;
     RefControl ctl;
     Set<Account.Id> reviewer = Sets.newLinkedHashSet();
@@ -1243,7 +1245,14 @@ public class ReceiveCommits {
     boolean merged;
 
     @Option(name = "--publish-comments", usage = "publish all draft comments on updated changes")
-    boolean publishComments;
+    private boolean publishComments;
+
+    @Option(
+      name = "--no-publish-comments",
+      aliases = {"--np"},
+      usage = "do not publish draft comments"
+    )
+    private boolean noPublishComments;
 
     @Option(
       name = "--notify",
@@ -1328,11 +1337,17 @@ public class ReceiveCommits {
       //TODO(dpursehouse): validate hashtags
     }
 
-    MagicBranchInput(ReceiveCommand cmd, LabelTypes labelTypes, NotesMigration notesMigration) {
+    MagicBranchInput(
+        IdentifiedUser user,
+        ReceiveCommand cmd,
+        LabelTypes labelTypes,
+        NotesMigration notesMigration) {
       this.cmd = cmd;
       this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE);
       this.labelTypes = labelTypes;
       this.notesMigration = notesMigration;
+      this.defaultPublishComments =
+          firstNonNull(user.getAccount().getGeneralPreferencesInfo().publishCommentsOnPush, false);
     }
 
     MailRecipients getMailRecipients() {
@@ -1348,6 +1363,15 @@ public class ReceiveCommits {
       return accountsToNotify;
     }
 
+    boolean shouldPublishComments() {
+      if (publishComments) {
+        return true;
+      } else if (noPublishComments) {
+        return false;
+      }
+      return defaultPublishComments;
+    }
+
     String parse(
         CmdLineParser clp,
         Repository repo,
@@ -1417,7 +1441,7 @@ public class ReceiveCommits {
     }
 
     logDebug("Found magic branch {}", cmd.getRefName());
-    magicBranch = new MagicBranchInput(cmd, labelTypes, notesMigration);
+    magicBranch = new MagicBranchInput(user, cmd, labelTypes, notesMigration);
     magicBranch.reviewer.addAll(reviewersFromCommandLine);
     magicBranch.cc.addAll(ccFromCommandLine);
 
@@ -1495,6 +1519,11 @@ public class ReceiveCommits {
       reject(cmd, "the options 'wip' and 'ready' are mutually exclusive");
       return;
     }
+    if (magicBranch.publishComments && magicBranch.noPublishComments) {
+      reject(
+          cmd, "the options 'publish-comments' and 'no-publish-comments' are mutually exclusive");
+      return;
+    }
 
     if (magicBranch.draft && magicBranch.submit) {
       reject(cmd, "cannot submit draft");

gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java

@@ -260,7 +260,7 @@ public class ReplaceOp implements BatchUpdateOp {
         change.setWorkInProgress(true);
         update.setWorkInProgress(true);
       }
-      if (magicBranch.publishComments) {
+      if (magicBranch.shouldPublishComments()) {
         comments = publishComments(ctx);
       }
     }

polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html

@@ -205,6 +205,16 @@ limitations under the License.
                   on-change="_handleExpandInlineDiffsChanged">
             </span>
           </section>
+          <section>
+            <span class="title">Publish comments on push</span>
+            <span class="value">
+              <input
+                  id="publishCommentsOnPush"
+                  type="checkbox"
+                  checked$="[[_localPrefs.publish_comments_on_push]]"
+                  on-change="_handlePublishCommentsOnPushChanged">
+            </span>
+          </section>
           <gr-button
               id="savePrefs"
               on-tap="_handleSavePreferences"

polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js

@@ -21,6 +21,7 @@
     'email_strategy',
     'diff_view',
     'expand_inline_diffs',
+    'publish_comments_on_push',
     'email_format',
   ];
 
@@ -256,6 +257,11 @@
           this.$.expandInlineDiffs.checked);
     },
 
+    _handlePublishCommentsOnPushChanged: function() {
+      this.set('_localPrefs.publish_comments_on_push',
+          this.$.publishCommentsOnPush.checked);
+    },
+
     _handleMenuChanged: function() {
       if (this._isLoading()) { return; }
       this._menuChanged = true;

polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html

@@ -171,6 +171,8 @@ limitations under the License.
           .firstElementChild.bindValue, preferences.diff_view);
       assert.equal(valueOf('Expand inline diffs', 'preferences')
           .firstElementChild.checked, false);
+      assert.equal(valueOf('Publish comments on push', 'preferences')
+          .firstElementChild.checked, false);
 
       assert.isFalse(element._prefsChanged);
       assert.isFalse(element._menuChanged);
@@ -205,6 +207,29 @@ limitations under the License.
       });
     });
 
+    test('publish comments on push', function(done) {
+      var publishCommentsOnPush =
+        valueOf('Publish comments on push', 'preferences').firstElementChild;
+      MockInteractions.tap(publishCommentsOnPush);
+
+      assert.isFalse(element._menuChanged);
+      assert.isTrue(element._prefsChanged);
+
+      stub('gr-rest-api-interface', {
+        savePreferences: function(prefs) {
+          assert.equal(prefs.publish_comments_on_push, true);
+          return Promise.resolve();
+        }
+      });
+
+      // Save the change.
+      element._handleSavePreferences().then(function() {
+        assert.isFalse(element._prefsChanged);
+        assert.isFalse(element._menuChanged);
+        done();
+      });
+    });
+
     test('diff preferences', function(done) {
       // Rendered with the expected preferences selected.
       assert.equal(valueOf('Context', 'diffPreferences')