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

Re-approve when another review is requested #209

Closed
wants to merge 2 commits into from
Closed

Re-approve when another review is requested #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fcfe33
Re-approve when another review is requested
hmarr Oct 20, 2022
f12ddee
Update build artefact
hmarr Oct 20, 2022
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
46 changes: 25 additions & 21 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

57 changes: 42 additions & 15 deletions src/approve.test.ts
View file
Open in desktop
@@ -1,6 +1,5 @@
import * as core from "@actions/core";
import { Context } from "@actions/github/lib/context";
import { create } from "domain";
import nock from "nock";
import { approve } from "./approve";

Expand Down Expand Up @@ -32,6 +31,10 @@ const apiMocks = {
apiNock
.get("/repos/hmarr/test/pulls/101/reviews")
.reply(200, reviews ?? []),
getRequestedReviewers: (reviewers?: object) =>
apiNock
.get("/repos/hmarr/test/pulls/101/requested_reviewers")
.reply(200, reviewers ?? {}),
createReview: () =>
apiNock.post("/repos/hmarr/test/pulls/101/reviews").reply(200, {}),
};
Expand All @@ -40,6 +43,7 @@ test("a review is successfully created with a PAT", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews();
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", ghContext());
Expand All @@ -51,6 +55,7 @@ test("a review is successfully created with an Actions token", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews();
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", ghContext());
Expand All @@ -62,6 +67,7 @@ test("when a review is successfully created with message", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews();
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", ghContext(), undefined, "Review body");
Expand All @@ -74,6 +80,7 @@ test("when a review is successfully created using pull-request-number", async ()
apiNock
.get("/repos/hmarr/test/pulls/102")
.reply(200, { head: { sha: "24c5451bbf1fb09caa3ac8024df4788aff4d4974" } });
apiNock.get("/repos/hmarr/test/pulls/102/requested_reviewers").reply(200, []);
apiNock.get("/repos/hmarr/test/pulls/102/reviews").reply(200, []);

const createReview = apiNock
Expand All @@ -95,6 +102,7 @@ test("when a review has already been approved by current user", async () => {
state: "APPROVED",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", ghContext());
Expand All @@ -106,6 +114,23 @@ test("when a review has already been approved by current user", async () => {
)
);
});
test("when a review has already been approved but subsequently re-requested", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews([
{
user: { login: "hmarr" },
commit_id: "24c5451bbf1fb09caa3ac8024df4788aff4d4974",
state: "APPROVED",
},
]);
apiMocks.getRequestedReviewers({ users: [{ login: "hmarr" }] });
const createReview = apiMocks.createReview();

await approve("gh-tok", ghContext());

expect(createReview.isDone()).toBe(true);
});

test("when a review is pending", async () => {
apiMocks.getUser();
Expand All @@ -117,6 +142,7 @@ test("when a review is pending", async () => {
state: "PENDING",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", new Context(), 101);
Expand All @@ -134,6 +160,7 @@ test("when a review is dismissed", async () => {
state: "DISMISSED",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", new Context(), 101);
Expand All @@ -151,6 +178,7 @@ test("when a review is not approved", async () => {
state: "CHANGES_REQUESTED",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", new Context(), 101);
Expand All @@ -168,6 +196,7 @@ test("when a review is commented", async () => {
state: "COMMENTED",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", new Context(), 101);
Expand All @@ -185,6 +214,7 @@ test("when an old commit has already been approved", async () => {
state: "APPROVED",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", ghContext());
Expand All @@ -202,6 +232,7 @@ test("when a review has already been approved by another user", async () => {
state: "APPROVED",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", new Context(), 101);
Expand All @@ -219,6 +250,7 @@ test("when a review has already been approved by unknown user", async () => {
state: "APPROVED",
},
]);
apiMocks.getRequestedReviewers();
const createReview = apiMocks.createReview();

await approve("gh-tok", new Context(), 101);
Expand Down Expand Up @@ -252,6 +284,7 @@ test("when the token doesn't have write permissions", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews();
apiMocks.getRequestedReviewers();
apiNock
.post("/repos/hmarr/test/pulls/101/reviews")
.reply(403, { message: "Resource not accessible by integration" });
Expand All @@ -267,6 +300,7 @@ test("when a user tries to approve their own pull request", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews();
apiMocks.getRequestedReviewers();
apiNock
.post("/repos/hmarr/test/pulls/101/reviews")
.reply(422, { message: "Unprocessable Entity" });
Expand All @@ -278,25 +312,16 @@ test("when a user tries to approve their own pull request", async () => {
);
});

test("when pull request does not exist", async () => {
test("when pull request does not exist or the token doesn't have repo access", async () => {
apiMocks.getUser();
apiNock
.get("/repos/hmarr/test/pulls/101")
.reply(404, { message: "Not Found" });
const createReview = apiMocks.createReview();

await approve("gh-tok", ghContext());

expect(createReview.isDone()).toBe(false);
expect(core.setFailed).toHaveBeenCalledWith(
expect.stringContaining("doesn't have access")
);
});

test("when the token doesn't have read access to the repository", async () => {
apiMocks.getUser();
apiNock
.get("/repos/hmarr/test/pulls/101")
.get("/repos/hmarr/test/pulls/101/reviews")
.reply(404, { message: "Not Found" });
apiNock
.get("/repos/hmarr/test/pulls/101/requested_reviewers")
.reply(404, { message: "Not Found" });
const createReview = apiMocks.createReview();

Expand All @@ -312,6 +337,7 @@ test("when the token is read-only", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews();
apiMocks.getRequestedReviewers();
apiNock
.post("/repos/hmarr/test/pulls/101/reviews")
.reply(403, { message: "Not Authorized" });
Expand All @@ -327,6 +353,7 @@ test("when the token doesn't have write access to the repository", async () => {
apiMocks.getUser();
apiMocks.getPull();
apiMocks.getReviews();
apiMocks.getRequestedReviewers();
apiNock
.post("/repos/hmarr/test/pulls/101/reviews")
.reply(404, { message: "Not Found" });
Expand Down
54 changes: 27 additions & 27 deletions src/approve.ts
View file
Open in desktop
Expand Up @@ -30,43 +30,43 @@ export async function approve(
core.info(`Current user is ${login}`);

core.info(`Getting pull request #${prNumber} info`);
const pull_request = await client.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: prNumber,
});
const commit = pull_request.data.head.sha;
const { owner, repo } = context.repo;
const queryParams = { owner, repo, pull_number: prNumber };
const [pullRequest, reviews, requestedReviewers] = await Promise.all([
Copy link
Contributor

@vincejv vincejv Oct 20, 2022 •
edited

Choose a reason for hiding this comment

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

Hi @hmarr, while doing the fixes, I realized we can get requested_reviewers from pull_request, I was about to PR this, so no additional rest calls should be necessary, if my understanding is correct with how Github API works

for (const review of reviews.data) {
      if (
        review.user?.login == login &&
        review.commit_id == commit &&
        review.state == "APPROVED" &&
        pull_request.data
        .requested_reviewers?.filter(
          reviewer => reviewer.login == login
        ).length == 0
      ) {
        core.info(
          `Current user already approved pull request #${prNumber}, nothing to do`
        );
        return;
      }
    }

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh great point! Please do change that and submit the PR. I'd definitely rather avoid the extra request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh great point! Please do change that and submit the PR. I'd definitely rather avoid the extra request.

The code is vastly different now, so I recommend for you to do the change 😁 on how you like it, but just suggesting that we can get the field without the additional rest call.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hmarr nevermind I PRd #210, let me know your thoughts...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great, I'll close this out in favour of your pull request. Thanks for contributing!

client.rest.pulls.get(queryParams),
client.rest.pulls.listReviews(queryParams),
client.rest.pulls.listRequestedReviewers(queryParams),
]);

const commit = pullRequest.data.head.sha;
core.info(`Commit SHA is ${commit}`);

core.info(
`Getting reviews for pull request #${prNumber} and commit ${commit}`
// If the authenticated user has already approve this commit, and there
// are no outstanding review requests, then we don't need to do anything.
// Review requests show up when the review is dismissed and re-requested,
// so we do want to re-approve in that case.
const pendingReviewRequest = requestedReviewers.data?.users?.some(
(user) => user.login === login
);
const reviews = await client.rest.pulls.listReviews({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: prNumber,
});

for (const review of reviews.data) {
if (
review.user?.login == login &&
review.commit_id == commit &&
review.state == "APPROVED"
) {
core.info(
`Current user already approved pull request #${prNumber}, nothing to do`
);
return;
}
const approvalForCommit = reviews.data?.some(
(review) =>
review.user?.login === login &&
review.commit_id === commit &&
review.state === "APPROVED"
);
if (!pendingReviewRequest && approvalForCommit) {
core.info(
`Current user already approved pull request #${prNumber}, nothing to do`
);
return;
}

core.info(
`Pull request #${prNumber} has not been approved yet, creating approving review`
);
await client.rest.pulls.createReview({
owner: context.repo.owner,
repo: context.repo.repo,
owner,
repo,
pull_number: prNumber,
body: reviewMessage,
event: "APPROVE",
Expand Down