From ea213906f42dab30939fa86d248b2d80bd82ac69 Mon Sep 17 00:00:00 2001 From: Kristaps Fabians Geikins Date: Thu, 26 Sep 2024 16:44:27 +0300 Subject: [PATCH] chore(server): core IoC 10 - updateBranchAndNotifyFactory --- .../core/domain/branches/operations.ts | 14 ++- .../modules/core/graph/resolvers/branches.js | 15 +++- .../modules/core/graph/resolvers/models.ts | 16 +++- .../modules/core/repositories/branches.ts | 26 +++--- .../core/services/branch/management.ts | 85 ++++++++++--------- .../server/modules/core/services/branches.js | 16 +--- .../modules/core/tests/branches.spec.js | 64 ++++++++------ 7 files changed, 137 insertions(+), 99 deletions(-) diff --git a/packages/server/modules/core/domain/branches/operations.ts b/packages/server/modules/core/domain/branches/operations.ts index 92a59a978..974f62a78 100644 --- a/packages/server/modules/core/domain/branches/operations.ts +++ b/packages/server/modules/core/domain/branches/operations.ts @@ -2,10 +2,12 @@ import { Branch, ModelTreeItem } from '@/modules/core/domain/branches/types' import { BranchLatestCommit } from '@/modules/core/domain/commits/types' import { BranchCreateInput, + BranchUpdateInput, CreateModelInput, ModelsTreeItemCollection, ProjectModelsArgs, - ProjectModelsTreeArgs + ProjectModelsTreeArgs, + UpdateModelInput } from '@/modules/core/graph/generated/graphql' import { ModelsTreeItemGraphQLReturn } from '@/modules/core/helpers/graphTypes' import { Nullable, Optional } from '@speckle/shared' @@ -131,3 +133,13 @@ export type CreateBranchAndNotify = ( input: BranchCreateInput | CreateModelInput, creatorId: string ) => Promise + +export type UpdateBranch = ( + branchId: string, + branch: Partial +) => Promise + +export type UpdateBranchAndNotify = ( + input: BranchUpdateInput | UpdateModelInput, + userId: string +) => Promise diff --git a/packages/server/modules/core/graph/resolvers/branches.js b/packages/server/modules/core/graph/resolvers/branches.js index 429731da3..10fdafe41 100644 --- a/packages/server/modules/core/graph/resolvers/branches.js +++ b/packages/server/modules/core/graph/resolvers/branches.js @@ -9,9 +9,9 @@ const { const { authorizeResolver } = require('@/modules/shared') const { - updateBranchAndNotify, deleteBranchAndNotify, - createBranchAndNotifyFactory + createBranchAndNotifyFactory, + updateBranchAndNotifyFactory } = require('@/modules/core/services/branch/management') const { getPaginatedStreamBranches @@ -22,11 +22,13 @@ const { Roles } = require('@speckle/shared') const { getBranchByIdFactory, getStreamBranchByNameFactory, - createBranchFactory + createBranchFactory, + updateBranchFactory } = require('@/modules/core/repositories/branches') const { db } = require('@/db/knex') const { - addBranchCreatedActivity + addBranchCreatedActivity, + addBranchUpdatedActivity } = require('@/modules/activitystream/services/branchActivity') // subscription events @@ -41,6 +43,11 @@ const createBranchAndNotify = createBranchAndNotifyFactory({ createBranch: createBranchFactory({ db }), addBranchCreatedActivity }) +const updateBranchAndNotify = updateBranchAndNotifyFactory({ + getBranchById, + updateBranch: updateBranchFactory({ db }), + addBranchUpdatedActivity +}) /** @type {import('@/modules/core/graph/generated/graphql').Resolvers} */ module.exports = { diff --git a/packages/server/modules/core/graph/resolvers/models.ts b/packages/server/modules/core/graph/resolvers/models.ts index 39402e515..543a2f23f 100644 --- a/packages/server/modules/core/graph/resolvers/models.ts +++ b/packages/server/modules/core/graph/resolvers/models.ts @@ -3,7 +3,7 @@ import { Resolvers } from '@/modules/core/graph/generated/graphql' import { createBranchAndNotifyFactory, deleteBranchAndNotify, - updateBranchAndNotify + updateBranchAndNotifyFactory } from '@/modules/core/services/branch/management' import { getPaginatedProjectModelsFactory, @@ -24,6 +24,7 @@ import { } from '@/modules/shared/utils/subscriptions' import { createBranchFactory, + getBranchByIdFactory, getBranchLatestCommitsFactory, getModelTreeItemsFactory, getModelTreeItemsFilteredFactory, @@ -32,7 +33,8 @@ import { getPaginatedProjectModelsItemsFactory, getPaginatedProjectModelsTotalCountFactory, getStreamBranchByNameFactory, - getStreamBranchesByNameFactory + getStreamBranchesByNameFactory, + updateBranchFactory } from '@/modules/core/repositories/branches' import { BranchNotFoundError } from '@/modules/core/errors/branch' import { CommitNotFoundError } from '@/modules/core/errors/commit' @@ -42,7 +44,10 @@ import { getSpecificBranchCommits } from '@/modules/core/repositories/commits' import { db } from '@/db/knex' -import { addBranchCreatedActivity } from '@/modules/activitystream/services/branchActivity' +import { + addBranchCreatedActivity, + addBranchUpdatedActivity +} from '@/modules/activitystream/services/branchActivity' const getViewerResourceGroups = getViewerResourceGroupsFactory({ getStreamObjects, @@ -72,6 +77,11 @@ const createBranchAndNotify = createBranchAndNotifyFactory({ createBranch: createBranchFactory({ db }), addBranchCreatedActivity }) +const updateBranchAndNotify = updateBranchAndNotifyFactory({ + getBranchById: getBranchByIdFactory({ db }), + updateBranch: updateBranchFactory({ db }), + addBranchUpdatedActivity +}) export = { User: { diff --git a/packages/server/modules/core/repositories/branches.ts b/packages/server/modules/core/repositories/branches.ts index cde66b623..fdb069540 100644 --- a/packages/server/modules/core/repositories/branches.ts +++ b/packages/server/modules/core/repositories/branches.ts @@ -29,7 +29,8 @@ import { GetStreamBranchByName, GetStreamBranchesByName, GetStructuredProjectModels, - StoreBranch + StoreBranch, + UpdateBranch } from '@/modules/core/domain/branches/operations' import { BranchLatestCommit } from '@/modules/core/domain/commits/types' import { ModelTreeItem } from '@/modules/core/domain/branches/types' @@ -667,17 +668,20 @@ export const createBranchFactory = return newBranch } -export async function updateBranch(branchId: string, branch: Partial) { - if (branch.name) { - validateBranchName(branch.name) - branch.name = branch.name.toLowerCase() - } +export const updateBranchFactory = + (deps: { db: Knex }): UpdateBranch => + async (branchId: string, branch: Partial) => { + if (branch.name) { + validateBranchName(branch.name) + branch.name = branch.name.toLowerCase() + } - const [newBranch] = (await Branches.knex() - .where(Branches.col.id, branchId) - .update(branch, '*')) as BranchRecord[] - return newBranch -} + const [newBranch] = (await tables + .branches(deps.db) + .where(Branches.col.id, branchId) + .update(branch, '*')) as BranchRecord[] + return newBranch + } export async function deleteBranchById(branchId: string) { // this needs to happen before deleting the branch, otherwise the diff --git a/packages/server/modules/core/services/branch/management.ts b/packages/server/modules/core/services/branch/management.ts index 90bcf66f0..18ccfaabf 100644 --- a/packages/server/modules/core/services/branch/management.ts +++ b/packages/server/modules/core/services/branch/management.ts @@ -20,8 +20,7 @@ import { import { BranchRecord } from '@/modules/core/helpers/types' import { deleteBranchById, - getBranchByIdFactory, - updateBranch + getBranchByIdFactory } from '@/modules/core/repositories/branches' import { getStream, markBranchStreamUpdated } from '@/modules/core/repositories/streams' import { has } from 'lodash' @@ -30,8 +29,11 @@ import { ModelsEmitter } from '@/modules/core/events/modelsEmitter' import { db } from '@/db/knex' import { CreateBranchAndNotify, + GetBranchById, GetStreamBranchByName, - StoreBranch + StoreBranch, + UpdateBranch, + UpdateBranchAndNotify } from '@/modules/core/domain/branches/operations' const isBranchCreateInput = ( @@ -62,48 +64,51 @@ export const createBranchAndNotifyFactory = return branch } -export async function updateBranchAndNotify( - input: BranchUpdateInput | UpdateModelInput, - userId: string -) { - const streamId = isBranchUpdateInput(input) ? input.streamId : input.projectId - const existingBranch = await getBranchByIdFactory({ db })(input.id) - if (!existingBranch) { - throw new BranchUpdateError('Branch not found', { info: { ...input, userId } }) - } - if (existingBranch.streamId !== streamId) { - throw new BranchUpdateError( - 'The branch ID and stream ID do not match, please check your inputs', - { - info: { ...input, userId } - } - ) - } +export const updateBranchAndNotifyFactory = + (deps: { + getBranchById: GetBranchById + updateBranch: UpdateBranch + addBranchUpdatedActivity: typeof addBranchUpdatedActivity + }): UpdateBranchAndNotify => + async (input: BranchUpdateInput | UpdateModelInput, userId: string) => { + const streamId = isBranchUpdateInput(input) ? input.streamId : input.projectId + const existingBranch = await deps.getBranchById(input.id) + if (!existingBranch) { + throw new BranchUpdateError('Branch not found', { info: { ...input, userId } }) + } + if (existingBranch.streamId !== streamId) { + throw new BranchUpdateError( + 'The branch ID and stream ID do not match, please check your inputs', + { + info: { ...input, userId } + } + ) + } - const updates: Partial = { - ...(!isNullOrUndefined(input.description) - ? { description: input.description } - : {}), - ...(input.name?.length ? { name: input.name } : {}) - } - if (!Object.values(updates).length) { - throw new BranchUpdateError('Please specify a property to update') - } + const updates: Partial = { + ...(!isNullOrUndefined(input.description) + ? { description: input.description } + : {}), + ...(input.name?.length ? { name: input.name } : {}) + } + if (!Object.values(updates).length) { + throw new BranchUpdateError('Please specify a property to update') + } - const newBranch = await updateBranch(input.id, updates) + const newBranch = await deps.updateBranch(input.id, updates) - if (newBranch) { - await addBranchUpdatedActivity({ - update: input, - userId, - oldBranch: existingBranch, - newBranch - }) + if (newBranch) { + await deps.addBranchUpdatedActivity({ + update: input, + userId, + oldBranch: existingBranch, + newBranch + }) + } + + return newBranch } - return newBranch -} - export async function deleteBranchAndNotify( input: BranchDeleteInput | DeleteModelInput, userId: string diff --git a/packages/server/modules/core/services/branches.js b/packages/server/modules/core/services/branches.js index 9672acdc1..674f505c4 100644 --- a/packages/server/modules/core/services/branches.js +++ b/packages/server/modules/core/services/branches.js @@ -1,25 +1,11 @@ 'use strict' const knex = require('@/db/knex') const { getStreamBranchCount } = require('@/modules/core/repositories/branches') -const { - updateBranchAndNotify, - deleteBranchAndNotify -} = require('@/modules/core/services/branch/management') +const { deleteBranchAndNotify } = require('@/modules/core/services/branch/management') const Branches = () => knex('branches') module.exports = { - /** - * @deprecated Use 'updateBranchAndNotify' - */ - async updateBranch({ id, name, description, streamId, userId }) { - const newBranch = await updateBranchAndNotify( - { id, name, description, streamId }, - userId - ) - return newBranch ? 1 : 0 - }, - /** * @returns {Promise<{ * items: import('@/modules/core/helpers/types').BranchRecord[], diff --git a/packages/server/modules/core/tests/branches.spec.js b/packages/server/modules/core/tests/branches.spec.js index c6409ac2d..9a81bdb05 100644 --- a/packages/server/modules/core/tests/branches.spec.js +++ b/packages/server/modules/core/tests/branches.spec.js @@ -11,24 +11,32 @@ const knex = require('@/db/knex') const { createUser } = require('../services/users') const { createStream } = require('../services/streams') const { createObject } = require('../services/objects') -const { - updateBranch, - getBranchesByStreamId, - deleteBranchById -} = require('../services/branches') +const { getBranchesByStreamId, deleteBranchById } = require('../services/branches') const { createCommitByBranchName } = require('../services/commits') -const { deleteBranchAndNotify } = require('@/modules/core/services/branch/management') +const { + deleteBranchAndNotify, + updateBranchAndNotifyFactory +} = require('@/modules/core/services/branch/management') const { getBranchByIdFactory, getStreamBranchByNameFactory, - createBranchFactory + createBranchFactory, + updateBranchFactory } = require('@/modules/core/repositories/branches') +const { + addBranchUpdatedActivity +} = require('@/modules/activitystream/services/branchActivity') const Commits = () => knex('commits') const getBranchById = getBranchByIdFactory({ db: knex }) const getStreamBranchByName = getStreamBranchByNameFactory({ db: knex }) const createBranch = createBranchFactory({ db: knex }) +const updateBranchAndNotify = updateBranchAndNotifyFactory({ + getBranchById: getBranchByIdFactory({ db: knex }), + updateBranch: updateBranchFactory({ db: knex }), + addBranchUpdatedActivity +}) describe('Branches @core-branches', () => { const user = { @@ -94,24 +102,28 @@ describe('Branches @core-branches', () => { } try { - await updateBranch({ - id: branch.id, - name: '/super/part/two', - streamId: stream.id, - userId: user.id - }) + await updateBranchAndNotify( + { + id: branch.id, + name: '/super/part/two', + streamId: stream.id + }, + user.id + ) assert.fail('Illegal branch name passed through in update operation.') } catch (err) { expect(err.message).to.contain('Branch names cannot start with') } try { - await updateBranch({ - id: branch.id, - name: '#super#part#three', - streamId: stream.id, - userId: user.id - }) + await updateBranchAndNotify( + { + id: branch.id, + name: '#super#part#three', + streamId: stream.id + }, + user.id + ) assert.fail('Illegal branch name passed through in update operation.') } catch (err) { expect(err.message).to.contain('Branch names cannot start with') @@ -158,12 +170,14 @@ describe('Branches @core-branches', () => { }) it('Should update a branch', async () => { - await updateBranch({ - id: branch.id, - description: 'lorem ipsum', - streamId: stream.id, - userId: user.id - }) + await updateBranchAndNotify( + { + id: branch.id, + description: 'lorem ipsum', + streamId: stream.id + }, + user.id + ) const b1 = await getBranchById(branch.id) expect(b1.description).to.equal('lorem ipsum')