Merge changes I86a8c524,Icd3b72b1,I178b0395

* changes: Expose the history of all reviewers ever on a change Move Change out of AbstractChangeNotes Store draft PatchLineComments in Git notes
2014-07-25 21:48:45 +00:00
parent 8e63fb29e3 3d60276a2b
commit e985c14c30
13 changed files with 970 additions and 61 deletions
--- 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
@@ -50,6 +50,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountInfo;
 import com.google.gerrit.server.account.CapabilityControl;
 import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.config.AnonymousCowardNameProvider;
 import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -110,6 +111,7 @@ public class CommentsTest  {
  private Injector injector;
  private Project.NameKey project;
  private InMemoryRepositoryManager repoManager;
+  private AllUsersNameProvider allUsers;
  private PatchLineCommentsUtil plcUtil;
  private RevisionResource revRes1;
  private RevisionResource revRes2;
@@ -165,6 +167,9 @@ public class CommentsTest  {
    injector = Guice.createInjector(mod);

    NotesMigration migration = injector.getInstance(NotesMigration.class);
+    allUsers = injector.getInstance(AllUsersNameProvider.class);
+    repoManager.createRepository(allUsers.get());
+
    plcUtil = new PatchLineCommentsUtil(migration);

    Account co = new Account(new Account.Id(1), TimeUtil.nowTs());
@@ -250,7 +255,7 @@ public class CommentsTest  {
  }

  private ChangeControl stubChangeControl(Change c) throws OrmException {
-    return TestChanges.stubChangeControl(repoManager, c, changeOwner);
+    return TestChanges.stubChangeControl(repoManager, c, allUsers, changeOwner);
  }

  private Change newChange() {
@@ -258,7 +263,7 @@ public class CommentsTest  {
  }

  private ChangeUpdate newUpdate(Change c, final IdentifiedUser user) throws Exception {
-    return TestChanges.newUpdate(injector, repoManager, c, user);
+    return TestChanges.newUpdate(injector, repoManager, c, allUsers, user);
  }

  @Test
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -49,6 +49,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.CapabilityControl;
 import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.Realm;
+import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.config.AnonymousCowardNameProvider;
 import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -88,6 +89,7 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;

+import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Date;
@@ -104,8 +106,10 @@ public class ChangeNotesTest {
  private InMemoryRepositoryManager repoManager;
  private InMemoryRepository repo;
  private FakeAccountCache accountCache;
+  private AllUsersNameProvider allUsers;
  private IdentifiedUser changeOwner;
  private IdentifiedUser otherUser;
+  private Account.Id otherUserId;
  private Injector injector;
  private String systemTimeZone;
  private volatile long clockStepMs;
@@ -157,8 +161,11 @@ public class ChangeNotesTest {

    IdentifiedUser.GenericFactory userFactory =
        injector.getInstance(IdentifiedUser.GenericFactory.class);
+    allUsers = injector.getInstance(AllUsersNameProvider.class);
+    repoManager.createRepository(allUsers.get());
    changeOwner = userFactory.create(co.getId());
    otherUser = userFactory.create(ou.getId());
+    otherUserId = otherUser.getAccountId();
  }

  private void setTimeForTesting() {
@@ -858,7 +865,7 @@ public class ChangeNotesTest {
        uuid, range1, range1.getEndLine(), otherUser, null, time1, message1,
        (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment1);
+    update.upsertComment(comment1);
    update.commit();

    update = newUpdate(c, otherUser);
@@ -867,7 +874,7 @@ public class ChangeNotesTest {
        uuid, range2, range2.getEndLine(), otherUser, null, time2, message2,
        (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment2);
+    update.upsertComment(comment2);
    update.commit();

    update = newUpdate(c, otherUser);
@@ -876,7 +883,7 @@ public class ChangeNotesTest {
        uuid, range3, range3.getEndLine(), otherUser, null, time3, message3,
        (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment3);
+    update.upsertComment(comment3);
    update.commit();

    ChangeNotes notes = newNotes(c);
@@ -936,7 +943,7 @@ public class ChangeNotesTest {
        uuid, range1, range1.getEndLine(), otherUser, null, time1, message1,
        (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment1);
+    update.upsertComment(comment1);
    update.commit();

    update = newUpdate(c, otherUser);
@@ -945,7 +952,7 @@ public class ChangeNotesTest {
        uuid, range2, range2.getEndLine(), otherUser, null, time2, message2,
        (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment2);
+    update.upsertComment(comment2);
    update.commit();

    ChangeNotes notes = newNotes(c);
@@ -998,7 +1005,7 @@ public class ChangeNotesTest {
        range, range.getEndLine(), otherUser, null, now, messageForBase,
        (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(commentForBase);
+    update.upsertComment(commentForBase);
    update.commit();

    update = newUpdate(c, otherUser);
@@ -1007,7 +1014,7 @@ public class ChangeNotesTest {
        range, range.getEndLine(), otherUser, null, now, messageForPS,
        (short) 1, "abcd4567abcd4567abcd4567abcd4567abcd4567");
    update.setPatchSetId(psId);
-    update.putComment(commentForPS);
+    update.upsertComment(commentForPS);
    update.commit();

    ChangeNotes notes = newNotes(c);
@@ -1040,7 +1047,7 @@ public class ChangeNotesTest {
        uuid, range, range.getEndLine(), otherUser, null, timeForComment1,
        "comment 1", side, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment1);
+    update.upsertComment(comment1);
    update.commit();

    update = newUpdate(c, otherUser);
@@ -1048,7 +1055,7 @@ public class ChangeNotesTest {
        uuid, range, range.getEndLine(), otherUser, null, timeForComment2,
        "comment 2", side, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment2);
+    update.upsertComment(comment2);
    update.commit();

    ChangeNotes notes = newNotes(c);
@@ -1086,7 +1093,7 @@ public class ChangeNotesTest {
        uuid, range, range.getEndLine(), otherUser, null, now, "comment 1",
        side, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment1);
+    update.upsertComment(comment1);
    update.commit();

    update = newUpdate(c, otherUser);
@@ -1094,7 +1101,7 @@ public class ChangeNotesTest {
        uuid, range, range.getEndLine(), otherUser, null, now, "comment 2",
        side, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(psId);
-    update.putComment(comment2);
+    update.upsertComment(comment2);
    update.commit();

    ChangeNotes notes = newNotes(c);
@@ -1130,7 +1137,7 @@ public class ChangeNotesTest {
        uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps1",
        side, "abcd1234abcd1234abcd1234abcd1234abcd1234");
    update.setPatchSetId(ps1);
-    update.putComment(comment1);
+    update.upsertComment(comment1);
    update.commit();

    incrementPatchSet(c);
@@ -1142,7 +1149,7 @@ public class ChangeNotesTest {
        uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2",
        side, "abcd4567abcd4567abcd4567abcd4567abcd4567");
    update.setPatchSetId(ps2);
-    update.putComment(comment2);
+    update.upsertComment(comment2);
    update.commit();

    ChangeNotes notes = newNotes(c);
@@ -1165,6 +1172,160 @@ public class ChangeNotesTest {
    assertEquals(comment2, commentFromPs2);
  }

+  @Test
+  public void patchLineCommentSingleDraftToPublished() throws Exception {
+    Change c = newChange();
+    String uuid = "uuid";
+    CommentRange range = new CommentRange(1, 1, 2, 1);
+    PatchSet.Id ps1 = c.currentPatchSetId();
+    String filename = "filename1";
+    short side = (short) 1;
+
+    ChangeUpdate update = newUpdate(c, otherUser);
+    Timestamp now = TimeUtil.nowTs();
+    PatchLineComment comment1 = newPatchLineComment(ps1, filename, uuid,
+        range, range.getEndLine(), otherUser, null, now, "comment on ps1", side,
+        "abcd4567abcd4567abcd4567abcd4567abcd4567", Status.DRAFT);
+    update.setPatchSetId(ps1);
+    update.insertComment(comment1);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertEquals(1, notes.getDraftPsComments(otherUserId).values().size());
+    assertEquals(0, notes.getDraftBaseComments(otherUserId).values().size());
+
+    comment1.setStatus(Status.PUBLISHED);
+    update = newUpdate(c, otherUser);
+    update.setPatchSetId(ps1);
+    update.updateComment(comment1);
+    update.commit();
+
+    notes = newNotes(c);
+
+    assertTrue(notes.getDraftPsComments(otherUserId).values().isEmpty());
+    assertTrue(notes.getDraftBaseComments(otherUserId).values().isEmpty());
+
+    assertTrue(notes.getBaseComments().values().isEmpty());
+    PatchLineComment commentFromNotes =
+        Iterables.getOnlyElement(notes.getPatchSetComments().values());
+    assertEquals(comment1, commentFromNotes);
+  }
+
+  @Test
+  public void patchLineCommentMultipleDraftsSameSidePublishOne()
+      throws OrmException, IOException {
+    Change c = newChange();
+    String uuid1 = "uuid1";
+    String uuid2 = "uuid2";
+    CommentRange range1 = new CommentRange(1, 1, 2, 2);
+    CommentRange range2 = new CommentRange(2, 2, 3, 3);
+    String filename = "filename1";
+    short side = (short) 1;
+    Timestamp now = TimeUtil.nowTs();
+    String commitSHA1 = "abcd4567abcd4567abcd4567abcd4567abcd4567";
+    PatchSet.Id psId = c.currentPatchSetId();
+
+    // Write two drafts on the same side of one patch set.
+    ChangeUpdate update = newUpdate(c, otherUser);
+    update.setPatchSetId(psId);
+    PatchLineComment comment1 = newPatchLineComment(psId, filename, uuid1,
+        range1, range1.getEndLine(), otherUser, null, now, "comment on ps1",
+        side, commitSHA1, Status.DRAFT);
+    PatchLineComment comment2 = newPatchLineComment(psId, filename, uuid2,
+        range2, range2.getEndLine(), otherUser, null, now, "other on ps1",
+        side, commitSHA1, Status.DRAFT);
+    update.insertComment(comment1);
+    update.insertComment(comment2);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertTrue(notes.getDraftBaseComments(otherUserId).values().isEmpty());
+    assertEquals(2, notes.getDraftPsComments(otherUserId).values().size());
+
+    assertTrue(notes.getDraftPsComments(otherUserId).containsValue(comment1));
+    assertTrue(notes.getDraftPsComments(otherUserId).containsValue(comment2));
+
+    // Publish first draft.
+    update = newUpdate(c, otherUser);
+    update.setPatchSetId(psId);
+    comment1.setStatus(Status.PUBLISHED);
+    update.updateComment(comment1);
+    update.commit();
+
+    notes = newNotes(c);
+    assertEquals(comment1,
+        Iterables.getOnlyElement(notes.getPatchSetComments().get(psId)));
+    assertEquals(comment2,
+        Iterables.getOnlyElement(
+            notes.getDraftPsComments(otherUserId).values()));
+
+    assertTrue(notes.getBaseComments().values().isEmpty());
+    assertTrue(notes.getDraftBaseComments(otherUserId).values().isEmpty());
+  }
+
+  @Test
+  public void patchLineCommentsMultipleDraftsBothSidesPublishAll()
+      throws OrmException, IOException {
+    Change c = newChange();
+    String uuid1 = "uuid1";
+    String uuid2 = "uuid2";
+    CommentRange range1 = new CommentRange(1, 1, 2, 2);
+    CommentRange range2 = new CommentRange(2, 2, 3, 3);
+    String filename = "filename1";
+    Timestamp now = TimeUtil.nowTs();
+    String commitSHA1 = "abcd4567abcd4567abcd4567abcd4567abcd4567";
+    String baseSHA1 = "abcd1234abcd1234abcd1234abcd1234abcd1234";
+    PatchSet.Id psId = c.currentPatchSetId();
+
+    // Write two drafts, one on each side of the patchset.
+    ChangeUpdate update = newUpdate(c, otherUser);
+    update.setPatchSetId(psId);
+    PatchLineComment baseComment = newPatchLineComment(psId, filename, uuid1,
+        range1, range1.getEndLine(), otherUser, null, now, "comment on base",
+        (short) 0, baseSHA1, Status.DRAFT);
+    PatchLineComment psComment = newPatchLineComment(psId, filename, uuid2,
+        range2, range2.getEndLine(), otherUser, null, now, "comment on ps",
+        (short) 1, commitSHA1, Status.DRAFT);
+
+    update.insertComment(baseComment);
+    update.insertComment(psComment);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    PatchLineComment baseDraftCommentFromNotes =
+        Iterables.getOnlyElement(
+            notes.getDraftBaseComments(otherUserId).values());
+    PatchLineComment psDraftCommentFromNotes =
+        Iterables.getOnlyElement(
+            notes.getDraftPsComments(otherUserId).values());
+
+    assertEquals(baseComment, baseDraftCommentFromNotes);
+    assertEquals(psComment, psDraftCommentFromNotes);
+
+    // Publish both comments.
+    update = newUpdate(c, otherUser);
+    update.setPatchSetId(psId);
+
+    baseComment.setStatus(Status.PUBLISHED);
+    psComment.setStatus(Status.PUBLISHED);
+    update.updateComment(baseComment);
+    update.updateComment(psComment);
+    update.commit();
+
+    notes = newNotes(c);
+
+    PatchLineComment baseCommentFromNotes =
+        Iterables.getOnlyElement(notes.getBaseComments().values());
+    PatchLineComment psCommentFromNotes =
+        Iterables.getOnlyElement(notes.getPatchSetComments().values());
+
+    assertEquals(baseComment, baseCommentFromNotes);
+    assertEquals(psComment, psCommentFromNotes);
+
+    assertTrue(notes.getDraftBaseComments(otherUserId).values().isEmpty());
+    assertTrue(notes.getDraftPsComments(otherUserId).values().isEmpty());
+  }
+
  private Change newChange() {
    return TestChanges.newChange(project, changeOwner);
  }
@@ -1194,12 +1355,12 @@ public class ChangeNotesTest {
  }

  private ChangeUpdate newUpdate(Change c, IdentifiedUser user)
-      throws Exception {
-    return TestChanges.newUpdate(injector, repoManager, c, user);
+      throws OrmException {
+    return TestChanges.newUpdate(injector, repoManager, c, allUsers, user);
  }

  private ChangeNotes newNotes(Change c) throws OrmException {
-    return new ChangeNotes(repoManager, c).load();
+    return new ChangeNotes(repoManager, allUsers, c).load();
  }

  private static Timestamp truncate(Timestamp ts) {
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java
@@ -23,8 +23,11 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.config.FactoryModule;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeDraftUpdate;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
@@ -52,26 +55,29 @@ public class TestChanges {
  }

  public static ChangeUpdate newUpdate(Injector injector,
-      GitRepositoryManager repoManager, Change c, final IdentifiedUser user)
+      GitRepositoryManager repoManager, Change c,
+      final AllUsersNameProvider allUsers, final IdentifiedUser user)
      throws OrmException {
    return injector.createChildInjector(new FactoryModule() {
      @Override
      public void configure() {
        factory(ChangeUpdate.Factory.class);
+        factory(ChangeDraftUpdate.Factory.class);
        bind(IdentifiedUser.class).toInstance(user);
+        bind(AllUsersName.class).toProvider(allUsers);
      }
    }).getInstance(ChangeUpdate.Factory.class).create(
-        stubChangeControl(repoManager, c, user), TimeUtil.nowTs(),
+        stubChangeControl(repoManager, c, allUsers, user), TimeUtil.nowTs(),
        Ordering.<String> natural());
  }

  public static ChangeControl stubChangeControl(
-      GitRepositoryManager repoManager, Change c, IdentifiedUser user)
-      throws OrmException {
+      GitRepositoryManager repoManager, Change c, AllUsersNameProvider allUsers,
+      IdentifiedUser user) throws OrmException {
    ChangeControl ctl = EasyMock.createNiceMock(ChangeControl.class);
    expect(ctl.getChange()).andStubReturn(c);
    expect(ctl.getCurrentUser()).andStubReturn(user);
-    ChangeNotes notes = new ChangeNotes(repoManager, c);
+    ChangeNotes notes = new ChangeNotes(repoManager, allUsers, c);
    notes = notes.load();
    expect(ctl.getNotes()).andStubReturn(notes);
    EasyMock.replay(ctl);