Skip to content

Commit

Permalink
Fixed bug where only 30 members where shown (paritytech#90)
Browse files Browse the repository at this point in the history
Fixed a pagination issue where the system was only obtaining the first
30 membes of a team.

By adding the [pagination method of
Octokit](https://actions-cool.github.io/octokit-rest/guide/05_pagination/)
we can obtain all of the results at once.

Updated project version to 2.0.1 to generate a new release.

This fixes paritytech#88
  • Loading branch information
Bullrich authored Oct 3, 2023
1 parent c7b8f9b commit a2988bc
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 25 deletions.
2 changes: 1 addition & 1 deletion action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ outputs:

runs:
using: 'docker'
image: 'docker://ghcr.io/paritytech/review-bot/action:2.0.0'
image: 'docker://ghcr.io/paritytech/review-bot/action:2.0.1'
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "review-bot",
"version": "2.0.0",
"version": "2.0.1",
"description": "Have custom review rules for PRs with auto assignment",
"main": "src/index.ts",
"scripts": {
Expand Down
8 changes: 6 additions & 2 deletions src/github/teams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@ export class GitHubTeamsApi implements TeamApi {
// We first verify that this information hasn't been fetched yet
if (!this.teamsCache.has(teamName)) {
this.logger.debug(`Fetching team '${teamName}'`);
const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName });
const members = data.map((d) => d.login);

const membersRequest = await this.api.paginate(this.api.rest.teams.listMembersInOrg, {
org: this.org,
team_slug: teamName,
});
const members = membersRequest.map((m) => m.login);
this.logger.debug(`Members are ${JSON.stringify(members)}`);
this.teamsCache.set(teamName, members);
}
Expand Down
15 changes: 7 additions & 8 deletions src/test/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable @typescript-eslint/ban-ts-comment */
import { PullRequest, PullRequestReview } from "@octokit/webhooks-types";
import { existsSync, openSync, readFileSync, unlinkSync } from "fs";
import { DeepMockProxy, Matcher, mock, mockDeep, MockProxy } from "jest-mock-extended";
import { DeepMockProxy, mock, mockDeep, MockProxy } from "jest-mock-extended";
import { join } from "path";

import { GitHubChecksApi } from "../github/check";
Expand Down Expand Up @@ -89,16 +89,15 @@ describe("Integration testing", () => {
client.rest.repos.getContent.mockResolvedValue({ data: { content: Buffer.from(config, "utf-8") } });
mockReviews([]);
for (const [teamName, members] of teamsMembers) {
client.rest.teams.listMembersInOrg
// @ts-ignore as the error is related to the matcher type
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-call
.calledWith(new Matcher<{ team_slug: string }>((value) => value.team_slug === teamName, "Different team name"))
.mockResolvedValue({
client.paginate
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
.calledWith(client.rest.teams.listMembersInOrg, expect.objectContaining({ team_slug: teamName }))
.mockResolvedValue(
// @ts-ignore as we don't need the full type
data: members.map((m) => {
members.map((m) => {
return { login: m };
}),
});
);
}

// @ts-ignore missing more of the required types
Expand Down
29 changes: 16 additions & 13 deletions src/test/teams.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable @typescript-eslint/no-unsafe-argument */
/* eslint-disable @typescript-eslint/ban-ts-comment */
import { DeepMockProxy, Matcher, mock, mockDeep, MockProxy } from "jest-mock-extended";
import { DeepMockProxy, mock, mockDeep, MockProxy } from "jest-mock-extended";

import { GitHubTeamsApi } from "../github/teams";
import { ActionLogger, GitHubClient, TeamApi } from "../github/types";
Expand All @@ -17,41 +18,43 @@ describe("Pull Request API Tests", () => {

test("should call team", async () => {
// @ts-ignore
client.rest.teams.listMembersInOrg.mockResolvedValue({ data: [] });
client.paginate.mockResolvedValue([]);
await teams.getTeamMembers("example");
expect(client.rest.teams.listMembersInOrg).toHaveBeenCalledWith({ org: "org", team_slug: "example" });
expect(client.paginate).toHaveBeenCalledWith(client.rest.teams.listMembersInOrg, {
org: "org",
team_slug: "example",
});
});

test("should return team members", async () => {
// @ts-ignore
client.rest.teams.listMembersInOrg.mockResolvedValue({ data: [{ login: "abc" }, { login: "bcd" }] });
client.paginate.mockResolvedValue([{ login: "abc" }, { login: "bcd" }]);
const members = await teams.getTeamMembers("example");
expect(members).toEqual(["abc", "bcd"]);
});

test("should cache team members call", async () => {
// @ts-ignore
client.rest.teams.listMembersInOrg.mockResolvedValue({ data: [{ login: "abc" }, { login: "bcd" }] });
client.paginate.mockResolvedValue([{ login: "abc" }, { login: "bcd" }]);
for (let i = 0; i < 10; i++) {
const members = await teams.getTeamMembers("example");
expect(members).toEqual(["abc", "bcd"]);
}
expect(client.rest.teams.listMembersInOrg).toHaveBeenCalledTimes(1);
expect(client.paginate).toHaveBeenCalledTimes(1);
});

/**
* Helper class that evades the compiler errors
*/
const mockTeamMembers = (teamName: string, members: string[]) => {
client.rest.teams.listMembersInOrg
// @ts-ignore as the error is related to the matcher type
.calledWith(new Matcher<{ team_slug: string }>((value) => value.team_slug === teamName, "Different team name"))
.mockResolvedValue({
client.paginate
.calledWith(client.rest.teams.listMembersInOrg, expect.objectContaining({ team_slug: teamName }))
.mockResolvedValue(
// @ts-ignore as we don't need the full type
data: members.map((m) => {
members.map((m) => {
return { login: m };
}),
});
);
};

test("should call different teams", async () => {
Expand All @@ -76,6 +79,6 @@ describe("Pull Request API Tests", () => {
expect(team3).toEqual(["qwerty", "dvorak"]);
}

expect(client.rest.teams.listMembersInOrg).toHaveBeenCalledTimes(3);
expect(client.paginate).toHaveBeenCalledTimes(3);
});
});

0 comments on commit a2988bc

Please sign in to comment.