feat(api): migrate users.getPreferences to OpenAPI pattern#38325
feat(api): migrate users.getPreferences to OpenAPI pattern#38325Mohamed-Sobea wants to merge 4 commits intoRocketChat:developfrom
Conversation
🦋 Changeset detectedLatest commit: 5664ef8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
WalkthroughReworked the users.getPreferences route to add AJV-based 200/400/401 response validation, moved handler logic into an action returning standardized responses, exported Changes
Sequence Diagram(s)(Skipped — changes are localized to a single endpoint and do not introduce a multi-component sequential flow warranting a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
hi @Mohamed-Sobea , Please include the Swagger documentation and clean up the old code in rest-typings so we don't lose track of it across PRs. It’s also a good idea to finish the API in a single PR to keep things clean and concise. Also I noticed there are a few missing spaces and linting issues that need fixing |
|
Hi @ahmed-n-abdeltwab , thank you for the guidance and the detailed feedback! I have implemented the requested changes and consolidated everything into this PR: Swagger Documentation: I’ve added the response block to the route definition and verified that the documentation is correctly generated at the /api-docs/ route. I've also updated the PR description with a verification screenshot. Typing Cleanup: I purged the legacy manual definitions from packages/rest-typings as the route now uses ExtractRoutesFromAPI for a single source of truth. Linting & Formatting: I resolved the spacing issues and other linting errors by running yarn lint -- --fix. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/v1/users.ts`:
- Around line 5-7: There is a duplicate import of the symbol "ajv" causing a
TypeScript duplicate identifier error; remove the second/duplicate import so
"ajv" is only imported once (keep the original import line where "ajv" appears
alongside validateBadRequestErrorResponse and validateUnauthorizedErrorResponse
and delete the later redundant import statement or entry that re-exports "ajv"),
then run a build to confirm the compilation error is resolved.
- Around line 875-879: Remove the duplicated UsersEndpoints/type export and
module augmentation added here and instead merge its definitions into the
existing augmentation: delete the new declaration of UsersEndpoints and the
declare module '@rocket.chat/rest-typings' block in this file, then update the
original augmentation (the existing declare module block that defines Endpoints)
to include the additional endpoint routes by combining
ExtractRoutesFromAPI<typeof usersEndpoints> with any other relevant endpoint
types (e.g., ExtractRoutesFromAPI<typeof usersGetPreferencesEndpoint>), and use
the original pattern interface Endpoints extends UsersEndpoints {} (or extend
the combined type via an interface) so the module augmentation remains
consistent and non-duplicative.
- Around line 851-873: The issue is a duplicate const declaration of
usersEndpoints (first used for the users.createToken endpoint and again for
users.getPreferences), so rename the second declaration (e.g.,
usersEndpointsGetPreferences) or refactor to chain the API.v1.get calls into a
single variable instead of redeclaring; then update the exported type/combined
endpoints (the export that currently references usersEndpoints) to include both
endpoint variables or the single chained variable so both users.createToken and
users.getPreferences are included. Ensure you update all references to
usersEndpoints accordingly (including the combined endpoints/type export).
|
Hey @Mohamed-Sobea , I noticed CodeRabbit flagged a few items on this PR. I know AI bots can sometimes be hit-or-miss, but it’s usually worth double-checking their logic before dismissing them. If you ever feel like the bot is 'hallucinating' or being too picky, try pushing back! You can actually reply to its comment with something like, 'Are you sure?' or 'Can you explain why this is a problem?' Usually, it will either admit it was wrong or give you a really clear explanation that helps the fix make sense. Could you take these issue? |
Nice work on the linting! Just wanted to share the workflow I usually use to double-check everything. it might save you some time: For formatting and fixing issues, I usually run: To make sure the logic is actually working, I run the specific API test like this: Finally, I always use ripgrep to make sure the endpoint is registered correctly and the paths match between the frontend and the SDK. I usually run something like this: Or for simplicity, you can just use your editor to search for things like |
github-project-automation
bot
moved this to Backlog
in rest api conversion to new endpoints
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38325 +/- ##
===========================================
- Coverage 70.86% 70.86% -0.01%
===========================================
Files 3160 3160
Lines 109775 109775
Branches 19709 19715 +6
===========================================
- Hits 77795 77793 -2
- Misses 29957 29965 +8
+ Partials 2023 2017 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4 participants
Description:
This PR completes the migration of the users.getPreferences endpoint to the new OpenAPI + AJV pattern. It introduces robust schema validation for responses, improves type safety by utilizing ExtractRoutesFromAPI, and ensures the endpoint is fully documented and verified within the Swagger UI.
Key Changes:
Full Migration: Moved users.getPreferences to the new API pattern with compiled AJV validation.
Swagger Documentation: Included the response block in the route definition to enable automatic documentation generation in the API Explorer.
Typing Cleanup: Purged legacy manual interfaces and endpoint types from packages/rest-typings to ensure a single source of truth via ExtractRoutesFromAPI.
Linting & Formatting: Resolved all spacing and formatting issues by running yarn lint -- --fix.
Testing & Verification:
Swagger UI: Verified at /api-docs/. The schema correctly reflects the validated response structure.
Functional Test: Successfully authorized and executed the endpoint in the Swagger explorer, receiving a 200 OK with the expected user preferences.
E2E Tests: Ran yarn testapi --grep users.getPreferences in apps/meteor; all assertions passed.
Successful API Response (200 OK):
Ready for final review and merge.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.