Skip to content
Permalink
Browse files

17: Review comments not reflected on mailing lists

Reviewed-by: ehelin
  • Loading branch information
Robin Westberg
Robin Westberg committed Jul 1, 2019
1 parent 68e0bee commit 3565522ee7adb486e8785bc9709e07355c56e5f8
@@ -179,6 +179,39 @@ private String composeReply(ZonedDateTime date, EmailAddress author, String pare
filterComments(body);
}

private String verdictToString(Review.Verdict verdict) {
switch (verdict) {
case APPROVED:
return "changes are approved";
case DISAPPROVED:
return "more changes are needed";
case NONE:
return "a comment has been added";
default:
throw new RuntimeException("Unknown verdict: " + verdict);
}
}

private String composeReview(ZonedDateTime date, EmailAddress parentAuthor, String parentBody, Review review) {
var body = new StringBuilder();
var author = getAuthorAddress(review.reviewer());
body.append("This PR has been reviewed by ");
body.append(author.fullName().orElse(author.localPart()));
body.append(" - ");
body.append(verdictToString(review.verdict()));
body.append(".");
if (review.body().isPresent()) {
body.append(" Review comment:\n\n");
body.append(review.body().get());
}

return "On " + date.format(DateTimeFormatter.RFC_1123_DATE_TIME) + ", " + parentAuthor.toString() + " wrote:\n" +
"\n" +
quoteBody(parentBody) +
"\n\n" +
filterComments(body.toString());
}

