Merge changes Id3095a54,Id00e19d6 * changes: Make comment removal element less obtrusive Invert comment action alignments
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt index 056cf43..97c4bea 100644 --- a/Documentation/intro-user.txt +++ b/Documentation/intro-user.txt
@@ -488,8 +488,8 @@ Private changes are changes that are only visible to their owners and reviewers. Private changes are useful in a number of cases: -* You want to check what the change looks before formal review starts. - By marking the change private without reviewers, nobody can't +* You want to check what the change looks like before formal review starts. + By marking the change private without reviewers, nobody can prematurely comment on your changes. * You want to use Gerrit to sync data between different devices. By @@ -680,7 +680,7 @@ + Email notifications are enabled. + -** [[cc-me]]`CC Me On Comments I Write`: +** [[cc-me]]`Every comment`: + Email notifications are enabled and you get notified by email as CC on comments that you write yourself.
diff --git a/Documentation/user-notify.txt b/Documentation/user-notify.txt index d83ca87..4b928f3 100644 --- a/Documentation/user-notify.txt +++ b/Documentation/user-notify.txt
@@ -26,8 +26,8 @@ the change owner. Notification mails for comments added on changes are not sent to the user -who added the comment unless the user has enabled the 'CC Me On Comments I -Write' option in the user preferences. +who added the comment unless the user has enabled the 'Every comment' +option in the user preferences. [[project]]
diff --git a/WORKSPACE b/WORKSPACE index 1a516ea..9bfd28f 100644 --- a/WORKSPACE +++ b/WORKSPACE
@@ -1057,8 +1057,8 @@ bower_archive( name = "iron-dropdown", package = "polymerelements/iron-dropdown", - sha1 = "63e3d669a09edaa31c4f05afc76b53b919ef0595", - version = "1.4.0", + sha1 = "ac96fe31cdf203a63426fa75131b43c98c0597d3", + version = "1.5.5", ) bower_archive( @@ -1071,15 +1071,22 @@ bower_archive( name = "iron-overlay-behavior", package = "polymerelements/iron-overlay-behavior", - sha1 = "83181085fda59446ce74fd0d5ca30c223f38ee4a", - version = "1.7.6", + sha1 = "74cda9d7bf98e7a5e5004bc7ebdb6d208d49e11e", + version = "2.0.0", ) bower_archive( name = "iron-selector", package = "polymerelements/iron-selector", - sha1 = "c57235dfda7fbb987c20ad0e97aac70babf1a1bf", - version = "1.5.2", + sha1 = "e0ee46c28523bf17730318c3b481a8ed4331c3b2", + version = "2.0.0", +) + +bower_archive( + name = "paper-input", + package = "polymerelements/paper-input", + sha1 = "6c934805e80ab201e143406edc73ea0ef35abf80", + version = "1.1.18", ) bower_archive( @@ -1097,6 +1104,20 @@ ) bower_archive( + name = "paper-item", + package = "polymerelements/paper-item", + sha1 = "803273ceb9ffebec8ecc9373ea638af4cd34af58", + version = "1.1.4", +) + +bower_archive( + name = "paper-listbox", + package = "polymerelements/paper-listbox", + sha1 = "ccc1a90ab0a96878c7bf7c9c4cfe47c85b09c8e3", + version = "2.0.0", +) + +bower_archive( name = "polymer", package = "polymer/polymer", sha1 = "566b5fe9a2a3eea2cf3417c67d975a6752d131eb", @@ -1106,8 +1127,8 @@ bower_archive( name = "polymer-resin", package = "polymer/polymer-resin", - sha1 = "d759c8c09054a7ec04608a6cb586801c904f79a2", - version = "1.2.6-beta", + sha1 = "93ac118f2b9209cfbfd6dc8022d9492743d17f24", + version = "1.2.7", ) bower_archive(
diff --git a/contrib/abandon_stale.py b/contrib/abandon_stale.py index d734cd1..99022aa 100755 --- a/contrib/abandon_stale.py +++ b/contrib/abandon_stale.py
@@ -191,7 +191,7 @@ try: gerrit.post("/changes/" + change_id + "/abandon", - data={"message" : "%s" % abandon_message}) + json={"message" : "%s" % abandon_message}) abandoned += 1 except Exception as e: errors += 1
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 1af8f7d..d5e2d9e 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -93,6 +93,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.group.Groups; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.index.change.ChangeIndex; @@ -254,6 +255,7 @@ @Inject private InProcessProtocol inProcessProtocol; @Inject private Provider<AnonymousUser> anonymousUser; @Inject private SchemaFactory<ReviewDb> reviewDbProvider; + @Inject private Groups groups; private List<Repository> toClose; @@ -344,6 +346,8 @@ Transport.register(inProcessProtocol); toClose = Collections.synchronizedList(new ArrayList<Repository>()); + db = reviewDbProvider.open(); + // All groups which were added during the server start (e.g. in SchemaCreator) aren't contained // in the instance of the group index which is available here and in tests. There are two // reasons: @@ -353,7 +357,7 @@ // later on. As test indexes are non-permanent, closing an instance and opening another one // removes all indexed data. // As a workaround, we simply reindex all available groups here. - for (AccountGroup group : groupCache.all()) { + for (AccountGroup group : groups.getAll(db).collect(toList())) { groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey()); } @@ -367,8 +371,6 @@ adminRestSession = new RestSession(server, admin); userRestSession = new RestSession(server, user); - db = reviewDbProvider.open(); - testRequiresSsh = classDesc.useSshAnnotation() || methodDesc.useSshAnnotation(); if (testRequiresSsh && SshMode.useSsh()
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java index eb4df15..305a2b0 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -30,7 +30,7 @@ import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.extensions.api.groups.GroupApi; import com.google.gerrit.extensions.api.groups.GroupInput; -import com.google.gerrit.extensions.api.groups.Groups; +import com.google.gerrit.extensions.api.groups.Groups.ListRequest; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.GroupAuditEventInfo; import com.google.gerrit.extensions.common.GroupAuditEventInfo.GroupMemberAuditEventInfo; @@ -46,6 +46,7 @@ import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.server.group.Groups; import com.google.gerrit.server.group.GroupsUpdate; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.ServerInitiated; @@ -63,6 +64,7 @@ @NoHttpd public class GroupsIT extends AbstractDaemonTest { @Inject @ServerInitiated private Provider<GroupsUpdate> groupsUpdateProvider; + @Inject private Groups groups; @Test public void systemGroupCanBeRetrievedFromIndex() throws Exception { @@ -481,7 +483,7 @@ @Test public void listAllGroups() throws Exception { List<String> expectedGroups = - groupCache.all().stream().map(a -> a.getName()).sorted().collect(toList()); + groups.getAll(db).map(a -> a.getName()).sorted().collect(toList()); assertThat(expectedGroups.size()).isAtLeast(2); assertThat(gApi.groups().list().getAsMap().keySet()) .containsExactlyElementsIn(expectedGroups) @@ -692,7 +694,7 @@ groupsUpdateProvider.get().updateGroup(db, groupUuid, group -> group.setCreatedOn(null)); } - private void assertBadRequest(Groups.ListRequest req) throws Exception { + private void assertBadRequest(ListRequest req) throws Exception { try { req.get(); fail("Expected BadRequestException");
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/LogFileCompressor.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/LogFileCompressor.java index 3b910bb..666d03c 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/LogFileCompressor.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/LogFileCompressor.java
@@ -93,17 +93,21 @@ @Override public void run() { - if (!Files.isDirectory(logs_dir)) { - return; - } - try (DirectoryStream<Path> list = Files.newDirectoryStream(logs_dir)) { - for (Path entry : list) { - if (!isLive(entry) && !isCompressed(entry) && isLogFile(entry)) { - compress(entry); - } + try { + if (!Files.isDirectory(logs_dir)) { + return; } - } catch (IOException e) { - log.error("Error listing logs to compress in " + logs_dir, e); + try (DirectoryStream<Path> list = Files.newDirectoryStream(logs_dir)) { + for (Path entry : list) { + if (!isLive(entry) && !isCompressed(entry) && isLogFile(entry)) { + compress(entry); + } + } + } catch (IOException e) { + log.error("Error listing logs to compress in " + logs_dir, e); + } + } catch (Exception e) { + log.error("Failed to compress log files: " + e.getMessage(), e); } }
diff --git a/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/init/libraries.config b/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/init/libraries.config index 9c02d2f..7429cea 100644 --- a/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/init/libraries.config +++ b/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/init/libraries.config
@@ -19,9 +19,9 @@ remove = mysql-connector-java-.*[.]jar [library "mariadbDriver"] - name = MariaDB Connector/J 2.1.0 - url = https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client/2.1.0/mariadb-java-client-2.1.0.jar - sha1 = 639be502c0d191e1cc21e4e86d388486358fddf8 + name = MariaDB Connector/J 2.1.2 + url = https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client/2.1.2/mariadb-java-client-2.1.2.jar + sha1 = 002484a99a6a86491531d17d491c931de8942ae0 remove = mariadb-java-client-.*[.]jar [library "oracleDriver"]
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java index 2427ea2..adca6fb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -39,7 +39,6 @@ import com.google.gerrit.server.index.group.GroupField; import com.google.gerrit.server.index.group.GroupIndex; import com.google.gerrit.server.index.group.GroupIndexCollection; -import com.google.gerrit.server.query.account.InternalAccountQuery; import com.google.gerrit.server.query.group.InternalGroupQuery; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -237,20 +236,4 @@ return internalGroupStream.map(InternalGroup::getGroupUUID).collect(toImmutableSet()); } } - - static class ByNameLoader extends CacheLoader<String, Optional<Account.Id>> { - private final Provider<InternalAccountQuery> accountQueryProvider; - - @Inject - ByNameLoader(Provider<InternalAccountQuery> accountQueryProvider) { - this.accountQueryProvider = accountQueryProvider; - } - - @Override - public Optional<Account.Id> load(String username) throws Exception { - AccountState accountState = - accountQueryProvider.get().oneByExternalId(SCHEME_USERNAME, username); - return Optional.ofNullable(accountState).map(s -> s.getAccount().getId()); - } - } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java index 82bcce3..d985426 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCache.java
@@ -14,7 +14,6 @@ package com.google.gerrit.server.account; -import com.google.common.collect.ImmutableList; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.group.InternalGroup; import java.io.IOException; @@ -49,9 +48,6 @@ */ Optional<InternalGroup> get(AccountGroup.UUID groupUuid); - /** @return sorted list of groups. */ - ImmutableList<AccountGroup> all(); - /** Notify the cache that a new group was constructed. */ void onCreateGroup(AccountGroup group) throws IOException;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java index edbc2d8..81996ab 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java
@@ -14,11 +14,8 @@ package com.google.gerrit.server.account; -import static com.google.common.collect.ImmutableList.toImmutableList; - import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; -import com.google.common.collect.ImmutableList; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; @@ -26,7 +23,6 @@ import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.index.group.GroupIndexer; import com.google.gerrit.server.query.group.InternalGroupQuery; -import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Module; @@ -71,24 +67,18 @@ private final LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId; private final LoadingCache<String, Optional<InternalGroup>> byName; private final LoadingCache<String, Optional<InternalGroup>> byUUID; - private final SchemaFactory<ReviewDb> schema; private final Provider<GroupIndexer> indexer; - private final Groups groups; @Inject GroupCacheImpl( @Named(BYID_NAME) LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId, @Named(BYNAME_NAME) LoadingCache<String, Optional<InternalGroup>> byName, @Named(BYUUID_NAME) LoadingCache<String, Optional<InternalGroup>> byUUID, - SchemaFactory<ReviewDb> schema, - Provider<GroupIndexer> indexer, - Groups groups) { + Provider<GroupIndexer> indexer) { this.byId = byId; this.byName = byName; this.byUUID = byUUID; - this.schema = schema; this.indexer = indexer; - this.groups = groups; } @Override @@ -152,16 +142,6 @@ } @Override - public ImmutableList<AccountGroup> all() { - try (ReviewDb db = schema.open()) { - return groups.getAll(db).collect(toImmutableList()); - } catch (OrmException e) { - log.warn("Cannot list internal groups", e); - return ImmutableList.of(); - } - } - - @Override public void onCreateGroup(AccountGroup group) throws IOException { indexer.get().index(group.getGroupUUID()); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java index e471d57c..60e6297 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
@@ -19,12 +19,17 @@ import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.group.Groups; import com.google.gerrit.server.group.InternalGroupDescription; import com.google.gerrit.server.project.ProjectState; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import java.util.Collection; +import java.util.Collections; import org.eclipse.jgit.lib.ObjectId; /** Implementation of GroupBackend for the internal group system. */ @@ -32,15 +37,21 @@ public class InternalGroupBackend implements GroupBackend { private final GroupControl.Factory groupControlFactory; private final GroupCache groupCache; + private final Groups groups; + private final Provider<ReviewDb> db; private final IncludingGroupMembership.Factory groupMembershipFactory; @Inject InternalGroupBackend( GroupControl.Factory groupControlFactory, GroupCache groupCache, + Groups groups, + Provider<ReviewDb> db, IncludingGroupMembership.Factory groupMembershipFactory) { this.groupControlFactory = groupControlFactory; this.groupCache = groupCache; + this.groups = groups; + this.db = db; this.groupMembershipFactory = groupMembershipFactory; } @@ -61,16 +72,19 @@ @Override public Collection<GroupReference> suggest(String name, ProjectState project) { - return groupCache - .all() - .stream() - .filter( - group -> - // startsWithIgnoreCase && isVisible - group.getName().regionMatches(true, 0, name, 0, name.length()) - && groupControlFactory.controlFor(group).isVisible()) - .map(GroupReference::forGroup) - .collect(toList()); + try { + return groups + .getAll(db.get()) + .filter( + group -> + // startsWithIgnoreCase && isVisible + group.getName().regionMatches(true, 0, name, 0, name.length()) + && groupControlFactory.controlFor(group).isVisible()) + .map(GroupReference::forGroup) + .collect(toList()); + } catch (OrmException e) { + return Collections.emptyList(); + } } @Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 59a48fe..4600dca 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -119,6 +119,7 @@ import com.google.gerrit.server.permissions.LabelPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.RemoveReviewerControl; @@ -165,7 +166,15 @@ ChangeField.SUBMIT_RULE_OPTIONS_STRICT.toBuilder().fastEvalLabels(true).build(); public static final ImmutableSet<ListChangesOption> REQUIRE_LAZY_LOAD = - ImmutableSet.of(ALL_REVISIONS, CHANGE_ACTIONS, CHECK, CURRENT_ACTIONS, MESSAGES); + ImmutableSet.of( + ALL_COMMITS, + ALL_REVISIONS, + CHANGE_ACTIONS, + CHECK, + COMMIT_FOOTERS, + CURRENT_ACTIONS, + CURRENT_COMMIT, + MESSAGES); @Singleton public static class Factory { @@ -219,6 +228,7 @@ private final ApprovalsUtil approvalsUtil; private final RemoveReviewerControl removeReviewerControl; private final TrackingFooters trackingFooters; + private final ChangeControl.GenericFactory changeControlFactory; private boolean lazyLoad = true; private AccountLoader accountLoader; private FixInput fix; @@ -251,6 +261,7 @@ ApprovalsUtil approvalsUtil, RemoveReviewerControl removeReviewerControl, TrackingFooters trackingFooters, + ChangeControl.GenericFactory changeControlFactory, @Assisted Iterable<ListChangesOption> options) { this.db = db; this.userProvider = user; @@ -276,6 +287,7 @@ this.indexes = indexes; this.approvalsUtil = approvalsUtil; this.removeReviewerControl = removeReviewerControl; + this.changeControlFactory = changeControlFactory; this.options = Sets.immutableEnumSet(options); this.trackingFooters = trackingFooters; } @@ -345,7 +357,7 @@ } public ChangeInfo format(RevisionResource rsrc) throws OrmException { - ChangeData cd = changeDataFactory.create(db.get(), rsrc.getChangeResource()); + ChangeData cd = changeDataFactory.create(db.get(), rsrc.getNotes()); return format(cd, Optional.of(rsrc.getPatchSet().getId()), true); } @@ -494,7 +506,11 @@ } } - PermissionBackend.ForChange perm = permissionBackend.user(user).database(db).change(cd); + PermissionBackend.WithUser withUser = permissionBackend.user(user).database(db); + PermissionBackend.ForChange perm = + lazyLoad + ? withUser.change(cd) + : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change())); Change in = cd.change(); out.project = in.getProject().get(); out.branch = in.getDest().getShortName(); @@ -588,15 +604,20 @@ } else { src = null; } + + ChangeControl ctl = null; + if (needMessages || needRevisions) { + ctl = changeControlFactory.controlFor(db.get(), cd.change(), userProvider.get()); + } if (needMessages) { - out.messages = messages(cd, src); + out.messages = messages(ctl, cd, src); } finish(out); // This block must come after the ChangeInfo is mostly populated, since // it will be passed to ActionVisitors as-is. if (needRevisions) { - out.revisions = revisions(cd, src, limitToPsId, out); + out.revisions = revisions(ctl, cd, src, limitToPsId, out); if (out.revisions != null) { for (Map.Entry<String, RevisionInfo> entry : out.revisions.entrySet()) { if (entry.getValue().isCurrent) { @@ -653,11 +674,11 @@ return result; } - private boolean submittable(ChangeData cd) { + private boolean submittable(ChangeData cd) throws OrmException { return SubmitRecord.findOkRecord(cd.submitRecords(SUBMIT_RULE_OPTIONS_STRICT)).isPresent(); } - private List<SubmitRecord> submitRecords(ChangeData cd) { + private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException { return cd.submitRecords(SUBMIT_RULE_OPTIONS_LENIENT); } @@ -709,7 +730,7 @@ } private Map<String, LabelWithStatus> initLabels( - ChangeData cd, LabelTypes labelTypes, boolean standard) { + ChangeData cd, LabelTypes labelTypes, boolean standard) throws OrmException { Map<String, LabelWithStatus> labels = new TreeMap<>(labelTypes.nameComparator()); for (SubmitRecord rec : submitRecords(cd)) { if (rec.labels == null) { @@ -1091,8 +1112,8 @@ return result; } - private Collection<ChangeMessageInfo> messages(ChangeData cd, Map<PatchSet.Id, PatchSet> map) - throws OrmException { + private Collection<ChangeMessageInfo> messages( + ChangeControl ctl, ChangeData cd, Map<PatchSet.Id, PatchSet> map) throws OrmException { List<ChangeMessage> messages = cmUtil.byChange(db.get(), cd.notes()); if (messages.isEmpty()) { return Collections.emptyList(); @@ -1102,7 +1123,7 @@ for (ChangeMessage message : messages) { PatchSet.Id patchNum = message.getPatchSetId(); PatchSet ps = patchNum != null ? map.get(patchNum) : null; - if (patchNum == null || cd.changeControl().isPatchVisible(ps, db.get())) { + if (patchNum == null || ctl.isPatchVisible(ps, db.get())) { ChangeMessageInfo cmi = new ChangeMessageInfo(); cmi.id = message.getKey().get(); cmi.author = accountLoader.get(message.getAuthor()); @@ -1217,6 +1238,7 @@ } private Map<String, RevisionInfo> revisions( + ChangeControl ctl, ChangeData cd, Map<PatchSet.Id, PatchSet> map, Optional<PatchSet.Id> limitToPsId, @@ -1235,7 +1257,7 @@ } else { want = id.equals(cd.change().currentPatchSetId()); } - if (want && cd.changeControl().isPatchVisible(in, db.get())) { + if (want && ctl.isPatchVisible(in, db.get())) { res.put(in.getRevision().get(), toRevisionInfo(cd, in, repo, rw, false, changeInfo)); } } @@ -1392,6 +1414,7 @@ private Map<String, FetchInfo> makeFetchMap(ChangeData cd, PatchSet in) throws OrmException { Map<String, FetchInfo> r = new LinkedHashMap<>(); + ChangeControl ctl = changeControlFactory.controlFor(db.get(), cd.change(), anonymous); for (DynamicMap.Entry<DownloadScheme> e : downloadSchemes) { String schemeName = e.getExportName(); DownloadScheme scheme = e.getProvider().get(); @@ -1400,8 +1423,7 @@ continue; } - if (!scheme.isAuthSupported() - && !cd.changeControl().forUser(anonymous).isPatchVisible(in, db.get())) { + if (!scheme.isAuthSupported() && !ctl.isPatchVisible(in, db.get())) { continue; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java index a6ed806..a6583b1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java
@@ -27,6 +27,7 @@ import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.RelatedChangesSorter.PatchSetData; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.OrmException; @@ -63,13 +64,14 @@ @Override public RelatedInfo apply(RevisionResource rsrc) - throws RepositoryNotFoundException, IOException, OrmException { + throws RepositoryNotFoundException, IOException, OrmException, NoSuchProjectException { RelatedInfo relatedInfo = new RelatedInfo(); relatedInfo.changes = getRelated(rsrc); return relatedInfo; } - private List<ChangeAndCommit> getRelated(RevisionResource rsrc) throws OrmException, IOException { + private List<ChangeAndCommit> getRelated(RevisionResource rsrc) + throws OrmException, IOException, NoSuchProjectException { Set<String> groups = getAllGroups(rsrc.getNotes()); if (groups.isEmpty()) { return Collections.emptyList(); @@ -93,7 +95,7 @@ reloadChangeIfStale(cds, basePs); - for (PatchSetData d : sorter.sort(cds, basePs)) { + for (PatchSetData d : sorter.sort(cds, basePs, rsrc.getUser())) { PatchSet ps = d.patchSet(); RevCommit commit; if (isEdit && ps.getId().equals(basePs.getId())) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java index 8e0d9d9..d1c085a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java
@@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.BranchOrderSection; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; @@ -108,8 +109,8 @@ return result; } - ChangeData cd = changeDataFactory.create(db.get(), resource.getChangeResource()); - result.submitType = getSubmitType(cd, ps); + ChangeData cd = changeDataFactory.create(db.get(), resource.getNotes()); + result.submitType = getSubmitType(resource.getUser(), cd, ps); try (Repository git = gitManager.openRepository(change.getProject())) { ObjectId commit = toId(ps); @@ -141,9 +142,10 @@ return result; } - private SubmitType getSubmitType(ChangeData cd, PatchSet patchSet) throws OrmException { + private SubmitType getSubmitType(CurrentUser user, ChangeData cd, PatchSet patchSet) + throws OrmException { SubmitTypeRecord rec = - submitRuleEvaluatorFactory.create(cd).setPatchSet(patchSet).getSubmitType(); + submitRuleEvaluatorFactory.create(user, cd).setPatchSet(patchSet).getSubmitType(); if (rec.status != SubmitTypeRecord.Status.OK) { throw new OrmException("Submit type rule failed: " + rec); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 81edada..58634a5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -50,6 +50,7 @@ import com.google.gerrit.extensions.api.changes.ReviewResult; import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.client.Comment.Range; +import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.AccountInfo; @@ -92,7 +93,11 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.patch.DiffSummary; +import com.google.gerrit.server.patch.DiffSummaryKey; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListKey; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.permissions.ChangePermission; import com.google.gerrit.server.permissions.LabelPermission; import com.google.gerrit.server.permissions.PermissionBackend; @@ -129,6 +134,7 @@ import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; @Singleton public class PostReview @@ -206,14 +212,14 @@ protected Response<ReviewResult> applyImpl( BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input) throws RestApiException, UpdateException, OrmException, IOException, - PermissionBackendException, ConfigInvalidException { + PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException { return apply(updateFactory, revision, input, TimeUtil.nowTs()); } public Response<ReviewResult> apply( BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input, Timestamp ts) throws RestApiException, UpdateException, OrmException, IOException, - PermissionBackendException, ConfigInvalidException { + PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException { // Respect timestamp, but truncate at change created-on time. ts = Ordering.natural().max(ts, revision.getChange().getCreatedOn()); if (revision.getEdit().isPresent()) { @@ -553,7 +559,7 @@ private <T extends CommentInput> void checkComments( RevisionResource revision, Map<String, List<T>> commentsPerPath) - throws OrmException, BadRequestException { + throws BadRequestException, PatchListNotAvailableException { Set<String> revisionFilePaths = getAffectedFilePaths(revision); for (Map.Entry<String, List<T>> entry : commentsPerPath.entrySet()) { String path = entry.getKey(); @@ -569,9 +575,14 @@ } } - private Set<String> getAffectedFilePaths(RevisionResource revision) throws OrmException { - ChangeData changeData = changeDataFactory.create(db.get(), revision.getChangeResource()); - return new HashSet<>(changeData.filePaths(revision.getPatchSet())); + private Set<String> getAffectedFilePaths(RevisionResource revision) + throws PatchListNotAvailableException { + ObjectId newId = ObjectId.fromString(revision.getPatchSet().getRevision().get()); + DiffSummaryKey key = + DiffSummaryKey.fromPatchListKey( + PatchListKey.againstDefaultBase(newId, Whitespace.IGNORE_NONE)); + DiffSummary ds = patchListCache.getDiffSummary(key, revision.getProject()); + return new HashSet<>(ds.getPaths()); } private static void ensurePathRefersToAvailableOrMagicFile( @@ -600,7 +611,7 @@ private void checkRobotComments( RevisionResource revision, Map<String, List<RobotCommentInput>> in) - throws BadRequestException, OrmException { + throws BadRequestException, PatchListNotAvailableException { cleanUpComments(in); for (Map.Entry<String, List<RobotCommentInput>> e : in.entrySet()) { String commentPath = e.getKey(); @@ -1107,7 +1118,7 @@ if (ctx.getAccountId().equals(ctx.getChange().getOwner())) { return true; } - ChangeData cd = changeDataFactory.create(db.get(), ctx); + ChangeData cd = changeDataFactory.create(db.get(), ctx.getNotes()); ReviewerSet reviewers = cd.reviewers(); if (reviewers.byState(REVIEWER).contains(ctx.getAccountId())) { return true;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index 7b9f099..f642aa4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
@@ -446,7 +446,7 @@ result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size()); for (Account.Id accountId : opResult.addedCCs()) { IdentifiedUser u = identifiedUserFactory.create(accountId); - result.ccs.add(json.format(new ReviewerInfo(accountId.get()), perm.user(u), cd)); + result.ccs.add(json.format(caller, new ReviewerInfo(accountId.get()), perm.user(u), cd)); } accountLoaderFactory.create(true).fill(result.ccs); for (Address a : reviewersByEmail) { @@ -459,6 +459,7 @@ IdentifiedUser u = identifiedUserFactory.create(psa.getAccountId()); result.reviewers.add( json.format( + caller, new ReviewerInfo(psa.getAccountId().get()), perm.user(u), cd,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java index 4810a02..86d6e81 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java
@@ -27,7 +27,9 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; @@ -53,20 +55,23 @@ @Singleton class RelatedChangesSorter { private final GitRepositoryManager repoManager; + private final ProjectControl.GenericFactory projectControlFactory; @Inject - RelatedChangesSorter(GitRepositoryManager repoManager) { + RelatedChangesSorter( + GitRepositoryManager repoManager, ProjectControl.GenericFactory projectControlFactory) { this.repoManager = repoManager; + this.projectControlFactory = projectControlFactory; } - public List<PatchSetData> sort(List<ChangeData> in, PatchSet startPs) - throws OrmException, IOException { + public List<PatchSetData> sort(List<ChangeData> in, PatchSet startPs, CurrentUser user) + throws OrmException, IOException, NoSuchProjectException { checkArgument(!in.isEmpty(), "Input may not be empty"); // Map of all patch sets, keyed by commit SHA-1. Map<String, PatchSetData> byId = collectById(in); PatchSetData start = byId.get(startPs.getRevision().get()); checkArgument(start != null, "%s not found in %s", startPs, in); - ProjectControl ctl = start.data().changeControl().getProjectControl(); + ProjectControl ctl = projectControlFactory.controlFor(start.data().project(), user); // Map of patch set -> immediate parent. ListMultimap<PatchSetData, PatchSetData> parents =
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java index 4b2f5b7..5ff2cd8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.permissions.LabelPermission; import com.google.gerrit.server.permissions.PermissionBackend; @@ -77,6 +78,7 @@ } ReviewerInfo info = format( + rsrc.getChangeResource().getUser(), new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()), permissionBackend.user(rsrc.getReviewerUser()).database(db).change(cd), cd); @@ -92,10 +94,12 @@ return format(ImmutableList.<ReviewerResource>of(rsrc)); } - public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd) + public ReviewerInfo format( + CurrentUser user, ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd) throws OrmException, PermissionBackendException { PatchSet.Id psId = cd.change().currentPatchSetId(); return format( + user, out, perm, cd, @@ -104,6 +108,7 @@ } public ReviewerInfo format( + CurrentUser user, ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd, @@ -125,7 +130,7 @@ if (ps != null) { for (SubmitRecord rec : submitRuleEvaluatorFactory - .create(cd) + .create(user, cd) .setFastEvalLabels(true) .setAllowDraft(true) .evaluate()) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index 78aae96..f648e1b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -322,12 +322,7 @@ } ReviewDb db = dbProvider.get(); - ChangeData cd; - try { - cd = changeDataFactory.create(db, resource.getChangeResource()); - } catch (NoSuchChangeException e) { - return null; // submit not visible - } + ChangeData cd = changeDataFactory.create(db, resource.getNotes()); try { MergeOp.checkSubmitRule(cd, false); } catch (ResourceConflictException e) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java index 371c796..1792c83 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java
@@ -71,7 +71,7 @@ input.filters = MoreObjects.firstNonNull(input.filters, filters); SubmitRuleEvaluator evaluator = submitRuleEvaluatorFactory.create( - changeDataFactory.create(db.get(), rsrc.getChangeResource())); + rsrc.getUser(), changeDataFactory.create(db.get(), rsrc.getNotes())); List<SubmitRecord> records = evaluator
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java index 0dd1019..ca6f9cf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java
@@ -65,7 +65,7 @@ input.filters = MoreObjects.firstNonNull(input.filters, filters); SubmitRuleEvaluator evaluator = submitRuleEvaluatorFactory.create( - changeDataFactory.create(db.get(), rsrc.getChangeResource())); + rsrc.getUser(), changeDataFactory.create(db.get(), rsrc.getNotes())); SubmitTypeRecord rec = evaluator
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 73a3bc2..b6fbbe2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -327,7 +327,8 @@ return allowClosed ? SUBMIT_RULE_OPTIONS_ALLOW_CLOSED : SUBMIT_RULE_OPTIONS; } - private static List<SubmitRecord> getSubmitRecords(ChangeData cd, boolean allowClosed) { + private static List<SubmitRecord> getSubmitRecords(ChangeData cd, boolean allowClosed) + throws OrmException { return cd.submitRecords(submitRuleOptions(allowClosed)); } @@ -391,7 +392,7 @@ commitStatus.maybeFailVerbose(); } - private void bypassSubmitRules(ChangeSet cs, boolean allowClosed) { + private void bypassSubmitRules(ChangeSet cs, boolean allowClosed) throws OrmException { checkArgument( !cs.furtherHiddenChanges(), "cannot bypass submit rules for topic with hidden change"); for (ChangeData cd : cs.changes()) { @@ -705,15 +706,16 @@ Change.Id changeId = cd.getId(); ChangeNotes notes; Change chg; + SubmitType st; try { notes = cd.notes(); chg = cd.change(); + st = getSubmitType(cd); } catch (OrmException e) { commitStatus.logProblem(changeId, e); continue; } - SubmitType st = getSubmitType(cd); if (st == null) { commitStatus.logProblem(changeId, "No submit type for change"); continue; @@ -828,7 +830,7 @@ } } - private SubmitType getSubmitType(ChangeData cd) { + private SubmitType getSubmitType(ChangeData cd) throws OrmException { SubmitTypeRecord str = cd.submitTypeRecord(); return str.isOk() ? str.type : null; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java index 32dc7bc..4e0c3ae 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java
@@ -160,7 +160,7 @@ } } - private SubmitType submitType(ChangeData cd, PatchSet ps, boolean visible) + private SubmitType submitType(CurrentUser user, ChangeData cd, PatchSet ps, boolean visible) throws OrmException, IOException { // Submit type prolog rules mean that the submit type can depend on the // submitting user and the content of the change. @@ -179,7 +179,7 @@ SubmitTypeRecord str = ps == cd.currentPatchSet() ? cd.submitTypeRecord() - : submitRuleEvaluatorFactory.create(cd).setPatchSet(ps).getSubmitType(); + : submitRuleEvaluatorFactory.create(user, cd).setPatchSet(ps).getSubmitType(); if (!str.isOk()) { logErrorAndThrow("Failed to get submit type for " + cd.getId() + ": " + str.errorMessage); } @@ -258,7 +258,7 @@ } } - if (submitType(cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) { + if (submitType(user, cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) { if (visible) { visibleChanges.add(cd); } else {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java index de95f61..c91e5cf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -299,7 +299,7 @@ recipients.add( getRecipientsFromFooters(ctx.getDb(), accountResolver, draft, commit.getFooterLines())); recipients.remove(ctx.getAccountId()); - ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx); + ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx.getNotes()); MailRecipients oldRecipients = getRecipientsFromReviewers(cd.reviewers()); Iterable<PatchSetApproval> newApprovals = approvalsUtil.addApprovalsForNewPatchSet(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java index 51e47a0..8ccf081 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java
@@ -267,7 +267,7 @@ if (!cd.currentFilePaths().contains(AccountConfig.ACCOUNT_CONFIG)) { return; } - } catch (OrmException e) { + } catch (IOException | OrmException e) { log.error("Cannot validate account update", e); throw new MergeValidationException("account validation unavailable"); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java index ff8396b..0f9cc89 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java
@@ -33,6 +33,7 @@ import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResource; import com.google.gerrit.server.account.GetGroups; @@ -75,6 +76,8 @@ private final GetGroups accountGetGroups; private final GroupJson json; private final GroupBackend groupBackend; + private final Groups groups; + private final Provider<ReviewDb> db; private EnumSet<ListGroupsOption> options = EnumSet.noneOf(ListGroupsOption.class); private boolean visibleToAll; @@ -215,7 +218,9 @@ final IdentifiedUser.GenericFactory userFactory, final GetGroups accountGetGroups, GroupJson json, - GroupBackend groupBackend) { + GroupBackend groupBackend, + Groups groups, + Provider<ReviewDb> db) { this.groupCache = groupCache; this.groupControlFactory = groupControlFactory; this.genericGroupControlFactory = genericGroupControlFactory; @@ -224,6 +229,8 @@ this.accountGetGroups = accountGetGroups; this.json = json; this.groupBackend = groupBackend; + this.groups = groups; + this.db = db; } public void setOptions(EnumSet<ListGroupsOption> options) { @@ -275,7 +282,7 @@ if (!projects.isEmpty()) { Map<AccountGroup.UUID, GroupDescription.Internal> groups = new HashMap<>(); for (ProjectControl projectControl : projects) { - final Set<GroupReference> groupsRefs = projectControl.getAllGroups(); + final Set<GroupReference> groupsRefs = projectControl.getProjectState().getAllGroups(); for (GroupReference groupRef : groupsRefs) { Optional<InternalGroup> internalGroup = groupCache.get(groupRef.getUUID()); internalGroup.ifPresent( @@ -379,11 +386,14 @@ } private ImmutableList<GroupDescription.Internal> getAllExistingInternalGroups() { - return groupCache - .all() - .stream() - .map(GroupDescriptions::forAccountGroup) - .collect(toImmutableList()); + try { + return groups + .getAll(db.get()) + .map(GroupDescriptions::forAccountGroup) + .collect(toImmutableList()); + } catch (OrmException e) { + return ImmutableList.of(); + } } private List<GroupDescription.Internal> filterGroups(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java index f0421a5..57dff1f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java
@@ -15,6 +15,7 @@ package com.google.gerrit.server.group; import static com.google.common.base.Preconditions.checkNotNull; +import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; import com.google.common.annotations.VisibleForTesting; @@ -24,15 +25,17 @@ import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.StartupCheck; import com.google.gerrit.server.StartupException; import com.google.gerrit.server.account.AbstractGroupBackend; -import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.ListGroupMembership; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.project.ProjectState; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Singleton; import java.util.ArrayList; @@ -184,12 +187,14 @@ public static class NameCheck implements StartupCheck { private final Config cfg; - private final GroupCache groupCache; + private final Groups groups; + private final SchemaFactory<ReviewDb> schema; @Inject - NameCheck(@GerritServerConfig Config cfg, GroupCache groupCache) { + NameCheck(@GerritServerConfig Config cfg, Groups groups, SchemaFactory<ReviewDb> schema) { this.cfg = cfg; - this.groupCache = groupCache; + this.groups = groups; + this.schema = schema; } @Override @@ -206,7 +211,13 @@ if (configuredNames.isEmpty()) { return; } - for (AccountGroup g : groupCache.all()) { + List<AccountGroup> allGroups; + try { + allGroups = groups.getAll(schema.open()).collect(toList()); + } catch (OrmException e) { + return; + } + for (AccountGroup g : allGroups) { String name = g.getName().toLowerCase(Locale.US); if (byLowerCaseConfiguredName.keySet().contains(name)) { AccountGroup.UUID uuidSystemGroup = byLowerCaseConfiguredName.get(name);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java index db71ef5..24bde80 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -145,10 +145,13 @@ .buildRepeatable(cd -> firstNonNull(cd.currentFilePaths(), ImmutableList.of())); public static Set<String> getFileParts(ChangeData cd) throws OrmException { - List<String> paths = cd.currentFilePaths(); - if (paths == null) { - return ImmutableSet.of(); + List<String> paths; + try { + paths = cd.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); } + Splitter s = Splitter.on('/').omitEmptyStrings(); Set<String> r = new HashSet<>(); for (String path : paths) { @@ -658,7 +661,8 @@ return Lists.transform(records, r -> GSON.toJson(new StoredSubmitRecord(r)).getBytes(UTF_8)); } - private static Iterable<byte[]> storedSubmitRecords(ChangeData cd, SubmitRuleOptions opts) { + private static Iterable<byte[]> storedSubmitRecords(ChangeData cd, SubmitRuleOptions opts) + throws OrmException { return storedSubmitRecords(cd.submitRecords(opts)); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java index 129c8ac..41e8d10 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -86,7 +86,9 @@ ChangeField.PENDING_REVIEWER, ChangeField.PENDING_REVIEWER_BY_EMAIL); - static final Schema<ChangeData> V45 = schema(V44, ChangeField.REVERT_OF); + @Deprecated static final Schema<ChangeData> V45 = schema(V44, ChangeField.REVERT_OF); + + static final Schema<ChangeData> V46 = schema(V45); public static final String NAME = "changes"; public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java index eedd9da..53e7d22 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -177,7 +177,7 @@ authors = getAuthors(); try { - stars = args.starredChangesUtil.byChangeFromIndex(change.getId()); + stars = changeData.stars(); } catch (OrmException e) { throw new EmailException("Failed to load stars for change " + change.getChangeId(), e); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java index ffe3e8f..869d7d1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java
@@ -22,7 +22,6 @@ import com.google.gerrit.server.GerritPersonIdentProvider; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser.GenericFactory; -import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupIncludeCache; @@ -80,7 +79,6 @@ final SoyTofu soyTofu; final EmailSettings settings; final DynamicSet<OutgoingEmailValidationListener> outgoingEmailValidationListeners; - final StarredChangesUtil starredChangesUtil; final Provider<InternalAccountQuery> accountQueryProvider; final OutgoingEmailValidator validator; @@ -114,7 +112,6 @@ @SshAdvertisedAddresses List<String> sshAddresses, SitePaths site, DynamicSet<OutgoingEmailValidationListener> outgoingEmailValidationListeners, - StarredChangesUtil starredChangesUtil, Provider<InternalAccountQuery> accountQueryProvider, OutgoingEmailValidator validator) { this.server = server; @@ -145,7 +142,6 @@ this.sshAddresses = sshAddresses; this.site = site; this.outgoingEmailValidationListeners = outgoingEmailValidationListeners; - this.starredChangesUtil = starredChangesUtil; this.accountQueryProvider = accountQueryProvider; this.validator = validator; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java index 2afb4a5..425ac65 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java
@@ -72,7 +72,7 @@ args.approvalsUtil.byPatchSet( args.db.get(), changeData.notes(), - changeData.changeControl().getUser(), + args.identifiedUserFactory.create(changeData.change().getOwner()), patchSet.getId(), null, null)) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java index c32a3f6..728d227 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java
@@ -30,9 +30,6 @@ IntraLineDiff getIntraLineDiff(IntraLineDiffKey key, IntraLineDiffArgs args); - DiffSummary getDiffSummary(Change change, PatchSet patchSet) - throws PatchListNotAvailableException; - DiffSummary getDiffSummary(DiffSummaryKey key, Project.NameKey project) throws PatchListNotAvailableException; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index b985723..7777400 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
@@ -164,16 +164,6 @@ } @Override - public DiffSummary getDiffSummary(Change change, PatchSet patchSet) - throws PatchListNotAvailableException { - Project.NameKey project = change.getProject(); - ObjectId b = ObjectId.fromString(patchSet.getRevision().get()); - Whitespace ws = Whitespace.IGNORE_NONE; - return getDiffSummary( - DiffSummaryKey.fromPatchListKey(PatchListKey.againstDefaultBase(b, ws)), project); - } - - @Override public DiffSummary getDiffSummary(DiffSummaryKey key, Project.NameKey project) throws PatchListNotAvailableException { try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index b5a3d25..6a55e50 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -397,7 +397,7 @@ } private ChangeData changeData(ReviewDb db, @Nullable ChangeData cd) { - return cd != null ? cd : changeDataFactory.create(db, this); + return cd != null ? cd : changeDataFactory.create(db, getNotes()); } public boolean isDraftVisible(ReviewDb db, ChangeData cd) throws OrmException { @@ -442,7 +442,7 @@ if (cd == null) { ReviewDb reviewDb = db(); checkState(reviewDb != null, "need ReviewDb"); - cd = changeDataFactory.create(reviewDb, ChangeControl.this); + cd = changeDataFactory.create(reviewDb, getNotes()); } return cd; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java index 522aa89..dc3610c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java
@@ -357,7 +357,8 @@ final ProjectControl pctl = e.controlFor(currentUser); if (groupUuid != null - && !pctl.getLocalGroups() + && !pctl.getProjectState() + .getLocalGroups() .contains(GroupReference.forGroup(groupsCollection.parseId(groupUuid.get())))) { continue; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index a3bee39..c5425e8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -23,7 +23,6 @@ import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.Capable; import com.google.gerrit.common.data.ContributorAgreement; -import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule.Action; @@ -141,7 +140,6 @@ private final Metrics metrics; private List<SectionMatcher> allSections; - private List<SectionMatcher> localSections; private Map<String, RefControl> refControls; private Boolean declaredOwner; @@ -216,6 +214,54 @@ return state.getProject(); } + public boolean allRefsAreVisible(Set<String> ignore) { + // TODO(hiesel) Hide refs/changes and replace this method by a proper READ check of all refs + return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore); + } + + /** Is this user a project owner? */ + public boolean isOwner() { + return (isDeclaredOwner() && !controlForRef("refs/*").isBlocked(Permission.OWNER)) || isAdmin(); + } + + /** @return {@code Capable.OK} if the user can upload to at least one reference */ + public Capable canPushToAtLeastOneRef() { + if (!canPerformOnAnyRef(Permission.PUSH) + && !canPerformOnAnyRef(Permission.CREATE_TAG) + && !isOwner()) { + return new Capable("Upload denied for project '" + state.getName() + "'"); + } + if (state.isUseContributorAgreements()) { + return verifyActiveContributorAgreement(); + } + return Capable.OK; + } + + /** Can the user run upload pack? */ + public boolean canRunUploadPack() { + for (AccountGroup.UUID group : uploadGroups) { + if (match(group)) { + return true; + } + } + return false; + } + + /** Can the user run receive pack? */ + public boolean canRunReceivePack() { + for (AccountGroup.UUID group : receiveGroups) { + if (match(group)) { + return true; + } + } + return false; + } + + /** Does this user have ownership on at least one reference name? */ + public boolean isOwnerAnyRef() { + return canPerformOnAnyRef(Permission.OWNER) || isAdmin(); + } + /** Returns whether the project is hidden. */ private boolean isHidden() { return getProject().getState().equals(com.google.gerrit.extensions.client.ProjectState.HIDDEN); @@ -238,15 +284,6 @@ return false; } - public boolean allRefsAreVisible(Set<String> ignore) { - return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore); - } - - /** Is this user a project owner? */ - public boolean isOwner() { - return (isDeclaredOwner() && !controlForRef("refs/*").isBlocked(Permission.OWNER)) || isAdmin(); - } - boolean isAdmin() { try { perm.check(GlobalPermission.ADMINISTRATE_SERVER); @@ -264,45 +301,6 @@ return declaredOwner; } - /** Does this user have ownership on at least one reference name? */ - public boolean isOwnerAnyRef() { - return canPerformOnAnyRef(Permission.OWNER) || isAdmin(); - } - - /** @return true if the user can upload to at least one reference */ - public Capable canPushToAtLeastOneRef() { - if (!canPerformOnAnyRef(Permission.PUSH) - && !canPerformOnAnyRef(Permission.CREATE_TAG) - && !isOwner()) { - return new Capable("Upload denied for project '" + state.getName() + "'"); - } - if (state.isUseContributorAgreements()) { - return verifyActiveContributorAgreement(); - } - return Capable.OK; - } - - public Set<GroupReference> getAllGroups() { - return getGroups(access()); - } - - public Set<GroupReference> getLocalGroups() { - return getGroups(localAccess()); - } - - private static Set<GroupReference> getGroups(List<SectionMatcher> sectionMatcherList) { - final Set<GroupReference> all = new HashSet<>(); - for (SectionMatcher matcher : sectionMatcherList) { - final AccessSection section = matcher.section; - for (Permission permission : section.getPermissions()) { - for (PermissionRule rule : permission.getRules()) { - all.add(rule.getGroup()); - } - } - } - return all; - } - private Capable verifyActiveContributorAgreement() { metrics.claCheckCount.increment(); if (!(user.isIdentifiedUser())) { @@ -410,13 +408,6 @@ return allSections; } - private List<SectionMatcher> localAccess() { - if (localSections == null) { - localSections = state.getLocalAccessSections(); - } - return localSections; - } - boolean match(PermissionRule rule) { return match(rule.getGroup().getUUID()); } @@ -439,24 +430,6 @@ } } - public boolean canRunUploadPack() { - for (AccountGroup.UUID group : uploadGroups) { - if (match(group)) { - return true; - } - } - return false; - } - - public boolean canRunReceivePack() { - for (AccountGroup.UUID group : receiveGroups) { - if (match(group)) { - return true; - } - } - return false; - } - boolean isReachableFromHeadsOrTags(Repository repo, RevCommit commit) { try { RefDatabase refdb = repo.getRefDatabase();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java index 72e5ee6..3015164 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
@@ -512,6 +512,27 @@ return theme; } + public Set<GroupReference> getAllGroups() { + return getGroups(getAllSections()); + } + + public Set<GroupReference> getLocalGroups() { + return getGroups(getLocalAccessSections()); + } + + private static Set<GroupReference> getGroups(List<SectionMatcher> sectionMatcherList) { + final Set<GroupReference> all = new HashSet<>(); + for (SectionMatcher matcher : sectionMatcherList) { + final AccessSection section = matcher.section; + for (Permission permission : section.getPermissions()) { + for (PermissionRule rule : permission.getRules()) { + all.add(rule.getGroup()); + } + } + } + return all; + } + private ThemeInfo loadTheme() { String name = getConfig().getProject().getName(); Path dir = sitePaths.themes_dir.resolve(name);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index 3736360..8aa475b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
@@ -32,6 +32,7 @@ import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; +import com.google.inject.util.Providers; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -517,7 +518,10 @@ @Override public ForChange change(ChangeData cd) { try { - return cd.changeControl().forUser(getUser()).asForChange(cd, db); + // TODO(hiesel) Force callers to call database() and use db instead of cd.db() + return getProjectControl() + .controlFor(cd.db(), cd.change()) + .asForChange(cd, Providers.of(cd.db())); } catch (OrmException e) { return FailedPermissionBackend.change("unavailable", e); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index 87d10c9..e4352e4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.rules.PrologEnvironment; import com.google.gerrit.rules.StoredValues; import com.google.gerrit.server.CurrentUser; @@ -33,6 +34,7 @@ import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; import com.googlecode.prolog_cafe.exceptions.CompileException; import com.googlecode.prolog_cafe.exceptions.ReductionLimitException; @@ -43,6 +45,7 @@ import com.googlecode.prolog_cafe.lang.SymbolTerm; import com.googlecode.prolog_cafe.lang.Term; import com.googlecode.prolog_cafe.lang.VariableTerm; +import java.io.IOException; import java.io.StringReader; import java.util.ArrayList; import java.util.Collections; @@ -87,29 +90,45 @@ } public interface Factory { - SubmitRuleEvaluator create(ChangeData cd); + SubmitRuleEvaluator create(CurrentUser user, ChangeData cd); } private final AccountCache accountCache; private final Accounts accounts; private final Emails emails; + private final ProjectCache projectCache; + private final Provider<ReviewDb> dbProvider; + private final ChangeControl.GenericFactory changeControlFactory; private final ChangeData cd; private SubmitRuleOptions.Builder optsBuilder = SubmitRuleOptions.defaults(); private SubmitRuleOptions opts; + private Change change; + private CurrentUser user; private PatchSet patchSet; private boolean logErrors = true; private long reductionsConsumed; - private ChangeControl control; + private ProjectState projectState; private Term submitRule; @Inject SubmitRuleEvaluator( - AccountCache accountCache, Accounts accounts, Emails emails, @Assisted ChangeData cd) { + AccountCache accountCache, + Accounts accounts, + Emails emails, + ProjectCache projectCache, + Provider<ReviewDb> dbProvider, + ChangeControl.GenericFactory changeControlFactory, + @Assisted CurrentUser user, + @Assisted ChangeData cd) { this.accountCache = accountCache; this.accounts = accounts; this.emails = emails; + this.projectCache = projectCache; + this.dbProvider = dbProvider; + this.changeControlFactory = changeControlFactory; + this.user = user; this.cd = cd; } @@ -225,19 +244,18 @@ public List<SubmitRecord> evaluate() { initOptions(); try { - initChange(); + init(); } catch (OrmException e) { return ruleError("Error looking up change " + cd.getId(), e); } - Change c = control.getChange(); - if (!opts.allowClosed() && c.getStatus().isClosed()) { + if (!opts.allowClosed() && change.getStatus().isClosed()) { SubmitRecord rec = new SubmitRecord(); rec.status = SubmitRecord.Status.CLOSED; return Collections.singletonList(rec); } if (!opts.allowDraft()) { - if (c.getStatus() == Change.Status.DRAFT || patchSet.isDraft()) { + if (change.getStatus() == Change.Status.DRAFT || patchSet.isDraft()) { return cannotSubmitDraft(); } } @@ -250,7 +268,7 @@ "can_submit", "locate_submit_filter", "filter_submit_results", - control.getUser()); + user); } catch (RuleEvalException e) { return ruleError(e.getMessage(), e); } @@ -271,7 +289,9 @@ private List<SubmitRecord> cannotSubmitDraft() { try { - if (!control.isDraftVisible(cd.db(), cd)) { + if (!changeControlFactory + .controlFor(dbProvider.get(), change, user) + .isDraftVisible(cd.db(), cd)) { return createRuleError("Patch set " + patchSet.getId() + " not found"); } if (patchSet.isDraft()) { @@ -279,8 +299,7 @@ } return createRuleError("Cannot submit draft changes"); } catch (OrmException err) { - PatchSet.Id psId = - patchSet != null ? patchSet.getId() : control.getChange().currentPatchSetId(); + PatchSet.Id psId = patchSet != null ? patchSet.getId() : patchSet.getId(); String msg = "Cannot check visibility of patch set " + psId; log.error(msg, err); return createRuleError(msg); @@ -414,17 +433,22 @@ public SubmitTypeRecord getSubmitType() { initOptions(); try { - initChange(); + init(); } catch (OrmException e) { return typeError("Error looking up change " + cd.getId(), e); } try { - if (control.getChange().getStatus() == Change.Status.DRAFT - && !control.isDraftVisible(cd.db(), cd)) { + if (change.getStatus() == Change.Status.DRAFT + && !changeControlFactory + .controlFor(dbProvider.get(), change, user) + .isDraftVisible(cd.db(), cd)) { return SubmitTypeRecord.error("Patch set " + patchSet.getId() + " not found"); } - if (patchSet.isDraft() && !control.isDraftVisible(cd.db(), cd)) { + if (patchSet.isDraft() + && !changeControlFactory + .controlFor(dbProvider.get(), change, user) + .isDraftVisible(cd.db(), cd)) { return SubmitTypeRecord.error("Patch set " + patchSet.getId() + " not found"); } } catch (OrmException err) { @@ -561,7 +585,6 @@ } private PrologEnvironment getPrologEnvironment(CurrentUser user) throws RuleEvalException { - ProjectState projectState = control.getProjectControl().getProjectState(); PrologEnvironment env; try { if (opts.rule() == null) { @@ -571,12 +594,10 @@ } } catch (CompileException err) { String msg; - if (opts.rule() == null && control.getProjectControl().isOwner()) { + if (opts.rule() == null) { msg = String.format("Cannot load rules.pl for %s: %s", getProjectName(), err.getMessage()); - } else if (opts.rule() != null) { - msg = err.getMessage(); } else { - msg = String.format("Cannot load rules.pl for %s", getProjectName()); + msg = err.getMessage(); } throw new RuleEvalException(msg, err); } @@ -598,7 +619,6 @@ String filterRuleLocatorName, String filterRuleWrapperName) throws RuleEvalException { - ProjectState projectState = control.getProjectControl().getProjectState(); PrologEnvironment childEnv = env; for (ProjectState parentState : projectState.parents()) { PrologEnvironment parentEnv; @@ -684,9 +704,20 @@ } } - private void initChange() throws OrmException { - if (control == null) { - control = cd.changeControl(); + private void init() throws OrmException { + if (change == null) { + change = cd.change(); + if (change == null) { + throw new OrmException("No change found"); + } + } + + if (projectState == null) { + try { + projectState = projectCache.checkedGet(change.getProject()); + } catch (IOException e) { + throw new OrmException("Can't load project state", e); + } } if (patchSet == null) { @@ -698,6 +729,6 @@ } private String getProjectName() { - return control.getProjectControl().getProjectState().getName(); + return projectState.getName(); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index e3ee265..30bb760 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -35,6 +35,7 @@ import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitTypeRecord; +import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.Account; @@ -58,7 +59,6 @@ import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.StarredChangesUtil.StarRef; -import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.GetPureRevert; import com.google.gerrit.server.change.MergeabilityCache; import com.google.gerrit.server.config.AllUsersName; @@ -68,7 +68,9 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.DiffSummary; +import com.google.gerrit.server.patch.DiffSummaryKey; import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListKey; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; @@ -76,7 +78,6 @@ import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.project.SubmitRuleOptions; -import com.google.gerrit.server.update.ChangeContext; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; @@ -282,44 +283,23 @@ public static class Factory { private final AssistedFactory assistedFactory; - private final ChangeControl.GenericFactory changeControlFactory; @Inject - Factory(AssistedFactory assistedFactory, ChangeControl.GenericFactory changeControlFactory) { + Factory(AssistedFactory assistedFactory) { this.assistedFactory = assistedFactory; - this.changeControlFactory = changeControlFactory; } public ChangeData create(ReviewDb db, Project.NameKey project, Change.Id id) { - return assistedFactory.create(db, project, id, null, null, null); + return assistedFactory.create(db, project, id, null, null); } public ChangeData create(ReviewDb db, Change change) { - return assistedFactory.create(db, change.getProject(), change.getId(), change, null, null); + return assistedFactory.create(db, change.getProject(), change.getId(), change, null); } public ChangeData create(ReviewDb db, ChangeNotes notes) { return assistedFactory.create( - db, notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes, null); - } - - public ChangeData create(ReviewDb db, ChangeControl control) { - return assistedFactory.create( - db, - control.getChange().getProject(), - control.getId(), - control.getChange(), - control.getNotes(), - control); - } - - // TODO(hiesel): Remove these after ChangeControl is removed from ChangeData - public ChangeData create(ReviewDb db, ChangeResource rsrc) throws NoSuchChangeException { - return create(db, changeControlFactory.controlFor(rsrc.getNotes(), rsrc.getUser())); - } - - public ChangeData create(ReviewDb db, ChangeContext ctx) throws NoSuchChangeException { - return create(db, changeControlFactory.controlFor(ctx.getNotes(), ctx.getUser())); + db, notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes); } } @@ -329,8 +309,7 @@ Project.NameKey project, Change.Id id, @Nullable Change change, - @Nullable ChangeNotes notes, - @Nullable ChangeControl control); + @Nullable ChangeNotes notes); } /** @@ -347,7 +326,7 @@ ChangeData cd = new ChangeData( null, null, null, null, null, null, null, null, null, null, null, null, null, null, - null, null, null, null, null, project, id, null, null, null); + null, null, null, null, null, project, id, null, null); cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId)); return cd; } @@ -356,7 +335,6 @@ private @Nullable final StarredChangesUtil starredChangesUtil; private final AllUsersName allUsersName; private final ApprovalsUtil approvalsUtil; - private final ChangeControl.GenericFactory changeControlFactory; private final ChangeMessagesUtil cmUtil; private final ChangeNotes.Factory notesFactory; private final CommentsUtil commentsUtil; @@ -371,6 +349,7 @@ private final TrackingFooters trackingFooters; private final GetPureRevert pureRevert; private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory; + private final ChangeControl.GenericFactory changeControlFactory; // Required assisted injected fields. private final ReviewDb db; @@ -391,8 +370,8 @@ private Collection<PatchSet> patchSets; private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals; private List<PatchSetApproval> currentApprovals; - private Map<Integer, List<String>> files; - private Map<Integer, Optional<DiffSummary>> diffSummaries; + private List<String> currentFiles; + private Optional<DiffSummary> diffSummary; private Collection<Comment> publishedComments; private Collection<RobotComment> robotComments; private CurrentUser visibleTo; @@ -415,6 +394,7 @@ private List<ReviewerStatusUpdate> reviewerUpdates; private PersonIdent author; private PersonIdent committer; + private int parentCount; private Integer unresolvedCommentCount; private LabelTypes labelTypes; @@ -426,7 +406,6 @@ @Nullable StarredChangesUtil starredChangesUtil, ApprovalsUtil approvalsUtil, AllUsersName allUsersName, - ChangeControl.GenericFactory changeControlFactory, ChangeMessagesUtil cmUtil, ChangeNotes.Factory notesFactory, CommentsUtil commentsUtil, @@ -441,15 +420,14 @@ TrackingFooters trackingFooters, GetPureRevert pureRevert, SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory, + ChangeControl.GenericFactory changeControlFactory, @Assisted ReviewDb db, @Assisted Project.NameKey project, @Assisted Change.Id id, @Assisted @Nullable Change change, - @Assisted @Nullable ChangeNotes notes, - @Assisted @Nullable ChangeControl control) { + @Assisted @Nullable ChangeNotes notes) { this.approvalsUtil = approvalsUtil; this.allUsersName = allUsersName; - this.changeControlFactory = changeControlFactory; this.cmUtil = cmUtil; this.notesFactory = notesFactory; this.commentsUtil = commentsUtil; @@ -465,6 +443,7 @@ this.trackingFooters = trackingFooters; this.pureRevert = pureRevert; this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory; + this.changeControlFactory = changeControlFactory; // May be null in tests when created via createForTest above, in which case lazy-loading will // intentionally fail with NPE. Still not marked @Nullable in the constructor, to force callers @@ -476,7 +455,6 @@ this.change = change; this.notes = notes; - this.changeControl = control; } public ChangeData setLazyLoad(boolean load) { @@ -492,86 +470,61 @@ return allUsersName; } - private Map<Integer, List<String>> initFiles() { - if (files == null) { - files = new HashMap<>(); - } - return files; - } - public void setCurrentFilePaths(List<String> filePaths) throws OrmException { PatchSet ps = currentPatchSet(); if (ps != null) { - initFiles().put(ps.getPatchSetId(), ImmutableList.copyOf(filePaths)); + currentFiles = ImmutableList.copyOf(filePaths); } } - public List<String> currentFilePaths() throws OrmException { - PatchSet ps = currentPatchSet(); - return ps != null ? filePaths(ps) : null; - } - - public List<String> filePaths(PatchSet ps) throws OrmException { - Integer psId = ps.getPatchSetId(); - List<String> r = initFiles().get(psId); - if (r == null) { - Change c = change(); - if (c == null) { - return null; + public List<String> currentFilePaths() throws IOException, OrmException { + if (currentFiles == null) { + if (!lazyLoad) { + return Collections.emptyList(); } - - Optional<DiffSummary> p = getDiffSummary(c, ps); - if (!p.isPresent()) { - List<String> emptyFileList = Collections.emptyList(); - if (lazyLoad) { - files.put(ps.getPatchSetId(), emptyFileList); - } - return emptyFileList; - } - - r = p.get().getPaths(); - files.put(psId, r); + Optional<DiffSummary> p = getDiffSummary(); + currentFiles = p.map(DiffSummary::getPaths).orElse(Collections.emptyList()); } - return r; + return currentFiles; } - private Optional<DiffSummary> getDiffSummary(Change c, PatchSet ps) { - Integer psId = ps.getId().get(); - if (diffSummaries == null) { - diffSummaries = new HashMap<>(); - } - Optional<DiffSummary> r = diffSummaries.get(psId); - if (r == null) { + private Optional<DiffSummary> getDiffSummary() throws OrmException, IOException { + if (diffSummary == null) { if (!lazyLoad) { return Optional.empty(); } - try { - r = Optional.of(patchListCache.getDiffSummary(c, ps)); - } catch (PatchListNotAvailableException e) { - r = Optional.empty(); + + Change c = change(); + PatchSet ps = currentPatchSet(); + if (c == null || ps == null || !loadCommitData()) { + return Optional.empty(); } - diffSummaries.put(psId, r); + + ObjectId id = ObjectId.fromString(ps.getRevision().get()); + Whitespace ws = Whitespace.IGNORE_NONE; + PatchListKey pk = + parentCount > 1 + ? PatchListKey.againstParentNum(1, id, ws) + : PatchListKey.againstDefaultBase(id, ws); + DiffSummaryKey key = DiffSummaryKey.fromPatchListKey(pk); + try { + diffSummary = Optional.of(patchListCache.getDiffSummary(key, c.getProject())); + } catch (PatchListNotAvailableException e) { + diffSummary = Optional.empty(); + } } - return r; + return diffSummary; } - private Optional<ChangedLines> computeChangedLines() throws OrmException { - Change c = change(); - if (c == null) { - return Optional.empty(); - } - PatchSet ps = currentPatchSet(); - if (ps == null) { - return Optional.empty(); - } - Optional<DiffSummary> ds = getDiffSummary(c, ps); + private Optional<ChangedLines> computeChangedLines() throws OrmException, IOException { + Optional<DiffSummary> ds = getDiffSummary(); if (ds.isPresent()) { return Optional.of(ds.get().getChangedLines()); } return Optional.empty(); } - public Optional<ChangedLines> changedLines() throws OrmException { + public Optional<ChangedLines> changedLines() throws OrmException, IOException { if (changedLines == null) { if (!lazyLoad) { return Optional.empty(); @@ -601,7 +554,8 @@ return visibleTo == user; } - public ChangeControl changeControl() throws OrmException { + private ChangeControl changeControl() throws OrmException { + // TODO(hiesel): Remove this method. if (changeControl == null) { Change c = change(); try { @@ -648,8 +602,7 @@ } catch (IOException e) { throw new OrmException("project state not available", e); } - labelTypes = - state.getLabelTypes(changeControl().getChange().getDest(), changeControl().getUser()); + labelTypes = state.getLabelTypes(change().getDest(), userFactory.create(change().getOwner())); } return labelTypes; } @@ -693,7 +646,12 @@ currentApprovals = ImmutableList.copyOf( approvalsUtil.byPatchSet( - db, notes(), changeControl().getUser(), c.currentPatchSetId(), null, null)); + db, + notes(), + userFactory.create(c.getOwner()), + c.currentPatchSetId(), + null, + null)); } catch (OrmException e) { if (e.getCause() instanceof NoSuchChangeException) { currentApprovals = Collections.emptyList(); @@ -765,6 +723,7 @@ commitFooters = c.getFooterLines(); author = c.getAuthorIdent(); committer = c.getCommitterIdent(); + parentCount = c.getParentCount(); } return true; } @@ -965,13 +924,17 @@ return messages; } - public List<SubmitRecord> submitRecords(SubmitRuleOptions options) { + public List<SubmitRecord> submitRecords(SubmitRuleOptions options) throws OrmException { List<SubmitRecord> records = submitRecords.get(options); if (records == null) { if (!lazyLoad) { return Collections.emptyList(); } - records = submitRuleEvaluatorFactory.create(this).setOptions(options).evaluate(); + records = + submitRuleEvaluatorFactory + .create(userFactory.create(change().getOwner()), this) + .setOptions(options) + .evaluate(); submitRecords.put(options, records); } return records; @@ -986,9 +949,12 @@ submitRecords.put(options, records); } - public SubmitTypeRecord submitTypeRecord() { + public SubmitTypeRecord submitTypeRecord() throws OrmException { if (submitTypeRecord == null) { - submitTypeRecord = submitRuleEvaluatorFactory.create(this).getSubmitType(); + submitTypeRecord = + submitRuleEvaluatorFactory + .create(userFactory.create(change().getOwner()), this) + .getSubmitType(); } return submitTypeRecord; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java index 7890edd..dbcb879 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
@@ -17,8 +17,8 @@ import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.index.query.Predicate; import com.google.gerrit.index.query.QueryParseException; +import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; import com.google.gerrit.server.git.IntegrationException; @@ -28,20 +28,15 @@ import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; import com.google.gwtorm.server.OrmException; -import com.google.inject.Provider; import java.io.IOException; import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; -import org.eclipse.jgit.revwalk.filter.RevFilter; -import org.eclipse.jgit.treewalk.TreeWalk; -import org.eclipse.jgit.treewalk.filter.TreeFilter; public class ConflictsPredicate { // UI code may depend on this string, so use caution when changing. @@ -51,9 +46,15 @@ public static Predicate<ChangeData> create(Arguments args, String value, Change c) throws QueryParseException, OrmException { - ChangeDataCache changeDataCache = - new ChangeDataCache(c, args.db, args.changeDataFactory, args.projectCache); - List<String> files = listFiles(c, args, changeDataCache); + ChangeData cd; + List<String> files; + try { + cd = args.changeDataFactory.create(args.db.get(), c); + files = cd.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); + } + if (3 + files.size() > args.indexConfig.maxTerms()) { // Short-circuit with a nice error message if we exceed the index // backend's term limit. This assumes that "conflicts:foo" is the entire @@ -73,61 +74,29 @@ and.add(new RefPredicate(c.getDest().get())); and.add(Predicate.not(new LegacyChangeIdPredicate(c.getId()))); and.add(Predicate.or(filePredicates)); + + ChangeDataCache changeDataCache = new ChangeDataCache(cd, args.projectCache); and.add(new CheckConflict(ChangeQueryBuilder.FIELD_CONFLICTS, value, args, c, changeDataCache)); return Predicate.and(and); } - private static List<String> listFiles(Change c, Arguments args, ChangeDataCache changeDataCache) - throws OrmException { - try (Repository repo = args.repoManager.openRepository(c.getProject()); - RevWalk rw = new RevWalk(repo)) { - RevCommit ps = rw.parseCommit(changeDataCache.getTestAgainst()); - if (ps.getParentCount() > 1) { - String dest = c.getDest().get(); - Ref destBranch = repo.getRefDatabase().exactRef(dest); - rw.setRevFilter(RevFilter.MERGE_BASE); - rw.markStart(rw.parseCommit(destBranch.getObjectId())); - rw.markStart(ps); - RevCommit base = rw.next(); - // TODO(zivkov): handle the case with multiple merge bases - - List<String> files = new ArrayList<>(); - try (TreeWalk tw = new TreeWalk(repo)) { - if (base != null) { - tw.setFilter(TreeFilter.ANY_DIFF); - tw.addTree(base.getTree()); - } - tw.addTree(ps.getTree()); - tw.setRecursive(true); - while (tw.next()) { - files.add(tw.getPathString()); - } - } - return files; - } - return args.changeDataFactory.create(args.db.get(), c).currentFilePaths(); - } catch (IOException e) { - throw new OrmException(e); - } - } - private static final class CheckConflict extends ChangeOperatorPredicate { private final Arguments args; - private final Change c; + private final Branch.NameKey dest; private final ChangeDataCache changeDataCache; CheckConflict( String field, String value, Arguments args, Change c, ChangeDataCache changeDataCache) { super(field, value); this.args = args; - this.c = c; + this.dest = c.getDest(); this.changeDataCache = changeDataCache; } @Override public boolean match(ChangeData object) throws OrmException { Change otherChange = object.change(); - if (otherChange == null || !otherChange.getDest().equals(c.getDest())) { + if (otherChange == null || !otherChange.getDest().equals(dest)) { return false; } @@ -136,13 +105,17 @@ return false; } + ProjectState projectState; + try { + projectState = changeDataCache.getProjectState(); + } catch (NoSuchProjectException e) { + return false; + } + ObjectId other = ObjectId.fromString(object.currentPatchSet().getRevision().get()); ConflictKey conflictsKey = new ConflictKey( - changeDataCache.getTestAgainst(), - other, - str.type, - changeDataCache.getProjectState().isUseContentMerge()); + changeDataCache.getTestAgainst(), other, str.type, projectState.isUseContentMerge()); Boolean conflicts = args.conflictsCache.getIfPresent(conflictsKey); if (conflicts != null) { return conflicts; @@ -187,41 +160,31 @@ } } - public static class ChangeDataCache { - protected final Change change; - protected final Provider<ReviewDb> db; - protected final ChangeData.Factory changeDataFactory; - protected final ProjectCache projectCache; + private static class ChangeDataCache { + private final ChangeData cd; + private final ProjectCache projectCache; - protected ObjectId testAgainst; - protected ProjectState projectState; - protected Set<ObjectId> alreadyAccepted; + private ObjectId testAgainst; + private ProjectState projectState; + private Set<ObjectId> alreadyAccepted; - public ChangeDataCache( - Change change, - Provider<ReviewDb> db, - ChangeData.Factory changeDataFactory, - ProjectCache projectCache) { - this.change = change; - this.db = db; - this.changeDataFactory = changeDataFactory; + ChangeDataCache(ChangeData cd, ProjectCache projectCache) { + this.cd = cd; this.projectCache = projectCache; } - protected ObjectId getTestAgainst() throws OrmException { + ObjectId getTestAgainst() throws OrmException { if (testAgainst == null) { - testAgainst = - ObjectId.fromString( - changeDataFactory.create(db.get(), change).currentPatchSet().getRevision().get()); + testAgainst = ObjectId.fromString(cd.currentPatchSet().getRevision().get()); } return testAgainst; } - protected ProjectState getProjectState() { + ProjectState getProjectState() throws NoSuchProjectException { if (projectState == null) { - projectState = projectCache.get(change.getProject()); + projectState = projectCache.get(cd.project()); if (projectState == null) { - throw new IllegalStateException(new NoSuchProjectException(change.getProject())); + throw new NoSuchProjectException(cd.project()); } } return projectState;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java index 56ed797..fc00283 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsPathPredicate.java
@@ -16,6 +16,7 @@ import com.google.gerrit.server.index.change.ChangeField; import com.google.gwtorm.server.OrmException; +import java.io.IOException; import java.util.Collections; import java.util.List; @@ -26,8 +27,13 @@ @Override public boolean match(ChangeData object) throws OrmException { - List<String> files = object.currentFilePaths(); - return files != null && Collections.binarySearch(files, value) >= 0; + List<String> files; + try { + files = object.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); + } + return Collections.binarySearch(files, value) >= 0; } @Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java index 7e7d456..361b57a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
@@ -31,6 +31,7 @@ import com.google.gerrit.server.data.QueryStatsAttribute; import com.google.gerrit.server.events.EventFactory; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gson.Gson; import com.google.gwtorm.server.OrmException; @@ -78,6 +79,7 @@ private final EventFactory eventFactory; private final TrackingFooters trackingFooters; private final CurrentUser user; + private final ChangeControl.GenericFactory changeControlFactory; private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory; private OutputFormat outputFormat = OutputFormat.TEXT; @@ -103,6 +105,7 @@ EventFactory eventFactory, TrackingFooters trackingFooters, CurrentUser user, + ChangeControl.GenericFactory changeControlFactory, SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) { this.db = db; this.repoManager = repoManager; @@ -111,6 +114,7 @@ this.eventFactory = eventFactory; this.trackingFooters = trackingFooters; this.user = user; + this.changeControlFactory = changeControlFactory; this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory; } @@ -250,7 +254,11 @@ if (includeSubmitRecords) { eventFactory.addSubmitRecords( c, - submitRuleEvaluatorFactory.create(d).setAllowClosed(true).setAllowDraft(true).evaluate()); + submitRuleEvaluatorFactory + .create(user, d) + .setAllowClosed(true) + .setAllowDraft(true) + .evaluate()); } if (includeCommitMessage) { @@ -270,12 +278,13 @@ } } + ChangeControl ctl = changeControlFactory.controlFor(db, d.change(), user); if (includePatchSets) { eventFactory.addPatchSets( db, rw, c, - d.changeControl().getVisiblePatchSets(d.patchSets(), db), + ctl.getVisiblePatchSets(d.patchSets(), db), includeApprovals ? d.approvals().asMap() : null, includeFiles, d.change(), @@ -284,7 +293,7 @@ if (includeCurrentPatchSet) { PatchSet current = d.currentPatchSet(); - if (current != null && d.changeControl().forUser(user).isPatchVisible(current, d.db())) { + if (current != null && ctl.isPatchVisible(current, d.db())) { c.currentPatchSet = eventFactory.asPatchSetAttribute(db, rw, d.change(), current); eventFactory.addApprovals(c.currentPatchSet, d.currentApprovals(), labelTypes); @@ -304,7 +313,7 @@ db, rw, c, - d.changeControl().getVisiblePatchSets(d.patchSets(), db), + ctl.getVisiblePatchSets(d.patchSets(), db), includeApprovals ? d.approvals().asMap() : null, includeFiles, d.change(),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java index b288bd5..65de137 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
@@ -15,6 +15,7 @@ package com.google.gerrit.server.query.change; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.common.ChangeInfo; @@ -143,8 +144,8 @@ for (int n = 0; n < cnt; n++) { List<ChangeInfo> info = res.get(n); - if (results.get(n).more()) { - info.get(info.size() - 1)._moreChanges = true; + if (results.get(n).more() && !info.isEmpty()) { + Iterables.getLast(info)._moreChanges = true; } } return res;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java index ca21247..46b4cd5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RegexPathPredicate.java
@@ -17,6 +17,7 @@ import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.util.RegexListSearcher; import com.google.gwtorm.server.OrmException; +import java.io.IOException; import java.util.List; public class RegexPathPredicate extends ChangeRegexPredicate { @@ -26,15 +27,13 @@ @Override public boolean match(ChangeData object) throws OrmException { - List<String> files = object.currentFilePaths(); - if (files != null) { - return RegexListSearcher.ofStrings(getValue()).hasMatch(files); + List<String> files; + try { + files = object.currentFilePaths(); + } catch (IOException e) { + throw new OrmException(e); } - // The ChangeData can't do expensive lookups right now. Bypass - // them and include the result anyway. We might be able to do - // a narrow later on to a smaller set. - // - return true; + return RegexListSearcher.ofStrings(getValue()).hasMatch(files); } @Override
diff --git a/lib/js/bower_archives.bzl b/lib/js/bower_archives.bzl index 3ac2351..e8dcf12 100644 --- a/lib/js/bower_archives.bzl +++ b/lib/js/bower_archives.bzl
@@ -23,6 +23,11 @@ version = "3.5.0", sha1 = "849ad3ee7c77506548b7b5db603a4e150b9431aa") bower_archive( + name = "font-roboto", + package = "PolymerElements/font-roboto", + version = "1.0.3", + sha1 = "edf478d20ae2fc0704d7c155e20162caaabdd5ae") + bower_archive( name = "iron-a11y-announcer", package = "PolymerElements/iron-a11y-announcer", version = "1.0.6", @@ -34,7 +39,7 @@ sha1 = "f58358ee652c67e6e721364ba50fb77a2ece1465") bower_archive( name = "iron-behaviors", - package = "polymerelements/iron-behaviors", + package = "PolymerElements/iron-behaviors", version = "1.0.18", sha1 = "e231a1a02b090f5183db917639fdb96cdd0dca18") bower_archive( @@ -45,8 +50,18 @@ bower_archive( name = "iron-flex-layout", package = "PolymerElements/iron-flex-layout", - version = "2.0.0", - sha1 = "feae42cc5d2d948a50074f430cfb8ab28cb6dc9e") + version = "1.3.7", + sha1 = "4d4cf3232cf750a17a7df0a37476117f831ac633") + bower_archive( + name = "iron-form-element-behavior", + package = "PolymerElements/iron-form-element-behavior", + version = "1.0.7", + sha1 = "7b5a79e02cc32f0918725dd26925d0df1e03ed12") + bower_archive( + name = "iron-menu-behavior", + package = "PolymerElements/iron-menu-behavior", + version = "2.0.1", + sha1 = "139528ee1e8d86257e2aa445de7761b8ec70ae91") bower_archive( name = "iron-meta", package = "PolymerElements/iron-meta", @@ -54,7 +69,7 @@ sha1 = "f77eba3f6f6817f10bda33918bde8f963d450041") bower_archive( name = "iron-resizable-behavior", - package = "PolymerElements/iron-resizable-behavior", + package = "polymerelements/iron-resizable-behavior", version = "1.0.6", sha1 = "719c2a8a1a784f8aefcdeef41fcc2e5a03518d9e") bower_archive( @@ -70,14 +85,19 @@ bower_archive( name = "mocha", package = "mocha", - version = "3.5.0", - sha1 = "09aa92f4f89949ed5f501a57f082e96510b18318") + version = "3.5.3", + sha1 = "c14f149821e4e96241b20f85134aa757b73038f1") bower_archive( name = "neon-animation", package = "polymerelements/neon-animation", version = "1.2.5", sha1 = "588d289f779d02b21ce5b676e257bbd6155649e8") bower_archive( + name = "paper-styles", + package = "PolymerElements/paper-styles", + version = "1.3.1", + sha1 = "4ee9c692366949a754e0e39f8031aa60ce66f24d") + bower_archive( name = "sinon-chai", package = "sinon-chai", version = "2.13.0",
diff --git a/lib/js/bower_components.bzl b/lib/js/bower_components.bzl index e36a759..2f5abd4 100644 --- a/lib/js/bower_components.bzl +++ b/lib/js/bower_components.bzl
@@ -30,6 +30,10 @@ seed = True, ) bower_component( + name = "font-roboto", + license = "//lib:LICENSE-polymer", + ) + bower_component( name = "iron-a11y-announcer", license = "//lib:LICENSE-polymer", deps = [ ":polymer" ], @@ -62,7 +66,6 @@ name = "iron-dropdown", license = "//lib:LICENSE-polymer", deps = [ - ":iron-a11y-keys-behavior", ":iron-behaviors", ":iron-overlay-behavior", ":iron-resizable-behavior", @@ -82,6 +85,11 @@ deps = [ ":polymer" ], ) bower_component( + name = "iron-form-element-behavior", + license = "//lib:LICENSE-polymer", + deps = [ ":polymer" ], + ) + bower_component( name = "iron-input", license = "//lib:LICENSE-polymer", deps = [ @@ -92,6 +100,16 @@ seed = True, ) bower_component( + name = "iron-menu-behavior", + license = "//lib:LICENSE-polymer", + deps = [ + ":iron-a11y-keys-behavior", + ":iron-flex-layout", + ":iron-selector", + ":polymer", + ], + ) + bower_component( name = "iron-meta", license = "//lib:LICENSE-polymer", deps = [ ":polymer" ], @@ -162,6 +180,51 @@ seed = True, ) bower_component( + name = "paper-input", + license = "//lib:LICENSE-polymer", + deps = [ + ":iron-a11y-keys-behavior", + ":iron-autogrow-textarea", + ":iron-behaviors", + ":iron-form-element-behavior", + ":iron-input", + ":paper-styles", + ":polymer", + ], + seed = True, + ) + bower_component( + name = "paper-item", + license = "//lib:LICENSE-polymer", + deps = [ + ":iron-behaviors", + ":iron-flex-layout", + ":paper-styles", + ":polymer", + ], + seed = True, + ) + bower_component( + name = "paper-listbox", + license = "//lib:LICENSE-polymer", + deps = [ + ":iron-behaviors", + ":iron-menu-behavior", + ":paper-styles", + ":polymer", + ], + seed = True, + ) + bower_component( + name = "paper-styles", + license = "//lib:LICENSE-polymer", + deps = [ + ":font-roboto", + ":iron-flex-layout", + ":polymer", + ], + ) + bower_component( name = "polymer-resin", license = "//lib:LICENSE-polymer", deps = [
diff --git a/plugins/hooks b/plugins/hooks index a96c0b9..c52e0e6 160000 --- a/plugins/hooks +++ b/plugins/hooks
@@ -1 +1 @@ -Subproject commit a96c0b937e412a44b00a7574fe0f7c5f010aabf5 +Subproject commit c52e0e6b5de088c820038654312711bba9eceaeb
diff --git a/polygerrit-ui/BUILD b/polygerrit-ui/BUILD index 31ab6aa..cb1966d 100644 --- a/polygerrit-ui/BUILD +++ b/polygerrit-ui/BUILD
@@ -20,6 +20,9 @@ "//lib/js:iron-selector", "//lib/js:moment", "//lib/js:page", + "//lib/js:paper-input", + "//lib/js:paper-item", + "//lib/js:paper-listbox", "//lib/js:polymer", "//lib/js:polymer-resin", "//lib/js:promise-polyfill",
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html index 4ff10cb..e1b88e8 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html +++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
@@ -23,6 +23,7 @@ <link rel="import" href="../../shared/gr-account-link/gr-account-link.html"> <link rel="import" href="../../shared/gr-change-star/gr-change-star.html"> <link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html"> +<link rel="import" href="../../shared/gr-limited-text/gr-limited-text.html"> <link rel="import" href="../../../styles/shared-styles.html"> <dom-module id="gr-change-list-item"> @@ -146,7 +147,10 @@ [[change.branch]] </a> <template is="dom-if" if="[[change.topic]]"> - (<a href$="[[_computeTopicURL(change)]]">[[change.topic]]</a>) + (<a href$="[[_computeTopicURL(change)]]"><!-- + --><gr-limited-text limit="30" text="[[change.topic]]"> + </gr-limited-text><!-- + --></a>) </template> </td> <td class="cell updated"
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html index 978fab2..cbaf605 100644 --- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html +++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
@@ -215,12 +215,14 @@ <template is="dom-if" if="[[change.topic]]"> <gr-linked-chip text="[[change.topic]]" + limit="40" href="[[_computeTopicURL(change.topic)]]" removable="[[!_topicReadOnly]]" on-remove="_handleTopicRemoved"></gr-linked-chip> </template> <template is="dom-if" if="[[!change.topic]]"> <gr-editable-label + label-text="Add a topic" value="[[change.topic]]" placeholder="[[_computeTopicPlaceholder(_topicReadOnly)]]" read-only="[[_topicReadOnly]]" @@ -245,6 +247,7 @@ </gr-linked-chip> </template> <gr-editable-label + label-text="Add a hashtag" value="{{_newHashtag}}" placeholder="[[_computeHashtagPlaceholder(_hashtagReadOnly)]]" read-only="[[_hashtagReadOnly]]" @@ -264,7 +267,7 @@ <span class$="[[label.className]]"> <gr-label has-tooltip - title="[[_computeValueTooltip(label.value, labelName)]]" + title="[[_computeValueTooltip(change, label.value, labelName)]]" class="labelValue"> [[label.value]] </gr-label>
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js index 88b2444..b5b909a 100644 --- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js +++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js
@@ -158,9 +158,11 @@ return result; }, - _computeValueTooltip(score, labelName) { - const values = this.change.labels[labelName].values; - return values[score]; + _computeValueTooltip(change, score, labelName) { + if (!change.labels[labelName] || + !change.labels[labelName].values || + !change.labels[labelName].values[score]) { return ''; } + return change.labels[labelName].values[score]; }, _handleTopicChanged(e, topic) { @@ -344,9 +346,10 @@ _handleHashtagRemoved(e) { e.preventDefault(); - const target = Polymer.dom(e).rootTarget.text; + const target = Polymer.dom(e).rootTarget; target.disabled = true; - this.$.restAPI.setChangeHashtag(this.change._number, {remove: [target]}) + this.$.restAPI.setChangeHashtag(this.change._number, + {remove: [target.text]}) .then(newHashtag => { target.disabled = false; this.set(['change', 'hashtags'], newHashtag);
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html index 2c3330f..634e21c 100644 --- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html +++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html
@@ -276,6 +276,32 @@ element._computeShowUploaderHide(change), 'hideDisplay'); }); + test('_computeValueTooltip', () => { + // Existing label. + const change = {labels: {'Foo-bar': {values: {0: 'Baz'}}}}; + let score = '0'; + let labelName = 'Foo-bar'; + let actual = element._computeValueTooltip(change, score, labelName); + assert.equal(actual, 'Baz'); + + // Non-extsistent label. + labelName = 'xyz'; + actual = element._computeValueTooltip(change, score, labelName); + assert.equal(actual, ''); + + // Non-extsistent score. + score = '2'; + actual = element._computeValueTooltip(change, score, labelName); + assert.equal(actual, ''); + + // No values on label. + labelName = 'abcd'; + score = '0'; + change.labels.abcd = {}; + actual = element._computeValueTooltip(change, score, labelName); + assert.equal(actual, ''); + }); + suite('Topic removal', () => { let change; setup(() => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html index 5c2422d..252ea71 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html +++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
@@ -140,6 +140,7 @@ <gr-editable-label id="descriptionLabel" class="descriptionLabel" + label-text="Add patchset description" value="[[_computePatchSetDescription(change, patchRange.patchNum)]]" placeholder="[[_computeDescriptionPlaceholder(_descriptionReadOnly)]]" read-only="[[_descriptionReadOnly]]"
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html index 2ebf7c7..401aaf8 100644 --- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html +++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html
@@ -15,9 +15,9 @@ --> <link rel="import" href="../../../bower_components/polymer/polymer.html"> -<link rel="import" href="../../../behaviors/base-url-behavior/base-url-behavior.html"> <link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html"> <link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html"> +<link rel="import" href="../../core/gr-navigation/gr-navigation.html"> <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html"> <link rel="import" href="../../../styles/shared-styles.html"> @@ -112,7 +112,7 @@ items="[[_relatedResponse.changes]]" as="related"> <div class$="rightIndent [[_computeChangeContainerClass(change, related)]]"> - <a href$="[[_computeChangeURL(related._change_number, related._revision_number)]]" + <a href$="[[_computeChangeURL(related._change_number, related.project, related._revision_number)]]" class$="[[_computeLinkClass(related)]]" title$="[[related.commit.subject]]"> [[related.commit.subject]] @@ -127,7 +127,7 @@ <h4>Submitted together</h4> <template is="dom-repeat" items="[[_submittedTogether]]" as="change"> <div> - <a href$="[[_computeChangeURL(change._number)]]" + <a href$="[[_computeChangeURL(change._number, change.project)]]" class$="[[_computeLinkClass(change)]]" title$="[[change.project]]: [[change.branch]]: [[change.subject]]"> [[change.project]]: [[change.branch]]: [[change.subject]] @@ -143,7 +143,7 @@ <h4>Same topic</h4> <template is="dom-repeat" items="[[_sameTopic]]" as="change"> <div> - <a href$="[[_computeChangeURL(change._number)]]" + <a href$="[[_computeChangeURL(change._number, change.project)]]" class$="[[_computeLinkClass(change)]]" title$="[[change.project]]: [[change.branch]]: [[change.subject]]"> [[change.project]]: [[change.branch]]: [[change.subject]] @@ -155,7 +155,7 @@ <h4>Merge conflicts</h4> <template is="dom-repeat" items="[[_conflicts]]" as="change"> <div> - <a href$="[[_computeChangeURL(change._number)]]" + <a href$="[[_computeChangeURL(change._number, change.project)]]" class$="[[_computeLinkClass(change)]]" title$="[[change.subject]]"> [[change.subject]] @@ -167,7 +167,7 @@ <h4>Cherry picks</h4> <template is="dom-repeat" items="[[_cherryPicks]]" as="change"> <div> - <a href$="[[_computeChangeURL(change._number)]]" + <a href$="[[_computeChangeURL(change._number, change.project)]]" class$="[[_computeLinkClass(change)]]" title$="[[change.branch]]: [[change.subject]]"> [[change.branch]]: [[change.subject]]
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js index f330581..39c736d 100644 --- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js +++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js
@@ -64,7 +64,6 @@ }, behaviors: [ - Gerrit.BaseUrlBehavior, Gerrit.PatchSetBehavior, Gerrit.RESTClientBehavior, ], @@ -164,12 +163,14 @@ return this.$.restAPI.getChangesWithSameTopic(this.change.topic); }, - _computeChangeURL(changeNum, patchNum) { - let urlStr = this.getBaseUrl() + '/c/' + changeNum; - if (patchNum != null) { - urlStr += '/' + patchNum; - } - return urlStr; + /** + * @param {number} changeNum + * @param {string} project + * @param {number=} opt_patchNum + * @return {string} + */ + _computeChangeURL(changeNum, project, opt_patchNum) { + return Gerrit.Nav.getUrlForChangeById(changeNum, project, opt_patchNum); }, _computeChangeContainerClass(currentChange, relatedChange) {
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.html b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.html index 66d6b02..df4391e 100644 --- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.html
@@ -357,5 +357,11 @@ assert.isFalse(element.hidden); assert.isTrue(updateHandler.called); }); + + test('_computeChangeURL uses Gerrit.Nav', () => { + const getUrlStub = sandbox.stub(Gerrit.Nav, 'getUrlForChangeById'); + element._computeChangeURL(123, 'abc/def', 12); + assert.isTrue(getUrlStub.called); + }); }); </script>
diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html index a056cf1..0ba84f4 100644 --- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html +++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
@@ -189,6 +189,21 @@ }, /** + * @param {number} changeNum + * @param {string} project The name of the project. + * @param {number=} opt_patchNum + * @return {string} + */ + getUrlForChangeById(changeNum, project, opt_patchNum) { + return this._getUrlFor({ + view: Gerrit.Nav.View.CHANGE, + changeNum, + project, + patchNum: opt_patchNum, + }); + }, + + /** * @param {!Object} change The change object. * @param {number=} opt_patchNum * @param {number|string=} opt_basePatchNum The string 'PARENT' can be
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js index 3a724bb..211e328 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@@ -19,7 +19,7 @@ DASHBOARD: '/dashboard/(.*)', ADMIN_PLACEHOLDER: '/admin/(.*)', AGREEMENTS: /^\/settings\/(agreements|new-agreement)/, - REGISTER: /^\/register(\/.*)?/, + REGISTER: /^\/register(\/.*)?$/, // Pattern for login and logout URLs intended to be passed-through. May // include a return URL. @@ -961,7 +961,11 @@ _handleRegisterRoute(ctx) { this._setParams({justRegistered: true}); - const path = ctx.params[0] || '/'; + let path = ctx.params[0] || '/'; + + // Prevent redirect looping. + if (path.startsWith('/register')) { path = '/'; } + if (path[0] !== '/') { return; } this._redirect(this.getBaseUrl() + path); },
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html index c294b22..8c9f3d8 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
@@ -478,6 +478,32 @@ '/c/test/+/42#foo'); }); + suite('_handleRegisterRoute', () => { + test('happy path', () => { + const ctx = {params: ['/foo/bar']}; + element._handleRegisterRoute(ctx); + assert.isTrue(redirectStub.calledWithExactly('/foo/bar')); + assert.isTrue(setParamsStub.calledOnce); + assert.isTrue(setParamsStub.lastCall.args[0].justRegistered); + }); + + test('no param', () => { + const ctx = {params: ['']}; + element._handleRegisterRoute(ctx); + assert.isTrue(redirectStub.calledWithExactly('/')); + assert.isTrue(setParamsStub.calledOnce); + assert.isTrue(setParamsStub.lastCall.args[0].justRegistered); + }); + + test('prevent redirect', () => { + const ctx = {params: ['/register']}; + element._handleRegisterRoute(ctx); + assert.isTrue(redirectStub.calledWithExactly('/')); + assert.isTrue(setParamsStub.calledOnce); + assert.isTrue(setParamsStub.lastCall.args[0].justRegistered); + }); + }); + suite('_handleRootRoute', () => { test('closes for closeAfterLogin', () => { const data = {querystring: 'closeAfterLogin', canonicalPath: ''};
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html index 72f557e..c29d494 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -58,8 +58,10 @@ suite('gr-diff-builder tests', () => { let element; let builder; + let sandbox; setup(() => { + sandbox = sinon.sandbox.create(); stub('gr-rest-api-interface', { getLoggedIn() { return Promise.resolve(false); }, getProjectConfig() { return Promise.resolve({}); }, @@ -74,6 +76,8 @@ {content: []}, {left: [], right: []}, prefs, projectName); }); + teardown(() => { sandbox.restore(); }); + test('context control buttons', () => { const section = {}; const line = {contextGroup: {lines: []}}; @@ -141,11 +145,10 @@ const text = (new Array(52)).join('a'); const line = {text, highlights: []}; - const newLineStub = sinon.stub(builder, '_addNewlines'); + const newLineStub = sandbox.stub(builder, '_addNewlines'); builder._createTextEl(line); flush(() => { assert.isFalse(newLineStub.called); - newLineStub.restore(); done(); }); }); @@ -156,12 +159,11 @@ const text = (new Array(52)).join('a'); const line = {text, highlights: []}; - const newLineStub = sinon.stub(builder, '_addNewlines'); + const newLineStub = sandbox.stub(builder, '_addNewlines'); builder._createTextEl(line); flush(() => { assert.isTrue(newLineStub.called); - newLineStub.restore(); done(); }); }); @@ -358,15 +360,11 @@ setup(() => { el = fixture('div-with-text'); str = el.textContent; - annotateElementSpy = sinon.spy(GrAnnotation, 'annotateElement'); + annotateElementSpy = sandbox.spy(GrAnnotation, 'annotateElement'); layer = document.createElement('gr-diff-builder') ._createIntralineLayer(); }); - teardown(() => { - annotateElementSpy.restore(); - }); - test('annotate no highlights', () => { const line = { text: str, @@ -513,21 +511,15 @@ }); suite('tab indicators', () => { - let sandbox; let element; let layer; setup(() => { - sandbox = sinon.sandbox.create(); element = fixture('basic'); element._showTabs = true; layer = element._createTabIndicatorLayer(); }); - teardown(() => { - sandbox.restore(); - }); - test('does nothing with empty line', () => { const line = {text: ''}; const el = document.createElement('div'); @@ -630,21 +622,15 @@ }); suite('trailing whitespace', () => { - let sandbox; let element; let layer; setup(() => { - sandbox = sinon.sandbox.create(); element = fixture('basic'); element._showTrailingWhitespace = true; layer = element._createTrailingWhitespaceLayer(); }); - teardown(() => { - sandbox.restore(); - }); - test('does nothing with empty line', () => { const line = {text: ''}; const el = document.createElement('div'); @@ -733,10 +719,8 @@ suite('rendering', () => { let content; let outputEl; - let sandbox; setup(done => { - sandbox = sinon.sandbox.create(); const prefs = { line_length: 10, show_tabs: true, @@ -779,10 +763,6 @@ element.render({left: [], right: []}, prefs).then(done); }); - teardown(() => { - sandbox.restore(); - }); - test('reporting', done => { const timeStub = element.$.reporting.time; const timeEndStub = element.$.reporting.timeEnd; @@ -828,7 +808,7 @@ }); test('render-start and render are fired', done => { - const dispatchEventStub = sinon.stub(element, 'dispatchEvent'); + const dispatchEventStub = sandbox.stub(element, 'dispatchEvent'); element.render({left: [], right: []}, {}).then(() => { const firedEventTypes = dispatchEventStub.getCalls() .map(c => { return c.args[0].type; }); @@ -929,7 +909,7 @@ }); test('_renderContentByRange', () => { - const spy = sinon.spy(builder, '_createTextEl'); + const spy = sandbox.spy(builder, '_createTextEl'); const start = 9; const end = 14; const count = end - start + 1; @@ -940,8 +920,6 @@ spy.getCalls().forEach((call, i) => { assert.equal(call.args[0].beforeNumber, start + i); }); - - spy.restore(); }); test('_getNextContentOnSide side-by-side left', () => {
diff --git a/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html b/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html index 1bd345e..1c5e4ae 100644 --- a/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html +++ b/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html
@@ -69,14 +69,14 @@ </section> <section> <div class="title">Preferred Email</div> - <gr-select id="email"> - <select disabled="[[_saving]]"> - <option value="[[_account.email]]">[[_account.email]]</option> - <template is="dom-repeat" items="[[_account.secondary_emails]]"> - <option value="[[item]]">[[item]]</option> - </template> - </select> - </gr-select> + <select + id="email" + disabled="[[_saving]]"> + <option value="[[_account.email]]">[[_account.email]]</option> + <template is="dom-repeat" items="[[_account.secondary_emails]]"> + <option value="[[item]]">[[item]]</option> + </template> + </select> </section> </main> <footer>
diff --git a/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog_test.html b/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog_test.html index bf8995e..858c3ae 100644 --- a/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog_test.html +++ b/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog_test.html
@@ -144,5 +144,13 @@ assert.equal(account.name, 'entered name'); }).then(done); }); + + test('email select properly populated', done => { + element._account = {email: 'foo', secondary_emails: ['bar', 'baz']}; + flush(() => { + assert.equal(element.$.email.value, 'foo'); + done(); + }); + }); }); </script>
diff --git a/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.html b/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.html index ab91fc5..21552d9 100644 --- a/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.html +++ b/polygerrit-ui/app/elements/shared/gr-date-formatter/gr-date-formatter.html
@@ -20,6 +20,7 @@ <link rel="import" href="../../../styles/shared-styles.html"> <script src="../../../bower_components/moment/moment.js"></script> +<script src="../../../scripts/util.js"></script> <dom-module id="gr-date-formatter"> <template>
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.html b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.html new file mode 100644 index 0000000..59f63fa --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.html
@@ -0,0 +1,174 @@ +<!-- +Copyright (C) 2017 The Android Open Source Project + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> +<link rel="import" href="../../../bower_components/polymer/polymer.html"> + +<link rel="import" href="../../../bower_components/iron-dropdown/iron-dropdown.html"> +<link rel="import" href="../../../bower_components/paper-item/paper-item.html"> +<link rel="import" href="../../../bower_components/paper-listbox/paper-listbox.html"> + +<link rel="import" href="../../../styles/shared-styles.html"> +<link rel="import" href="../../shared/gr-button/gr-button.html"> +<link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html"> +<link rel="import" href="../../shared/gr-select/gr-select.html"> + + +<dom-module id="gr-dropdown-list"> + <template> + <style include="shared-styles"> + :host { + display: inline-block; + } + #trigger { + -moz-user-select: text; + -ms-user-select: text; + -webkit-user-select: text; + user-select: text; + } + .downArrow { + display: inline-block; + font-size: .6em; + user-select: none; + vertical-align: middle; + } + .dropdown-trigger { + cursor: pointer; + padding: 0; + } + .dropdown-content { + background-color: #fff; + box-shadow: 0 1px 5px rgba(0, 0, 0, .3); + max-height: 70vh; + margin-top: 1.5em; + width: 266px; + } + paper-listbox { + --paper-listbox: { + padding: 0; + } + } + paper-item { + cursor: pointer; + flex-direction: column; + --paper-item: { + min-height: 0; + padding: 10px 16px; + } + --paper-item-selected: { + background-color: rgba(161,194,250,.12); + } + --paper-item-focused-before: { + background-color: #f2f2f2; + } + --paper-item-focused: { + background-color: #f2f2f2; + } + } + paper-item:not(:last-of-type) { + border-bottom: 1px solid #ddd; + } + gr-button { + color: black; + font: inherit; + padding: .3em 0; + text-decoration: none; + } + .bottomContent { + color: rgba(0,0,0,.54); + font-size: .85em; + line-height: 16px; + } + .bottomContent, + .topContent { + display: flex; + line-height: 16px; + justify-content: space-between; + flex-direction: row; + width: 100%; + } + gr-date-formatter { + color: rgba(0,0,0,.54); + margin-left: 2em; + } + gr-select { + display: none; + } + @media only screen and (max-width: 50em) { + gr-select { + display: inline; + } + gr-button, + iron-dropdown { + display: none; + } + select { + max-width: 5.25em; + } + } + </style> + <gr-button + link + id="trigger" + class="dropdown-trigger" + on-tap="_showDropdownTapHandler"> + <span>[[text]]</span> + <span + class="downArrow" + on-tap="_showDropdownTapHandler">▼</span> + </gr-button> + <iron-dropdown + id="dropdown" + vertical-align="top" + allow-outside-scroll="true"> + <paper-listbox + class="dropdown-content" + slot="dropdown-content" + attr-for-selected="value" + on-tap="_handleDropdownTap" + selected="{{value}}"> + <template is="dom-repeat" items="[[items]]"> + <paper-item + disabled="[[item.disabled]]" + value="[[item.value]]"> + <div class="topContent"> + <div>[[item.text]]</div> + <template is="dom-if" if="[[item.date]]"> + <gr-date-formatter + date-str="[[item.date]]"></gr-date-formatter> + </template> + </div> + <template is="dom-if" if="[[item.bottomText]]"> + <div class="bottomContent"> + <div>[[item.bottomText]]</div> + </div> + </template> + </paper-item> + </template> + </paper-listbox> + </iron-dropdown> + <gr-select bind-value="{{value}}"> + <select> + <template is="dom-repeat" items="[[items]]"> + <option + disabled$="[[item.disabled]]" + value="[[item.value]]"> + [[_computeMobileText(item)]] + </option> + </template> + </select> + </gr-select> + </template> + <script src="gr-dropdown-list.js"></script> +</dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.js b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.js new file mode 100644 index 0000000..27c6ba8 --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.js
@@ -0,0 +1,102 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +(function() { + 'use strict'; + + /** + * fired when the selected value of the dropdown changes + * + * @event {change} + */ + + const Defs = {}; + + /** + * Requred values are text and value. mobileText and triggerText will + * fall back to text if not provided. + * + * If bottomText is not provided, nothing will display on the second + * line. + * + * If date is not provided, nothing will be displayed in its place. + * + * @typedef {{ + * text: string, + * value: (string|number), + * bottomText: (string|undefined), + * triggerText: (string|undefined), + * mobileText: (string|undefined), + * date: (!Date|undefined), + * }} + */ + Defs.item; + + Polymer({ + is: 'gr-dropdown-list', + + properties: { + /** @type {!Array<!Defs.item>} */ + items: Object, + text: String, + value: { + type: String, + notify: true, + }, + }, + + observers: [ + '_handleValueChange(value, items)', + ], + + /** + * Handle a click on the iron-dropdown element. + * @param {!Event} e + */ + _handleDropdownTap(e) { + // async is needed so that that the click event is fired before the + // dropdown closes (This was a bug for touch devices). + this.async(() => { + this.$.dropdown.close(); + }, 1); + }, + + /** + * Handle a click on the button to open the dropdown. + * @param {!Event} e + */ + _showDropdownTapHandler(e) { + this._open(); + }, + + /** + * Open the dropdown. + */ + _open() { + this.$.dropdown.open(); + }, + + _computeMobileText(item) { + return item.mobileText ? item.mobileText : item.text; + }, + + _handleValueChange(value, items) { + if (!value) { return; } + const selectedObj = items.find(item => { + return item.value + '' === value + ''; + }); + this.text = selectedObj.triggerText? selectedObj.triggerText : + selectedObj.text; + }, + }); +})();
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list_test.html b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list_test.html new file mode 100644 index 0000000..d3c6d83 --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list_test.html
@@ -0,0 +1,167 @@ +<!DOCTYPE html> +<!-- +Copyright (C) 2017 The Android Open Source Project + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes"> +<title>gr-dropdown-list</title> + +<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script> +<script src="../../../bower_components/web-component-tester/browser.js"></script> +<link rel="import" href="../../../test/common-test-setup.html"/> +<link rel="import" href="gr-dropdown-list.html"> + +<script>void(0);</script> + +<test-fixture id="basic"> + <template> + <gr-dropdown-list></gr-dropdown-list> + </template> +</test-fixture> + +<script> + suite('gr-dropdown-list tests', () => { + let element; + let sandbox; + + setup(() => { + stub('gr-rest-api-interface', { + getConfig() { return Promise.resolve({}); }, + }); + element = fixture('basic'); + sandbox = sinon.sandbox.create(); + }); + + teardown(() => { + sandbox.restore(); + }); + + test('tap on trigger opens menu', () => { + sandbox.stub(element, '_open', () => { element.$.dropdown.open(); }); + assert.isFalse(element.$.dropdown.opened); + MockInteractions.tap(element.$.trigger); + assert.isTrue(element.$.dropdown.opened); + }); + + test('_computeMobileText', () => { + const item = { + value: 1, + text: 'text', + }; + assert.equal(element._computeMobileText(item), item.text); + item.mobileText = 'mobile text'; + assert.equal(element._computeMobileText(item), item.mobileText); + }); + + test('options are selected and laid out correctly', () => { + element.value = 2; + element.items = [ + { + value: 1, + text: 'Top Text 1', + }, + { + value: 2, + bottomText: 'Bottom Text 2', + triggerText: 'Button Text 2', + text: 'Top Text 2', + mobileText: 'Mobile Text 2', + }, + { + value: 3, + disabled: true, + bottomText: 'Bottom Text 3', + triggerText: 'Button Text 3', + date: '2017-08-18 23:11:42.569000000', + text: 'Top Text 3', + mobileText: 'Mobile Text 3', + }, + ]; + assert.equal(element.$$('paper-listbox').selected, element.value); + assert.equal(element.text, 'Button Text 2'); + flushAsynchronousOperations(); + const items = Polymer.dom(element.root).querySelectorAll('paper-item'); + const mobileItems = Polymer.dom(element.root).querySelectorAll('option'); + assert.equal(items.length, 3); + assert.equal(mobileItems.length, 3); + + // First Item + // The first item should be disabled, has no bottom text, and no date. + assert.isFalse(!!items[0].disabled); + assert.isFalse(mobileItems[0].disabled); + assert.isFalse(items[0].classList.contains('iron-selected')); + assert.isFalse(mobileItems[0].selected); + + assert.isNotOk(Polymer.dom(items[0]).querySelector('gr-date-formatter')); + assert.isNotOk(Polymer.dom(items[0]).querySelector('.bottomContent')); + assert.equal(items[0].value, element.items[0].value); + assert.equal(mobileItems[0].value, element.items[0].value); + assert.equal(Polymer.dom(items[0]).querySelector('.topContent div') + .innerText, element.items[0].text); + + // Since no mobile specific text, it should fall back to text. + assert.equal(mobileItems[0].text, element.items[0].text); + + + // Second Item + // The second item should have top text, bottom text, and no date. + assert.isFalse(!!items[1].disabled); + assert.isFalse(mobileItems[1].disabled); + assert.isTrue(items[1].classList.contains('iron-selected')); + assert.isTrue(mobileItems[1].selected); + + assert.isNotOk(Polymer.dom(items[1]).querySelector('gr-date-formatter')); + assert.isOk(Polymer.dom(items[1]).querySelector('.bottomContent')); + assert.equal(items[1].value, element.items[1].value); + assert.equal(mobileItems[1].value, element.items[1].value); + assert.equal(Polymer.dom(items[1]).querySelector('.topContent div') + .innerText, element.items[1].text); + + // Since there is mobile specific text, it should that. + assert.equal(mobileItems[1].text, element.items[1].mobileText); + + // Since this item is selected, and it has triggerText defined, that + // should be used. + assert.equal(element.text, element.items[1].triggerText); + + // Third item + // The third item should be disabled, and have a date, and bottom content. + assert.isTrue(!!items[2].disabled); + assert.isTrue(mobileItems[2].disabled); + assert.isFalse(items[2].classList.contains('iron-selected')); + assert.isFalse(mobileItems[2].selected); + + assert.isOk(Polymer.dom(items[2]).querySelector('gr-date-formatter')); + assert.isOk(Polymer.dom(items[2]).querySelector('.bottomContent')); + assert.equal(items[2].value, element.items[2].value); + assert.equal(mobileItems[2].value, element.items[2].value); + assert.equal(Polymer.dom(items[2]).querySelector('.topContent div') + .innerText, element.items[2].text); + + // Since there is mobile specific text, it should that. + assert.equal(mobileItems[2].text, element.items[2].mobileText); + + // Select a new item. + MockInteractions.tap(items[0]); + flushAsynchronousOperations(); + assert.equal(element.value, 1); + assert.isTrue(items[0].classList.contains('iron-selected')); + assert.isTrue(mobileItems[0].selected); + + // Since no triggerText, the fallback is used. + assert.equal(element.text, element.items[0].text); + }); + }); +</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html index 08ecf6f..79d69f5 100644 --- a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html +++ b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html
@@ -13,9 +13,11 @@ See the License for the specific language governing permissions and limitations under the License. --> -<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html"> <link rel="import" href="../../../bower_components/polymer/polymer.html"> -<link rel="import" href="../../../bower_components/iron-input/iron-input.html"> + +<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html"> +<link rel="import" href="../../../bower_components/iron-dropdown/iron-dropdown.html"> +<link rel="import" href="../../../bower_components/paper-input/paper-input.html"> <link rel="import" href="../../../styles/shared-styles.html"> <dom-module id="gr-editable-label"> @@ -44,17 +46,55 @@ cursor: pointer; text-decoration: underline; } + #dropdown { + box-shadow: rgba(0, 0, 0, 0.3) 0 1px 3px; + } + .inputContainer { + padding: .8em; + background-color: #fff; + } + .buttons { + display: flex; + padding-top: 1.2em; + justify-content: flex-end; + width: 100%; + } + .buttons gr-button { + margin-left: .5em; + } + paper-input { + --paper-input-container: { + padding: 0; + min-width: 15em; + } + --paper-input-container-input: { + font-size: 1em; + } + } </style> - <input - is="iron-input" - id="input" - hidden$="[[!editing]]" - bind-value="{{_inputText}}"> - <label - hidden$="[[editing]]" - class$="[[_computeLabelClass(readOnly, value, placeholder)]]" - title$="[[_computeLabel(value, placeholder)]]" - on-tap="_open">[[_computeLabel(value, placeholder)]]</label> + <label + class$="[[_computeLabelClass(readOnly, value, placeholder)]]" + title$="[[_computeLabel(value, placeholder)]]" + on-tap="_showDropdown">[[_computeLabel(value, placeholder)]]</label> + <iron-dropdown id="dropdown" + vertical-align="auto" + horizontal-align="auto" + vertical-offset="[[_verticalOffset]]" + allow-outside-scroll="true" + on-iron-overlay-canceled="_cancel"> + <div class="dropdown-content" slot="dropdown-content"> + <div class="inputContainer"> + <paper-input + id="input" + label="[[labelText]]" + value="{{_inputText}}"></paper-input> + <div class="buttons"> + <gr-button id="cancelBtn" on-tap="_cancel">cancel</gr-button> + <gr-button id="saveBtn" on-tap="_save">save</gr-button> + </div> + </div> + </div> + </iron-dropdown> </template> <script src="gr-editable-label.js"></script> </dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.js b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.js index a78e94c..f87a546 100644 --- a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.js +++ b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.js
@@ -14,6 +14,9 @@ (function() { 'use strict'; + const AWAIT_MAX_ITERS = 10; + const AWAIT_STEP = 5; + Polymer({ is: 'gr-editable-label', @@ -24,6 +27,7 @@ */ properties: { + labelText: String, editing: { type: Boolean, value: false, @@ -43,6 +47,14 @@ value: false, }, _inputText: String, + // This is used to push the iron-input element up on the page, so + // the input is placed in approximately the same position as the + // trigger. + _verticalOffset: { + type: Number, + readOnly: true, + value: -30, + }, }, behaviors: [ @@ -69,21 +81,51 @@ return value; }, - _open() { + _showDropdown() { if (this.readOnly || this.editing) { return; } + this._open().then(() => { + this.$.input.$.input.focus(); + if (!this.$.input.value) { return; } + this.$.input.$.input.setSelectionRange(0, this.$.input.value.length); + }); + }, + _open(...args) { + this.$.dropdown.open(); this._inputText = this.value; this.editing = true; - this.async(() => { - this.$.input.focus(); - this.$.input.setSelectionRange(0, this.$.input.value.length); + return new Promise(resolve => { + Polymer.IronOverlayBehaviorImpl.open.apply(this.$.dropdown, args); + this._awaitOpen(resolve); }); }, + /** + * NOTE: (wyatta) Slightly hacky way to listen to the overlay actually + * opening. Eventually replace with a direct way to listen to the overlay. + */ + _awaitOpen(fn) { + let iters = 0; + const step = () => { + this.async(() => { + if (this.style.display !== 'none') { + fn.call(this); + } else if (iters++ < AWAIT_MAX_ITERS) { + step.call(this); + } + }, AWAIT_STEP); + }; + step.call(this); + }, + + _id() { + return this.getAttribute('id') || 'global'; + }, + _save() { if (!this.editing) { return; } - + this.$.dropdown.close(); this.value = this._inputText; this.editing = false; this.fire('changed', this.value); @@ -91,7 +133,7 @@ _cancel() { if (!this.editing) { return; } - + this.$.dropdown.close(); this.editing = false; this._inputText = this.value; }, @@ -104,7 +146,7 @@ _handleEnter(e) { e = this.getKeyboardEvent(e); const target = Polymer.dom(e).rootTarget; - if (target === this.$.input) { + if (target === this.$.input.$.input) { e.preventDefault(); this._save(); } @@ -118,7 +160,7 @@ _handleEsc(e) { e = this.getKeyboardEvent(e); const target = Polymer.dom(e).rootTarget; - if (target === this.$.input) { + if (target === this.$.input.$.input) { e.preventDefault(); this._cancel(); }
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label_test.html b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label_test.html index dc1a5aa..74f1bdd 100644 --- a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label_test.html +++ b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label_test.html
@@ -54,7 +54,7 @@ setup(() => { element = fixture('basic'); - input = element.$$('input'); + input = element.$.input.$.input; label = element.$$('label'); sandbox = sinon.sandbox.create(); }); @@ -64,33 +64,30 @@ }); test('element render', () => { - // The input is hidden and the label is visible: - assert.isNotNull(input.getAttribute('hidden')); - assert.isNull(label.getAttribute('hidden')); - + // The dropdown is closed and the label is visible: + assert.isFalse(element.$.dropdown.opened); assert.isTrue(label.classList.contains('editable')); - assert.equal(label.textContent, 'value text'); MockInteractions.tap(label); Polymer.dom.flush(); - // The input is visible and the label is hidden: - assert.isNull(input.getAttribute('hidden')); - assert.isNotNull(label.getAttribute('hidden')); - + // The dropdown is open (which covers up the label): + assert.isTrue(element.$.dropdown.opened); assert.equal(input.value, 'value text'); }); test('edit value', done => { const editedStub = sandbox.stub(); element.addEventListener('changed', editedStub); + assert.isFalse(element.editing); MockInteractions.tap(label); Polymer.dom.flush(); + assert.isTrue(element.editing); element._inputText = 'new text'; assert.isFalse(editedStub.called); @@ -98,38 +95,111 @@ element.async(() => { assert.isTrue(editedStub.called); assert.equal(input.value, 'new text'); + assert.isFalse(element.editing); done(); }); // Press enter: MockInteractions.keyDownOn(input, 13); }); - }); - suite('gr-editable-label read-only tests', () => { - let element; - let input; - let label; - - setup(() => { - element = fixture('read-only'); - - input = element.$$('input'); - label = element.$$('label'); - }); - - test('disallows edit when read-only', () => { - // The input is hidden and the label is visible: - assert.isNotNull(input.getAttribute('hidden')); - assert.isNull(label.getAttribute('hidden')); + test('save button', done => { + const editedStub = sandbox.stub(); + element.addEventListener('changed', editedStub); + assert.isFalse(element.editing); MockInteractions.tap(label); Polymer.dom.flush(); - // The input is still hidden and the label is still visible: - assert.isNotNull(input.getAttribute('hidden')); - assert.isNull(label.getAttribute('hidden')); + assert.isTrue(element.editing); + element._inputText = 'new text'; + + assert.isFalse(editedStub.called); + + element.async(() => { + assert.isTrue(editedStub.called); + assert.equal(input.value, 'new text'); + assert.isFalse(element.editing); + done(); + }); + + // Press enter: + MockInteractions.tap(element.$.saveBtn, 13); + }); + + + test('edit and then escape key', done => { + const editedStub = sandbox.stub(); + element.addEventListener('changed', editedStub); + assert.isFalse(element.editing); + + MockInteractions.tap(label); + + Polymer.dom.flush(); + + assert.isTrue(element.editing); + element._inputText = 'new text'; + + assert.isFalse(editedStub.called); + + element.async(() => { + assert.isFalse(editedStub.called); + // Text changes sould be discarded. + assert.equal(input.value, 'value text'); + assert.isFalse(element.editing); + done(); + }); + + // Press escape: + MockInteractions.keyDownOn(input, 27); + }); + + test('cancel button', done => { + const editedStub = sandbox.stub(); + element.addEventListener('changed', editedStub); + assert.isFalse(element.editing); + + MockInteractions.tap(label); + + Polymer.dom.flush(); + + assert.isTrue(element.editing); + element._inputText = 'new text'; + + assert.isFalse(editedStub.called); + + element.async(() => { + assert.isFalse(editedStub.called); + // Text changes sould be discarded. + assert.equal(input.value, 'value text'); + assert.isFalse(element.editing); + done(); + }); + + // Press escape: + MockInteractions.tap(element.$.cancelBtn); + }); + }); + + suite('gr-editable-label read-only tests', () => { + let element; + let label; + + setup(() => { + element = fixture('read-only'); + label = element.$$('label'); + }); + + test('disallows edit when read-only', () => { + // The dropdown is closed. + assert.isFalse(element.$.dropdown.opened); + MockInteractions.tap(label); + + Polymer.dom.flush(); + + // The dropdown is still closed. + assert.isFalse(element.$.dropdown.opened); }); test('label is not marked as editable', () => {
diff --git a/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text.html b/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text.html new file mode 100644 index 0000000..8f88b6a --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text.html
@@ -0,0 +1,23 @@ +<!-- +Copyright (C) 2017 The Android Open Source Project + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +<link rel="import" href="../../../bower_components/polymer/polymer.html"> +<link rel="import" href="../../../behaviors/gr-tooltip-behavior/gr-tooltip-behavior.html"> + +<dom-module id="gr-limited-text"> + <template>[[_computeDisplayText(text, limit)]]</template> + <script src="gr-limited-text.js"></script> +</dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text.js b/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text.js new file mode 100644 index 0000000..bd99306 --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text.js
@@ -0,0 +1,72 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +(function() { + 'use strict'; + + /* + * The gr-limited-text element is for displaying text with a maximum length + * (in number of characters) to display. If the length of the text exceeds the + * configured limit, then an ellipsis indicates that the text was truncated + * and a tooltip containing the full text is enabled. + */ + + Polymer({ + is: 'gr-limited-text', + + properties: { + /** The un-truncated text to display. */ + text: String, + + /** The maximum length for the text to display before truncating. */ + limit: { + type: Number, + value: null, + }, + + /** Boolean property used by Gerrit.TooltipBehavior. */ + hasTooltip: { + type: Boolean, + value: false, + }, + }, + + observers: [ + '_updateTitle(text, limit)', + ], + + behaviors: [ + Gerrit.TooltipBehavior, + ], + + /** + * The text or limit have changed. Recompute whether a tooltip needs to be + * enabled. + */ + _updateTitle(text, limit) { + this.hasTooltip = !!limit && text.length > limit; + if (this.hasTooltip) { + this.setAttribute('title', text); + } else { + this.removeAttribute('title'); + } + }, + + _computeDisplayText(text, limit) { + if (!!limit && text.length > limit) { + return text.substr(0, limit - 1) + '…'; + } + return text; + }, + }); +})();
diff --git a/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text_test.html b/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text_test.html new file mode 100644 index 0000000..9e00331 --- /dev/null +++ b/polygerrit-ui/app/elements/shared/gr-limited-text/gr-limited-text_test.html
@@ -0,0 +1,88 @@ +<!DOCTYPE html> +<!-- +Copyright (C) 2017 The Android Open Source Project + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes"> +<title>gr-limited-text</title> + +<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script> +<script src="../../../bower_components/web-component-tester/browser.js"></script> +<link rel="import" href="../../../test/common-test-setup.html"/> + +<link rel="import" href="gr-limited-text.html"> + +<script>void(0);</script> + +<test-fixture id="basic"> + <template> + <gr-limited-text></gr-limited-text> + </template> +</test-fixture> + +<script> + suite('gr-limited-text tests', () => { + let element; + let sandbox; + + setup(() => { + sandbox = sinon.sandbox.create(); + element = fixture('basic'); + }); + + teardown(() => { + sandbox.restore(); + }); + + test('_updateTitle', () => { + const updateSpy = sandbox.spy(element, '_updateTitle'); + element.text = 'abc 123'; + flushAsynchronousOperations(); + assert.isTrue(updateSpy.calledOnce); + assert.isNotOk(element.getAttribute('title')); + assert.isFalse(element.hasTooltip); + + element.limit = 10; + flushAsynchronousOperations(); + assert.isTrue(updateSpy.calledTwice); + assert.isNotOk(element.getAttribute('title')); + assert.isFalse(element.hasTooltip); + + element.limit = 3; + flushAsynchronousOperations(); + assert.isTrue(updateSpy.calledThrice); + assert.equal(element.getAttribute('title'), 'abc 123'); + assert.isTrue(element.hasTooltip); + + element.limit = 100; + flushAsynchronousOperations(); + assert.equal(updateSpy.callCount, 4); + assert.isNotOk(element.getAttribute('title')); + assert.isFalse(element.hasTooltip); + + element.limit = null; + flushAsynchronousOperations(); + assert.equal(updateSpy.callCount, 5); + assert.isNotOk(element.getAttribute('title')); + assert.isFalse(element.hasTooltip); + }); + + test('_computeDisplayText', () => { + assert.equal(element._computeDisplayText('foo bar', 100), 'foo bar'); + assert.equal(element._computeDisplayText('foo bar', 4), 'foo…'); + assert.equal(element._computeDisplayText('foo bar', null), 'foo bar'); + }); + }); +</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.html b/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.html index ef5dab8..be71eb6 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.html +++ b/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.html
@@ -15,7 +15,9 @@ --> <link rel="import" href="../../../bower_components/polymer/polymer.html"> +<link rel="import" href="../../../behaviors/gr-tooltip-behavior/gr-tooltip-behavior.html"> <link rel="import" href="../gr-button/gr-button.html"> +<link rel="import" href="../gr-limited-text/gr-limited-text.html"> <link rel="import" href="../../../styles/shared-styles.html"> <dom-module id="gr-linked-chip"> @@ -61,7 +63,9 @@ } </style> <div class$="container [[_getBackgroundClass(transparentBackground)]]"> - <a href$="[[href]]">[[text]]</a> + <a href$="[[href]]"> + <gr-limited-text limit="[[limit]]" text="[[text]]"></gr-limited-text> + </a> <gr-button id="remove" hidden$="[[!removable]]"
diff --git a/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.js b/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.js index dca1417..bfb8dbb 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.js +++ b/polygerrit-ui/app/elements/shared/gr-linked-chip/gr-linked-chip.js
@@ -33,6 +33,9 @@ type: Boolean, value: false, }, + + /** If provided, sets the maximum length of the content. */ + limit: Number, }, _getBackgroundClass(transparent) {
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html index 2dba4fc..7c8b6ff 100644 --- a/polygerrit-ui/app/test/index.html +++ b/polygerrit-ui/app/test/index.html
@@ -130,12 +130,14 @@ 'shared/gr-cursor-manager/gr-cursor-manager_test.html', 'shared/gr-date-formatter/gr-date-formatter_test.html', 'shared/gr-download-commands/gr-download-commands_test.html', + 'shared/gr-dropdown-list/gr-dropdown-list_test.html', 'shared/gr-editable-content/gr-editable-content_test.html', 'shared/gr-editable-label/gr-editable-label_test.html', 'shared/gr-formatted-text/gr-formatted-text_test.html', 'shared/gr-js-api-interface/gr-change-actions-js-api_test.html', 'shared/gr-js-api-interface/gr-change-reply-js-api_test.html', 'shared/gr-js-api-interface/gr-js-api-interface_test.html', + 'shared/gr-limited-text/gr-limited-text_test.html', 'shared/gr-linked-chip/gr-linked-chip_test.html', 'shared/gr-linked-text/gr-linked-text_test.html', 'shared/gr-list-view/gr-list-view_test.html',
diff --git a/tools/bzl/gwt.bzl b/tools/bzl/gwt.bzl index 803c45b..0cc2988 100644 --- a/tools/bzl/gwt.bzl +++ b/tools/bzl/gwt.bzl
@@ -93,7 +93,7 @@ def gwt_module(gwt_xml=None, resources=[], srcs=[], **kwargs): if gwt_xml: - resources += [gwt_xml] + resources = resources + [gwt_xml] java_library2( srcs = srcs,
diff --git a/tools/bzl/java.bzl b/tools/bzl/java.bzl index b0c3619..5fca724 100644 --- a/tools/bzl/java.bzl +++ b/tools/bzl/java.bzl
@@ -17,8 +17,8 @@ def java_library2(deps=[], exported_deps=[], exports=[], **kwargs): if exported_deps: - deps += exported_deps - exports += exported_deps + deps = deps + exported_deps + exports = exports + exported_deps native.java_library( deps = deps, exports = exports,
diff --git a/tools/js/bower2bazel.py b/tools/js/bower2bazel.py index 9e7895d..407b71a 100755 --- a/tools/js/bower2bazel.py +++ b/tools/js/bower2bazel.py
@@ -35,6 +35,7 @@ package_licenses = { "es6-promise": "es6-promise", "fetch": "fetch", + "font-roboto": "polymer", "iron-a11y-announcer": "polymer", "iron-a11y-keys-behavior": "polymer", "iron-autogrow-textarea": "polymer", @@ -44,6 +45,7 @@ "iron-flex-layout": "polymer", "iron-form-element-behavior": "polymer", "iron-input": "polymer", + "iron-menu-behavior": "polymer", "iron-meta": "polymer", "iron-overlay-behavior": "polymer", "iron-resizable-behavior": "polymer", @@ -52,11 +54,21 @@ "moment": "moment", "neon-animation": "polymer", "page": "page.js", + "paper-input": "polymer", + "paper-item": "polymer", + "paper-listbox": "polymer", + "paper-styles": "polymer", "polymer": "polymer", "polymer-resin": "polymer", "promise-polyfill": "promise-polyfill", "web-animations-js": "Apache2.0", "webcomponentsjs": "polymer", + "paper-material": "polymer", + "paper-styles": "polymer", + "paper-behaviors": "polymer", + "paper-ripple": "polymer", + "iron-checked-element-behavior": "polymer", + "font-roboto": "polymer", }