Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughThe PR modifies two files: (1) WeatherAlertService.cs refactors the message delivery workflow by separating save and send operations into distinct calls, building messages with per-member recipients and setting SystemMessageId from the persisted message; (2) RequireActivePlanFilter.cs updates controller and action exemption lists for plan enforcement checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Services/WeatherAlertService.cs (1)
375-389:⚠️ Potential issue | 🟠 MajorAvoid marking the alert notified when enqueue fails.
This split introduces a partial-success window:
SaveMessageAsynccan succeed,SendMessageAsynccan throw, and the catch only logs. Execution then reaches Lines 387-389 and marks the alert as notified anyway, so it will not be retried even though delivery failed. Move theNotificationSentupdate into the success path, or skip the post-send update after the catch.Possible fix
- try + var notificationSent = false; + try { var members = await _departmentsService.GetAllMembersForDepartmentAsync(departmentId); if (members != null && members.Any()) { // Use department managing user as sender for system messages @@ var savedMessage = await _messageService.SaveMessageAsync(message, ct); await _messageService.SendMessageAsync(savedMessage, "Weather Alert System", departmentId, false, ct); alert.SystemMessageId = savedMessage.MessageId; + notificationSent = true; } } catch (Exception ex) { Logging.LogException(ex); + continue; } } - alert.NotificationSent = true; + alert.NotificationSent = notificationSent; alert.LastUpdatedUtc = DateTime.UtcNow; await _weatherAlertRepository.UpdateAsync(alert, ct, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 375 - 389, The code marks alert.NotificationSent = true and calls _weatherAlertRepository.UpdateAsync(alert...) regardless of whether SendMessageAsync succeeded, causing failed deliveries to be treated as sent; modify the flow in the block that calls _messageService.SaveMessageAsync and _messageService.SendMessageAsync so that alert.SystemMessageId is set and then, only after SendMessageAsync completes without exception, set alert.NotificationSent = true and alert.LastUpdatedUtc = DateTime.UtcNow and call _weatherAlertRepository.UpdateAsync(alert, ct, true); keep the catch to LogException(ex) but ensure the UpdateAsync that marks NotificationSent is only executed on the successful path (or alternatively add a boolean success flag around SaveMessageAsync/SendMessageAsync and only update when true).
🤖 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.Services/WeatherAlertService.cs`:
- Around line 369-376: The code currently reuses a single Message instance
(variable message) for all recipients, then calls
_messageService.SaveMessageAsync and _messageService.SendMessageAsync, which
incorrectly shares recipient-scoped state (ReceivingUserId, ReadOn, IsDeleted)
across users; instead, iterate the members and for each member (in the loop
where you call message.AddRecipient(member.UserId)) create a new Message
instance or clone the original, set the recipient-specific fields
(ReceivingUserId etc.) for that user, then call
_messageService.SaveMessageAsync(newMessage, ct) and
_messageService.SendMessageAsync(savedMessage, "Weather Alert System",
departmentId, false, ct) per recipient so each user gets their own Message row
as the previous broadcastSingle:true path did.
In `@Web/Resgrid.Web/Filters/RequireActivePlanFilter.cs`:
- Around line 22-23: The exemption list in RequireActivePlanFilter currently
whitelists the entire "subscription" controller which is too broad and bypasses
plan enforcement; remove "subscription" from the controller-level exemptions in
the ExemptControllers/whitelist array inside RequireActivePlanFilter and instead
add explicit route/action-level checks: keep controller-level "account" if
needed but implement granular exemptions by matching controller+action names
(e.g., only allow specific read-only or webhook actions) in the filter's
OnActionExecuting (or equivalent) logic so only those explicit actions are
skipped while all other SubscriptionController endpoints still enforce an active
plan.
---
Outside diff comments:
In `@Core/Resgrid.Services/WeatherAlertService.cs`:
- Around line 375-389: The code marks alert.NotificationSent = true and calls
_weatherAlertRepository.UpdateAsync(alert...) regardless of whether
SendMessageAsync succeeded, causing failed deliveries to be treated as sent;
modify the flow in the block that calls _messageService.SaveMessageAsync and
_messageService.SendMessageAsync so that alert.SystemMessageId is set and then,
only after SendMessageAsync completes without exception, set
alert.NotificationSent = true and alert.LastUpdatedUtc = DateTime.UtcNow and
call _weatherAlertRepository.UpdateAsync(alert, ct, true); keep the catch to
LogException(ex) but ensure the UpdateAsync that marks NotificationSent is only
executed on the successful path (or alternatively add a boolean success flag
around SaveMessageAsync/SendMessageAsync and only update when true).
🪄 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: 1364680f-6b30-4927-8932-0c2ff50384a5
📒 Files selected for processing (2)
Core/Resgrid.Services/WeatherAlertService.csWeb/Resgrid.Web/Filters/RequireActivePlanFilter.cs
Comment on lines
+369
to
+376
| foreach (var member in members) | ||
| { | ||
| alert.SystemMessageId = message.MessageId; | ||
| if (member.UserId != senderId) | ||
| message.AddRecipient(member.UserId); | ||
| } | ||
|
|
||
| var savedMessage = await _messageService.SaveMessageAsync(message, ct); | ||
| await _messageService.SendMessageAsync(savedMessage, "Weather Alert System", departmentId, false, ct); |
Contributor
There was a problem hiding this comment.
Don't persist one shared Message for all recipients.
The previous broadcastSingle: true path created one Message row per recipient. This version saves a single row with multiple MessageRecipients, but ReceivingUserId, ReadOn, and IsDeleted still live on Message itself. That changes inbox semantics for weather alerts from recipient-scoped state to shared state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 369 - 376, The
code currently reuses a single Message instance (variable message) for all
recipients, then calls _messageService.SaveMessageAsync and
_messageService.SendMessageAsync, which incorrectly shares recipient-scoped
state (ReceivingUserId, ReadOn, IsDeleted) across users; instead, iterate the
members and for each member (in the loop where you call
message.AddRecipient(member.UserId)) create a new Message instance or clone the
original, set the recipient-specific fields (ReceivingUserId etc.) for that
user, then call _messageService.SaveMessageAsync(newMessage, ct) and
_messageService.SendMessageAsync(savedMessage, "Weather Alert System",
departmentId, false, ct) per recipient so each user gets their own Message row
as the previous broadcastSingle:true path did.
Comment on lines
+22
to
+23
| "account", | ||
| "subscription" |
Contributor
There was a problem hiding this comment.
Controller-level subscription exemption is too broad and bypasses plan enforcement.
On Line 23, exempting the entire Subscription controller skips this filter for all subscription endpoints, including sensitive mutation flows (e.g., cancel/addon management). That weakens the intended “active plan required” gate and can allow no-plan departments into operations that should stay restricted.
Suggested fix (narrow exemption to explicit routes)
- private static readonly string[] ExemptControllers = new[]
- {
- "account",
- "subscription"
- };
+ private static readonly string[] ExemptControllers = new[]
+ {
+ "account"
+ };
+
+ private static readonly (string Controller, string Action)[] ExemptRoutes = new[]
+ {
+ ("subscription", "selectregistrationplan"),
+ ("subscription", "logstriperesponse"),
+ ("subscription", "validatecoupon")
+ };
@@
- // Skip exempt actions (payment flow, plan selection itself)
+ // Skip explicitly exempt controller/action routes
+ foreach (var route in ExemptRoutes)
+ {
+ if (string.Equals(controller, route.Controller, StringComparison.OrdinalIgnoreCase) &&
+ string.Equals(action, route.Action, StringComparison.OrdinalIgnoreCase))
+ {
+ await next();
+ return;
+ }
+ }
+
+ // Skip exempt actions
var action = routeData.Values["action"]?.ToString() ?? string.Empty;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Filters/RequireActivePlanFilter.cs` around lines 22 - 23, The
exemption list in RequireActivePlanFilter currently whitelists the entire
"subscription" controller which is too broad and bypasses plan enforcement;
remove "subscription" from the controller-level exemptions in the
ExemptControllers/whitelist array inside RequireActivePlanFilter and instead add
explicit route/action-level checks: keep controller-level "account" if needed
but implement granular exemptions by matching controller+action names (e.g.,
only allow specific read-only or webhook actions) in the filter's
OnActionExecuting (or equivalent) logic so only those explicit actions are
skipped while all other SubscriptionController endpoints still enforce an active
plan.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Merged
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
Bug Fixes
Chores