chore(webhooks): refactor delete webhook multiregion

packages/server/modules/webhooks/domain/operations.ts

@@ -26,3 +26,5 @@ export type UpdateWebhook = ({
       >
     >
 }) => Promise<string>
+
+export type DeleteWebhook = ({ id }: Pick<Webhook, 'id'>) => Promise<number>

packages/server/modules/webhooks/graph/resolvers/webhooks-new.ts

@@ -1,10 +1,15 @@
 import { Resolvers } from '@/modules/core/graph/generated/graphql'
 import { authorizeResolver } from '@/modules/shared'
-import { createWebhook, updateWebhook } from '@/modules/webhooks/services/webhooks-new'
+import {
+  createWebhook,
+  deleteWebhook,
+  updateWebhook
+} from '@/modules/webhooks/services/webhooks-new'
 import { Roles } from '@speckle/shared'
 import {
   countWebhooksByStreamIdFactory,
   createWebhookFactory,
+  deleteWebhookFactory,
   getWebhookByIdFactory,
   updateWebhookFactory
 } from '@/modules/webhooks/repositories/webhooks'
@@ -61,6 +66,19 @@ export = {
       })
 
       return updated
+    },
+    webhookDelete: async (_parent, args, context) => {
+      await authorizeResolver(
+        context.userId,
+        args.webhook.streamId,
+        Roles.Stream.Owner,
+        context.resourceAccessRules
+      )
+
+      return await deleteWebhook({
+        deleteWebhookConfig: deleteWebhookFactory({ db }),
+        getWebhookById: getWebhookByIdFactory({ db })
+      })(args.webhook)
     }
   }
 } as Resolvers

packages/server/modules/webhooks/graph/resolvers/webhooks.js

@@ -1,6 +1,5 @@
 const { authorizeResolver } = require('@/modules/shared')
 const {
-  deleteWebhook,
   getStreamWebhooks,
   getLastWebhookEvents,
   getWebhookEventsCount
@@ -8,7 +7,6 @@ const {
 const { Roles } = require('@speckle/shared')
 const { getWebhookByIdFactory } = require('../../repositories/webhooks')
 const { db } = require('@/db/knex')
-const { ForbiddenError } = require('@/modules/shared/errors')
 
 const streamWebhooksResolver = async (parent, args, context) => {
   await authorizeResolver(
@@ -49,26 +47,5 @@ module.exports = {
 
       return { items, totalCount }
     }
-  },
-
-  Mutation: {
-    async webhookDelete(parent, args, context) {
-      await authorizeResolver(
-        context.userId,
-        args.webhook.streamId,
-        Roles.Stream.Owner,
-        context.resourceAccessRules
-      )
-
-      const wh = await getWebhookByIdFactory({ db })({ id: args.webhook.id })
-      if (args.webhook.streamId !== wh.streamId)
-        throw new ForbiddenError(
-          'The webhook id and stream id do not match. Please check your inputs.'
-        )
-
-      const deleted = await deleteWebhook({ id: args.webhook.id })
-
-      return !!deleted
-    }
   }
 }

packages/server/modules/webhooks/repositories/webhooks.ts

@@ -3,6 +3,7 @@ import { Webhook } from '@/modules/webhooks/domain/types'
 import {
   CountWebhooksByStreamId,
   CreateWebhook,
+  DeleteWebhook,
   GetWebhookById,
   UpdateWebhook
 } from '@/modules/webhooks/domain/operations'
@@ -72,3 +73,9 @@ export const updateWebhookFactory =
 
     return webhookId
   }
+
+export const deleteWebhookFactory =
+  ({ db }: { db: Knex }): DeleteWebhook =>
+  async ({ id }) => {
+    return await tables(db).webhooksConfigs.where({ id }).del()
+  }

packages/server/modules/webhooks/services/webhooks-new.ts

@@ -1,6 +1,9 @@
+import { ForbiddenError } from '@/modules/shared/errors'
 import {
   CountWebhooksByStreamId,
   CreateWebhook,
+  DeleteWebhook,
+  GetWebhookById,
   UpdateWebhook
 } from '@/modules/webhooks/domain/operations'
 import { Webhook } from '@/modules/webhooks/domain/types'
@@ -64,3 +67,23 @@ export const updateWebhook =
       }
     })
   }
+
+export const deleteWebhook =
+  ({
+    deleteWebhookConfig,
+    getWebhookById
+  }: {
+    deleteWebhookConfig: DeleteWebhook
+    getWebhookById: GetWebhookById
+  }) =>
+  async ({ id, streamId }: { id: string; streamId: string }) => {
+    const wh = await getWebhookById({ id })
+    if (streamId !== wh?.streamId)
+      throw new ForbiddenError(
+        'The webhook id and stream id do not match. Please check your inputs.'
+      )
+
+    await deleteWebhookConfig({ id })
+
+    return id
+  }

packages/server/modules/webhooks/services/webhooks.js

