opendev/gerrit
Code Issues Proposed changes

Refactor byChange() into two methods: drafts and published

Browse Source 
Callers of the PatchLineCommentUtils.byChange() mostly just want
either the published comments or the drafts comments. For that reason,
I refactored this method into two pieces and updated its callers.

I modified ChangeData.comments() to be ChangeData.publishedComments()
in order to more accurately the contents of the returned list.

Change-Id: I4222b894896af1ae635fd8c390fe014db9f6778d
This commit is contained in:
Yacob Yonas
2014-07-25 17:05:29 -07:00
parent 5020ff2fd0
commit e45d24a2a4
5 changed files with 66 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

39
gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java
View File

@@ -22,6 +22,7 @@ import com.google.common.collect.Lists;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RevId;
@@ -37,6 +38,7 @@ import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -81,7 +83,12 @@ public class PatchLineCommentsUtil {
if (!migration.readComments()) {
return Optional.fromNullable(db.patchComments().get(key));
}
for (PatchLineComment c : byChange(db, notes)) {
for (PatchLineComment c : publishedByChange(db, notes)) {
if (key.equals(c.getKey())) {
return Optional.of(c);
}
}
for (PatchLineComment c : draftByChange(db, notes)) {
if (key.equals(c.getKey())) {
return Optional.of(c);
}
@@ -89,17 +96,28 @@ public class PatchLineCommentsUtil {
return Optional.absent();
}
public List<PatchLineComment> byChange(ReviewDb db,
public List<PatchLineComment> publishedByChange(ReviewDb db,
ChangeNotes notes) throws OrmException {
if (!migration.readComments()) {
return db.patchComments().byChange(notes.getChangeId()).toList();
return byCommentStatus(db.patchComments().byChange(notes.getChangeId()),
Status.PUBLISHED);
}
notes.load();
notes.load();
List<PatchLineComment> comments = Lists.newArrayList();
comments.addAll(notes.getBaseComments().values());
comments.addAll(notes.getPatchSetComments().values());
return comments;
}
public List<PatchLineComment> draftByChange(ReviewDb db,
ChangeNotes notes) throws OrmException {
if (!migration.readComments()) {
return byCommentStatus(db.patchComments().byChange(notes.getChangeId()),
Status.DRAFT);
}
List<PatchLineComment> comments = Lists.newArrayList();
Iterable<String> filtered = getDraftRefs(notes.getChangeId());
for (String refName : filtered) {
Account.Id account = Account.Id.fromRefPart(refName);
@@ -110,6 +128,19 @@ public class PatchLineCommentsUtil {
return comments;
}
private static List<PatchLineComment> byCommentStatus(
ResultSet<PatchLineComment> comments,
final PatchLineComment.Status status) {
return Lists.newArrayList(
Iterables.filter(comments, new Predicate<PatchLineComment>() {
@Override
public boolean apply(PatchLineComment input) {
return (input.getStatus() == status);
}
})
);
}
public List<PatchLineComment> byPatchSet(ReviewDb db,
ChangeNotes notes, PatchSet.Id psId) throws OrmException {
if (!migration.readComments()) {

2
gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
View File

@@ -408,7 +408,7 @@ public class ChangeField {
public Iterable<String> get(ChangeData input, FillArgs args)
throws OrmException {
Set<String> r = Sets.newHashSet();
for (PatchLineComment c : input.comments()) {
for (PatchLineComment c : input.publishedComments()) {
r.add(c.getMessage());
}
for (ChangeMessage m : input.messages()) {

10
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -181,7 +181,7 @@ public class ChangeData {
private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals;
private List<PatchSetApproval> currentApprovals;
private Map<Integer, List<String>> files = new HashMap<>();
private Collection<PatchLineComment> comments;
private Collection<PatchLineComment> publishedComments;
private CurrentUser visibleTo;
private ChangeControl changeControl;
private List<ChangeMessage> messages;
@@ -527,12 +527,12 @@ public class ChangeData {
return approvalsUtil.getReviewers(notes(), approvals().values());
}
public Collection<PatchLineComment> comments()
public Collection<PatchLineComment> publishedComments()
throws OrmException {
if (comments == null) {
comments = plcUtil.byChange(db, notes());
if (publishedComments == null) {
publishedComments = plcUtil.publishedByChange(db, notes());
}
return comments;
return publishedComments;
}
public List<ChangeMessage> messages()

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
View File

@@ -366,7 +366,7 @@ public class QueryProcessor {
eventFactory.addComments(c, d.messages());
if (includePatchSets) {
for (PatchSetAttribute attribute : c.patchSets) {
eventFactory.addPatchSetComments(attribute, d.comments());
eventFactory.addPatchSetComments(attribute, d.publishedComments());
}
}
}

25
gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java
View File

@@ -21,6 +21,7 @@ import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import com.google.common.base.Objects;
@@ -223,7 +224,7 @@ public class CommentsTest {
PatchSet.Id psId2 = new PatchSet.Id(change.getId(), 2);
PatchSet ps2 = new PatchSet(psId2);
long timeBase = TimeUtil.nowMs();
long timeBase = TimeUtil.roundToSecond(TimeUtil.nowTs()).getTime();
plc1 = newPatchLineComment(psId1, "Comment1", null,
"FileOne.txt", Side.REVISION, 3, ownerId, timeBase,
"First Comment", new CommentRange(1, 2, 3, 4));
@@ -269,6 +270,8 @@ public class CommentsTest {
.andAnswer(results()).anyTimes();
expect(plca.draftByPatchSetAuthor(psId2, ownerId))
.andAnswer(results(plc4, plc5)).anyTimes();
expect(plca.byChange(change.getId()))
.andAnswer(results(plc1, plc2, plc3, plc4, plc5)).anyTimes();
replay(db, plca);
ChangeUpdate update = newUpdate(change, changeOwner);
@@ -334,6 +337,26 @@ public class CommentsTest {
"FileOne.txt", Lists.newArrayList(plc4, plc5)));
}
@Test
public void testPatchLineCommentsUtilByCommentStatus() throws OrmException {
List<PatchLineComment> publishedActual = plcUtil.publishedByChange(
injector.getInstance(ReviewDb.class), revRes2.getNotes());
List<PatchLineComment> draftActual = plcUtil.draftByChange(
injector.getInstance(ReviewDb.class), revRes2.getNotes());
List<PatchLineComment> publishedExpected =
Lists.newArrayList(plc1, plc2, plc3);
List<PatchLineComment> draftExpected =
Lists.newArrayList(plc4, plc5);
assertEquals(publishedExpected.size(), publishedActual.size());
assertEquals(draftExpected.size(), draftActual.size());
for (PatchLineComment c : draftExpected) {
assertTrue(draftActual.contains(c));
}
for (PatchLineComment c : publishedExpected) {
assertTrue(publishedActual.contains(c));
}
}
private static IAnswer<ResultSet<PatchLineComment>> results(
final PatchLineComment... comments) {
return new IAnswer<ResultSet<PatchLineComment>>() {