-
Notifications
You must be signed in to change notification settings - Fork 21
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
encapsulating entities: user model #850
base: master
Are you sure you want to change the base?
Conversation
WalkthroughA new Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Clients/src/domain/User.ts (2)
1-10
: Consider using an interface with JSDoc documentation.While the type definition is well-structured, consider using an interface instead of a type alias for better extensibility and adding JSDoc documentation for improved IDE support.
+/** + * Represents a user in the system. + * @interface User + */ -export type User = { +export interface User { id?: number; //automatically created by database name: string; //will be filled by user surname: string; //will be filled by user email: string; //will be filled by user password_hash?: string; //created by the database role?: number; //will be filled by user created_at?: Date; //automatically filled by database last_login?: Date; //automatically filled by database -}; +}
2-9
: Enhance field documentation with JSDoc.Convert inline comments to JSDoc format for better documentation and IDE support.
export interface User { - id?: number; //automatically created by database + /** Unique identifier automatically generated by the database */ + id?: PositiveInteger; - name: string; //will be filled by user + /** User's first name */ + name: string; - surname: string; //will be filled by user + /** User's last name */ + surname: string; - email: string; //will be filled by user + /** User's email address used for authentication */ + email: EmailAddress; - password_hash?: string; //created by the database + /** Hashed password stored in the database */ + password_hash?: string; - role?: number; //will be filled by user + /** User's role determining their permissions */ + role?: UserRole; - created_at?: Date; //automatically filled by database + /** Timestamp when the user account was created */ + created_at?: Date; - last_login?: Date; //automatically filled by database + /** Timestamp of the user's most recent login */ + last_login?: Date;
id?: number; //automatically created by database | ||
name: string; //will be filled by user | ||
surname: string; //will be filled by user | ||
email: string; //will be filled by user | ||
password_hash?: string; //created by the database | ||
role?: number; //will be filled by user | ||
created_at?: Date; //automatically filled by database | ||
last_login?: Date; //automatically filled by database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Strengthen type safety with more specific types.
Consider the following improvements:
- Define role as an enum instead of number
- Use a custom type for email validation
- Add constraints for ID (positive integers)
+/** User role in the system */
+export enum UserRole {
+ ADMIN = 1,
+ USER = 2,
+ // Add other roles as needed
+}
+
+/** Email address string type with basic validation */
+type EmailAddress = string & { __brand: 'EmailAddress' };
+
+/** Positive integer type for IDs */
+type PositiveInteger = number & { __brand: 'PositiveInteger' };
+
export interface User {
- id?: number;
+ id?: PositiveInteger;
name: string;
surname: string;
- email: string;
+ email: EmailAddress;
password_hash?: string;
- role?: number;
+ role?: UserRole;
created_at?: Date;
last_login?: Date;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
id?: number; //automatically created by database | |
name: string; //will be filled by user | |
surname: string; //will be filled by user | |
email: string; //will be filled by user | |
password_hash?: string; //created by the database | |
role?: number; //will be filled by user | |
created_at?: Date; //automatically filled by database | |
last_login?: Date; //automatically filled by database | |
/** User role in the system */ | |
export enum UserRole { | |
ADMIN = 1, | |
USER = 2, | |
// Add other roles as needed | |
} | |
/** Email address string type with basic validation */ | |
type EmailAddress = string & { __brand: 'EmailAddress' }; | |
/** Positive integer type for IDs */ | |
type PositiveInteger = number & { __brand: 'PositiveInteger' }; | |
export interface User { | |
id?: PositiveInteger; //automatically created by database | |
name: string; //will be filled by user | |
surname: string; //will be filled by user | |
email: EmailAddress; //will be filled by user | |
password_hash?: string; //created by the database | |
role?: UserRole; //will be filled by user | |
created_at?: Date; //automatically filled by database | |
last_login?: Date; //automatically filled by database | |
} |
export type User = { | ||
id?: number; //automatically created by database | ||
name: string; //will be filled by user | ||
surname: string; //will be filled by user | ||
email: string; //will be filled by user | ||
password_hash?: string; //created by the database | ||
role?: number; //will be filled by user | ||
created_at?: Date; //automatically filled by database | ||
last_login?: Date; //automatically filled by database | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address security concerns in the User type.
- Consider removing
password_hash
from the User type to prevent accidental exposure. Create a separate type for authentication-related fields. - Add validation patterns for email and name fields.
+/** Validation patterns for user fields */
+const USER_VALIDATION = {
+ EMAIL_PATTERN: /^[^\s@]+@[^\s@]+\.[^\s@]+$/,
+ NAME_PATTERN: /^[a-zA-Z\s-']{2,50}$/,
+} as const;
+
+/** Type for authentication-related fields */
+interface UserAuth {
+ email: EmailAddress;
+ password_hash: string;
+}
+
export interface User {
id?: PositiveInteger;
name: string;
surname: string;
email: EmailAddress;
- password_hash?: string;
role?: UserRole;
created_at?: Date;
last_login?: Date;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Clients/src/presentation/pages/Vendors/index.tsx (2)
102-107
: User object properly typed but consider improving default values.The user object is now correctly typed as User, which improves type safety. However, using "N/A" as placeholder values and 0 as a default ID might not be ideal.
Consider using more meaningful defaults or handling the case where user data isn't available differently:
const user : User = { - id: Number(localStorage.getItem("userId")) || 0, + id: Number(localStorage.getItem("userId")) || -1, // Using -1 to indicate no valid ID email: "N/A", name: "N/A", surname: "N/A", };
154-159
: Duplicated user object structure in error logging.This is a duplicate of the user object structure used in the previous error logging block. Consider extracting this to a helper function to avoid duplication.
Extract the user mapping logic to a reusable function:
+ const getUserForLogging = (user: User) => ({ + id: String(user.id), + email: user.email ?? "N/A", + firstname: user.name, + lastname: user.surname, + }); logEngine({ type: "error", message: `An error occurred: ${error}`, - user : { - id: String(user.id), - email: user.email ?? "N/A", - firstname: user.name, - lastname: user.surname, - }, + user: getUserForLogging(user), });This would also need to be applied to the previous logEngine call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Clients/src/presentation/pages/Vendors/index.tsx
(3 hunks)
🔇 Additional comments (2)
Clients/src/presentation/pages/Vendors/index.tsx (2)
25-25
: Good addition of the User type import.Adding the User type from the domain layer is a good step toward proper encapsulation and type safety.
141-146
: Property name mismatch between User model and logging.There's a mismatch between the User model properties (name/surname) and the logging structure (firstname/lastname).
This suggests the logging system might expect a different user structure than what's defined in the User model. Verify whether this mapping is intentional and if the logging system needs to be updated to match the new User model.
Describe your changes
Briefly describe the changes you made and their purpose.
Issue number
Resolves #815
Please ensure all items are checked off before requesting a review:
Summary by CodeRabbit