diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java index 358fe96a3..dc5c04199 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java @@ -35,10 +35,11 @@ public class ReviewerCommand implements CommandHandler { private static final Pattern commandPattern = Pattern.compile("^(add|remove)\\s+(.+)$"); private void showHelp(PullRequest pr, PrintWriter reply) { - reply.println("Syntax: `/reviewer (add|remove) [@user | openjdk-user]`. For example:"); + reply.println("Syntax: `/reviewer (add|remove) [@user | openjdk-user]+`. For example:"); reply.println(); reply.println(" * `/reviewer add @openjdk-bot`"); reply.println(" * `/reviewer add duke`"); + reply.println(" * `/reviewer add @user1 @user2`"); } private Optional parseUser(String user, PullRequest pr, CensusInstance censusInstance) { @@ -82,43 +83,53 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst return; } - var reviewer = parseUser(matcher.group(2), pr, censusInstance); - if (reviewer.isEmpty()) { - reply.println("Could not parse `" + matcher.group(2) + "` as a valid reviewer."); - showHelp(pr, reply);; - return; + var reviewers = new ArrayList(); + for (var entry : matcher.group(2).split("[\\s,]+")) { + var reviewer = parseUser(entry, pr, censusInstance); + if (reviewer.isEmpty()) { + reply.println("Could not parse `" + entry + "` as a valid reviewer."); + showHelp(pr, reply); + return; + } + + reviewers.add(reviewer.get()); } var namespace = censusInstance.namespace(); var authenticatedReviewers = PullRequestUtils.reviewerNames(pr.reviews(), namespace); - switch (matcher.group(1)) { - case "add": { - if (!authenticatedReviewers.contains(reviewer.get().username())) { - reply.println(Reviewers.addReviewerMarker(reviewer.get())); - reply.println("Reviewer `" + reviewer.get().username() + "` successfully added."); + var action = matcher.group(1); + if (action.equals("add")) { + for (var reviewer : reviewers) { + if (!authenticatedReviewers.contains(reviewer.username())) { + reply.println(Reviewers.addReviewerMarker(reviewer)); + reply.println("Reviewer `" + reviewer.username() + "` successfully added."); } else { - reply.println("Reviewer `" + reviewer.get().username() + "` has already made an authenticated review of this PR, and does not need to be added manually."); + reply.println("Reviewer `" + reviewer.username() + "` has already made an authenticated review of this PR, and does not need to be added manually."); } - break; } - case "remove": { - var existing = new HashSet<>(Reviewers.reviewers(pr.repository().forge().currentUser(), allComments)); - if (existing.contains(reviewer.get().username())) { - reply.println(Reviewers.removeReviewerMarker(reviewer.get())); - reply.println("Reviewer `" + reviewer.get().username() + "` successfully removed."); + } else if (action.equals("remove")) { + var failed = false; + var existing = new HashSet<>(Reviewers.reviewers(pr.repository().forge().currentUser(), allComments)); + for (var reviewer : reviewers) { + if (existing.contains(reviewer.username())) { + reply.println(Reviewers.removeReviewerMarker(reviewer)); + reply.println("Reviewer `" + reviewer.username() + "` successfully removed."); } else { if (existing.isEmpty()) { reply.println("There are no additional reviewers associated with this pull request."); + failed = true; } else { - reply.println("Reviewer `" + reviewer.get().username() + "` was not found."); - reply.println("Current additional reviewers are:"); - for (var e : existing) { - reply.println("- `" + e + "`"); - } + reply.println("Reviewer `" + reviewer.username() + "` was not found."); + failed = true; } - break; } - break; + } + + if (failed) { + reply.println("Current additional reviewers are:"); + for (var e : existing) { + reply.println("- `" + e + "`"); + } } } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/Reviewers.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/Reviewers.java index 920fdb775..079515f3d 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/Reviewers.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/Reviewers.java @@ -29,6 +29,7 @@ import java.util.*; import java.util.regex.*; import java.util.stream.Collectors; +import java.util.stream.Stream; class Reviewers { private final static String addMarker = ""; @@ -46,7 +47,8 @@ static String removeReviewerMarker(Contributor contributor) { static List reviewers(HostUser botUser, List comments) { var reviewerActions = comments.stream() .filter(comment -> comment.author().equals(botUser)) - .map(comment -> markerPattern.matcher(comment.body())) + .flatMap(comment -> Stream.of(comment.body().split("\n"))) + .map(line -> markerPattern.matcher(line)) .filter(Matcher::find) .collect(Collectors.toList()); var contributors = new LinkedHashSet(); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewerTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewerTests.java index 3571a06d4..53072852f 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewerTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewerTests.java @@ -210,7 +210,7 @@ void invalidReviewer(TestInfo testInfo) throws IOException { // Use a full name pr.addComment("/reviewer add Moo "); TestBotRunner.runPeriodicItems(prBot); - assertLastCommentContains(pr, "Could not parse `Moo ` as a valid reviewer"); + assertLastCommentContains(pr, "Could not parse `Moo` as a valid reviewer"); // Empty platform id pr.addComment("/reviewer add @"); @@ -460,4 +460,47 @@ void addAuthenticated(TestInfo testInfo) throws IOException { assertLastCommentContains(pr,"Reviewer `integrationreviewer1` has already made an authenticated review of this PR"); } } + + @Test + void multiple(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + var extra = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addReviewer(integrator.forge().currentUser().id()) + .addAuthor(extra.forge().currentUser().id()) + .addCommitter(author.forge().currentUser().id()); + var prBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build(); + + // Populate the projects repository + var localRepoFolder = tempFolder.path().resolve("localrepo"); + var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request"); + + // Add two reviewers + pr.addComment("/reviewer add integrationreviewer1 integrationauthor2"); + TestBotRunner.runPeriodicItems(prBot); + + // Expect success + assertLastCommentContains(pr, "Reviewed-by: integrationreviewer1, integrationauthor2"); + + // Remove both reviewers + pr.addComment("/reviewer remove integrationreviewer1 integrationauthor2"); + TestBotRunner.runPeriodicItems(prBot); + + // Expect success + assertLastCommentContains(pr, "Reviewer `integrationreviewer1` successfully removed"); + assertLastCommentContains(pr, "Reviewer `integrationauthor2` successfully removed"); + } + } }