RG-T133 Customer bug fixes #482
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
|
|
@@ -174,42 +175,105 @@ public void ClearAllUserProfilesFromCache(int departmentId) | |
|
|
||
| public async Task<UserProfile> GetProfileByMobileNumberAsync(string number) | ||
| { | ||
| return await FindProfileByPhoneAsync(number, | ||
|
There was a problem hiding this comment.
Unhandled exception path in Core/Resgrid.Services/UserProfileService.cs at lines 185, 211, and this await site: return await FindProfileByPhoneAsync(number, ...) propagates repository lookup failures without contextual diagnostics. Wrap the await in try/catch, log the failure with the phone number, and then rethrow or map the exception to an application error. Kody rule violation: Handle async operations with proper error handling try
{
return await FindProfileByPhoneAsync(number,
_userProfileRepository.GetProfileByMobileNumberAsync,
profile => profile.MobileNumberVerified);
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to get profile by mobile number for {PhoneNumber}", number);
throw;
}Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. There was a problem hiding this comment.
Missing structured error logging in Core/Resgrid.Services/UserProfileService.cs at lines 185, 211, and this await site obscures failures in FindProfileByPhoneAsync(number, ...) and removes operation and identifier context from diagnostics. Add a try/catch that logs the operation name and input phone number as structured fields before rethrowing. Kody rule violation: Include error context in structured logs try
{
return await FindProfileByPhoneAsync(number,
_userProfileRepository.GetProfileByMobileNumberAsync,
profile => profile.MobileNumberVerified);
}
catch (Exception ex)
{
_logger.LogError(ex, "GetProfileByMobileNumberAsync failed", new { operation = nameof(GetProfileByMobileNumberAsync), phoneNumber = number });
throw;
}Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| _userProfileRepository.GetProfileByMobileNumberAsync, | ||
| profile => profile.MobileNumberVerified); | ||
| } | ||
|
|
||
| public async Task<UserProfile> GetProfileByHomeNumberAsync(string number) | ||
| { | ||
| return await FindProfileByPhoneAsync(number, | ||
| _userProfileRepository.GetProfileByHomeNumberAsync, | ||
| profile => profile.HomeNumberVerified); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Resolves the profile that owns a phone number, preferring one that has actually proven it. | ||
| /// <para> | ||
| /// The same number can sit on more than one profile - a stale or secondary account, or someone | ||
| /// who mistyped it and never completed verification. A profile that verified the number is the | ||
| /// only one that has demonstrated possession, so it wins outright, even over a closer match on | ||
| /// the number's shape. Everything else falls back to candidate order (the number exactly as | ||
| /// dialled before its country-code variant). | ||
| /// </para> | ||
| /// <para> | ||
| /// Within a single candidate the query does the same ranking, so this only has to arbitrate | ||
| /// between candidates. | ||
| /// </para> | ||
| /// </summary> | ||
| private static async Task<UserProfile> FindProfileByPhoneAsync(string number, | ||
| Func<string, Task<UserProfile>> lookup, Func<UserProfile, bool?> isVerified) | ||
| { | ||
| UserProfile unverifiedMatch = null; | ||
|
|
||
| foreach (var candidate in PhoneLookupCandidates(number)) | ||
| { | ||
| var profile = await lookup(candidate); | ||
|
There was a problem hiding this comment.
Unwrapped external call boundary in Core/Resgrid.Services/UserProfileService.cs: var profile = await lookup(candidate); can propagate raw infrastructure exceptions without candidate context. Wrap the lookup delegate invocation in try/catch, log the candidate, and rethrow or map the exception. Kody rule violation: Add try-catch blocks for external calls try
{
var profile = await lookup(candidate);
// existing logic
}
catch (Exception ex)
{
_logger.LogError(ex, "External lookup failed for {Candidate}", candidate);
throw;
}Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| if (profile == null) | ||
| continue; | ||
|
|
||
| if (isVerified(profile) == true) | ||
| return profile; | ||
|
|
||
| // Keep the first one found so a candidate that matches nothing verified still resolves, | ||
| // but keep looking in case a later candidate did verify the number. | ||
| unverifiedMatch ??= profile; | ||
| } | ||
|
|
||
| return unverifiedMatch; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The stored numbers a lookup should be tried against, most-specific first. | ||
| /// <para> | ||
| /// Profiles are saved in E.164 (+12015550123) while inbound SMS and voice hand us the number in | ||
| /// whatever shape the carrier used, so a lookup has to cover the country code being present on | ||
| /// one side but not the other. The leading "+" is covered by the query itself, which matches the | ||
| /// stored value both bare and plus-prefixed. | ||
| /// </para> | ||
| /// <para> | ||
| /// The order matters and the candidates are tried one at a time rather than matched together: | ||
| /// 2015550123 and 12015550123 can be two different profiles, and the repository takes | ||
| /// FirstOrDefault() with no ORDER BY. Asking for the number exactly as dialled first means the | ||
| /// country-code variant is only ever reached as a fallback. | ||
| /// </para> | ||
| /// </summary> | ||
| private static IEnumerable<string> PhoneLookupCandidates(string number) | ||
| { | ||
| var digits = NormalizePhoneNumber(number); | ||
|
|
||
| // A blank inbound number must never match: the stored column can also be blank and an | ||
| // empty-to-empty compare would hand back an arbitrary profile. | ||
| if (string.IsNullOrWhiteSpace(digits)) | ||
| yield break; | ||
|
|
||
| yield return digits; | ||
|
|
||
| if (digits.Length == 11 && digits[0] == '1') | ||
| yield return digits.Substring(1); | ||
| else if (digits.Length == 10) | ||
| yield return "1" + digits; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reduces a number to bare digits. Inbound numbers arrive formatted in assorted ways | ||
| /// ("+1 (201) 555-0123"), and only the digits are comparable against a stored number. | ||
| /// </summary> | ||
| private static string NormalizePhoneNumber(string number) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(number)) | ||
| return null; | ||
|
|
||
| var digits = new StringBuilder(number.Length); | ||
|
|
||
| foreach (var character in number) | ||
| { | ||
| if (character >= '0' && character <= '9') | ||
| digits.Append(character); | ||
| } | ||
|
|
||
| return digits.ToString(); | ||
| } | ||
|
|
||
| public async Task<List<UserProfile>> GetSelectedUserProfilesAsync(List<string> userIds) | ||
|
|
||
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid gateway address formatting in
Core/Resgrid.Services/SmsService.cs:ResolveGatewayNumberalways returnsPhoneNumberResult.LocalNumberfor carrier-gateway sends, but several carriers inCarriersMaprequire country-code digits in the mailbox address. Select the gateway format per carrier and preserveprocessed.InternationalNumberwithout the leading+for carriers such asRogersWirelessand the UK gateway carriers.Prompt for LLM
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.