diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java index f935d8a64..bc568d00a 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java @@ -124,10 +124,21 @@ private Set sentItemIds(List 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> collapsableItems(List 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 parentsToQuote(ArchiveItem item, int quoteLevel, Set alreadyQuoted) { + var ret = new ArrayList(); + + 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 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 sentEmails, List newEmails) { @@ -201,12 +224,21 @@ List generateNewEmails(List sentEmails, Duration cooldown, Reposit var combinedItems = collapsableItems(unsentItems); for (var itemList : combinedItems) { - // Simply combine all message bodies + var quotedParents = new HashSet(); + + // 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 generateNewEmails(List 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()) { diff --git a/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java b/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java index 7a9236b87..99fa16ab6 100644 --- a/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java +++ b/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java @@ -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:")); } }