Skip to content
Permalink
Browse files
Use @mentions to improve email threading
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Feb 28, 2020
1 parent ca1bbe5 commit 7d37ea9b2f5eb08475d8c0d594ac909be75fc186
Showing 2 changed files with 161 additions and 1 deletion.
@@ -10,6 +10,7 @@
import java.time.ZonedDateTime;
import java.util.*;
import java.util.function.Supplier;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

class ArchiveItem {
@@ -135,16 +136,39 @@ static ArchiveItem from(PullRequest pr, ReviewComment reviewComment, HostUserToE
() -> ArchiveMessages.composeReplyFooter(pr));
}

private static Pattern mentionPattern = Pattern.compile("^@([\\w-]+).*");

private static Optional<ArchiveItem> findLastMention(String commentText, List<ArchiveItem> eligibleParents) {
var mentionMatcher = mentionPattern.matcher(commentText);
if (mentionMatcher.matches()) {
var username = mentionMatcher.group(1);
for (int i = eligibleParents.size() - 1; i != 0; --i) {
if (eligibleParents.get(i).author.userName().equals(username)) {
return Optional.of(eligibleParents.get(i));
}
}
}
return Optional.empty();
}

static ArchiveItem findParent(List<ArchiveItem> generated, Comment comment) {
ArchiveItem lastCommentOrReview = generated.get(0);
var eligible = new ArrayList<ArchiveItem>();
eligible.add(lastCommentOrReview);
for (var item : generated) {
if (item.id().startsWith("pc") || item.id().startsWith("rv")) {
if (item.createdAt().isBefore(comment.createdAt()) && item.createdAt().isAfter(lastCommentOrReview.createdAt())) {
lastCommentOrReview = item;
eligible.add(lastCommentOrReview);
}
}
}

var lastMention = findLastMention(comment.body(), eligible);
if (lastMention.isPresent()) {
return lastMention.get();
}

return lastCommentOrReview;
}

@@ -183,17 +207,24 @@ static ArchiveItem findParent(List<ArchiveItem> generated, List<ReviewComment> r
.filter(comment -> comment.threadId().equals(threadId))
.collect(Collectors.toList());
ReviewComment previousComment = null;
var eligible = new ArrayList<ArchiveItem>();
for (var threadComment : reviewThread) {
if (threadComment.equals(reviewComment)) {
break;
}
previousComment = threadComment;
eligible.add(findReviewCommentItem(generated, previousComment));
}

if (previousComment == null) {
return findRevisionItem(generated, reviewComment.hash());
} else {
return findReviewCommentItem(generated, previousComment);
var mentionedParent = findLastMention(reviewComment.body(), eligible);
if (mentionedParent.isPresent()) {
return mentionedParent.get();
} else {
return eligible.get(eligible.size() - 1);
}
}
}

@@ -650,6 +650,135 @@ void commentThreadingSeparated(TestInfo testInfo) throws IOException {
}
}

@Test
void commentWithMention(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var archiveFolder = new TemporaryDirectory();
var listServer = new TestMailmanServer();
var webrevServer = new TestWebrevServer()) {
var author = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();
var archive = credentials.getHostedRepository();
var listAddress = EmailAddress.parse(listServer.createList("test"));
var censusBuilder = credentials.getCensusBuilder()
.addReviewer(reviewer.forge().currentUser().id())
.addAuthor(author.forge().currentUser().id());
var from = EmailAddress.from("test", "test@test.mail");
var mlBot = MailingListBridgeBot.newBuilder()
.from(from)
.repo(author)
.archive(archive)
.censusRepo(censusBuilder.build())
.list(listAddress)
.listArchive(listServer.getArchive())
.smtpServer(listServer.getSMTP())
.webrevStorageRepository(archive)
.webrevStorageRef("webrev")
.webrevStorageBase(Path.of("test"))
.webrevStorageBaseUri(webrevServer.uri())
.issueTracker(URIBuilder.base("http://issues.test/browse/").build())
.build();

// Populate the projects repository
var reviewFile = Path.of("reviewfile.txt");
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType(), reviewFile);
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.url(), "master", true);
localRepo.push(masterHash, archive.url(), "webrev", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(archive, "master", "edit", "This is a pull request");
pr.setBody("This is now ready");
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// Make two comments from different authors
var reviewPr = reviewer.pullRequest(pr.id());
reviewPr.addComment("First comment");
pr.addComment("Second comment");

TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

pr.addComment("@" + reviewer.forge().currentUser().userName() + " reply to first");
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// The first comment should be quoted more often than the second
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertEquals(3, archiveContainsCount(archiveFolder.path(), "First comment"));
assertEquals(1, archiveContainsCount(archiveFolder.path(), "Second comment"));
}
}

@Test
void reviewCommentWithMention(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var archiveFolder = new TemporaryDirectory();
var listServer = new TestMailmanServer();
var webrevServer = new TestWebrevServer()) {
var author = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();
var archive = credentials.getHostedRepository();
var listAddress = EmailAddress.parse(listServer.createList("test"));
var censusBuilder = credentials.getCensusBuilder()
.addReviewer(reviewer.forge().currentUser().id())
.addAuthor(author.forge().currentUser().id());
var from = EmailAddress.from("test", "test@test.mail");
var mlBot = MailingListBridgeBot.newBuilder()
.from(from)
.repo(author)
.archive(archive)
.censusRepo(censusBuilder.build())
.list(listAddress)
.listArchive(listServer.getArchive())
.smtpServer(listServer.getSMTP())
.webrevStorageRepository(archive)
.webrevStorageRef("webrev")
.webrevStorageBase(Path.of("test"))
.webrevStorageBaseUri(webrevServer.uri())
.issueTracker(URIBuilder.base("http://issues.test/browse/").build())
.build();

// Populate the projects repository
var reviewFile = Path.of("reviewfile.txt");
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType(), reviewFile);
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.url(), "master", true);
localRepo.push(masterHash, archive.url(), "webrev", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(archive, "master", "edit", "This is a pull request");
pr.setBody("This is now ready");
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// Make two review comments from different authors
var reviewPr = reviewer.pullRequest(pr.id());
var comment = reviewPr.addReviewComment(masterHash, editHash, reviewFile.toString(), 2, "Review comment");
reviewPr.addReviewCommentReply(comment, "First review comment");
pr.addReviewCommentReply(comment, "Second review comment");

TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

pr.addReviewCommentReply(comment, "@" + reviewer.forge().currentUser().userName() + " reply to first");
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// The first comment should be quoted more often than the second
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertEquals(3, archiveContainsCount(archiveFolder.path(), "First review comment"));
assertEquals(1, archiveContainsCount(archiveFolder.path(), "Second review comment"));
}
}

@Test
void reviewContext(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);

0 comments on commit 7d37ea9

Please sign in to comment.