Control auto-publishing comments on push with preference
Change-Id: Ibf6c61d8c34ee790ca7ed29a0fad0b5e81910f09
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index 2c42d74..9aa0a3b 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -773,6 +773,12 @@
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 13fca66..fbfd5e4 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -1248,6 +1248,7 @@
"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 @@
"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 @@
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 fd35a29..49dbf30 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -1065,6 +1065,7 @@
"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 @@
"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 c12d60a..deec660 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -272,6 +272,11 @@
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 5cd089e..b9b6812 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.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.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 @@
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 @@
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 7192ff9..9dcba5e 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 EmailStrategy emailStrategy;
public EmailFormat emailFormat;
public DefaultBase defaultBaseForMerges;
+ public Boolean publishCommentsOnPush;
public boolean isShowInfoInReviewCategory() {
return getReviewCategoryStrategy() != ReviewCategoryStrategy.NONE;
@@ -225,6 +226,7 @@
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 23e1a93..1dcb284 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 @@
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 @@
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) {
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 4a3a5f8..314871e 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 @@
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 9d87365..d31abdb 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 @@
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 2edc137..9be15ff 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 c398131..ac504c4 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 @@
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 @@
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 @@
//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 @@
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 @@
}
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 @@
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 377cd44..a86e163 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 @@
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 f485dd6..de3134b 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 @@
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"
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
index 4647a2d..6c26550 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
+++ b/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;
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
index cb471d4..33e6bd5 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
@@ -171,6 +171,8 @@
.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 @@
});
});
+ 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')
