RG-T117 Changes to Chatbot and Chat UI #450
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
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
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
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.Threading.Tasks; | ||
| using Newtonsoft.Json; | ||
| using Resgrid.Config; | ||
| using Resgrid.Framework; | ||
| using Resgrid.Model; | ||
| using Resgrid.Model.Events; | ||
| using Resgrid.Model.Providers; | ||
|
|
@@ -84,12 +85,51 @@ async Task<List<ChatChannel>> getChannels() | |
| if (departmentChannel != null) | ||
| results[departmentChannel.ChatChannelId] = departmentChannel; | ||
|
|
||
| // Loaded once: source for the admin group-channel matching here AND the | ||
| // implicit-audience pass further down. | ||
| var allChannels = await _chatChannelRepository.GetAllByDepartmentIdAsync(departmentId, includeArchived); | ||
|
There was a problem hiding this comment.
Unhandled exception propagation occurs if Kody rule violation: Handle async operations with proper error handling Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| // Department admins get every group's default channel; everyone else gets only the group | ||
| // they belong to. Existing channels come from the bulk load above — only groups with no | ||
| // channel yet hit the provisioning path, and a failure there is contained per group so one | ||
| // bad group can never blank the admin's whole channel list. | ||
| if (await _chatPermissionService.IsDepartmentAdminAsync(departmentId, userId)) | ||
| { | ||
| var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId); | ||
| if (allGroups != null && allGroups.Count > 0) | ||
| { | ||
| var groupChannelsByGroupId = allChannels? | ||
| .Where(c => c.ChannelType == (int)ChatChannelType.GroupDefault && c.GroupId.HasValue) | ||
| .GroupBy(c => c.GroupId.Value) | ||
| .ToDictionary(g => g.Key, g => g.First()); | ||
|
Comment on lines
+101
to
+104
There was a problem hiding this comment.
Complex multi-step LINQ chain reduces readability and debuggability. Extract the Kody rule violation: Limit Lengthy LINQ Chains Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| foreach (var departmentGroup in allGroups) | ||
| { | ||
| try | ||
| { | ||
| ChatChannel groupChannel; | ||
| if (groupChannelsByGroupId == null || !groupChannelsByGroupId.TryGetValue(departmentGroup.DepartmentGroupId, out groupChannel)) | ||
| groupChannel = await EnsureGroupChannelAsync(departmentGroup); | ||
|
|
||
| if (groupChannel != null) | ||
| results[groupChannel.ChatChannelId] = groupChannel; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex); | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+96
to
+122
There was a problem hiding this comment.
Query amplification in // Fetch all existing group-default channels for this department in a single query.
var existingGroupChannels = await _chatChannelRepository.GetAllGroupChannelsByDepartmentAsync(departmentId);
var channelByGroupId = existingGroupChannels?.ToDictionary(c => c.GroupId.GetValueOrDefault()) ?? new Dictionary<int, ChatChannel>();
var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId);
if (allGroups != null)
{
foreach (var departmentGroup in allGroups)
{
if (channelByGroupId.TryGetValue(departmentGroup.DepartmentGroupId, out var existing) && existing != null)
{
results[existing.ChatChannelId] = existing;
continue;
}
// Only provision (query + insert) for groups that don't have a channel yet.
var groupChannel = await EnsureGroupChannelAsync(departmentGroup);
if (groupChannel != null)
results[groupChannel.ChatChannelId] = groupChannel;
}
}Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| } | ||
|
Comment on lines
+96
to
+123
There was a problem hiding this comment.
N+1 query issue in // Fetch all existing GroupDefault channels for the department in one query instead of one
// GetByGroupIdAsync round-trip per group (the per-group EnsureGroupChannelAsync N+1).
if (await _chatPermissionService.IsDepartmentAdminAsync(departmentId, userId))
{
var allGroups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId);
if (allGroups != null && allGroups.Count > 0)
{
var groupIds = allGroups.Select(g => g.DepartmentGroupId).ToHashSet();
var existingGroupChannels = await _chatChannelRepository.GetByGroupIdsAsync(groupIds);
var byGroupId = (existingGroupChannels ?? Enumerable.Empty<ChatChannel>())
.Where(c => c.ChannelType == (int)ChatChannelType.GroupDefault)
.ToDictionary(c => c.GroupId);
foreach (var departmentGroup in allGroups)
{
if (byGroupId.TryGetValue(departmentGroup.DepartmentGroupId, out var existing) && existing != null)
{
results[existing.ChatChannelId] = existing;
}
else
{
var groupChannel = await EnsureGroupChannelAsync(departmentGroup);
if (groupChannel != null)
results[groupChannel.ChatChannelId] = groupChannel;
}
}
}
}Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| else | ||
| { | ||
| var group = await _departmentGroupsService.GetGroupForUserAsync(userId, departmentId); | ||
| if (group != null) | ||
| { | ||
| var groupChannel = await EnsureGroupChannelAsync(group); | ||
| if (groupChannel != null) | ||
| results[groupChannel.ChatChannelId] = groupChannel; | ||
|
Comment on lines
+129
to
+131
There was a problem hiding this comment.
Code duplication increases maintenance burden because the Kody rule violation: Extract duplicated logic into functions Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| } | ||
| } | ||
|
|
||
| // Chatbot channels are provisioned when a chatbot session starts — the list path only | ||
|
|
@@ -115,7 +155,6 @@ async Task<List<ChatChannel>> getChannels() | |
|
|
||
| // Implicit-audience channels (custom rule-based + active incident channels): evaluate access | ||
| // per channel; evaluations are cached by the permission service. | ||
| if (allChannels != null) | ||
| { | ||
| foreach (var channel in allChannels) | ||
|
|
||
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 |
|---|---|---|
|
|
@@ -682,7 +682,7 @@ private async Task AddUnitCrewAsync(int unitId, HashSet<string> userIds) | |
| AddIfSet(userIds, role.UserId); | ||
| } | ||
|
|
||
| public async Task<bool> IsDepartmentAdminAsync(int departmentId, string userId) | ||
|
There was a problem hiding this comment.
Overly broad access modifier widens the API surface unnecessarily by changing Kody rule violation: Use private access modifiers for encapsulation Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| { | ||
| return await _authorizationService.CanUserModifyDepartmentAsync(userId, departmentId); | ||
| } | ||
|
|
||
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 |
|---|---|---|
|
|
@@ -64,8 +64,20 @@ public FeatureToggleService(IFeatureFlagRepository featureFlagRepository, IFeatu | |
|
|
||
| public async Task<bool> IsEnabledAsync(string key, int departmentId, bool defaultValue = false, IDictionary<string, string> context = null) | ||
| { | ||
| try | ||
| { | ||
| var evaluation = await EvaluateInternalAsync(key, departmentId, context, defaultValue, new HashSet<int>()); | ||
| return evaluation.IsEnabled; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Fail shut: a flag-store outage (cache AND database unreachable) must never 500 every | ||
| // gated page/endpoint — the feature simply reads as disabled until the store recovers. | ||
| // Deliberately ignores defaultValue here: that is the "flag not defined" default, not | ||
| // the "evaluation infrastructure down" answer. | ||
| Logging.LogException(ex, $"FeatureToggle evaluation failed for '{key}' in department {departmentId}; failing shut (disabled)"); | ||
|
There was a problem hiding this comment.
Unstructured logging detected in Kody rule violation: Include error context in structured logs Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| return false; | ||
| } | ||
|
Comment on lines
+67
to
+80
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Keep plan-gate failures fail-closed.
Proposed plan-gate failure handlingcatch (Exception ex)
{
Logging.LogException(ex, $"FeatureToggle plan gate check failed for department {departmentId}");
- return true;
+ return false;
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| public async Task<FeatureFlagEvaluation> EvaluateAsync(string key, int departmentId, IDictionary<string, string> context = null) | ||
|
|
||
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 |
|---|---|---|
|
|
@@ -524,6 +524,107 @@ public async Task implicit_channel_without_membership_should_create_row() | |
| } | ||
| } | ||
|
|
||
| [TestFixture] | ||
| public class when_listing_channels_for_users : with_the_chat_channel_service | ||
| { | ||
| private ChatChannel SetupDepartmentChannel() | ||
| { | ||
| var departmentChannel = new ChatChannel | ||
| { | ||
| ChatChannelId = "dept-chan", | ||
| DepartmentId = 1, | ||
| ChannelType = (int)ChatChannelType.DepartmentDefault, | ||
| Name = "First Battalion", | ||
| CreatedOn = DateTime.UtcNow | ||
| }; | ||
| _chatChannelRepositoryMock.Setup(x => x.GetDepartmentDefaultAsync(1)).ReturnsAsync(departmentChannel); | ||
| return departmentChannel; | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task department_admin_should_get_every_group_channel_provisioned() | ||
| { | ||
| SetupDepartmentChannel(); | ||
| _chatPermissionServiceMock.Setup(x => x.IsDepartmentAdminAsync(1, "admin-a")).ReturnsAsync(true); | ||
| _departmentGroupsServiceMock.Setup(x => x.GetAllGroupsForDepartmentAsync(1)).ReturnsAsync(new List<DepartmentGroup> | ||
| { | ||
| new DepartmentGroup { DepartmentGroupId = 9, DepartmentId = 1, Name = "Station 1" }, | ||
| new DepartmentGroup { DepartmentGroupId = 10, DepartmentId = 1, Name = "Station 2" } | ||
| }); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetByGroupIdAsync(It.IsAny<int>())).ReturnsAsync((ChatChannel)null); | ||
|
There was a problem hiding this comment.
Type casting violation requires using the Kody rule violation: Use safe type casting with as operator Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| var result = await _chatChannelService.GetChannelsForUserAsync(1, "admin-a", null); | ||
|
|
||
| result.Should().Contain(c => c.ChannelType == (int)ChatChannelType.GroupDefault && c.GroupId == 9); | ||
| result.Should().Contain(c => c.ChannelType == (int)ChatChannelType.GroupDefault && c.GroupId == 10); | ||
| _chatChannelRepositoryMock.Verify(x => x.InsertAsync(It.Is<ChatChannel>(c => c.ChannelType == (int)ChatChannelType.GroupDefault), | ||
| It.IsAny<CancellationToken>(), It.IsAny<bool>()), Times.Exactly(2)); | ||
| _departmentGroupsServiceMock.Verify(x => x.GetGroupForUserAsync(It.IsAny<string>(), It.IsAny<int>()), Times.Never); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task admin_existing_group_channels_should_come_from_bulk_load_without_per_group_lookups() | ||
| { | ||
| SetupDepartmentChannel(); | ||
| _chatPermissionServiceMock.Setup(x => x.IsDepartmentAdminAsync(1, "admin-a")).ReturnsAsync(true); | ||
| _departmentGroupsServiceMock.Setup(x => x.GetAllGroupsForDepartmentAsync(1)).ReturnsAsync(new List<DepartmentGroup> | ||
| { | ||
| new DepartmentGroup { DepartmentGroupId = 9, DepartmentId = 1, Name = "Station 1" }, | ||
| new DepartmentGroup { DepartmentGroupId = 10, DepartmentId = 1, Name = "Station 2" } | ||
| }); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetAllByDepartmentIdAsync(1, false)).ReturnsAsync(new List<ChatChannel> | ||
| { | ||
| new ChatChannel { ChatChannelId = "group-9", DepartmentId = 1, ChannelType = (int)ChatChannelType.GroupDefault, GroupId = 9, Name = "Station 1" }, | ||
| new ChatChannel { ChatChannelId = "group-10", DepartmentId = 1, ChannelType = (int)ChatChannelType.GroupDefault, GroupId = 10, Name = "Station 2" } | ||
| }); | ||
|
|
||
| var result = await _chatChannelService.GetChannelsForUserAsync(1, "admin-a", null); | ||
|
|
||
| result.Should().Contain(c => c.ChatChannelId == "group-9"); | ||
| result.Should().Contain(c => c.ChatChannelId == "group-10"); | ||
| _chatChannelRepositoryMock.Verify(x => x.GetByGroupIdAsync(It.IsAny<int>()), Times.Never); | ||
| _chatChannelRepositoryMock.Verify(x => x.InsertAsync(It.IsAny<ChatChannel>(), It.IsAny<CancellationToken>(), It.IsAny<bool>()), Times.Never); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task admin_single_group_provisioning_failure_should_not_abort_the_channel_list() | ||
| { | ||
| SetupDepartmentChannel(); | ||
| _chatPermissionServiceMock.Setup(x => x.IsDepartmentAdminAsync(1, "admin-a")).ReturnsAsync(true); | ||
| _departmentGroupsServiceMock.Setup(x => x.GetAllGroupsForDepartmentAsync(1)).ReturnsAsync(new List<DepartmentGroup> | ||
| { | ||
| new DepartmentGroup { DepartmentGroupId = 9, DepartmentId = 1, Name = "Station 1" }, | ||
| new DepartmentGroup { DepartmentGroupId = 10, DepartmentId = 1, Name = "Station 2" } | ||
| }); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetByGroupIdAsync(9)).ThrowsAsync(new InvalidOperationException("db down")); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetByGroupIdAsync(10)).ReturnsAsync((ChatChannel)null); | ||
|
There was a problem hiding this comment.
Type casting rule violation detected. Use the Kody rule violation: Use safe type casting with as operator Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| var result = await _chatChannelService.GetChannelsForUserAsync(1, "admin-a", null); | ||
|
|
||
| result.Should().Contain(c => c.ChannelType == (int)ChatChannelType.GroupDefault && c.GroupId == 10); | ||
| result.Should().Contain(c => c.ChannelType == (int)ChatChannelType.DepartmentDefault); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task non_admin_should_only_get_their_own_group_channel() | ||
| { | ||
| SetupDepartmentChannel(); | ||
| _chatPermissionServiceMock.Setup(x => x.IsDepartmentAdminAsync(1, "user-a")).ReturnsAsync(false); | ||
| _departmentGroupsServiceMock.Setup(x => x.GetGroupForUserAsync("user-a", 1)).ReturnsAsync(new DepartmentGroup | ||
| { | ||
| DepartmentGroupId = 9, | ||
| DepartmentId = 1, | ||
| Name = "Station 1" | ||
| }); | ||
| _chatChannelRepositoryMock.Setup(x => x.GetByGroupIdAsync(9)).ReturnsAsync((ChatChannel)null); | ||
|
|
||
| var result = await _chatChannelService.GetChannelsForUserAsync(1, "user-a", null); | ||
|
|
||
| result.Should().ContainSingle(c => c.ChannelType == (int)ChatChannelType.GroupDefault).Which.GroupId.Should().Be(9); | ||
| _departmentGroupsServiceMock.Verify(x => x.GetAllGroupsForDepartmentAsync(It.IsAny<int>()), Times.Never); | ||
| } | ||
| } | ||
|
|
||
| [TestFixture] | ||
| public class when_creating_ad_hoc_group_channels : with_the_chat_channel_service | ||
| { | ||
|
|
||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the unknown-flag contract with
BuildDefault.When
FeatureFlagsConfig.CodeDefaultscontains the key,BuildDefaultreturns that configured value instead of thedefaultValuemethod argument. Update this documentation to describe that precedence, or change the implementation to honordefaultValue.Proposed documentation correction
📝 Committable suggestion
🤖 Prompt for AI Agents