Skip to content

Commit 3484c1f

Browse files
d-gubertd-gubert
authored
fix: less strict slash command registration condition (RocketChat#36855)
1 parent 84df8b6 commit 3484c1f

6 files changed

Lines changed: 210 additions & 10 deletions

File tree

‎.changeset/old-meals-pull.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@rocket.chat/apps-engine': patch
3+
'@rocket.chat/meteor': patch
4+
---
5+
6+
Changes a strict behavior on reporting slash commands provided by apps

‎apps/meteor/client/hooks/useAppSlashCommands.spec.tsx‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
import type { SlashCommand } from '@rocket.chat/core-typings';
2+
import { mockAppRoot, type StreamControllerRef } from '@rocket.chat/mock-providers';
3+
import { renderHook, waitFor } from '@testing-library/react';
4+
5+
import { useAppSlashCommands } from './useAppSlashCommands';
6+
import { slashCommands } from '../../app/utils/client/slashCommand';
7+
8+
const mockSlashCommands: SlashCommand[] = [
9+
{
10+
command: '/test',
11+
description: 'Test command',
12+
params: 'param1 param2',
13+
clientOnly: false,
14+
providesPreview: false,
15+
appId: 'test-app-1',
16+
permission: undefined,
17+
},
18+
{
19+
command: '/weather',
20+
description: 'Get weather information',
21+
params: 'city',
22+
clientOnly: false,
23+
providesPreview: true,
24+
appId: 'weather-app',
25+
permission: undefined,
26+
},
27+
];
28+
29+
const mockApiResponse = {
30+
commands: mockSlashCommands,
31+
total: mockSlashCommands.length,
32+
};
33+
34+
describe('useAppSlashCommands', () => {
35+
let mockGetSlashCommands: jest.Mock;
36+
37+
beforeEach(() => {
38+
mockGetSlashCommands = jest.fn().mockResolvedValue(mockApiResponse);
39+
40+
slashCommands.commands = {};
41+
});
42+
43+
afterEach(() => {
44+
jest.clearAllMocks();
45+
});
46+
47+
it('should not fetch data when user ID is not available', () => {
48+
renderHook(() => useAppSlashCommands(), {
49+
wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).build(),
50+
});
51+
52+
expect(mockGetSlashCommands).not.toHaveBeenCalled();
53+
expect(slashCommands.commands).toEqual({});
54+
});
55+
56+
it('should fetch slash commands when user ID is available', async () => {
57+
renderHook(() => useAppSlashCommands(), {
58+
wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).withJohnDoe().build(),
59+
});
60+
61+
await waitFor(() => {
62+
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
63+
});
64+
});
65+
66+
it('should handle command/removed event by invalidating queries', async () => {
67+
const streamRef: StreamControllerRef<'apps'> = {};
68+
69+
renderHook(() => useAppSlashCommands(), {
70+
wrapper: mockAppRoot()
71+
.withJohnDoe()
72+
.withStream('apps', streamRef)
73+
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
74+
.build(),
75+
});
76+
77+
expect(streamRef.controller).toBeDefined();
78+
79+
await waitFor(() => {
80+
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
81+
});
82+
83+
streamRef.controller?.emit('apps', [['command/removed', ['/test']]]);
84+
85+
expect(slashCommands.commands['/test']).toBeUndefined();
86+
expect(slashCommands.commands['/weather']).toBeDefined();
87+
});
88+
89+
it('should handle command/added event by invalidating queries', async () => {
90+
const streamRef: StreamControllerRef<'apps'> = {};
91+
92+
renderHook(() => useAppSlashCommands(), {
93+
wrapper: mockAppRoot()
94+
.withJohnDoe()
95+
.withStream('apps', streamRef)
96+
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
97+
.build(),
98+
});
99+
100+
expect(streamRef.controller).toBeDefined();
101+
102+
await waitFor(() => {
103+
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
104+
});
105+
106+
mockGetSlashCommands.mockResolvedValue({
107+
commands: [
108+
...mockSlashCommands,
109+
{
110+
command: '/newcommand',
111+
description: 'New command',
112+
params: 'param1 param2',
113+
clientOnly: false,
114+
},
115+
],
116+
total: mockSlashCommands.length + 1,
117+
});
118+
119+
streamRef.controller?.emit('apps', [['command/added', ['/newcommand']]]);
120+
121+
await waitFor(() => {
122+
expect(slashCommands.commands['/newcommand']).toBeDefined();
123+
});
124+
125+
expect(slashCommands.commands['/test']).toBeDefined();
126+
expect(slashCommands.commands['/weather']).toBeDefined();
127+
});
128+
129+
it('should handle command/updated event by invalidating queries', async () => {
130+
const streamRef: StreamControllerRef<'apps'> = {};
131+
132+
renderHook(() => useAppSlashCommands(), {
133+
wrapper: mockAppRoot()
134+
.withJohnDoe()
135+
.withStream('apps', streamRef)
136+
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
137+
.build(),
138+
});
139+
140+
expect(streamRef.controller).toBeDefined();
141+
142+
await waitFor(() => {
143+
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
144+
});
145+
146+
streamRef.controller?.emit('apps', [['command/updated', ['/test']]]);
147+
148+
expect(slashCommands.commands['/test']).toBeUndefined();
149+
expect(slashCommands.commands['/weather']).toBeDefined();
150+
});
151+
152+
it('should ignore command/disabled event', async () => {
153+
const streamRef: StreamControllerRef<'apps'> = {};
154+
155+
renderHook(() => useAppSlashCommands(), {
156+
wrapper: mockAppRoot()
157+
.withJohnDoe()
158+
.withStream('apps', streamRef)
159+
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
160+
.build(),
161+
});
162+
163+
expect(streamRef.controller).toBeDefined();
164+
165+
await waitFor(() => {
166+
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
167+
});
168+
169+
streamRef.controller?.emit('apps', [['command/disabled', ['/test']]]);
170+
171+
expect(slashCommands.commands['/test']).toBeDefined();
172+
expect(slashCommands.commands['/weather']).toBeDefined();
173+
});
174+
175+
it('should not set up stream listener when user ID is not available', () => {
176+
const streamRef: StreamControllerRef<'apps'> = {};
177+
178+
renderHook(() => useAppSlashCommands(), {
179+
wrapper: mockAppRoot().withStream('apps', streamRef).build(),
180+
});
181+
182+
expect(streamRef.controller).toBeDefined();
183+
expect(streamRef.controller?.has('apps')).toBe(false);
184+
});
185+
});

