Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

443: /reviewer pull request command should accept multiple reviewers #687

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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");
}
}
}