From 51ec1bd0939f90eaddaa999e9c4eae0cca5c28cf Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 21 Oct 2015 18:22:03 +0200 Subject: [PATCH] Notedb: Fix loading of draft comment refs The draft comment refs are stored in the metadata repository, but the code tried to load them from the original repository. As result the loaded ref set was always empty (except when gerrit.noteDbPath was set to the same path as gerrit.basePath). Once draft comment refs were found the parsing of the Change ID's from the refs failed with a NumberFormatException. The new code scans only for draft comment refs of the author. This is more efficient than scanning all draft comment refs and then filtering out all draft comment refs that were not for the author. Due to these bugs when Notedb was enabled, the 'Reply' button on the ChangeScreen was not highlighted when there were draft comments on the change. Change-Id: I4e0906cc838cfad02aa43ab5fb8e46d0efe189c2 Signed-off-by: Edwin Kempin --- .../acceptance/server/change/CommentsIT.java | 14 ++++++++++++++ .../google/gerrit/reviewdb/client/RefNames.java | 14 ++++++++++++-- .../gerrit/reviewdb/client/RefNamesTest.java | 6 ++++++ .../gerrit/server/PatchLineCommentsUtil.java | 10 +++------- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java index 5592755dc6..db6e764c52 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -265,6 +265,20 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(c2.line).isEqualTo(1); } + @Test + public void listChangeWithDrafts() throws Exception { + for (Integer line : lines) { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraft( + "file1", Side.REVISION, line, "comment 1"); + addDraft(changeId, revId, comment); + assertThat(gApi.changes().query( + "change:" + changeId + " has:draft").get()).hasSize(1); + } + } + @Test public void publishCommentsAllRevisions() throws Exception { PushOneCommit.Result r1 = createChange(); diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java index 5d2a1fd026..2dc556ed9e 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java @@ -88,6 +88,17 @@ public class RefNames { public static String refsDraftComments(Account.Id accountId, Change.Id changeId) { + StringBuilder r = buildRefsDraftCommentsPrefix(accountId); + r.append(changeId.get()); + return r.toString(); + } + + public static String refsDraftCommentsPrefix(Account.Id accountId) { + return buildRefsDraftCommentsPrefix(accountId).toString(); + } + + public static StringBuilder buildRefsDraftCommentsPrefix( + Account.Id accountId) { StringBuilder r = new StringBuilder(); r.append(REFS_DRAFT_COMMENTS); int n = accountId.get() % 100; @@ -98,8 +109,7 @@ public class RefNames { r.append('/'); r.append(accountId.get()); r.append('-'); - r.append(changeId.get()); - return r.toString(); + return r; } /** diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java index b0981a73f6..baae696f5e 100644 --- a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java +++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java @@ -42,6 +42,12 @@ public class RefNamesTest { .isEqualTo("refs/draft-comments/23/1011123-67473"); } + @Test + public void refsDraftCommentsPrefix() throws Exception { + assertThat(RefNames.refsDraftCommentsPrefix(accountId)) + .isEqualTo("refs/draft-comments/23/1011123-"); + } + @Test public void refsEdit() throws Exception { assertThat(RefNames.refsEdit(accountId, changeId, psId)) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java index 7b182b1c49..3e67b277d8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java @@ -275,14 +275,10 @@ public class PatchLineCommentsUtil { return sort(db.patchComments().draftByAuthor(author).toList()); } - // TODO(dborowitz): Just scan author space. - Set refNames = getRefNamesAllUsers(RefNames.REFS_DRAFT_COMMENTS); + Set refNames = + getRefNamesAllUsers(RefNames.refsDraftCommentsPrefix(author)); List comments = Lists.newArrayList(); for (String refName : refNames) { - Account.Id id = Account.Id.fromRefPart(refName); - if (!author.equals(id)) { - continue; - } Change.Id changeId = Change.Id.parse(refName); comments.addAll( draftFactory.create(changeId, author).load().getComments().values()); @@ -364,7 +360,7 @@ public class PatchLineCommentsUtil { } private Set getRefNamesAllUsers(String prefix) throws OrmException { - try (Repository repo = repoManager.openRepository(allUsers)) { + try (Repository repo = repoManager.openMetadataRepository(allUsers)) { RefDatabase refDb = repo.getRefDatabase(); return refDb.getRefs(prefix).keySet(); } catch (IOException e) {