diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt index 2c42d7401b..9aa0a3b23e 100644 --- a/Documentation/intro-user.txt +++ b/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 diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index 13fca66f17..fbfd5e4cd5 100644 --- a/Documentation/rest-api-accounts.txt +++ b/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]] diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt index fd35a29d1c..49dbf30984 100644 --- a/Documentation/rest-api-config.txt +++ b/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", diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index c12d60af10..deec66062f 100644 --- a/Documentation/user-upload.txt +++ b/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 diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 5cd089edad..b9b6812ac3 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/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; diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java index 7192ff945c..9dcba5e4b9 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java +++ b/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; } } diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java index 23e1a93ac6..1dcb284ed1 100644 --- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java +++ b/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 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 myMenus) { initMy(); for (TopMenuItem n : myMenus) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java index 4a3a5f8ea5..314871e76b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java +++ b/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(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties index 9d87365b25..d31abdbcef 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties +++ b/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. \ diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java index 2edc137461..9be15fff54 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java +++ b/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())); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index c398131f25..ac504c4787 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/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 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"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java index 377cd44320..a86e163805 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java +++ b/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); } } diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html index f485dd6574..de3134bd1b 100644 --- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html +++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html @@ -205,6 +205,16 @@ limitations under the License. on-change="_handleExpandInlineDiffsChanged"> +
+ Publish comments on push + + + +