@@ -11,10 +11,6 @@ const Users = () => knex('users')
 const { getServerInfo } = require('../../core/services/generic')
 
 module.exports = {
-  async deleteWebhook({ id }) {
-    return await WebhooksConfig().where({ id }).del()
-  },
-
   async getStreamWebhooks({ streamId }) {
     const webhooks = await WebhooksConfig()
       .select('*')

packages/server/modules/webhooks/tests/webhooks.spec.js

@@ -12,7 +12,6 @@ const { createPersonalAccessToken } = require('../../core/services/tokens')
 const {
   getStreamWebhooks,
   getLastWebhookEvents,
-  deleteWebhook,
   dispatchStreamEvent
 } = require('../services/webhooks')
 const { createUser } = require('../../core/services/users')
@@ -22,12 +21,14 @@ const {
   createWebhookFactory,
   countWebhooksByStreamIdFactory,
   getWebhookByIdFactory,
-  updateWebhookFactory
+  updateWebhookFactory,
+  deleteWebhookFactory
 } = require('@/modules/webhooks/repositories/webhooks')
 const { db } = require('@/db/knex')
 const {
   createWebhook,
-  updateWebhook: updateWebhookService
+  updateWebhook: updateWebhookService,
+  deleteWebhook
 } = require('@/modules/webhooks/services/webhooks-new')
 const { Users, Streams } = require('@/modules/core/dbSchema')
 
@@ -113,9 +114,31 @@ describe('Webhooks @webhooks', () => {
     })
 
     it('Should delete a webhook', async () => {
-      await deleteWebhook({ id: webhookOne.id })
-      const webhook = await getWebhook({ id: webhookOne.id })
-      expect(webhook).to.be.null
+      const stream = {
+        name: 'test stream',
+        description: 'stream',
+        isPublic: true,
+        ownerId: userOne.id
+      }
+      const streamId = await createStream(stream)
+      const webhook = {
+        streamId,
+        url: 'http://localhost:42/non-existent',
+        description: 'test wh',
+        secret: 'secret',
+        enabled: true,
+        triggers: ['commit_create', 'commit_update']
+      }
+      webhook.id = await createWebhook({
+        createWebhookConfig: createWebhookFactory({ db }),
+        countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db })
+      })(webhook)
+      await deleteWebhook({
+        deleteWebhookConfig: deleteWebhookFactory({ db }),
+        getWebhookById: getWebhookByIdFactory({ db })
+      })(webhook)
+      const webhookDeleted = await getWebhookByIdFactory({ db })({ id: webhook.id })
+      expect(webhookDeleted).to.be.null
     })
 
     it('Should get webhooks for stream', async () => {
@@ -298,11 +321,30 @@ describe('Webhooks @webhooks', () => {
     })
 
     it('Should delete a webhook', async () => {
-      const res = await sendRequest(userTwo.token, {
-        query: `mutation { webhookDelete(webhook: { id: "${webhookTwo.id}", streamId: "${streamTwo.id}" } ) }`
+      const stream = {
+        name: 'test stream',
+        description: 'stream',
+        isPublic: true,
+        ownerId: userOne.id
+      }
+      const streamId = await createStream(stream)
+      const webhook = {
+        streamId,
+        url: 'http://localhost:42/non-existent',
+        description: 'test wh',
+        secret: 'secret',
+        enabled: true,
+        triggers: ['commit_create', 'commit_update']
+      }
+      webhook.id = await createWebhook({
+        createWebhookConfig: createWebhookFactory({ db }),
+        countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db })
+      })(webhook)
+      const res = await sendRequest(userOne.token, {
+        query: `mutation { webhookDelete(webhook: { id: "${webhook.id}", streamId: "${streamId}" } ) }`
       })
       expect(noErrors(res))
-      expect(res.body.data.webhookDelete).to.equal('true')
+      expect(res.body.data.webhookDelete).to.equal(webhook.id)
     })
 
     it('Should *not* create a webhook if user is not a stream owner', async () => {
@@ -328,18 +370,33 @@ describe('Webhooks @webhooks', () => {
 
     it('Should have a webhook limit for streams', async () => {
       const limit = 100
-      for (let i = 0; i < limit - 1; i++) {
+      const stream = {
+        name: 'test stream',
+        description: 'stream',
+        isPublic: true,
+        ownerId: userOne.id
+      }
+      const streamId = await createStream(stream)
+      const webhook = {
+        streamId,
+        url: 'http://localhost:42/non-existent',
+        description: 'test wh',
+        secret: 'secret',
+        enabled: true,
+        triggers: ['commit_create', 'commit_update']
+      }
+      for (let i = 0; i < limit; i++) {
         await createWebhook({
           createWebhookConfig: createWebhookFactory({ db }),
           countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db })
-        })(webhookOne)
+        })(webhook)
       }
 
       try {
         await createWebhook({
           createWebhookConfig: createWebhookFactory({ db }),
           countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db })
-        })(webhookOne)
+        })(webhook)
       } catch (err) {
         if (err.toString().indexOf('Maximum') > -1) return
       }