Skip to content
Permalink
Browse files
443: /reviewer pull request command should accept multiple reviewers
Reviewed-by: rwestberg
  • Loading branch information
edvbld committed Jul 3, 2020
1 parent 66d1ba5 commit a5891a83332da1db78a60474d9b2bfa52e16c86a
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 27 deletions.
@@ -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<Contributor> 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<Contributor>();
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 + "`");
}
}
}
}
@@ -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 = "<!-- add reviewer: '%s' -->";
@@ -46,7 +47,8 @@ static String removeReviewerMarker(Contributor contributor) {
static List<String> reviewers(HostUser botUser, List<Comment> 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<String>();
@@ -210,7 +210,7 @@ void invalidReviewer(TestInfo testInfo) throws IOException {
// Use a full name
pr.addComment("/reviewer add Moo <Foo.Bar (at) host.com>");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "Could not parse `Moo <Foo.Bar (at) host.com>` 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");
}
}
}

1 comment on commit a5891a8

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented on a5891a8 Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.