Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use @mentions to improve email threading #486

Closed
wants to merge 2 commits into from
Closed

Use @mentions to improve email threading #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07cc97e
Find mentions in plain comments
Feb 27, 2020
15668b8
Add support for review comment threads as well
Feb 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -106,16 +107,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;
}

Expand Down Expand Up @@ -154,17 +178,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);
}
}
}

Expand Down
129 changes: 129 additions & 0 deletions bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down