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.

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

Improve combining of nested review replies #475

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -124,10 +124,21 @@ private Set<String> sentItemIds(List<Email> sentEmails) {
.collect(Collectors.toSet());
}

private String parentAuthorPath(ArchiveItem item) {
var ret = new StringBuilder();
ret.append(item.author().id());
while (item.parent().isPresent()) {
item = item.parent().get();
ret.append(".");
ret.append(item.author().id());
}
return ret.toString();
}

// Group items that has the same author and the same parent
private List<List<ArchiveItem>> collapsableItems(List<ArchiveItem> items) {
var grouped = items.stream()
.collect(Collectors.groupingBy(item -> item.author().id() + "." + (item.parent().isPresent() ? item.parent().get() : "xxx"),
.collect(Collectors.groupingBy(this::parentAuthorPath,
LinkedHashMap::new, Collectors.toList()));
return new ArrayList<>(grouped.values());
}
@@ -138,16 +149,28 @@ private String quoteBody(String body) {
.collect(Collectors.joining("\n"));
}

private String quotedParent(ArchiveItem item, int quoteLevel) {
if (item.parent().isPresent() && quoteLevel > 0) {
var quotedParentBody = quotedParent(item.parent().get(), quoteLevel - 1);
if (!quotedParentBody.isBlank()) {
return quoteBody(quotedParentBody) + "\n> \n" + quoteBody(item.parent().get().body());
private List<ArchiveItem> parentsToQuote(ArchiveItem item, int quoteLevel, Set<ArchiveItem> alreadyQuoted) {
var ret = new ArrayList<ArchiveItem>();

if (item.parent().isPresent() && quoteLevel > 0 && !alreadyQuoted.contains(item.parent().get())) {
ret.add(item.parent().get());
ret.addAll(parentsToQuote(item.parent().get(), quoteLevel - 1, alreadyQuoted));
}

return ret;
}

private String quoteSelectedParents(List<ArchiveItem> parentsToQuote) {
Collections.reverse(parentsToQuote);
var ret = "";
for (var parent : parentsToQuote) {
if (!ret.isBlank()) {
ret = quoteBody(ret) + "\n>\n" + quoteBody(parent.body());
} else {
return quoteBody(item.parent().get().body());
ret = quoteBody(parent.body());
}
}
return "";
return ret;
}

private Email findArchiveItemEmail(ArchiveItem item, List<Email> sentEmails, List<Email> newEmails) {
@@ -201,12 +224,21 @@ List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Reposit

var combinedItems = collapsableItems(unsentItems);
for (var itemList : combinedItems) {
// Simply combine all message bodies
var quotedParents = new HashSet<ArchiveItem>();

// Simply combine all message bodies together with unique quotes
var body = new StringBuilder();
for (var item : itemList) {
if (body.length() > 0) {
body.append("\n\n");
}
var newQuotes = parentsToQuote(item, 2, quotedParents);
var quote = quoteSelectedParents(newQuotes);
if (!quote.isBlank()) {
body.append(quote);
body.append("\n\n");
}
quotedParents.addAll(newQuotes);
body.append(item.body());
}

@@ -221,15 +253,11 @@ List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Reposit
includedFooterFragments.addAll(newFooterFragments);
}

// All items have the same parent and author after collapsing -> should have the same header
// All items have parents from the same author after collapsing -> should have the same header
var firstItem = itemList.get(0);
var header = firstItem.header();
var quote = quotedParent(firstItem, 2);
if (!quote.isBlank()) {
quote = quote + "\n\n";
}

var combined = (header.isBlank() ? "" : header + "\n\n") + quote + body.toString() + (footer.length() == 0 ? "" : "\n\n-------------\n\n" + footer.toString());
var combined = (header.isBlank() ? "" : header + "\n\n") + body.toString() + (footer.length() == 0 ? "" : "\n\n-------------\n\n" + footer.toString());

var emailBuilder = Email.create(firstItem.subject(), combined);
if (firstItem.parent().isPresent()) {
@@ -413,7 +413,7 @@ void combineComments(TestInfo testInfo) throws IOException {

// Make several file specific comments
var first = pr.addReviewComment(masterHash, editHash, reviewFile.toString(), 2, "Review comment");
pr.addReviewComment(masterHash, editHash, reviewFile.toString(), 2, "Another review comment");
var second = pr.addReviewComment(masterHash, editHash, reviewFile.toString(), 2, "Another review comment");
pr.addReviewComment(masterHash, editHash, reviewFile.toString(), 2, "Further review comment");
pr.addReviewComment(masterHash, editHash, reviewFile.toString(), 2, "Final review comment");
TestBotRunner.runPeriodicItems(mlBot);
@@ -443,8 +443,9 @@ void combineComments(TestInfo testInfo) throws IOException {
assertTrue(reviewReply.body().contains("Review comment\n\n"), reviewReply.body());
assertTrue(reviewReply.body().contains("Another review comment"), reviewReply.body());

// Now reply to the first (collapsed) comment
// Now reply to the first and second (collapsed) comment
pr.addReviewCommentReply(first, "I agree");
pr.addReviewCommentReply(second, "Not with this one though");
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

@@ -645,7 +646,7 @@ void commentThreadingSeparated(TestInfo testInfo) throws IOException {

// Sanity check the archive
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertEquals(3, archiveContainsCount(archiveFolder.path(), "^On.*wrote:"));
assertEquals(2, archiveContainsCount(archiveFolder.path(), "^On.*wrote:"));
}
}