diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java index 8ea321d7..b255bb19 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java @@ -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 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 getReviewVerdictMails(Email parent, List archiveMails) { - // Determine the latest reported reviews - var ret = new ArrayList(); - 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 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; } 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 239c675d..2c8eae6e 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 @@ -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")); } } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java index 809e0202..c872fa49 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java @@ -38,7 +38,8 @@ private final PullRequest pr; private final PullRequestInstance prInstance; private final List comments; - private final List reviews; + private final List allReviews; + private final List activeReviews; private final Set labels; private final CensusInstance censusInstance; private final Map blockingLabels; @@ -50,12 +51,14 @@ private final Set newLabels; private CheckRun(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prInstance, List comments, - List reviews, Set labels, CensusInstance censusInstance, Map blockingLabels) { + List allReviews, List activeReviews, Set labels, + CensusInstance censusInstance, Map blockingLabels) { this.workItem = workItem; this.pr = pr; this.prInstance = prInstance; this.comments = comments; - this.reviews = reviews; + this.allReviews = allReviews; + this.activeReviews = activeReviews; this.labels = new HashSet<>(labels); this.newLabels = new HashSet<>(labels); this.censusInstance = censusInstance; @@ -63,8 +66,8 @@ private CheckRun(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prI } static void execute(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prInstance, List comments, - List reviews, Set labels, CensusInstance censusInstance, Map blockingLabels) { - var run = new CheckRun(workItem, pr, prInstance, comments, reviews, labels, censusInstance, blockingLabels); + List allReviews, List activeReviews, Set labels, CensusInstance censusInstance, Map blockingLabels) { + var run = new CheckRun(workItem, pr, prInstance, comments, allReviews, activeReviews, labels, censusInstance, blockingLabels); run.checkStatus(); } @@ -244,11 +247,11 @@ private String getChecksList(PullRequestCheckIssueVisitor visitor) { private Optional getReviewersList(List reviews) { var reviewers = reviews.stream() - .filter(review -> review.verdict == Review.Verdict.APPROVED) + .filter(review -> review.verdict() == Review.Verdict.APPROVED) .map(review -> { - var entry = " * " + formatReviewer( review.reviewer); - if (!review.hash.equals(pr.getHeadHash())) { - entry += " **Note!** Review applies to " + review.hash; + var entry = " * " + formatReviewer(review.reviewer()); + if (!review.hash().equals(pr.getHeadHash())) { + entry += " **Note!** Review applies to " + review.hash(); } return entry; }) @@ -311,23 +314,8 @@ private void updateReviewedMessages(List comments, List reviews for (var added : reviewTracker.newReviews().entrySet()) { var body = added.getValue() + "\n" + "This PR has been reviewed by " + - formatReviewer(added.getKey().reviewer) + " - " + - verdictToString(added.getKey().verdict) + "."; - pr.addComment(body); - } - - for (var updated : reviewTracker.updatedReviews().entrySet()) { - var body = updated.getValue() + "\n" + - "The PR review by " + formatReviewer(updated.getKey().reviewer) + - " has been updated - " + verdictToString(updated.getKey().verdict) + "."; - pr.addComment(body); - } - - for (var removed : reviewTracker.removedReviews().entrySet()) { - var user = pr.repository().host().getUserDetails(removed.getKey()); - var body = removed.getValue() + "\n" + - "This PR is no longer reviewed by " + - formatReviewer(user) + "."; + formatReviewer(added.getKey().reviewer()) + " - " + + verdictToString(added.getKey().verdict()) + "."; pr.addComment(body); } } @@ -396,8 +384,8 @@ private String getMergeReadyComment(String commitMessage, List reviews, message.append("an existing [Committer](http://openjdk.java.net/bylaws#committer) must agree to "); message.append("[sponsor](http://openjdk.java.net/sponsor/) your change. "); var candidates = reviews.stream() - .filter(review -> ProjectPermissions.mayCommit(censusInstance, review.reviewer)) - .map(review -> "@" + review.reviewer.userName()) + .filter(review -> ProjectPermissions.mayCommit(censusInstance, review.reviewer())) + .map(review -> "@" + review.reviewer().userName()) .collect(Collectors.joining(", ")); if (candidates.length() > 0) { message.append("Possible candidates are the reviewers of this PR ("); @@ -463,16 +451,16 @@ private void checkStatus() { var rebasePossible = prInstance.rebasePossible(localHash); // Calculate and update the status message if needed - var statusMessage = getStatusMessage(reviews, visitor); + var statusMessage = getStatusMessage(activeReviews, visitor); var updatedBody = updateStatusMessage(statusMessage); // Post / update approval messages - updateReviewedMessages(comments, reviews); + updateReviewedMessages(comments, allReviews); var commit = prInstance.localRepo().lookup(localHash).orElseThrow(); var commitMessage = String.join("\n", commit.message()); updateMergeReadyComment(checkBuilder.build().status() == CheckStatus.SUCCESS, commitMessage, - comments, reviews, rebasePossible); + comments, activeReviews, rebasePossible); if (checkBuilder.build().status() == CheckStatus.SUCCESS) { newLabels.add("ready"); } else { @@ -485,12 +473,12 @@ private void checkStatus() { } // Calculate current metadata to avoid unnecessary future checks - var metadata = workItem.getMetadata(pr.getTitle(), updatedBody, pr.getComments(), reviews, newLabels, censusInstance, pr.getTargetHash()); + var metadata = workItem.getMetadata(pr.getTitle(), updatedBody, pr.getComments(), activeReviews, newLabels, censusInstance, pr.getTargetHash()); checkBuilder.metadata(metadata); } catch (Exception e) { log.throwing("CommitChecker", "checkStatus", e); newLabels.remove("ready"); - var metadata = workItem.getMetadata(pr.getTitle(), pr.getBody(), pr.getComments(), reviews, newLabels, censusInstance, pr.getTargetHash()); + var metadata = workItem.getMetadata(pr.getTitle(), pr.getBody(), pr.getComments(), activeReviews, newLabels, censusInstance, pr.getTargetHash()); checkBuilder.metadata(metadata); checkBuilder.title("Exception occurred during jcheck"); checkBuilder.summary(e.getMessage()); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java index 9e051bd7..f8896373 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java @@ -72,8 +72,8 @@ String getMetadata(String title, String body, List comments, List review.verdict == Review.Verdict.APPROVED) - .map(review -> encodeReviewer(review.reviewer, censusInstance) + review.hash.hex()) + .filter(review -> review.verdict() == Review.Verdict.APPROVED) + .map(review -> encodeReviewer(review.reviewer(), censusInstance) + review.hash().hex()) .sorted() .collect(Collectors.joining()); var commentString = comments.stream() @@ -156,13 +156,15 @@ public void run(Path scratchPath) { // First determine if the current state of the PR has already been checked var census = CensusInstance.create(censusRepo, censusRef, scratchPath.resolve("census"), pr); var comments = pr.getComments(); - var reviews = pr.getReviews(); - var labels = new HashSet(pr.getLabels()); + var allReviews = pr.getReviews(); + var labels = new HashSet<>(pr.getLabels()); - if (!currentCheckValid(census, comments, reviews, labels)) { + // Filter out the active reviews + var activeReviews = PullRequestInstance.filterActiveReviews(allReviews); + if (!currentCheckValid(census, comments, activeReviews, labels)) { try { var prInstance = new PullRequestInstance(scratchPath.resolve("pr"), pr); - CheckRun.execute(this, pr, prInstance, comments, reviews, labels, census, blockingLabels); + CheckRun.execute(this, pr, prInstance, comments, allReviews, activeReviews, labels, census, blockingLabels); } catch (IOException e) { throw new UncheckedIOException(e); } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java index 505d2000..1c51599d 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java @@ -50,10 +50,24 @@ baseHash = localRepo.mergeBase(targetHash, headHash); } - private String commitMessage(Namespace namespace, boolean isMerge) throws IOException { - var reviewers = pr.getReviews().stream() - .filter(review -> review.verdict == Review.Verdict.APPROVED) - .map(review -> review.reviewer.id()) + /** + * The Review list is in chronological order, the latest one from a particular reviewer is the + * one that is "active". + * @param allReviews + * @return + */ + static List filterActiveReviews(List allReviews) { + var reviewPerUser = new LinkedHashMap(); + for (var review : allReviews) { + reviewPerUser.put(review.reviewer(), review); + } + return new ArrayList<>(reviewPerUser.values()); + } + + private String commitMessage(List activeReviews, Namespace namespace, boolean isMerge) throws IOException { + var reviewers = activeReviews.stream() + .filter(review -> review.verdict() == Review.Verdict.APPROVED) + .map(review -> review.reviewer().id()) .map(namespace::get) .filter(Objects::nonNull) .map(Contributor::username) @@ -70,7 +84,7 @@ private String commitMessage(Namespace namespace, boolean isMerge) throws IOExce return String.join("\n", commitMessage.format(CommitMessageFormatters.v1)); } - private Hash commitSquashed(Namespace namespace, String censusDomain, String sponsorId) throws IOException { + private Hash commitSquashed(List activeReviews, Namespace namespace, String censusDomain, String sponsorId) throws IOException { localRepo.checkout(baseHash, true); localRepo.squash(headHash); @@ -93,11 +107,11 @@ private Hash commitSquashed(Namespace namespace, String censusDomain, String spo committer = author; } - var commitMessage = commitMessage(namespace, false); + var commitMessage = commitMessage(activeReviews, namespace, false); return localRepo.commit(commitMessage, author.name(), author.email(), committer.name(), committer.email()); } - private Hash commitMerge(Namespace namespace, String censusDomain) throws IOException { + private Hash commitMerge(List activeReviews, Namespace namespace, String censusDomain) throws IOException { localRepo.checkout(headHash, true); var contributor = namespace.get(pr.getAuthor().id()); @@ -107,15 +121,16 @@ private Hash commitMerge(Namespace namespace, String censusDomain) throws IOExce var author = new Author(contributor.fullName().orElseThrow(), contributor.username() + "@" + censusDomain); - var commitMessage = commitMessage(namespace, true); + var commitMessage = commitMessage(activeReviews, namespace, true); return localRepo.amend(commitMessage, author.name(), author.email(), author.name(), author.email()); } Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException { + var activeReviews = filterActiveReviews(pr.getReviews()); if (!pr.getTitle().startsWith("Merge")) { - return commitSquashed(namespace, censusDomain, sponsorId); + return commitSquashed(activeReviews, namespace, censusDomain, sponsorId); } else { - return commitMerge(namespace, censusDomain); + return commitMerge(activeReviews, namespace, censusDomain); } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewTracker.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewTracker.java index 36b56d0e..dd2c7e22 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewTracker.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewTracker.java @@ -28,76 +28,35 @@ import java.util.regex.Pattern; class ReviewTracker { - private final String reviewMarker = ""; - private final String unreviewMarker = ""; + private final String reviewMarker = ""; private final Pattern reviewMarkerPattern = Pattern.compile( - ""); - private final Pattern unreviewMarkerPattern = Pattern.compile( - ""); - - private static class ReviewState { - Comment comment; - String userId; - String userName; - String hash; - int verdict; - } + ""); private final Map newComments = new HashMap<>(); - private final Map removedReviews = new HashMap<>(); - private final Map updatedReviews = new HashMap<>(); ReviewTracker(List comments, List reviews) { - var reviewStates = new HashMap(); + var notified = new HashSet(); // Calculate current state for (var comment : comments) { var reviewMarkerMatcher = reviewMarkerPattern.matcher(comment.body()); - var unreviewMarkerMatcher = unreviewMarkerPattern.matcher(comment.body()); if (reviewMarkerMatcher.find()) { - var reviewState = new ReviewState(); - reviewState.verdict = Integer.parseInt(reviewMarkerMatcher.group(1)); - reviewState.userId = reviewMarkerMatcher.group(2); - reviewState.userName = reviewMarkerMatcher.group(3); - reviewState.hash = reviewMarkerMatcher.group(4); - reviewState.comment = comment; - reviewStates.put(reviewState.userId, reviewState); - } else if (unreviewMarkerMatcher.find()) { - var userId = unreviewMarkerMatcher.group(1); - reviewStates.remove(userId); + var reviewId = Integer.parseInt(reviewMarkerMatcher.group(1)); + notified.add(reviewId); } } // Find all reviews without a comment for (var review : reviews) { // Not notified yet - if (!reviewStates.containsKey(review.reviewer.id())) { - newComments.put(review, String.format(reviewMarker, review.verdict.ordinal(), review.reviewer.id(), review.reviewer.userName(), review.hash.hex())); - } else { - var oldReview = reviewStates.get(review.reviewer.id()); - if (review.verdict.ordinal() != oldReview.verdict) { - updatedReviews.put(review, String.format(reviewMarker, review.verdict.ordinal(), review.reviewer.id(), review.reviewer.userName(), review.hash.hex())); - } - reviewStates.remove(review.reviewer.id()); + if (!notified.contains(review.id())) { + newComments.put(review, String.format(reviewMarker, review.id())); } } - - // Check if there are any states not covered by reviews - these must have been removed - for (var reviewState : reviewStates.values()) { - removedReviews.put(reviewState.userName, String.format(unreviewMarker, reviewState.userId)); - } } Map newReviews() { return newComments; } - - Map removedReviews() { - return removedReviews; - } - - Map updatedReviews() { - return updatedReviews; - } } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java index 3c9a8025..65ee2b49 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java @@ -71,7 +71,7 @@ void simpleCommit(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = reviewer.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Check the status again TestBotRunner.runPeriodicItems(checkBot); @@ -119,7 +119,7 @@ void whitespaceIssue(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = reviewer.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Check the status TestBotRunner.runPeriodicItems(checkBot); @@ -193,7 +193,7 @@ void multipleReviews(TestInfo testInfo) throws IOException { // Approve it var reviewerPr = reviewer.getPullRequest(authorPr.getId()); - reviewerPr.addReview(Review.Verdict.APPROVED); + reviewerPr.addReview(Review.Verdict.APPROVED, "Approved"); TestBotRunner.runPeriodicItems(checkBot); // Refresh the PR and check that it has been approved @@ -220,26 +220,26 @@ void multipleReviews(TestInfo testInfo) throws IOException { assertTrue(authorPr.getBody().contains("Note")); // Now we can approve it again - reviewerPr.addReview(Review.Verdict.APPROVED); + reviewerPr.addReview(Review.Verdict.APPROVED, "Approved"); TestBotRunner.runPeriodicItems(checkBot); // Refresh the PR and check that it has been approved (once) and is no longer stale authorPr = author.getPullRequest(authorPr.getId()); assertTrue(authorPr.getBody().contains("Approvers")); assertEquals(1, authorPr.getBody().split("Generated Reviewer", -1).length - 1); - assertEquals(1, authorPr.getReviews().size()); + assertTrue(authorPr.getReviews().size() >= 1); assertFalse(authorPr.getBody().contains("Note")); // Add a review with disapproval var commenterPr = commenter.getPullRequest(authorPr.getId()); - commenterPr.addReview(Review.Verdict.DISAPPROVED); + commenterPr.addReview(Review.Verdict.DISAPPROVED, "Disapproved"); TestBotRunner.runPeriodicItems(checkBot); // Refresh the PR and check that it still only approved once (but two reviews) and is no longer stale authorPr = author.getPullRequest(authorPr.getId()); assertTrue(authorPr.getBody().contains("Approvers")); assertEquals(1, authorPr.getBody().split("Generated Reviewer", -1).length - 1); - assertEquals(2, authorPr.getReviews().size()); + assertTrue(authorPr.getReviews().size() >= 2); assertFalse(authorPr.getBody().contains("Note")); } } @@ -279,7 +279,7 @@ void multipleCommitters(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = reviewer.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Check the status again TestBotRunner.runPeriodicItems(checkBot); @@ -332,7 +332,7 @@ void updatedContentFailsCheck(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = reviewer.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Check the status again TestBotRunner.runPeriodicItems(checkBot); @@ -393,38 +393,40 @@ void individualReviewComments(TestInfo testInfo) throws IOException { // Check the status TestBotRunner.runPeriodicItems(checkBot); + var comments = pr.getComments(); + var commentCount = comments.size(); // Approve it as another user var approvalPr = reviewer.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Check the status again TestBotRunner.runPeriodicItems(checkBot); - // There should now be a comment - var comments = pr.getComments(); - assertEquals(2, comments.size()); - var comment = comments.get(0); + // There should now be two additional comments + comments = pr.getComments(); + assertEquals(commentCount + 2, comments.size()); + var comment = comments.get(commentCount); assertTrue(comment.body().contains(reviewer.host().getCurrentUserDetails().userName())); assertTrue(comment.body().contains("approved")); // Drop the review - approvalPr.addReview(Review.Verdict.NONE); + approvalPr.addReview(Review.Verdict.NONE, "Unreviewed"); // Check the status again TestBotRunner.runPeriodicItems(checkBot); // There should now be yet another comment comments = pr.getComments(); - assertEquals(3, comments.size()); - comment = comments.get(2); + assertEquals(commentCount + 3, comments.size()); + comment = comments.get(commentCount + 2); assertTrue(comment.body().contains(reviewer.host().getCurrentUserDetails().userName())); assertTrue(comment.body().contains("comment")); // No changes should not generate additional comments TestBotRunner.runPeriodicItems(checkBot); comments = pr.getComments(); - assertEquals(3, comments.size()); + assertEquals(commentCount + 3, comments.size()); } } @@ -454,7 +456,7 @@ void mergeMessage(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Get all messages up to date TestBotRunner.runPeriodicItems(mergeBot); @@ -505,7 +507,7 @@ void cannotRebase(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Get all messages up to date TestBotRunner.runPeriodicItems(mergeBot); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ContributorTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ContributorTests.java index d0359cc9..976f6635 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ContributorTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ContributorTests.java @@ -92,7 +92,7 @@ void simple(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); TestBotRunner.runPeriodicItems(prBot); TestBotRunner.runPeriodicItems(prBot); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java index e9d42860..0a033278 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java @@ -60,7 +60,7 @@ void simpleMerge(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Attempt a merge (the bot should only process the first one) pr.addComment("/integrate"); @@ -118,8 +118,8 @@ void reviewersRetained(TestInfo testInfo) throws IOException { // Review it twice var integratorPr = integrator.getPullRequest(pr.getId()); var committerPr = committer.getPullRequest(pr.getId()); - integratorPr.addReview(Review.Verdict.APPROVED); - committerPr.addReview(Review.Verdict.APPROVED); + integratorPr.addReview(Review.Verdict.APPROVED, "Approved"); + committerPr.addReview(Review.Verdict.APPROVED, "Approved"); // Attempt a merge pr.addComment("/integrate"); @@ -282,7 +282,7 @@ void mergeNotification(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot see it (a few times) TestBotRunner.runPeriodicItems(mergeBot); @@ -303,7 +303,7 @@ void mergeNotification(TestInfo testInfo) throws IOException { assertEquals(0, error); // Drop the approval - approvalPr.addReview(Review.Verdict.DISAPPROVED); + approvalPr.addReview(Review.Verdict.DISAPPROVED, "Disapproved"); TestBotRunner.runPeriodicItems(mergeBot); // The instructional message should have been updated @@ -313,7 +313,7 @@ void mergeNotification(TestInfo testInfo) throws IOException { assertEquals(1, pushed); // Restore the approval - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); TestBotRunner.runPeriodicItems(mergeBot); // The instructional message should have been updated @@ -325,7 +325,7 @@ void mergeNotification(TestInfo testInfo) throws IOException { // Approve it as yet another user var reviewerPr = reviewer.getPullRequest(pr.getId()); - reviewerPr.addReview(Review.Verdict.APPROVED); + reviewerPr.addReview(Review.Verdict.APPROVED, "Approved"); TestBotRunner.runPeriodicItems(mergeBot); // The instructional message should have been updated @@ -399,7 +399,7 @@ void autoRebase(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Push something unrelated to master localRepo.checkout(masterHash, true); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java index ab08cb66..c4126c19 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java @@ -79,7 +79,7 @@ void branchMerge(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); @@ -158,7 +158,7 @@ void branchMergeRebase(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); @@ -244,7 +244,7 @@ void mergedCommitsFailingJCheck(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); @@ -299,7 +299,7 @@ void invalidSourceRepo(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); @@ -357,7 +357,7 @@ void invalidSourceBranch(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); @@ -420,7 +420,7 @@ void wrongSourceBranch(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); @@ -478,7 +478,7 @@ void invalidAuthor(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); @@ -550,7 +550,7 @@ void unrelatedHistory(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot check the status TestBotRunner.runPeriodicItems(mergeBot); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java index fd5929d8..0db509f4 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java @@ -62,7 +62,7 @@ private void runSponsortest(TestInfo testInfo, boolean isAuthor) throws IOExcept // Approve it as another user var approvalPr = reviewer.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot see it TestBotRunner.runPeriodicItems(mergeBot); @@ -253,7 +253,7 @@ void sponsorAfterChanges(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = reviewer.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Let the bot see it TestBotRunner.runPeriodicItems(mergeBot); @@ -338,7 +338,7 @@ void autoRebase(TestInfo testInfo) throws IOException { // Approve it as another user var approvalPr = integrator.getPullRequest(pr.getId()); - approvalPr.addReview(Review.Verdict.APPROVED); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); // Push something unrelated to master localRepo.checkout(masterHash, true); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/VetoTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/VetoTests.java index e972c8a9..78d2a3dd 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/VetoTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/VetoTests.java @@ -183,7 +183,7 @@ void vetoAndMerge(TestInfo testInfo) throws IOException { // Place a veto var vetoPr = vetoer.getPullRequest(pr.getId()); - vetoPr.addReview(Review.Verdict.APPROVED); + vetoPr.addReview(Review.Verdict.APPROVED, "Approved"); vetoPr.addComment("/reject"); TestBotRunner.runPeriodicItems(prBot); @@ -253,7 +253,7 @@ void vetoAndSponsor(TestInfo testInfo) throws IOException { // Place a veto var vetoPr = vetoer.getPullRequest(pr.getId()); - vetoPr.addReview(Review.Verdict.APPROVED); + vetoPr.addReview(Review.Verdict.APPROVED, "Approved"); vetoPr.addComment("/reject"); TestBotRunner.runPeriodicItems(prBot); diff --git a/host/src/main/java/org/openjdk/skara/host/PullRequest.java b/host/src/main/java/org/openjdk/skara/host/PullRequest.java index 0129780e..f28b1efc 100644 --- a/host/src/main/java/org/openjdk/skara/host/PullRequest.java +++ b/host/src/main/java/org/openjdk/skara/host/PullRequest.java @@ -45,7 +45,7 @@ HostUserDetails getAuthor(); /** - * List of reviews. + * List of reviews, in descending chronological order. * @return */ List getReviews(); @@ -53,7 +53,7 @@ /** * Adds a review with the given verdict. */ - void addReview(Review.Verdict verdict); + void addReview(Review.Verdict verdict, String body); /** * Add a file specific comment. diff --git a/host/src/main/java/org/openjdk/skara/host/Review.java b/host/src/main/java/org/openjdk/skara/host/Review.java index 75d13e73..ceef2045 100644 --- a/host/src/main/java/org/openjdk/skara/host/Review.java +++ b/host/src/main/java/org/openjdk/skara/host/Review.java @@ -24,10 +24,42 @@ import org.openjdk.skara.vcs.Hash; +import java.util.Optional; + public class Review { - public HostUserDetails reviewer; - public Verdict verdict; - public Hash hash; + private final HostUserDetails reviewer; + private final Verdict verdict; + private final Hash hash; + private final int id; + private final String body; + + public Review(HostUserDetails reviewer, Verdict verdict, Hash hash, int id, String body) { + this.reviewer = reviewer; + this.verdict = verdict; + this.hash = hash; + this.id = id; + this.body = body; + } + + public HostUserDetails reviewer() { + return reviewer; + } + + public Verdict verdict() { + return verdict; + } + + public Hash hash() { + return hash; + } + + public int id() { + return id; + } + + public Optional body() { + return Optional.ofNullable(body); + } public enum Verdict { NONE, diff --git a/host/src/main/java/org/openjdk/skara/host/github/GitHubPullRequest.java b/host/src/main/java/org/openjdk/skara/host/github/GitHubPullRequest.java index 56f4f00a..89b62a3a 100644 --- a/host/src/main/java/org/openjdk/skara/host/github/GitHubPullRequest.java +++ b/host/src/main/java/org/openjdk/skara/host/github/GitHubPullRequest.java @@ -65,41 +65,34 @@ public HostUserDetails getAuthor() { @Override public List getReviews() { - // Reviews are returned in chronological order, we only care about the latest var reviews = request.get("pulls/" + json.get("number").toString() + "/reviews").execute().stream() .map(JSONValue::asObject) + .filter(obj -> !(obj.get("state").asString().equals("COMMENTED") && obj.get("body").asString().isEmpty())) .map(obj -> { - var ret = new Review(); - ret.reviewer = host.parseUserDetails(obj); - ret.hash = new Hash(obj.get("commit_id").asString()); + var reviewer = host.parseUserDetails(obj); + var hash = new Hash(obj.get("commit_id").asString()); + Review.Verdict verdict; switch (obj.get("state").asString()) { case "APPROVED": - ret.verdict = Review.Verdict.APPROVED; + verdict = Review.Verdict.APPROVED; break; - case "REQUEST_CHANGES": - ret.verdict = Review.Verdict.DISAPPROVED; + case "CHANGES_REQUESTED": + verdict = Review.Verdict.DISAPPROVED; break; default: - ret.verdict = Review.Verdict.NONE; + verdict = Review.Verdict.NONE; break; } - return ret; + var id = obj.get("id").asInt(); + var body = obj.get("body").asString(); + return new Review(reviewer, verdict, hash, id, body); }) .collect(Collectors.toList()); - - var reviewMap = new HashMap(); - for (var review : reviews) { - reviewMap.put(review.reviewer.id(), review); - } - - return reviewMap.entrySet().stream() - .sorted(Map.Entry.comparingByKey()) - .map(Map.Entry::getValue) - .collect(Collectors.toList()); + return reviews; } @Override - public void addReview(Review.Verdict verdict) { + public void addReview(Review.Verdict verdict, String body) { var query = JSON.object(); switch (verdict) { case APPROVED: @@ -107,13 +100,12 @@ public void addReview(Review.Verdict verdict) { break; case DISAPPROVED: query.put("event", "REQUEST_CHANGES"); - query.put("body", "Disapproved by API function setApproval"); break; case NONE: query.put("event", "COMMENT"); - query.put("body", "Review comment by API function setApproval"); break; } + query.put("body", body); request.post("pulls/" + json.get("number").toString() + "/reviews") .body(query) .execute(); diff --git a/host/src/main/java/org/openjdk/skara/host/gitlab/GitLabMergeRequest.java b/host/src/main/java/org/openjdk/skara/host/gitlab/GitLabMergeRequest.java index d05dcced..6f6b3eb4 100644 --- a/host/src/main/java/org/openjdk/skara/host/gitlab/GitLabMergeRequest.java +++ b/host/src/main/java/org/openjdk/skara/host/gitlab/GitLabMergeRequest.java @@ -89,32 +89,46 @@ public HostUserDetails getAuthor() { return request.get("award_emoji").execute().stream() .map(JSONValue::asObject) .filter(obj -> obj.get("name").asString().equals("thumbsup") || - obj.get("name").asString().equals("thumbsdown")) + obj.get("name").asString().equals("thumbsdown") || + obj.get("name").asString().equals("question")) .map(obj -> { - var ret = new Review(); - ret.reviewer = repository.host().getUserDetails(obj.get("user").get("username").asString()); - ret.verdict = obj.get("name").asString().equals("thumbsup") ? Review.Verdict.APPROVED : Review.Verdict.DISAPPROVED; - var createdAt = ZonedDateTime.parse(obj.get("updated_at").asString()); - - // Find the latest commit that isn't created after our review - ret.hash = commits.get(0).hash; - for (var cd : commits) { - if (createdAt.isAfter(cd.date)) { - ret.hash = cd.hash; - } - } - return ret; - }) + var reviewer = repository.host().getUserDetails(obj.get("user").get("username").asString()); + Review.Verdict verdict; + switch (obj.get("name").asString()) { + case "thumbsup": + verdict = Review.Verdict.APPROVED; + break; + case "thumbsdown": + verdict = Review.Verdict.DISAPPROVED; + break; + default: + verdict = Review.Verdict.NONE; + break; + } + + var createdAt = ZonedDateTime.parse(obj.get("updated_at").asString()); + + // Find the latest commit that isn't created after our review + var hash = commits.get(0).hash; + for (var cd : commits) { + if (createdAt.isAfter(cd.date)) { + hash = cd.hash; + } + } + var id = obj.get("id").asInt(); + return new Review(reviewer, verdict, hash, id, null); + }) .collect(Collectors.toList()); } @Override - public void addReview(Review.Verdict verdict) { + public void addReview(Review.Verdict verdict, String body) { // Remove any previous awards var awards = request.get("award_emoji").execute().stream() .map(JSONValue::asObject) .filter(obj -> obj.get("name").asString().equals("thumbsup") || - obj.get("name").asString().equals("thumbsdown")) + obj.get("name").asString().equals("thumbsdown") || + obj.get("name").asString().equals("question")) .filter(obj -> obj.get("user").get("username").asString().equals(repository.host().getCurrentUserDetails().userName())) .map(obj -> obj.get("id").toString()) .collect(Collectors.toList()); @@ -131,8 +145,8 @@ public void addReview(Review.Verdict verdict) { award = "thumbsdown"; break; default: - // No action - return; + award = "question"; + break; } request.post("award_emoji") .body("name", award) @@ -323,7 +337,6 @@ private String encodeMarkdown(String message) { @Override public Map getChecks(Hash hash) { - var pattern = Pattern.compile(String.format(checkResultPattern, hash.hex())); var matchers = getComments().stream() .collect(Collectors.toMap(comment -> comment, @@ -341,6 +354,7 @@ private String encodeMarkdown(String message) { if (!entry.getValue().group(3).equals("NONE")) { checkBuilder.metadata(new String(Base64.getDecoder().decode(entry.getValue().group(3)), StandardCharsets.UTF_8)); } + checkBuilder.summary(entry.getKey().body()); return checkBuilder.build(); })); } diff --git a/test/src/main/java/org/openjdk/skara/test/TestPullRequest.java b/test/src/main/java/org/openjdk/skara/test/TestPullRequest.java index 195679bb..f848bf67 100644 --- a/test/src/main/java/org/openjdk/skara/test/TestPullRequest.java +++ b/test/src/main/java/org/openjdk/skara/test/TestPullRequest.java @@ -105,23 +105,19 @@ public HostUserDetails getAuthor() { } @Override - public void addReview(Review.Verdict verdict) { - var reviewer = repository.host().getCurrentUserDetails(); - var existing = data.reviews.stream() - .filter(review -> review.reviewer.equals(reviewer)) - .findAny(); - existing.ifPresent(data.reviews::remove); - - var review = new Review(); - review.reviewer = reviewer; - review.verdict = verdict; + public void addReview(Review.Verdict verdict, String body) { try { - review.hash = repository.localRepository().resolve(sourceRef).orElseThrow(); + var review = new Review(repository.host().getCurrentUserDetails(), + verdict, repository.localRepository().resolve(sourceRef).orElseThrow(), + data.reviews.size(), + body); + + data.reviews.add(review); + data.lastUpdate = ZonedDateTime.now(); + } catch (IOException e) { throw new UncheckedIOException(e); } - data.reviews.add(review); - data.lastUpdate = ZonedDateTime.now(); } @Override