private String composeRebaseComment(Hash lastBase, PullRequestInstance prInstance, URI fullWebrev) {
var commitMessages = prInstance.formatCommitMessages(prInstance.baseHash(), prInstance.headHash(), this::formatCommit);
return "The pull request has been updated with a complete new set of changes (possibly due to a rebase).\n\n" +
@@ -217,41 +250,6 @@ private String composeReadyForIntegrationComment() {
"integration command from the author.";
}

private String verdictToString(Review.Verdict verdict) {
switch (verdict) {
case APPROVED:
return "changes are approved";
case DISAPPROVED:
return "more changes needed";
case NONE:
return "comment added";
default:
throw new RuntimeException("Unknown verdict: " + verdict);
}
}

private String composeNewReviewVerdict(Review review) {
var ret = new StringBuilder();
var author = getAuthorAddress(review.reviewer);
ret.append("This PR has now been reviewed by ");
ret.append(author.fullName().orElse(author.localPart()));
ret.append(" - ");
ret.append(verdictToString(review.verdict));
ret.append(".");
return ret.toString();
}

private String composeUpdatedReviewVerdict(Review review) {
var ret = new StringBuilder();
var author = getAuthorAddress(review.reviewer);
ret.append("The PR reviewed by ");
ret.append(author.fullName().orElse(author.localPart()));
ret.append(" has now been updated - ");
ret.append(verdictToString(review.verdict));
ret.append(".");
return ret.toString();
}

private Repository materializeArchive(Path scratchPath) {
try {
return Repository.materialize(scratchPath, bot.archiveRepo().getUrl(), pr.getTargetRef());
@@ -314,8 +312,8 @@ private EmailAddress getMessageId(String raw) {
return getUniqueMessageId("rw" + raw);
}

private EmailAddress getMessageId(HostUserDetails reviewer, String verdict, int reviewCount) {
return getUniqueMessageId("vd" + reviewer.id() + ";" + verdict + ";" + reviewCount);
private EmailAddress getMessageId(Review review) {
return getUniqueMessageId("rv" + review.id());
}

private EmailAddress getAuthorAddress(HostUserDetails originalAuthor) {
@@ -352,6 +350,19 @@ private Email comment(Email parent, Comment comment) {
return email;
}

private Email review(Email parent, Review review) {
var body = composeReview(parent.date(), parent.author(), parent.body(), review);
var email = Email.create(getAuthorAddress(review.reviewer()), "Re: RFR: " + pr.getTitle(), body)
.sender(bot.emailAddress())
.recipient(parent.author())
.id(getMessageId(review))
.header("In-Reply-To", parent.id().toString())
.header("References", parent.id().toString())
.build();
return email;

}

private Email reviewComment(Email parent, ReviewComment comment) {
var body = new StringBuilder();

@@ -424,50 +435,6 @@ private Email readyForIntegrationComment(Email parent, Set<String> currentLabels
return email;
}

private Email reviewVerdictComment(Email parent, HostUserDetails reviewer, String verdict, int reviewCount, String body) {
var email = Email.create(getAuthorAddress(reviewer), "Re: RFR: " + pr.getTitle(), body)
.sender(bot.emailAddress())
.recipient(parent.author())
.id(getMessageId(reviewer, verdict, reviewCount))
.header("In-Reply-To", parent.id().toString())
.header("References", parent.id().toString())
.header("PR-Review-Verdict", reviewer.id() + ";" + verdict)
.build();
return email;

}

private List<Email> getReviewVerdictMails(Email parent, List<Email> archiveMails) {
// Determine the latest reported reviews
var ret = new ArrayList<Email>();
var reportedReviews = archiveMails.stream()
.filter(email -> email.hasHeader("PR-Review-Verdict"))
.map(email -> email.headerValue("PR-Review-Verdict"))
.collect(Collectors.toMap(
value -> value.substring(0, value.indexOf(";")),
value -> value.substring(value.indexOf(";") + 1),
(a, b) -> b)
);
var reviews = pr.getReviews();
var newReviews = reviews.stream()
.filter(review -> !reportedReviews.containsKey(review.reviewer.id()))
.collect(Collectors.toList());
var updatedReviews = reviews.stream()
.filter(review -> reportedReviews.containsKey(review.reviewer.id()))
.filter(review -> !reportedReviews.get(review.reviewer.id()).equals(review.verdict.toString()))
.collect(Collectors.toList());

for (var newReview : newReviews) {
var body = composeNewReviewVerdict(newReview);
ret.add(reviewVerdictComment(parent, newReview.reviewer, newReview.verdict.toString(), reportedReviews.size(), body));
}
for (var updatedReview : updatedReviews) {
var body = composeUpdatedReviewVerdict(updatedReview);
ret.add(reviewVerdictComment(parent, updatedReview.reviewer, updatedReview.verdict.toString(), reportedReviews.size(), body));
}
return ret;
}

private List<Email> parseArchive(MailingList archive) {
var conversations = archive.conversations(Duration.ofDays(365));

@@ -672,8 +639,26 @@ public void run(Path scratchPath) {
previous = commentMail;
}

// File specific comments
// Review comments
final var first = archiveMails.size() > 0 ? archiveMails.get(0) : newMails.get(0);
var reviews = pr.getReviews();
for (var review : reviews) {
var id = getStableMessageId(getMessageId(review));
if (stableIdToMail.containsKey(id)) {
continue;
}
if (ignoreComment(review.reviewer(), review.body().orElse(""))) {
continue;
}

var commentMail = review(first, review);
archive.post(commentMail);
newMails.add(commentMail);
stableIdToMail.put(getStableMessageId(commentMail.id()), commentMail);
}


// File specific comments
var reviewComments = pr.getReviewComments();
for (var reviewComment : reviewComments) {
var id = getStableMessageId(getMessageId(reviewComment));
@@ -691,14 +676,6 @@ public void run(Path scratchPath) {
stableIdToMail.put(getStableMessageId(commentMail.id()), commentMail);
}

// Review verdicts
var reviewVerdictMails = getReviewVerdictMails(first, archiveMails);
for (var reviewVerdictMail : reviewVerdictMails) {
archive.post(reviewVerdictMail);
newMails.add(reviewVerdictMail);
stableIdToMail.put(getStableMessageId(reviewVerdictMail.id()), reviewVerdictMail);
}

if (newMails.isEmpty()) {
return;
}
@@ -497,6 +497,16 @@ void incrementalChanges(TestInfo testInfo) throws IOException {
var nextHash = CheckableRepository.appendAndCommit(localRepo, "Yet one more line", "Fixing");
localRepo.push(nextHash, author.getUrl(), "edit");

// Make sure that the push registered
var lastHeadHash = pr.getHeadHash();
var refreshCount = 0;
do {
pr = author.getPullRequest(pr.getId());
if (refreshCount++ > 100) {
fail("The PR did not update after the new push");
}
} while (pr.getHeadHash().equals(lastHeadHash));

// Run another archive pass
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);
@@ -536,6 +546,17 @@ void incrementalChanges(TestInfo testInfo) throws IOException {
for (int i = 0; i < 3; ++i) {
var anotherHash = CheckableRepository.appendAndCommit(localRepo, "Another line", "Fixing");
localRepo.push(anotherHash, author.getUrl(), "edit");

// Make sure that the push registered
lastHeadHash = pr.getHeadHash();
refreshCount = 0;
do {
pr = author.getPullRequest(pr.getId());
if (refreshCount++ > 100) {
fail("The PR did not update after the new push");
}
} while (pr.getHeadHash().equals(lastHeadHash));

TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();
@@ -586,6 +607,16 @@ void rebased(TestInfo testInfo) throws IOException {
var newEditHash = CheckableRepository.appendAndCommit(newLocalRepo, "Another line", "Replaced msg");
newLocalRepo.push(newEditHash, author.getUrl(), "edit", true);

// Make sure that the push registered
var lastHeadHash = pr.getHeadHash();
var refreshCount = 0;
do {
pr = author.getPullRequest(pr.getId());
if (refreshCount++ > 100) {
fail("The PR did not update after the new push");
}
} while (pr.getHeadHash().equals(lastHeadHash));

// Run another archive pass
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();
@@ -725,34 +756,35 @@ void notifyReviewVerdicts(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(mlBot);

// First unapprove it
pr.addReview(Review.Verdict.DISAPPROVED);
var reviewedPr = credentials.getHostedRepository().getPullRequest(pr.getId());
reviewedPr.addReview(Review.Verdict.DISAPPROVED, "Reason 1");
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);

// The archive should contain a note
Repository.materialize(archiveFolder.path(), archive.getUrl(), "master");
assertEquals(1, archiveContainsCount(archiveFolder.path(), "This PR has now been reviewed.*more changes needed"));
assertEquals(1, archiveContainsCount(archiveFolder.path(), "This PR has been reviewed.*more changes are needed"));

// Then approve it
pr.addReview(Review.Verdict.APPROVED);
reviewedPr.addReview(Review.Verdict.APPROVED, "Reason 2");
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);

// The archive should contain another note
Repository.materialize(archiveFolder.path(), archive.getUrl(), "master");
assertEquals(1, archiveContainsCount(archiveFolder.path(), "The PR reviewed by.*has now been updated.*approved"));
assertEquals(1, archiveContainsCount(archiveFolder.path(), "This PR.*approved"));

// Yet another change
pr.addReview(Review.Verdict.DISAPPROVED);
reviewedPr.addReview(Review.Verdict.DISAPPROVED, "Reason 3");
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);

// The archive should contain another note
Repository.materialize(archiveFolder.path(), archive.getUrl(), "master");
assertEquals(1, archiveContainsCount(archiveFolder.path(), "The PR reviewed by.*has now been updated.*more changes"));
assertEquals(2, archiveContainsCount(archiveFolder.path(), "This PR.*more changes"));
}
}
}

0 comments on commit 3565522

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