From 42f7d7b51fc6f34b7ecb539e2e1853707e156c7a Mon Sep 17 00:00:00 2001 From: rich Date: Mon, 16 Sep 2024 13:06:24 +0100 Subject: [PATCH 1/6] Adjust mapping of repo Previously we mapped the repo from its API format to the format we want in our JSON just before pushing into into our array. This meant we add to re-add properties that we just added to the repo again (eg Dependencies). Now we convert it from the API shape to our JSON shape then add the custom properties. This means we can make changes in getOpenPRsForRepo to add properties without having to update `mapRepoFromApiForStorage` too. --- utils/githubApi/fetchAllRepos.js | 10 ++++++---- utils/index.js | 2 -- utils/index.test.js | 21 --------------------- 3 files changed, 6 insertions(+), 27 deletions(-) diff --git a/utils/githubApi/fetchAllRepos.js b/utils/githubApi/fetchAllRepos.js index a9b90b5..c11567c 100644 --- a/utils/githubApi/fetchAllRepos.js +++ b/utils/githubApi/fetchAllRepos.js @@ -16,6 +16,8 @@ const fetchAllRepos = async () => { await OctokitApp.app.eachRepository(async ({ repository, octokit }) => { if (repository.archived) return; + let repo = mapRepoFromApiForStorage(repository); + await Promise.all([ await getDependenciesForRepo({ repository, @@ -25,12 +27,12 @@ const fetchAllRepos = async () => { repository, octokit, }), - ]).then(([dependencies, openPrsCount]) => { - repository.dependencies = dependencies; - repository.openPrsCount = openPrsCount; + ]).then(([dependencies, prInfo]) => { + repo.dependencies = dependencies; + repo = { ...repo, ...prInfo }; }); - repos.push(mapRepoFromApiForStorage(repository)); + repos.push(repo); }); return repos; diff --git a/utils/index.js b/utils/index.js index c658c86..9be6d4f 100644 --- a/utils/index.js +++ b/utils/index.js @@ -171,6 +171,4 @@ export const mapRepoFromApiForStorage = (repo) => ({ language: repo.language, topics: repo.topics, openIssues: repo.open_issues, - dependencies: repo.dependencies, - openPrsCount: repo.openPrsCount, }); diff --git a/utils/index.test.js b/utils/index.test.js index e085198..dbbc8dd 100644 --- a/utils/index.test.js +++ b/utils/index.test.js @@ -95,23 +95,6 @@ describe("mapRepoFromStorageToUi", () => { describe("mapRepo", () => { it("maps the repo from the data returned from the api", async () => { - const repoDependencies = [ - { - user: { - login: "some-user", - }, - pull_request: {}, - body: "Here's a pull request to manually update dependency `foo-utils 1.2.3` to `foo-utils 1.2.4`.", - }, - { - user: { - login: "renovate[bot]", - }, - pull_request: {}, - body: "Configure Renovate", - }, - ]; - const apiRepo = { id: 248204237, node_id: "MDEwOlJlcG9zaXRvcnkyNDgyMDQyMzc=", @@ -258,8 +241,6 @@ describe("mapRepoFromStorageToUi", () => { triage: false, pull: false, }, - dependencies: repoDependencies, - openPrsCount: 0, }; const repoToSave = { @@ -276,8 +257,6 @@ describe("mapRepoFromStorageToUi", () => { language: "Ruby", topics: ["delivery-plus", "internal", "tech-ops"], openIssues: 2, - dependencies: repoDependencies, - openPrsCount: 0, }; expect.deepEqual(mapRepoFromApiForStorage(apiRepo), repoToSave); From 6a1787a124aa6aca6371e9044afbb173c1aae468 Mon Sep 17 00:00:00 2001 From: rich Date: Mon, 16 Sep 2024 13:12:53 +0100 Subject: [PATCH 2/6] Make fetchOpenPrs return an object We will soon want to consume multiple values from this function so lets convert its return type to be an object in order to make it possible --- utils/githubApi/fetchOpenPrs.js | 4 +++- utils/githubApi/fetchOpenPrs.test.js | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/utils/githubApi/fetchOpenPrs.js b/utils/githubApi/fetchOpenPrs.js index 2213b0b..b724770 100644 --- a/utils/githubApi/fetchOpenPrs.js +++ b/utils/githubApi/fetchOpenPrs.js @@ -13,4 +13,6 @@ export const getOpenPRsForRepo = async ({ octokit, repository }) => { * @param {any} {data} * @returns {number} */ -export const handlePrsApiResponse = ({ data }) => data?.length || 0; +export const handlePrsApiResponse = ({ data }) => { + return { openPrCount: data?.length || 0 }; +}; diff --git a/utils/githubApi/fetchOpenPrs.test.js b/utils/githubApi/fetchOpenPrs.test.js index 3902801..c3f93af 100644 --- a/utils/githubApi/fetchOpenPrs.test.js +++ b/utils/githubApi/fetchOpenPrs.test.js @@ -3,11 +3,19 @@ import expect from "node:assert"; import { handlePrsApiResponse } from "./fetchOpenPrs.js"; describe("handlePrsApiResponse", () => { - it("returns the length of the array containing PRs", () => { - expect.equal(handlePrsApiResponse({ data: [1, 2, 3] }), 3); - }); + describe("openPrCount", () => { + it("returns the length of the array containing PRs", () => { + const actual = handlePrsApiResponse({ data: [1, 2, 3] }); + const expected = { openPrCount: 3 }; + + expect.deepEqual(actual, expected); + }); + + it("returns 0 if there are no open PRs", () => { + const actual = handlePrsApiResponse({ data: [] }); + const expected = { openPrCount: 0 }; - it("returns 0 if there are no open PRs", () => { - expect.equal(handlePrsApiResponse({ data: undefined }), 0); + expect.equal(actual, expected); + }); }); }); From 5658f101505b683f960e05367e5d82ac4034a1f9 Mon Sep 17 00:00:00 2001 From: rich Date: Mon, 16 Sep 2024 13:52:05 +0100 Subject: [PATCH 3/6] Make 'PR count' singular --- e2es/testData/repos.json | 6 +++--- index.njk | 2 +- utils/sorting.js | 6 +++--- utils/sorting.test.js | 12 ++++++------ 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/e2es/testData/repos.json b/e2es/testData/repos.json index 40e3595..7061ef2 100644 --- a/e2es/testData/repos.json +++ b/e2es/testData/repos.json @@ -13,7 +13,7 @@ "topics": ["composer", "govpress", "packagist", "php"], "openIssues": 0, "dependencies": [], - "openPrsCount": 0 + "openPrCount": 0 }, { "name": "govuk-blogs", @@ -27,7 +27,7 @@ "topics": ["govpress", "wordpress-theme"], "openIssues": 1, "dependencies": [], - "openPrsCount": 0 + "openPrCount": 0 }, { "name": "php-missing", @@ -41,7 +41,7 @@ "topics": ["composer", "govpress", "packagist", "php"], "openIssues": 7, "dependencies": [], - "openPrsCount": 2 + "openPrCount": 2 } ] } diff --git a/index.njk b/index.njk index d20f674..223ba9a 100644 --- a/index.njk +++ b/index.njk @@ -38,7 +38,7 @@ Language Topics {{macros.sortableTableHeader("Open issues count", "openIssues", sortDirection, sortBy)}} - {{macros.sortableTableHeader("Open PRs count", "openPrsCount", sortDirection, sortBy)}} + {{macros.sortableTableHeader("Open PR count", "openPrCount", sortDirection, sortBy)}} {{macros.sortableTableHeader("Updated at", "updatedAt", sortDirection, sortBy)}} Dependencies diff --git a/utils/sorting.js b/utils/sorting.js index 366fa46..ac9bb87 100644 --- a/utils/sorting.js +++ b/utils/sorting.js @@ -41,15 +41,15 @@ export const sortByUpdatedAt = (repos, sortDirection) => { /** * Sorts repositories based on the specified type and direction. - * @param {"openPrsCount"|"openIssues"} sortBy - The type of sorting to apply, either by open PRs count or open issues. + * @param {"openPrCount"|"openIssues"|} sortBy - The type of sorting to apply, either by open PRs count or open issues. * @param {SortDirection} sortDirection - The direction to sort, either "asc" for ascending or "desc" for descending. * @param {UiRepo[]} repos - An array of repository objects to sort. * @returns {UiRepo[]} The sorted array of repository objects. */ export const sortByType = (repos, sortDirection, sortBy) => { switch (sortBy) { - case "openPrsCount": - return sortByNumericValue(repos, sortDirection, "openPrsCount"); + case "openPrCount": + return sortByNumericValue(repos, sortDirection, "openPrCount"); case "openIssues": return sortByNumericValue(repos, sortDirection, "openIssues"); diff --git a/utils/sorting.test.js b/utils/sorting.test.js index cf2fca4..6a0cdc9 100644 --- a/utils/sorting.test.js +++ b/utils/sorting.test.js @@ -48,7 +48,7 @@ describe("sortByNumericValue", () => { }); }); -describe.only("sortByUpdatedAt", () => { +describe("sortByUpdatedAt", () => { it("sorts the repos by the date they were last updated in ascending order", () => { const reposToSort = [ { name: "Repo 1", updatedAtISO8601: "2022-01-01T00:00:00Z" }, @@ -81,14 +81,14 @@ describe.only("sortByUpdatedAt", () => { describe("sortByType", () => { it("sorts the repos by number of open PRs", () => { const reposToSort = [ - { name: "Repo 1", openPrsCount: 5 }, - { name: "Repo 2", openPrsCount: 3 }, - { name: "Repo 3", openPrsCount: 7 }, + { name: "Repo 1", openPrCount: 5 }, + { name: "Repo 2", openPrCount: 3 }, + { name: "Repo 3", openPrCount: 7 }, ]; expect.deepEqual( - sortByType(reposToSort, "asc", "openPrsCount"), - sortByNumericValue(reposToSort, "asc", "openPrsCount") + sortByType(reposToSort, "asc", "openPrCount"), + sortByNumericValue(reposToSort, "asc", "openPrCount") ); }); From bd1ecb91491f3d085bd6391e31efe984ac7439c4 Mon Sep 17 00:00:00 2001 From: rich Date: Mon, 16 Sep 2024 13:15:01 +0100 Subject: [PATCH 4/6] Add open bot PR count to table User need to know how many of the open PRs are automated depdency updates. This column shows how many open PRs exist for a repo where the user login is either `renovate[bot]` or `dependabot[bot]`. This may need to be finetuned later. --- e2es/testData/repos.json | 9 +++-- index.njk | 4 ++- utils/githubApi/fetchOpenPrs.js | 11 +++++- utils/githubApi/fetchOpenPrs.test.js | 52 ++++++++++++++++++++++++++-- utils/sorting.js | 5 ++- 5 files changed, 72 insertions(+), 9 deletions(-) diff --git a/e2es/testData/repos.json b/e2es/testData/repos.json index 7061ef2..a070341 100644 --- a/e2es/testData/repos.json +++ b/e2es/testData/repos.json @@ -13,7 +13,8 @@ "topics": ["composer", "govpress", "packagist", "php"], "openIssues": 0, "dependencies": [], - "openPrCount": 0 + "openPrCount": 0, + "openBotPrCount": 0 }, { "name": "govuk-blogs", @@ -27,7 +28,8 @@ "topics": ["govpress", "wordpress-theme"], "openIssues": 1, "dependencies": [], - "openPrCount": 0 + "openPrCount": 0, + "openBotPrCount": 1 }, { "name": "php-missing", @@ -41,7 +43,8 @@ "topics": ["composer", "govpress", "packagist", "php"], "openIssues": 7, "dependencies": [], - "openPrCount": 2 + "openPrCount": 2, + "openBotPrCount": 3 } ] } diff --git a/index.njk b/index.njk index 223ba9a..1f04b00 100644 --- a/index.njk +++ b/index.njk @@ -38,6 +38,7 @@ Language Topics {{macros.sortableTableHeader("Open issues count", "openIssues", sortDirection, sortBy)}} + {{macros.sortableTableHeader("Open bot PR count", "openBotPrCount", sortDirection, sortBy)}} {{macros.sortableTableHeader("Open PR count", "openPrCount", sortDirection, sortBy)}} {{macros.sortableTableHeader("Updated at", "updatedAt", sortDirection, sortBy)}} Dependencies @@ -51,7 +52,8 @@ {{ repo.language }} {{ repo.topics | join(", ")}} {{ repo.openIssues }} - {{ repo.openPrsCount }} + {{ repo.openBotPrCount }} + {{ repo.openPrCount }} {{ repo.updatedAt }} {% if repo.dependencies.length %} diff --git a/utils/githubApi/fetchOpenPrs.js b/utils/githubApi/fetchOpenPrs.js index b724770..cc77fe7 100644 --- a/utils/githubApi/fetchOpenPrs.js +++ b/utils/githubApi/fetchOpenPrs.js @@ -8,11 +8,20 @@ export const getOpenPRsForRepo = async ({ octokit, repository }) => { return octokit.request(repository.pulls_url).then(handlePrsApiResponse); }; +const depdencyUpdateBots = ["renovate[bot]", "dependabot[bot]"]; + /** * Returns the length of the PRs array or 0 if there are no PRs * @param {any} {data} * @returns {number} */ export const handlePrsApiResponse = ({ data }) => { - return { openPrCount: data?.length || 0 }; + const openBotPrCount = data.reduce((acc, pr) => { + if (depdencyUpdateBots.includes(pr?.user?.login)) { + return acc + 1; + } + return acc; + }, 0); + + return { openPrCount: data?.length || 0, openBotPrCount }; }; diff --git a/utils/githubApi/fetchOpenPrs.test.js b/utils/githubApi/fetchOpenPrs.test.js index c3f93af..aa58afb 100644 --- a/utils/githubApi/fetchOpenPrs.test.js +++ b/utils/githubApi/fetchOpenPrs.test.js @@ -6,16 +6,62 @@ describe("handlePrsApiResponse", () => { describe("openPrCount", () => { it("returns the length of the array containing PRs", () => { const actual = handlePrsApiResponse({ data: [1, 2, 3] }); - const expected = { openPrCount: 3 }; + const expected = { openPrCount: 3, openBotPrCount: 0 }; expect.deepEqual(actual, expected); }); it("returns 0 if there are no open PRs", () => { const actual = handlePrsApiResponse({ data: [] }); - const expected = { openPrCount: 0 }; + const expected = { openPrCount: 0, openBotPrCount: 0 }; - expect.equal(actual, expected); + expect.deepEqual(actual, expected); + }); + }); + + describe("openBotPrCount", () => { + it("returns 1 if there is a PR authored by dependabot", () => { + const dependabotPr = { user: { login: "dependabot[bot]" } }; + const actual = handlePrsApiResponse({ data: [dependabotPr] }); + + expect.equal(actual.openBotPrCount, 1); + }); + + it("returns 1 if there is a PR authored by renovate", () => { + const renovatePr = { user: { login: "renovate[bot]" } }; + const actual = handlePrsApiResponse({ data: [renovatePr] }); + + expect.equal(actual.openBotPrCount, 1); + }); + + it("returns 0 if there is a PR authored by other authors", () => { + const humanPr = { user: { login: "rich" } }; + const actual = handlePrsApiResponse({ data: [humanPr] }); + + expect.equal(actual.openBotPrCount, 0); + }); + + it("handles a variety of PR authors correctly", () => { + const dependabotPr = { + user: { + login: "dependabot[bot]", + }, + }; + const renovatePr = { + user: { + login: "renovate[bot]", + }, + }; + const humanPr = { + user: { + login: "rich", + }, + }; + const actual = handlePrsApiResponse({ + data: [dependabotPr, renovatePr, humanPr], + }); + + expect.equal(actual.openBotPrCount, 2); }); }); }); diff --git a/utils/sorting.js b/utils/sorting.js index ac9bb87..3f2f175 100644 --- a/utils/sorting.js +++ b/utils/sorting.js @@ -41,7 +41,7 @@ export const sortByUpdatedAt = (repos, sortDirection) => { /** * Sorts repositories based on the specified type and direction. - * @param {"openPrCount"|"openIssues"|} sortBy - The type of sorting to apply, either by open PRs count or open issues. + * @param {"openPrCount"|"openBotPrCount"|"openIssues"|} sortBy - The type of sorting to apply, either by open PRs count or open issues. * @param {SortDirection} sortDirection - The direction to sort, either "asc" for ascending or "desc" for descending. * @param {UiRepo[]} repos - An array of repository objects to sort. * @returns {UiRepo[]} The sorted array of repository objects. @@ -51,6 +51,9 @@ export const sortByType = (repos, sortDirection, sortBy) => { case "openPrCount": return sortByNumericValue(repos, sortDirection, "openPrCount"); + case "openBotPrCount": + return sortByNumericValue(repos, sortDirection, "openBotPrCount"); + case "openIssues": return sortByNumericValue(repos, sortDirection, "openIssues"); From d303dc204a8e4dfe1f1d6723d3fb53cdb976794b Mon Sep 17 00:00:00 2001 From: rich Date: Tue, 17 Sep 2024 09:04:39 +0100 Subject: [PATCH 5/6] Add scope to row headers By adding this we ensure screen readers can comprehend the table correctly --- index.njk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.njk b/index.njk index 1f04b00..d17c1c6 100644 --- a/index.njk +++ b/index.njk @@ -47,7 +47,7 @@ {% for repo in repos %} - {{ repo.name }} + {{ repo.name }} {{ repo.description }} {{ repo.language }} {{ repo.topics | join(", ")}} From cdca04730722958321beab91f98dd72a3133b9ce Mon Sep 17 00:00:00 2001 From: rich Date: Tue, 17 Sep 2024 09:04:52 +0100 Subject: [PATCH 6/6] Update E2Es --- e2es/seedTestData.js | 2 +- e2es/testData/repos.json | 8 +++--- e2es/towtruck.spec.js | 56 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 7 deletions(-) mode change 100644 => 100755 e2es/seedTestData.js diff --git a/e2es/seedTestData.js b/e2es/seedTestData.js old mode 100644 new mode 100755 index c8b39d8..b5eff69 --- a/e2es/seedTestData.js +++ b/e2es/seedTestData.js @@ -2,7 +2,7 @@ import { copyFile, mkdir } from "fs/promises"; export const createDestinationDirectory = async () => { console.info("Creating destination directory..."); - await mkdir("./data"); + await mkdir("./data", { recursive: true }); }; export const seedTestData = async () => { diff --git a/e2es/testData/repos.json b/e2es/testData/repos.json index a070341..750b656 100644 --- a/e2es/testData/repos.json +++ b/e2es/testData/repos.json @@ -11,7 +11,7 @@ "updatedAt": "2024-07-08T12:39:14Z", "language": "PHP", "topics": ["composer", "govpress", "packagist", "php"], - "openIssues": 0, + "openIssues": 10, "dependencies": [], "openPrCount": 0, "openBotPrCount": 0 @@ -28,8 +28,8 @@ "topics": ["govpress", "wordpress-theme"], "openIssues": 1, "dependencies": [], - "openPrCount": 0, - "openBotPrCount": 1 + "openPrCount": 20, + "openBotPrCount": 10 }, { "name": "php-missing", @@ -43,7 +43,7 @@ "topics": ["composer", "govpress", "packagist", "php"], "openIssues": 7, "dependencies": [], - "openPrCount": 2, + "openPrCount": 200, "openBotPrCount": 3 } ] diff --git a/e2es/towtruck.spec.js b/e2es/towtruck.spec.js index 21dbc13..0f469fb 100644 --- a/e2es/towtruck.spec.js +++ b/e2es/towtruck.spec.js @@ -13,10 +13,62 @@ test("has dependency info", async ({ page, baseURL }) => { "Language", "Last Updated", "Open issues count", - "Open PRs count", + "Open PR count", "Dependencies", ]; tableHeadings.forEach((heading) => { - page.getByRole("columnheader", { name: heading }); + expect(page.getByRole("columnheader", { name: heading })).toBeTruthy(); }); + + await testSortingForColumn( + { + name: "Open issues count", + topAscending: "govuk-blogs", + topDescending: "optionparser", + }, + page + ); + + await testSortingForColumn( + { + name: "Open bot PR count", + topAscending: "optionparse", + topDescending: "govuk-blogs", + }, + page + ); + + await testSortingForColumn( + { + name: "Open PR count", + topAscending: "optionparser", + topDescending: "php-missing", + }, + page + ); + + await testSortingForColumn( + { + name: "Updated at", + topAscending: "optionparser", + topDescending: "govuk-blogs ", + }, + page + ); }); + +const testSortingForColumn = async ( + { name, topAscending, topDescending }, + page +) => { + await page.getByRole("link", { name, exact: true }).click(); + await assertFirstDependencyRow(topAscending, page); + + await page.getByRole("link", { name, exact: true }).click(); + await assertFirstDependencyRow(topDescending, page); +}; + +const assertFirstDependencyRow = async (expectedFirstDependency, page) => { + const firstDependencyRow = page.getByRole("row").nth(1); + await expect(firstDependencyRow).toContainText(expectedFirstDependency); +};