From eef24783a1d72c097efaed541211d1945c76d992 Mon Sep 17 00:00:00 2001 From: Erik Helin Date: Mon, 14 Sep 2020 17:54:05 +0200 Subject: [PATCH 1/3] 673 --- .../skara/bots/pr/ReviewersCommand.java | 4 +-- .../openjdk/skara/bots/pr/ReviewersTests.java | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java index 031b0fd12..0e60f4ec0 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java @@ -49,8 +49,8 @@ private void showHelp(PrintWriter reply) { @Override public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { - if (!censusInstance.isReviewer(command.user())) { - reply.println("Only [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed to change the number of required reviewers."); + if (!pr.author().equals(command.user()) && !censusInstance.isReviewer(command.user())) { + reply.println("Only the author of the pull request or [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed to change the number of required reviewers."); return; } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java index 4ab10d93f..f14921bd4 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java @@ -28,6 +28,7 @@ import org.junit.jupiter.api.*; import java.io.IOException; +import java.util.List; import static org.junit.jupiter.api.Assertions.*; import static org.openjdk.skara.bots.pr.PullRequestAsserts.assertLastCommentContains; @@ -250,4 +251,38 @@ void noSponsoring(TestInfo testInfo) throws IOException { assertLastCommentContains(reviewerPr,"Pushed as commit"); } } + + @Test + void prAuthorShouldBeAllowedToExecute(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var bot = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addCommitter(author.forge().currentUser().id()); + var prBot = PullRequestBot.newBuilder().repo(bot).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", "123: This is a pull request"); + + var authorPR = author.pullRequest(pr.id()); + + // The author deems that two reviewers are required + authorPR.addComment("/reviewers 2"); + + // The bot should reply with a success message + TestBotRunner.runPeriodicItems(prBot); + assertLastCommentContains(authorPR,"The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers)."); + } + } } From 347594f0b439be1829cf6237856d7c29a469fac9 Mon Sep 17 00:00:00 2001 From: Erik Helin Date: Tue, 15 Sep 2020 10:43:07 +0200 Subject: [PATCH 2/3] PR authors should not be allowed to decrease --- .../skara/bots/pr/CommandInvocation.java | 17 ++++++- .../skara/bots/pr/CommandWorkItem.java | 12 ++--- .../skara/bots/pr/ReviewersCommand.java | 46 ++++++++++++++++- .../skara/bots/pr/ReviewersTracker.java | 5 ++ .../openjdk/skara/bots/pr/ReviewersTests.java | 49 ++++++++++++++++++- 5 files changed, 120 insertions(+), 9 deletions(-) diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java index aded111e2..47eacc5c0 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java @@ -1,6 +1,7 @@ package org.openjdk.skara.bots.pr; import org.openjdk.skara.host.HostUser; +import org.openjdk.skara.issuetracker.Comment; import java.util.Optional; @@ -10,13 +11,15 @@ class CommandInvocation { private final CommandHandler handler; private final String name; private final String args; + private final Comment comment; - CommandInvocation(String id, HostUser user, CommandHandler handler, String name, String args) { + CommandInvocation(String id, HostUser user, CommandHandler handler, String name, String args, Comment comment) { this.id = id; this.user = user; this.handler = handler; this.name = name; this.args = args != null ? args.strip() : ""; + this.comment = comment; } String id() { @@ -38,4 +41,16 @@ String name() { String args() { return args; } + + Optional comment() { + return Optional.ofNullable(comment); + } + + boolean isInBody() { + return comment == null; + } + + boolean isInComment() { + return comment != null; + } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java index 887df4062..7fad79a66 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java @@ -118,7 +118,7 @@ public String description() { } } - private List extractCommands(String text, String baseId, HostUser user) { + private List extractCommands(String text, String baseId, HostUser user, Comment c) { var ret = new ArrayList(); CommandHandler multiLineHandler = null; List multiLineBuffer = null; @@ -128,7 +128,7 @@ private List extractCommands(String text, String baseId, Host var commandMatcher = commandPattern.matcher(line); if (commandMatcher.matches()) { if (multiLineHandler != null) { - ret.add(new CommandInvocation(formatId(baseId, subId++), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer))); + ret.add(new CommandInvocation(formatId(baseId, subId++), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer), c)); multiLineHandler = null; } var command = commandMatcher.group(1).toLowerCase(); @@ -144,7 +144,7 @@ private List extractCommands(String text, String baseId, Host } multiLineCommand = command; } else { - ret.add(new CommandInvocation(formatId(baseId, subId++), user, handler, command, commandMatcher.group(2))); + ret.add(new CommandInvocation(formatId(baseId, subId++), user, handler, command, commandMatcher.group(2), c)); } } else { if (multiLineHandler != null) { @@ -153,17 +153,17 @@ private List extractCommands(String text, String baseId, Host } } if (multiLineHandler != null) { - ret.add(new CommandInvocation(formatId(baseId, subId), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer))); + ret.add(new CommandInvocation(formatId(baseId, subId), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer), c)); } return ret; } private Optional nextCommand(PullRequest pr, List comments) { var self = pr.repository().forge().currentUser(); - var allCommands = Stream.concat(extractCommands(pr.body(), "body", pr.author()).stream(), + var allCommands = Stream.concat(extractCommands(pr.body(), "body", pr.author(), null).stream(), comments.stream() .filter(comment -> !comment.author().equals(self) || comment.body().endsWith(selfCommandMarker)) - .flatMap(c -> extractCommands(c.body(), c.id(), c.author()).stream())) + .flatMap(c -> extractCommands(c.body(), c.id(), c.author(), c).stream())) .collect(Collectors.toList()); var handled = comments.stream() diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java index 0e60f4ec0..177719129 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java @@ -38,7 +38,7 @@ public class ReviewersCommand implements CommandHandler { "committers", "committers", "committer", "committers", "authors", "authors", - "author", "author", + "author", "authors", "contributors", "contributors", "contributor", "contributors"); @@ -47,6 +47,31 @@ private void showHelp(PrintWriter reply) { "If role is set, the reviewers need to have that project role. If omitted, role defaults to `authors`."); } + private static boolean roleIsLower(String updated, String existing) { + if (existing.equals("lead")) { + return !updated.equals("lead"); + } + if (existing.equals("reviewers")) { + return !updated.equals("lead") && + !updated.equals("reviewers"); + } + if (existing.equals("committers")) { + return !updated.equals("lead") && + !updated.equals("reviewers") && + !updated.equals("committers"); + } + if (existing.equals("authors")) { + return !updated.equals("lead") && + !updated.equals("reviewers") && + !updated.equals("committers") && + !updated.equals("authors"); + } + if (existing.equals("contributors")) { + return false; + } + throw new IllegalArgumentException("Unexpected existing role: " + existing); + } + @Override public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { if (!pr.author().equals(command.user()) && !censusInstance.isReviewer(command.user())) { @@ -83,6 +108,25 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst role = roleMappings.get(splitArgs[1].toLowerCase()); } + if (pr.author().equals(command.user()) && !censusInstance.isReviewer(command.user())) { + var user = pr.repository().forge().currentUser(); + + // If the command is in the PR body there cannot be any previous commands + if (!command.isInBody()) { + var previous = ReviewersTracker.additionalRequiredReviewers(user, allComments, command.comment().get()); + if (previous.isPresent()) { + if (roleIsLower(role, previous.get().role())) { + reply.println("Cannot lower the role for additional reviewers"); + return; + } + if (numReviewers < previous.get().number()) { + reply.println("Cannot decrease the number of required reviewers"); + return; + } + } + } + } + var updatedLimits = ReviewersTracker.updatedRoleLimits(censusInstance.configuration(), numReviewers, role); if (updatedLimits.get(role) > numReviewers) { showHelp(reply); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java index a33dca173..7d27a3ac2 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java @@ -99,9 +99,14 @@ String role() { } static Optional additionalRequiredReviewers(HostUser botUser, List comments) { + return additionalRequiredReviewers(botUser, comments, null); + } + + static Optional additionalRequiredReviewers(HostUser botUser, List comments, Comment exclude) { var ret = new HashMap(); var reviewersActions = comments.stream() .filter(comment -> comment.author().equals(botUser)) + .filter(comment -> exclude == null || !comment.id().equals(exclude.id())) .map(comment -> reviewersMarkerPattern.matcher(comment.body())) .filter(Matcher::find) .collect(Collectors.toList()); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java index f14921bd4..63957917d 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java @@ -282,7 +282,54 @@ void prAuthorShouldBeAllowedToExecute(TestInfo testInfo) throws IOException { // The bot should reply with a success message TestBotRunner.runPeriodicItems(prBot); - assertLastCommentContains(authorPR,"The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers)."); + assertLastCommentContains(authorPR, "The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers)."); + } + } + + @Test + void prAuthorShouldNotBeAllowedToDecrease(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + var bot = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addReviewer(integrator.forge().currentUser().id()) + .addAuthor(author.forge().currentUser().id()); + var prBot = PullRequestBot.newBuilder().repo(bot).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", "123: This is a pull request"); + + var authorPR = author.pullRequest(pr.id()); + + // The author deems that two reviewers are required + authorPR.addComment("/reviewers 2"); + + // The bot should reply with a success message + TestBotRunner.runPeriodicItems(prBot); + assertLastCommentContains(authorPR, "The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers)."); + + // The author should not be allowed to decrease even its own /reviewers command + authorPR.addComment("/reviewers 1"); + TestBotRunner.runPeriodicItems(prBot); + assertLastCommentContains(authorPR, "Cannot decrease the number of required reviewers"); + + // Reviewer should be allowed to decrease + var reviewerPr = integrator.pullRequest(pr.id()); + reviewerPr.addComment("/reviewers 1"); + TestBotRunner.runPeriodicItems(prBot); + assertLastCommentContains(authorPR, "The number of required reviews for this PR is now set to 1"); } } } From 7b1776838535efbdcb492fc57b102acf402ff1af Mon Sep 17 00:00:00 2001 From: Erik Helin Date: Tue, 15 Sep 2020 11:44:14 +0200 Subject: [PATCH 3/3] Simplify code based on reviewer feedback --- .../skara/bots/pr/CommandInvocation.java | 16 +------------- .../skara/bots/pr/CommandWorkItem.java | 12 +++++----- .../skara/bots/pr/ReviewersCommand.java | 22 ++++++++----------- .../skara/bots/pr/ReviewersTracker.java | 5 ----- 4 files changed, 16 insertions(+), 39 deletions(-) diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java index 47eacc5c0..57dfbc687 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java @@ -11,15 +11,13 @@ class CommandInvocation { private final CommandHandler handler; private final String name; private final String args; - private final Comment comment; - CommandInvocation(String id, HostUser user, CommandHandler handler, String name, String args, Comment comment) { + CommandInvocation(String id, HostUser user, CommandHandler handler, String name, String args) { this.id = id; this.user = user; this.handler = handler; this.name = name; this.args = args != null ? args.strip() : ""; - this.comment = comment; } String id() { @@ -41,16 +39,4 @@ String name() { String args() { return args; } - - Optional comment() { - return Optional.ofNullable(comment); - } - - boolean isInBody() { - return comment == null; - } - - boolean isInComment() { - return comment != null; - } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java index 7fad79a66..887df4062 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java @@ -118,7 +118,7 @@ public String description() { } } - private List extractCommands(String text, String baseId, HostUser user, Comment c) { + private List extractCommands(String text, String baseId, HostUser user) { var ret = new ArrayList(); CommandHandler multiLineHandler = null; List multiLineBuffer = null; @@ -128,7 +128,7 @@ private List extractCommands(String text, String baseId, Host var commandMatcher = commandPattern.matcher(line); if (commandMatcher.matches()) { if (multiLineHandler != null) { - ret.add(new CommandInvocation(formatId(baseId, subId++), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer), c)); + ret.add(new CommandInvocation(formatId(baseId, subId++), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer))); multiLineHandler = null; } var command = commandMatcher.group(1).toLowerCase(); @@ -144,7 +144,7 @@ private List extractCommands(String text, String baseId, Host } multiLineCommand = command; } else { - ret.add(new CommandInvocation(formatId(baseId, subId++), user, handler, command, commandMatcher.group(2), c)); + ret.add(new CommandInvocation(formatId(baseId, subId++), user, handler, command, commandMatcher.group(2))); } } else { if (multiLineHandler != null) { @@ -153,17 +153,17 @@ private List extractCommands(String text, String baseId, Host } } if (multiLineHandler != null) { - ret.add(new CommandInvocation(formatId(baseId, subId), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer), c)); + ret.add(new CommandInvocation(formatId(baseId, subId), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer))); } return ret; } private Optional nextCommand(PullRequest pr, List comments) { var self = pr.repository().forge().currentUser(); - var allCommands = Stream.concat(extractCommands(pr.body(), "body", pr.author(), null).stream(), + var allCommands = Stream.concat(extractCommands(pr.body(), "body", pr.author()).stream(), comments.stream() .filter(comment -> !comment.author().equals(self) || comment.body().endsWith(selfCommandMarker)) - .flatMap(c -> extractCommands(c.body(), c.id(), c.author(), c).stream())) + .flatMap(c -> extractCommands(c.body(), c.id(), c.author()).stream())) .collect(Collectors.toList()); var handled = comments.stream() diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java index 177719129..fac4b16bf 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java @@ -110,19 +110,15 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst if (pr.author().equals(command.user()) && !censusInstance.isReviewer(command.user())) { var user = pr.repository().forge().currentUser(); - - // If the command is in the PR body there cannot be any previous commands - if (!command.isInBody()) { - var previous = ReviewersTracker.additionalRequiredReviewers(user, allComments, command.comment().get()); - if (previous.isPresent()) { - if (roleIsLower(role, previous.get().role())) { - reply.println("Cannot lower the role for additional reviewers"); - return; - } - if (numReviewers < previous.get().number()) { - reply.println("Cannot decrease the number of required reviewers"); - return; - } + var previous = ReviewersTracker.additionalRequiredReviewers(user, allComments); + if (previous.isPresent()) { + if (roleIsLower(role, previous.get().role())) { + reply.println("Cannot lower the role for additional reviewers"); + return; + } + if (numReviewers < previous.get().number()) { + reply.println("Cannot decrease the number of required reviewers"); + return; } } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java index 7d27a3ac2..a33dca173 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java @@ -99,14 +99,9 @@ String role() { } static Optional additionalRequiredReviewers(HostUser botUser, List comments) { - return additionalRequiredReviewers(botUser, comments, null); - } - - static Optional additionalRequiredReviewers(HostUser botUser, List comments, Comment exclude) { var ret = new HashMap(); var reviewersActions = comments.stream() .filter(comment -> comment.author().equals(botUser)) - .filter(comment -> exclude == null || !comment.id().equals(exclude.id())) .map(comment -> reviewersMarkerPattern.matcher(comment.body())) .filter(Matcher::find) .collect(Collectors.toList());