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.

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

feat: return sites from db for email login #754

Merged
merged 3 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 0 additions & 20 deletions src/integration/Sites.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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("/")

Expand Down
1 change: 0 additions & 1 deletion src/routes/v2/authenticated/sites.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ export class SitesRouter {

router.get(
"/",
this.statsMiddleware.countGithubSites,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the countGithubSites middleware to prevent excessive calls to github

cc @isomerpages/iso-engineers

this.statsMiddleware.countMigratedSites,
attachReadRouteHandlerWrapper(this.getSites)
)
Expand Down
34 changes: 23 additions & 11 deletions src/services/identity/SitesService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
harishv7 marked this conversation as resolved.
Show resolved Hide resolved

// 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 {
Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 2 additions & 5 deletions src/services/identity/__tests__/SitesService.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
})
Expand Down Expand Up @@ -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)
})
Expand Down
6 changes: 3 additions & 3 deletions src/types/repoInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Comment on lines +18 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm actually removing isPrivate has implications specifically for loading homepages on private repos - without this param available the images on editHomepage don't load properly. I feel like this can be a future fix though since the affected repos should be small, but let's check with prod ops first if any existing email login sites are private

Copy link
Contributor Author

@harishv7 harishv7 May 5, 2023

Choose a reason for hiding this comment

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

We should then be tracking this info in our DB right? Should we add a column to track this?

But yes, for now, lets check with Ops

}

type SiteUrlTypes = "staging" | "prod"
Expand Down