opendev/gerrit
Code Issues Proposed changes

Move general preferences from Account to AccountState

Browse Source 
All data that is hosted in the Account class is persisted in the
'account.config' file, except general preferences which are stored in an
own file. This makes general preferences similar to project watches
which are also stored in an own file. Project watches are hosted in
AccountState, hence to treat general preferences and project watches
consistently it makes sense to move the general preferences to
AccountState so that general preferences and project watches are hosted
at the same place.

Change-Id: Ic063facd85fd8b8060c0e95b3ed7037f04076e64
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-01-08 16:19:45 +01:00
parent 246c7ad643
commit f8f1684d8e
13 changed files with 75 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/pgm/init/InitAdminUser.java
View File

@@ -21,6 +21,7 @@ import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.client.AuthType;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.pgm.init.api.AllUsersNameOnInitProvider;
import com.google.gerrit.pgm.init.api.ConsoleUI;
import com.google.gerrit.pgm.init.api.InitFlags;
@@ -151,7 +152,12 @@ public class InitAdminUser implements InitStep {
}
AccountState as =
new AccountState(new AllUsersName(allUsers.get()), a, extIds, new HashMap<>());
new AccountState(
new AllUsersName(allUsers.get()),
a,
extIds,
new HashMap<>(),
GeneralPreferencesInfo.defaults());
for (AccountIndex accountIndex : accountIndexCollection.getWriteIndexes()) {
accountIndex.replace(as);
}

12
java/com/google/gerrit/reviewdb/client/Account.java
View File

