Preferences: Fix overwriting configured default with original default
If a user didn't set a value for a preference then the hard-coded
default value is used, unless another default value is configured in the
refs/users/default branch in All-Users.
If a default value was configured in refs/users/default then overwriting
this configured default with the original hard-coded default didn't
work. Only overwriting it with any other value worked.
This didn't work and the effective user setting was 80:
* hard-coded default: 100
* refs/users/default: 80
* user setting: 100
This worked:
* hard-coded default: 100
* refs/users/default: 80
* user setting: 101 (and any value other than 100)
This is the reason why setting lineLength ("Columns") in the diff
preferences on gerrit-review to 100 is not working, as in the
refs/users/default branch we have configured a default lineLength of 80
and 100 is the hard-coded default value. Please note that setting
lineLength to 100 works transiently until the page is reloaded because
the client stores the updated diff preferences locally.
Change-Id: I5f63f9d62f639c755b39d753d8b0b30832c8c9f6
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java
index fe80032..fcf939d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java
@@ -21,6 +21,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.extensions.client.Theme;
@@ -35,6 +36,7 @@
import org.junit.Test;
@NoHttpd
+@Sandboxed
public class DiffPreferencesIT extends AbstractDaemonTest {
@Inject private AllUsersName allUsers;
@@ -136,4 +138,39 @@
// assert hard-coded defaults
assertPrefs(o, d, "lineLength", "tabSize", "fontSize");
}
+
+ @Test
+ public void overwriteConfiguredDefaults() throws Exception {
+ DiffPreferencesInfo d = DiffPreferencesInfo.defaults();
+ int configuredDefaultLineLength = d.lineLength + 10;
+ DiffPreferencesInfo update = new DiffPreferencesInfo();
+ update.lineLength = configuredDefaultLineLength;
+ gApi.config().server().setDefaultDiffPreferences(update);
+
+ DiffPreferencesInfo o = gApi.accounts().id(admin.getId().toString()).getDiffPreferences();
+ assertThat(o.lineLength).isEqualTo(configuredDefaultLineLength);
+ assertPrefs(o, d, "lineLength");
+
+ int newLineLength = configuredDefaultLineLength + 10;
+ DiffPreferencesInfo i = new DiffPreferencesInfo();
+ i.lineLength = newLineLength;
+ DiffPreferencesInfo a = gApi.accounts().id(admin.getId().toString()).setDiffPreferences(i);
+ assertThat(a.lineLength).isEqualTo(newLineLength);
+ assertPrefs(a, d, "lineLength");
+
+ a = gApi.accounts().id(admin.getId().toString()).getDiffPreferences();
+ assertThat(a.lineLength).isEqualTo(newLineLength);
+ assertPrefs(a, d, "lineLength");
+
+ // overwrite the configured default with original hard-coded default
+ i = new DiffPreferencesInfo();
+ i.lineLength = d.lineLength;
+ a = gApi.accounts().id(admin.getId().toString()).setDiffPreferences(i);
+ assertThat(a.lineLength).isEqualTo(d.lineLength);
+ assertPrefs(a, d, "lineLength");
+
+ a = gApi.accounts().id(admin.getId().toString()).getDiffPreferences();
+ assertThat(a.lineLength).isEqualTo(d.lineLength);
+ assertPrefs(a, d, "lineLength");
+ }
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
index 334f336..8ed2a45 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
@@ -19,6 +19,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.DateFormat;
@@ -41,6 +42,7 @@
import org.junit.Test;
@NoHttpd
+@Sandboxed
public class GeneralPreferencesIT extends AbstractDaemonTest {
@Inject private AllUsersName allUsers;
@@ -122,4 +124,39 @@
// assert hard-coded defaults
assertPrefs(o, d, "my", "changeTable", "changesPerPage");
}
+
+ @Test
+ public void overwriteConfiguredDefaults() throws Exception {
+ GeneralPreferencesInfo d = GeneralPreferencesInfo.defaults();
+ int configuredChangesPerPage = d.changesPerPage * 2;
+ GeneralPreferencesInfo update = new GeneralPreferencesInfo();
+ update.changesPerPage = configuredChangesPerPage;
+ gApi.config().server().setDefaultPreferences(update);
+
+ GeneralPreferencesInfo o = gApi.accounts().id(admin.getId().toString()).getPreferences();
+ assertThat(o.changesPerPage).isEqualTo(configuredChangesPerPage);
+ assertPrefs(o, d, "my", "changeTable", "changesPerPage");
+
+ int newChangesPerPage = configuredChangesPerPage * 2;
+ GeneralPreferencesInfo i = new GeneralPreferencesInfo();
+ i.changesPerPage = newChangesPerPage;
+ GeneralPreferencesInfo a = gApi.accounts().id(admin.getId().toString()).setPreferences(i);
+ assertThat(a.changesPerPage).isEqualTo(newChangesPerPage);
+ assertPrefs(a, d, "my", "changeTable", "changesPerPage");
+
+ a = gApi.accounts().id(admin.getId().toString()).getPreferences();
+ assertThat(a.changesPerPage).isEqualTo(newChangesPerPage);
+ assertPrefs(a, d, "my", "changeTable", "changesPerPage");
+
+ // overwrite the configured default with original hard-coded default
+ i = new GeneralPreferencesInfo();
+ i.changesPerPage = d.changesPerPage;
+ a = gApi.accounts().id(admin.getId().toString()).setPreferences(i);
+ assertThat(a.changesPerPage).isEqualTo(d.changesPerPage);
+ assertPrefs(a, d, "my", "changeTable", "changesPerPage");
+
+ a = gApi.accounts().id(admin.getId().toString()).getPreferences();
+ assertThat(a.changesPerPage).isEqualTo(d.changesPerPage);
+ assertPrefs(a, d, "my", "changeTable", "changesPerPage");
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java
index 1c44670..df64c0b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java
@@ -77,14 +77,6 @@
// Load all users default prefs
VersionedAccountPreferences dp = VersionedAccountPreferences.forDefault();
dp.load(allUsers);
- GeneralPreferencesInfo allUserPrefs = new GeneralPreferencesInfo();
- loadSection(
- dp.getConfig(),
- UserConfigSections.GENERAL,
- null,
- allUserPrefs,
- GeneralPreferencesInfo.defaults(),
- in);
// Load user prefs
VersionedAccountPreferences p = VersionedAccountPreferences.forUser(id);
@@ -95,13 +87,33 @@
UserConfigSections.GENERAL,
null,
new GeneralPreferencesInfo(),
- updateDefaults(allUserPrefs),
+ readDefaultsFromGit(dp.getConfig(), in),
in);
loadChangeTableColumns(r, p, dp);
return loadMyMenusAndUrlAliases(r, p, dp);
}
}
+ public GeneralPreferencesInfo readDefaultsFromGit(Repository git, GeneralPreferencesInfo in)
+ throws ConfigInvalidException, IOException {
+ VersionedAccountPreferences dp = VersionedAccountPreferences.forDefault();
+ dp.load(git);
+ return readDefaultsFromGit(dp.getConfig(), in);
+ }
+
+ private GeneralPreferencesInfo readDefaultsFromGit(Config config, GeneralPreferencesInfo in)
+ throws ConfigInvalidException {
+ GeneralPreferencesInfo allUserPrefs = new GeneralPreferencesInfo();
+ loadSection(
+ config,
+ UserConfigSections.GENERAL,
+ null,
+ allUserPrefs,
+ GeneralPreferencesInfo.defaults(),
+ in);
+ return updateDefaults(allUserPrefs);
+ }
+
private GeneralPreferencesInfo updateDefaults(GeneralPreferencesInfo input) {
GeneralPreferencesInfo result = GeneralPreferencesInfo.defaults();
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java
index 5a39bea..0edff4f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java
@@ -69,28 +69,30 @@
Account.Id id, GitRepositoryManager gitMgr, AllUsersName allUsersName, DiffPreferencesInfo in)
throws IOException, ConfigInvalidException, RepositoryNotFoundException {
try (Repository git = gitMgr.openRepository(allUsersName)) {
- // Load all users prefs.
- VersionedAccountPreferences dp = VersionedAccountPreferences.forDefault();
- dp.load(git);
- DiffPreferencesInfo allUserPrefs = new DiffPreferencesInfo();
- loadSection(
- dp.getConfig(),
- UserConfigSections.DIFF,
- null,
- allUserPrefs,
- DiffPreferencesInfo.defaults(),
- in);
-
- // Load user prefs
VersionedAccountPreferences p = VersionedAccountPreferences.forUser(id);
p.load(git);
DiffPreferencesInfo prefs = new DiffPreferencesInfo();
loadSection(
- p.getConfig(), UserConfigSections.DIFF, null, prefs, updateDefaults(allUserPrefs), in);
+ p.getConfig(), UserConfigSections.DIFF, null, prefs, readDefaultsFromGit(git, in), in);
return prefs;
}
}
+ static DiffPreferencesInfo readDefaultsFromGit(Repository git, DiffPreferencesInfo in)
+ throws ConfigInvalidException, IOException {
+ VersionedAccountPreferences dp = VersionedAccountPreferences.forDefault();
+ dp.load(git);
+ DiffPreferencesInfo allUserPrefs = new DiffPreferencesInfo();
+ loadSection(
+ dp.getConfig(),
+ UserConfigSections.DIFF,
+ null,
+ allUserPrefs,
+ DiffPreferencesInfo.defaults(),
+ in);
+ return updateDefaults(allUserPrefs);
+ }
+
private static DiffPreferencesInfo updateDefaults(DiffPreferencesInfo input) {
DiffPreferencesInfo result = DiffPreferencesInfo.defaults();
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java
index 986124a..ac0cc96 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.account;
+import static com.google.gerrit.server.account.GetDiffPreferences.readDefaultsFromGit;
import static com.google.gerrit.server.account.GetDiffPreferences.readFromGit;
import static com.google.gerrit.server.config.ConfigUtil.loadSection;
import static com.google.gerrit.server.config.ConfigUtil.storeSection;
@@ -74,18 +75,12 @@
throws RepositoryNotFoundException, IOException, ConfigInvalidException {
DiffPreferencesInfo out = new DiffPreferencesInfo();
try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) {
+ DiffPreferencesInfo allUserPrefs = readDefaultsFromGit(md.getRepository(), null);
VersionedAccountPreferences prefs = VersionedAccountPreferences.forUser(userId);
prefs.load(md);
- DiffPreferencesInfo defaults = DiffPreferencesInfo.defaults();
- storeSection(prefs.getConfig(), UserConfigSections.DIFF, null, in, defaults);
+ storeSection(prefs.getConfig(), UserConfigSections.DIFF, null, in, allUserPrefs);
prefs.commit(md);
- loadSection(
- prefs.getConfig(),
- UserConfigSections.DIFF,
- null,
- out,
- DiffPreferencesInfo.defaults(),
- null);
+ loadSection(prefs.getConfig(), UserConfigSections.DIFF, null, out, allUserPrefs, null);
}
return out;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/SetPreferences.java
index e1ed6aa..91672f7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetPreferences.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/SetPreferences.java
@@ -104,7 +104,7 @@
UserConfigSections.GENERAL,
null,
i,
- GeneralPreferencesInfo.defaults());
+ loader.readDefaultsFromGit(md.getRepository(), null));
storeMyChangeTableColumns(prefs, i.changeTable);
storeMyMenus(prefs, i.my);
