chore: User model to TS #35056
chore: User model to TS #35056
Conversation
|
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 |
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #35056 +/- ##
===========================================
- Coverage 59.66% 59.66% -0.01%
===========================================
Files 2826 2826
Lines 68369 68397 +28
Branches 15148 15155 +7
===========================================
+ Hits 40793 40809 +16
- Misses 24962 24971 +9
- Partials 2614 2617 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
apps/meteor/imports/personal-access-tokens/server/api/methods/regenerateToken.ts
Outdated
Show resolved
Hide resolved
pierre-lehnen-rc
left a comment
pierre-lehnen-rc
left a comment
There was a problem hiding this comment.
32/34 files reviewed, just missing the two big ones.
KevLehman
force-pushed
the
chore/users-to-ts
branch
from
69f8f78 to
1ce9f60
Compare
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
kodiakhq
bot
removed
the
stat: ready to merge
There was a problem hiding this comment.
PR Overview
This pull request updates various parts of the codebase to TypeScript by adding appropriate type casts, type imports, and minor refactoring changes for better type safety and consistency.
- Type adjustments and cast fixes in several API routes and utility functions
- Enhanced error handling and logging in LDAP and livechat features
- Optional parameter changes and type guard additions for improved robustness
Reviewed Changes
| File | Description |
|---|---|
| apps/meteor/imports/personal-access-tokens/server/api/methods/regenerateToken.ts | Added a ts-expect-error comment to flag an apparent type discrepancy in token generation |
| apps/meteor/app/api/server/v1/groups.ts | Updated type cast for user status filter |
| apps/meteor/app/api/server/v1/channels.ts | Added type annotations and removed unnecessary async/await |
| apps/meteor/ee/app/livechat-enterprise/server/lib/routing/LoadRotation.ts | Enhanced check for agent existence using optional chaining on the username property |
| apps/meteor/app/importer/server/classes/converters/ConverterCache.ts | Improved user existence check before caching username to ID mapping |
| apps/meteor/ee/server/lib/ldap/Manager.ts | Added error logging when a required user is not found for channel ownership |
| apps/meteor/app/api/server/v1/users.ts | Updated user lookup to include a TODO related to types and projection |
| apps/meteor/app/api/server/lib/users.ts | Ensured options are correctly cast as FindOptions for Mongo queries |
| apps/meteor/app/api/server/helpers/addUserToFileObj.ts | Added a helper to ensure only valid string IDs are processed |
| apps/meteor/app/livechat/server/lib/isMessageFromBot.ts | Changed return type annotation, though the implementation remains unchanged |
| apps/meteor/app/livechat/server/business-hour/AbstractBusinessHour.ts | Replaced string literal 'offline' with UserStatus.OFFLINE for consistency |
| apps/meteor/app/livechat/server/lib/contacts/getContacts.ts | Ensured manager IDs are filtered for undefined values |
| apps/meteor/app/importer/server/classes/converters/UserConverter.ts | Updated function types to better handle optional user records |
| apps/meteor/app/lib/server/functions/updateGroupDMsName.ts | Refactored member filtering to use an explicit type guard |
| apps/meteor/app/api/server/v1/voip/rooms.ts | Adjusted type annotations when fetching the agent object |
| apps/meteor/app/e2e/server/methods/setUserPublicAndPrivateKeys.ts | Added a type guard to verify key pair results before proceeding |
| apps/meteor/app/apps/server/bridges/activation.ts | Updated activation bridge to use a TS constant for user status |
| apps/meteor/app/api/server/v1/im.ts | Added explicit FindOptions type to query options |
| apps/meteor/app/livechat/server/api/lib/users.ts | Removed unnecessary generic parameter for agent lookup |
| apps/meteor/app/livechat/server/business-hour/Helper.ts | Converted function parameter types to use optional parameters |
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
apps/meteor/imports/personal-access-tokens/server/api/methods/regenerateToken.ts:36
- Consider addressing the underlying type mismatch to ensure consistent token generation behavior, or formally deprecate the method instead of suppressing the TypeScript error.
// @ts-expect-error - This doesn't make sense. We're querying a user and expecting Mongo to return a token. We should look into this one
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| @@ -24,9 +25,7 @@ async function getUsersWhoAreInTheSameGroupDMsAs(user: IUser) { | |||
| room.uids.forEach((uid) => uid !== user._id && userIds.add(uid)); | |||
There was a problem hiding this comment.
room.uids.forEach((uid) => {
if (uid !== user._id) {
userIds.add(uid);
}
});Use of the comma operator in forEach loops and other expressions reduces readability and can make the code harder to debug.
This issue appears in multiple locations:
- apps/meteor/app/lib/server/functions/updateGroupDMsName.ts: Lines 25-25
- packages/models/src/models/Users.ts: Lines 598-598
- packages/models/src/models/Users.ts: Lines 138-140
Please avoid using the comma operator in favor of separate statements or explicit control structures to improve code readability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const uids = files.map(({ userId }) => userId).filter(Boolean); | ||
| const uids = files.map(({ userId }) => userId).filter(isString); | ||
|
|
||
| const users = await Users.findByIds(uids, { projection: { name: 1, username: 1 } }).toArray(); |
There was a problem hiding this comment.
try {
const users = await Users.findByIds(uids, { projection: { name: 1, username: 1 } }).toArray();
return files.map((file) => {
const user = users.find(({ _id }) => _id === file.userId);
return { ...file, user };
});
} catch (error) {
console.error('Failed to fetch users:', error);
throw new Error('Failed to fetch user data');
}Multiple instances of missing or inadequate error handling in database queries and asynchronous operations, which could lead to unhandled promise rejections or runtime errors.
This issue appears in multiple locations:
- apps/meteor/app/api/server/helpers/addUserToFileObj.ts: Lines 9-9
- apps/meteor/app/api/server/v1/channels.ts: Lines 1011-1011
- apps/meteor/app/api/server/v1/channels.ts: Lines 1050-1050
- apps/meteor/server/lib/ldap/Manager.ts: Lines 504-511
- packages/models/src/models/Users.ts: Lines 1588-1609
Please ensure all database queries and asynchronous operations are wrapped in try-catch blocks to handle potential errors and prevent unhandled promise rejections.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (fields.userId) { | ||
| user = await Users.findOneById(fields.userId, { projection: { username: 1 } }); | ||
| } else if (fields.username) { | ||
| user = await Users.findOneByUsernameIgnoringCase(fields.username, { projection: { username: 1 } }); | ||
| // TODO: Fix these types. We're passing a projection but the `user` variable is expecting way more data than this projection | ||
| user = await Users.findOneByUsernameIgnoringCase<IUser>(fields.username, { projection: { username: 1 } }); |
There was a problem hiding this comment.
// TODO: Fix these types. We're passing a projection but the `user` variable is expecting way more data than this projection
const userWithUsername = await Users.findOneByUsernameIgnoringCase<Pick<IUser, 'username'>>(fields.username, { projection: { username: 1 } });
if (userWithUsername) {
user = userWithUsername as IUser; // Temporary workaround until proper refactoring
} else {
// Handle the case where no user is found
}The TODO comment correctly identifies a type safety issue, but it would be better to also add a type assertion or refactor the code to ensure type safety. The current implementation could lead to runtime errors if the user object is used with the assumption of having more fields than just username. This implementation uses Pick to select only the 'username' property, ensuring type safety with the projection, and then asserts the type as IUser to maintain compatibility with the existing code. This is a temporary workaround until a proper refactoring can be done to address the underlying type mismatch.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
apps/meteor/ee/app/livechat-enterprise/server/lib/routing/LoadRotation.ts
Show resolved
Hide resolved
| // @ts-expect-error - This doesn't make sense. We're querying a user and expecting Mongo to return a token. We should look into this one | ||
| // or deprecate the method | ||
| return generatePersonalAccessTokenOfUser({ tokenName, userId, bypassTwoFactor: tokenExist.bypassTwoFactor || false }); |
There was a problem hiding this comment.
// @ts-expect-error - This doesn't make sense. We're querying a user and expecting Mongo to return a token. We should look into this one
// or deprecate the method
const newToken = await generatePersonalAccessTokenOfUser({ tokenName, userId, bypassTwoFactor: tokenExist.bypassTwoFactor || false });
if (!newToken) {
throw new Meteor.Error('token-generation-failed', 'Failed to generate personal access token');
}
return newToken;The @ts-expect-error comment indicates a potential type safety issue. While preserving the existing architecture and the comment indicating the need for future refactoring or deprecation, it's crucial to handle the potential for generatePersonalAccessTokenOfUser to return a falsy value, which could lead to unexpected behavior. This suggestion introduces error handling to address this issue.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| getAgentInfo( | ||
| agentId: string, | ||
| agentId: IUser['_id'], | ||
| showAgentEmail?: boolean, | ||
| ): Promise<Pick<ILivechatAgent, '_id' | 'name' | 'username' | 'phone' | 'customFields' | 'status' | 'livechat'> | null>; | ||
| ): Promise<Pick<ILivechatAgent, '_id' | 'name' | 'username' | 'phone' | 'customFields' | 'status' | 'livechat' | 'emails'> | null>; | ||
| roleBaseQuery(userId: string): { _id: string }; |
There was a problem hiding this comment.
getAgentInfo(
agentId: IUser['_id'],
showAgentEmail?: boolean,
): Promise<Partial<Pick<ILivechatAgent, '_id' | 'name' | 'username' | 'phone' | 'customFields' | 'status' | 'livechat' | 'emails'>> | null>;The return type of getAgentInfo should use Partial<Pick<>> instead of Pick<> since some fields might be undefined as indicated by the optional fields in the return type.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| setE2EPublicAndPrivateKeysByUserId(userId: string, e2e: { public_key: string; private_key: string }): Promise<UpdateResult>; | ||
| rocketMailUnsubscribe(userId: string, createdAt: string): Promise<number>; | ||
| fetchKeysByUserId(userId: string): Promise<{ public_key: string; private_key: string } | Record<string, never>>; | ||
| fetchKeysByUserId(userId: string): Promise<{ public_key: string; private_key: string } | object>; |
There was a problem hiding this comment.
fetchKeysByUserId(userId: string): Promise<{ public_key: string; private_key: string } | Record<string, never>>;The fetchKeysByUserId method's return type uses 'object' which is too broad and should be replaced with Record<string, never> for an empty object or a more specific type.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| updateStatusById( | ||
| userId: IUser['_id'], | ||
| { | ||
| statusDefault, | ||
| status, | ||
| statusConnection, | ||
| statusText, | ||
| }: { statusDefault?: UserStatus; status: UserStatus; statusConnection: UserStatus; statusText?: string }, | ||
| ) { |
There was a problem hiding this comment.
interface IUserStatusUpdate {
statusDefault?: UserStatus;
status: UserStatus;
statusConnection: UserStatus;
statusText?: string;
}
updateStatusById(userId: IUser['_id'], statusUpdate: IUserStatusUpdate)The updateStatusById method contains multiple status-related parameters in an object. Consider creating a dedicated interface/type for status updates to improve maintainability and type safety.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| const user = await Users.findOneByUsernameIgnoringCase(roomOwner); | ||
| if (!user) { | ||
| logger.error(`Unable to find user '${roomOwner}' to be the owner of the channel '${channel}'.`); |
There was a problem hiding this comment.
not sure if this should be just a log or a throw, because on the old code if user was not found then createRoom would actually throw a Meteor.Error
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
CORE-1063
Steps to test or reproduce
Further comments
This pull request focuses on migrating the User model to TypeScript in the Rocket.Chat repository. The changes span multiple files, primarily enhancing type safety and code maintainability. Key updates include:
FindOptionsfrom MongoDB to type the options parameter in thefindUsersToAutocompletefunction.UserStatusand optimizing query logic.AppActivationBridgeclass and LDAP management.IUserinterface andIUsersModelinterface with new properties and refined method signatures.tsconfig.jsonfile.Overall, these changes aim to improve type safety, consistency, and maintainability across the codebase.