Merge pull request #3209 from specklesystems/fabians/core-ioc-34

chore(server): IoC 34 - updateStreamAndNotifyFactory

packages/server/modules/comments/tests/comments.graph.spec.js

@@ -4,10 +4,7 @@ const crs = require('crypto-random-string')
 const { buildApolloServer } = require('@/app')
 const { beforeEachContext } = require('@/test/hooks')
 const { Roles } = require('@/modules/core/helpers/mainConstants')
-const {
-  grantPermissionsStream,
-  updateStream
-} = require('@/modules/core/services/streams')
+const { grantPermissionsStream } = require('@/modules/core/services/streams')
 const { createUser } = require('@/modules/core/services/users')
 const { gql } = require('graphql-tag')
 const { createObject } = require('@/modules/core/services/objects')
@@ -54,7 +51,8 @@ const {
 const {
   markCommitStreamUpdated,
   getStreamFactory,
-  createStreamFactory
+  createStreamFactory,
+  updateStreamFactory
 } = require('@/modules/core/repositories/streams')
 const { VersionsEmitter } = require('@/modules/core/events/versionsEmitter')
 const {
@@ -63,7 +61,8 @@ const {
 const { getObjectFactory } = require('@/modules/core/repositories/objects')
 const {
   legacyCreateStreamFactory,
-  createStreamReturnRecordFactory
+  createStreamReturnRecordFactory,
+  legacyUpdateStreamFactory
 } = require('@/modules/core/services/streams/management')
 const {
   inviteUsersToProjectFactory
@@ -157,6 +156,10 @@ const createStream = legacyCreateStreamFactory({
   })
 })
 
+const updateStream = legacyUpdateStreamFactory({
+  updateStream: updateStreamFactory({ db })
+})
+
 function buildCommentInputFromString(textString) {
   return convertBasicStringToDocument(textString)
 }

packages/server/modules/core/domain/streams/operations.ts

@@ -7,10 +7,12 @@ import {
 import { TokenResourceIdentifier } from '@/modules/core/domain/tokens/types'
 import {
   ProjectCreateInput,
-  StreamCreateInput
+  ProjectUpdateInput,
+  StreamCreateInput,
+  StreamUpdateInput
 } from '@/modules/core/graph/generated/graphql'
 import { ContextResourceAccessRules } from '@/modules/core/helpers/token'
-import { MaybeNullOrUndefined, Optional, StreamRoles } from '@speckle/shared'
+import { MaybeNullOrUndefined, Nullable, Optional, StreamRoles } from '@speckle/shared'
 import { Knex } from 'knex'
 
 export type GetStreams = (
@@ -56,6 +58,10 @@ export type StoreStream = (
 
 export type DeleteStreamRecords = (streamId: string) => Promise<number>
 
+export type UpdateStreamRecord = (
+  update: StreamUpdateInput | ProjectUpdateInput
+) => Promise<Nullable<Stream>>
+
 export type CreateStream = (
   params: (StreamCreateInput | ProjectCreateInput) & {
     ownerId: string
@@ -78,3 +84,13 @@ export type DeleteStream = (
     skipAccessChecks?: boolean
   }
 ) => Promise<boolean>
+
+export type UpdateStream = (
+  update: StreamUpdateInput | ProjectUpdateInput,
+  updaterId: string,
+  updaterResourceAccessRules: ContextResourceAccessRules
+) => Promise<Stream>
+
+export type LegacyUpdateStream = (
+  update: StreamUpdateInput
+) => Promise<Nullable<string>>

packages/server/modules/core/graph/resolvers/projects.ts

@@ -2,7 +2,8 @@ import { db } from '@/db/knex'
 import { saveActivityFactory } from '@/modules/activitystream/repositories'
 import {
   addStreamCreatedActivityFactory,
-  addStreamDeletedActivity
+  addStreamDeletedActivity,
+  addStreamUpdatedActivity
 } from '@/modules/activitystream/services/streamActivity'
 import { RateLimitError } from '@/modules/core/errors/ratelimit'
 import { StreamNotFoundError } from '@/modules/core/errors/stream'
@@ -23,7 +24,8 @@ import {
   getStreamFactory,
   getStreamCollaboratorsFactory,
   createStreamFactory,
-  deleteStreamFactory
+  deleteStreamFactory,
+  updateStreamFactory
 } from '@/modules/core/repositories/streams'
 import { getUsers } from '@/modules/core/repositories/users'
 import {
@@ -33,7 +35,7 @@ import {
 import {
   createStreamReturnRecordFactory,
   deleteStreamAndNotifyFactory,
-  updateStreamAndNotify,
+  updateStreamAndNotifyFactory,
   updateStreamRoleAndNotify
 } from '@/modules/core/services/streams/management'
 import { createOnboardingStream } from '@/modules/core/services/streams/onboarding'
@@ -93,6 +95,12 @@ const deleteStreamAndNotify = deleteStreamAndNotifyFactory({
   addStreamDeletedActivity,
   deleteAllResourceInvites: deleteAllResourceInvitesFactory({ db })
 })
+const updateStreamAndNotify = updateStreamAndNotifyFactory({
+  authorizeResolver,
+  getStream,
+  updateStream: updateStreamFactory({ db }),
+  addStreamUpdatedActivity
+})
 
 export = {
   Query: {

packages/server/modules/core/graph/resolvers/streams.ts

@@ -30,12 +30,13 @@ import {
   getUserStreams,
   getStreamFactory,
   createStreamFactory,
-  deleteStreamFactory
+  deleteStreamFactory,
+  updateStreamFactory
 } from '@/modules/core/repositories/streams'
 import {
   createStreamReturnRecordFactory,
   deleteStreamAndNotifyFactory,
-  updateStreamAndNotify,
+  updateStreamAndNotifyFactory,
   updateStreamRoleAndNotify
 } from '@/modules/core/services/streams/management'
 import { adminOverrideEnabled } from '@/modules/shared/helpers/envHelper'
@@ -67,7 +68,8 @@ import { getEventBus } from '@/modules/shared/services/eventBus'
 import { createBranchFactory } from '@/modules/core/repositories/branches'
 import {
   addStreamCreatedActivityFactory,
-  addStreamDeletedActivity
+  addStreamDeletedActivity,
+  addStreamUpdatedActivity
 } from '@/modules/activitystream/services/streamActivity'
 import { saveActivityFactory } from '@/modules/activitystream/repositories'
 import { ProjectsEmitter } from '@/modules/core/events/projectsEmitter'
@@ -106,6 +108,12 @@ const deleteStreamAndNotify = deleteStreamAndNotifyFactory({
   addStreamDeletedActivity,
   deleteAllResourceInvites: deleteAllResourceInvitesFactory({ db })
 })
+const updateStreamAndNotify = updateStreamAndNotifyFactory({
+  authorizeResolver,
+  getStream,
+  updateStream: updateStreamFactory({ db }),
+  addStreamUpdatedActivity
+})
 
 const getUserStreamsCore = async (
   forOtherUser: boolean,

packages/server/modules/core/repositories/streams.ts

@@ -78,7 +78,8 @@ import {
   GetStream,
   GetStreamCollaborators,
   GetStreams,
-  DeleteStreamRecords
+  DeleteStreamRecords,
+  UpdateStreamRecord
 } from '@/modules/core/domain/streams/operations'
 export type { StreamWithOptionalRole, StreamWithCommitId }
 
@@ -955,58 +956,56 @@ const isProjectUpdateInput = (
   i: StreamUpdateInput | ProjectUpdateInput
 ): i is ProjectUpdateInput => has(i, 'visibility')
 
-/** @deprecated Replace all calls with `updateProjectFacotry` */
-export async function updateStream(
-  update: StreamUpdateInput | ProjectUpdateInput,
-  db?: Knex
-) {
-  const { id: streamId } = update
+export const updateStreamFactory =
+  (deps: { db: Knex }): UpdateStreamRecord =>
+  async (update: StreamUpdateInput | ProjectUpdateInput) => {
+    const { id: streamId } = update
 
-  if (!update.name) update.name = null // to prevent saving name ''
-  const validUpdate: Partial<StreamRecord> & Record<string, unknown> = omitBy(
-    update,
-    (v) => isNull(v) || isUndefined(v)
-  )
+    if (!update.name) update.name = null // to prevent saving name ''
+    const validUpdate: Partial<StreamRecord> & Record<string, unknown> = omitBy(
+      update,
+      (v) => isNull(v) || isUndefined(v)
+    )
 
-  if (isProjectUpdateInput(update)) {
-    if (has(validUpdate, 'visibility')) {
-      validUpdate.isPublic = update.visibility !== ProjectVisibility.Private
-      validUpdate.isDiscoverable = update.visibility === ProjectVisibility.Public
-      delete validUpdate['visibility'] // cause it's not a real column
+    if (isProjectUpdateInput(update)) {
+      if (has(validUpdate, 'visibility')) {
+        validUpdate.isPublic = update.visibility !== ProjectVisibility.Private
+        validUpdate.isDiscoverable = update.visibility === ProjectVisibility.Public
+        delete validUpdate['visibility'] // cause it's not a real column
+      }
+    } else {
+      if (has(validUpdate, 'isPublic') && !validUpdate.isPublic) {
+        validUpdate.isDiscoverable = false
+      }
     }
-  } else {
+
     if (has(validUpdate, 'isPublic') && !validUpdate.isPublic) {
-      validUpdate.isDiscoverable = false
+      validUpdate.allowPublicComments = false
+    } else if (
+      has(validUpdate, 'allowPublicComments') &&
+      validUpdate.allowPublicComments
+    ) {
+      validUpdate.isPublic = true
     }
+
+    if (!Object.keys(validUpdate).length) return null
+
+    const [updatedStream] = await tables
+      .streams(deps.db)
+      .returning('*')
+      .where({ id: streamId })
+      .update<StreamRecord[]>({
+        ...validUpdate,
+        updatedAt: knex.fn.now()
+      })
+
+    return updatedStream
   }
 
-  if (has(validUpdate, 'isPublic') && !validUpdate.isPublic) {
-    validUpdate.allowPublicComments = false
-  } else if (
-    has(validUpdate, 'allowPublicComments') &&
-    validUpdate.allowPublicComments
-  ) {
-    validUpdate.isPublic = true
-  }
-
-  if (!Object.keys(validUpdate).length) return null
-
-  const [updatedStream] = await tables
-    .streams(db ?? knex)
-    .returning('*')
-    .where({ id: streamId })
-    .update<StreamRecord[]>({
-      ...validUpdate,
-      updatedAt: knex.fn.now()
-    })
-
-  return updatedStream
-}
-
 export const updateProjectFactory =
   ({ db }: { db: Knex }): UpdateProject =>
   async ({ projectUpdate }) => {
-    const updatedStream = await updateStream(projectUpdate, db)
+    const updatedStream = await updateStreamFactory({ db })(projectUpdate)
 
     if (!updatedStream) {
       throw new StreamUpdateError()
@@ -1209,7 +1208,7 @@ export async function markOnboardingBaseStream(streamId: string, version: string
   if (!stream) {
     throw new Error(`Stream ${streamId} not found`)
   }
-  await updateStream({
+  await updateStreamFactory({ db })({
     id: streamId,
     name: 'Onboarding Stream Local Source - Do Not Delete'
   })

packages/server/modules/core/services/streams.js

@@ -6,7 +6,6 @@ const {
   getFavoritedStreamsCount,
   setStreamFavorited,
   canUserFavoriteStream,
-  updateStream: updateStreamInDb,
   revokeStreamPermissions,
   grantStreamPermissions,
   getStreamFactory
@@ -25,15 +24,6 @@ const {
  */
 
 module.exports = {
-  /**
-   * @deprecated Use updateStreamAndNotify or use the repository function directly
-   * @param {import('@/modules/core/graph/generated/graphql').StreamUpdateInput} update
-   */
-  async updateStream(update) {
-    const updatedStream = await updateStreamInDb(update)
-    return updatedStream?.id || null
-  },
-
   setStreamFavorited,
 
   /**

packages/server/modules/core/services/streams/management.ts

@@ -14,7 +14,6 @@ import {
   StreamUpdatePermissionInput
 } from '@/modules/core/graph/generated/graphql'
 import { StreamRecord } from '@/modules/core/helpers/types'
-import { getStreamFactory, updateStream } from '@/modules/core/repositories/streams'
 import {
   StreamInvalidAccessError,
   StreamUpdateError
@@ -30,8 +29,6 @@ import {
   ContextResourceAccessRules,
   isNewResourceAllowed
 } from '@/modules/core/helpers/token'
-import { authorizeResolver } from '@/modules/shared'
-import db from '@/db/knex'
 import {
   TokenResourceIdentifier,
   TokenResourceIdentifierType
@@ -46,8 +43,12 @@ import {
   CreateStream,
   DeleteStream,
   DeleteStreamRecords,
+  GetStream,
   LegacyCreateStream,
-  StoreStream
+  LegacyUpdateStream,
+  StoreStream,
+  UpdateStream,
+  UpdateStreamRecord
 } from '@/modules/core/domain/streams/operations'
 import { StoreBranch } from '@/modules/core/domain/branches/operations'
 import { AuthorizeResolver } from '@/modules/shared/domain/operations'
@@ -183,42 +184,58 @@ export const deleteStreamAndNotifyFactory =
 /**
  * Update stream metadata & notify users (emit events & save activity)
  */
-export async function updateStreamAndNotify(
-  update: StreamUpdateInput | ProjectUpdateInput,
-  updaterId: string,
-  updaterResourceAccessRules: ContextResourceAccessRules
-) {
-  await authorizeResolver(
-    updaterId,
-    update.id,
-    Roles.Stream.Owner,
-    updaterResourceAccessRules
-  )
+export const updateStreamAndNotifyFactory =
+  (deps: {
+    authorizeResolver: AuthorizeResolver
+    getStream: GetStream
+    updateStream: UpdateStreamRecord
+    addStreamUpdatedActivity: typeof addStreamUpdatedActivity
+  }): UpdateStream =>
+  async (
+    update: StreamUpdateInput | ProjectUpdateInput,
+    updaterId: string,
+    updaterResourceAccessRules: ContextResourceAccessRules
+  ) => {
+    await deps.authorizeResolver(
+      updaterId,
+      update.id,
+      Roles.Stream.Owner,
+      updaterResourceAccessRules
+    )
 
-  const getStream = getStreamFactory({ db })
-  const oldStream = await getStream({ streamId: update.id, userId: updaterId })
-  if (!oldStream) {
-    throw new StreamUpdateError('Stream not found', {
-      info: { updaterId, streamId: update.id }
+    const oldStream = await deps.getStream({ streamId: update.id, userId: updaterId })
+    if (!oldStream) {
+      throw new StreamUpdateError('Stream not found', {
+        info: { updaterId, streamId: update.id }
+      })
+    }
+
+    const newStream = await deps.updateStream(update)
+    if (!newStream) {
+      return oldStream
+    }
+
+    await deps.addStreamUpdatedActivity({
+      streamId: update.id,
+      updaterId,
+      oldStream,
+      newStream,
+      update
     })
+
+    return newStream
   }
 
-  const newStream = await updateStream(update)
-  if (!newStream) {
-    return oldStream
+/**
+ * @deprecated Use updateStreamAndNotifyFactory() or the repo fn directly
+ */
+export const legacyUpdateStreamFactory =
+  (deps: { updateStream: UpdateStreamRecord }): LegacyUpdateStream =>
+  async (update) => {
+    const updatedStream = await deps.updateStream(update)
+    return updatedStream?.id || null
   }
 
-  await addStreamUpdatedActivity({
-    streamId: update.id,
-    updaterId,
-    oldStream,
-    newStream,
-    update
-  })
-
-  return newStream
-}
-
 type PermissionUpdateInput =
   | StreamUpdatePermissionInput
   | StreamRevokePermissionInput

packages/server/modules/core/services/streams/onboarding.ts

@@ -11,7 +11,7 @@ import {
   createStreamFactory,
   getOnboardingBaseStream,
   getStreamFactory,
-  updateStream
+  updateStreamFactory
 } from '@/modules/core/repositories/streams'
 import { getUser } from '@/modules/core/services/users'
 import {
@@ -51,6 +51,7 @@ export async function createOnboardingStream(
     )
   }
 
+  const updateStream = updateStreamFactory({ db })
   const addStreamCreatedActivity = addStreamCreatedActivityFactory({
     saveActivity: saveActivityFactory({ db }),
     publish

packages/server/modules/core/tests/streams.spec.ts

@@ -1,9 +1,5 @@
 import { expect } from 'chai'
-import {
-  updateStream,
-  getStreamUsers,
-  grantPermissionsStream
-} from '@/modules/core/services/streams'
+import { getStreamUsers, grantPermissionsStream } from '@/modules/core/services/streams'
 
 import { createObject } from '@/modules/core/services/objects'
 
@@ -31,7 +27,8 @@ import {
   getStreamFactory,
   markBranchStreamUpdated,
   markCommitStreamUpdated,
-  revokeStreamPermissions
+  revokeStreamPermissions,
+  updateStreamFactory
 } from '@/modules/core/repositories/streams'
 import { has, times } from 'lodash'
 import { Streams } from '@/modules/core/dbSchema'
@@ -75,7 +72,8 @@ import { addCommitCreatedActivity } from '@/modules/activitystream/services/comm
 import { getObjectFactory } from '@/modules/core/repositories/objects'
 import {
   createStreamReturnRecordFactory,
-  legacyCreateStreamFactory
+  legacyCreateStreamFactory,
+  legacyUpdateStreamFactory
 } from '@/modules/core/services/streams/management'
 import { inviteUsersToProjectFactory } from '@/modules/serverinvites/services/projectInviteManagement'
 import { createAndSendInviteFactory } from '@/modules/serverinvites/services/creation'
@@ -154,6 +152,9 @@ const createStream = legacyCreateStreamFactory({
   })
 })
 const deleteStream = deleteStreamFactory({ db })
+const updateStream = legacyUpdateStreamFactory({
+  updateStream: updateStreamFactory({ db })
+})
 
 describe('Streams @core-streams', () => {
   const userOne: BasicTestUser = {