@@ -19,7 +19,6 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_STARRED_CHANGES;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gwtorm.client.Column;
import com.google.gwtorm.client.IntKey;
import java.sql.Timestamp;
@@ -180,9 +179,6 @@ public final class Account {
/** <i>computed</i> the username selected from the identities. */
protected String userName;
/** <i>stored in git, used for caching</i> the user's preferences. */
private GeneralPreferencesInfo generalPreferences;
/**
* ID of the user branch from which the account was read, {@code null} if the account was read
* from ReviewDb.
@@ -286,14 +282,6 @@ public final class Account {
return registeredOn;
}
public GeneralPreferencesInfo getGeneralPreferencesInfo() {
return generalPreferences;
}
public void setGeneralPreferences(GeneralPreferencesInfo p) {
generalPreferences = p;
}
public String getMetaId() {
return metaId;
}

8
java/com/google/gerrit/server/account/AccountCacheImpl.java
View File

@@ -20,6 +20,7 @@ import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
@@ -129,7 +130,12 @@ public class AccountCacheImpl implements AccountCache {
private AccountState missing(Account.Id accountId) {
Account account = new Account(accountId, TimeUtil.nowTs());
account.setActive(false);
return new AccountState(allUsersName, account, Collections.emptySet(), new HashMap<>());
return new AccountState(
allUsersName,
account,
Collections.emptySet(),
new HashMap<>(),
GeneralPreferencesInfo.defaults());
}
static class ByIdLoader extends CacheLoader<Account.Id, Optional<AccountState>> {

11
java/com/google/gerrit/server/account/AccountState.java
View File

@@ -22,6 +22,7 @@ import com.google.common.base.Strings;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser.PropertyKey;
import com.google.gerrit.server.IdentifiedUser;
@@ -51,17 +52,20 @@ public class AccountState {
private final Account account;
private final Collection<ExternalId> externalIds;
private final Map<ProjectWatchKey, Set<NotifyType>> projectWatches;
private final GeneralPreferencesInfo generalPreferences;
private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties;
public AccountState(
AllUsersName allUsersName,
Account account,
Collection<ExternalId> externalIds,
Map<ProjectWatchKey, Set<NotifyType>> projectWatches) {
Map<ProjectWatchKey, Set<NotifyType>> projectWatches,
GeneralPreferencesInfo generalPreferences) {
this.allUsersName = allUsersName;
this.account = account;
this.externalIds = externalIds;
this.projectWatches = projectWatches;
this.generalPreferences = generalPreferences;
this.account.setUserName(getUserName(externalIds));
}
@@ -117,6 +121,11 @@ public class AccountState {
return projectWatches;
}
/** The general preferences of the account. */
public GeneralPreferencesInfo getGeneralPreferences() {
return generalPreferences;
}
public static String getUserName(Collection<ExternalId> ids) {
for (ExternalId extId : ids) {
if (extId.isScheme(SCHEME_USERNAME)) {

4
java/com/google/gerrit/server/account/Accounts.java
View File

@@ -141,7 +141,6 @@ public class Accounts {
return Optional.empty();
}
Account account = accountConfig.getLoadedAccount().get();
account.setGeneralPreferences(accountConfig.getGeneralPreferences());
return Optional.of(
new AccountState(
allUsersName,
@@ -149,7 +148,8 @@ public class Accounts {
accountConfig.getExternalIdsRev().isPresent()
? externalIds.byAccount(accountId, accountConfig.getExternalIdsRev().get())
: ImmutableSet.of(),
accountConfig.getProjectWatches()));
accountConfig.getProjectWatches(),
accountConfig.getGeneralPreferences()));
}
public static Stream<Account.Id> readUserRefs(Repository repo) throws IOException {

5
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -1333,11 +1333,10 @@ class ReceiveCommits {
this.publish = cmd.getRefName().startsWith(MagicBranch.NEW_PUBLISH_CHANGE);
this.labelTypes = labelTypes;
this.notesMigration = notesMigration;
GeneralPreferencesInfo prefs = user.getAccount().getGeneralPreferencesInfo();
GeneralPreferencesInfo prefs = user.state().getGeneralPreferences();
this.defaultPublishComments =
prefs != null
? firstNonNull(
user.getAccount().getGeneralPreferencesInfo().publishCommentsOnPush, false)
? firstNonNull(user.state().getGeneralPreferences().publishCommentsOnPush, false)
: false;
}

48
java/com/google/gerrit/server/mail/send/OutgoingEmail.java
View File

@@ -121,34 +121,38 @@ public abstract class OutgoingEmail {
Set<Address> smtpRcptToPlaintextOnly = new HashSet<>();
if (shouldSendMessage()) {
if (fromId != null) {
final Account fromUser = args.accountCache.get(fromId).getAccount();
GeneralPreferencesInfo senderPrefs = fromUser.getGeneralPreferencesInfo();
if (senderPrefs != null && senderPrefs.getEmailStrategy() == CC_ON_OWN_COMMENTS) {
// If we are impersonating a user, make sure they receive a CC of
// this message so they can always review and audit what we sent
// on their behalf to others.
//
add(RecipientType.CC, fromId);
} else if (!accountsToNotify.containsValue(fromId) && rcptTo.remove(fromId)) {
// If they don't want a copy, but we queued one up anyway,
// drop them from the recipient lists.
//
removeUser(fromUser);
AccountState fromUser = args.accountCache.get(fromId);
if (fromUser != null) {
GeneralPreferencesInfo senderPrefs = fromUser.getGeneralPreferences();
if (senderPrefs != null && senderPrefs.getEmailStrategy() == CC_ON_OWN_COMMENTS) {
// If we are impersonating a user, make sure they receive a CC of
// this message so they can always review and audit what we sent
// on their behalf to others.
//
add(RecipientType.CC, fromId);
} else if (!accountsToNotify.containsValue(fromId) && rcptTo.remove(fromId)) {
// If they don't want a copy, but we queued one up anyway,
// drop them from the recipient lists.
//
removeUser(fromUser.getAccount());
}
}
}
// Check the preferences of all recipients. If any user has disabled
// his email notifications then drop him from recipients' list.
// In addition, check if users only want to receive plaintext email.
for (Account.Id id : rcptTo) {
Account thisUser = args.accountCache.get(id).getAccount();
GeneralPreferencesInfo prefs = thisUser.getGeneralPreferencesInfo();
if (prefs == null || prefs.getEmailStrategy() == DISABLED) {
removeUser(thisUser);
} else if (useHtml() && prefs.getEmailFormat() == EmailFormat.PLAINTEXT) {
removeUser(thisUser);
smtpRcptToPlaintextOnly.add(
new Address(thisUser.getFullName(), thisUser.getPreferredEmail()));
AccountState thisUser = args.accountCache.get(id);
if (thisUser != null) {
Account thisUserAccount = thisUser.getAccount();
GeneralPreferencesInfo prefs = thisUser.getGeneralPreferences();
if (prefs == null || prefs.getEmailStrategy() == DISABLED) {
removeUser(thisUserAccount);
} else if (useHtml() && prefs.getEmailFormat() == EmailFormat.PLAINTEXT) {
removeUser(thisUserAccount);
smtpRcptToPlaintextOnly.add(
new Address(thisUserAccount.getFullName(), thisUserAccount.getPreferredEmail()));
}
}
if (smtpRcptTo.isEmpty() && smtpRcptToPlaintextOnly.isEmpty()) {
return;

2
java/com/google/gerrit/server/restapi/account/GetPreferences.java
View File

@@ -50,6 +50,6 @@ public class GetPreferences implements RestReadView<AccountResource> {
}
Account.Id id = rsrc.getUser().getAccountId();
return accountCache.get(id).getAccount().getGeneralPreferencesInfo();
return accountCache.get(id).getGeneralPreferences();
}
}

2
java/com/google/gerrit/server/restapi/account/SetPreferences.java
View File

@@ -90,7 +90,7 @@ public class SetPreferences implements RestModifyView<AccountResource, GeneralPr
accountsUpdate
.create()
.update("Set Preferences via API", id, u -> u.setGeneralPreferences(input));
return cache.get(id).getAccount().getGeneralPreferencesInfo();
return cache.get(id).getGeneralPreferences();
}
public static void storeMyMenus(VersionedAccountPreferences prefs, List<MenuItem> my)

2
java/com/google/gerrit/server/restapi/change/CreateChange.java
View File

@@ -231,7 +231,7 @@ public class CreateChange
IdentifiedUser me = user.get().asIdentifiedUser();
PersonIdent author = me.newCommitterIdent(now, serverTimeZone);
AccountState account = accountCache.get(me.getAccountId());
GeneralPreferencesInfo info = account.getAccount().getGeneralPreferencesInfo();
GeneralPreferencesInfo info = account.getGeneralPreferences();
ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree();
ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, input.subject);

4
java/com/google/gerrit/testing/FakeAccountCache.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.testing;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
@@ -79,6 +80,7 @@ public class FakeAccountCache implements AccountCache {
new AllUsersName(AllUsersNameProvider.DEFAULT),
account,
ImmutableSet.of(),
new HashMap<>());
new HashMap<>(),
GeneralPreferencesInfo.defaults());
}
}

14
javatests/com/google/gerrit/server/index/account/AccountFieldTest.java
View File

@@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.account.AccountState;
@@ -44,7 +45,12 @@ public class AccountFieldTest extends GerritBaseTests {
List<String> values =
toStrings(
AccountField.REF_STATE.get(
new AccountState(allUsersName, account, ImmutableSet.of(), ImmutableMap.of())));
new AccountState(
allUsersName,
account,
ImmutableSet.of(),
ImmutableMap.of(),
GeneralPreferencesInfo.defaults())));
assertThat(values).hasSize(1);
String expectedValue =
allUsersName.get() + ":" + RefNames.refsUsers(account.getId()) + ":" + metaId;
@@ -73,7 +79,11 @@ public class AccountFieldTest extends GerritBaseTests {
toStrings(
AccountField.EXTERNAL_ID_STATE.get(
new AccountState(
null, account, ImmutableSet.of(extId1, extId2), ImmutableMap.of())));
null,
account,
ImmutableSet.of(extId1, extId2),
ImmutableMap.of(),
GeneralPreferencesInfo.defaults())));
String expectedValue1 = extId1.key().sha1().name() + ":" + extId1.blobId().name();
String expectedValue2 = extId2.key().sha1().name() + ":" + extId2.blobId().name();
assertThat(values).containsExactly(expectedValue1, expectedValue2);

4
javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
View File

@@ -22,6 +22,7 @@ import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.verify;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
@@ -388,6 +389,7 @@ public class FromAddressGeneratorProviderTest {
new AllUsersName(AllUsersNameProvider.DEFAULT),
account,
Collections.emptySet(),
new HashMap<>());
new HashMap<>(),
GeneralPreferencesInfo.defaults());
}
}