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..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 @@ -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; 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..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 @@ -38,7 +38,7 @@ public class ReviewersCommand implements CommandHandler { "committers", "committers", "committer", "committers", "authors", "authors", - "author", "author", + "author", "authors", "contributors", "contributors", "contributor", "contributors"); @@ -47,10 +47,35 @@ 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 (!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; } @@ -83,6 +108,21 @@ 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(); + 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; + } + } + } + var updatedLimits = ReviewersTracker.updatedRoleLimits(censusInstance.configuration(), numReviewers, role); if (updatedLimits.get(role) > numReviewers) { showHelp(reply); 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..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 @@ -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,85 @@ 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)."); + } + } + + @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"); + } + } }