Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (18)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a forced password change feature to allow administrators to require users to change their password on next login. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
actor User as User
participant AC as AccountController
participant DS as DepartmentsService
participant DB as Database
participant Session as Session
User->>AC: Login with credentials
AC->>AC: Authenticate username/password
alt Authentication successful
AC->>DS: GetDepartmentMemberAsync(userId, deptId)
DS->>DB: Query DepartmentMembers
DB-->>DS: Return member record
DS-->>AC: Return member
alt member.MustChangePassword == true
AC->>Session: Store ForcePasswordChangeUserId
AC->>Session: Store ForcePasswordChangeDeptId
AC-->>User: Redirect to ForcePasswordChange
User->>AC: POST new password to ForcePasswordChange
AC->>AC: Hash and update password
AC->>DS: GetDepartmentMemberAsync(userId, deptId)
DS->>DB: Query DepartmentMembers
DB-->>DS: Return member record
DS-->>AC: Return member
AC->>AC: member.MustChangePassword = false
AC->>DS: SaveDepartmentMemberAsync(member)
DS->>DB: Update DepartmentMembers
DB-->>DS: Confirm update
AC->>Session: Remove force change flags
AC-->>User: Redirect to Dashboard
else MustChangePassword == false
AC-->>User: Proceed to Dashboard
end
else Authentication failed
AC-->>User: Show login error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Web/Resgrid.Web/Areas/User/Models/Profile/ResetPasswordForUserView.cs (1)
29-29: Prefer setting the default in the model.Initialize
MustChangePasswordOnLoginhere (e.g.,= true) and let the Razor checkbox bind naturally, instead of forcing checked state in markup.♻️ Proposed change
- public bool MustChangePasswordOnLogin { get; set; } + public bool MustChangePasswordOnLogin { get; set; } = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Models/Profile/ResetPasswordForUserView.cs` at line 29, Set the default value for MustChangePasswordOnLogin on the ResetPasswordForUserView model (e.g., initialize the property to true) so the Razor checkbox can bind naturally; update the property declaration for MustChangePasswordOnLogin to include the default initializer and remove any forced checked logic in the Razor view so the checkbox state comes from the model binding instead.Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs (1)
1003-1008: PassCancellationTokentoSaveDepartmentMemberAsyncfor consistency.The
SaveDepartmentMemberAsynccall on line 1007 omits thecancellationTokenparameter, inconsistent with other async service calls in this controller (e.g., line 1001 and similar calls throughout the codebase). This prevents proper request cancellation propagation. The method accepts aCancellationTokenparameter with a default value, but should be passed explicitly for consistency.🔧 Proposed fix
var member = await _departmentsService.GetDepartmentMemberAsync(model.UserId, DepartmentId); if (member != null) { member.MustChangePassword = model.MustChangePasswordOnLogin; - await _departmentsService.SaveDepartmentMemberAsync(member); + await _departmentsService.SaveDepartmentMemberAsync(member, cancellationToken); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs` around lines 1003 - 1008, The SaveDepartmentMemberAsync call is missing the cancellationToken parameter causing inconsistent cancellation propagation; update the block that retrieves and updates the member (using _departmentsService.GetDepartmentMemberAsync and member.MustChangePassword) to pass the existing cancellationToken into SaveDepartmentMemberAsync (i.e., call _departmentsService.SaveDepartmentMemberAsync(member, cancellationToken)) so the request cancellation is propagated consistently like other async service calls in this controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Model/DepartmentMember.cs`:
- Around line 83-89: The MustChangePassword property on the DepartmentMember
class is missing a ProtoMember attribute and will be omitted during protobuf
serialization; add the [ProtoMember(13)] annotation directly above the public
bool MustChangePassword { get; set; } property so it is included in
SerializerHelper.PrepareSerializer<DepartmentMember>() serialization; ensure the
numeric tag 13 continues the existing sequence (1–12) used by the other members.
In `@Web/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtml`:
- Around line 188-197: The checkbox label and help text in the AddPerson view
are hard-coded; replace them with localized strings by using the view's
IViewLocalizer/Localizer indexer (e.g., localizer["RequirePasswordChange"] and
localizer["MustChangePasswordOnLoginHelp"]) instead of plain text for the label
"Require Password Change" and the help-block text; update the markup that
references the MustChangePasswordOnLogin checkbox and add the corresponding
resource keys to your resource files so the UI is translated correctly.
In `@Web/Resgrid.Web/Areas/User/Views/Profile/ResetPasswordForUser.cshtml`:
- Around line 99-108: The Require Password Change UI hard-codes the checkbox
state and plain text; modify the view to use the MustChangePasswordOnLogin model
binding (keep asp-for="MustChangePasswordOnLogin" but remove the static
checked="checked") so the checkbox state comes from the model, and replace
static label and help text with localized strings using localizer[...] (use the
same keys for the label and the help-block) so the label and help text are
pulled from the localization resources.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs`:
- Around line 1003-1008: The SaveDepartmentMemberAsync call is missing the
cancellationToken parameter causing inconsistent cancellation propagation;
update the block that retrieves and updates the member (using
_departmentsService.GetDepartmentMemberAsync and member.MustChangePassword) to
pass the existing cancellationToken into SaveDepartmentMemberAsync (i.e., call
_departmentsService.SaveDepartmentMemberAsync(member, cancellationToken)) so the
request cancellation is propagated consistently like other async service calls
in this controller.
In `@Web/Resgrid.Web/Areas/User/Models/Profile/ResetPasswordForUserView.cs`:
- Line 29: Set the default value for MustChangePasswordOnLogin on the
ResetPasswordForUserView model (e.g., initialize the property to true) so the
Razor checkbox can bind naturally; update the property declaration for
MustChangePasswordOnLogin to include the default initializer and remove any
forced checked logic in the Razor view so the checkbox state comes from the
model binding instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 642bba93-9136-4bb5-9d40-0cc0c42b3000
📒 Files selected for processing (12)
Core/Resgrid.Model/DepartmentMember.csCore/Resgrid.Services/DepartmentsService.csCore/Resgrid.Services/UsersService.csProviders/Resgrid.Providers.Migrations/Migrations/M0064_AddingMustChangePassword.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0064_AddingMustChangePasswordPg.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/ProfileController.csWeb/Resgrid.Web/Areas/User/Models/AddPersonModel.csWeb/Resgrid.Web/Areas/User/Models/Profile/ResetPasswordForUserView.csWeb/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtmlWeb/Resgrid.Web/Areas/User/Views/Profile/ResetPasswordForUser.cshtmlWeb/Resgrid.Web/Controllers/AccountController.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes