Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughContact verification sends now return explicit statuses for configuration, validation, rate limiting, delivery, and success. Per-channel send windows are persisted. API responses expose error codes and messages. The profile UI displays localized messages for each failure status. ChangesContact verification status flow
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ContactVerificationController
participant ContactVerificationService
participant UserProfile
participant ContactProvider
Client->>ContactVerificationController: Request verification code
ContactVerificationController->>ContactVerificationService: Send verification code
ContactVerificationService->>UserProfile: Check and record send window
UserProfile-->>ContactVerificationService: Allow or rate-limit result
ContactVerificationService->>ContactProvider: Deliver verification code
ContactProvider-->>ContactVerificationService: Delivery result
ContactVerificationService-->>ContactVerificationController: ContactVerificationSendStatus
ContactVerificationController-->>Client: Success or status-specific error
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/ContactVerificationService.cs`:
- Around line 91-92: Remove userId from the informational invalid-contact log
messages in ContactVerificationService.cs at lines 91-92 and 123-124, logging
only the contact type and failure reason. Keep userId unchanged in the
corresponding WriteAuditAsync calls.
- Around line 64-65: Update the email, mobile, and home-number send flows in
ContactVerificationService.cs at lines 64-65, 96-97, and 128-129 to use
dedicated send-window state rather than confirmation-attempt fields. Record each
send atomically before delivery, while preserving the existing hourly rate-limit
behavior and leaving ConfirmVerificationCodeAsync attempt tracking separate.
In `@Web/Resgrid.Web/wwwroot/js/ng/react-elements.js`:
- Line 1: Add the generated elements-DHTbn7Xd.min.js chunk under the ng/chunks
bundle directory so the import in react-elements.js resolves in committed
deployments; preserve the existing filename and relative import path exactly.
🪄 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 Plus
Run ID: a54ee81f-9943-443c-a787-ebf31c406b91
⛔ Files ignored due to path filters (10)
Core/Resgrid.Localization/Areas/User/Home/EditProfile.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.uk.resxis excluded by!**/*.resxTests/Resgrid.Tests/Services/ContactVerificationServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (9)
Core/Resgrid.Model/ContactVerificationSendStatus.csCore/Resgrid.Model/Services/IContactVerificationService.csCore/Resgrid.Services/ContactVerificationService.csWeb/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.csWeb/Resgrid.Web.Services/Models/v4/ContactVerification/ContactVerificationModels.csWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.jsWeb/Resgrid.Web/wwwroot/js/ng/react-elements.js
| Logging.LogInfo($"Mobile verification SMS skipped for user {userId}: phone number is not a valid sendable number (needs international format, e.g. +<country code><number>)."); | ||
| await WriteAuditAsync(userId, departmentId, ContactVerificationType.MobileNumber, false, "Send-InvalidNumber", null, cancellationToken); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove raw user identifiers from informational logs.
Both invalid-contact paths write userId to general application logs. Keep the identifier in the audit record, but log only the contact type and failure reason at informational level.
Core/Resgrid.Services/ContactVerificationService.cs#L91-L92: RemoveuserIdfrom the mobile invalid-contact log message.Core/Resgrid.Services/ContactVerificationService.cs#L123-L124: RemoveuserIdfrom the home-number invalid-contact log message.
📍 Affects 1 file
Core/Resgrid.Services/ContactVerificationService.cs#L91-L92(this comment)Core/Resgrid.Services/ContactVerificationService.cs#L123-L124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/ContactVerificationService.cs` around lines 91 - 92,
Remove userId from the informational invalid-contact log messages in
ContactVerificationService.cs at lines 91-92 and 123-124, logging only the
contact type and failure reason. Keep userId unchanged in the corresponding
WriteAuditAsync calls.
This comment has been minimized.
This comment has been minimized.
| public class SendVerificationCodeResult | ||
| { | ||
| public bool Successful { get; set; } | ||
| public string ErrorCode { get; set; } |
There was a problem hiding this comment.
Uninitialized property: The ErrorCode string property defaults to null, violating Rule [31] and risking NullReferenceException downstream. Initialize it with = string.Empty (or = null only if explicitly intended).
Kody rule violation: Initialize properties with default values
Prompt for LLM
File Web/Resgrid.Web.Services/Models/v4/ContactVerification/ContactVerificationModels.cs:
Line 22:
Uninitialized property: The `ErrorCode` string property defaults to null, violating Rule [31] and risking `NullReferenceException` downstream. Initialize it with `= string.Empty` (or `= null` only if explicitly intended).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| case ContactVerificationType.Email: | ||
| sent = await _contactVerificationService.SendEmailVerificationCodeAsync(UserId, DepartmentId, cancellationToken); | ||
| sendStatus = await _contactVerificationService.SendEmailVerificationCodeAsync(UserId, DepartmentId, cancellationToken); |
There was a problem hiding this comment.
Unhandled exception risk: The awaited call to SendEmailVerificationCodeAsync lacks a try/catch, causing failures like SMTP errors or service timeouts to surface as unstructured 500 responses. Wrap the await (or the entire switch block) in try/catch, log with context (UserId, DepartmentId, operation name), and return a meaningful error response.
Also found in:
Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1150-1150Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1153-1153Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:53-53Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:59-59Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:65-65
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:
Line 1147:
Unhandled exception risk: The awaited call to `SendEmailVerificationCodeAsync` lacks a try/catch, causing failures like SMTP errors or service timeouts to surface as unstructured 500 responses. Wrap the await (or the entire switch block) in try/catch, log with context (UserId, DepartmentId, operation name), and return a meaningful error response.
**Also found in:**
- `Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1150-1150`
- `Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1153-1153`
- `Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:53-53`
- `Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:59-59`
- `Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:65-65`
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| case ContactVerificationType.Email: | ||
| sent = await _contactVerificationService.SendEmailVerificationCodeAsync(UserId, DepartmentId, cancellationToken); | ||
| sendStatus = await _contactVerificationService.SendEmailVerificationCodeAsync(UserId, DepartmentId, cancellationToken); |
There was a problem hiding this comment.
Missing error handling: The external call to SendEmailVerificationCodeAsync lacks try/catch, so transient or connectivity errors from email/SMS gateways propagate without context. Enclose the call in try/catch, log with operation name and identifiers, and return an appropriate error code or ContactVerificationSendStatus.Failed.
Also found in:
Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1153-1153Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:59-59Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:65-65Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1150-1150Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:53-53
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:
Line 1147:
Missing error handling: The external call to `SendEmailVerificationCodeAsync` lacks try/catch, so transient or connectivity errors from email/SMS gateways propagate without context. Enclose the call in try/catch, log with operation name and identifiers, and return an appropriate error code or `ContactVerificationSendStatus.Failed`.
**Also found in:**
- `Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1153-1153`
- `Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:59-59`
- `Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:65-65`
- `Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:1150-1150`
- `Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:53-53`
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return Json(new | ||
| { | ||
| success = sendStatus == ContactVerificationSendStatus.Sent, | ||
| errorCode = sendStatus == ContactVerificationSendStatus.Sent ? null : sendStatus.ToString() | ||
| }); |
There was a problem hiding this comment.
Incorrect HTTP status code: The endpoint returns HTTP 200 via Json() even when sendStatus != Sent, violating Rule [60] and obscuring failures from clients and monitoring tools that rely on status codes. Return StatusCode(400/500) or BadRequest/ObjectResult with the error payload when sendStatus != Sent, reserving 200 for the genuinely successful case.
Also found in:
Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:74-80
Kody rule violation: Use appropriate HTTP status codes
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:
Line 1159 to 1163:
Incorrect HTTP status code: The endpoint returns HTTP 200 via `Json()` even when `sendStatus != Sent`, violating Rule [60] and obscuring failures from clients and monitoring tools that rely on status codes. Return `StatusCode(400/500)` or `BadRequest/ObjectResult` with the error payload when `sendStatus != Sent`, reserving 200 for the genuinely successful case.
**Also found in:**
- `Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs:74-80`
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| function getSendFailureLabel(errorCode) { | ||
| switch (errorCode) { | ||
| case 'ContactNotConfigured': |
There was a problem hiding this comment.
Magic string in switch case: The literal 'ContactNotConfigured' risks typos and drift with the server contract. Define a constants object (e.g., SendFailureCodes = { ContactNotConfigured: 'ContactNotConfigured', ... }) and reference SendFailureCodes.ContactNotConfigured in the switch.
Also found in:
Web/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.js:76-76Web/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.js:74-74
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.js:
Line 72:
Magic string in switch case: The literal `'ContactNotConfigured'` risks typos and drift with the server contract. Define a constants object (e.g., `SendFailureCodes = { ContactNotConfigured: 'ContactNotConfigured', ... }`) and reference `SendFailureCodes.ContactNotConfigured` in the switch.
**Also found in:**
- `Web/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.js:76-76`
- `Web/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.js:74-74`
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| $btn.hide(); | ||
| } else { | ||
| $(w.msgSpan).text(rgVerifyLabels.rateLimited).css('color', '#a94442').show(); | ||
| $(w.msgSpan).text(getSendFailureLabel(result && result.errorCode)).css('color', '#a94442').show(); |
There was a problem hiding this comment.
Magic color value: The hex '#a94442' is inlined directly in the .css() call, reducing readability and complicating theming changes. Extract it into a named constant (e.g., ErrorTextColor) or reference a shared styles/theme constant.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.js:
Line 106:
Magic color value: The hex `'#a94442'` is inlined directly in the `.css()` call, reducing readability and complicating theming changes. Extract it into a named constant (e.g., `ErrorTextColor`) or reference a shared styles/theme constant.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/ContactVerificationService.cs`:
- Around line 64-68: The email reservation at
Core/Resgrid.Services/ContactVerificationService.cs:64-68, mobile reservation at
Core/Resgrid.Services/ContactVerificationService.cs:99-103, and home reservation
at Core/Resgrid.Services/ContactVerificationService.cs:134-138 must reserve
sends atomically before delivery. Replace the current
TryRecordSend/read-then-later-save flow in ContactVerificationService with a
store-level conditional update, transaction lock, or optimistic-concurrency
retry that increments the persisted count only while it remains below
MaxVerificationSendsPerHour, returning RateLimited when reservation fails.
🪄 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 Plus
Run ID: 6122765f-4671-4ecf-8157-f40591343bc7
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Services/ContactVerificationServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (7)
.gitignoreCore/Resgrid.Model/UserProfile.csCore/Resgrid.Services/ContactVerificationService.csProviders/Resgrid.Providers.Migrations/Migrations/M0110_AddingVerificationSendWindows.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0110_AddingVerificationSendWindowsPg.csWeb/Resgrid.Web/wwwroot/js/ng/react-elements.cssWeb/Resgrid.Web/wwwroot/js/ng/react-elements.js
💤 Files with no reviewable changes (1)
- Web/Resgrid.Web/wwwroot/js/ng/react-elements.js
| if (!TryRecordSend(profile.EmailVerificationSendWindowStart, profile.EmailVerificationSendCount, out DateTime emailWindowStart, out int emailSendCount)) | ||
| return ContactVerificationSendStatus.RateLimited; | ||
|
|
||
| profile.EmailVerificationSendWindowStart = emailWindowStart; | ||
| profile.EmailVerificationSendCount = emailSendCount; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make send-window reservation atomic.
Each flow reads a profile, calculates a count, and saves later. Concurrent requests can read the same count and all pass the cap. This can issue more verification messages than MaxVerificationSendsPerHour permits.
Use a store-level conditional update, transaction lock, or optimistic-concurrency retry that reserves one send only when the persisted count remains below the cap.
Core/Resgrid.Services/ContactVerificationService.cs#L64-L68: Atomically reserve the email send before delivery.Core/Resgrid.Services/ContactVerificationService.cs#L99-L103: Atomically reserve the mobile send before delivery.Core/Resgrid.Services/ContactVerificationService.cs#L134-L138: Atomically reserve the home send before delivery.
#!/bin/bash
set -euo pipefail
ast-grep outline Core/Resgrid.Services/ContactVerificationService.cs --items all --type method
# Inspect the persistence implementation for transactional or optimistic-concurrency protection.
rg -n -C 10 --glob '*.cs' \
'\bSaveProfileAsync\s*\(|class\s+\w*UserProfileService\b|RowVersion|ConcurrencyCheck|FOR UPDATE|Serializable' .📍 Affects 1 file
Core/Resgrid.Services/ContactVerificationService.cs#L64-L68(this comment)Core/Resgrid.Services/ContactVerificationService.cs#L99-L103Core/Resgrid.Services/ContactVerificationService.cs#L134-L138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/ContactVerificationService.cs` around lines 64 - 68,
The email reservation at
Core/Resgrid.Services/ContactVerificationService.cs:64-68, mobile reservation at
Core/Resgrid.Services/ContactVerificationService.cs:99-103, and home reservation
at Core/Resgrid.Services/ContactVerificationService.cs:134-138 must reserve
sends atomically before delivery. Replace the current
TryRecordSend/read-then-later-save flow in ContactVerificationService with a
store-level conditional update, transaction lock, or optimistic-concurrency
retry that increments the persisted count only while it remains below
MaxVerificationSendsPerHour, returning RateLimited when reservation fails.
| public async Task should_allow_send_and_reset_count_after_window_elapses() | ||
| { | ||
| var profile = BuildProfile(); | ||
| profile.EmailVerificationSendWindowStart = DateTime.UtcNow.AddMinutes(-61); // window elapsed |
There was a problem hiding this comment.
The literal -61 is a magic number that implicitly couples the test to the configured window duration, causing silent test failures without a compiler signal if the window changes. Derive the offset from the configuration constant using DateTime.UtcNow.AddMinutes(-(VerificationConfig.SendWindowMinutes + 1)) or declare a named local const such as const int ElapsedWindowOffsetMinutes = 61.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Tests/Resgrid.Tests/Services/ContactVerificationServiceTests.cs:
Line 131:
The literal -61 is a magic number that implicitly couples the test to the configured window duration, causing silent test failures without a compiler signal if the window changes. Derive the offset from the configuration constant using DateTime.UtcNow.AddMinutes(-(VerificationConfig.SendWindowMinutes + 1)) or declare a named local const such as const int ElapsedWindowOffsetMinutes = 61.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Pull Request Description
RG-T117 Fixes: Improve contact verification error reporting with detailed status codes
This PR replaces the previous boolean (
true/false) return type from contact verification send methods with a newContactVerificationSendStatusenum that provides granular failure reasons. This allows the system to communicate specific error conditions to users instead of a generic failure message.Changes:
New
ContactVerificationSendStatusenum with five states:Sent,ContactNotConfigured,InvalidContact,RateLimited, andDeliveryFailedUpdated
IContactVerificationServiceandContactVerificationServiceso thatSendEmailVerificationCodeAsync,SendMobileVerificationCodeAsync, andSendHomeVerificationCodeAsyncnow return the detailed status enum instead of a booleanUpdated controllers (Web API
ContactVerificationControllerand MVCHomeController) to propagate the error code in API responses, enabling clients to display appropriate messagesAdded
ErrorCodeproperty to theSendVerificationCodeResultAPI modelAdded localized error messages for the three new failure conditions (
VerificationContactNotConfigured,VerificationInvalidContact,VerificationDeliveryFailed) across all supported languages (Arabic, German, English, Spanish, French, Italian, Polish, Swedish, Ukrainian)Updated frontend (EditUserProfile) to map error codes to user-friendly localized messages, replacing the previous behavior where all failures showed a rate-limit message
Updated and expanded unit tests to validate the new status-based return values and cover additional failure scenarios (invalid contact, rate limiting, delivery failure)
Summary by CodeRabbit
New Features
Bug Fixes