From f38c1c5c1aba5789e9ce5d2d05ce2fe2ff03b2a5 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 30 Apr 2015 08:42:36 -0700 Subject: [PATCH] Fix PatchLineCommentsUtil.draftByChangeAuthor There is not a native index for this, and the ReviewDb case was not properly filtering a result by change. Add a test for this case. Change-Id: I5ca17bfbc820a6003770ffa1198fff2ce52366c3 --- .../gerrit/server/PatchLineCommentsUtil.java | 13 +++- .../gerrit/server/change/CommentsTest.java | 62 ++++++++++++++----- 2 files changed, 59 insertions(+), 16 deletions(-) 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 e412f9857a..d5242c2bd8 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 @@ -17,6 +17,7 @@ package com.google.gerrit.server; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.base.Predicate; +import com.google.common.collect.FluentIterable; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.gerrit.reviewdb.client.Account; @@ -222,7 +223,17 @@ public class PatchLineCommentsUtil { ChangeNotes notes, Account.Id author) throws OrmException { if (!migration.readChanges()) { - return sort(db.patchComments().byChange(notes.getChangeId()).toList()); + final Change.Id matchId = notes.getChangeId(); + return FluentIterable + .from(db.patchComments().draftByAuthor(author)) + .filter(new Predicate() { + @Override + public boolean apply(PatchLineComment in) { + Change.Id changeId = + in.getKey().getParentKey().getParentKey().getParentKey(); + return changeId.equals(matchId); + } + }).toSortedList(ChangeNotes.PLC_ORDER); } List comments = Lists.newArrayList(); comments.addAll(notes.getDraftBaseComments(author).values()); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java index e578b6d75c..8bedd176c1 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java @@ -113,13 +113,16 @@ public class CommentsTest { private Injector injector; private ReviewDb db; private Project.NameKey project; + private Account.Id ownerId; private RevisionResource revRes1; private RevisionResource revRes2; + private RevisionResource revRes3; private PatchLineComment plc1; private PatchLineComment plc2; private PatchLineComment plc3; private PatchLineComment plc4; private PatchLineComment plc5; + private PatchLineComment plc6; private IdentifiedUser changeOwner; @Inject private AllUsersNameProvider allUsers; @@ -156,7 +159,7 @@ public class CommentsTest { co.setFullName("Change Owner"); co.setPreferredEmail("change@owner.com"); accountCache.put(co); - final Account.Id ownerId = co.getId(); + ownerId = co.getId(); Account ou = new Account(new Account.Id(2), TimeUtil.nowTs()); ou.setFullName("Other Account"); @@ -223,12 +226,16 @@ public class CommentsTest { PatchLineCommentAccess plca = createMock(PatchLineCommentAccess.class); expect(db.patchComments()).andReturn(plca).anyTimes(); - Change change = newChange(); - PatchSet.Id psId1 = new PatchSet.Id(change.getId(), 1); + Change change1 = newChange(); + PatchSet.Id psId1 = new PatchSet.Id(change1.getId(), 1); PatchSet ps1 = new PatchSet(psId1); - PatchSet.Id psId2 = new PatchSet.Id(change.getId(), 2); + PatchSet.Id psId2 = new PatchSet.Id(change1.getId(), 2); PatchSet ps2 = new PatchSet(psId2); + Change change2 = newChange(); + PatchSet.Id psId3 = new PatchSet.Id(change2.getId(), 1); + PatchSet ps3 = new PatchSet(psId3); + long timeBase = TimeUtil.roundToSecond(TimeUtil.nowTs()).getTime(); plc1 = newPatchLineComment(psId1, "Comment1", null, "FileOne.txt", Side.REVISION, 3, ownerId, timeBase, @@ -250,21 +257,30 @@ public class CommentsTest { Side.REVISION, 5, ownerId, timeBase + 4000, "Third Comment", new CommentRange(3, 4, 5, 6), Status.DRAFT); plc5.setRevId(new RevId("BCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDE")); + plc5.setRevId(new RevId("BCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDE")); + plc6 = newPatchLineComment(psId3, "Comment6", null, "FileOne.txt", + Side.REVISION, 5, ownerId, timeBase + 5000, "Sixth Comment", + new CommentRange(3, 4, 5, 6), Status.DRAFT); + plc6.setRevId(new RevId("1234123412341234123412341234123412341234")); List commentsByOwner = Lists.newArrayList(); commentsByOwner.add(plc1); commentsByOwner.add(plc3); List commentsByReviewer = Lists.newArrayList(); commentsByReviewer.add(plc2); - List drafts = Lists.newArrayList(); - drafts.add(plc4); - drafts.add(plc5); + List drafts1 = Lists.newArrayList(); + drafts1.add(plc4); + drafts1.add(plc5); + List drafts2 = Lists.newArrayList(); + drafts2.add(plc6); plca.upsert(commentsByOwner); expectLastCall().anyTimes(); plca.upsert(commentsByReviewer); expectLastCall().anyTimes(); - plca.upsert(drafts); + plca.upsert(drafts1); + expectLastCall().anyTimes(); + plca.upsert(drafts2); expectLastCall().anyTimes(); expect(plca.publishedByPatchSet(psId1)) @@ -275,28 +291,36 @@ public class CommentsTest { .andAnswer(results()).anyTimes(); expect(plca.draftByPatchSetAuthor(psId2, ownerId)) .andAnswer(results(plc4, plc5)).anyTimes(); - expect(plca.byChange(change.getId())) + expect(plca.byChange(change1.getId())) .andAnswer(results(plc1, plc2, plc3, plc4, plc5)).anyTimes(); + expect(plca.draftByAuthor(ownerId)) + .andAnswer(results(plc4, plc5, plc6)).anyTimes(); replay(db, plca); - ChangeUpdate update = newUpdate(change, changeOwner); + ChangeUpdate update = newUpdate(change1, changeOwner); update.setPatchSetId(psId1); plcUtil.upsertComments(db, update, commentsByOwner); update.commit(); - update = newUpdate(change, otherUser); + update = newUpdate(change1, otherUser); update.setPatchSetId(psId1); plcUtil.upsertComments(db, update, commentsByReviewer); update.commit(); - update = newUpdate(change, changeOwner); + update = newUpdate(change1, changeOwner); update.setPatchSetId(psId2); - plcUtil.upsertComments(db, update, drafts); + plcUtil.upsertComments(db, update, drafts1); update.commit(); - ChangeControl ctl = stubChangeControl(change); + update = newUpdate(change2, changeOwner); + update.setPatchSetId(psId3); + plcUtil.upsertComments(db, update, drafts2); + update.commit(); + + ChangeControl ctl = stubChangeControl(change1); revRes1 = new RevisionResource(new ChangeResource(ctl, null), ps1); revRes2 = new RevisionResource(new ChangeResource(ctl, null), ps2); + revRes3 = new RevisionResource(new ChangeResource(stubChangeControl(change2), null), ps3); } private ChangeControl stubChangeControl(Change c) throws OrmException { @@ -352,6 +376,14 @@ public class CommentsTest { .containsExactly(plc4, plc5).inOrder(); } + @Test + public void testPatchLineCommentsUtilDraftByChangeAuthor() throws Exception { + assertThat(plcUtil.draftByChangeAuthor(db, revRes1.getNotes(), ownerId)) + .containsExactly(plc4, plc5).inOrder(); + assertThat(plcUtil.draftByChangeAuthor(db, revRes3.getNotes(), ownerId)) + .containsExactly(plc6); + } + private static IAnswer> results( final PatchLineComment... comments) { return new IAnswer>() { @@ -390,7 +422,7 @@ public class CommentsTest { private void assertCommentMap(Map> actual, Map> expected, boolean isPublished) { - assertThat(actual.keySet()).containsExactlyElementsIn(expected.keySet()); + assertThat((Iterable)actual.keySet()).containsExactlyElementsIn(expected.keySet()); for (Map.Entry> entry : actual.entrySet()) { List actualList = entry.getValue(); List expectedList = expected.get(entry.getKey());