Merge "Lock and unlock hovercard when its shown / hidden"
diff --git a/.bazelversion b/.bazelversion
index 47b322c..1545d96 100644
--- a/.bazelversion
+++ b/.bazelversion
@@ -1 +1 @@
-3.4.1
+3.5.0
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index d96ae46..6b89d67 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2828,7 +2828,8 @@
+
Enable (or disable) the `'$site_path'/logs/httpd_log` request log.
If enabled, an NCSA combined log format request log file is written
-out by the internal HTTP daemon.
+out by the internal HTTP daemon. The httpd log format is documented
+link:logs.html#_httpd_log[here].
+
`log4j.appender` with the name `httpd_log` can be configured to overwrite
programmatic configuration.
@@ -4881,6 +4882,21 @@
+
By default, 30s.
+[[sshd.gracefulStopTimeout]]sshd.gracefulStopTimeout::
++
+Set a graceful stop time. If set, Gerrit ensures that all open SSH
+sessions are preserved for a maximum period of time, before forcing the
+shutdown of the SSH daemon. During this period, no new requests
+will be accepted. This option is meant to be used in setups performing
+rolling restarts.
++
+Values should use common unit suffixes to express their setting:
++
+* s, sec, second, seconds
+* m, min, minute, minutes
++
+By default, 0 seconds (immediate shutdown).
+
[[sshd.maxConnectionsPerUser]]sshd.maxConnectionsPerUser::
+
Maximum number of concurrent SSH sessions that a user account
@@ -5006,6 +5022,7 @@
+
Enable (or disable) the `'$site_path'/logs/sshd_log` request log.
If enabled, a request log file is written out by the SSH daemon.
+The sshd log format is documented link:logs.html#_sshd_log[here].
+
`log4j.appender` with the name `sshd_log` can be configured to overwrite
programmatic configuration.
diff --git a/Documentation/config-reverseproxy.txt b/Documentation/config-reverseproxy.txt
index eff777b..3a9bcc7 100644
--- a/Documentation/config-reverseproxy.txt
+++ b/Documentation/config-reverseproxy.txt
@@ -21,6 +21,13 @@
listenUrl = proxy-http://127.0.0.1:8081/r/
----
+== Reverse proxy and client IPs
+
+When behind a reverse proxy the http_log will log the IP of the reverse proxy
+as client.ip. To log the correct client IP you must provide the
+'X-Forwarded-For' header from the reverse proxy.
+See the nginx configuration example below.
+
== Apache 2 Configuration
diff --git a/Documentation/dev-bazel.txt b/Documentation/dev-bazel.txt
index 01cd494..ee2f4a1 100644
--- a/Documentation/dev-bazel.txt
+++ b/Documentation/dev-bazel.txt
@@ -90,12 +90,12 @@
```
$ cat << EOF > ~/.bazelrc
-> build --define=ABSOLUTE_JAVABASE=<path-to-java-13>
-> build --javabase=@bazel_tools//tools/jdk:absolute_javabase
-> build --host_javabase=@bazel_tools//tools/jdk:absolute_javabase
-> build --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_vanilla
-> build --java_toolchain=@bazel_tools//tools/jdk:toolchain_vanilla
-> EOF
+build --define=ABSOLUTE_JAVABASE=<path-to-java-13>
+build --javabase=@bazel_tools//tools/jdk:absolute_javabase
+build --host_javabase=@bazel_tools//tools/jdk:absolute_javabase
+build --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_vanilla
+build --java_toolchain=@bazel_tools//tools/jdk:toolchain_vanilla
+EOF
```
Now, invoking Bazel with just `bazel build :release` would include
diff --git a/Documentation/dev-e2e-tests.txt b/Documentation/dev-e2e-tests.txt
index 78b2c15..c5b3bfc 100644
--- a/Documentation/dev-e2e-tests.txt
+++ b/Documentation/dev-e2e-tests.txt
@@ -27,14 +27,33 @@
leveraged to run tests at the Git protocol level.
Gatling is written in Scala, but the abstraction provided by the Gatling DSL makes the scenarios
-implementation easy even without any Scala knowledge. The
-link:https://gitenterprise.me/2019/12/20/stress-your-gerrit-with-gatling/[Stress your Gerrit with Gatling,role=external,window=_blank]
-blog post has more introductory information.
+implementation easy even without any Scala knowledge. The online `End-to-end tests`
+link:https://www.gerritcodereview.com/presentations.html#list-of-presentations[presentation,role=external,window=_blank]
+links posted on the homepage have more introductory information.
+
+== IDE: IntelliJ
Examples of scenarios can be found in the `e2e-tests` directory. The files in that directory should
be formatted using the mainstream
link:https://plugins.jetbrains.com/plugin/1347-scala[Scala plugin for IntelliJ,role=external,window=_blank].
The latter is not mandatory but preferred for `sbt` and Scala IDE purposes in this project.
+So, Eclipse can also be used alongside as a development IDE; this is described below.
+
+=== Eclipse
+
+1. Install the link:http://scala-ide.org/docs/user/gettingstarted.html[Scala plugin for Eclipse,role=external,window=_blank].
+1. Run `sbt eclipse` from the `e2e-tests` root directory.
+1. Import the resulting `e2e-tests` eclipse file inside the Gerrit project, in Eclipse.
+1. You should see errors in Eclipse telling you there are missing packages.
+1. This is due to the sbt-eclipse plugin not properly linking the Gerrit Gatling e2e tests with
+ Gatling Git plugin.
+1. You then have to right-click on the root directory and choose the build path->link source option.
+1. Then you have to browse to `.sbt/1.0/staging`, find the folder where gatling-git is contained,
+ and choose that.
+1. That last step should link the gatling-git plugin to the project; e2e tests should not show
+ errors anymore.
+1. You may get errors in the gatling-git directory; these should not affect Gerrit Gatling
+ development and can be ignored.
== How to build the tests
@@ -163,10 +182,11 @@
Plugin or otherwise non-core scenarios may do so just as well. The core java package
`com.google.gerrit.scenarios` from the example above has to be replaced with the one under which
those scenario classes are. Such extending scenarios can also add extension-specific properties.
-Early examples of this can be found in the Gerrit
-`link:https://gerrit.googlesource.com/plugins/high-availability[high-availability,role=external,window=_blank]`
-and `link:https://gerrit.googlesource.com/plugins/multi-site[multi-site,role=external,window=_blank]`
-plugins test code.
+Examples of this can be found in these Gerrit plugins test code:
+
+* `link:https://gerrit.googlesource.com/plugins/high-availability[high-availability,role=external,window=_blank]`
+* `link:https://gerrit.googlesource.com/plugins/multi-site[multi-site,role=external,window=_blank]`
+* `link:https://gerrit.googlesource.com/plugins/rename-project[rename-project,role=external,window=_blank]`
Further above, the `_PROJECT` keyword is prefixed with an underscore, which means that its value
gets automatically generated by the scenario. Any property setting for it is therefore not
diff --git a/Documentation/dev-eclipse.txt b/Documentation/dev-eclipse.txt
index 9596a55..742cf42 100644
--- a/Documentation/dev-eclipse.txt
+++ b/Documentation/dev-eclipse.txt
@@ -41,6 +41,37 @@
Filters on a folder, they will be overwritten the next time you run
`tools/eclipse/project.py`.
+=== Eclipse project on MacOS
+
+By default, bazel uses `/private/var/tmp` as the
+link:https://docs.bazel.build/versions/master/output_directories.html[outputRoot on MacOS].
+This means that the eclipse project will reference libraries stored under that directory.
+However, MacOS runs periodic cleanup task which deletes the content under `/private/var/tmp`
+which wasn't accessed or modified for some days, by default 3 days. This can lead to a broken
+Eclipse project as referenced libraries get deleted.
+
+There are two possibilities to mitigate this issue.
+
+==== Change the location of the bazel output directory
+On Linux, the output directory defaults to `$HOME/.cache/bazel` and the same can be configured
+on Mac too. Edit, or create, the `$HOME/.bazelrc` file and add the following line:
+----
+startup --output_user_root=/Users/johndoe/.cache/bazel
+----
+
+==== Increase the treshold for the cleanup of temporary files
+The default treshold for the cleanup can be overriden by creating a configuration file under
+`/etc/periodic.conf` and setting a larger value for the `daily_clean_tmps_days`.
+
+An example `/etc/periodic.conf` file:
+
+----
+# This file overrides the settings from /etc/defaults/periodic.conf
+daily_clean_tmps_days="45" # If not accessed for
+----
+
+For more details about the proposed workaround see link:https://superuser.com/a/187105[this post]
+
=== Eclipse project with custom plugins ===
To add custom plugins to the eclipse project add them to `tools/bzl/plugins.bzl`
diff --git a/Documentation/images/inline-edit-create-change-project-screen-dialog.png b/Documentation/images/inline-edit-create-change-project-screen-dialog.png
deleted file mode 100644
index ea5daa9..0000000
--- a/Documentation/images/inline-edit-create-change-project-screen-dialog.png
+++ /dev/null
Binary files differ
diff --git a/Documentation/images/inline-edit-create-change-project-screen.png b/Documentation/images/inline-edit-create-change-project-screen.png
deleted file mode 100644
index e9c7033..0000000
--- a/Documentation/images/inline-edit-create-change-project-screen.png
+++ /dev/null
Binary files differ
diff --git a/Documentation/images/inline-edit-create-follow-up-change.png b/Documentation/images/inline-edit-create-follow-up-change.png
deleted file mode 100644
index 3e81eee..0000000
--- a/Documentation/images/inline-edit-create-follow-up-change.png
+++ /dev/null
Binary files differ
diff --git a/Documentation/images/inline-edit-edit-in-diff-screen-patch-list.png b/Documentation/images/inline-edit-edit-in-diff-screen-patch-list.png
deleted file mode 100644
index bdbc59d..0000000
--- a/Documentation/images/inline-edit-edit-in-diff-screen-patch-list.png
+++ /dev/null
Binary files differ
diff --git a/Documentation/images/inline-edit-edit-in-patch-list.png b/Documentation/images/inline-edit-edit-in-patch-list.png
deleted file mode 100644
index 9a31e02..0000000
--- a/Documentation/images/inline-edit-edit-in-patch-list.png
+++ /dev/null
Binary files differ
diff --git a/Documentation/index.txt b/Documentation/index.txt
index 4fb977a..8f36ecc 100644
--- a/Documentation/index.txt
+++ b/Documentation/index.txt
@@ -69,6 +69,7 @@
. link:cmd-index.html[Command Line Tools]
. link:config-plugins.html#replication[Replication]
. link:config-plugins.html[Plugins]
+. link:logs.html[Log Files]
. link:metrics.html[Metrics]
. link:config-reverseproxy.html[Reverse Proxy]
. link:config-auto-site-initialization.html[Automatic Site Initialization on Startup]
diff --git a/Documentation/logs.txt b/Documentation/logs.txt
new file mode 100644
index 0000000..6624366
--- /dev/null
+++ b/Documentation/logs.txt
@@ -0,0 +1,165 @@
+= Gerrit Code Review - Logs
+
+Gerrit writes log files in the `$site_path/logs/` folder tracking requests,
+background and plugin activity and errors. By default logs are written in
+link:config-gerrit.html#log.textLogging[text format], optionally in
+link:config-gerrit.html#log.jsonLogging[JSON format].
+By default log files are link:config-gerrit.html#log.compress[compressed]
+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.
+
+=== HTTPD Log
+
+The httpd log tracks HTTP requests processed by Gerrit's http daemon
+and is written to `$site_path/logs/httpd_log`. Enabled or disabled via the
+link:config-gerrit.html#httpd.requestLog[httpd.requestLog] option.
+
+Format is an enhanced
+link:https://httpd.apache.org/docs/2.4/logs.html#combined[NCSA combined log],
+if a log field is not present, a "-" is substituted:
+
+* `host`: The IP address of the HTTP client that made the HTTP resource request.
+ If you are using a reverse proxy it depends on the proxy configuration if the
+ proxy IP address or the client IP address is logged.
+* `[thread name]`: name of the Java thread executing the request.
+* `remote logname`: the identifier used to
+ link: https://tools.ietf.org/html/rfc1413[identify the client making the HTTP request],
+ Gerrit always logs a dash `-`.
+* `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.
+* `request`: The request line from the client is given in double quotes.
+** the HTTP method used by the client.
+** the resource the client requested.
+** the protocol/version used by the client.
+* `statuscode`: the link:https://tools.ietf.org/html/rfc2616#section-10[HTTP status code]
+ that the server sent back to the client.
+* `response size`: the number of bytes of data transferred as part of the HTTP
+ response, not including the HTTP header.
+* `latency`: response time in milliseconds.
+* `referer`: the `Referer` HTTP request header. This gives the site that
+ the client reports having been referred from.
+* `client agent`: the client agent which sent the request.
+
+Example:
+```
+12.34.56.78 [HTTP-4136374] - johndoe [28/Aug/2020:10:02:20 +0200] "GET /a/plugins/metrics-reporter-prometheus/metrics HTTP/1.1" 200 1247498 1900 - "Prometheus/2.13.1"
+```
+
+=== SSHD Log
+
+The sshd log tracks ssh requests processed by Gerrit's ssh daemon
+and is written to `$site_path/logs/sshd_log`. Enabled or disabled
+via option link:config-gerrit.html#sshd.requestLog[sshd.requestLog].
+
+Log format:
+
+* `[date time]`: The time that the request was received.
+* `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
+ get all requests from that session.
+* `[thread name]`: name of the Java thread executing the request.
+* `username`: the username used by the client for authentication.
+* `a/accountid`: identifier of the Gerrit account which is logged on.
+* `operation`: the operation being executed via ssh.
+** `LOGIN FROM <host>`: login and start new SSH session from the given host.
+** `AUTH FAILURE FROM <host> <message>`: failed authentication from given host and cause of failure.
+** `LOGOUT`: logout and terminate SSH session.
+** `git-upload-pack.<projectname>`: git fetch or clone command for given project.
+** `git-receive-pack.<projectname>`: git push command for given project.
+** Gerrit ssh commands which may be logged in this field are documented
+ link:cmd-index.html#_server[here].
+* `wait`: command wait time, time in milliseconds the command waited for an execution thread.
+* `exec`: command execution time, time in milliseconds to execute the command.
+* `status`: status code. 0 means success, any other value is an error.
+
+The `git-upload-pack` command provides the following additional fields after the `exec`
+and before the `status` field. All times are in milliseconds. Fields are -1 if not available
+when the upload-pack request returns an empty result since the client's repository was up to date:
+
+* `time negotiating`: time for negotiating which objects need to be transferred.
+* `time searching for reuse`: time jgit searched for deltas which can be reused.
+ That is the time spent matching existing representations against objects that
+ will be transmitted, or that the client can be assumed to already have.
+* `time searching for sizes`: time jgit was searching for sizes of all objects that
+ will enter the delta compression search window. The sizes need to
+ be known to better match similar objects together and improve
+ delta compression ratios.
+* `time counting`: time jgit spent enumerating the objects that need to
+ be included in the output. This time includes any restarts that
+ occur when a cached pack is selected for reuse.
+* `time compressing`: time jgit was compressing objects. This is observed
+ wall-clock time and does not accurately track CPU time used when
+ multiple threads were used to perform the delta compression.
+* `time writing`: time jgit needed to write packfile, from start of
+ header until end of trailer. The transfer speed can be
+ approximated by dividing `total bytes` by this value.
+* `total time in UploadPack`: total time jgit spent in upload-pack.
+* `bitmap index misses`: number of misses when trying to use bitmap index,
+ -1 means no bitmap index available. This is the count of objects that
+ needed to be discovered through an object walk because they were not found
+ in bitmap indices.
+* `total deltas`: total number of deltas transferred. This may be lower than the actual
+ number of deltas if a cached pack was reused.
+* `total objects`: total number of objects transferred. This total includes
+ the value of `total deltas`.
+* `total bytes`: total number of bytes transferred. This size includes the pack
+ header, trailer, thin pack, and reused cached packs.
+* `client agent`: the client agent and version which sent the request.
+
+Example: a CI system established a SSH connection, sent an upload-pack command (git fetch) and closed the connection:
+```
+[2020-08-28 11:00:01,391 +0200] 8a154cae [sshd-SshServer[570fc452]-nio2-thread-299] voter a/1000023 LOGIN FROM 12.34.56.78
+[2020-08-28 11:00:01,556 +0200] 8a154cae [SSH git-upload-pack /AP/ajs/jpaas-msg-svc.git (voter)] voter a/1000056 git-upload-pack./demo/project.git 0ms 115ms 92ms 1ms 0ms 6ms 0ms 0ms 7ms 3 10 26 2615 0 git/2.26.2
+[2020-08-28 11:00:01,583 +0200] 8a154cae [sshd-SshServer[570fc452]-nio2-thread-168] voter a/1000023 LOGOUT
+```
+
+=== Error Log
+
+The error log tracks errors and stack traces and is written to
+`$site_path/logs/error_log`.
+
+Log format:
+
+* `[date time]`: The time that the request was received.
+* `[thread name]`: : name of the Java thread executing the request.
+* `level`: log level (ERROR, WARN, INFO, DEBUG).
+* `logger`: name of the logger.
+* `message`: log message.
+* `stacktrace`: Java stacktrace when an execption was caught, usually spans multiple lines.
+
+=== GC Log
+
+The gc log tracks git garbage collection running in a background thread
+if enabled and is written to `$site_path/logs/gc_log`.
+
+Log format:
+
+* `[date time]`: The time that the request was received.
+* `level`: log level (ERROR, WARN, INFO, DEBUG).
+* `message`: log message.
+
+=== Plugin Logs
+
+Some plugins write their own log file.
+E.g. the replication plugin writes its log to `$site_path/logs/replication_log`.
+Refer to each plugin's documentation for more details on their logs.
+
+GERRIT
+------
+Part of link:index.html[Gerrit Code Review]
+
+SEARCHBOX
+---------
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/WORKSPACE b/WORKSPACE
index 6be35cf..4c2fe35 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -219,15 +219,15 @@
sha1 = GUAVA_BIN_SHA1,
)
-CAFFEINE_VERS = "2.8.0"
+CAFFEINE_VERS = "2.8.5"
maven_jar(
name = "caffeine",
artifact = "com.github.ben-manes.caffeine:caffeine:" + CAFFEINE_VERS,
- sha1 = "6000774d7f8412ced005a704188ced78beeed2bb",
+ sha1 = "f0eafef6e1529a44e36549cd9d1fc06d3a57f384",
)
-CAFFEINE_GUAVA_SHA256 = "3a66ee3ec70971dee0bae6e56bda7b8742bc4bedd7489161bfbbaaf7137d89e1"
+CAFFEINE_GUAVA_SHA256 = "a7ce6d29c40bccd688815a6734070c55b20cd326351a06886a6144005aa32299"
# TODO(davido): Rename guava.jar to caffeine-guava.jar on fetch to prevent potential
# naming collision between caffeine guava adapter and guava library itself.
@@ -758,8 +758,8 @@
# Keep this version of Soy synchronized with the version used in Gitiles.
maven_jar(
name = "soy",
- artifact = "com.google.template:soy:2019-10-08",
- sha1 = "4518bf8bac2dbbed684849bc209c39c4cb546237",
+ artifact = "com.google.template:soy:2020-08-24",
+ sha1 = "e774bf5cc95923d2685292883fe219e231346e50",
)
maven_jar(
diff --git a/e2e-tests/build.sbt b/e2e-tests/build.sbt
index a322970..294212c 100644
--- a/e2e-tests/build.sbt
+++ b/e2e-tests/build.sbt
@@ -2,7 +2,6 @@
enablePlugins(GatlingPlugin)
-lazy val gatlingGitExtension = RootProject(uri("git://github.com/GerritForge/gatling-git.git"))
lazy val root = (project in file("."))
.settings(
inThisBuild(List(
@@ -12,8 +11,8 @@
)),
name := "gerrit",
libraryDependencies ++=
- gatling ++
+ gatling ++ gatlingGit ++
Seq("io.gatling" % "gatling-core" % GatlingVersion) ++
Seq("io.gatling" % "gatling-app" % GatlingVersion),
scalacOptions += "-language:postfixOps"
- ) dependsOn gatlingGitExtension
+ )
diff --git a/e2e-tests/project/Dependencies.scala b/e2e-tests/project/Dependencies.scala
index 63328f9..56ef740 100644
--- a/e2e-tests/project/Dependencies.scala
+++ b/e2e-tests/project/Dependencies.scala
@@ -2,9 +2,17 @@
object Dependencies {
val GatlingVersion = "3.2.0"
+ val GatlingGitVersion = "1.0.12"
lazy val gatling = Seq(
"io.gatling.highcharts" % "gatling-charts-highcharts",
"io.gatling" % "gatling-test-framework",
).map(_ % GatlingVersion % Test)
+
+ lazy val gatlingGit = Seq(
+ "com.gerritforge" %% "gatling-git" % GatlingGitVersion excludeAll(
+ ExclusionRule(organization = "io.gatling"),
+ ExclusionRule(organization = "io.gatling.highcharts")
+ )
+ )
}
diff --git a/e2e-tests/project/plugins.sbt b/e2e-tests/project/plugins.sbt
index 36cd201..9ed0f89 100644
--- a/e2e-tests/project/plugins.sbt
+++ b/e2e-tests/project/plugins.sbt
@@ -1 +1,2 @@
addSbtPlugin("io.gatling" % "gatling-sbt" % "3.0.0")
+addSbtPlugin("com.typesafe.sbteclipse" % "sbteclipse-plugin" % "5.2.4")
diff --git a/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject-body.json b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject-body.json
index bcf4708..282ac99 100644
--- a/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject-body.json
+++ b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject-body.json
@@ -1,3 +1,4 @@
{
- "create_empty_commit": "true"
+ "create_empty_commit": "true",
+ "parent": "${parent}"
}
diff --git a/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject.json b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject.json
index cd90739..c141bb8 100644
--- a/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject.json
+++ b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject.json
@@ -1,5 +1,6 @@
[
{
- "url": "HTTP_SCHEME://HOSTNAME:HTTP_PORT/a/projects/PROJECT"
+ "url": "HTTP_SCHEME://HOSTNAME:HTTP_PORT/a/projects/PROJECT",
+ "parent": "PARENT"
}
]
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateProject.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateProject.scala
index d631292..f2b3d12 100644
--- a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateProject.scala
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateProject.scala
@@ -28,7 +28,7 @@
val test: ScenarioBuilder = scenario(unique)
.feed(data)
- .exec(httpRequest.body(RawFileBody(body)).asJson)
+ .exec(httpRequest.body(ElFileBody(body)).asJson)
setUp(
test.inject(
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala
index 4832392..679320d 100644
--- a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala
@@ -67,6 +67,8 @@
case ("number", number) =>
val precedes = replaceKeyWith("_number", 0, number.toString)
replaceProperty("number", 1, precedes)
+ case ("parent", parent) =>
+ replaceProperty("parent", "All-Projects", parent.toString)
case ("project", project) =>
var precedes = replaceKeyWith("_project", name, project.toString)
precedes = replaceOverride(precedes)
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/sshd/SshDaemon.java b/java/com/google/gerrit/sshd/SshDaemon.java
index 5145c13..c43bf91 100644
--- a/java/com/google/gerrit/sshd/SshDaemon.java
+++ b/java/com/google/gerrit/sshd/SshDaemon.java
@@ -62,7 +62,9 @@
import java.util.Iterator;
import java.util.List;
import java.util.Set;
+import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.mina.transport.socket.SocketSessionConfig;
import org.apache.sshd.common.BaseBuilder;
@@ -72,6 +74,8 @@
import org.apache.sshd.common.compression.BuiltinCompressions;
import org.apache.sshd.common.compression.Compression;
import org.apache.sshd.common.forward.DefaultForwarderFactory;
+import org.apache.sshd.common.future.CloseFuture;
+import org.apache.sshd.common.future.SshFutureListener;
import org.apache.sshd.common.io.AbstractIoServiceFactory;
import org.apache.sshd.common.io.IoAcceptor;
import org.apache.sshd.common.io.IoServiceFactory;
@@ -142,6 +146,7 @@
private final List<HostKey> hostKeys;
private volatile IoAcceptor daemonAcceptor;
private final Config cfg;
+ private final long gracefulStopTimeout;
@Inject
SshDaemon(
@@ -212,6 +217,8 @@
SshSessionBackend backend = cfg.getEnum("sshd", null, "backend", SshSessionBackend.NIO2);
boolean channelIdTracking = cfg.getBoolean("sshd", "enableChannelIdTracking", true);
+ gracefulStopTimeout = cfg.getTimeUnit("sshd", null, "gracefulStopTimeout", 0, TimeUnit.SECONDS);
+
System.setProperty(
IoServiceFactoryFactory.class.getName(),
backend == SshSessionBackend.MINA
@@ -341,6 +348,12 @@
public synchronized void stop() {
if (daemonAcceptor != null) {
try {
+ if (gracefulStopTimeout > 0) {
+ logger.atInfo().log(
+ "Stopping SSHD sessions gracefully with %d seconds timeout.", gracefulStopTimeout);
+ daemonAcceptor.unbind(daemonAcceptor.getBoundAddresses());
+ waitForSessionClose();
+ }
daemonAcceptor.close(true).await();
shutdownExecutors();
logger.atInfo().log("Stopped Gerrit SSHD");
@@ -352,6 +365,30 @@
}
}
+ private void waitForSessionClose() {
+ Collection<IoSession> ioSessions = daemonAcceptor.getManagedSessions().values();
+ CountDownLatch allSessionsClosed = new CountDownLatch(ioSessions.size());
+ for (IoSession io : ioSessions) {
+ logger.atFine().log("Waiting for session %s to stop.", io.getId());
+ io.addCloseFutureListener(
+ new SshFutureListener<CloseFuture>() {
+ @Override
+ public void operationComplete(CloseFuture future) {
+ allSessionsClosed.countDown();
+ }
+ });
+ }
+ try {
+ if (!allSessionsClosed.await(gracefulStopTimeout, TimeUnit.SECONDS)) {
+ logger.atWarning().log(
+ "Timeout waiting for SSH session to close. SSHD will be shut down immediately.");
+ }
+ } catch (InterruptedException e) {
+ logger.atWarning().withCause(e).log(
+ "Interrupted waiting for SSH-sessions to close. SSHD will be shut down immediately.");
+ }
+ }
+
private void shutdownExecutors() {
if (executor != null) {
executor.shutdownNow();
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..a6301f9 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,10 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
+import java.time.LocalDateTime;
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 +58,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 +76,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 +93,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 +110,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 +122,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 +137,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 +179,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();
+ 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 +204,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 +235,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 +252,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 +268,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 +285,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 +300,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 +309,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 +330,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 +344,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 +358,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 +373,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 +392,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 +406,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 +420,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 +437,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 +452,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 +469,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 +483,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 +497,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 +514,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 +538,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -625,10 +569,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -654,10 +595,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 +630,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -735,10 +670,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -778,10 +710,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -810,10 +739,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(3)
@@ -844,10 +770,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(2)
@@ -878,10 +801,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -912,10 +832,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -944,10 +861,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(3)
@@ -977,10 +891,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1009,10 +920,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -1049,10 +957,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1076,10 +981,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 +1010,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 +1031,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 +1047,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 +1071,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 +1101,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 +1128,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 +1151,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 +1172,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 +1193,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 +1214,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 +1230,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 +1253,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 +1269,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 +1294,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 +1312,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 +1335,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 +1351,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 +1372,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 +1409,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 +1451,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 +1493,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 +1541,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 +1552,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/elasticsearch/ElasticContainer.java b/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java
index 60bf64c..1a43269 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java
@@ -41,7 +41,7 @@
case V6_7:
return "blacktop/elasticsearch:6.7.2";
case V6_8:
- return "blacktop/elasticsearch:6.8.11";
+ return "blacktop/elasticsearch:6.8.12";
case V7_0:
return "blacktop/elasticsearch:7.0.1";
case V7_1:
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticVersionTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticVersionTest.java
index 9296a6b..b9d5313 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticVersionTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticVersionTest.java
@@ -72,15 +72,15 @@
public void atLeastMinorVersion() throws Exception {
assertThat(ElasticVersion.V6_7.isAtLeastMinorVersion(ElasticVersion.V6_7)).isTrue();
assertThat(ElasticVersion.V6_8.isAtLeastMinorVersion(ElasticVersion.V6_8)).isTrue();
- assertThat(ElasticVersion.V7_0.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_1.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_2.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_3.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_4.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_5.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_6.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_7.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
- assertThat(ElasticVersion.V7_8.isAtLeastMinorVersion(ElasticVersion.V6_7)).isFalse();
+ assertThat(ElasticVersion.V7_0.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_1.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_2.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_3.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_4.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_5.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_6.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_7.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
+ assertThat(ElasticVersion.V7_8.isAtLeastMinorVersion(ElasticVersion.V6_8)).isFalse();
}
@Test
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/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
deleted file mode 100644
index 9489b94..0000000
--- a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
+++ /dev/null
@@ -1,211 +0,0 @@
-/**
- * @license
- * Copyright (C) 2016 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 '../../shared/gr-dialog/gr-dialog.js';
-import '../../../styles/shared-styles.js';
-import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.js';
-import '../../shared/gr-js-api-interface/gr-js-api-interface.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-confirm-revert-dialog_html.js';
-
-const ERR_COMMIT_NOT_FOUND =
- 'Unable to find the commit hash of this change.';
-const CHANGE_SUBJECT_LIMIT = 50;
-
-// TODO(dhruvsri): clean up repeated definitions after moving to js modules
-const REVERT_TYPES = {
- REVERT_SINGLE_CHANGE: 1,
- REVERT_SUBMISSION: 2,
-};
-
-/**
- * @extends PolymerElement
- */
-class GrConfirmRevertDialog extends GestureEventListeners(
- LegacyElementMixin(PolymerElement)) {
- static get template() { return htmlTemplate; }
-
- static get is() { return 'gr-confirm-revert-dialog'; }
- /**
- * Fired when the confirm button is pressed.
- *
- * @event confirm
- */
-
- /**
- * Fired when the cancel button is pressed.
- *
- * @event cancel
- */
-
- static get properties() {
- return {
- /* The revert message updated by the user
- The default value is set by the dialog */
- _message: String,
- _revertType: {
- type: Number,
- value: REVERT_TYPES.REVERT_SINGLE_CHANGE,
- },
- _showRevertSubmission: {
- type: Boolean,
- value: false,
- },
- _changesCount: Number,
- _showErrorMessage: {
- type: Boolean,
- value: false,
- },
- /* store the default revert messages per revert type so that we can
- check if user has edited the revert message or not
- Set when populate() is called */
- _originalRevertMessages: {
- type: Array,
- value() { return []; },
- },
- // Store the actual messages that the user has edited
- _revertMessages: {
- type: Array,
- value() { return []; },
- },
- };
- }
-
- _computeIfSingleRevert(revertType) {
- return revertType === REVERT_TYPES.REVERT_SINGLE_CHANGE;
- }
-
- _computeIfRevertSubmission(revertType) {
- return revertType === REVERT_TYPES.REVERT_SUBMISSION;
- }
-
- _modifyRevertMsg(change, commitMessage, message) {
- return this.$.jsAPI.modifyRevertMsg(change,
- message, commitMessage);
- }
-
- populate(change, commitMessage, changes) {
- this._changesCount = changes.length;
- // The option to revert a single change is always available
- this._populateRevertSingleChangeMessage(
- change, commitMessage, change.current_revision);
- this._populateRevertSubmissionMessage(change, changes, commitMessage);
- }
-
- _populateRevertSingleChangeMessage(change, commitMessage, commitHash) {
- // Figure out what the revert title should be.
- const originalTitle = (commitMessage || '').split('\n')[0];
- const revertTitle = `Revert "${originalTitle}"`;
- if (!commitHash) {
- this.dispatchEvent(new CustomEvent('show-alert', {
- detail: {message: ERR_COMMIT_NOT_FOUND},
- composed: true, bubbles: true,
- }));
- return;
- }
- const revertCommitText = `This reverts commit ${commitHash}.`;
-
- this._message = `${revertTitle}\n\n${revertCommitText}\n\n` +
- `Reason for revert: <INSERT REASONING HERE>\n`;
- // This is to give plugins a chance to update message
- this._message = this._modifyRevertMsg(change, commitMessage,
- this._message);
- this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
- this._showRevertSubmission = false;
- this._revertMessages[this._revertType] = this._message;
- this._originalRevertMessages[this._revertType] = this._message;
- }
-
- _getTrimmedChangeSubject(subject) {
- if (!subject) return '';
- if (subject.length < CHANGE_SUBJECT_LIMIT) return subject;
- return subject.substring(0, CHANGE_SUBJECT_LIMIT) + '...';
- }
-
- _modifyRevertSubmissionMsg(change, msg, commitMessage) {
- return this.$.jsAPI.modifyRevertSubmissionMsg(change, msg,
- commitMessage);
- }
-
- _populateRevertSubmissionMessage(change, changes, commitMessage) {
- // Follow the same convention of the revert
- const commitHash = change.current_revision;
- if (!commitHash) {
- this.dispatchEvent(new CustomEvent('show-alert', {
- detail: {message: ERR_COMMIT_NOT_FOUND},
- composed: true, bubbles: true,
- }));
- return;
- }
- if (!changes || changes.length <= 1) return;
- const submissionId = change.submission_id;
- const revertTitle = 'Revert submission ' + submissionId;
- this._message = revertTitle + '\n\n' + 'Reason for revert: <INSERT ' +
- 'REASONING HERE>\n';
- this._message += 'Reverted Changes:\n';
- changes.forEach(change => {
- this._message += change.change_id.substring(0, 10) + ':'
- + this._getTrimmedChangeSubject(change.subject) + '\n';
- });
- this._message = this._modifyRevertSubmissionMsg(change, this._message,
- commitMessage);
- this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
- this._revertMessages[this._revertType] = this._message;
- this._originalRevertMessages[this._revertType] = this._message;
- this._showRevertSubmission = true;
- }
-
- _handleRevertSingleChangeClicked() {
- this._showErrorMessage = false;
- this._revertMessages[REVERT_TYPES.REVERT_SUBMISSION] = this._message;
- this._message = this._revertMessages[REVERT_TYPES.REVERT_SINGLE_CHANGE];
- this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
- }
-
- _handleRevertSubmissionClicked() {
- this._showErrorMessage = false;
- this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
- this._revertMessages[REVERT_TYPES.REVERT_SINGLE_CHANGE] = this._message;
- this._message = this._revertMessages[REVERT_TYPES.REVERT_SUBMISSION];
- }
-
- _handleConfirmTap(e) {
- e.preventDefault();
- e.stopPropagation();
- if (this._message === this._originalRevertMessages[this._revertType]) {
- this._showErrorMessage = true;
- return;
- }
- this.dispatchEvent(new CustomEvent('confirm', {
- detail: {revertType: this._revertType,
- message: this._message},
- composed: true, bubbles: false,
- }));
- }
-
- _handleCancelTap(e) {
- e.preventDefault();
- e.stopPropagation();
- this.dispatchEvent(new CustomEvent('cancel', {
- detail: {revertType: this._revertType},
- composed: true, bubbles: false,
- }));
- }
-}
-
-customElements.define(GrConfirmRevertDialog.is, GrConfirmRevertDialog);
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.ts
new file mode 100644
index 0000000..beaf0f8
--- /dev/null
+++ b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.ts
@@ -0,0 +1,248 @@
+/**
+ * @license
+ * Copyright (C) 2016 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 '../../shared/gr-dialog/gr-dialog';
+import '../../../styles/shared-styles';
+import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
+import '../../shared/gr-js-api-interface/gr-js-api-interface';
+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-confirm-revert-dialog_html';
+import {customElement, property} from '@polymer/decorators';
+import {JsApiService} from '../../shared/gr-js-api-interface/gr-js-api-types';
+import {ChangeInfo, CommitId} from '../../../types/common';
+
+const ERR_COMMIT_NOT_FOUND = 'Unable to find the commit hash of this change.';
+const CHANGE_SUBJECT_LIMIT = 50;
+
+// TODO(dhruvsri): clean up repeated definitions after moving to js modules
+const REVERT_TYPES = {
+ REVERT_SINGLE_CHANGE: 1,
+ REVERT_SUBMISSION: 2,
+};
+
+export interface GrConfirmRevertDialog {
+ $: {
+ jsAPI: JsApiService & Element;
+ };
+}
+@customElement('gr-confirm-revert-dialog')
+export class GrConfirmRevertDialog extends GestureEventListeners(
+ LegacyElementMixin(PolymerElement)
+) {
+ static get template() {
+ return htmlTemplate;
+ }
+
+ /**
+ * Fired when the confirm button is pressed.
+ *
+ * @event confirm
+ */
+
+ /**
+ * Fired when the cancel button is pressed.
+ *
+ * @event cancel
+ */
+
+ /* The revert message updated by the user
+ The default value is set by the dialog */
+ @property({type: String})
+ _message?: string;
+
+ @property({type: Number})
+ _revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
+
+ @property({type: Boolean})
+ _showRevertSubmission = false;
+
+ @property({type: Number})
+ _changesCount?: number;
+
+ @property({type: Boolean})
+ _showErrorMessage = false;
+
+ /* store the default revert messages per revert type so that we can
+ check if user has edited the revert message or not
+ Set when populate() is called */
+ @property({type: Array})
+ _originalRevertMessages: string[] = [];
+
+ // Store the actual messages that the user has edited
+ @property({type: Array})
+ _revertMessages: string[] = [];
+
+ _computeIfSingleRevert(revertType: number) {
+ return revertType === REVERT_TYPES.REVERT_SINGLE_CHANGE;
+ }
+
+ _computeIfRevertSubmission(revertType: number) {
+ return revertType === REVERT_TYPES.REVERT_SUBMISSION;
+ }
+
+ _modifyRevertMsg(change: ChangeInfo, commitMessage: string, message: string) {
+ return this.$.jsAPI.modifyRevertMsg(change, message, commitMessage);
+ }
+
+ populate(change: ChangeInfo, commitMessage: string, changes: ChangeInfo[]) {
+ this._changesCount = changes.length;
+ // The option to revert a single change is always available
+ this._populateRevertSingleChangeMessage(
+ change,
+ commitMessage,
+ change.current_revision
+ );
+ this._populateRevertSubmissionMessage(change, changes, commitMessage);
+ }
+
+ _populateRevertSingleChangeMessage(
+ change: ChangeInfo,
+ commitMessage: string,
+ commitHash?: CommitId
+ ) {
+ // Figure out what the revert title should be.
+ const originalTitle = (commitMessage || '').split('\n')[0];
+ const revertTitle = `Revert "${originalTitle}"`;
+ if (!commitHash) {
+ this.dispatchEvent(
+ new CustomEvent('show-alert', {
+ detail: {message: ERR_COMMIT_NOT_FOUND},
+ composed: true,
+ bubbles: true,
+ })
+ );
+ return;
+ }
+ const revertCommitText = `This reverts commit ${commitHash}.`;
+
+ const message =
+ `${revertTitle}\n\n${revertCommitText}\n\n` +
+ 'Reason for revert: <INSERT REASONING HERE>\n';
+ // This is to give plugins a chance to update message
+ this._message = this._modifyRevertMsg(change, commitMessage, message);
+ this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
+ this._showRevertSubmission = false;
+ this._revertMessages[this._revertType] = this._message;
+ this._originalRevertMessages[this._revertType] = this._message;
+ }
+
+ _getTrimmedChangeSubject(subject: string) {
+ if (!subject) return '';
+ if (subject.length < CHANGE_SUBJECT_LIMIT) return subject;
+ return subject.substring(0, CHANGE_SUBJECT_LIMIT) + '...';
+ }
+
+ _modifyRevertSubmissionMsg(
+ change: ChangeInfo,
+ msg: string,
+ commitMessage: string
+ ) {
+ return this.$.jsAPI.modifyRevertSubmissionMsg(change, msg, commitMessage);
+ }
+
+ _populateRevertSubmissionMessage(
+ change: ChangeInfo,
+ changes: ChangeInfo[],
+ commitMessage: string
+ ) {
+ // Follow the same convention of the revert
+ const commitHash = change.current_revision;
+ if (!commitHash) {
+ this.dispatchEvent(
+ new CustomEvent('show-alert', {
+ detail: {message: ERR_COMMIT_NOT_FOUND},
+ composed: true,
+ bubbles: true,
+ })
+ );
+ return;
+ }
+ if (!changes || changes.length <= 1) return;
+ const revertTitle = `Revert submission ${change.submission_id}`;
+ let message =
+ revertTitle +
+ '\n\n' +
+ 'Reason for revert: <INSERT ' +
+ 'REASONING HERE>\n';
+ message += 'Reverted Changes:\n';
+ changes.forEach(change => {
+ message +=
+ `${change.change_id.substring(0, 10)}:` +
+ `${this._getTrimmedChangeSubject(change.subject)}\n`;
+ });
+ this._message = this._modifyRevertSubmissionMsg(
+ change,
+ message,
+ commitMessage
+ );
+ this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
+ this._revertMessages[this._revertType] = this._message;
+ this._originalRevertMessages[this._revertType] = this._message;
+ this._showRevertSubmission = true;
+ }
+
+ _handleRevertSingleChangeClicked() {
+ this._showErrorMessage = false;
+ if (this._message)
+ this._revertMessages[REVERT_TYPES.REVERT_SUBMISSION] = this._message;
+ this._message = this._revertMessages[REVERT_TYPES.REVERT_SINGLE_CHANGE];
+ this._revertType = REVERT_TYPES.REVERT_SINGLE_CHANGE;
+ }
+
+ _handleRevertSubmissionClicked() {
+ this._showErrorMessage = false;
+ this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
+ if (this._message)
+ this._revertMessages[REVERT_TYPES.REVERT_SINGLE_CHANGE] = this._message;
+ this._message = this._revertMessages[REVERT_TYPES.REVERT_SUBMISSION];
+ }
+
+ _handleConfirmTap(e: MouseEvent) {
+ e.preventDefault();
+ e.stopPropagation();
+ if (this._message === this._originalRevertMessages[this._revertType]) {
+ this._showErrorMessage = true;
+ return;
+ }
+ this.dispatchEvent(
+ new CustomEvent('confirm', {
+ detail: {revertType: this._revertType, message: this._message},
+ composed: true,
+ bubbles: false,
+ })
+ );
+ }
+
+ _handleCancelTap(e: MouseEvent) {
+ e.preventDefault();
+ e.stopPropagation();
+ this.dispatchEvent(
+ new CustomEvent('cancel', {
+ detail: {revertType: this._revertType},
+ composed: true,
+ bubbles: false,
+ })
+ );
+ }
+}
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-confirm-revert-dialog': GrConfirmRevertDialog;
+ }
+}
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.js b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.js
deleted file mode 100644
index 42afcc2..0000000
--- a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.js
+++ /dev/null
@@ -1,92 +0,0 @@
-/**
- * @license
- * Copyright (C) 2018 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 '@polymer/iron-icon/iron-icon.js';
-import '../../shared/gr-icons/gr-icons.js';
-import '../../shared/gr-dialog/gr-dialog.js';
-import '../../shared/gr-rest-api-interface/gr-rest-api-interface.js';
-import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.js';
-import '../../plugins/gr-endpoint-param/gr-endpoint-param.js';
-import '../../../styles/shared-styles.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-confirm-submit-dialog_html.js';
-
-/** @extends PolymerElement */
-class GrConfirmSubmitDialog extends GestureEventListeners(
- LegacyElementMixin(
- PolymerElement)) {
- static get template() { return htmlTemplate; }
-
- static get is() { return 'gr-confirm-submit-dialog'; }
- /**
- * Fired when the confirm button is pressed.
- *
- * @event confirm
- */
-
- /**
- * Fired when the cancel button is pressed.
- *
- * @event cancel
- */
-
- static get properties() {
- return {
- /**
- * @type {Gerrit.Change}
- */
- change: Object,
-
- /**
- * @type {{
- * label: string,
- * }}
- */
- action: Object,
- };
- }
-
- resetFocus(e) {
- this.$.dialog.resetFocus();
- }
-
- _computeHasChangeEdit(change) {
- return !!change.revisions &&
- Object.values(change.revisions).some(rev => rev._number == 'edit');
- }
-
- _computeUnresolvedCommentsWarning(change) {
- const unresolvedCount = change.unresolved_comment_count;
- const plural = unresolvedCount > 1 ? 's' : '';
- return `Heads Up! ${unresolvedCount} unresolved comment${plural}.`;
- }
-
- _handleConfirmTap(e) {
- e.preventDefault();
- e.stopPropagation();
- this.dispatchEvent(new CustomEvent('confirm', {bubbles: false}));
- }
-
- _handleCancelTap(e) {
- e.preventDefault();
- e.stopPropagation();
- this.dispatchEvent(new CustomEvent('cancel', {bubbles: false}));
- }
-}
-
-customElements.define(GrConfirmSubmitDialog.is, GrConfirmSubmitDialog);
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
new file mode 100644
index 0000000..666f95d
--- /dev/null
+++ b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog.ts
@@ -0,0 +1,98 @@
+/**
+ * @license
+ * Copyright (C) 2018 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 '@polymer/iron-icon/iron-icon';
+import '../../shared/gr-icons/gr-icons';
+import '../../shared/gr-dialog/gr-dialog';
+import '../../shared/gr-rest-api-interface/gr-rest-api-interface';
+import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
+import '../../plugins/gr-endpoint-param/gr-endpoint-param';
+import '../../../styles/shared-styles';
+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-confirm-submit-dialog_html';
+import {customElement, property} from '@polymer/decorators';
+import {ChangeInfo, ActionInfo} from '../../../types/common';
+import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
+
+export interface GrConfirmSubmitDialog {
+ $: {
+ dialog: GrDialog;
+ };
+}
+@customElement('gr-confirm-submit-dialog')
+export class GrConfirmSubmitDialog extends GestureEventListeners(
+ LegacyElementMixin(PolymerElement)
+) {
+ static get template() {
+ return htmlTemplate;
+ }
+
+ /**
+ * Fired when the confirm button is pressed.
+ *
+ * @event confirm
+ */
+
+ /**
+ * Fired when the cancel button is pressed.
+ *
+ * @event cancel
+ */
+
+ @property({type: Object})
+ change?: ChangeInfo;
+
+ @property({type: Object})
+ action?: ActionInfo;
+
+ resetFocus() {
+ this.$.dialog.resetFocus();
+ }
+
+ _computeHasChangeEdit(change?: ChangeInfo) {
+ return (
+ !!change &&
+ !!change.revisions &&
+ Object.values(change.revisions).some(rev => rev._number === 'edit')
+ );
+ }
+
+ _computeUnresolvedCommentsWarning(change: ChangeInfo) {
+ const unresolvedCount = change.unresolved_comment_count;
+ const plural = unresolvedCount && unresolvedCount > 1 ? 's' : '';
+ return `Heads Up! ${unresolvedCount} unresolved comment${plural}.`;
+ }
+
+ _handleConfirmTap(e: MouseEvent) {
+ e.preventDefault();
+ e.stopPropagation();
+ this.dispatchEvent(new CustomEvent('confirm', {bubbles: false}));
+ }
+
+ _handleCancelTap(e: MouseEvent) {
+ e.preventDefault();
+ e.stopPropagation();
+ this.dispatchEvent(new CustomEvent('cancel', {bubbles: false}));
+ }
+}
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-confirm-submit-dialog': GrConfirmSubmitDialog;
+ }
+}
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
index 6ecdabb..630681c 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
@@ -540,7 +540,7 @@
if (reviewer.account) {
reviewerId = reviewer.account._account_id || reviewer.account.email;
} else if (reviewer.group) {
- reviewerId = reviewer.group.id;
+ reviewerId = decodeURIComponent(reviewer.group.id);
confirmed = reviewer.group.confirmed;
}
return {reviewer: reviewerId, confirmed};
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 6f1bc60..aa2e4ce 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -72,6 +72,7 @@
DASHBOARD: /^\/dashboard\/(.+)$/,
CUSTOM_DASHBOARD: /^\/dashboard\/?$/,
PROJECT_DASHBOARD: /^\/p\/(.+)\/\+\/dashboard\/(.+)/,
+ LEGACY_PROJECT_DASHBOARD: /^\/projects\/(.+),dashboards\/(.+)/,
AGREEMENTS: /^\/settings\/agreements\/?/,
NEW_AGREEMENTS: /^\/settings\/new-agreement\/?/,
@@ -880,6 +881,11 @@
'_handleProjectDashboardRoute'
);
+ this._mapRoute(
+ RoutePattern.LEGACY_PROJECT_DASHBOARD,
+ '_handleLegacyProjectDashboardRoute'
+ );
+
this._mapRoute(RoutePattern.GROUP_INFO, '_handleGroupInfoRoute', true);
this._mapRoute(
@@ -1255,6 +1261,10 @@
this.reporting.setRepoName(project);
}
+ _handleLegacyProjectDashboardRoute(data: PageContextWithQueryMap) {
+ this._redirect('/p/' + data.params[0] + '/+/dashboard/' + data.params[1]);
+ }
+
_handleGroupInfoRoute(data: PageContextWithQueryMap) {
this._redirect('/admin/groups/' + encodeURIComponent(data.params[0]));
}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js
index 5ee58c9..927434b 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js
@@ -197,6 +197,7 @@
'_handleImproperlyEncodedPlusRoute',
'_handlePassThroughRoute',
'_handleProjectDashboardRoute',
+ '_handleLegacyProjectDashboardRoute',
'_handleProjectsOldRoute',
'_handleRepoAccessRoute',
'_handleRepoDashboardsRoute',
@@ -617,6 +618,14 @@
handlePassThroughRoute = sinon.stub(element, '_handlePassThroughRoute');
});
+ test('_handleLegacyProjectDashboardRoute', () => {
+ const params = {0: 'gerrit/project', 1: 'dashboard:main'};
+ element._handleLegacyProjectDashboardRoute({params});
+ assert.isTrue(redirectStub.calledOnce);
+ assert.equal(redirectStub.lastCall.args[0],
+ '/p/gerrit/project/+/dashboard/dashboard:main');
+ });
+
test('_handleAgreementsRoute', () => {
const data = {params: {}};
element._handleAgreementsRoute(data);
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
index 7455cad..0a5dbc9 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
@@ -39,5 +39,10 @@
origMsg: string
): string;
handleEvent(eventName: EventType, detail: any): void;
+ modifyRevertMsg(
+ change: ChangeInfo,
+ revertMsg: string,
+ origMsg: string
+ ): string;
// TODO(TS): Add more methods when needed for the TS conversion.
}
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(', ');
}
