diff --git a/.changeset/old-meals-pull.md b/.changeset/old-meals-pull.md new file mode 100644 index 0000000000000..80a5dadf14f02 --- /dev/null +++ b/.changeset/old-meals-pull.md @@ -0,0 +1,6 @@ +--- +'@rocket.chat/apps-engine': patch +'@rocket.chat/meteor': patch +--- + +Changes a strict behavior on reporting slash commands provided by apps diff --git a/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx b/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx new file mode 100644 index 0000000000000..1640e50d9189d --- /dev/null +++ b/apps/meteor/client/hooks/useAppSlashCommands.spec.tsx @@ -0,0 +1,185 @@ +import type { SlashCommand } from '@rocket.chat/core-typings'; +import { mockAppRoot, type StreamControllerRef } from '@rocket.chat/mock-providers'; +import { renderHook, waitFor } from '@testing-library/react'; + +import { useAppSlashCommands } from './useAppSlashCommands'; +import { slashCommands } from '../../app/utils/client/slashCommand'; + +const mockSlashCommands: SlashCommand[] = [ + { + command: '/test', + description: 'Test command', + params: 'param1 param2', + clientOnly: false, + providesPreview: false, + appId: 'test-app-1', + permission: undefined, + }, + { + command: '/weather', + description: 'Get weather information', + params: 'city', + clientOnly: false, + providesPreview: true, + appId: 'weather-app', + permission: undefined, + }, +]; + +const mockApiResponse = { + commands: mockSlashCommands, + total: mockSlashCommands.length, +}; + +describe('useAppSlashCommands', () => { + let mockGetSlashCommands: jest.Mock; + + beforeEach(() => { + mockGetSlashCommands = jest.fn().mockResolvedValue(mockApiResponse); + + slashCommands.commands = {}; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should not fetch data when user ID is not available', () => { + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).build(), + }); + + expect(mockGetSlashCommands).not.toHaveBeenCalled(); + expect(slashCommands.commands).toEqual({}); + }); + + it('should fetch slash commands when user ID is available', async () => { + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).withJohnDoe().build(), + }); + + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); + }); + + it('should handle command/removed event by invalidating queries', async () => { + const streamRef: StreamControllerRef<'apps'> = {}; + + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot() + .withJohnDoe() + .withStream('apps', streamRef) + .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) + .build(), + }); + + expect(streamRef.controller).toBeDefined(); + + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); + + streamRef.controller?.emit('apps', [['command/removed', ['/test']]]); + + expect(slashCommands.commands['/test']).toBeUndefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); + }); + + it('should handle command/added event by invalidating queries', async () => { + const streamRef: StreamControllerRef<'apps'> = {}; + + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot() + .withJohnDoe() + .withStream('apps', streamRef) + .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) + .build(), + }); + + expect(streamRef.controller).toBeDefined(); + + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); + + mockGetSlashCommands.mockResolvedValue({ + commands: [ + ...mockSlashCommands, + { + command: '/newcommand', + description: 'New command', + params: 'param1 param2', + clientOnly: false, + }, + ], + total: mockSlashCommands.length + 1, + }); + + streamRef.controller?.emit('apps', [['command/added', ['/newcommand']]]); + + await waitFor(() => { + expect(slashCommands.commands['/newcommand']).toBeDefined(); + }); + + expect(slashCommands.commands['/test']).toBeDefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); + }); + + it('should handle command/updated event by invalidating queries', async () => { + const streamRef: StreamControllerRef<'apps'> = {}; + + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot() + .withJohnDoe() + .withStream('apps', streamRef) + .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) + .build(), + }); + + expect(streamRef.controller).toBeDefined(); + + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); + + streamRef.controller?.emit('apps', [['command/updated', ['/test']]]); + + expect(slashCommands.commands['/test']).toBeUndefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); + }); + + it('should ignore command/disabled event', async () => { + const streamRef: StreamControllerRef<'apps'> = {}; + + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot() + .withJohnDoe() + .withStream('apps', streamRef) + .withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands) + .build(), + }); + + expect(streamRef.controller).toBeDefined(); + + await waitFor(() => { + expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length); + }); + + streamRef.controller?.emit('apps', [['command/disabled', ['/test']]]); + + expect(slashCommands.commands['/test']).toBeDefined(); + expect(slashCommands.commands['/weather']).toBeDefined(); + }); + + it('should not set up stream listener when user ID is not available', () => { + const streamRef: StreamControllerRef<'apps'> = {}; + + renderHook(() => useAppSlashCommands(), { + wrapper: mockAppRoot().withStream('apps', streamRef).build(), + }); + + expect(streamRef.controller).toBeDefined(); + expect(streamRef.controller?.has('apps')).toBe(false); + }); +}); diff --git a/apps/meteor/client/hooks/useAppSlashCommands.ts b/apps/meteor/client/hooks/useAppSlashCommands.ts index df33dc8af6150..0425d5172c476 100644 --- a/apps/meteor/client/hooks/useAppSlashCommands.ts +++ b/apps/meteor/client/hooks/useAppSlashCommands.ts @@ -29,7 +29,7 @@ export const useAppSlashCommands = () => { return; } return apps('apps', ([key, [command]]) => { - if (['command/added', 'command/updated', 'command/disabled', 'command/removed'].includes(key)) { + if (['command/added', 'command/updated', 'command/removed'].includes(key)) { if (typeof command === 'string') { delete slashCommands.commands[command]; } diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 745aabc6105c7..9b7e8fcde1a9d 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -58,6 +58,7 @@ export interface IAppManagerDeps { interface IPurgeAppConfigOpts { keepScheduledJobs?: boolean; + keepSlashcommands?: boolean; } export class AppManager { @@ -491,7 +492,7 @@ export class AppManager { await app.call(AppMethod.ONDISABLE).catch((e) => console.warn('Error while disabling:', e)); } - await this.purgeAppConfig(app, { keepScheduledJobs: true }); + await this.purgeAppConfig(app, { keepScheduledJobs: true, keepSlashcommands: true }); await app.setStatus(status, silent); @@ -825,6 +826,7 @@ export class AppManager { } })(); + // We don't keep slashcommands here as the update could potentially not provide the same list await this.purgeAppConfig(app, { keepScheduledJobs: true }); this.apps.set(app.getID(), app); @@ -1049,6 +1051,8 @@ export class AppManager { await app.call(AppMethod.INITIALIZE); await app.setStatus(AppStatus.INITIALIZED, silenceStatus); + await this.commandManager.registerCommands(app.getID()); + result = true; } catch (e) { let status = AppStatus.ERROR_DISABLED; @@ -1085,9 +1089,13 @@ export class AppManager { if (!opts.keepScheduledJobs) { await this.schedulerManager.cleanUp(app.getID()); } + + if (!opts.keepSlashcommands) { + await this.commandManager.unregisterCommands(app.getID()); + } + this.listenerManager.unregisterListeners(app); this.listenerManager.lockEssentialEvents(app); - await this.commandManager.unregisterCommands(app.getID()); this.externalComponentManager.unregisterExternalComponents(app.getID()); await this.apiManager.unregisterApis(app.getID()); this.accessorManager.purifyApp(app.getID()); @@ -1161,7 +1169,6 @@ export class AppManager { } if (enable) { - await this.commandManager.registerCommands(app.getID()); this.externalComponentManager.registerExternalComponents(app.getID()); await this.apiManager.registerApis(app.getID()); this.listenerManager.registerListeners(app); @@ -1169,7 +1176,7 @@ export class AppManager { this.videoConfProviderManager.registerProviders(app.getID()); await this.outboundCommunicationProviderManager.registerProviders(app.getID()); } else { - await this.purgeAppConfig(app); + await this.purgeAppConfig(app, { keepScheduledJobs: true, keepSlashcommands: true }); } if (saveToDb) { diff --git a/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts b/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts index 6bdbc7bf85e95..b5c5662adf2a7 100644 --- a/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts +++ b/packages/apps-engine/src/server/managers/AppSlashCommandManager.ts @@ -333,10 +333,12 @@ export class AppSlashCommandManager { const app = this.manager.getOneById(this.touchedCommandsToApps.get(cmd)); - if (!app || AppStatusUtils.isDisabled(await app.getStatus())) { - // Just in case someone decides to do something they shouldn't - // let's ensure the app actually exists - return; + if (!app) { + throw new Error('App not found'); + } + + if (!AppStatusUtils.isEnabled(await app.getStatus())) { + throw new Error('App not enabled'); } const appCmd = this.retrieveCommandInfo(cmd, app.getID()); diff --git a/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts b/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts index 7ac88e13ddf69..73d09c18f5e91 100644 --- a/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts +++ b/packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts @@ -407,7 +407,7 @@ export class AppSlashCommandManagerTestFixture { failedItems.set('failure', asm); (ascm as any).providedCommands.set('failMePlease', failedItems); (ascm as any).touchedCommandsToApps.set('failure', 'failMePlease'); - await Expect(() => ascm.executeCommand('failure', context)).not.toThrowAsync(); + await Expect(() => ascm.executeCommand('failure', context)).toThrowAsync(); AppSlashCommandManagerTestFixture.doThrow = true; await Expect(() => ascm.executeCommand('command', context)).not.toThrowAsync();