Merge changes from topic "gr-group-*-to-ts"
* changes:
Convert gr-group-* to typescript
Rename files to preserve history
diff --git a/Documentation/logs.txt b/Documentation/logs.txt
index 43a8e62..6624366 100644
--- a/Documentation/logs.txt
+++ b/Documentation/logs.txt
@@ -8,6 +8,12 @@
at server startup and then daily at 11pm and
link:config-gerrit.html#log.rotate[rotated] every midnight.
+== Time format
+
+For all timestamps the format `[yyyy-MM-dd'T'HH:mm:ss,SSSXXX]` is used.
+This format is both link:https://www.w3.org/TR/NOTE-datetime[ISO 8601] and
+link:https://tools.ietf.org/html/rfc3339[RFC3339] compatible.
+
== Logs
The following logs can be written.
@@ -32,10 +38,7 @@
* `username`: the username used by the client for authentication. "-" for
anonymous requests.
* `[date:time]`: The date and time stamp of the HTTP request.
- The time that the request was received. The format is until Gerrit 3.1
- `[dd/MMM/yyyy:HH:mm:ss.SSS ZZZZ]`. For Gerrit 3.2 or newer
- link:https://www.w3.org/TR/NOTE-datetime[ISO 8601 format] `[yyyy-MM-dd'T'HH:mm:ss,SSSZ]`
- is used for all timestamps.
+ The time that the request was received.
* `request`: The request line from the client is given in double quotes.
** the HTTP method used by the client.
** the resource the client requested.
@@ -63,10 +66,6 @@
Log format:
* `[date time]`: The time that the request was received.
- The format is until Gerrit 3.1 `[yyyy-mm-dd HH:mm:ss.SSS ZZZZ]`.
- For Gerrit 3.2 or newer
- link:https://www.w3.org/TR/NOTE-datetime[ISO 8601 format] `[yyyy-MM-dd'T'HH:mm:ss,SSSZ]`
- is used for all timestamps.
* `sessionid`: hexadecimal session identifier, all requests of the
same connection share the same sessionid. Gerrit does not support multiplexing multiple
sessions on the same connection. Grep the log file using the sessionid as filter to
@@ -135,10 +134,6 @@
Log format:
* `[date time]`: The time that the request was received.
- The format is until Gerrit 3.1 `[yyyy-mm-dd HH:mm:ss.SSS ZZZZ]`.
- For Gerrit 3.2 or newer
- link:https://www.w3.org/TR/NOTE-datetime[ISO 8601 format] `[yyyy-MM-dd'T'HH:mm:ss,SSSZ]`
- is used for all timestamps.
* `[thread name]`: : name of the Java thread executing the request.
* `level`: log level (ERROR, WARN, INFO, DEBUG).
* `logger`: name of the logger.
@@ -153,10 +148,6 @@
Log format:
* `[date time]`: The time that the request was received.
- The format is until Gerrit 3.1 `[yyyy-mm-dd HH:mm:ss.SSS ZZZZ]`.
- For Gerrit 3.2 or newer
- link:https://www.w3.org/TR/NOTE-datetime[ISO 8601 format] `[yyyy-MM-dd'T'HH:mm:ss,SSSZ]`
- is used for all timestamps.
* `level`: log level (ERROR, WARN, INFO, DEBUG).
* `message`: log message.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 4227655..e09d936 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5026,6 +5026,137 @@
}
----
+[[get-ported-comments]]
+=== Get Ported Comments
+--
+'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/ported_comments'
+--
+
+Ports comments of other revisions to the requested revision.
+
+Only comments added on earlier patchsets are ported. That set of comments is filtered even further
+due to some additional rules. Callers of this endpoint shouldn't rely on the exact logic of which
+comments are ported as that logic might change in the future. Instead, callers must be able to
+handle any smaller/larger set of comments returned by this endpoint.
+
+Typically, a comment thread is returned fully or excluded fully. However, draft comments and
+robot comments are ignored and not returned via this endpoint. Hence, it's possible to get ported
+comments from this endpoint which are a reply to a non-ported robot comment. Callers must be
+able to deal with this situation.
+
+The returned comments are organized in a map of file path to link:#comment-info[CommentInfo] entries
+in the same fashion as for the link:#list-comments[List Revision Comments] endpoint.
+The map is filled with the original comment attributes except for these attributes: `path`, `line`,
+and `range` point to the computed position in the target revision. If the exactly correct position
+can't be determined, those fields will be filled with the next best position. That can also mean
+not filling the `line` or `range` attribute anymore and thus converting the comment to a file
+comment (or even moving the comment to a different file or the patchset level). Callers of this
+endpoint must be able to deal with this and not rely on the original comment position.
+
+It's possible that this endpoint returns different link:#comment-info[CommentInfo] entries with
+the same comment UUID. This is not a bug but a feature. If a comment appears on a file which Gerrit
+recognizes as copied between patchsets, the ported version of this comment consists of two ported
+instances having the same UUID but different `file`/`line`/`range` positions. Callers must be able
+to handle this situation.
+
+Repeated calls of this endpoint might produce different results. Internal errors during the
+position computation are mapped to fallback locations for affected comments. Those errors might
+have vanished on later calls, upon which this endpoint returns the actually mapped position. In
+addition, comments can be deleted and draft comments can be published, upon which the set of ported
+comments may change.
+
+.Request
+----
+ GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/revisions/4/ported_comments/ HTTP/1.0
+----
+
+.Response
+----
+ HTTP/1.1 200 OK
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ {
+ "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java": [
+ {
+ "id": "TvcXrmjM",
+ "patch_set": 2,
+ "line": 23,
+ "message": "[nit] trailing whitespace",
+ "updated": "2013-02-26 15:40:43.986000000",
+ "author": {
+ "_account_id": 1000096,
+ "name": "John Doe",
+ "email": "john.doe@example.com"
+ },
+ "unresolved": true
+ },
+ {
+ "id": "TveXwFiA",
+ "patch_set": 2,
+ "line": 23,
+ "in_reply_to": "TvcXrmjM",
+ "message": "Done",
+ "updated": "2013-02-26 15:40:45.328000000",
+ "author": {
+ "_account_id": 1000097,
+ "name": "Jane Roe",
+ "email": "jane.roe@example.com"
+ },
+ "unresolved": true
+ }
+ ]
+ }
+----
+
+[[get-ported-drafts]]
+=== Get Ported Drafts
+--
+'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/ported_drafts'
+--
+
+Ports draft comments of other revisions to the requested revision.
+
+This endpoint behaves similarly to the link:#get-ported-comments[Get Ported Comments] endpoint.
+With this endpoint, only draft comments of the calling user are ported, though. If a draft comment
+is a reply to a published comment, only the ported draft comment is returned.
+
+Depending on the filtering rules, it's possible that this endpoint returns a draft comment which is
+a reply to a comment thread which is not returned by the
+link:#get-ported-comments[Get Ported Comments] endpoint. That's intended behavior. Callers must be
+able to handle this situation. The same holds for drafts which are a reply to a robot comment.
+
+Different than the link:#get-ported-comments[Get Ported Comments] endpoint, the `author` of the
+returned comments is not filled for this endpoint as only comments of the calling user are returned.
+
+.Request
+----
+ GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/revisions/674ac754f91e64a0efb8087e59a176484bd534d1/ported_drafts/ HTTP/1.0
+----
+
+.Response
+----
+ HTTP/1.1 200 OK
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ {
+ "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java": [
+ {
+ "id": "TveXwFiA",
+ "patch_set": 2,
+ "line": 23,
+ "in_reply_to": "TvcXrmjM",
+ "message": "Done",
+ "updated": "2013-02-26 15:40:45.328000000",
+ "unresolved": true
+ }
+ ]
+ }
+----
+
[[apply-fix]]
=== Apply Fix
--
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
index 20f9c49..465c419 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
@@ -173,11 +173,13 @@
short side = commentCreation.side().orElse(CommentSide.PATCHSET_COMMIT).getNumericSide();
Boolean unresolved = commentCreation.unresolved().orElse(null);
String parentUuid = commentCreation.parentUuid().orElse(null);
+ Timestamp createdOn =
+ commentCreation.createdOn().map(Timestamp::from).orElse(context.getWhen());
HumanComment newComment =
commentsUtil.newHumanComment(
context.getNotes(),
context.getUser(),
- context.getWhen(),
+ createdOn,
filePath,
patchsetId,
side,
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java b/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java
index 2f4ddd0..2031bde 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java
@@ -21,6 +21,9 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.Patch;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
import java.util.Optional;
/**
@@ -49,6 +52,8 @@
public abstract Optional<Account.Id> author();
+ public abstract Optional<Instant> createdOn();
+
abstract Comment.Status status();
abstract ThrowingFunction<TestCommentCreation, String> commentCreator();
@@ -175,6 +180,22 @@
public abstract Builder author(Account.Id accountId);
/**
+ * Creation time of the comment. Like {@link #createdOn(Instant)} but with an arbitrary, fixed
+ * time zone (-> deterministic test execution).
+ */
+ public Builder createdOn(LocalDateTime createdOn) {
+ // We don't care about the exact time zone in most tests, just that it's fixed so that tests
+ // are deterministic.
+ return createdOn(createdOn.atZone(ZoneOffset.UTC).toInstant());
+ }
+
+ /**
+ * Creation time of the comment. This may also lie in the past or future. Comments stored in
+ * NoteDb support only second precision.
+ */
+ public abstract Builder createdOn(Instant createdOn);
+
+ /**
* Status of the comment. Hidden in the API surface. Use {@link
* PerPatchsetOperations#newComment()} or {@link PerPatchsetOperations#newDraftComment()}
* depending on which type of comment you want to create.
diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java
index b83eae8..4d5778a 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -21,7 +21,6 @@
import static java.util.stream.Collectors.toList;
import com.google.common.collect.ComparisonChain;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.Nullable;
@@ -51,12 +50,8 @@
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
-import java.util.HashSet;
import java.util.List;
-import java.util.Map;
import java.util.Optional;
-import java.util.Set;
-import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -354,43 +349,6 @@
update.deleteCommentByRewritingHistory(commentKey.uuid, newMessage);
}
- /**
- * Gets all of the {@link HumanComment} in the comment threads that received a reply.
- *
- * @param changeNotes notes of this change.
- * @param newComments set of all the new comments added on the change by the current user.
- * @return set of all comments in the comments thread that received a reply.
- */
- public Set<HumanComment> getAllHumanCommentsInCommentThreads(
- ChangeNotes changeNotes, ImmutableSet<HumanComment> newComments) {
- Map<String, HumanComment> uuidToComment =
- publishedHumanCommentsByChange(changeNotes).stream()
- .collect(Collectors.toMap(c -> c.key.uuid, c -> c));
-
- // Copy the set so that it won't be mutated.
- List<HumanComment> toTraverse = new ArrayList<>(newComments);
- Set<String> seen = new HashSet<>();
- Set<HumanComment> allCommentsInCommentThreads = new HashSet<>();
- while (!toTraverse.isEmpty()) {
- HumanComment current = toTraverse.remove(0);
- allCommentsInCommentThreads.add(current);
-
- if (current.parentUuid != null) {
- HumanComment parent = uuidToComment.get(current.parentUuid);
- if (parent == null) {
- // If we can't find the parent within the human comments, the parent must be a robot
- // comment and can be ignored.
- continue;
- }
- if (!seen.contains(current.parentUuid)) {
- toTraverse.add(parent);
- seen.add(current.parentUuid);
- }
- }
- }
- return allCommentsInCommentThreads;
- }
-
private static List<HumanComment> commentsOnFile(
Collection<HumanComment> allComments, String file) {
List<HumanComment> result = new ArrayList<>(allComments.size());
diff --git a/java/com/google/gerrit/server/change/CommentThread.java b/java/com/google/gerrit/server/change/CommentThread.java
new file mode 100644
index 0000000..7b729d2
--- /dev/null
+++ b/java/com/google/gerrit/server/change/CommentThread.java
@@ -0,0 +1,69 @@
+// Copyright (C) 2020 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.
+
+package com.google.gerrit.server.change;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.entities.Comment;
+import java.util.List;
+
+/**
+ * Representation of a comment thread.
+ *
+ * <p>A comment thread consists of at least one comment.
+ *
+ * @param <T> type of comments in the thread. Can also be {@link Comment} if the thread mixes
+ * comments of different types.
+ */
+@AutoValue
+public abstract class CommentThread<T extends Comment> {
+
+ /** Comments in the thread in exactly the order they appear in the thread. */
+ public abstract ImmutableList<T> comments();
+
+ /** Whether the whole thread is considered as unresolved. */
+ public boolean unresolved() {
+ return Iterables.getLast(comments()).unresolved;
+ }
+
+ public static <T extends Comment> Builder<T> builder() {
+ return new AutoValue_CommentThread.Builder<>();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder<T extends Comment> {
+
+ public abstract Builder<T> comments(List<T> value);
+
+ public Builder<T> addComment(T comment) {
+ commentsBuilder().add(comment);
+ return this;
+ }
+
+ abstract ImmutableList.Builder<T> commentsBuilder();
+
+ abstract ImmutableList<T> comments();
+
+ abstract CommentThread<T> autoBuild();
+
+ public CommentThread<T> build() {
+ Preconditions.checkState(
+ !comments().isEmpty(), "A comment thread must contain at least one comment.");
+ return autoBuild();
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/change/CommentThreads.java b/java/com/google/gerrit/server/change/CommentThreads.java
new file mode 100644
index 0000000..b948737
--- /dev/null
+++ b/java/com/google/gerrit/server/change/CommentThreads.java
@@ -0,0 +1,137 @@
+// Copyright (C) 2020 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.
+
+package com.google.gerrit.server.change;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static java.util.stream.Collectors.groupingBy;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
+import com.google.gerrit.entities.Comment;
+import java.util.Comparator;
+import java.util.Map;
+import java.util.PriorityQueue;
+import java.util.Queue;
+import java.util.function.Function;
+
+/**
+ * Identifier of comment threads.
+ *
+ * <p>Comments are ordered into threads according to their parent relationship indicated via {@link
+ * Comment#parentUuid}. It's possible that two comments refer to the same parent, which especially
+ * happens when two persons reply in parallel. If such branches exist, we merge them into a flat
+ * list taking the comment creation date ({@link Comment#writtenOn} into account (but still
+ * preserving the general parent order). Remaining ties are resolved by using the natural order of
+ * the comment UUID, which is unique.
+ *
+ * @param <T> type of comments in the threads. Can also be {@link Comment} if the threads mix
+ * comments of different types.
+ */
+public class CommentThreads<T extends Comment> {
+
+ private final ImmutableMap<String, T> commentPerUuid;
+ private final Map<String, ImmutableSet<T>> childrenPerParent;
+
+ public CommentThreads(
+ ImmutableMap<String, T> commentPerUuid, Map<String, ImmutableSet<T>> childrenPerParent) {
+ this.commentPerUuid = commentPerUuid;
+ this.childrenPerParent = childrenPerParent;
+ }
+
+ public static <T extends Comment> CommentThreads<T> forComments(Iterable<T> comments) {
+ ImmutableMap<String, T> commentPerUuid =
+ Streams.stream(comments)
+ .distinct()
+ .collect(ImmutableMap.toImmutableMap(comment -> comment.key.uuid, Function.identity()));
+
+ Map<String, ImmutableSet<T>> childrenPerParent =
+ commentPerUuid.values().stream()
+ .filter(comment -> comment.parentUuid != null)
+ .collect(groupingBy(comment -> comment.parentUuid, toImmutableSet()));
+ return new CommentThreads<>(commentPerUuid, childrenPerParent);
+ }
+
+ /**
+ * Returns all comments organized into threads.
+ *
+ * <p>Comments appear only once.
+ */
+ public ImmutableSet<CommentThread<T>> getThreads() {
+ ImmutableSet<T> roots =
+ commentPerUuid.values().stream().filter(this::isRoot).collect(toImmutableSet());
+
+ return buildThreadsOf(roots);
+ }
+
+ /**
+ * Returns only the comment threads to which the specified comments are a reply.
+ *
+ * <p>If the specified child comments are part of the comments originally provided to {@link
+ * CommentThreads#forComments(Iterable)}, they will also appear in the returned comment threads.
+ * They don't need to be part of the originally provided comments, though, but should refer to one
+ * of these comments via their {@link Comment#parentUuid}. Child comments not referring to any
+ * known comments will be ignored.
+ *
+ * @param childComments comments for which the matching threads should be determined
+ * @return threads to which the provided child comments are a reply
+ */
+ public ImmutableSet<CommentThread<T>> getThreadsForChildren(Iterable<? extends T> childComments) {
+ ImmutableSet<T> relevantRoots =
+ Streams.stream(childComments)
+ .map(this::findRoot)
+ .filter(root -> commentPerUuid.containsKey(root.key.uuid))
+ .collect(toImmutableSet());
+ return buildThreadsOf(relevantRoots);
+ }
+
+ private T findRoot(T comment) {
+ T current = comment;
+ while (!isRoot(current)) {
+ current = commentPerUuid.get(current.parentUuid);
+ }
+ return current;
+ }
+
+ private boolean isRoot(T current) {
+ return current.parentUuid == null || !commentPerUuid.containsKey(current.parentUuid);
+ }
+
+ private ImmutableSet<CommentThread<T>> buildThreadsOf(ImmutableSet<T> roots) {
+ return roots.stream()
+ .map(root -> buildCommentThread(root, childrenPerParent))
+ .collect(toImmutableSet());
+ }
+
+ private static <T extends Comment> CommentThread<T> buildCommentThread(
+ T root, Map<String, ImmutableSet<T>> childrenPerParent) {
+ CommentThread.Builder<T> commentThread = CommentThread.builder();
+ // Expand comments gradually from the root. If there is more than one child per level, place the
+ // earlier-created child earlier in the thread. Break ties with the UUID to be deterministic.
+ Queue<T> unvisited =
+ new PriorityQueue<>(
+ Comparator.comparing((T comment) -> comment.writtenOn)
+ .thenComparing(comment -> comment.key.uuid));
+ unvisited.add(root);
+ while (!unvisited.isEmpty()) {
+ T nextComment = unvisited.remove();
+ commentThread.addComment(nextComment);
+ ImmutableSet<T> children =
+ childrenPerParent.getOrDefault(nextComment.key.uuid, ImmutableSet.of());
+ unvisited.addAll(children);
+ }
+ return commentThread.build();
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 7f7df8c..8301576 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -60,6 +60,8 @@
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.CommentThread;
+import com.google.gerrit.server.change.CommentThreads;
import com.google.gerrit.server.change.MergeabilityCache;
import com.google.gerrit.server.change.PureRevert;
import com.google.gerrit.server.config.AllUsersName;
@@ -790,47 +792,15 @@
List<Comment> comments =
Stream.concat(publishedComments().stream(), robotComments().stream()).collect(toList());
- // Build a map of uuid to list of direct descendants.
- Map<String, List<Comment>> forest = new HashMap<>();
- for (Comment comment : comments) {
- List<Comment> siblings = forest.get(comment.parentUuid);
- if (siblings == null) {
- siblings = new ArrayList<>();
- forest.put(comment.parentUuid, siblings);
- }
- siblings.add(comment);
- }
-
- // Find latest comment in each thread and apply to unresolved counter.
- int unresolved = 0;
- if (forest.containsKey(null)) {
- for (Comment root : forest.get(null)) {
- if (getLatestComment(forest, root).unresolved) {
- unresolved++;
- }
- }
- }
- unresolvedCommentCount = unresolved;
+ ImmutableSet<CommentThread<Comment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+ unresolvedCommentCount =
+ (int) commentThreads.stream().filter(CommentThread::unresolved).count();
}
return unresolvedCommentCount;
}
- protected Comment getLatestComment(Map<String, List<Comment>> forest, Comment root) {
- List<Comment> children = forest.get(root.key.uuid);
- if (children == null) {
- return root;
- }
- Comment latest = null;
- for (Comment comment : children) {
- Comment branchLatest = getLatestComment(forest, comment);
- if (latest == null || branchLatest.writtenOn.after(latest.writtenOn)) {
- latest = branchLatest;
- }
- }
- return latest;
- }
-
public void setUnresolvedCommentCount(Integer count) {
this.unresolvedCommentCount = count;
}
diff --git a/java/com/google/gerrit/server/restapi/change/CommentPorter.java b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
index 3effa8c..05241e2 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentPorter.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
@@ -29,6 +29,8 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.CommentsUtil;
+import com.google.gerrit.server.change.CommentThread;
+import com.google.gerrit.server.change.CommentThreads;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.DiffMappings;
import com.google.gerrit.server.patch.GitPositionTransformer;
@@ -43,6 +45,7 @@
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
@@ -102,8 +105,18 @@
private ImmutableList<HumanComment> filterToRelevant(
List<HumanComment> allComments, PatchSet targetPatchset) {
- return allComments.stream()
- .filter(comment -> comment.key.patchSetId < targetPatchset.number())
+ ImmutableList<HumanComment> previousPatchsetsComments =
+ allComments.stream()
+ .filter(comment -> comment.key.patchSetId < targetPatchset.number())
+ .collect(toImmutableList());
+
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(previousPatchsetsComments).getThreads();
+
+ return commentThreads.stream()
+ .filter(CommentThread::unresolved)
+ .map(CommentThread::comments)
+ .flatMap(Collection::stream)
.collect(toImmutableList());
}
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index 1f6574f..c523036 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -17,6 +17,8 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.common.collect.Sets.SetView;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.PatchSet;
@@ -32,6 +34,8 @@
import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.change.AddToAttentionSetOp;
import com.google.gerrit.server.change.AttentionSetUnchangedOp;
+import com.google.gerrit.server.change.CommentThread;
+import com.google.gerrit.server.change.CommentThreads;
import com.google.gerrit.server.change.RemoveFromAttentionSetOp;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -44,6 +48,7 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
@@ -208,19 +213,22 @@
/** Adds all authors of all comment threads that received a reply during this update */
private void addAllAuthorsOfCommentThreads(
BatchUpdate bu, ChangeNotes changeNotes, ImmutableSet<HumanComment> allNewComments) {
- Set<HumanComment> allCommentsInCommentThreads =
- commentsUtil.getAllHumanCommentsInCommentThreads(changeNotes, allNewComments);
- // Copy the set to make it mutable, so that we can delete users that were already added.
- Set<Account.Id> possibleUsersToAdd =
- new HashSet<>(approvalsUtil.getReviewers(changeNotes).all());
+ List<HumanComment> publishedComments = commentsUtil.publishedHumanCommentsByChange(changeNotes);
+ ImmutableSet<CommentThread<HumanComment>> repliedToCommentThreads =
+ CommentThreads.forComments(publishedComments).getThreadsForChildren(allNewComments);
- for (HumanComment comment : allCommentsInCommentThreads) {
- Account.Id author = comment.author.getId();
- if (possibleUsersToAdd.contains(author)) {
- addToAttentionSet(
- bu, changeNotes, author, "Someone else replied on a comment you posted", false);
- possibleUsersToAdd.remove(author);
- }
+ ImmutableSet<Account.Id> repliedToUsers =
+ repliedToCommentThreads.stream()
+ .map(CommentThread::comments)
+ .flatMap(Collection::stream)
+ .map(comment -> comment.author.getId())
+ .collect(toImmutableSet());
+ ImmutableSet<Account.Id> possibleUsersToAdd = approvalsUtil.getReviewers(changeNotes).all();
+ SetView<Account.Id> usersToAdd = Sets.intersection(possibleUsersToAdd, repliedToUsers);
+
+ for (Account.Id user : usersToAdd) {
+ addToAttentionSet(
+ bu, changeNotes, user, "Someone else replied on a comment you posted", false);
}
}
diff --git a/java/com/google/gerrit/util/logging/LogTimestampFormatter.java b/java/com/google/gerrit/util/logging/LogTimestampFormatter.java
index 9637b8b..cf071de 100644
--- a/java/com/google/gerrit/util/logging/LogTimestampFormatter.java
+++ b/java/com/google/gerrit/util/logging/LogTimestampFormatter.java
@@ -24,7 +24,7 @@
/** Formatter for timestamps used in log entries. */
public class LogTimestampFormatter {
- public static final String TIMESTAMP_FORMAT = "yyyy-MM-dd'T'HH:mm:ss.SSSZ";
+ public static final String TIMESTAMP_FORMAT = "yyyy-MM-dd'T'HH:mm:ss.SSSXXX";
private final DateTimeFormatter dateFormatter;
private final ZoneOffset timeOffset;
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
index 702602b..783fa49 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
@@ -27,6 +27,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
+import com.google.gerrit.acceptance.testsuite.change.TestCommentCreation;
import com.google.gerrit.acceptance.testsuite.change.TestPatchset;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
@@ -37,10 +38,11 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
import java.util.Collection;
import java.util.List;
import java.util.Map;
-import org.junit.Ignore;
import org.junit.Test;
public class PortedCommentsIT extends AbstractDaemonTest {
@@ -57,9 +59,9 @@
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- changeOps.change(changeId).patchset(patchset2Id).newComment().create();
- changeOps.change(changeId).patchset(patchset3Id).newComment().create();
+ String comment1Uuid = newComment(patchset1Id).create();
+ newComment(patchset2Id).create();
+ newComment(patchset3Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -75,8 +77,8 @@
PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create();
PatchSet.Id patchset4Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- String comment3Uuid = changeOps.change(changeId).patchset(patchset3Id).newComment().create();
+ String comment1Uuid = newComment(patchset1Id).create();
+ String comment3Uuid = newComment(patchset3Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset4Id));
@@ -92,8 +94,8 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- String comment2Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String comment1Uuid = newComment(patchset1Id).create();
+ String comment2Uuid = newComment(patchset1Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -109,21 +111,9 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String rootCommentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- String child1CommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(rootCommentUuid)
- .create();
- String child2CommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(child1CommentUuid)
- .create();
+ String rootCommentUuid = newComment(patchset1Id).create();
+ String child1CommentUuid = newComment(patchset1Id).parentUuid(rootCommentUuid).create();
+ String child2CommentUuid = newComment(patchset1Id).parentUuid(child1CommentUuid).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -133,17 +123,14 @@
}
@Test
- // TODO(aliceks): Filter out unresolved comment threads.
- @Ignore
- public void onlyUnresolvedCommentsArePorted() throws Exception {
+ public void onlyUnresolvedPublishedCommentsArePorted() throws Exception {
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create();
- String comment2Uuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create();
+ newComment(patchset1Id).resolved().create();
+ String comment2Uuid = newComment(patchset1Id).unresolved().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -151,33 +138,34 @@
}
@Test
- // TODO(aliceks): Filter out unresolved comment threads.
- @Ignore
+ public void onlyUnresolvedDraftCommentsArePorted() throws Exception {
+ Account.Id accountId = accountOps.newAccount().create();
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().create();
+ PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
+ // Add comments.
+ newDraftComment(patchset1Id).author(accountId).resolved().create();
+ String comment2Uuid = newDraftComment(patchset1Id).author(accountId).unresolved().create();
+
+ List<CommentInfo> portedComments =
+ flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
+
+ assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(comment2Uuid);
+ }
+
+ @Test
public void unresolvedStateOfLastCommentInThreadMatters() throws Exception {
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String rootComment1Uuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create();
+ String rootComment1Uuid = newComment(patchset1Id).resolved().create();
String childComment1Uuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(rootComment1Uuid)
- .unresolved()
- .create();
- String rootComment2Uuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create();
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(rootComment2Uuid)
- .resolved()
- .create();
+ newComment(patchset1Id).parentUuid(rootComment1Uuid).unresolved().create();
+ String rootComment2Uuid = newComment(patchset1Id).unresolved().create();
+ newComment(patchset1Id).parentUuid(rootComment2Uuid).resolved().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -192,24 +180,21 @@
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
- // Add comments.
- String rootCommentUuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create();
+ // Add comments. Comments should be more than 1 second apart as NoteDb only supports second
+ // precision.
+ LocalDateTime now = LocalDateTime.now(ZoneOffset.UTC);
+ String rootCommentUuid = newComment(patchset1Id).resolved().createdOn(now).create();
String childComment1Uuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.parentUuid(rootCommentUuid)
.resolved()
+ .createdOn(now.plusSeconds(5))
.create();
String childComment2Uuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.parentUuid(rootCommentUuid)
.unresolved()
+ .createdOn(now.plusSeconds(10))
.create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -220,6 +205,30 @@
}
@Test
+ public void unresolvedStateOfDraftCommentsIsIgnoredForPublishedComments() throws Exception {
+ Account.Id accountId = accountOps.newAccount().create();
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().create();
+ PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
+ // Add comments.
+ String rootComment1Uuid = newComment(patchset1Id).resolved().create();
+ newDraftComment(patchset1Id)
+ .author(accountId)
+ .parentUuid(rootComment1Uuid)
+ .unresolved()
+ .create();
+ String rootComment2Uuid = newComment(patchset1Id).unresolved().create();
+ newDraftComment(patchset1Id).author(accountId).parentUuid(rootComment2Uuid).resolved().create();
+
+ // Draft comments are only visible to their author.
+ requestScopeOps.setApiUser(accountId);
+ List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
+
+ assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(rootComment2Uuid);
+ }
+
+ @Test
public void draftCommentsAreNotPortedViaApiForPublishedComments() throws Exception {
Account.Id accountId = accountOps.newAccount().create();
// Set up change and patchsets.
@@ -227,7 +236,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
- changeOps.change(changeId).patchset(patchset1Id).newDraftComment().author(accountId).create();
+ newDraftComment(patchset1Id).author(accountId).create();
// Draft comments are only visible to their author.
requestScopeOps.setApiUser(accountId);
@@ -244,7 +253,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(accountId).create();
+ newComment(patchset1Id).author(accountId).create();
List<CommentInfo> portedComments =
flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
@@ -260,7 +269,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(accountId).create();
+ newComment(patchset1Id).author(accountId).create();
List<CommentInfo> portedComments =
flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
@@ -277,7 +286,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(otherUserId).create();
+ newComment(patchset1Id).author(otherUserId).create();
List<CommentInfo> portedComments = flatten(getPortedDraftCommentsOfUser(patchset2Id, userId));
@@ -292,10 +301,7 @@
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String rangeCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.message("Range comment")
.fromLine(1)
.charOffset(2)
@@ -304,30 +310,11 @@
.ofFile("myFile")
.create();
String lineCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .message("Line comment")
- .onLine(1)
- .ofFile("myFile")
- .create();
+ newComment(patchset1Id).message("Line comment").onLine(1).ofFile("myFile").create();
String fileCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .message("File comment")
- .onFileLevelOf("myFile")
- .create();
+ newComment(patchset1Id).message("File comment").onFileLevelOf("myFile").create();
String patchsetLevelCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .message("Patchset-level comment")
- .onPatchsetLevel()
- .create();
+ newComment(patchset1Id).message("Patchset-level comment").onPatchsetLevel().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -344,8 +331,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String commentUuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().onParentCommit().create();
+ String commentUuid = newComment(patchset1Id).onParentCommit().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -359,7 +345,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -373,7 +359,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -388,13 +374,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newDraftComment()
- .author(authorId)
- .create();
+ String commentUuid = newDraftComment(patchset1Id).author(authorId).create();
Map<String, List<CommentInfo>> portedComments =
getPortedDraftCommentsOfUser(patchset2Id, authorId);
@@ -413,8 +393,7 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create();
+ String commentUuid = newComment(patchset1.patchsetId()).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -428,13 +407,7 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1.patchsetId())
- .newComment()
- .message("My comment text")
- .create();
+ String commentUuid = newComment(patchset1.patchsetId()).message("My comment text").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -448,15 +421,9 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String rootCommentUuid =
- changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create();
+ String rootCommentUuid = newComment(patchset1.patchsetId()).create();
String childCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1.patchsetId())
- .newComment()
- .parentUuid(rootCommentUuid)
- .create();
+ newComment(patchset1.patchsetId()).parentUuid(rootCommentUuid).create();
CommentInfo portedComment = getPortedComment(patchset2Id, childCommentUuid);
@@ -471,8 +438,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(authorId).create();
+ String commentUuid = newComment(patchset1Id).author(authorId).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -487,13 +453,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newDraftComment()
- .author(authorId)
- .create();
+ String commentUuid = newDraftComment(patchset1Id).author(authorId).create();
Map<String, List<CommentInfo>> portedComments =
getPortedDraftCommentsOfUser(patchset2Id, authorId);
@@ -510,13 +470,7 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1.patchsetId())
- .newComment()
- .tag("My comment tag")
- .create();
+ String commentUuid = newComment(patchset1.patchsetId()).tag("My comment tag").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -530,7 +484,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -544,7 +498,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -561,13 +515,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -591,10 +539,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -625,10 +570,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -654,10 +596,7 @@
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -692,10 +631,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -735,10 +671,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -778,10 +711,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -810,10 +740,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(3)
@@ -844,10 +771,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(2)
@@ -878,10 +802,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -912,10 +833,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -944,10 +862,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(3)
@@ -977,10 +892,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1009,10 +921,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -1049,10 +958,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1076,10 +982,7 @@
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -1108,14 +1011,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1136,14 +1032,7 @@
.content("Line 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1159,14 +1048,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1190,14 +1072,7 @@
PatchSet.Id patchset3Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset3Id);
@@ -1227,14 +1102,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1261,14 +1129,7 @@
.content("Line 1\nLine two\nLine three\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(2)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1291,14 +1152,7 @@
.content("Line 1\nLine 2\nLine three\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(2)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1319,14 +1173,7 @@
.content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1347,14 +1194,7 @@
.content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(4)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(4).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1375,14 +1215,7 @@
.content("Line 1\nLine 2\nLine three\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(4)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(4).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1398,14 +1231,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
@@ -1428,13 +1254,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1450,13 +1270,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1481,7 +1295,7 @@
.content("Line 1\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().onFileLevelOf("myFile").create();
+ newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1499,13 +1313,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
@@ -1528,7 +1336,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create();
+ newComment(patchset1Id).onPatchsetLevel().create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1544,7 +1352,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create();
+ newComment(patchset1Id).onPatchsetLevel().create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1565,10 +1373,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
// The /COMMIT_MSG file has a header of 6 lines, so the summary line is in line 7.
// Place comment on 'Text 2' which is line 10.
.onLine(10)
@@ -1605,14 +1410,7 @@
changeOps.change(childChangeId).newPatchset().parent().patchset(parentPatchset2Id).create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onParentCommit()
- .onLine(1)
- .ofFile("myFile")
- .create();
+ newComment(childPatchset1Id).onParentCommit().onLine(1).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1654,14 +1452,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onParentCommit()
- .onLine(1)
- .ofFile("file1")
- .create();
+ newComment(childPatchset1Id).onParentCommit().onLine(1).ofFile("file1").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1703,14 +1494,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onSecondParentCommit()
- .onLine(1)
- .ofFile("file2")
- .create();
+ newComment(childPatchset1Id).onSecondParentCommit().onLine(1).ofFile("file2").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1758,14 +1542,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onAutoMergeCommit()
- .onLine(1)
- .ofFile("file1")
- .create();
+ newComment(childPatchset1Id).onAutoMergeCommit().onLine(1).ofFile("file1").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1776,6 +1553,22 @@
assertThat(portedComment).line().isGreaterThan(2);
}
+ private TestCommentCreation.Builder newComment(PatchSet.Id patchsetId) {
+ // Create unresolved comments by default as only those are ported. Tests get override the
+ // unresolved state by explicitly setting it.
+ return changeOps.change(patchsetId.changeId()).patchset(patchsetId).newComment().unresolved();
+ }
+
+ private TestCommentCreation.Builder newDraftComment(PatchSet.Id patchsetId) {
+ // Create unresolved comments by default as only those are ported. Tests get override the
+ // unresolved state by explicitly setting it.
+ return changeOps
+ .change(patchsetId.changeId())
+ .patchset(patchsetId)
+ .newDraftComment()
+ .unresolved();
+ }
+
private CommentInfo getPortedComment(PatchSet.Id patchsetId, String commentUuid)
throws RestApiException {
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchsetId);
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index b156d1b..ed4c33a 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -22,14 +22,19 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseClockStep;
+import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
@@ -39,12 +44,17 @@
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.TestCommentHelper;
+import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import java.time.Duration;
import java.time.Instant;
+import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;
import java.util.function.LongSupplier;
@@ -56,9 +66,13 @@
@UseClockStep(clockStepUnit = TimeUnit.MINUTES)
public class AttentionSetIT extends AbstractDaemonTest {
+ @Inject private ChangeOperations changeOperations;
+ @Inject private AccountOperations accountOperations;
@Inject private RequestScopeOperations requestScopeOperations;
+
@Inject private FakeEmailSender email;
@Inject private TestCommentHelper testCommentHelper;
+ @Inject private Provider<InternalChangeQuery> changeQueryProvider;
/** Simulates a fake clock. Uses second granularity. */
private static class FakeClock implements LongSupplier {
@@ -165,7 +179,8 @@
assertThat(emailBody)
.contains(
user.fullName()
- + " removed themselves from the attention set of this change.\n The reason is: removed.");
+ + " removed themselves from the attention set of this change.\n"
+ + " The reason is: removed.");
}
@Test
@@ -611,7 +626,8 @@
assertThat(exception.getMessage())
.isEqualTo(
- "user can not be added/removed twice, and can not be added and removed at the same time");
+ "user can not be added/removed twice, and can not be added and removed at the same"
+ + " time");
}
@Test
@@ -627,7 +643,8 @@
assertThat(exception.getMessage())
.isEqualTo(
- "user can not be added/removed twice, and can not be added and removed at the same time");
+ "user can not be added/removed twice, and can not be added and removed at the same"
+ + " time");
}
@Test
@@ -663,7 +680,8 @@
assertThat(exception.getMessage())
.isEqualTo(
- "user can not be added/removed twice, and can not be added and removed at the same time");
+ "user can not be added/removed twice, and can not be added and removed at the same"
+ + " time");
}
@Test
@@ -958,6 +976,64 @@
}
@Test
+ public void reviewAddsAllUsersInCommentThreadEvenOfDifferentChildBranch() throws Exception {
+ Account.Id changeOwner = accountOperations.newAccount().create();
+ Change.Id changeId = changeOperations.newChange().owner(changeOwner).create();
+ Account.Id user1 = accountOperations.newAccount().create();
+ Account.Id user2 = accountOperations.newAccount().create();
+ Account.Id user3 = accountOperations.newAccount().create();
+ Account.Id user4 = accountOperations.newAccount().create();
+ // Add users as reviewers.
+ gApi.changes().id(changeId.get()).addReviewer(user1.toString());
+ gApi.changes().id(changeId.get()).addReviewer(user2.toString());
+ gApi.changes().id(changeId.get()).addReviewer(user3.toString());
+ gApi.changes().id(changeId.get()).addReviewer(user4.toString());
+ // Add a comment thread with branches. Such threads occur if people reply in parallel without
+ // having seen/loaded the reply of another person.
+ String root =
+ changeOperations.change(changeId).currentPatchset().newComment().author(user1).create();
+ String sibling1 =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newComment()
+ .author(user2)
+ .parentUuid(root)
+ .create();
+ String sibling2 =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newComment()
+ .author(user3)
+ .parentUuid(root)
+ .create();
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newComment()
+ .author(user4)
+ .parentUuid(sibling2)
+ .create();
+ // Clear the attention set. Necessary as we used Gerrit APIs above which affect the attention
+ // set.
+ AttentionSetInput clearAttention = new AttentionSetInput("clear attention set");
+ gApi.changes().id(changeId.get()).attention(user1.toString()).remove(clearAttention);
+ gApi.changes().id(changeId.get()).attention(user2.toString()).remove(clearAttention);
+ gApi.changes().id(changeId.get()).attention(user3.toString()).remove(clearAttention);
+ gApi.changes().id(changeId.get()).attention(user4.toString()).remove(clearAttention);
+
+ requestScopeOperations.setApiUser(changeOwner);
+ // Simulate that this reply is a child of sibling1 and thus parallel to sibling2 and its child.
+ gApi.changes().id(changeId.get()).current().review(reviewInReplyToComment(sibling1));
+
+ List<AttentionSetUpdate> attentionSetUpdates = getAttentionSetUpdates(changeId);
+ assertThat(attentionSetUpdates)
+ .comparingElementsUsing(hasAccount())
+ .containsExactly(user1, user2, user3, user4);
+ }
+
+ @Test
public void reviewAddsAllUsersInCommentThreadWhenPostedAsDraft() throws Exception {
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
@@ -1281,11 +1357,20 @@
private List<AttentionSetUpdate> getAttentionSetUpdatesForUser(
PushOneCommit.Result r, TestAccount account) {
- return r.getChange().attentionSet().stream()
- .filter(a -> a.account().get() == account.id().get())
+ return getAttentionSetUpdates(r.getChange().getId()).stream()
+ .filter(a -> a.account().equals(account.id()))
.collect(Collectors.toList());
}
+ private List<AttentionSetUpdate> getAttentionSetUpdates(Change.Id changeId) {
+ List<ChangeData> changeData = changeQueryProvider.get().byLegacyChangeId(changeId);
+ if (changeData.size() != 1) {
+ throw new IllegalStateException(
+ String.format("Not exactly one change found for ID %s.", changeId));
+ }
+ return new ArrayList<>(Iterables.getOnlyElement(changeData).attentionSet());
+ }
+
private ReviewInput reviewWithComment() {
return reviewInReplyToComment(null);
}
@@ -1301,4 +1386,8 @@
reviewInput.comments = ImmutableMap.of(Patch.COMMIT_MSG, ImmutableList.of(comment));
return reviewInput;
}
+
+ private Correspondence<AttentionSetUpdate, Account.Id> hasAccount() {
+ return NullAwareCorrespondence.transforming(AttentionSetUpdate::account, "hasAccount");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
index fa28396..c29cf99 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
@@ -14,6 +14,7 @@
package com.google.gerrit.acceptance.testsuite.change;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.assertThat;
import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.assertThatList;
import static com.google.gerrit.extensions.common.testing.RobotCommentInfoSubject.assertThat;
@@ -33,6 +34,12 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.time.Duration;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.Month;
+import java.time.ZoneOffset;
import java.util.List;
import org.junit.Test;
@@ -315,6 +322,59 @@
}
@Test
+ public void commentIsCreatedWithSpecifiedCreationTime() throws Exception {
+ Change.Id changeId = changeOperations.newChange().create();
+
+ // Don't use nanos. NoteDb supports only second precision.
+ Instant creationTime =
+ LocalDateTime.of(2020, Month.SEPTEMBER, 15, 12, 10, 43).atZone(ZoneOffset.UTC).toInstant();
+ String commentUuid =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newComment()
+ .createdOn(creationTime)
+ .create();
+
+ Timestamp creationTimestamp = Timestamp.from(creationTime);
+ CommentInfo comment = getCommentFromServer(changeId, commentUuid);
+ assertThat(comment).updated().isEqualTo(creationTimestamp);
+ }
+
+ @Test
+ public void zoneOfCreationDateCanBeOmitted() throws Exception {
+ Change.Id changeId = changeOperations.newChange().create();
+
+ // As we don't care about the exact time zone internally used as a default, do a relative test
+ // so that we don't need to assert on exact instants in time. For a relative test, we need two
+ // comments whose creation date should be exactly the specified amount apart.
+ // Don't use nanos or millis. NoteDb supports only second precision.
+ LocalDateTime creationTime1 = LocalDateTime.of(2020, Month.SEPTEMBER, 15, 12, 10, 43);
+ LocalDateTime creationTime2 = creationTime1.plusMinutes(10);
+ String commentUuid1 =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newComment()
+ .createdOn(creationTime1)
+ .create();
+ String commentUuid2 =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newComment()
+ .createdOn(creationTime2)
+ .create();
+
+ CommentInfo comment1 = getCommentFromServer(changeId, commentUuid1);
+ Instant comment1Creation = comment1.updated.toInstant();
+ CommentInfo comment2 = getCommentFromServer(changeId, commentUuid2);
+ Instant comment2Creation = comment2.updated.toInstant();
+ Duration commentCreationDifference = Duration.between(comment1Creation, comment2Creation);
+ assertThat(commentCreationDifference).isEqualTo(Duration.ofMinutes(10));
+ }
+
+ @Test
public void draftCommentCanBeCreatedWithoutSpecifyingAnyParameters() throws Exception {
Change.Id changeId = changeOperations.newChange().create();
@@ -626,6 +686,59 @@
}
@Test
+ public void draftCommentIsCreatedWithSpecifiedCreationTime() throws Exception {
+ Change.Id changeId = changeOperations.newChange().create();
+
+ // Don't use nanos. NoteDb supports only second precision.
+ Instant creationTime =
+ LocalDateTime.of(2020, Month.SEPTEMBER, 15, 12, 10, 43).atZone(ZoneOffset.UTC).toInstant();
+ String commentUuid =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newDraftComment()
+ .createdOn(creationTime)
+ .create();
+
+ Timestamp creationTimestamp = Timestamp.from(creationTime);
+ CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+ assertThat(comment).updated().isEqualTo(creationTimestamp);
+ }
+
+ @Test
+ public void zoneOfCreationDateOfDraftCommentCanBeOmitted() throws Exception {
+ Change.Id changeId = changeOperations.newChange().create();
+
+ // As we don't care about the exact time zone internally used as a default, do a relative test
+ // so that we don't need to assert on exact instants in time. For a relative test, we need two
+ // comments whose creation date should be exactly the specified amount apart.
+ // Don't use nanos or millis. NoteDb supports only second precision.
+ LocalDateTime creationTime1 = LocalDateTime.of(2020, Month.SEPTEMBER, 15, 12, 10, 43);
+ LocalDateTime creationTime2 = creationTime1.plusMinutes(10);
+ String commentUuid1 =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newDraftComment()
+ .createdOn(creationTime1)
+ .create();
+ String commentUuid2 =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newDraftComment()
+ .createdOn(creationTime2)
+ .create();
+
+ CommentInfo comment1 = getDraftCommentFromServer(changeId, commentUuid1);
+ Instant comment1Creation = comment1.updated.toInstant();
+ CommentInfo comment2 = getDraftCommentFromServer(changeId, commentUuid2);
+ Instant comment2Creation = comment2.updated.toInstant();
+ Duration commentCreationDifference = Duration.between(comment1Creation, comment2Creation);
+ assertThat(commentCreationDifference).isEqualTo(Duration.ofMinutes(10));
+ }
+
+ @Test
public void noDraftCommentsAreCreatedOnCreationOfPublishedComment() throws Exception {
Change.Id changeId = changeOperations.newChange().create();
diff --git a/javatests/com/google/gerrit/server/change/CommentThreadTest.java b/javatests/com/google/gerrit/server/change/CommentThreadTest.java
new file mode 100644
index 0000000..dc46e48
--- /dev/null
+++ b/javatests/com/google/gerrit/server/change/CommentThreadTest.java
@@ -0,0 +1,80 @@
+// Copyright (C) 2020 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.
+
+package com.google.gerrit.server.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Comment;
+import com.google.gerrit.entities.Comment.Key;
+import com.google.gerrit.entities.HumanComment;
+import java.sql.Timestamp;
+import org.junit.Test;
+
+public class CommentThreadTest {
+
+ @Test
+ public void threadMustContainAtLeastOneComment() {
+ assertThrows(IllegalStateException.class, () -> CommentThread.builder().build());
+ }
+
+ @Test
+ public void threadCanBeUnresolved() {
+ HumanComment root = unresolved(createComment("root"));
+ CommentThread<Comment> commentThread = CommentThread.builder().addComment(root).build();
+
+ assertThat(commentThread.unresolved()).isTrue();
+ }
+
+ @Test
+ public void threadCanBeResolved() {
+ HumanComment root = resolved(createComment("root"));
+ CommentThread<Comment> commentThread = CommentThread.builder().addComment(root).build();
+
+ assertThat(commentThread.unresolved()).isFalse();
+ }
+
+ @Test
+ public void lastCommentInThreadDeterminesUnresolvedStatus() {
+ HumanComment root = resolved(createComment("root"));
+ HumanComment child = unresolved(createComment("child"));
+ CommentThread<Comment> commentThread =
+ CommentThread.builder().addComment(root).addComment(child).build();
+
+ assertThat(commentThread.unresolved()).isTrue();
+ }
+
+ private static HumanComment createComment(String commentUuid) {
+ return new HumanComment(
+ new Key(commentUuid, "myFile", 1),
+ Account.id(100),
+ new Timestamp(1234),
+ (short) 1,
+ "Comment text",
+ "serverId",
+ true);
+ }
+
+ private static HumanComment resolved(HumanComment comment) {
+ comment.unresolved = false;
+ return comment;
+ }
+
+ private static HumanComment unresolved(HumanComment comment) {
+ comment.unresolved = true;
+ return comment;
+ }
+}
diff --git a/javatests/com/google/gerrit/server/change/CommentThreadsTest.java b/javatests/com/google/gerrit/server/change/CommentThreadsTest.java
new file mode 100644
index 0000000..56566d3
--- /dev/null
+++ b/javatests/com/google/gerrit/server/change/CommentThreadsTest.java
@@ -0,0 +1,285 @@
+// Copyright (C) 2020 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.
+
+package com.google.gerrit.server.change;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Comment.Key;
+import com.google.gerrit.entities.HumanComment;
+import java.sql.Timestamp;
+import org.junit.Test;
+
+public class CommentThreadsTest {
+
+ @Test
+ public void threadsAreEmptyWhenNoCommentsAreProvided() {
+ ImmutableList<HumanComment> comments = ImmutableList.of();
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads = ImmutableSet.of();
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsCanBeCreatedFromSingleRoot() {
+ HumanComment root = createComment("root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads = ImmutableSet.of(toThread(root));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsCanBeCreatedFromUnorderedComments() {
+ HumanComment root = createComment("root");
+ HumanComment child1 = asReply(createComment("child1"), "root");
+ HumanComment child2 = asReply(createComment("child2"), "child1");
+ HumanComment child3 = asReply(createComment("child3"), "child2");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(child2, child1, root, child3);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1, child2, child3));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void childWithNotAvailableParentIsAssumedToBeRoot() {
+ HumanComment child1 = asReply(createComment("child1"), "root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads = ImmutableSet.of(toThread(child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsIgnoreDuplicateRoots() {
+ HumanComment root = createComment("root");
+ HumanComment child1 = asReply(createComment("child1"), "root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root, root, child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsIgnoreDuplicateChildren() {
+ HumanComment root = createComment("root");
+ HumanComment child1 = asReply(createComment("child1"), "root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root, child1, child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void commentsAreOrderedIntoCorrectThreads() {
+ HumanComment thread1Root = createComment("thread1Root");
+ HumanComment thread1Child1 = asReply(createComment("thread1Child1"), "thread1Root");
+ HumanComment thread1Child2 = asReply(createComment("thread1Child2"), "thread1Child1");
+ HumanComment thread2Root = createComment("thread2Root");
+ HumanComment thread2Child1 = asReply(createComment("thread2Child1"), "thread2Root");
+
+ ImmutableList<HumanComment> comments =
+ ImmutableList.of(thread2Root, thread1Child2, thread1Child1, thread1Root, thread2Child1);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(
+ toThread(thread1Root, thread1Child1, thread1Child2),
+ toThread(thread2Root, thread2Child1));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void branchedThreadsAreFlattenedAccordingToDate() {
+ HumanComment root = writtenOn(createComment("root"), new Timestamp(1));
+ HumanComment sibling1 = writtenOn(asReply(createComment("sibling1"), "root"), new Timestamp(2));
+ HumanComment sibling2 = writtenOn(asReply(createComment("sibling2"), "root"), new Timestamp(3));
+ HumanComment sibling1Child =
+ writtenOn(asReply(createComment("sibling1Child"), "sibling1"), new Timestamp(4));
+ HumanComment sibling2Child =
+ writtenOn(asReply(createComment("sibling2Child"), "sibling2"), new Timestamp(5));
+
+ ImmutableList<HumanComment> comments =
+ ImmutableList.of(sibling2, sibling2Child, sibling1, sibling1Child, root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, sibling1, sibling2, sibling1Child, sibling2Child));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsConsiderParentRelationshipStrongerThanDate() {
+ HumanComment root = writtenOn(createComment("root"), new Timestamp(3));
+ HumanComment child1 = writtenOn(asReply(createComment("child1"), "root"), new Timestamp(2));
+ HumanComment child2 = writtenOn(asReply(createComment("child2"), "child1"), new Timestamp(1));
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(child2, child1, root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child1, child2));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void threadsFallBackToUuidOrderIfParentAndDateAreTheSame() {
+ HumanComment root = writtenOn(createComment("root"), new Timestamp(1));
+ HumanComment sibling1 = writtenOn(asReply(createComment("sibling1"), "root"), new Timestamp(2));
+ HumanComment sibling2 = writtenOn(asReply(createComment("sibling2"), "root"), new Timestamp(2));
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(sibling2, sibling1, root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreads();
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, sibling1, sibling2));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void specificThreadsCanBeRequestedByTheirReply() {
+ HumanComment thread1Root = createComment("thread1Root");
+ HumanComment thread2Root = createComment("thread2Root");
+
+ HumanComment thread1Reply = asReply(createComment("thread1Reply"), "thread1Root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(thread1Root, thread2Root, thread1Reply);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreadsForChildren(ImmutableList.of(thread1Reply));
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(thread1Root, thread1Reply));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void requestedThreadsDoNotNeedToContainReply() {
+ HumanComment thread1Root = createComment("thread1Root");
+ HumanComment thread2Root = createComment("thread2Root");
+
+ HumanComment thread1Reply = asReply(createComment("thread1Reply"), "thread1Root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(thread1Root, thread2Root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreadsForChildren(ImmutableList.of(thread1Reply));
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(thread1Root));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void completeThreadCanBeRequestedByReplyToRootComment() {
+ HumanComment root = createComment("root");
+ HumanComment child = asReply(createComment("child"), "root");
+
+ HumanComment reply = asReply(createComment("reply"), "root");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root, child);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreadsForChildren(ImmutableList.of(reply));
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, child));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void completeThreadWithBranchesCanBeRequestedByReplyToIntermediateComment() {
+ HumanComment root = writtenOn(createComment("root"), new Timestamp(1));
+ HumanComment sibling1 = writtenOn(asReply(createComment("sibling1"), "root"), new Timestamp(2));
+ HumanComment sibling2 = writtenOn(asReply(createComment("sibling2"), "root"), new Timestamp(3));
+ HumanComment sibling1Child =
+ writtenOn(asReply(createComment("sibling1Child"), "sibling1"), new Timestamp(4));
+ HumanComment sibling2Child =
+ writtenOn(asReply(createComment("sibling2Child"), "sibling2"), new Timestamp(5));
+
+ HumanComment reply = asReply(createComment("sibling1"), "root");
+
+ ImmutableList<HumanComment> comments =
+ ImmutableList.of(root, sibling1, sibling2, sibling1Child, sibling2Child);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreadsForChildren(ImmutableList.of(reply));
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads =
+ ImmutableSet.of(toThread(root, sibling1, sibling2, sibling1Child, sibling2Child));
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ @Test
+ public void requestedThreadsAreEmptyIfReplyDoesNotReferToAThread() {
+ HumanComment root = createComment("root");
+
+ HumanComment reply = asReply(createComment("reply"), "invalid");
+
+ ImmutableList<HumanComment> comments = ImmutableList.of(root);
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(comments).getThreadsForChildren(ImmutableList.of(reply));
+
+ ImmutableSet<CommentThread<HumanComment>> expectedThreads = ImmutableSet.of();
+ assertThat(commentThreads).isEqualTo(expectedThreads);
+ }
+
+ private static HumanComment createComment(String commentUuid) {
+ return new HumanComment(
+ new Key(commentUuid, "myFile", 1),
+ Account.id(100),
+ new Timestamp(1234),
+ (short) 1,
+ "Comment text",
+ "serverId",
+ true);
+ }
+
+ private static HumanComment asReply(HumanComment comment, String parentUuid) {
+ comment.parentUuid = parentUuid;
+ return comment;
+ }
+
+ private static HumanComment writtenOn(HumanComment comment, Timestamp writtenOn) {
+ comment.writtenOn = writtenOn;
+ return comment;
+ }
+
+ private static CommentThread<HumanComment> toThread(HumanComment... comments) {
+ return CommentThread.<HumanComment>builder().comments(ImmutableList.copyOf(comments)).build();
+ }
+}
diff --git a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
index cb29315..9c30fc9 100644
--- a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
+++ b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
@@ -57,6 +57,8 @@
@Mock private PatchListCache patchListCache;
+ private int uuidCounter = 0;
+
@Test
public void commentsAreNotDroppedWhenDiffNotAvailable() throws Exception {
Project.NameKey project = Project.nameKey("myProject");
@@ -206,7 +208,7 @@
private HumanComment createComment(PatchSet.Id patchsetId, String filePath) {
return new HumanComment(
- new Comment.Key("commentUuid", filePath, patchsetId.get()),
+ new Comment.Key(getUniqueUuid(), filePath, patchsetId.get()),
Account.id(100),
new Timestamp(1234),
(short) 1,
@@ -215,6 +217,10 @@
true);
}
+ private String getUniqueUuid() {
+ return "commentUuid" + uuidCounter++;
+ }
+
private Correspondence<HumanComment, String> hasFilePath() {
return NullAwareCorrespondence.transforming(comment -> comment.key.filename, "hasFilePath");
}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js
deleted file mode 100644
index 673332b..0000000
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js
+++ /dev/null
@@ -1,321 +0,0 @@
-/**
- * @license
- * Copyright (C) 2015 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.
- */
-
-import '../../../styles/gr-change-list-styles.js';
-import '../../shared/gr-account-link/gr-account-link.js';
-import '../../shared/gr-change-star/gr-change-star.js';
-import '../../shared/gr-change-status/gr-change-status.js';
-import '../../shared/gr-date-formatter/gr-date-formatter.js';
-import '../../shared/gr-icons/gr-icons.js';
-import '../../shared/gr-limited-text/gr-limited-text.js';
-import '../../shared/gr-tooltip-content/gr-tooltip-content.js';
-import '../../../styles/shared-styles.js';
-import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.js';
-import '../../plugins/gr-endpoint-param/gr-endpoint-param.js';
-import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners.js';
-import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin.js';
-import {PolymerElement} from '@polymer/polymer/polymer-element.js';
-import {htmlTemplate} from './gr-change-list-item_html.js';
-import {ChangeTableMixin} from '../../../mixins/gr-change-table-mixin/gr-change-table-mixin.js';
-import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
-import {getDisplayName} from '../../../utils/display-name-util.js';
-import {getPluginEndpoints} from '../../shared/gr-js-api-interface/gr-plugin-endpoints.js';
-import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader.js';
-import {appContext} from '../../../services/app-context.js';
-import {truncatePath} from '../../../utils/path-list-util.js';
-import {changeStatuses} from '../../../utils/change-util.js';
-import {isServiceUser} from '../../../utils/account-util.js';
-
-const CHANGE_SIZE = {
- XS: 10,
- SMALL: 50,
- MEDIUM: 250,
- LARGE: 1000,
-};
-
-// How many reviewers should be shown with an account-label?
-const PRIMARY_REVIEWERS_COUNT = 2;
-
-/**
- * @extends PolymerElement
- */
-class GrChangeListItem extends ChangeTableMixin(GestureEventListeners(
- LegacyElementMixin(PolymerElement))) {
- static get template() { return htmlTemplate; }
-
- static get is() { return 'gr-change-list-item'; }
-
- static get properties() {
- return {
- /** The logged-in user's account, or null if no user is logged in. */
- account: {
- type: Object,
- value: null,
- },
- visibleChangeTableColumns: Array,
- labelNames: {
- type: Array,
- },
-
- /** @type {?} */
- change: Object,
- config: Object,
- /** Name of the section in the change-list. Used for reporting. */
- sectionName: String,
- changeURL: {
- type: String,
- computed: '_computeChangeURL(change)',
- },
- statuses: {
- type: Array,
- computed: '_changeStatuses(change)',
- },
- showStar: {
- type: Boolean,
- value: false,
- },
- showNumber: Boolean,
- _changeSize: {
- type: String,
- computed: '_computeChangeSize(change)',
- },
- _dynamicCellEndpoints: {
- type: Array,
- },
- };
- }
-
- constructor() {
- super();
- this.reporting = appContext.reportingService;
- }
-
- /** @override */
- attached() {
- super.attached();
- getPluginLoader().awaitPluginsLoaded()
- .then(() => {
- this._dynamicCellEndpoints = getPluginEndpoints().getDynamicEndpoints(
- 'change-list-item-cell');
- });
- }
-
- _changeStatuses(change) {
- return changeStatuses(change);
- }
-
- _computeChangeURL(change) {
- return GerritNav.getUrlForChange(change);
- }
-
- _computeLabelTitle(change, labelName) {
- const label = change.labels[labelName];
- if (!label) { return 'Label not applicable'; }
- const significantLabel = label.rejected || label.approved ||
- label.disliked || label.recommended;
- if (significantLabel && significantLabel.name) {
- return labelName + '\nby ' + significantLabel.name;
- }
- return labelName;
- }
-
- _computeLabelClass(change, labelName) {
- const label = change.labels[labelName];
- // Mimic a Set.
- const classes = {
- cell: true,
- label: true,
- };
- if (label) {
- if (label.approved) {
- classes['u-green'] = true;
- }
- if (label.value == 1) {
- classes['u-monospace'] = true;
- classes['u-green'] = true;
- } else if (label.value == -1) {
- classes['u-monospace'] = true;
- classes['u-red'] = true;
- }
- if (label.rejected) {
- classes['u-red'] = true;
- }
- } else {
- classes['u-gray-background'] = true;
- }
- return Object.keys(classes).sort()
- .join(' ');
- }
-
- _computeLabelValue(change, labelName) {
- const label = change.labels[labelName];
- if (!label) { return ''; }
- if (label.approved) {
- return '✓';
- }
- if (label.rejected) {
- return '✕';
- }
- if (label.value > 0) {
- return '+' + label.value;
- }
- if (label.value < 0) {
- return label.value;
- }
- return '';
- }
-
- _computeRepoUrl(change) {
- return GerritNav.getUrlForProjectChanges(change.project, true,
- change.internalHost);
- }
-
- _computeRepoBranchURL(change) {
- return GerritNav.getUrlForBranch(change.branch, change.project, null,
- change.internalHost);
- }
-
- _computeTopicURL(change) {
- if (!change.topic) { return ''; }
- return GerritNav.getUrlForTopic(change.topic, change.internalHost);
- }
-
- /**
- * Computes the display string for the project column. If there is a host
- * specified in the change detail, the string will be prefixed with it.
- *
- * @param {!Object} change
- * @param {string=} truncate whether or not the project name should be
- * truncated. If this value is truthy, the name will be truncated.
- * @return {string}
- */
- _computeRepoDisplay(change, truncate) {
- if (!change || !change.project) { return ''; }
- let str = '';
- if (change.internalHost) { str += change.internalHost + '/'; }
- str += truncate ? truncatePath(change.project, 2) : change.project;
- return str;
- }
-
- _computeSizeTooltip(change) {
- if (change.insertions + change.deletions === 0 ||
- isNaN(change.insertions + change.deletions)) {
- return 'Size unknown';
- } else {
- return `added ${change.insertions}, removed ${change.deletions} lines`;
- }
- }
-
- _hasAttention(account) {
- if (!this.change || !this.change.attention_set) return false;
- return this.change.attention_set.hasOwnProperty(account._account_id);
- }
-
- /**
- * Computes the array of all reviewers with sorting the reviewers in the
- * attention set before others, and the current user first.
- */
- _computeReviewers(change) {
- if (!change || !change.reviewers || !change.reviewers.REVIEWER) return [];
- const reviewers = [...change.reviewers.REVIEWER].filter(r =>
- (!change.owner || change.owner._account_id !== r._account_id) &&
- !isServiceUser(r)
- );
- reviewers.sort((r1, r2) => {
- if (this.account) {
- if (r1._account_id === this.account._account_id) return -1;
- if (r2._account_id === this.account._account_id) return 1;
- }
- if (this._hasAttention(r1) && !this._hasAttention(r2)) return -1;
- if (this._hasAttention(r2) && !this._hasAttention(r1)) return 1;
- return (r1.name || '').localeCompare(r2.name || '');
- });
- return reviewers;
- }
-
- _computePrimaryReviewers(change) {
- return this._computeReviewers(change).slice(0, PRIMARY_REVIEWERS_COUNT);
- }
-
- _computeAdditionalReviewers(change) {
- return this._computeReviewers(change).slice(PRIMARY_REVIEWERS_COUNT);
- }
-
- _computeAdditionalReviewersCount(change) {
- return this._computeAdditionalReviewers(change).length;
- }
-
- _computeAdditionalReviewersTitle(change, config) {
- if (!change || !config) return '';
- return this._computeAdditionalReviewers(change)
- .map(user => getDisplayName(config, user, true))
- .join(', ');
- }
-
- _computeComments(unresolved_comment_count) {
- if (!unresolved_comment_count || unresolved_comment_count < 1) return '';
- return `${unresolved_comment_count} unresolved`;
- }
-
- /**
- * TShirt sizing is based on the following paper:
- * http://dirkriehle.com/wp-content/uploads/2008/09/hicss-42-csdistr-final-web.pdf
- */
- _computeChangeSize(change) {
- const delta = change.insertions + change.deletions;
- if (isNaN(delta) || delta === 0) {
- return null; // Unknown
- }
- if (delta < CHANGE_SIZE.XS) {
- return 'XS';
- } else if (delta < CHANGE_SIZE.SMALL) {
- return 'S';
- } else if (delta < CHANGE_SIZE.MEDIUM) {
- return 'M';
- } else if (delta < CHANGE_SIZE.LARGE) {
- return 'L';
- } else {
- return 'XL';
- }
- }
-
- toggleReviewed() {
- const newVal = !this.change.reviewed;
- this.set('change.reviewed', newVal);
- this.dispatchEvent(new CustomEvent('toggle-reviewed', {
- bubbles: true,
- composed: true,
- detail: {change: this.change, reviewed: newVal},
- }));
- }
-
- _handleChangeClick(e) {
- // Don't prevent the default and neither stop bubbling. We just want to
- // report the click, but then let the browser handle the click on the link.
-
- const selfId = (this.account && this.account._account_id) || -1;
- const ownerId = (this.change && this.change.owner
- && this.change.owner._account_id) || -1;
-
- this.reporting.reportInteraction('change-row-clicked', {
- section: this.sectionName,
- isOwner: selfId === ownerId,
- });
- }
-}
-
-customElements.define(GrChangeListItem.is, GrChangeListItem);
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
new file mode 100644
index 0000000..5d898bd
--- /dev/null
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.ts
@@ -0,0 +1,366 @@
+/**
+ * @license
+ * Copyright (C) 2015 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.
+ */
+
+import '../../../styles/gr-change-list-styles';
+import '../../shared/gr-account-link/gr-account-link';
+import '../../shared/gr-change-star/gr-change-star';
+import '../../shared/gr-change-status/gr-change-status';
+import '../../shared/gr-date-formatter/gr-date-formatter';
+import '../../shared/gr-icons/gr-icons';
+import '../../shared/gr-limited-text/gr-limited-text';
+import '../../shared/gr-tooltip-content/gr-tooltip-content';
+import '../../../styles/shared-styles';
+import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
+import '../../plugins/gr-endpoint-param/gr-endpoint-param';
+import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners';
+import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
+import {PolymerElement} from '@polymer/polymer/polymer-element';
+import {htmlTemplate} from './gr-change-list-item_html';
+import {ChangeTableMixin} from '../../../mixins/gr-change-table-mixin/gr-change-table-mixin';
+import {GerritNav} from '../../core/gr-navigation/gr-navigation';
+import {getDisplayName} from '../../../utils/display-name-util';
+import {getPluginEndpoints} from '../../shared/gr-js-api-interface/gr-plugin-endpoints';
+import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader';
+import {appContext} from '../../../services/app-context';
+import {truncatePath} from '../../../utils/path-list-util';
+import {changeStatuses} from '../../../utils/change-util';
+import {isServiceUser} from '../../../utils/account-util';
+import {customElement, property} from '@polymer/decorators';
+import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
+import {
+ ChangeInfo,
+ ServerInfo,
+ AccountInfo,
+ QuickLabelInfo,
+} from '../../../types/common';
+import {hasOwnProperty} from '../../../utils/common-util';
+
+enum CHANGE_SIZE {
+ XS = 10,
+ SMALL = 50,
+ MEDIUM = 250,
+ LARGE = 1000,
+}
+
+// How many reviewers should be shown with an account-label?
+const PRIMARY_REVIEWERS_COUNT = 2;
+
+@customElement('gr-change-list-item')
+class GrChangeListItem extends ChangeTableMixin(
+ GestureEventListeners(LegacyElementMixin(PolymerElement))
+) {
+ static get template() {
+ return htmlTemplate;
+ }
+
+ /** The logged-in user's account, or null if no user is logged in. */
+ @property({type: Object})
+ account: AccountInfo | null = null;
+
+ @property({type: Array})
+ visibleChangeTableColumns?: string[];
+
+ @property({type: Array})
+ labelNames?: string[];
+
+ @property({type: Object})
+ change?: ChangeInfo;
+
+ @property({type: Object})
+ config?: ServerInfo;
+
+ /** Name of the section in the change-list. Used for reporting. */
+ @property({type: String})
+ sectionName?: string;
+
+ @property({type: String, computed: '_computeChangeURL(change)'})
+ changeURL?: string;
+
+ @property({type: Array, computed: '_changeStatuses(change)'})
+ statuses?: string[];
+
+ @property({type: Boolean})
+ showStar = false;
+
+ @property({type: Boolean})
+ showNumber = false;
+
+ @property({type: String, computed: '_computeChangeSize(change)'})
+ _changeSize?: string;
+
+ @property({type: Array})
+ _dynamicCellEndpoints?: string[];
+
+ reporting: ReportingService = appContext.reportingService;
+
+ /** @override */
+ attached() {
+ super.attached();
+ getPluginLoader()
+ .awaitPluginsLoaded()
+ .then(() => {
+ this._dynamicCellEndpoints = getPluginEndpoints().getDynamicEndpoints(
+ 'change-list-item-cell'
+ );
+ });
+ }
+
+ _changeStatuses(change?: ChangeInfo) {
+ if (!change) return [];
+ return changeStatuses(change);
+ }
+
+ _computeChangeURL(change?: ChangeInfo) {
+ if (!change) return '';
+ return GerritNav.getUrlForChange(change);
+ }
+
+ _computeLabelTitle(change: ChangeInfo | undefined, labelName: string) {
+ const label: QuickLabelInfo | undefined = change?.labels?.[labelName];
+ if (!label) {
+ return 'Label not applicable';
+ }
+ const significantLabel =
+ label.rejected || label.approved || label.disliked || label.recommended;
+ if (significantLabel && significantLabel.name) {
+ return `${labelName}\nby ${significantLabel.name}`;
+ }
+ return labelName;
+ }
+
+ _computeLabelClass(change: ChangeInfo | undefined, labelName: string) {
+ const label: QuickLabelInfo | undefined = change?.labels?.[labelName];
+ // Mimic a Set.
+ // TODO(TS): replace with `u_green` to remove the quotes and brackets
+ const classes: {
+ cell: boolean;
+ label: boolean;
+ ['u-green']?: boolean;
+ ['u-monospace']?: boolean;
+ ['u-red']?: boolean;
+ ['u-gray-background']?: boolean;
+ } = {
+ cell: true,
+ label: true,
+ };
+ if (label) {
+ if (label.approved) {
+ classes['u-green'] = true;
+ }
+ if (label.value === 1) {
+ classes['u-monospace'] = true;
+ classes['u-green'] = true;
+ } else if (label.value === -1) {
+ classes['u-monospace'] = true;
+ classes['u-red'] = true;
+ }
+ if (label.rejected) {
+ classes['u-red'] = true;
+ }
+ } else {
+ classes['u-gray-background'] = true;
+ }
+ return Object.keys(classes).sort().join(' ');
+ }
+
+ _computeLabelValue(change: ChangeInfo | undefined, labelName: string) {
+ const label: QuickLabelInfo | undefined = change?.labels?.[labelName];
+ if (!label) {
+ return '';
+ }
+ if (label.approved) {
+ return '✓';
+ }
+ if (label.rejected) {
+ return '✕';
+ }
+ if (label.value && label.value > 0) {
+ return `+${label.value}`;
+ }
+ if (label.value && label.value < 0) {
+ return label.value;
+ }
+ return '';
+ }
+
+ _computeRepoUrl(change?: ChangeInfo) {
+ if (!change) return '';
+ return GerritNav.getUrlForProjectChanges(
+ change.project,
+ true,
+ change.internalHost
+ );
+ }
+
+ _computeRepoBranchURL(change?: ChangeInfo) {
+ if (!change) return '';
+ return GerritNav.getUrlForBranch(
+ change.branch,
+ change.project,
+ undefined,
+ change.internalHost
+ );
+ }
+
+ _computeTopicURL(change?: ChangeInfo) {
+ if (!change?.topic) {
+ return '';
+ }
+ return GerritNav.getUrlForTopic(change.topic, change.internalHost);
+ }
+
+ /**
+ * Computes the display string for the project column. If there is a host
+ * specified in the change detail, the string will be prefixed with it.
+ *
+ * @param truncate whether or not the project name should be
+ * truncated. If this value is truthy, the name will be truncated.
+ */
+ _computeRepoDisplay(change: ChangeInfo | undefined, truncate: boolean) {
+ if (!change?.project) {
+ return '';
+ }
+ let str = '';
+ if (change.internalHost) {
+ str += change.internalHost + '/';
+ }
+ str += truncate ? truncatePath(change.project, 2) : change.project;
+ return str;
+ }
+
+ _computeSizeTooltip(change?: ChangeInfo) {
+ if (
+ !change ||
+ change.insertions + change.deletions === 0 ||
+ isNaN(change.insertions + change.deletions)
+ ) {
+ return 'Size unknown';
+ } else {
+ return `added ${change.insertions}, removed ${change.deletions} lines`;
+ }
+ }
+
+ _hasAttention(account: AccountInfo) {
+ if (!this.change || !this.change.attention_set) return false;
+ return hasOwnProperty(this.change.attention_set, account._account_id);
+ }
+
+ /**
+ * Computes the array of all reviewers with sorting the reviewers in the
+ * attention set before others, and the current user first.
+ */
+ _computeReviewers(change?: ChangeInfo) {
+ if (!change?.reviewers || !change?.reviewers.REVIEWER) return [];
+ const reviewers = [...change.reviewers.REVIEWER].filter(
+ r =>
+ (!change.owner || change.owner._account_id !== r._account_id) &&
+ !isServiceUser(r)
+ );
+ reviewers.sort((r1, r2) => {
+ if (this.account) {
+ if (r1._account_id === this.account._account_id) return -1;
+ if (r2._account_id === this.account._account_id) return 1;
+ }
+ if (this._hasAttention(r1) && !this._hasAttention(r2)) return -1;
+ if (this._hasAttention(r2) && !this._hasAttention(r1)) return 1;
+ return (r1.name || '').localeCompare(r2.name || '');
+ });
+ return reviewers;
+ }
+
+ _computePrimaryReviewers(change?: ChangeInfo) {
+ return this._computeReviewers(change).slice(0, PRIMARY_REVIEWERS_COUNT);
+ }
+
+ _computeAdditionalReviewers(change?: ChangeInfo) {
+ return this._computeReviewers(change).slice(PRIMARY_REVIEWERS_COUNT);
+ }
+
+ _computeAdditionalReviewersCount(change?: ChangeInfo) {
+ return this._computeAdditionalReviewers(change).length;
+ }
+
+ _computeAdditionalReviewersTitle(
+ change: ChangeInfo | undefined,
+ config: ServerInfo
+ ) {
+ if (!change || !config) return '';
+ return this._computeAdditionalReviewers(change)
+ .map(user => getDisplayName(config, user, true))
+ .join(', ');
+ }
+
+ _computeComments(unresolved_comment_count?: number) {
+ if (!unresolved_comment_count || unresolved_comment_count < 1) return '';
+ return `${unresolved_comment_count} unresolved`;
+ }
+
+ /**
+ * TShirt sizing is based on the following paper:
+ * http://dirkriehle.com/wp-content/uploads/2008/09/hicss-42-csdistr-final-web.pdf
+ */
+ _computeChangeSize(change?: ChangeInfo) {
+ if (!change) return null;
+ const delta = change.insertions + change.deletions;
+ if (isNaN(delta) || delta === 0) {
+ return null; // Unknown
+ }
+ if (delta < CHANGE_SIZE.XS) {
+ return 'XS';
+ } else if (delta < CHANGE_SIZE.SMALL) {
+ return 'S';
+ } else if (delta < CHANGE_SIZE.MEDIUM) {
+ return 'M';
+ } else if (delta < CHANGE_SIZE.LARGE) {
+ return 'L';
+ } else {
+ return 'XL';
+ }
+ }
+
+ toggleReviewed() {
+ const newVal = !this.change?.reviewed;
+ this.set('change.reviewed', newVal);
+ this.dispatchEvent(
+ new CustomEvent('toggle-reviewed', {
+ bubbles: true,
+ composed: true,
+ detail: {change: this.change, reviewed: newVal},
+ })
+ );
+ }
+
+ _handleChangeClick() {
+ // Don't prevent the default and neither stop bubbling. We just want to
+ // report the click, but then let the browser handle the click on the link.
+
+ const selfId = (this.account && this.account._account_id) || -1;
+ const ownerId =
+ (this.change && this.change.owner && this.change.owner._account_id) || -1;
+
+ this.reporting.reportInteraction('change-row-clicked', {
+ section: this.sectionName,
+ isOwner: selfId === ownerId,
+ });
+ }
+}
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-change-list-item': GrChangeListItem;
+ }
+}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.js b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.js
index 6d51310..1970928 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.js
@@ -278,7 +278,7 @@
assert.deepEqual(GerritNav.getUrlForProjectChanges.lastCall.args,
[change.project, true, change.internalHost]);
assert.deepEqual(GerritNav.getUrlForBranch.lastCall.args,
- [change.branch, change.project, null, change.internalHost]);
+ [change.branch, change.project, undefined, change.internalHost]);
assert.deepEqual(GerritNav.getUrlForTopic.lastCall.args,
[change.topic, change.internalHost]);
});
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
index affa8b5..c4ebe1e 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
@@ -24,6 +24,10 @@
import {dedupingMixin} from '@polymer/polymer/lib/utils/mixin';
import {property, observe} from '@polymer/decorators';
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
+import {
+ pushScrollLock,
+ removeScrollLock,
+} from '@polymer/iron-overlay-behavior/iron-scroll-manager';
const HOVER_CLASS = 'hovered';
const HIDE_CLASS = 'hide';
@@ -127,8 +131,10 @@
// show the hovercard if mouse moves to hovercard
// this will cancel pending hide as well
this.listen(this, 'mouseenter', 'show');
+ this.listen(this, 'mouseenter', 'lock');
// when leave hovercard, hide it immediately
this.listen(this, 'mouseleave', 'hide');
+ this.listen(this, 'mouseleave', 'unlock');
}
/** @override */
@@ -213,6 +219,13 @@
}
/**
+ * unlock scroll, this will resume the scroll outside of the hovercard.
+ */
+ unlock() {
+ removeScrollLock(this);
+ }
+
+ /**
* Hides/closes the hovercard. This occurs when the user triggers the
* `mouseleave` event on the hovercard's `target` element (as long as the
* user is not hovering over the hovercard).
@@ -287,6 +300,13 @@
}
/**
+ * Lock background scroll but enable scroll inside of current hovercard.
+ */
+ lock() {
+ pushScrollLock(this);
+ }
+
+ /**
* Shows/opens the hovercard. This occurs when the user triggers the
* `mousenter` event on the hovercard's `target` element.
*/
diff --git a/polygerrit-ui/app/utils/change-util.ts b/polygerrit-ui/app/utils/change-util.ts
index 298ae9d..de43884c 100644
--- a/polygerrit-ui/app/utils/change-util.ts
+++ b/polygerrit-ui/app/utils/change-util.ts
@@ -16,16 +16,7 @@
*/
import {getBaseUrl} from './url-util';
import {ChangeStatus} from '../constants/constants';
-import {LegacyChangeId, PatchSetNum} from '../types/common';
-
-// This can be wrong! See WARNING above
-interface Change {
- status: string; // This can be wrong! See WARNING above
- mergeable: boolean; // This can be wrong! See WARNING above
- work_in_progress: boolean; // This can be wrong! See WARNING above
- is_private: boolean; // This can be wrong! See WARNING above
- submittable: boolean; // This can be wrong! See WARNING above
-}
+import {LegacyChangeId, PatchSetNum, ChangeInfo} from '../types/common';
// This can be wrong! See WARNING above
interface ChangeStatusesOptions {
@@ -132,12 +123,12 @@
return `${getBaseUrl()}/c/${changeNum}`;
}
-export function changeIsOpen(change?: Change) {
+export function changeIsOpen(change?: ChangeInfo) {
return change?.status === ChangeStatus.NEW;
}
export function changeStatuses(
- change: Change,
+ change: ChangeInfo,
opt_options?: ChangeStatusesOptions
) {
const states = [];
@@ -175,6 +166,6 @@
return states;
}
-export function changeStatusString(change: Change) {
+export function changeStatusString(change: ChangeInfo) {
return changeStatuses(change).join(', ');
}
