AMM-118: Add time-based account lockout with auto-unlock#122
AMM-118: Add time-based account lockout with auto-unlock#122varundeepsaini wants to merge 1 commit intoPSMRI:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds UI controls to check and unlock user accounts, implements component methods to call new service endpoints, adds corresponding service methods and environment URLs, and surfaces fallback login error alerts. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant UI as EmployeeMasterComponent (UI)
participant Svc as EmployeeMasterNewServices
participant API as User Lock API
Admin->>UI: Click "Check lock status" (userID, userName)
UI->>Svc: getUserLockStatus(userID)
Svc->>API: POST /user/getUserLockStatus { userId }
API-->>Svc: 200 { data: "...", statusCode: 200 }
Svc-->>UI: return response
UI-->>Admin: show lock-status alert
sequenceDiagram
actor Admin
participant UI as EmployeeMasterComponent (UI)
participant Svc as EmployeeMasterNewServices
participant API as User Lock API
Admin->>UI: Click "Unlock" (confirm)
UI->>Svc: unlockUserAccount(userID)
Svc->>API: POST /user/unlockUserAccount { userId }
API-->>Svc: 200 { statusCode: 200, message }
Svc-->>UI: return response
UI-->>Admin: show success alert, refresh list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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
🤖 Fix all issues with AI agents
In
@src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts:
- Around line 1579-1619: The checkUserLockStatus method currently calls
JSON.parse(response.data) without guarding against malformed JSON; wrap the
parse in a try-catch around JSON.parse(response.data) (inside
checkUserLockStatus) to catch SyntaxError and handle it by logging the error and
showing a user-friendly dialogService.alert (e.g., 'Invalid lock status data
received' or fallback to the error path), and ensure subsequent logic only runs
when parsing succeeds (return early from the subscription error branch on parse
failure).
🧹 Nitpick comments (2)
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.html (2)
85-94: Inline styles for spacing could be consolidated.The
margin-right: 5px;styling is repeated across multiple buttons. While inline styles work, consider extracting these to a CSS class for better maintainability.♻️ Optional refactor to use CSS class
In your component CSS file, add:
.action-button-spacing { margin-right: 5px; }Then apply to buttons:
- style="margin-right: 5px;" + class="action-button-spacing"
95-104: Consider button color semantics for the Unlock action.The Unlock button uses
color="warn"(typically red), which might suggest a destructive or dangerous action. Since unlocking is a positive administrative action to restore access, consider usingcolor="accent"(green) to align with the Activate button's semantics.♻️ Suggested color adjustment
<button *ngIf="element.deleted === true" mat-raised-button - color="warn" + color="accent" matTooltip="Unlock account locked due to failed login attempts" (click)="unlockUserAccount(element.userID, element.userName)" style="margin-right: 5px;" > Unlock </button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.htmlsrc/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.tssrc/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.tssrc/app/user-login/login/login.component.tssrc/environments/environment.local.tssrc/environments/environment.prod.tssrc/environments/environment.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-25T11:01:28.621Z
Learnt from: devikasuresh20
Repo: PSMRI/ADMIN-UI PR: 50
File: src/app/app-provider-admin/provider-admin/activities/work-location-mapping/work-location-mapping.component.ts:1981-1981
Timestamp: 2024-11-25T11:01:28.621Z
Learning: In the `filterComponentList` method of `WorkLocationMappingComponent` in `src/app/app-provider-admin/provider-admin/activities/work-location-mapping/work-location-mapping.component.ts`, retain the manual filtering implementation as it provides better control over the filtering logic.
Applied to files:
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.htmlsrc/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts
🧬 Code graph analysis (1)
src/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.ts (3)
src/environments/environment.local.ts (1)
environment(40-513)src/environments/environment.prod.ts (1)
environment(40-514)src/environments/environment.test.ts (1)
environment(36-506)
🔇 Additional comments (9)
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.html (2)
105-114: LGTM - Consistent spacing added to Deactivate button.The margin-right styling maintains visual consistency with other action buttons.
115-121: LGTM - Check Lock Status button is well implemented.The button provides admins with proactive visibility into user lock status. The tooltip clearly describes the action, and the info icon is semantically appropriate.
src/environments/environment.test.ts (1)
225-229: LGTM - New API endpoints are well organized.The new User Account Lock Management endpoints are properly defined with clear comments and follow the existing URL pattern conventions. The placement near other user-related endpoints is logical.
src/environments/environment.prod.ts (1)
229-233: LGTM - Production environment endpoints are consistent.The endpoint definitions match the test environment configuration, ensuring consistency across environments.
src/app/user-login/login/login.component.ts (2)
179-218: Excellent improvement - unhandled errors now properly surface to users.The addition of the else branch ensures that error messages not matching specific patterns are displayed to users instead of being silently ignored. This improves the user experience and aids in debugging.
239-290: LGTM - Consistent error handling pattern for regular user authentication.The fallback error handler mirrors the Super Admin flow, providing consistent error messaging across both authentication paths.
src/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.ts (1)
192-202: LGTM - New service methods follow existing patterns.The
getUserLockStatus()andunlockUserAccount()methods are well implemented and consistent with the existing service architecture. They correctly use environment-based URLs and return Observables for reactive handling.♻️ Optional ES6 shorthand for cleaner syntax
The object literal can use ES6 property shorthand since the key and value names match:
getUserLockStatus(userId: number): Observable<any> { - return this.http.post(environment.getUserLockStatusUrl, { - userId: userId, - }); + return this.http.post(environment.getUserLockStatusUrl, { userId }); } unlockUserAccount(userId: number): Observable<any> { - return this.http.post(environment.unlockUserAccountUrl, { - userId: userId, - }); + return this.http.post(environment.unlockUserAccountUrl, { userId }); }src/environments/environment.local.ts (1)
230-234: LGTM!The new URL constants are properly structured, follow existing naming conventions, and are appropriately grouped with clear comments. The placement adjacent to related user account URLs is logical.
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts (1)
1538-1577: LGTM!The unlock implementation follows established patterns in this component (similar to
activateDeactivate). The confirmation dialog provides clear context, response handling is appropriate, and side effects (refresh list, reset search) align with user expectations.
...rovider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts
Outdated
Show resolved
Hide resolved
varundeepsaini
force-pushed
the
AMM-118-time-based-account-lockout
branch
from
f86014d to
885dcbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.html`:
- Around line 95-104: The Unlock button is hidden by the wrong condition
(*ngIf="element.deleted === true"), preventing admins from unlocking active but
locked users; update the template where the button with
(click)="unlockUserAccount(element.userID, element.userName)" is declared so the
button is shown for all users (remove or replace the *ngIf) — or, if you prefer
stricter control, change the condition to a lock-specific check (e.g.,
element.locked === true) instead of element.deleted.
🧹 Nitpick comments (1)
src/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.ts (1)
191-202: LGTM with optional style refinement.The new service methods follow the existing patterns in this file. Consider using ES6 shorthand property syntax for cleaner code.
✨ Optional: Use ES6 shorthand property syntax
getUserLockStatus(userId: number): Observable<any> { return this.http.post(environment.getUserLockStatusUrl, { - userId: userId, + userId, }); } unlockUserAccount(userId: number): Observable<any> { return this.http.post(environment.unlockUserAccountUrl, { - userId: userId, + userId, }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.htmlsrc/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.tssrc/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.tssrc/app/user-login/login/login.component.tssrc/environments/environment.local.tssrc/environments/environment.prod.tssrc/environments/environment.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/environments/environment.local.ts
- src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts
- src/app/user-login/login/login.component.ts
- src/environments/environment.prod.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.ts (1)
src/environments/environment.test.ts (1)
environment(36-506)
🔇 Additional comments (2)
src/environments/environment.test.ts (1)
225-229: LGTM!The new API endpoint URLs for user lock management are properly defined using the
commonBaseURLtemplate literal, following the existing naming conventions in the file. The comment accurately identifies these as Common-API endpoints.src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.html (1)
115-121: LGTM!The "Check lock status" button is correctly shown for all users without any conditional, allowing admins to check the lock status of any account regardless of its activation state.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...vider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.html
Outdated
Show resolved
Hide resolved
varundeepsaini
force-pushed
the
AMM-118-time-based-account-lockout
branch
from
885dcbc to
0172709
Compare
|
Closing due to inactivity. |
|
@varundeepsaini is this tested locally? |
|
@snehar-nd please review. |
|
Yes all the prs are tested |
...vider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.html
Outdated
Show resolved
Hide resolved
Signed-off-by: Varun Deep Saini <varun.23bcs10048@ms.sst.scaler.com>
varundeepsaini
force-pushed
the
AMM-118-time-based-account-lockout
branch
from
0172709 to
8ded910
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts (2)
1601-1607: Double confirmation dialog when unlocking from lock status check.When a user confirms the unlock prompt here,
unlockUserAccount()displays another confirmation dialog. This results in the user needing to confirm twice to complete the unlock action.Consider either:
- Calling the service directly here (bypassing the second confirmation), or
- Creating a private helper method that performs the unlock without confirmation
♻️ Suggested refactor to avoid double confirmation
Extract the unlock logic into a private method and call it directly:
+ private performUnlock(userID: number) { + this.employeeMasterNewService.unlockUserAccount(userID).subscribe( + (response: any) => { + if (response && response.statusCode === 200) { + this.dialogService.alert( + 'User account unlocked successfully. The user can now log in.', + 'success', + ); + this.getAllUserDetails(); + this.searchTerm = null; + } else { + this.dialogService.alert( + response?.errorMessage || 'Failed to unlock user account', + 'error', + ); + } + }, + (err) => { + console.log('error', err); + this.dialogService.alert( + 'Error unlocking user account. Please try again.', + 'error', + ); + }, + ); + } + unlockUserAccount(userID: number, userName: string) { this.dialogService .confirm( 'Confirm Unlock', `Are you sure you want to unlock the account for user "${userName}"?...`, ) .subscribe( (res) => { if (res) { - this.employeeMasterNewService.unlockUserAccount(userID).subscribe( - // ... existing logic - ); + this.performUnlock(userID); } }, // ... ); }Then in
checkUserLockStatus, callthis.performUnlock(userID)directly instead ofthis.unlockUserAccount(userID, userName).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts` around lines 1601 - 1607, The confirm dialog in checkUserLockStatus leads to a duplicate confirmation because unlockUserAccount(userID, userName) itself shows a confirm; refactor by adding a private helper (e.g., performUnlock(userID: string, userName?: string)) that calls the service to unlock and handles success/error without prompting, then update checkUserLockStatus to call this.performUnlock(userID, userName) instead of unlockUserAccount(...); keep unlockUserAccount as the public method that first shows the confirmation and then calls performUnlock so existing callers remain unchanged.
1594-1627: Consider defensive checks for optional response fields.If the API response is missing expected fields like
lockTimestamp,failedAttempts, orremainingTime, the UI will display "undefined" to the user. Consider adding fallback values.🛡️ Example defensive handling
if (data.isLockedDueToFailedAttempts) { const message = `Account is LOCKED due to failed login attempts.\n\n` + - `Locked at: ${data.lockTimestamp}\n` + - `Failed attempts: ${data.failedAttempts}\n` + - `Time remaining: ${data.remainingTime}\n` + + `Locked at: ${data.lockTimestamp || 'Unknown'}\n` + + `Failed attempts: ${data.failedAttempts ?? 'Unknown'}\n` + + `Time remaining: ${data.remainingTime || 'Unknown'}\n` + (data.unlockTime ? `Auto-unlock at: ${data.unlockTime}\n` : '') + `\nWould you like to unlock this account now?`;Apply similar patterns to the other message constructions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts` around lines 1594 - 1627, The constructed messages in employee-master-new.component (the branches that call this.dialogService.confirm and this.dialogService.alert) use optional API fields directly (data.lockTimestamp, data.failedAttempts, data.remainingTime, data.unlockTime) and can render "undefined"; update those message constructions to apply defensive defaults for each optional field (e.g., local consts like lockTimestamp = data.lockTimestamp ?? 'N/A', failedAttempts = data.failedAttempts ?? '0', remainingTime = data.remainingTime ?? 'Unknown', unlockTime = data.unlockTime ?? '') and use those variables when building the strings; ensure the confirm/alert calls and the unlockUserAccount(userID, userName) flow remain unchanged but reference the sanitized values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.ts`:
- Around line 1601-1607: The confirm dialog in checkUserLockStatus leads to a
duplicate confirmation because unlockUserAccount(userID, userName) itself shows
a confirm; refactor by adding a private helper (e.g., performUnlock(userID:
string, userName?: string)) that calls the service to unlock and handles
success/error without prompting, then update checkUserLockStatus to call
this.performUnlock(userID, userName) instead of unlockUserAccount(...); keep
unlockUserAccount as the public method that first shows the confirmation and
then calls performUnlock so existing callers remain unchanged.
- Around line 1594-1627: The constructed messages in
employee-master-new.component (the branches that call this.dialogService.confirm
and this.dialogService.alert) use optional API fields directly
(data.lockTimestamp, data.failedAttempts, data.remainingTime, data.unlockTime)
and can render "undefined"; update those message constructions to apply
defensive defaults for each optional field (e.g., local consts like
lockTimestamp = data.lockTimestamp ?? 'N/A', failedAttempts =
data.failedAttempts ?? '0', remainingTime = data.remainingTime ?? 'Unknown',
unlockTime = data.unlockTime ?? '') and use those variables when building the
strings; ensure the confirm/alert calls and the unlockUserAccount(userID,
userName) flow remain unchanged but reference the sanitized values.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.htmlsrc/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.tssrc/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.tssrc/app/user-login/login/login.component.tssrc/environments/environment.local.tssrc/environments/environment.prod.tssrc/environments/environment.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/user-login/login/login.component.ts
- src/environments/environment.local.ts
- src/app/app-provider-admin/provider-admin/activities/employee-master-new/employee-master-new.component.html
- src/environments/environment.prod.ts
- src/app/app-provider-admin/provider-admin/activities/services/employee-master-new-services.service.ts
- src/environments/environment.test.ts
| } | ||
|
|
||
| getUserLockStatus(userId: number): Observable<any> { | ||
| return this.http.post(environment.getUserLockStatusUrl, { |
There was a problem hiding this comment.
@varundeepsaini As I see here, you are getting the status of only one user detials. What if I want to render value for all lock/ Unlock button.
We need to get this lock/unlock staus in the get admin-api/m/SearchEmployee4 API response, so that we can rend all values on load.
|
@varundeepsaini Please do the changes based on the below senario. If the account is Activated Based on lock status: and please let me know, In this admin-api/m/SearchEmployee4 API , are we getting the Lock / Unlock status? if not, please update that from BE. Based on the previous Active/ Deactive state, you can have initial values for Lock and Unlock status. Account State Matrix |
3 participants
Part of PSMRI/AMRIT#118
📋 Description
Adds admin UI functionality to view and manage user account lock status. Admins can now check if an account is locked due to failed login attempts and manually unlock it before the 24-hour auto-unlock period.
✅ Type of Change
ℹ️ Additional Information
Changes:
checkUserLockStatus()method to display lock detailsunlockUserAccount()method for manual unlockTesting:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes