Implement user account registration via invitation #846
Conversation
WalkthroughThis pull request updates the user registration flow across several components. The changes include modifications to the success status handling and error logging in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as RegisterUser Page
participant T as Token Extractor
participant H as useRegisterUser Hook
participant API as API Server
U->>P: Open registration page with token parameter
P->>T: Extract user token from URL
T-->>P: Return token data (id, email, name, role, expire, iat)
P->>P: Validate invitation via checkValidInvitation
alt Invitation valid
P->>P: Populate form fields with token data
else Invitation invalid
P->>P: Disable form and display alert
end
U->>P: Submit registration form
P->>H: Invoke registerUser
H->>API: Send registration data
API-->>H: Return HTTP status code
H-->>P: Return raw status code or false on error
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
Clients/src/application/hooks/useRegisterUser.tsx (2)
44-44: Consider renaming or returning a dedicated status field.
Currently,isSuccessis returning the raw numeric status code (response.status) instead of a boolean. This could be misleading to callers expecting a simple boolean. Renaming it to something likestatusCodeor returning a true/false value can improve clarity.- return { - isSuccess: response.status - }; + return { + statusCode: response.status + };
47-47: Avoid direct console logging in production code.
Usingconsole.log(error)can be useful for debugging, but consider using your existinglogEnginefor consistent logging and to prevent accidental logs in production.Clients/src/application/tools/extractToken.ts (1)
3-19: Ensure strong typing and validate the payload structure.
You've expanded the return type to include additional fields (name,role,expire,iat). Consider strictly typing these fields and parsing each with caution—especiallyexpire, which may need integer conversion. Relying onanycan lead to runtime errors or security oversights if the token shape changes unexpectedly.export const extractUserToken = ( token: string ): { - id: any; - email: any, - name: string, - role: string, - expire: string, - iat: string + id: string; + email: string; + name: string; + role: string; + expire: number; + iat: number; } | null => { // ... }Clients/src/presentation/pages/Authentication/RegisterUser/index.tsx (3)
77-77: Clarify variable naming for status checks.
SinceisSuccessnow holds the numeric status code, consider renaming to something likestatusCodefor easier reading:- if (isSuccess === 201) { + if (statusCode === 201) {
92-98: Use caution when mixing console logs and alert handling.
Though helpful for debugging, repeated console outputs can clutter production logs. If you prefer detailed logs, rely on thelogEngineor wrap the console logs under a development flag.
178-295: Well-structured registration form with invitation checks.
The dynamic disabling of fields based onisInvitationValidprovides a clear user experience. Consider extracting common field props (e.g.,sx={fieldStyles}) into a shared config or separate component to adhere to DRY principles. Otherwise, the usage here is consistent with the design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Clients/src/application/hooks/useRegisterUser.tsx(1 hunks)Clients/src/application/tools/extractToken.ts(2 hunks)Clients/src/presentation/pages/Authentication/RegisterUser/index.tsx(7 hunks)
🔇 Additional comments (8)
Clients/src/presentation/pages/Authentication/RegisterUser/index.tsx (8)
1-2: No issues with the new imports.
The additions ofBox,useEffect,lazy, andSuspensefrom these libraries are appropriate for the new functionalities.
13-15: Imports look correct for new token extraction and alert handling.
These imports align well with the functionalities added below.
25-28: Good inclusion of the email property in the initial form state.
Adding
36-40: State variables appear appropriately initialized.
Using the search param token to manage invitation validity and alert states is clear.
63-63: Form submission state change is fine.
SettingisSubmitting(true)at this stage is consistent with typical async form handling.
85-85: Logging is consistent with the current approach.
Creating amsgfromlogEngineand using it later is valid. Keep ensuring sensitive data is excluded.
125-142: Validateexpireparsing before invocation.
Ifexpireis missing or invalid,checkValidInvitationmay incorrectly report the invitation as expired or valid. Consider additional safeguards arounduserInfo?.expire.
151-163: Lazy loading the alert is a good optimization.
Your fallback ensures the UI remains responsive while loading the alert. This is a solid approach.
| const checkValidInvitation = (expDate: any) => { | ||
| let todayDate = new Date(); | ||
| let currentTime = todayDate.getTime(); | ||
| console.log(currentTime) | ||
|
|
||
| if(currentTime < expDate){ | ||
| setIsInvitationValid(true); | ||
| }else{ | ||
| console.log("The link has expired already.") | ||
| setIsInvitationValid(false); | ||
| } | ||
| return isInvitationValid; | ||
| } |
There was a problem hiding this comment.
Compare timestamps accurately and prevent possible string-compare bugs.
expDate is likely a string from the token. Converting it to a number ensures correct comparison with currentTime. Also, returning isInvitationValid immediately may not reflect React's updated state, which is asynchronous.
- const checkValidInvitation = (expDate: any) => {
- let todayDate = new Date();
- let currentTime = todayDate.getTime();
- if(currentTime < expDate){
- setIsInvitationValid(true);
- } else {
- setIsInvitationValid(false);
- }
- return isInvitationValid;
- }
+ const checkValidInvitation = (expDate: string | number) => {
+ const expiryTime = typeof expDate === "string"
+ ? parseInt(expDate, 10)
+ : expDate;
+ const currentTime = Date.now();
+ if (!Number.isNaN(expiryTime) && currentTime < expiryTime) {
+ setIsInvitationValid(true);
+ } else {
+ setIsInvitationValid(false);
+ }
+ // Return the comparison result to indicate validity
+ return currentTime < expiryTime;
+ }📝 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.
| const checkValidInvitation = (expDate: any) => { | |
| let todayDate = new Date(); | |
| let currentTime = todayDate.getTime(); | |
| console.log(currentTime) | |
| if(currentTime < expDate){ | |
| setIsInvitationValid(true); | |
| }else{ | |
| console.log("The link has expired already.") | |
| setIsInvitationValid(false); | |
| } | |
| return isInvitationValid; | |
| } | |
| const checkValidInvitation = (expDate: string | number) => { | |
| const expiryTime = typeof expDate === "string" | |
| ? parseInt(expDate, 10) | |
| : expDate; | |
| const currentTime = Date.now(); | |
| if (!Number.isNaN(expiryTime) && currentTime < expiryTime) { | |
| setIsInvitationValid(true); | |
| } else { | |
| setIsInvitationValid(false); | |
| } | |
| // Return the comparison result to indicate validity | |
| return currentTime < expiryTime; | |
| } |
|
Thanks @eieimon - looks good to me! |
Describe your changes
Issue number
Mention the issue number(s) this PR addresses (#663).
Please ensure all items are checked off before requesting a review:
Summary by CodeRabbit