refactor(ff): make flag a bool flag

isomerpages · Oct 9, 2023 · d18a23f · d18a23f
1 parent 5c430f0
commit d18a23f
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 137 deletions.
diff --git a/src/constants/featureFlags.ts b/src/constants/featureFlags.ts
@@ -1,3 +1,4 @@
 export const FEATURE_FLAGS = {
   GGS_WHITELISTED_REPOS: "ggs_whitelisted_repos",
+  IS_GGS_WHITELISTED: "is_ggs_whitelisted",
 } as const
diff --git a/src/middleware/routeHandler.js b/src/middleware/routeHandler.js
@@ -25,14 +25,13 @@ const gitFileSystemService = new GitFileSystemService(
   new SimpleGit({ maxConcurrentProcesses: MAX_CONCURRENT_GIT_PROCESSES })
 )
 
-const isRepoWhitelisted = (siteName, ggsWhitelistedRepos) => {
+const isRepoWhitelisted = (siteName, growthbook) => {
   // TODO: adding log to simplify debugging, to be removed after stabilising
-  logger.info(
-    `Checking if ${siteName} is GGS whitelisted: ${ggsWhitelistedRepos.includes(
-      siteName
-    )}`
-  )
-  ggsWhitelistedRepos.includes(siteName)
+  logger.info(`Evaluating if ${siteName} is GGS whitelisted.`)
+  // NOTE: Growthbook has to be optional
+  // as we cannot guarantee its presence on `sessionData`.
+  // It is present iff there is a `siteHandler` attached.
+  return !!growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_WHITELISTED, false)
 }
 
 const handleGitFileLock = async (repoName, next) => {
@@ -74,23 +73,15 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async (
   const { siteName } = req.params
   const { growthbook } = req
 
-  let ggsWhitelistedRepos = { repos: [] }
-  if (growthbook) {
-    ggsWhitelistedRepos = growthbook.getFeatureValue(
-      FEATURE_FLAGS.GGS_WHITELISTED_REPOS,
-      {
-        repos: [],
-      }
-    )
-  }
-
   let isGitAvailable = true
+
   // only check git file lock if the repo is whitelisted
-  if (isRepoWhitelisted(siteName, ggsWhitelistedRepos.repos)) {
+  if (isRepoWhitelisted(siteName, growthbook)) {
     isGitAvailable = await handleGitFileLock(siteName, next)
   }
 
   if (!isGitAvailable) return
+
   try {
     await lock(siteName)
   } catch (err) {
@@ -102,6 +93,7 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async (
     await unlock(siteName)
     next(err)
   })
+
   try {
     await unlock(siteName)
   } catch (err) {
@@ -120,20 +112,7 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async (
   const { accessToken } = userSessionData
   const { growthbook } = req
 
-  let ggsWhitelistedRepos = { repos: [] }
-  if (growthbook) {
-    ggsWhitelistedRepos = growthbook.getFeatureValue(
-      FEATURE_FLAGS.GGS_WHITELISTED_REPOS,
-      {
-        repos: [],
-      }
-    )
-  }
-
-  const shouldUseGitFileSystem = isRepoWhitelisted(
-    siteName,
-    ggsWhitelistedRepos.repos
-  )
+  const shouldUseGitFileSystem = isRepoWhitelisted(siteName, growthbook)
 
   const isGitAvailable = await handleGitFileLock(siteName, next)
   if (!isGitAvailable) return

diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts
@@ -38,27 +38,19 @@ export default class RepoService extends GitHubService {
     this.gitFileSystemService = gitFileSystemService
   }
 
-  getGgsWhitelistedRepos(
-    growthbook: GrowthBook<FeatureFlags> | undefined
-  ): string[] {
-    if (!growthbook) return []
-
-    const whitelistedRepos = growthbook.getFeatureValue(
-      FEATURE_FLAGS.GGS_WHITELISTED_REPOS,
-      { repos: [] }
-    )
-    return whitelistedRepos.repos
-  }
-
-  isRepoWhitelisted(repoName: string, ggsWhitelistedRepos: string[]): boolean {
+  isRepoWhitelisted(
+    repoName: string,
+    growthbook?: GrowthBook<FeatureFlags>
+  ): boolean {
     // TODO: Adding for initial debugging if required. Remove once stabilised
-    logger.info(
-      `Evaluating if ${repoName} is GGS whitelisted: ${ggsWhitelistedRepos.includes(
-        repoName
-      )}`
+    logger.info(`Evaluating if ${repoName} is GGS whitelisted.`)
+    // NOTE: Growthbook has to be optional
+    // as we cannot guarantee its presence on `sessionData`.
+    // It is present iff there is a `siteHandler` attached.
+    return !!growthbook?.getFeatureValue(
+      FEATURE_FLAGS.IS_GGS_WHITELISTED,
+      false
     )
-
-    return ggsWhitelistedRepos.includes(repoName)
   }
 
   getCommitDiff(siteName: string, base?: string, head?: string) {
@@ -152,12 +144,7 @@ export default class RepoService extends GitHubService {
       isMedia?: boolean
     }
   ): Promise<{ sha: string }> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info(
         `Writing file to local Git file system - Site name: ${sessionData.siteName}, directory name: ${directoryName}, file name: ${fileName}`
       )
@@ -189,12 +176,7 @@ export default class RepoService extends GitHubService {
     sessionData: UserWithSiteSessionData,
     { fileName, directoryName }: { fileName: string; directoryName?: string }
   ): Promise<GitFile> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info("Reading file from local Git file system")
       const filePath = directoryName ? `${directoryName}/${fileName}` : fileName
       const result = await this.gitFileSystemService.read(
@@ -224,12 +206,7 @@ export default class RepoService extends GitHubService {
     const { siteName } = sessionData
 
     // fetch from local disk
-    if (
-      this.isRepoWhitelisted(
-        siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) {
       logger.info(
         `Reading media file from disk. Site name: ${siteName}, directory name: ${directoryName}, file name: ${fileName},`
       )
@@ -274,12 +251,7 @@ export default class RepoService extends GitHubService {
     sessionData: UserWithSiteSessionData,
     { directoryName }: { directoryName: string }
   ): Promise<GitDirectoryItem[]> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info("Reading directory from local Git file system")
       const result = await this.gitFileSystemService.listDirectoryContents(
         sessionData.siteName,
@@ -311,12 +283,7 @@ export default class RepoService extends GitHubService {
       (file.type === "file" || file.type === "dir") &&
       file.name !== PLACEHOLDER_FILE_NAME
 
-    if (
-      this.isRepoWhitelisted(
-        siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) {
       const result = await this.gitFileSystemService.listDirectoryContents(
         siteName,
         directoryName
@@ -367,12 +334,7 @@ export default class RepoService extends GitHubService {
       directoryName?: string
     }
   ): Promise<GitCommitResult> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info("Updating file in local Git file system")
       const filePath = directoryName ? `${directoryName}/${fileName}` : fileName
       const result = await this.gitFileSystemService.update(
@@ -411,12 +373,7 @@ export default class RepoService extends GitHubService {
       githubSessionData: GithubSessionData
     }
   ): Promise<void> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info(
         `Deleting directory in local Git file system for repo: ${sessionData.siteName}, directory name: ${directoryName}`
       )
@@ -475,12 +432,7 @@ export default class RepoService extends GitHubService {
       directoryName: string
     }
   ): Promise<void> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info(
         `Deleting file in local Git file system for repo: ${sessionData.siteName}, directory name: ${directoryName}, file name: ${fileName}`
       )
@@ -518,12 +470,7 @@ export default class RepoService extends GitHubService {
     newPath: string,
     message?: string
   ): Promise<GitCommitResult> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info("Renaming file/directory in local Git file system")
       const result = await this.gitFileSystemService.renameSinglePath(
         sessionData.siteName,
@@ -608,12 +555,7 @@ export default class RepoService extends GitHubService {
     targetFiles: string[],
     message?: string
   ): Promise<GitCommitResult> {
-    if (
-      this.isRepoWhitelisted(
-        sessionData.siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(sessionData.siteName, sessionData.growthbook)) {
       logger.info("Moving files in local Git file system")
       const result = await this.gitFileSystemService.moveFiles(
         sessionData.siteName,
@@ -697,12 +639,7 @@ export default class RepoService extends GitHubService {
     branchName: string
   ): Promise<GitHubCommitData> {
     const { siteName } = sessionData
-    if (
-      this.isRepoWhitelisted(
-        siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) {
       logger.info(
         `Getting latest commit of branch ${branchName} for site ${siteName} from local Git file system`
       )
@@ -747,12 +684,7 @@ export default class RepoService extends GitHubService {
     }: { commitSha: string; branchName?: string }
   ): Promise<void> {
     const { siteName } = sessionData
-    if (
-      this.isRepoWhitelisted(
-        siteName,
-        this.getGgsWhitelistedRepos(sessionData.growthbook)
-      )
-    ) {
+    if (this.isRepoWhitelisted(siteName, sessionData.growthbook)) {
       logger.info(
         `Updating repo state for site ${siteName} to ${commitSha} on local Git file system`
       )

diff --git a/src/services/db/__tests__/RepoService.spec.ts b/src/services/db/__tests__/RepoService.spec.ts
@@ -66,35 +66,34 @@ describe("RepoService", () => {
           repos: ["fake-repo", mockSiteName],
         },
       },
+      is_ggs_whitelisted: {
+        defaultValue: true,
+      },
     })
   })
 
   describe("isRepoWhitelisted", () => {
-    it("should indicate whitelisted repos as whitelisted correctly", () => {
-      const actual1 = RepoService.isRepoWhitelisted(
-        "fake-repo",
-        RepoService.getGgsWhitelistedRepos(
-          mockUserWithSiteSessionDataAndGrowthBook.growthbook
-        )
-      )
+    it("should indicate that repos are whitelisted by default", () => {
+      const actual1 = RepoService.isRepoWhitelisted("fake-repo", mockGrowthBook)
       expect(actual1).toBe(true)
 
       const actual2 = RepoService.isRepoWhitelisted(
         mockSiteName,
-        RepoService.getGgsWhitelistedRepos(
-          mockUserWithSiteSessionDataAndGrowthBook.growthbook
-        )
+        mockGrowthBook
       )
       expect(actual2).toBe(true)
     })
 
-    it("should indicate non-whitelisted repos as non-whitelisted correctly", () => {
+    it("should indicate blacklisted repos as not being allowed", () => {
+      // NOTE: Mock a blacklisted repo
+      const gbSpy = jest.spyOn(mockGrowthBook, "getFeatureValue")
+      gbSpy.mockReturnValueOnce(false)
+
       const actual = RepoService.isRepoWhitelisted(
         "not-whitelisted",
-        RepoService.getGgsWhitelistedRepos(
-          mockUserWithSiteSessionDataAndGrowthBook.growthbook
-        )
+        mockGrowthBook
       )
+
       expect(actual).toBe(false)
     })
   })

diff --git a/src/types/featureFlags.ts b/src/types/featureFlags.ts
@@ -3,6 +3,7 @@
 // Note: key should mirror GrowthBook exactly as it is
 export interface FeatureFlags {
   ggs_whitelisted_repos: { repos: string[] }
+  is_ggs_whitelisted: boolean
 }
 
 // List of attributes we set in GrowthBook Instance in auth middleware