Use Record instead of Map for user settings

The syntax of using Map was getting frustrating for tests, so use Record instead. Record is just a javascript object, and so keys are always strings (despite the type of Record<>), and so just use strings for IDs within user settings keys.
symphoniacloud · Sep 16, 2024 · c6f8ad2 · c6f8ad2
1 parent badb704
commit c6f8ad2
Show file tree

Hide file tree

Showing 15 changed files with 647 additions and 317 deletions.
diff --git a/src/app/domain/entityStore/entities/UserSettingsEntity.ts b/src/app/domain/entityStore/entities/UserSettingsEntity.ts
@@ -1,37 +1,10 @@
-import { entityFromPkOnlyEntity } from '@symphoniacloud/dynamodb-entity-store'
+import { entityFromPkOnlyEntity, typePredicateParser } from '@symphoniacloud/dynamodb-entity-store'
 import { USER_SETTINGS } from '../entityTypes'
-import { PersistedUserSettings } from '../../types/UserSettings'
-import { DynamoDBValues } from '@symphoniacloud/dynamodb-entity-store/dist/cjs/entities'
-import { parseDynamoDBDictToMap } from '../entityStoreEntitySupport'
-import { identity } from '../../../util/functional'
-
-export function parseUserSettings(item: DynamoDBValues): PersistedUserSettings {
-  // TODO - check things actually exist
-  return {
-    userId: item.userId,
-    github: {
-      accounts: parseDynamoDBDictToMap(
-        Number.parseInt,
-        (value) => ({
-          ...value,
-          repos: parseDynamoDBDictToMap(
-            Number.parseInt,
-            (value) => ({
-              ...value,
-              workflows: parseDynamoDBDictToMap(Number.parseInt, identity, value.workflows ?? {})
-            }),
-            value.repos ?? {}
-          )
-        }),
-        item.github.accounts
-      )
-    }
-  }
-}
+import { isUserSettings, PersistedUserSettings } from '../../types/UserSettings'
 
 export const UserSettingsEntity = entityFromPkOnlyEntity({
   type: USER_SETTINGS,
-  parse: parseUserSettings,
+  parse: typePredicateParser(isUserSettings, USER_SETTINGS),
   pk(source: Pick<PersistedUserSettings, 'userId'>) {
     return `USERID#${source.userId}`
   }

diff --git a/src/app/domain/types/UserSettings.ts b/src/app/domain/types/UserSettings.ts
@@ -1,4 +1,4 @@
-import { GithubAccountId, GithubRepoId, GithubUserId, GithubWorkflowId, isGithubUserId } from './GithubKeys'
+import { GithubUserId, isGithubUserId } from './GithubKeys'
 import { isNotNullObject } from '../../util/types'
 
 export type UserSetting = 'visible' | 'notify'
@@ -19,10 +19,16 @@ export function isUserSettings(x: unknown): x is PersistedUserSettings {
   )
 }
 
+// Various types here use a string for Record key, rather than GithubAccountID, etc.
+// That's because JavaScript always uses strings for object keys, even if the Record type says number
+// An earlier version of this code used Maps to keep the strong typing of the ID keys
+// but it made the code more verbose - especially for tests - so I decided just to suck it up
+// and go with string ID keys
+
 export interface PersistedUserSettings {
   userId: GithubUserId
   github: {
-    accounts: Map<GithubAccountId, PersistedGithubAccountSettings>
+    accounts: Record<string, PersistedGithubAccountSettings>
   }
 }
 
@@ -32,11 +38,11 @@ export interface PersistedVisibleAndNotifyConfigurable {
 }
 
 export interface PersistedGithubAccountSettings extends PersistedVisibleAndNotifyConfigurable {
-  repos: Map<GithubRepoId, PersistedGithubRepoSettings>
+  repos: Record<string, PersistedGithubRepoSettings>
 }
 
 export interface PersistedGithubRepoSettings extends PersistedVisibleAndNotifyConfigurable {
-  workflows: Map<GithubWorkflowId, PersistedGithubWorkflowSettings>
+  workflows: Record<string, PersistedGithubWorkflowSettings>
 }
 
 export type PersistedGithubWorkflowSettings = PersistedVisibleAndNotifyConfigurable
@@ -46,27 +52,28 @@ export type PersistedGithubWorkflowSettings = PersistedVisibleAndNotifyConfigura
 export interface CalculatedUserSettings {
   userId: GithubUserId
   github: {
-    accounts: Map<GithubAccountId, CalculatedGithubAccountSettings>
+    accounts: Record<string, CalculatedGithubAccountSettings>
   }
 }
 
 export type CalculatedVisibleAndNotifyConfigurable = Required<PersistedVisibleAndNotifyConfigurable>
 
 export interface CalculatedGithubAccountSettings extends CalculatedVisibleAndNotifyConfigurable {
-  repos: Map<GithubRepoId, CalculatedGithubRepoSettings>
+  repos: Record<string, CalculatedGithubRepoSettings>
 }
 
 export interface CalculatedGithubRepoSettings extends CalculatedVisibleAndNotifyConfigurable {
-  workflows: Map<GithubWorkflowId, CalculatedGithubWorkflowSettings>
+  workflows: Record<string, CalculatedGithubWorkflowSettings>
 }
 
 export type CalculatedGithubWorkflowSettings = CalculatedVisibleAndNotifyConfigurable
 
 // ***
 
 export interface DisplayableUserSettings {
+  userId: GithubUserId
   github: {
-    accounts: Map<GithubAccountId, DisplayableGithubAccountSettings>
+    accounts: Record<string, DisplayableGithubAccountSettings>
   }
 }
 
@@ -77,11 +84,11 @@ export interface Displayable {
 export interface DisplayableGithubAccountSettings
   extends CalculatedVisibleAndNotifyConfigurable,
     Displayable {
-  repos: Map<GithubRepoId, DisplayableGithubRepoSettings>
+  repos: Record<string, DisplayableGithubRepoSettings>
 }
 
 export interface DisplayableGithubRepoSettings extends CalculatedVisibleAndNotifyConfigurable, Displayable {
-  workflows: Map<GithubWorkflowId, DisplayableGithubWorkflowSettings>
+  workflows: Record<string, DisplayableGithubWorkflowSettings>
 }
 
 export type DisplayableGithubWorkflowSettings = CalculatedGithubWorkflowSettings & Displayable
diff --git a/src/app/domain/user/calculatedUserSettings.ts b/src/app/domain/user/calculatedUserSettings.ts
@@ -1,7 +1,6 @@
 import {
   CalculatedGithubAccountSettings,
   CalculatedGithubRepoSettings,
-  CalculatedGithubWorkflowSettings,
   CalculatedUserSettings,
   CalculatedVisibleAndNotifyConfigurable,
   PersistedGithubAccountSettings,
@@ -10,7 +9,7 @@ import {
   PersistedVisibleAndNotifyConfigurable
 } from '../types/UserSettings'
 import { GithubWorkflow } from '../types/GithubWorkflow'
-import { GithubAccountId, GithubRepoId, GithubRepoKey, GithubWorkflowId } from '../types/GithubKeys'
+import { GithubAccountId, GithubRepoKey } from '../types/GithubKeys'
 import {
   findUniqueAccountIds,
   findUniqueRepoIdsForAccount,
@@ -23,16 +22,14 @@ export function calculateUserSettings(
   settings: PersistedUserSettings,
   allWorkflows: GithubWorkflow[]
 ): CalculatedUserSettings {
-  const accountEntries: [GithubAccountId, CalculatedGithubAccountSettings][] = findUniqueAccountIds(
-    allWorkflows
-  ).map((id) => {
-    return [id, calculateAccountSettings(settings.github.accounts.get(id), id, allWorkflows)]
-  })
-
   return {
     userId: settings.userId,
     github: {
-      accounts: new Map<GithubAccountId, CalculatedGithubAccountSettings>(accountEntries)
+      accounts: Object.fromEntries(
+        findUniqueAccountIds(allWorkflows).map((id) => {
+          return [id, calculateAccountSettings(settings.github.accounts[id], id, allWorkflows)]
+        })
+      )
     }
   }
 }
@@ -43,27 +40,23 @@ export function calculateAccountSettings(
   allWorkflows: GithubWorkflow[]
 ): CalculatedGithubAccountSettings {
   const visibleAndNotify = calculateVisibleAndNotifyConfigurable(settings, DEFAULT_ACCOUNT_NOTIFY)
+  const repos = visibleAndNotify.visible
+    ? Object.fromEntries(
+        findUniqueRepoIdsForAccount(allWorkflows, accountId).map((repoId) => {
+          return [
+            repoId,
+            calculateRepoSettings(
+              settings?.repos[repoId],
+              { ownerId: accountId, repoId },
+              allWorkflows,
+              visibleAndNotify.notify
+            )
+          ]
+        })
+      )
+    : {}
 
-  if (!visibleAndNotify.visible) {
-    return { ...visibleAndNotify, repos: new Map<GithubRepoId, CalculatedGithubRepoSettings>() }
-  }
-
-  return {
-    ...visibleAndNotify,
-    repos: new Map<GithubRepoId, CalculatedGithubRepoSettings>(
-      findUniqueRepoIdsForAccount(allWorkflows, accountId).map((repoId) => {
-        return [
-          repoId,
-          calculateRepoSettings(
-            settings?.repos.get(repoId),
-            { ownerId: accountId, repoId },
-            allWorkflows,
-            visibleAndNotify.notify
-          )
-        ]
-      })
-    )
-  }
+  return { ...visibleAndNotify, repos }
 }
 
 export function calculateRepoSettings(
@@ -73,22 +66,18 @@ export function calculateRepoSettings(
   defaultNotify: boolean
 ): CalculatedGithubRepoSettings {
   const visibleAndNotify = calculateVisibleAndNotifyConfigurable(repoSettings, defaultNotify)
+  const workflows = visibleAndNotify.visible
+    ? Object.fromEntries(
+        findWorkflowsForRepo(allWorkflows, repoKey).map((wf) => {
+          return [
+            wf.workflowId,
+            calculateWorkflowSettings(repoSettings?.workflows[wf.workflowId], visibleAndNotify.notify)
+          ]
+        })
+      )
+    : {}
 
-  if (!visibleAndNotify.visible) {
-    return { ...visibleAndNotify, workflows: new Map<GithubWorkflowId, CalculatedGithubWorkflowSettings>() }
-  }
-
-  return {
-    ...visibleAndNotify,
-    workflows: new Map<GithubWorkflowId, CalculatedGithubWorkflowSettings>(
-      findWorkflowsForRepo(allWorkflows, repoKey).map((wf) => {
-        return [
-          wf.workflowId,
-          calculateWorkflowSettings(repoSettings?.workflows.get(wf.workflowId), visibleAndNotify.notify)
-        ]
-      })
-    )
-  }
+  return { ...visibleAndNotify, workflows }
 }
 
 export const calculateWorkflowSettings = calculateVisibleAndNotifyConfigurable

diff --git a/src/app/domain/user/displayableUserSettings.ts b/src/app/domain/user/displayableUserSettings.ts
@@ -10,13 +10,7 @@ import {
   PersistedUserSettings
 } from '../types/UserSettings'
 import { GithubWorkflow } from '../types/GithubWorkflow'
-import {
-  GithubAccountId,
-  GithubRepoId,
-  GithubRepoKey,
-  GithubWorkflowId,
-  GithubWorkflowKey
-} from '../types/GithubKeys'
+import { GithubAccountId, GithubRepoKey, GithubWorkflowKey } from '../types/GithubKeys'
 import { findAccountName, findRepoName, findWorkflowName } from '../github/githubWorkflow'
 import { calculateUserSettings } from './calculatedUserSettings'
 
@@ -32,11 +26,12 @@ function toDisplayableUserSettings(
   workflows: GithubWorkflow[]
 ): DisplayableUserSettings {
   return {
+    userId: userSettings.userId,
     github: {
-      accounts: new Map<GithubAccountId, DisplayableGithubAccountSettings>(
-        Array.from(userSettings.github.accounts.entries()).map(([accountId, accountSettings]) => [
+      accounts: Object.fromEntries(
+        Object.entries(userSettings.github.accounts).map(([accountId, accountSettings]) => [
           accountId,
-          toDisplayableAccountSettings(accountId, accountSettings, workflows)
+          toDisplayableAccountSettings(Number(accountId), accountSettings, workflows)
         ])
       )
     }
@@ -51,13 +46,13 @@ export function toDisplayableAccountSettings(
   return {
     ...accountSettings,
     name: findAccountName(allWorkflows, accountId),
-    repos: new Map<GithubRepoId, DisplayableGithubRepoSettings>(
-      Array.from(accountSettings.repos.entries()).map(([repoId, repoSettings]) => [
+    repos: Object.fromEntries(
+      Object.entries(accountSettings.repos).map(([repoId, repoSettings]) => [
         repoId,
         toDisplayableRepoSettings(
           {
             ownerId: accountId,
-            repoId
+            repoId: Number(repoId)
           },
           repoSettings,
           allWorkflows
@@ -75,10 +70,14 @@ export function toDisplayableRepoSettings(
   return {
     ...repoSettings,
     name: findRepoName(allWorkflows, repoKey),
-    workflows: new Map<GithubWorkflowId, DisplayableGithubWorkflowSettings>(
-      Array.from(repoSettings.workflows.entries()).map(([workflowId, workflowSettings]) => [
+    workflows: Object.fromEntries(
+      Object.entries(repoSettings.workflows).map(([workflowId, workflowSettings]) => [
         workflowId,
-        toDisplayableWorkflowSettings({ ...repoKey, workflowId }, workflowSettings, allWorkflows)
+        toDisplayableWorkflowSettings(
+          { ...repoKey, workflowId: Number(workflowId) },
+          workflowSettings,
+          allWorkflows
+        )
       ])
     )
   }

diff --git a/src/app/domain/user/persistedUserSettings.ts b/src/app/domain/user/persistedUserSettings.ts
@@ -6,16 +6,9 @@ import {
   PersistedUserSettings,
   UserSetting
 } from '../types/UserSettings'
-import {
-  GithubAccountId,
-  GithubRepoId,
-  GithubRepoKey,
-  GithubUserId,
-  GithubWorkflowId,
-  GithubWorkflowKey
-} from '../types/GithubKeys'
+import { GithubAccountId, GithubRepoKey, GithubUserId, GithubWorkflowKey } from '../types/GithubKeys'
 import { UserSettingsEntity } from '../entityStore/entities/UserSettingsEntity'
-import { getFromMapOrSetNewAndReturn } from '../../util/collections'
+import { getOrSetNewAndReturn } from '../../util/collections'
 
 function userSettingsEntity(appState: AppState) {
   return appState.entityStore.for(UserSettingsEntity)
@@ -46,7 +39,7 @@ function initialUserSettings(userId: GithubUserId): PersistedUserSettings {
   return {
     userId,
     github: {
-      accounts: new Map<GithubAccountId, PersistedGithubAccountSettings>()
+      accounts: {}
     }
   }
 }
@@ -117,8 +110,8 @@ function getOrCreateAndReturnAccountSettings(
   settings: PersistedUserSettings,
   accountId: GithubAccountId
 ): PersistedGithubAccountSettings {
-  return getFromMapOrSetNewAndReturn(settings.github.accounts, accountId, () => ({
-    repos: new Map<GithubRepoId, PersistedGithubRepoSettings>()
+  return getOrSetNewAndReturn(settings.github.accounts, `${accountId}`, () => ({
+    repos: {}
   }))
 }
 
@@ -127,8 +120,8 @@ function getOrCreateAndReturnRepoSettings(
   repoKey: GithubRepoKey
 ): PersistedGithubRepoSettings {
   const accountSettings = getOrCreateAndReturnAccountSettings(settings, repoKey.ownerId)
-  return getFromMapOrSetNewAndReturn(accountSettings.repos, repoKey.repoId, () => ({
-    workflows: new Map<GithubWorkflowId, PersistedGithubWorkflowSettings>()
+  return getOrSetNewAndReturn(accountSettings.repos, `${repoKey.repoId}`, () => ({
+    workflows: {}
   }))
 }
 
@@ -137,5 +130,5 @@ function getOrCreateAndReturnWorkflowSettings(
   workflowKey: GithubWorkflowKey
 ): PersistedGithubWorkflowSettings {
   const repoSettings = getOrCreateAndReturnRepoSettings(settings, workflowKey)
-  return getFromMapOrSetNewAndReturn(repoSettings.workflows, workflowKey.workflowId, () => ({}))
+  return getOrSetNewAndReturn(repoSettings.workflows, `${workflowKey.workflowId}`, () => ({}))
 }
diff --git a/src/app/domain/user/userNotifyable.ts b/src/app/domain/user/userNotifyable.ts
@@ -28,10 +28,9 @@ export async function getWorkflowNotifyEnabledForUser(
     await getUserSettings(appState, userId),
     await getWorkflowsForUser(appState, userId)
   )
-  const yesNotify = userSettings.github.accounts
-    .get(workflow.ownerId)
-    ?.repos.get(workflow.repoId)
-    ?.workflows.get(workflow.workflowId)?.notify
+  const yesNotify =
+    userSettings.github.accounts[workflow.ownerId]?.repos[workflow.repoId]?.workflows[workflow.workflowId]
+      .notify
   if (yesNotify === undefined) {
     logger.warn(`No calculated user notify setting for workflow`, { workflow, userSettings })
     return false

diff --git a/src/app/domain/user/userVisible.ts b/src/app/domain/user/userVisible.ts
@@ -94,8 +94,7 @@ export async function getRecentActiveBranchesForUser(appState: AppState, userId:
 function toVisibleEvents(allEvents: GithubWorkflowRunEvent[], userSettings: CalculatedUserSettings) {
   const visibleEvents = allEvents.filter(
     ({ ownerId, repoId, workflowId }) =>
-      userSettings.github.accounts.get(ownerId)?.repos.get(repoId)?.workflows.get(workflowId)?.visible ??
-      false
+      userSettings.github.accounts[ownerId]?.repos[repoId]?.workflows[workflowId]?.visible ?? false
   )
   return {
     allEvents,
@@ -106,7 +105,7 @@ function toVisibleEvents(allEvents: GithubWorkflowRunEvent[], userSettings: Calc
 
 function toVisiblePushes(allEvents: GithubPush[], userSettings: CalculatedUserSettings) {
   const visibleEvents = allEvents.filter(
-    ({ ownerId, repoId }) => userSettings.github.accounts.get(ownerId)?.repos.get(repoId)?.visible ?? false
+    ({ ownerId, repoId }) => userSettings.github.accounts[ownerId]?.repos[repoId]?.visible ?? false
   )
   return {
     allEvents,