From 788095351a1bd2cd284e7b10bccacd9e1eb001fc Mon Sep 17 00:00:00 2001
From: seaerchin <jiachin@open.gov.sg>
Date: Mon, 9 Oct 2023 16:37:00 +0800
Subject: [PATCH] refactor(ff): make flag a bool flag

---
 src/constants/featureFlags.ts                 |  1 +
 src/middleware/routeHandler.js                | 43 +++++--------------
 src/services/db/RepoService.ts                | 13 +++---
 src/services/db/__tests__/RepoService.spec.ts | 10 ++++-
 src/types/featureFlags.ts                     |  1 +
 5 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/src/constants/featureFlags.ts b/src/constants/featureFlags.ts
index 50fa910dc..f40a93d9d 100644
--- a/src/constants/featureFlags.ts
+++ b/src/constants/featureFlags.ts
@@ -1,4 +1,5 @@
 export const FEATURE_FLAGS = {
   GGS_WHITELISTED_REPOS: "ggs_whitelisted_repos",
   IS_BUILD_TIMES_REDUCTION_ENABLED: "is_build_times_reduction_enabled",
+  IS_GGS_WHITELISTED: "is_ggs_whitelisted",
 } as const
diff --git a/src/middleware/routeHandler.js b/src/middleware/routeHandler.js
index a52b48cc5..3cd8cb8be 100644
--- 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
index 270af95f4..d909654ef 100644
--- a/src/services/db/RepoService.ts
+++ b/src/services/db/RepoService.ts
@@ -101,13 +101,14 @@ export default class RepoService extends GitHubService {
     ggsWhitelistedRepos: string[]
   ): 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) {
diff --git a/src/services/db/__tests__/RepoService.spec.ts b/src/services/db/__tests__/RepoService.spec.ts
index 36375b1ea..11c1e8409 100644
--- a/src/services/db/__tests__/RepoService.spec.ts
+++ b/src/services/db/__tests__/RepoService.spec.ts
@@ -88,6 +88,9 @@ describe("RepoService", () => {
           repos: ["fake-repo", mockSiteName],
         },
       },
+      is_ggs_whitelisted: {
+        defaultValue: true,
+      },
     })
   })
 
@@ -110,13 +113,18 @@ describe("RepoService", () => {
       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.isRepoGgsWhitelisted(
         "not-whitelisted",
         RepoService.getGgsWhitelistedRepos(
           mockUserWithSiteSessionDataAndGrowthBook.growthbook
         )
       )
+
       expect(actual).toBe(false)
     })
   })
diff --git a/src/types/featureFlags.ts b/src/types/featureFlags.ts
index 67fe9298e..8e124b1b9 100644
--- a/src/types/featureFlags.ts
+++ b/src/types/featureFlags.ts
@@ -4,6 +4,7 @@
 export interface FeatureFlags {
   ggs_whitelisted_repos: { repos: string[] }
   is_build_times_reduction_enabled: boolean
+  is_ggs_whitelisted: boolean
 }
 
 // List of attributes we set in GrowthBook Instance in auth middleware