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.

Sign up for GitHub

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

Jump to bottom

673: PR author should be able to use /reviewers command #820

Closed
wants to merge 3 commits into from
Closed

673: PR author should be able to use /reviewers command #820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eef2478
673
edvbld Sep 14, 2020
347594f
PR authors should not be allowed to decrease
edvbld Sep 15, 2020
7b17768
Simplify code based on reviewer feedback
edvbld Sep 15, 2020
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
1 change: 1 addition & 0 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java
View file
Open in desktop
@@ -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;

Expand Down
46 changes: 43 additions & 3 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java
View file
Open in desktop
Expand Up @@ -38,7 +38,7 @@ public class ReviewersCommand implements CommandHandler {
"committers", "committers",
"committer", "committers",
"authors", "authors",
"author", "author",
"author", "authors",
"contributors", "contributors",
"contributor", "contributors");

Expand All @@ -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<Comment> 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())) {
Copy link
Member

Choose a reason for hiding this comment

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

This would suggest that my assumption is wrong, and that an author can reduce the number of reviewers required even if they are not a Reviewer. I think this might not be what we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine to let them increase it.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. It's the case of an author reducing it after a Reviewer has increased it that I think should be disallowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinrushforth and @prrace - I agree, see changes in commit 347594f

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;
}

Expand Down Expand Up @@ -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())) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe simplify this to just ! isReviewer? Including the test for is author doesn't really add value (removing it seems to better reflect the intent and might be safer / more future proof).

Copy link
Member Author

@edvbld edvbld Sep 15, 2020 •
edited

Choose a reason for hiding this comment

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

No, we don't want people who are not the PR author and who are not Reviewers to be able to increase the required number of reviewers. Allowing that makes it possible to harass pull request by e.g. registering a spam account and just drop a bunch /reviewers 9 in a number of PRs.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but that isn't related to this block (your concern is a good one and is handled by the previous block). By adding the author && to !reviewer it makes this less restrictive not more. In any case, it doesn't matter...at least not given the prior test.

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);
Expand Down
82 changes: 82 additions & 0 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
}
}