diff --git a/packages/server/modules/core/domain/commits/operations.ts b/packages/server/modules/core/domain/commits/operations.ts index d8e04de9c..ea8824a3b 100644 --- a/packages/server/modules/core/domain/commits/operations.ts +++ b/packages/server/modules/core/domain/commits/operations.ts @@ -12,3 +12,12 @@ export type GetCommit = ( commitId: string, options?: Partial<{ streamId: string }> ) => Promise> + +export type DeleteCommits = (commitIds: string[]) => Promise +export type DeleteCommit = (commitId: string) => Promise + +export type DeleteCommitAndNotify = ( + commitId: string, + streamId: string, + userId: string +) => Promise diff --git a/packages/server/modules/core/graph/resolvers/commits.js b/packages/server/modules/core/graph/resolvers/commits.js index fa36c77a7..717ad7997 100644 --- a/packages/server/modules/core/graph/resolvers/commits.js +++ b/packages/server/modules/core/graph/resolvers/commits.js @@ -18,8 +18,8 @@ const { const { createCommitByBranchName, updateCommitAndNotify, - deleteCommitAndNotify, - markCommitReceivedAndNotify + markCommitReceivedAndNotify, + deleteCommitAndNotifyFactory } = require('@/modules/core/services/commit/management') const { RateLimitError } = require('@/modules/core/errors/ratelimit') @@ -38,12 +38,32 @@ const { StreamInvalidAccessError } = require('@/modules/core/errors/stream') const { Roles } = require('@speckle/shared') const { toProjectIdWhitelist } = require('@/modules/core/helpers/token') const { BadRequestError } = require('@/modules/shared/errors') +const { + getCommitFactory, + deleteCommitFactory +} = require('@/modules/core/repositories/commits') +const { db } = require('@/db/knex') +const { markCommitStreamUpdated } = require('@/modules/core/repositories/streams') +const { + markCommitBranchUpdatedFactory +} = require('@/modules/core/repositories/branches') +const { + addCommitDeletedActivity +} = require('@/modules/activitystream/services/commitActivity') // subscription events const COMMIT_CREATED = CommitPubsubEvents.CommitCreated const COMMIT_UPDATED = CommitPubsubEvents.CommitUpdated const COMMIT_DELETED = CommitPubsubEvents.CommitDeleted +const deleteCommitAndNotify = deleteCommitAndNotifyFactory({ + getCommit: getCommitFactory({ db }), + markCommitStreamUpdated, + markCommitBranchUpdated: markCommitBranchUpdatedFactory({ db }), + deleteCommit: deleteCommitFactory({ db }), + addCommitDeletedActivity +}) + /** * @param {boolean} publicOnly * @param {string} userId diff --git a/packages/server/modules/core/repositories/commits.ts b/packages/server/modules/core/repositories/commits.ts index 3d38ad9cb..86f9868b7 100644 --- a/packages/server/modules/core/repositories/commits.ts +++ b/packages/server/modules/core/repositories/commits.ts @@ -22,7 +22,12 @@ import { import { Knex } from 'knex' import { Nullable, Optional } from '@speckle/shared' import { CommitWithStreamBranchMetadata } from '@/modules/core/domain/commits/types' -import { GetCommit, GetCommits } from '@/modules/core/domain/commits/operations' +import { + DeleteCommit, + DeleteCommits, + GetCommit, + GetCommits +} from '@/modules/core/domain/commits/operations' const tables = { commits: (db: Knex) => db(Commits.name) @@ -97,14 +102,18 @@ export async function moveCommitsToBranch(commitIds: string[], branchId: string) return inserts.length } -export async function deleteCommits(commitIds: string[]) { - return await Commits.knex().whereIn(Commits.col.id, commitIds).del() -} +export const deleteCommitsFactory = + (deps: { db: Knex }): DeleteCommits => + async (commitIds: string[]) => { + return await tables.commits(deps.db).whereIn(Commits.col.id, commitIds).del() + } -export async function deleteCommit(commitId: string) { - const delCount = await deleteCommits([commitId]) - return !!delCount -} +export const deleteCommitFactory = + (deps: { db: Knex }): DeleteCommit => + async (commitId: string) => { + const delCount = await deleteCommitsFactory(deps)([commitId]) + return !!delCount + } export async function getStreamCommitsWithBranchId(streamId: string) { const baseQuery = Commits.knex() diff --git a/packages/server/modules/core/services/commit/batchCommitActions.ts b/packages/server/modules/core/services/commit/batchCommitActions.ts index 4536315d2..bf9a1611e 100644 --- a/packages/server/modules/core/services/commit/batchCommitActions.ts +++ b/packages/server/modules/core/services/commit/batchCommitActions.ts @@ -19,7 +19,7 @@ import { getStreamBranchByNameFactory } from '@/modules/core/repositories/branches' import { - deleteCommits, + deleteCommitsFactory, getCommitsFactory, moveCommitsToBranch } from '@/modules/core/repositories/commits' @@ -189,7 +189,7 @@ export async function batchDeleteCommits( const { commitsWithStreams } = await validateCommitsDelete(params, userId) try { - await deleteCommits(commitIds) + await deleteCommitsFactory({ db })(commitIds) await Promise.all( commitsWithStreams.map(({ commit, stream }) => addCommitDeletedActivity({ diff --git a/packages/server/modules/core/services/commit/management.ts b/packages/server/modules/core/services/commit/management.ts index 923148937..8fa8965fb 100644 --- a/packages/server/modules/core/services/commit/management.ts +++ b/packages/server/modules/core/services/commit/management.ts @@ -5,6 +5,12 @@ import { addCommitReceivedActivity, addCommitUpdatedActivity } from '@/modules/activitystream/services/commitActivity' +import { MarkCommitBranchUpdated } from '@/modules/core/domain/branches/operations' +import { + DeleteCommit, + DeleteCommitAndNotify, + GetCommit +} from '@/modules/core/domain/commits/operations' import { CommitCreateError, CommitDeleteError, @@ -26,7 +32,6 @@ import { } from '@/modules/core/repositories/branches' import { createCommit, - deleteCommit, getCommitBranch, getCommitFactory, insertBranchCommits, @@ -318,39 +323,43 @@ export async function updateCommitAndNotify( return newCommit } -export async function deleteCommitAndNotify( - commitId: string, - streamId: string, - userId: string -) { - const commit = await getCommitFactory({ db })(commitId) - if (!commit) { - throw new CommitDeleteError("Couldn't delete nonexistant commit", { - info: { commitId, streamId, userId } - }) +export const deleteCommitAndNotifyFactory = + (deps: { + getCommit: GetCommit + markCommitStreamUpdated: typeof markCommitStreamUpdated + markCommitBranchUpdated: MarkCommitBranchUpdated + deleteCommit: DeleteCommit + addCommitDeletedActivity: typeof addCommitDeletedActivity + }): DeleteCommitAndNotify => + async (commitId: string, streamId: string, userId: string) => { + const commit = await deps.getCommit(commitId) + if (!commit) { + throw new CommitDeleteError("Couldn't delete nonexistant commit", { + info: { commitId, streamId, userId } + }) + } + + if (commit.author !== userId) { + throw new CommitDeleteError('Only the author of a commit may delete it', { + info: { commitId, streamId, userId } + }) + } + + const [, updatedBranch] = await Promise.all([ + deps.markCommitStreamUpdated(commit.id), + deps.markCommitBranchUpdated(commit.id) + ]) + + const isDeleted = await deps.deleteCommit(commitId) + if (isDeleted) { + await deps.addCommitDeletedActivity({ + commitId, + streamId, + userId, + commit, + branchId: updatedBranch.id + }) + } + + return isDeleted } - - if (commit.author !== userId) { - throw new CommitDeleteError('Only the author of a commit may delete it', { - info: { commitId, streamId, userId } - }) - } - - const [, updatedBranch] = await Promise.all([ - markCommitStreamUpdated(commit.id), - markCommitBranchUpdatedFactory({ db })(commit.id) - ]) - - const isDeleted = await deleteCommit(commitId) - if (isDeleted) { - await addCommitDeletedActivity({ - commitId, - streamId, - userId, - commit, - branchId: updatedBranch.id - }) - } - - return isDeleted -} diff --git a/packages/server/modules/core/services/commits.js b/packages/server/modules/core/services/commits.js index 7dc90c77b..72f0f9f23 100644 --- a/packages/server/modules/core/services/commits.js +++ b/packages/server/modules/core/services/commits.js @@ -12,8 +12,7 @@ const { } = require('@/modules/core/repositories/commits') const { createCommitByBranchName: createCommitByBranchNameNew, - updateCommitAndNotify, - deleteCommitAndNotify + updateCommitAndNotify } = require('@/modules/core/services/commit/management') const { clamp } = require('lodash') @@ -86,13 +85,6 @@ module.exports = { return true }, - /** - * @deprecated Use 'deleteCommitAndNotify()' - */ - async deleteCommit({ commitId, streamId, userId }) { - return await deleteCommitAndNotify(commitId, streamId, userId) - }, - /** * @deprecated Use `getBranchCommitsTotalCount()` instead */ diff --git a/packages/server/modules/core/tests/commits.spec.js b/packages/server/modules/core/tests/commits.spec.js index 851971e2e..a139fa7b6 100644 --- a/packages/server/modules/core/tests/commits.spec.js +++ b/packages/server/modules/core/tests/commits.spec.js @@ -10,7 +10,6 @@ const { createObject } = require('../services/objects') const { createCommitByBranchName, updateCommit, - deleteCommit, getCommitsTotalCountByBranchName, getCommitsByBranchName, getCommitsByStreamId, @@ -23,13 +22,24 @@ const { const cryptoRandomString = require('crypto-random-string') const { createBranchFactory, - getStreamBranchByNameFactory + getStreamBranchByNameFactory, + markCommitBranchUpdatedFactory } = require('@/modules/core/repositories/branches') const { db } = require('@/db/knex') const { addBranchCreatedActivity } = require('@/modules/activitystream/services/branchActivity') -const { getCommitFactory } = require('@/modules/core/repositories/commits') +const { + getCommitFactory, + deleteCommitFactory +} = require('@/modules/core/repositories/commits') +const { + deleteCommitAndNotifyFactory +} = require('@/modules/core/services/commit/management') +const { markCommitStreamUpdated } = require('@/modules/core/repositories/streams') +const { + addCommitDeletedActivity +} = require('@/modules/activitystream/services/commitActivity') const createBranch = createBranchFactory({ db }) const createBranchAndNotify = createBranchAndNotifyFactory({ @@ -38,6 +48,13 @@ const createBranchAndNotify = createBranchAndNotifyFactory({ addBranchCreatedActivity }) const getCommit = getCommitFactory({ db }) +const deleteCommitAndNotify = deleteCommitAndNotifyFactory({ + getCommit, + markCommitStreamUpdated, + markCommitBranchUpdated: markCommitBranchUpdatedFactory({ db }), + deleteCommit: deleteCommitFactory({ db }), + addCommitDeletedActivity +}) describe('Commits @core-commits', () => { const user = { @@ -216,11 +233,7 @@ describe('Commits @core-commits', () => { authorId: user.id }) - const res = await deleteCommit({ - commitId: tempCommit, - streamId: stream.id, - userId: user.id - }) + const res = await deleteCommitAndNotify(tempCommit, stream.id, user.id) expect(res).to.be.ok })