From ce2441874470822bd7bfdfe1558bbdd8d98f3472 Mon Sep 17 00:00:00 2001 From: Harish V Date: Fri, 5 May 2023 14:23:22 +0800 Subject: [PATCH 1/3] feat: return sites from db for email login --- src/routes/v2/authenticated/sites.ts | 1 - src/services/identity/SitesService.ts | 34 ++++++++++++++++++--------- src/types/repoInfo.ts | 6 ++--- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/routes/v2/authenticated/sites.ts b/src/routes/v2/authenticated/sites.ts index 4bdf886e0..506c8c374 100644 --- a/src/routes/v2/authenticated/sites.ts +++ b/src/routes/v2/authenticated/sites.ts @@ -128,7 +128,6 @@ export class SitesRouter { router.get( "/", - this.statsMiddleware.countGithubSites, this.statsMiddleware.countMigratedSites, attachReadRouteHandlerWrapper(this.getSites) ) diff --git a/src/services/identity/SitesService.ts b/src/services/identity/SitesService.ts index 2698ad80b..e10807abd 100644 --- a/src/services/identity/SitesService.ts +++ b/src/services/identity/SitesService.ts @@ -331,6 +331,21 @@ class SitesService { }) ) + // helper type guard + // note: empty strings will return false as well + const isString = (val: string | undefined): val is string => !!val + + // get sites from DB for email login users + if (!isAdminUser && isEmailUser) { + const retrievedSitesByEmail = await this.getSitesForEmailUser(userId) + const filteredValidSites = retrievedSitesByEmail.filter(isString) + + const repoData: RepositoryData[] = filteredValidSites.map((site) => ({ + repoName: site, + })) + return repoData + } + const allSites = await Promise.all( paramsArr.map(async (params) => { const { @@ -358,25 +373,22 @@ class SitesService { isPrivate, } as RepositoryData }) - .filter( - (repoData) => + .filter((repoData) => { + if (!repoData || !repoData.permissions) { + return false + } + return ( repoData.permissions.push === true && !ISOMER_ADMIN_REPOS.includes(repoData.repoName) - ) + ) + }) }) ) const flattenedAllSites = _.flatten(allSites) // Github users are using their own access token, which already filters sites to only those they have write access to // Admin users should have access to all sites regardless - if (isAdminUser || !isEmailUser) return flattenedAllSites - - // Email users need to have the list of sites filtered to those they have access to in our db, since our centralised token returns all sites - const retrievedSitesByEmail = await this.getSitesForEmailUser(userId) - - return flattenedAllSites.filter((repoData) => - retrievedSitesByEmail.includes(repoData.repoName) - ) + return flattenedAllSites } async checkHasAccessForGitHubUser(sessionData: UserWithSiteSessionData) { diff --git a/src/types/repoInfo.ts b/src/types/repoInfo.ts index a36ce7f9d..ba9ead182 100644 --- a/src/types/repoInfo.ts +++ b/src/types/repoInfo.ts @@ -15,10 +15,10 @@ export type GitHubRepositoryData = { } export type RepositoryData = { - lastUpdated: GitHubRepositoryData["pushed_at"] - permissions: GitHubRepositoryData["permissions"] + lastUpdated?: GitHubRepositoryData["pushed_at"] + permissions?: GitHubRepositoryData["permissions"] repoName: GitHubRepositoryData["name"] - isPrivate: GitHubRepositoryData["private"] + isPrivate?: GitHubRepositoryData["private"] } type SiteUrlTypes = "staging" | "prod" From aec43c4e4dd50769b43dfa3ee2a245679e469047 Mon Sep 17 00:00:00 2001 From: Harish V Date: Fri, 5 May 2023 15:12:44 +0800 Subject: [PATCH 2/3] fix: tests --- src/integration/Sites.spec.ts | 20 ------------------- .../identity/__tests__/SitesService.spec.ts | 7 ++----- 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/src/integration/Sites.spec.ts b/src/integration/Sites.spec.ts index cf8020a92..f92795fcd 100644 --- a/src/integration/Sites.spec.ts +++ b/src/integration/Sites.spec.ts @@ -220,31 +220,11 @@ describe("Sites Router", () => { const expected = { siteNames: [ { - lastUpdated: mockUpdatedAt, repoName: mockSite, - isPrivate: mockPrivate, - permissions: mockPermissions, }, ], } - mockGenericAxios.get.mockResolvedValueOnce({ - data: [ - { - pushed_at: mockUpdatedAt, - permissions: mockPermissions, - name: mockSite, - private: mockPrivate, - }, - { - pushed_at: mockUpdatedAt, - permissions: mockPermissions, - name: mockAdminSite, - private: mockPrivate, - }, - ], - }) - // Act const actual = await request(app).get("/") diff --git a/src/services/identity/__tests__/SitesService.spec.ts b/src/services/identity/__tests__/SitesService.spec.ts index fd78b6359..c0025b0ae 100644 --- a/src/services/identity/__tests__/SitesService.spec.ts +++ b/src/services/identity/__tests__/SitesService.spec.ts @@ -916,10 +916,7 @@ describe("SitesService", () => { const expectedResp: RepositoryData[] = [ { - lastUpdated: repoInfo.pushed_at, - permissions: repoInfo.permissions, repoName: repoInfo.name, - isPrivate: repoInfo.private, }, ] MockIsomerAdminsService.getByUserId.mockImplementationOnce(() => null) @@ -939,7 +936,7 @@ describe("SitesService", () => { expect(MockIsomerAdminsService.getByUserId).toHaveBeenCalledWith( mockIsomerUserId ) - expect(mockAxios.get).toHaveBeenCalledTimes(3) + expect(mockAxios.get).toHaveBeenCalledTimes(0) config.set("sites.pageCount", currRepoCount) expect(config.get("sites.pageCount")).toBe(currRepoCount) }) @@ -968,7 +965,7 @@ describe("SitesService", () => { expect(MockUsersService.findSitesByUserId).toHaveBeenCalledWith( mockIsomerUserId ) - expect(mockAxios.get).toHaveBeenCalledTimes(3) + expect(mockAxios.get).toHaveBeenCalledTimes(0) config.set("sites.pageCount", currRepoCount) expect(config.get("sites.pageCount")).toBe(currRepoCount) }) From cdf77ae09377d805e06aa9d69c3bbd9bd16442ab Mon Sep 17 00:00:00 2001 From: Harish V Date: Fri, 5 May 2023 15:23:28 +0800 Subject: [PATCH 3/3] fix: use lodash function for type guard --- src/services/identity/SitesService.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/services/identity/SitesService.ts b/src/services/identity/SitesService.ts index e10807abd..17a73c2fe 100644 --- a/src/services/identity/SitesService.ts +++ b/src/services/identity/SitesService.ts @@ -331,14 +331,10 @@ class SitesService { }) ) - // helper type guard - // note: empty strings will return false as well - const isString = (val: string | undefined): val is string => !!val - // get sites from DB for email login users if (!isAdminUser && isEmailUser) { const retrievedSitesByEmail = await this.getSitesForEmailUser(userId) - const filteredValidSites = retrievedSitesByEmail.filter(isString) + const filteredValidSites = retrievedSitesByEmail.filter(_.isString) const repoData: RepositoryData[] = filteredValidSites.map((site) => ({ repoName: site,