diff --git a/WORKSPACE b/WORKSPACE index 83f5b2a95d..1f3a2073bc 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -83,8 +83,11 @@ closure_repositories( # Golang support for PolyGerrit local dev server. http_archive( name = "io_bazel_rules_go", - sha256 = "6776d68ebb897625dead17ae510eac3d5f6342367327875210df44dbe2aeeb19", - url = "https://github.com/bazelbuild/rules_go/releases/download/0.17.1/rules_go-0.17.1.tar.gz", + sha256 = "f04d2373bcaf8aa09bccb08a98a57e721306c8f6043a2a0ee610fd6853dcde3d", + urls = [ + "https://storage.googleapis.com/bazel-mirror/github.com/bazelbuild/rules_go/releases/download/0.18.6/rules_go-0.18.6.tar.gz", + "https://github.com/bazelbuild/rules_go/releases/download/0.18.6/rules_go-0.18.6.tar.gz", + ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 47ccb49ed0..09d8068108 100644 --- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -326,6 +326,15 @@ public interface ChangeApi { */ Map> comments() throws RestApiException; + /** + * Get all published comments on a change as a list. + * + * @return comments as a list; comments have the {@code revision} field set to indicate their + * patch set. + * @throws RestApiException + */ + List commentsAsList() throws RestApiException; + /** * Get all robot comments on a change. * @@ -344,6 +353,15 @@ public interface ChangeApi { */ Map> drafts() throws RestApiException; + /** + * Get all draft comments for the current user on a change as a list. + * + * @return drafts as a list; comments have the {@code revision} field set to indicate their patch + * set. + * @throws RestApiException + */ + List draftsAsList() throws RestApiException; + ChangeInfo check() throws RestApiException; ChangeInfo check(FixInput fix) throws RestApiException; @@ -551,6 +569,11 @@ public interface ChangeApi { throw new NotImplementedException(); } + @Override + public List commentsAsList() throws RestApiException { + throw new NotImplementedException(); + } + @Override public Map> robotComments() throws RestApiException { throw new NotImplementedException(); @@ -561,6 +584,11 @@ public interface ChangeApi { throw new NotImplementedException(); } + @Override + public List draftsAsList() throws RestApiException { + throw new NotImplementedException(); + } + @Override public ChangeInfo check() throws RestApiException { throw new NotImplementedException(); diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index f6887182ef..f027c92b3a 100644 --- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -556,6 +556,15 @@ class ChangeApiImpl implements ChangeApi { } } + @Override + public List commentsAsList() throws RestApiException { + try { + return listComments.getComments(change); + } catch (Exception e) { + throw asRestApiException("Cannot get comments", e); + } + } + @Override public Map> robotComments() throws RestApiException { try { @@ -574,6 +583,15 @@ class ChangeApiImpl implements ChangeApi { } } + @Override + public List draftsAsList() throws RestApiException { + try { + return listDrafts.getComments(change); + } catch (Exception e) { + throw asRestApiException("Cannot get drafts", e); + } + } + @Override public ChangeInfo check() throws RestApiException { try { diff --git a/java/com/google/gerrit/server/auth/AuthRequest.java b/java/com/google/gerrit/server/auth/AuthRequest.java index c6222f8cb7..d155f894d6 100644 --- a/java/com/google/gerrit/server/auth/AuthRequest.java +++ b/java/com/google/gerrit/server/auth/AuthRequest.java @@ -31,7 +31,7 @@ public abstract class AuthRequest { /** * Returns the username to be authenticated. * - * @return username for authentication or null for anonymous access. + * @return username for authentication or {@code empty} for anonymous access. */ public final Optional getUsername() { return username; @@ -40,7 +40,7 @@ public abstract class AuthRequest { /** * Returns the user's credentials * - * @return user's credentials or null + * @return user's credentials or {@code empty}. */ public final Optional getPassword() { return password; diff --git a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java index 992f602851..403922db99 100644 --- a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java +++ b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java @@ -14,44 +14,37 @@ package com.google.gerrit.server.restapi.change; -import com.google.gerrit.extensions.common.CommentInfo; -import com.google.gerrit.extensions.restapi.AuthException; -import com.google.gerrit.extensions.restapi.RestReadView; +import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.change.ChangeResource; -import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; -import java.util.List; -import java.util.Map; @Singleton -public class ListChangeComments implements RestReadView { - private final ChangeData.Factory changeDataFactory; - private final Provider commentJson; - private final CommentsUtil commentsUtil; - +public class ListChangeComments extends ListChangeDrafts { @Inject ListChangeComments( ChangeData.Factory changeDataFactory, Provider commentJson, CommentsUtil commentsUtil) { - this.changeDataFactory = changeDataFactory; - this.commentJson = commentJson; - this.commentsUtil = commentsUtil; + super(changeDataFactory, commentJson, commentsUtil); } @Override - public Map> apply(ChangeResource rsrc) - throws AuthException, PermissionBackendException { + protected Iterable listComments(ChangeResource rsrc) { ChangeData cd = changeDataFactory.create(rsrc.getNotes()); - return commentJson - .get() - .setFillAccounts(true) - .setFillPatchSet(true) - .newCommentFormatter() - .format(commentsUtil.publishedByChange(cd.notes())); + return commentsUtil.publishedByChange(cd.notes()); + } + + @Override + protected boolean includeAuthorInfo() { + return true; + } + + @Override + public boolean requireAuthentication() { + return false; } } diff --git a/java/com/google/gerrit/server/restapi/change/ListChangeDrafts.java b/java/com/google/gerrit/server/restapi/change/ListChangeDrafts.java index 1939385c3a..f8fda731c9 100644 --- a/java/com/google/gerrit/server/restapi/change/ListChangeDrafts.java +++ b/java/com/google/gerrit/server/restapi/change/ListChangeDrafts.java @@ -22,6 +22,7 @@ import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.restapi.change.CommentJson.CommentFormatter; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -30,9 +31,9 @@ import java.util.Map; @Singleton public class ListChangeDrafts implements RestReadView { - private final ChangeData.Factory changeDataFactory; - private final Provider commentJson; - private final CommentsUtil commentsUtil; + protected final ChangeData.Factory changeDataFactory; + protected final Provider commentJson; + protected final CommentsUtil commentsUtil; @Inject ListChangeDrafts( @@ -44,20 +45,41 @@ public class ListChangeDrafts implements RestReadView { this.commentsUtil = commentsUtil; } + protected Iterable listComments(ChangeResource rsrc) { + ChangeData cd = changeDataFactory.create(rsrc.getNotes()); + return commentsUtil.draftByChangeAuthor(cd.notes(), rsrc.getUser().getAccountId()); + } + + protected boolean includeAuthorInfo() { + return false; + } + + public boolean requireAuthentication() { + return true; + } + @Override public Map> apply(ChangeResource rsrc) throws AuthException, PermissionBackendException { - if (!rsrc.getUser().isIdentifiedUser()) { + if (requireAuthentication() && !rsrc.getUser().isIdentifiedUser()) { throw new AuthException("Authentication required"); } - ChangeData cd = changeDataFactory.create(rsrc.getNotes()); - List drafts = - commentsUtil.draftByChangeAuthor(cd.notes(), rsrc.getUser().getAccountId()); + return getCommentFormatter().format(listComments(rsrc)); + } + + public List getComments(ChangeResource rsrc) + throws AuthException, PermissionBackendException { + if (requireAuthentication() && !rsrc.getUser().isIdentifiedUser()) { + throw new AuthException("Authentication required"); + } + return getCommentFormatter().formatAsList(listComments(rsrc)); + } + + private CommentFormatter getCommentFormatter() { return commentJson .get() - .setFillAccounts(false) + .setFillAccounts(includeAuthorInfo()) .setFillPatchSet(true) - .newCommentFormatter() - .format(drafts); + .newCommentFormatter(); } } diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index 7b449167db..bd76efa6c6 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -111,6 +111,11 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(result).hasSize(1); CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path)); assertThat(comment).isEqualTo(infoToDraft(path).apply(actual)); + + List list = getDraftCommentsAsList(changeId); + assertThat(list).hasSize(1); + actual = list.get(0); + assertThat(comment).isEqualTo(infoToDraft(path).apply(actual)); } } @@ -133,6 +138,10 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(result).hasSize(1); assertThat(Lists.transform(result.get(path), infoToDraft(path))) .containsExactly(c1, c2, c3, c4); + + List list = getDraftCommentsAsList(changeId); + assertThat(list).hasSize(4); + assertThat(Lists.transform(list, infoToDraft(path))).containsExactly(c1, c2, c3, c4); } } @@ -235,6 +244,9 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(result).isNotEmpty(); assertThat(Lists.transform(result.get(file), infoToInput(file))) .containsExactly(c1, c2, c3, c4); + + List list = getPublishedCommentsAsList(changeId); + assertThat(Lists.transform(list, infoToInput(file))).containsExactly(c1, c2, c3, c4); } // for the commit message comments on the auto-merge are not possible @@ -253,6 +265,9 @@ public class CommentsIT extends AbstractDaemonTest { Map> result = getPublishedComments(changeId, revId); assertThat(result).isNotEmpty(); assertThat(Lists.transform(result.get(file), infoToInput(file))).containsExactly(c1, c2, c3); + + List list = getPublishedCommentsAsList(changeId); + assertThat(Lists.transform(list, infoToInput(file))).containsExactly(c1, c2, c3); } } @@ -277,6 +292,7 @@ public class CommentsIT extends AbstractDaemonTest { String changeId = r.getChangeId(); String revId = r.getCommit().getName(); assertThat(getPublishedComments(changeId, revId)).isEmpty(); + assertThat(getPublishedCommentsAsList(changeId)).isEmpty(); List expectedComments = new ArrayList<>(); for (Integer line : lines) { @@ -293,6 +309,10 @@ public class CommentsIT extends AbstractDaemonTest { List actualComments = result.get(file); assertThat(Lists.transform(actualComments, infoToInput(file))) .containsExactlyElementsIn(expectedComments); + + List list = getPublishedCommentsAsList(changeId); + assertThat(Lists.transform(list, infoToInput(file))) + .containsExactlyElementsIn(expectedComments); } @Test @@ -1093,11 +1113,19 @@ public class CommentsIT extends AbstractDaemonTest { return gApi.changes().id(changeId).revision(revId).comments(); } + private List getPublishedCommentsAsList(String changeId) throws Exception { + return gApi.changes().id(changeId).commentsAsList(); + } + private Map> getDraftComments(String changeId, String revId) throws Exception { return gApi.changes().id(changeId).revision(revId).drafts(); } + private List getDraftCommentsAsList(String changeId) throws Exception { + return gApi.changes().id(changeId).draftsAsList(); + } + private CommentInfo getDraftComment(String changeId, String revId, String uuid) throws Exception { return gApi.changes().id(changeId).revision(revId).draft(uuid).get(); } diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js index f53369ae03..a66c38d8de 100644 --- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js +++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js @@ -158,6 +158,9 @@ ], attached() { + // Polymer 2: anchor tag won't work on shadow DOM + // we need to manually calling scrollIntoView when hash changed + this.listen(window, 'location-change', '_handleLocationChange'); this.fire('title-change', {title: 'Settings'}); this._isDark = !!window.localStorage.getItem('dark-theme'); @@ -214,9 +217,28 @@ this._loadingPromise = Promise.all(promises).then(() => { this._loading = false; + + // Handle anchor tag for initial load + this._handleLocationChange(); }); }, + detached() { + this.unlisten(window, 'location-change', '_handleLocationChange'); + }, + + _handleLocationChange() { + // Handle anchor tag after dom attached + const urlHash = window.location.hash; + if (urlHash) { + // Use shadowRoot for Polymer 2 + const elem = (this.shadowRoot || document).querySelector(urlHash); + if (elem) { + elem.scrollIntoView(); + } + } + }, + reloadAccountDetail() { Promise.all([ this.$.accountInfo.loadData(),