-
Notifications
You must be signed in to change notification settings - Fork 13.7k
chore: migrate presence endpoints to new API pattern #38882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
12db41e
1d10f16
d114662
183bd29
9fbaa28
9b0406e
9d732ee
f9a2253
dd3e46b
6efacf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@rocket.chat/meteor': patch | ||
| --- | ||
|
|
||
| Migrated `presence.getConnections` and `presence.enableBroadcast` REST API endpoints from legacy `addRoute` pattern to the new chained `.get()`/`.post()` API pattern with typed response schemas. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,63 @@ | ||
| import { Presence } from '@rocket.chat/core-services'; | ||
| import { ajv, validateUnauthorizedErrorResponse, validateForbiddenErrorResponse } from '@rocket.chat/rest-typings'; | ||
|
|
||
| import { API } from '../api'; | ||
|
|
||
| API.v1.addRoute( | ||
| API.v1.get( | ||
| 'presence.getConnections', | ||
| { authRequired: true, permissionsRequired: ['manage-user-status'] }, | ||
| { | ||
| async get() { | ||
| const result = await Presence.getConnectionCount(); | ||
|
|
||
| return API.v1.success(result); | ||
| authRequired: true, | ||
| permissionsRequired: ['manage-user-status'], | ||
| response: { | ||
| 200: ajv.compile<{ current: number; max: number }>({ | ||
| type: 'object', | ||
| properties: { | ||
| current: { type: 'number' }, | ||
| max: { type: 'number' }, | ||
| success: { | ||
| type: 'boolean', | ||
| enum: [true], | ||
| }, | ||
| }, | ||
| required: ['current', 'max', 'success'], | ||
| additionalProperties: false, | ||
| }), | ||
| 401: validateUnauthorizedErrorResponse, | ||
| 403: validateForbiddenErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| const result = await Presence.getConnectionCount(); | ||
|
|
||
| return API.v1.success(result); | ||
| }, | ||
| ); | ||
|
|
||
| API.v1.addRoute( | ||
| API.v1.post( | ||
| 'presence.enableBroadcast', | ||
| { authRequired: true, permissionsRequired: ['manage-user-status'], twoFactorRequired: true }, | ||
| { | ||
| async post() { | ||
| await Presence.toggleBroadcast(true); | ||
|
|
||
| return API.v1.success(); | ||
| authRequired: true, | ||
| permissionsRequired: ['manage-user-status'], | ||
| twoFactorRequired: true, | ||
| response: { | ||
| 200: ajv.compile<void>({ | ||
| type: 'object', | ||
| properties: { | ||
| success: { | ||
| type: 'boolean', | ||
| enum: [true], | ||
| }, | ||
| }, | ||
| required: ['success'], | ||
| additionalProperties: false, | ||
| }), | ||
|
smirk-dev marked this conversation as resolved.
Outdated
|
||
| 401: validateUnauthorizedErrorResponse, | ||
| 403: validateForbiddenErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| await Presence.toggleBroadcast(true); | ||
|
|
||
| return API.v1.success(); | ||
| }, | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,7 +230,7 @@ it('renders a code block', async () => { | |
| </Suspense>, | ||
| ); | ||
|
|
||
| await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument()); | ||
| await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument(), { timeout: 5000 }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did you add a timeout here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests render components inside (code blocks and KaTeX), which means the actual component is lazy-loaded asynchronously. The default waitFor timeout is 1000ms, which can be too tight on slower CI runners causing flaky failures where the lazy-loaded chunk hasn't resolved yet. Bumping to 5000ms gives enough headroom for the dynamic import to complete without making the tests meaningfully slower since waitFor polls and resolves as soon as the assertion passes, it only waits the full 5s on actual failure. This was specifically triggered by intermittent CI failures on these three tests. |
||
|
|
||
| expect(screen.getByRole('region')).toHaveTextContent('```const foo = bar;```'); | ||
| }); | ||
|
|
@@ -250,7 +250,7 @@ it('renders a code block with language', async () => { | |
| </Suspense>, | ||
| ); | ||
|
|
||
| await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument()); | ||
| await waitFor(() => expect(screen.getByRole('region')).toBeInTheDocument(), { timeout: 5000 }); | ||
|
|
||
| expect(screen.getByRole('region')).toHaveTextContent('```const foo = bar;```'); | ||
| expect(screen.getByRole('region').querySelector('.language-javascript')).toBeInTheDocument(); | ||
|
|
@@ -271,7 +271,7 @@ it('renders a Katex block', async () => { | |
| ); | ||
|
|
||
| // workaround for jest-dom's inability to handle MathML | ||
| await waitFor(() => expect(container.querySelector('math')).toBeTruthy()); | ||
| await waitFor(() => expect(container.querySelector('math')).toBeTruthy(), { timeout: 5000 }); | ||
| container.querySelector('math')?.remove(); | ||
|
|
||
| expect(screen.getByRole('math', { name: 'x^2 + y^2 = z^2' })).toBeInTheDocument(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.