From a2988bc72cff40d2d531fce6d1d94df59d911563 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 3 Oct 2023 19:14:41 +0200 Subject: [PATCH] Fixed bug where only 30 members where shown (#90) 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 #88 --- action.yml | 2 +- package.json | 2 +- src/github/teams.ts | 8 ++++++-- src/test/index.test.ts | 15 +++++++-------- src/test/teams.test.ts | 29 ++++++++++++++++------------- 5 files changed, 31 insertions(+), 25 deletions(-) diff --git a/action.yml b/action.yml index 7555a3b..26f3030 100644 --- a/action.yml +++ b/action.yml @@ -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' diff --git a/package.json b/package.json index 19330b1..3f4c5b1 100644 --- a/package.json +++ b/package.json @@ -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": { diff --git a/src/github/teams.ts b/src/github/teams.ts index ca6c0f5..6591170 100644 --- a/src/github/teams.ts +++ b/src/github/teams.ts @@ -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); } diff --git a/src/test/index.test.ts b/src/test/index.test.ts index c25b9fc..42bef94 100644 --- a/src/test/index.test.ts +++ b/src/test/index.test.ts @@ -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"; @@ -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 diff --git a/src/test/teams.test.ts b/src/test/teams.test.ts index 7a24c79..1cda0b0 100644 --- a/src/test/teams.test.ts +++ b/src/test/teams.test.ts @@ -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"; @@ -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 () => { @@ -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); }); });