Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesSMS Opt-Out Compliance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
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/SmsService.cs (1)
172-174:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame double-processing issue in SendCallAsync.
The text is already HTML-converted and stripped at lines 172-174 (plus address/protocols appended), then
FormatTextForMessagewill process it again. Since additional content is appended here, consider updatingFormatTextForMessageto accept already-processed text instead.Also applies to: 203-203
🤖 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/SmsService.cs` around lines 172 - 174, In the SendCallAsync method, the text processing from call.NatureOfCall is being done twice: first at the HtmlToTextHelper.ConvertHtml and StringHelpers.StripHtmlTagsCharArray calls where the address is also appended, and then again when the result is passed to FormatTextForMessage. Since the text has already been converted from HTML and stripped before being sent to FormatTextForMessage, either update FormatTextForMessage to have a parameter or overload that accepts already-processed text and skips the HTML conversion and stripping steps, or refactor the code flow to avoid calling both the initial HTML processing steps and the processing inside FormatTextForMessage on the same text content.
🧹 Nitpick comments (2)
Core/Resgrid.Services/SmsService.cs (2)
22-32: 💤 Low valueConstructor injection should use Service Locator pattern.
The coding guidelines specify using
Bootstrapper.GetKernel().Resolve<T>()in constructors rather than constructor injection. Consider resolvingICacheProviderexplicitly.♻️ Suggested refactor to align with Service Locator pattern
- public SmsService(IUserProfileService userProfileService, IGeoLocationProvider geoLocationProvider, - ITextMessageProvider textMessageProvider, IDepartmentSettingsService departmentSettingsService, - IEmailSender emailSender, ISubscriptionsService subscriptionsService, ICacheProvider cacheProvider) + public SmsService(IUserProfileService userProfileService, IGeoLocationProvider geoLocationProvider, + ITextMessageProvider textMessageProvider, IDepartmentSettingsService departmentSettingsService, + IEmailSender emailSender, ISubscriptionsService subscriptionsService) { _userProfileService = userProfileService; _geoLocationProvider = geoLocationProvider; _textMessageProvider = textMessageProvider; _departmentSettingsService = departmentSettingsService; _emailSender = emailSender; _subscriptionsService = subscriptionsService; - _cacheProvider = cacheProvider; + _cacheProvider = Bootstrapper.GetKernel().Resolve<ICacheProvider>(); }🤖 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/SmsService.cs` around lines 22 - 32, The SmsService constructor is using constructor injection for dependencies, but the coding guidelines require using the Service Locator pattern instead. Remove the ICacheProvider parameter from the SmsService constructor signature and the corresponding assignment statement for _cacheProvider. Instead, resolve the ICacheProvider explicitly within the constructor body using Bootstrapper.GetKernel().Resolve<ICacheProvider>() and assign it to the _cacheProvider field to align with the Service Locator pattern guidelines.Source: Coding guidelines
501-522: ⚡ Quick winConsider a separate overload for pre-processed text.
FormatTextForMessagealways runs HTML conversion, but many call sites already process the text before calling. Consider either:
- Adding an overload that skips HTML processing for pre-processed inputs, or
- Renaming this method to clarify it expects raw HTML input
Current implementation works but has the redundant processing issue noted earlier.
♻️ Suggested: Add overload for pre-processed text
// For raw HTML input - processes and formats private string FormatTextForMessage(string title, string body, bool includeOptOut) { string text = HtmlToTextHelper.ConvertHtml(body); text = StringHelpers.StripHtmlTagsCharArray(text); return FormatTextForMessageInternal(title, text, includeOptOut); } // For already-processed text - just formats private string FormatTextForMessageInternal(string title, string processedBody, bool includeOptOut) { return AppendOptOutFooter(String.Format("{0} {1} : {2}", title, SmsViaResgrid, processedBody), includeOptOut); }🤖 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/SmsService.cs` around lines 501 - 522, The FormatTextForMessage method performs redundant HTML processing when call sites have already processed the text. Refactor by creating a new private method called FormatTextForMessageInternal that accepts a pre-processed text parameter and handles only the formatting logic by calling AppendOptOutFooter with the String.Format call. Then modify the existing FormatTextForMessage method to keep its current signature and HTML processing steps using HtmlToTextHelper.ConvertHtml and StringHelpers.StripHtmlTagsCharArray, but have it delegate to FormatTextForMessageInternal with the processed result instead of directly calling AppendOptOutFooter. This allows pre-processed call sites to use the new internal method while maintaining backward compatibility for raw HTML inputs.
🤖 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/SmsService.cs`:
- Around line 57-60: The message body is being processed for HTML conversion and
stripping twice: once inline with HtmlToTextHelper.ConvertHtml() and
StringHelpers.StripHtmlTagsCharArray() before passing to FormatTextForMessage(),
and again inside FormatTextForMessage() itself. Remove the inline HTML
processing calls (HtmlToTextHelper.ConvertHtml and
StringHelpers.StripHtmlTagsCharArray) and pass the raw message.Body directly to
FormatTextForMessage(), allowing that method to handle all HTML processing in
one place. This consolidation should be applied to both occurrences of this
pattern in the file.
---
Outside diff comments:
In `@Core/Resgrid.Services/SmsService.cs`:
- Around line 172-174: In the SendCallAsync method, the text processing from
call.NatureOfCall is being done twice: first at the HtmlToTextHelper.ConvertHtml
and StringHelpers.StripHtmlTagsCharArray calls where the address is also
appended, and then again when the result is passed to FormatTextForMessage.
Since the text has already been converted from HTML and stripped before being
sent to FormatTextForMessage, either update FormatTextForMessage to have a
parameter or overload that accepts already-processed text and skips the HTML
conversion and stripping steps, or refactor the code flow to avoid calling both
the initial HTML processing steps and the processing inside FormatTextForMessage
on the same text content.
---
Nitpick comments:
In `@Core/Resgrid.Services/SmsService.cs`:
- Around line 22-32: The SmsService constructor is using constructor injection
for dependencies, but the coding guidelines require using the Service Locator
pattern instead. Remove the ICacheProvider parameter from the SmsService
constructor signature and the corresponding assignment statement for
_cacheProvider. Instead, resolve the ICacheProvider explicitly within the
constructor body using Bootstrapper.GetKernel().Resolve<ICacheProvider>() and
assign it to the _cacheProvider field to align with the Service Locator pattern
guidelines.
- Around line 501-522: The FormatTextForMessage method performs redundant HTML
processing when call sites have already processed the text. Refactor by creating
a new private method called FormatTextForMessageInternal that accepts a
pre-processed text parameter and handles only the formatting logic by calling
AppendOptOutFooter with the String.Format call. Then modify the existing
FormatTextForMessage method to keep its current signature and HTML processing
steps using HtmlToTextHelper.ConvertHtml and
StringHelpers.StripHtmlTagsCharArray, but have it delegate to
FormatTextForMessageInternal with the processed result instead of directly
calling AppendOptOutFooter. This allows pre-processed call sites to use the new
internal method while maintaining backward compatibility for raw HTML inputs.
🪄 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: 00e9da25-e346-46f3-81b0-9fbe75b3c0b4
📒 Files selected for processing (1)
Core/Resgrid.Services/SmsService.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