Skip to content
Permalink
Browse files

148: Review comment general comment first

Reviewed-by: ehelin
  • Loading branch information
Robin Westberg
Robin Westberg committed Nov 6, 2019
1 parent 12ced19 commit f4cc08e26bb533f83f3336e0d174f7669c3fcdc4
@@ -136,10 +136,15 @@ static String composeCombinedReply(Email parent, String body, PullRequestInstanc
replyFooter(prInstance);
}

static String reviewCommentBody(String body, Review.Verdict verdict, String user, String role) {
var result = new StringBuilder(filterComments(body));
static String reviewCommentBody(String body) {
return filterComments(body);
}

static String reviewVerdictBody(String body, Review.Verdict verdict, String user, String role) {
var filteredBody = filterComments(body);
var result = new StringBuilder();
if (verdict != Review.Verdict.NONE) {
if (result.length() > 0) {
if (filteredBody.length() > 0) {
result.append("\n\n");
result.append(infoSeparator);
result.append("\n\n");
@@ -281,6 +281,15 @@ public void run(Path scratchPath) {
reviewArchive.addComment(comment);
}

// Review comments
var reviews = pr.reviews();
for (var review : reviews) {
if (ignoreComment(review.reviewer(), review.body().orElse(""))) {
continue;
}
reviewArchive.addReview(review);
}

// File specific comments
var reviewComments = pr.reviewComments();
for (var reviewComment : reviewComments) {
@@ -290,13 +299,12 @@ public void run(Path scratchPath) {
reviewArchive.addReviewComment(reviewComment);
}

// Review comments
var reviews = pr.reviews();
// Review verdict comments
for (var review : reviews) {
if (ignoreComment(review.reviewer(), review.body().orElse(""))) {
continue;
}
reviewArchive.addReview(review);
reviewArchive.addReviewVerdict(review);
}

var newMails = reviewArchive.generatedEmails();
@@ -21,6 +21,7 @@
private final Map<String, Email> existingIds = new HashMap<>();
private final List<Email> generated = new ArrayList<>();
private final Map<String, Email> generatedIds = new HashMap<>();
private final Set<EmailAddress> approvalIds = new HashSet<>();
private final List<Hash> reportedHeads;
private final List<Hash> reportedBases;

@@ -327,6 +328,21 @@ void addReview(Review review) {
return;
}

// Default parent and subject
var parent = topCommentForHash(review.hash());
var subject = parent.subject();

var replyBody = ArchiveMessages.reviewCommentBody(review.body().orElse(""));

addReplyCommon(parent, review.reviewer(), subject, replyBody, id);
}

void addReviewVerdict(Review review) {
var id = getMessageId(review);
if (existingIds.containsKey(getStableMessageId(id))) {
return;
}

var contributor = censusInstance.namespace().get(review.reviewer().id());
var isReviewer = contributor != null && censusInstance.project().isReviewer(contributor.username(), censusInstance.configuration().census().version());

@@ -336,13 +352,12 @@ void addReview(Review review) {

// Approvals by Reviewers get special treatment - post these as top-level comments
if (review.verdict() == Review.Verdict.APPROVED && isReviewer) {
parent = topEmail();
subject = "Re: [Approved] " + "RFR: " + prInstance.pr().title();
approvalIds.add(id);
}

var userName = contributor != null ? contributor.username() : review.reviewer().userName() + "@" + censusInstance.namespace().name();
var userRole = contributor != null ? projectRole(contributor) : "no project role";
var replyBody = ArchiveMessages.reviewCommentBody(review.body().orElse(""), review.verdict(), userName, userRole);
var replyBody = ArchiveMessages.reviewVerdictBody(review.body().orElse(""), review.verdict(), userName, userRole);

addReplyCommon(parent, review.reviewer(), subject, replyBody, id);
}
@@ -372,6 +387,20 @@ void addReviewComment(ReviewComment reviewComment) {
}

List<Email> generatedEmails() {
return generated;
var finalEmails = new ArrayList<Email>();
for (var email : generated) {
for (var approvalId : approvalIds) {
var collapsed = email.hasHeader("PR-Collapsed-IDs") ? email.headerValue("PR-Collapsed-IDs") + " " : "";
if (email.id().equals(approvalId) || collapsed.contains(getStableMessageId(approvalId))) {
email = Email.reparent(topEmail(), email)
.subject("Re: [Approved] " + "RFR: " + prInstance.pr().title())
.build();
break;
}
}
finalEmails.add(email);
}

return finalEmails;
}
}
@@ -491,6 +491,7 @@ void commentThreading(TestInfo testInfo) throws IOException {

// Finally some approvals and another comment
pr.addReview(Review.Verdict.APPROVED, "Nice");
reviewPr.addReviewComment(masterHash, editHash, reviewFile.toString(), 2, "The final review comment");
reviewPr.addReview(Review.Verdict.APPROVED, "Looks fine");
reviewPr.addReviewCommentReply(comment2, "You are welcome");
TestBotRunner.runPeriodicItems(mlBot);
@@ -502,9 +503,9 @@ void commentThreading(TestInfo testInfo) throws IOException {
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertEquals(9, archiveContainsCount(archiveFolder.path(), "^On.*wrote:"));

// File specific comments should appear before the approval
// File specific comments should appear after the approval
var archiveText = archiveContents(archiveFolder.path()).orElseThrow();
assertTrue(archiveText.indexOf("Looks fine") > archiveText.indexOf("You are welcome"));
assertTrue(archiveText.indexOf("Looks fine") < archiveText.indexOf("The final review comment"));

// Check the mailing list
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
@@ -543,10 +544,13 @@ void commentThreading(TestInfo testInfo) throws IOException {
assertTrue(thread2reply2.body().contains("Thanks"));

var replies = conversations.get(0).replies(mail);
var thread3 = conversations.get(0).replies(mail).get(2);
var thread3 = replies.get(2);
assertEquals("Re: RFR: This is a pull request", thread3.subject());
var thread4 = conversations.get(0).replies(mail).get(3);
var thread4 = replies.get(3);
assertEquals("Re: [Approved] RFR: This is a pull request", thread4.subject());
assertTrue(thread4.body().contains("Looks fine"));
assertTrue(thread4.body().contains("The final review comment"));
assertTrue(thread4.body().contains("Approved by integrationreviewer1 (Reviewer)"));
}
}

@@ -146,6 +146,14 @@ public static EmailBuilder reply(Email parent, String subject, String body) {
.header("References", references);
}

public static EmailBuilder reparent(Email newParent, Email email) {
var currentParent = email.headerValue("In-Reply-To");
var currentRefs = email.headerValue("References");

return from(email).header("In-Reply-To", newParent.id.toString())
.header("References", currentRefs.replace(currentParent, newParent.id.toString()));
}

@Override
public boolean equals(Object o) {
if (this == o) {
@@ -67,6 +67,24 @@ void buildFrom() {
assertEquals(original, copy);
}

@Test
void reparent() {
var first = Email.create(EmailAddress.from("A", "a@b.c"), "First", "body")
.recipient(EmailAddress.from("B", "b@b.c"))
.build();
var second = Email.create(EmailAddress.from("A", "a@b.c"), "Second", "body")
.recipient(EmailAddress.from("B", "b@b.c"))
.build();
var reply = Email.reply(first, "The reply", "reply body")
.author(EmailAddress.from("C", "c@b.c"))
.build();
assertEquals(first.id().toString(), reply.headerValue("In-Reply-To"));
assertEquals(first.id().toString(), reply.headerValue("References"));
var updated = Email.reparent(second, reply).build();
assertEquals(second.id().toString(), updated.headerValue("In-Reply-To"));
assertEquals(second.id().toString(), updated.headerValue("References"));
}

@Test
void caseInsensitiveHeaders() {
var mail = Email.parse("Message-ID: <a@b.c>\n" +

0 comments on commit f4cc08e

Please sign in to comment.
You can’t perform that action at this time.