‎apps/meteor/client/hooks/useAppSlashCommands.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export const useAppSlashCommands = () => {
2929
return;
3030
}
3131
return apps('apps', ([key, [command]]) => {
32-
if (['command/added', 'command/updated', 'command/disabled', 'command/removed'].includes(key)) {
32+
if (['command/added', 'command/updated', 'command/removed'].includes(key)) {
3333
if (typeof command === 'string') {
3434
delete slashCommands.commands[command];
3535
}

‎packages/apps-engine/src/server/AppManager.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ export interface IAppManagerDeps {
5858

5959
interface IPurgeAppConfigOpts {
6060
keepScheduledJobs?: boolean;
61+
keepSlashcommands?: boolean;
6162
}
6263

6364
export class AppManager {
@@ -491,7 +492,7 @@ export class AppManager {
491492
await app.call(AppMethod.ONDISABLE).catch((e) => console.warn('Error while disabling:', e));
492493
}
493494

494-
await this.purgeAppConfig(app, { keepScheduledJobs: true });
495+
await this.purgeAppConfig(app, { keepScheduledJobs: true, keepSlashcommands: true });
495496

496497
await app.setStatus(status, silent);
497498

@@ -825,6 +826,7 @@ export class AppManager {
825826
}
826827
})();
827828

829+
// We don't keep slashcommands here as the update could potentially not provide the same list
828830
await this.purgeAppConfig(app, { keepScheduledJobs: true });
829831

830832
this.apps.set(app.getID(), app);
@@ -1049,6 +1051,8 @@ export class AppManager {
10491051
await app.call(AppMethod.INITIALIZE);
10501052
await app.setStatus(AppStatus.INITIALIZED, silenceStatus);
10511053

1054+
await this.commandManager.registerCommands(app.getID());
1055+
10521056
result = true;
10531057
} catch (e) {
10541058
let status = AppStatus.ERROR_DISABLED;
@@ -1085,9 +1089,13 @@ export class AppManager {
10851089
if (!opts.keepScheduledJobs) {
10861090
await this.schedulerManager.cleanUp(app.getID());
10871091
}
1092+
1093+
if (!opts.keepSlashcommands) {
1094+
await this.commandManager.unregisterCommands(app.getID());
1095+
}
1096+
10881097
this.listenerManager.unregisterListeners(app);
10891098
this.listenerManager.lockEssentialEvents(app);
1090-
await this.commandManager.unregisterCommands(app.getID());
10911099
this.externalComponentManager.unregisterExternalComponents(app.getID());
10921100
await this.apiManager.unregisterApis(app.getID());
10931101
this.accessorManager.purifyApp(app.getID());
@@ -1161,15 +1169,14 @@ export class AppManager {
11611169
}
11621170

11631171
if (enable) {
1164-
await this.commandManager.registerCommands(app.getID());
11651172
this.externalComponentManager.registerExternalComponents(app.getID());
11661173
await this.apiManager.registerApis(app.getID());
11671174
this.listenerManager.registerListeners(app);
11681175
this.listenerManager.releaseEssentialEvents(app);
11691176
this.videoConfProviderManager.registerProviders(app.getID());
11701177
await this.outboundCommunicationProviderManager.registerProviders(app.getID());
11711178
} else {
1172-
await this.purgeAppConfig(app);
1179+
await this.purgeAppConfig(app, { keepScheduledJobs: true, keepSlashcommands: true });
11731180
}
11741181

11751182
if (saveToDb) {

‎packages/apps-engine/src/server/managers/AppSlashCommandManager.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -333,10 +333,12 @@ export class AppSlashCommandManager {
333333

334334
const app = this.manager.getOneById(this.touchedCommandsToApps.get(cmd));
335335

336-
if (!app || AppStatusUtils.isDisabled(await app.getStatus())) {
337-
// Just in case someone decides to do something they shouldn't
338-
// let's ensure the app actually exists
339-
return;
336+
if (!app) {
337+
throw new Error('App not found');
338+
}
339+
340+
if (!AppStatusUtils.isEnabled(await app.getStatus())) {
341+
throw new Error('App not enabled');
340342
}
341343

342344
const appCmd = this.retrieveCommandInfo(cmd, app.getID());

‎packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,7 @@ export class AppSlashCommandManagerTestFixture {
407407
failedItems.set('failure', asm);
408408
(ascm as any).providedCommands.set('failMePlease', failedItems);
409409
(ascm as any).touchedCommandsToApps.set('failure', 'failMePlease');
410-
await Expect(() => ascm.executeCommand('failure', context)).not.toThrowAsync();
410+
await Expect(() => ascm.executeCommand('failure', context)).toThrowAsync();
411411

412412
AppSlashCommandManagerTestFixture.doThrow = true;
413413
await Expect(() => ascm.executeCommand('command', context)).not.toThrowAsync();

0 commit comments

Comments
 (0)