Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughAdds a feature toggle subsystem: domain models, repository contracts and implementations, a layered FeatureToggleService with evaluation, management, analytics, caching and audit, v4 REST controller and DTOs, SQL/PG migrations, DI wiring, a worker to flush usage, Twilio integration gating, and review config updates. ChangesFeature Toggle System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 6
🧹 Nitpick comments (7)
Core/Resgrid.Model/FeatureToggles/FeatureFlag.cs (2)
107-111: 💤 Low valueConsider null-safe casting in IdValue setter.
The setter performs an unsafe cast that will throw
InvalidCastExceptionifvalueis null or not anint. Adding a null check or using pattern matching would make this more defensive.🛡️ Defensive casting option
[NotMapped] [JsonIgnore] public object IdValue { get { return FeatureFlagId; } - set { FeatureFlagId = (int)value; } + set { FeatureFlagId = value as int? ?? 0; } }🤖 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.Model/FeatureToggles/FeatureFlag.cs` around lines 107 - 111, The IdValue setter currently casts unsafely and can throw for null or wrong types; change it to perform a null-safe/pattern-match assignment: use "if (value is int v) FeatureFlagId = v; else if (value == null) FeatureFlagId = 0; else throw new ArgumentException(...)" (referencing the IdValue property and FeatureFlagId field) so you defensively handle null, accept ints, and provide a clear error for other types.
123-123: ⚡ Quick winOptimize IgnoredProperties to avoid repeated allocations.
The
IgnoredPropertiesproperty allocates a new array on every access. Since the value is constant, making it a static readonly field eliminates unnecessary allocations.♻️ Proposed optimization
+ private static readonly IEnumerable<string> _ignoredProperties = new[] { "IdValue", "IdType", "TableName", "IdName" }; + [NotMapped] - public IEnumerable<string> IgnoredProperties => new string[] { "IdValue", "IdType", "TableName", "IdName" }; + public IEnumerable<string> IgnoredProperties => _ignoredProperties; } }🤖 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.Model/FeatureToggles/FeatureFlag.cs` at line 123, The IgnoredProperties getter on class FeatureFlag currently allocates a new string[] on each access; change it to return a single pre-allocated static readonly collection to avoid repeated allocations. Add a private static readonly string[] (or IReadOnlyList<string>) field (e.g., s_ignoredProperties) initialized once with "IdValue","IdType","TableName","IdName" and have the public IgnoredProperties property return that field (preserve the IEnumerable<string> return type and keep the values immutable).Core/Resgrid.Config/FeatureFlagsConfig.cs (1)
21-21: 💤 Low valueConsider using modern C# target-typed
new()syntax.The explicit type in object initializers can be simplified using C# 9+ target-typed
new()expressions for improved readability.♻️ Modernize initialization syntax
- public static int CacheDurationMinutes = 60; + public static int CacheDurationMinutes = 60; - public static int EvaluationFlushIntervalSeconds = 60; + public static int EvaluationFlushIntervalSeconds = 60; - public static int StaleFlagThresholdDays = 90; + public static int StaleFlagThresholdDays = 90; - public static Dictionary<string, bool> CodeDefaults = new Dictionary<string, bool>(); + public static Dictionary<string, bool> CodeDefaults = new();Also applies to: 30-30, 33-33, 39-39
🤖 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.Config/FeatureFlagsConfig.cs` at line 21, In FeatureFlagsConfig replace explicit constructor types in field/property initializers with C# target-typed new() syntax: locate the object initializers in the FeatureFlagsConfig class (the entries referenced around lines 30, 33 and 39) and change patterns like "SomeType = new SomeType(...)" to "SomeType = new(...)" (leave primitives like CacheDurationMinutes as-is); ensure using directives and language version support (C# 9+) remain compatible.Core/Resgrid.Services/FeatureToggleService.cs (1)
205-223: ⚡ Quick winUse
plan.PlanIdfor the plan gate (but consider clarifying naming/ordering)
Core/Resgrid.Model/Planhas no separate ordered tier/type field—onlyPlanId(plusFrequency,Name, etc.). The gating fieldCore/Resgrid.Model/FeatureToggles/FeatureFlag.MinimumPlanTypeis documented as a “minimum subscription plan id required”, andFeatureToggleServiceconsistently usesplan.PlanIdboth forPassesPlanGateAsyncand for theFeatureFlagAttributeTypes.PlanTypeattribute (plan?.PlanId).
Optional: renameMinimumPlanType/PlanTypeto reflect that it’s a plan id (and/or avoid relying onPlanIdnumeric ordering by mapping to an explicit tier enum).🤖 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/FeatureToggleService.cs` around lines 205 - 223, PassesPlanGateAsync should compare the department's PlanId to the feature flag's MinimumPlanType (i.e., treat MinimumPlanType as a plan id) — ensure the method uses plan.PlanId in its comparison logic (and that any other consumer like FeatureFlagAttributeTypes.PlanType also reads plan?.PlanId) so the gate checks "plan.PlanId >= MinimumPlanType"; optionally consider renaming MinimumPlanType/PlanType to indicate they're plan IDs or introduce a mapping to an explicit tier enum to avoid relying on numeric ordering.Web/Resgrid.Web.Services/Controllers/v4/FeatureTogglesController.cs (1)
70-79: ⚡ Quick winFix: no
NullReferenceExceptionrisk here; handle unknown-key response semantics instead
_featureToggleService.EvaluateAsync(key, DepartmentId)flows intoEvaluateInternalAsync, which returnsBuildDefault/Buildfor unknown keys—both always construct a non-nullFeatureFlagEvaluation(andSource/ValueTypeare non-null enums). SoMapEvaluation(evaluation)won’t NRE inGet(includingMapEvaluationat ~534-545).If the API contract expects a 404 for unknown keys, update
Getto checkevaluation.Source == FeatureFlagEvaluationSource.NotFoundand callResponseHelper.PopulateV4ResponseNotFound(result)instead of returningResponseHelper.Successunconditionally.🤖 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 `@Web/Resgrid.Web.Services/Controllers/v4/FeatureTogglesController.cs` around lines 70 - 79, The Get method currently returns a success response unconditionally; update it to handle unknown feature keys by checking the evaluation.Source from _featureToggleService.EvaluateAsync(key, DepartmentId) (the returned FeatureFlagEvaluation), and if evaluation.Source == FeatureFlagEvaluationSource.NotFound then set result.Status via ResponseHelper.PopulateV4ResponseNotFound(result) (instead of ResponseHelper.Success); otherwise leave the existing behavior (map with MapEvaluation, set PageSize, call PopulateV4ResponseData and return result).Providers/Resgrid.Providers.Migrations/Migrations/M0071_AddingFeatureToggles.cs (1)
41-58: ⚡ Quick winConsider adding indexes for department-specific queries.
Queries filtering by
DepartmentIdalone (withoutFeatureFlagId) would benefit from dedicated indexes onFeatureFlagOverrides.DepartmentIdandFeatureFlagUsages.DepartmentId.📊 Proposed additional indexes
Create.Index("UX_FeatureFlagOverrides_Flag_Department") .OnTable("FeatureFlagOverrides") .OnColumn("FeatureFlagId").Ascending() .OnColumn("DepartmentId").Ascending() .WithOptions().Unique(); + + Create.Index("IX_FeatureFlagOverrides_Department") + .OnTable("FeatureFlagOverrides") + .OnColumn("DepartmentId").Ascending(); // FeatureFlagTargetingRules - attribute/segment targeting rules.And after the FeatureFlagUsages index:
Create.Index("IX_FeatureFlagUsages_Flag_Date") .OnTable("FeatureFlagUsages") .OnColumn("FeatureFlagId").Ascending() .OnColumn("UsageDate").Ascending(); + + Create.Index("IX_FeatureFlagUsages_Department") + .OnTable("FeatureFlagUsages") + .OnColumn("DepartmentId").Ascending(); }Also applies to: 92-104
🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0071_AddingFeatureToggles.cs` around lines 41 - 58, Add single-column indexes for department-only queries by creating non-unique indexes on the DepartmentId columns: add an index on FeatureFlagOverrides.DepartmentId (e.g., name UX_FeatureFlagOverrides_Department) in the same migration near the existing UX_FeatureFlagOverrides_Flag_Department definition, and add a similar non-unique index on FeatureFlagUsages.DepartmentId (e.g., UX_FeatureFlagUsages_Department) near where FeatureFlagUsages is created; use Create.Index(...).OnTable(...).OnColumn("DepartmentId").Ascending() to implement both.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs (1)
41-58: ⚡ Quick winConsider adding indexes for department-specific queries.
Same recommendation as SQL Server migration: add indexes on
DepartmentIdcolumns for department-filtered queries.📊 Proposed additional indexes
Create.Index("UX_FeatureFlagOverrides_Flag_Department".ToLower()) .OnTable("FeatureFlagOverrides".ToLower()) .OnColumn("FeatureFlagId".ToLower()).Ascending() .OnColumn("DepartmentId".ToLower()).Ascending() .WithOptions().Unique(); + + Create.Index("IX_FeatureFlagOverrides_Department".ToLower()) + .OnTable("FeatureFlagOverrides".ToLower()) + .OnColumn("DepartmentId".ToLower()).Ascending(); // FeatureFlagTargetingRules - attribute/segment targeting rules.And after the FeatureFlagUsages index:
Create.Index("IX_FeatureFlagUsages_Flag_Date".ToLower()) .OnTable("FeatureFlagUsages".ToLower()) .OnColumn("FeatureFlagId".ToLower()).Ascending() .OnColumn("UsageDate".ToLower()).Ascending(); + + Create.Index("IX_FeatureFlagUsages_Department".ToLower()) + .OnTable("FeatureFlagUsages".ToLower()) + .OnColumn("DepartmentId".ToLower()).Ascending(); }Also applies to: 92-104
🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs` around lines 41 - 58, The migration creates a unique composite index UX_FeatureFlagOverrides_Flag_Department on FeatureFlagId+DepartmentId but lacks a standalone index to support department-filtered queries; add non-unique indexes on the DepartmentId column for FeatureFlagOverrides (and likewise add DepartmentId indexes for FeatureFlags and FeatureFlagUsages where those tables are defined) by creating Create.Index calls targeting "DepartmentId" on the respective tables so department-scoped lookups use an index in addition to the existing composite UX_FeatureFlagOverrides_Flag_Department.
🤖 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.Config/FeatureFlagsConfig.cs`:
- Line 39: The static mutable Dictionary CodeDefaults is not thread-safe;
replace it with a thread-safe collection (e.g., change the type to
System.Collections.Concurrent.ConcurrentDictionary<string,bool> CodeDefaults and
update its initialization) or make it immutable/readonly and populate it at
startup to prevent concurrent writes; update any callers that use
Add/Remove/Indexer on CodeDefaults to use TryAdd/TryUpdate/TryRemove or to work
with the immutable pattern so concurrent access is safe.
In `@Core/Resgrid.Services/FeatureToggleService.cs`:
- Around line 791-799: RecordEvaluation currently only increments counter[0]
(EvaluationCount) and never updates counter[1]/counter[2]
(EnabledCount/DisabledCount), because EvaluateInternalAsync calls
RecordEvaluation from a finally without the resolved state; modify
EvaluateInternalAsync to capture the final FeatureFlagEvaluation result in a
local variable before returning (or route returns through a small helper) and
call RecordEvaluation(featureFlagId, departmentId, evaluation.IsEnabled) so
RecordEvaluation can increment counter[1] or counter[2] accordingly; update
RecordEvaluation signature to accept a nullable bool isEnabled, use
FeatureFlagsConfig.TrackEvaluations and _usageBuffer/GetOrAdd as before, and
ensure FlushEvaluationsAsync continues reading counter[0..2] to persist
EvaluationCount, EnabledCount, DisabledCount.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0071_AddingFeatureToggles.cs`:
- Around line 41-52: The migration creates child tables (e.g.,
Create.Table("FeatureFlagOverrides")) that reference FeatureFlagId (and
DepartmentId) but never adds foreign key constraints; update the Up() method to
add FK constraints after creating those tables (for example add
FK_FeatureFlagOverrides_FeatureFlags on FeatureFlagId ->
FeatureFlags(FeatureFlagId) and FK_FeatureFlagOverrides_Departments on
DepartmentId -> Departments(DepartmentId) for FeatureFlagOverrides and analogous
FKs for the other child tables that reference FeatureFlagId/DepartmentId), and
update Down() to drop those FK constraints before dropping the tables so
referential integrity is enforced and migrations rollback cleanly; ensure FK
names are unique and consistent with the pattern used in this project and
reference the Up(), Down(), Create.Table("FeatureFlagOverrides"), FeatureFlagId,
and DepartmentId symbols when making the changes.
- Around line 92-104: The migration creates the FeatureFlagUsages table but
lacks a uniqueness constraint for daily aggregates; update the migration so the
index "IX_FeatureFlagUsages_Flag_Date" includes the DepartmentId column and is
created as a unique index to enforce uniqueness on (FeatureFlagId, DepartmentId,
UsageDate); modify the Create.Index call that targets "FeatureFlagUsages"
(currently OnColumn("FeatureFlagId").OnColumn("UsageDate")) to also
OnColumn("DepartmentId") and mark the index as Unique().
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs`:
- Around line 41-52: The migration creates the FeatureFlagOverrides table but
does not enforce referential integrity: add a foreign key constraint in the Up()
method linking FeatureFlagOverrides.FeatureFlagId to the primary key of the
FeatureFlags table (e.g. FeatureFlags.FeatureFlagId) and name it clearly (e.g.
FK_FeatureFlagOverrides_FeatureFlags_FeatureFlagId); also add the corresponding
DropForeignKey call in Down() before dropping the table. Repeat the same pattern
for each other child table that references FeatureFlagId (ensure each table has
a FK to FeatureFlags and is removed in Down()) so orphaned FeatureFlagId values
cannot occur.
- Around line 92-104: The migration for FeatureFlagUsages in
M0071_AddingFeatureTogglesPg.cs is missing a unique constraint to prevent
duplicate daily aggregates; add a unique constraint on the combination of
FeatureFlagId, DepartmentId and UsageDate (columns "FeatureFlagId",
"DepartmentId", "UsageDate" on table "FeatureFlagUsages") by adding a
Create.UniqueConstraint(...) (e.g. "UX_FeatureFlagUsages_Flag_Department_Date")
that references those three columns after the Create.Table(...) block so
duplicates cannot be inserted.
---
Nitpick comments:
In `@Core/Resgrid.Config/FeatureFlagsConfig.cs`:
- Line 21: In FeatureFlagsConfig replace explicit constructor types in
field/property initializers with C# target-typed new() syntax: locate the object
initializers in the FeatureFlagsConfig class (the entries referenced around
lines 30, 33 and 39) and change patterns like "SomeType = new SomeType(...)" to
"SomeType = new(...)" (leave primitives like CacheDurationMinutes as-is); ensure
using directives and language version support (C# 9+) remain compatible.
In `@Core/Resgrid.Model/FeatureToggles/FeatureFlag.cs`:
- Around line 107-111: The IdValue setter currently casts unsafely and can throw
for null or wrong types; change it to perform a null-safe/pattern-match
assignment: use "if (value is int v) FeatureFlagId = v; else if (value == null)
FeatureFlagId = 0; else throw new ArgumentException(...)" (referencing the
IdValue property and FeatureFlagId field) so you defensively handle null, accept
ints, and provide a clear error for other types.
- Line 123: The IgnoredProperties getter on class FeatureFlag currently
allocates a new string[] on each access; change it to return a single
pre-allocated static readonly collection to avoid repeated allocations. Add a
private static readonly string[] (or IReadOnlyList<string>) field (e.g.,
s_ignoredProperties) initialized once with
"IdValue","IdType","TableName","IdName" and have the public IgnoredProperties
property return that field (preserve the IEnumerable<string> return type and
keep the values immutable).
In `@Core/Resgrid.Services/FeatureToggleService.cs`:
- Around line 205-223: PassesPlanGateAsync should compare the department's
PlanId to the feature flag's MinimumPlanType (i.e., treat MinimumPlanType as a
plan id) — ensure the method uses plan.PlanId in its comparison logic (and that
any other consumer like FeatureFlagAttributeTypes.PlanType also reads
plan?.PlanId) so the gate checks "plan.PlanId >= MinimumPlanType"; optionally
consider renaming MinimumPlanType/PlanType to indicate they're plan IDs or
introduce a mapping to an explicit tier enum to avoid relying on numeric
ordering.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0071_AddingFeatureToggles.cs`:
- Around line 41-58: Add single-column indexes for department-only queries by
creating non-unique indexes on the DepartmentId columns: add an index on
FeatureFlagOverrides.DepartmentId (e.g., name
UX_FeatureFlagOverrides_Department) in the same migration near the existing
UX_FeatureFlagOverrides_Flag_Department definition, and add a similar non-unique
index on FeatureFlagUsages.DepartmentId (e.g., UX_FeatureFlagUsages_Department)
near where FeatureFlagUsages is created; use
Create.Index(...).OnTable(...).OnColumn("DepartmentId").Ascending() to implement
both.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs`:
- Around line 41-58: The migration creates a unique composite index
UX_FeatureFlagOverrides_Flag_Department on FeatureFlagId+DepartmentId but lacks
a standalone index to support department-filtered queries; add non-unique
indexes on the DepartmentId column for FeatureFlagOverrides (and likewise add
DepartmentId indexes for FeatureFlags and FeatureFlagUsages where those tables
are defined) by creating Create.Index calls targeting "DepartmentId" on the
respective tables so department-scoped lookups use an index in addition to the
existing composite UX_FeatureFlagOverrides_Flag_Department.
In `@Web/Resgrid.Web.Services/Controllers/v4/FeatureTogglesController.cs`:
- Around line 70-79: The Get method currently returns a success response
unconditionally; update it to handle unknown feature keys by checking the
evaluation.Source from _featureToggleService.EvaluateAsync(key, DepartmentId)
(the returned FeatureFlagEvaluation), and if evaluation.Source ==
FeatureFlagEvaluationSource.NotFound then set result.Status via
ResponseHelper.PopulateV4ResponseNotFound(result) (instead of
ResponseHelper.Success); otherwise leave the existing behavior (map with
MapEvaluation, set PageSize, call PopulateV4ResponseData and return result).
🪄 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: ec9e1a94-1899-4f13-851e-19d9e97d4405
⛔ Files ignored due to path filters (1)
Web/Resgrid.Web/Areas/User/Apps/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.coderabbit.yamlCore/Resgrid.Config/FeatureFlagsConfig.csCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/FeatureToggles/FeatureFlag.csCore/Resgrid.Model/FeatureToggles/FeatureFlagAttributeTypes.csCore/Resgrid.Model/FeatureToggles/FeatureFlagEvaluation.csCore/Resgrid.Model/FeatureToggles/FeatureFlagEvaluationSource.csCore/Resgrid.Model/FeatureToggles/FeatureFlagOperatorTypes.csCore/Resgrid.Model/FeatureToggles/FeatureFlagOverride.csCore/Resgrid.Model/FeatureToggles/FeatureFlagPrerequisite.csCore/Resgrid.Model/FeatureToggles/FeatureFlagTargetingRule.csCore/Resgrid.Model/FeatureToggles/FeatureFlagUsage.csCore/Resgrid.Model/FeatureToggles/FeatureFlagValueTypes.csCore/Resgrid.Model/Repositories/IFeatureFlagOverrideRepository.csCore/Resgrid.Model/Repositories/IFeatureFlagPrerequisiteRepository.csCore/Resgrid.Model/Repositories/IFeatureFlagRepository.csCore/Resgrid.Model/Repositories/IFeatureFlagTargetingRuleRepository.csCore/Resgrid.Model/Repositories/IFeatureFlagUsageRepository.csCore/Resgrid.Model/Services/IFeatureToggleService.csCore/Resgrid.Services/FeatureToggleService.csCore/Resgrid.Services/ServicesModule.csProviders/Resgrid.Providers.Migrations/Migrations/M0071_AddingFeatureToggles.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.csRepositories/Resgrid.Repositories.DataRepository/FeatureFlagOverrideRepository.csRepositories/Resgrid.Repositories.DataRepository/FeatureFlagPrerequisiteRepository.csRepositories/Resgrid.Repositories.DataRepository/FeatureFlagRepository.csRepositories/Resgrid.Repositories.DataRepository/FeatureFlagTargetingRuleRepository.csRepositories/Resgrid.Repositories.DataRepository/FeatureFlagUsageRepository.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csWeb/Resgrid.Web.Services/Controllers/v4/FeatureTogglesController.csWeb/Resgrid.Web.Services/Models/v4/FeatureToggles/FeatureToggleInputs.csWeb/Resgrid.Web.Services/Models/v4/FeatureToggles/FeatureToggleResults.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWorkers/Resgrid.Workers.Framework/Logic/FeatureToggleUsageProcessor.cs
Comment on lines
+92
to
+104
| Create.Table("FeatureFlagUsages") | ||
| .WithColumn("FeatureFlagUsageId").AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId").AsInt32().NotNullable() | ||
| .WithColumn("DepartmentId").AsInt32().Nullable() | ||
| .WithColumn("UsageDate").AsDateTime2().NotNullable() | ||
| .WithColumn("EvaluationCount").AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("EnabledCount").AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("DisabledCount").AsInt64().NotNullable().WithDefaultValue(0); | ||
|
|
||
| Create.Index("IX_FeatureFlagUsages_Flag_Date") | ||
| .OnTable("FeatureFlagUsages") | ||
| .OnColumn("FeatureFlagId").Ascending() | ||
| .OnColumn("UsageDate").Ascending(); |
Contributor
There was a problem hiding this comment.
Add unique constraint to prevent duplicate usage records.
The FeatureFlagUsages table is described as "aggregated daily evaluation counts" but lacks a unique constraint on (FeatureFlagId, DepartmentId, UsageDate). This allows duplicate entries for the same flag/department/date combination, which would corrupt analytics data.
🛡️ Proposed fix to add unique constraint
Create.Index("IX_FeatureFlagUsages_Flag_Date")
.OnTable("FeatureFlagUsages")
.OnColumn("FeatureFlagId").Ascending()
- .OnColumn("UsageDate").Ascending();
+ .OnColumn("DepartmentId").Ascending()
+ .OnColumn("UsageDate").Ascending()
+ .WithOptions().Unique();
}Note: This changes the existing index to include DepartmentId and makes it unique, which serves both uniqueness enforcement and query optimization.
🤖 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
`@Providers/Resgrid.Providers.Migrations/Migrations/M0071_AddingFeatureToggles.cs`
around lines 92 - 104, The migration creates the FeatureFlagUsages table but
lacks a uniqueness constraint for daily aggregates; update the migration so the
index "IX_FeatureFlagUsages_Flag_Date" includes the DepartmentId column and is
created as a unique index to enforce uniqueness on (FeatureFlagId, DepartmentId,
UsageDate); modify the Create.Index call that targets "FeatureFlagUsages"
(currently OnColumn("FeatureFlagId").OnColumn("UsageDate")) to also
OnColumn("DepartmentId") and mark the index as Unique().
Comment on lines
+92
to
+104
| Create.Table("FeatureFlagUsages".ToLower()) | ||
| .WithColumn("FeatureFlagUsageId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("DepartmentId".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("UsageDate".ToLower()).AsDateTime2().NotNullable() | ||
| .WithColumn("EvaluationCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("EnabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("DisabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0); | ||
|
|
||
| Create.Index("IX_FeatureFlagUsages_Flag_Date".ToLower()) | ||
| .OnTable("FeatureFlagUsages".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending() | ||
| .OnColumn("UsageDate".ToLower()).Ascending(); |
Contributor
There was a problem hiding this comment.
Add unique constraint to prevent duplicate usage records.
Same issue as the SQL Server migration: FeatureFlagUsages lacks a unique constraint on (FeatureFlagId, DepartmentId, UsageDate), allowing duplicate daily aggregates.
🛡️ Proposed fix to add unique constraint
Create.Index("IX_FeatureFlagUsages_Flag_Date".ToLower())
.OnTable("FeatureFlagUsages".ToLower())
.OnColumn("FeatureFlagId".ToLower()).Ascending()
- .OnColumn("UsageDate".ToLower()).Ascending();
+ .OnColumn("DepartmentId".ToLower()).Ascending()
+ .OnColumn("UsageDate".ToLower()).Ascending()
+ .WithOptions().Unique();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Create.Table("FeatureFlagUsages".ToLower()) | |
| .WithColumn("FeatureFlagUsageId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | |
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | |
| .WithColumn("DepartmentId".ToLower()).AsInt32().Nullable() | |
| .WithColumn("UsageDate".ToLower()).AsDateTime2().NotNullable() | |
| .WithColumn("EvaluationCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | |
| .WithColumn("EnabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | |
| .WithColumn("DisabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0); | |
| Create.Index("IX_FeatureFlagUsages_Flag_Date".ToLower()) | |
| .OnTable("FeatureFlagUsages".ToLower()) | |
| .OnColumn("FeatureFlagId".ToLower()).Ascending() | |
| .OnColumn("UsageDate".ToLower()).Ascending(); | |
| Create.Table("FeatureFlagUsages".ToLower()) | |
| .WithColumn("FeatureFlagUsageId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | |
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | |
| .WithColumn("DepartmentId".ToLower()).AsInt32().Nullable() | |
| .WithColumn("UsageDate".ToLower()).AsDateTime2().NotNullable() | |
| .WithColumn("EvaluationCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | |
| .WithColumn("EnabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | |
| .WithColumn("DisabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0); | |
| Create.Index("IX_FeatureFlagUsages_Flag_Date".ToLower()) | |
| .OnTable("FeatureFlagUsages".ToLower()) | |
| .OnColumn("FeatureFlagId".ToLower()).Ascending() | |
| .OnColumn("DepartmentId".ToLower()).Ascending() | |
| .OnColumn("UsageDate".ToLower()).Ascending() | |
| .WithOptions().Unique(); |
🤖 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
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs`
around lines 92 - 104, The migration for FeatureFlagUsages in
M0071_AddingFeatureTogglesPg.cs is missing a unique constraint to prevent
duplicate daily aggregates; add a unique constraint on the combination of
FeatureFlagId, DepartmentId and UsageDate (columns "FeatureFlagId",
"DepartmentId", "UsageDate" on table "FeatureFlagUsages") by adding a
Create.UniqueConstraint(...) (e.g. "UX_FeatureFlagUsages_Flag_Department_Date")
that references those three columns after the Create.Table(...) block so
duplicates cannot be inserted.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost("SaveFlag")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.SystemAdmin)] | ||
| public async Task<ActionResult<FeatureFlagResult>> SaveFlag([FromBody] SaveFeatureFlagInput input) |
| [HttpPost("ArchiveFlag")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.SystemAdmin)] | ||
| public async Task<ActionResult<FeatureFlagResult>> ArchiveFlag(string key, bool archived = true) |
| [HttpPost("SetGlobalEnabled")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.SystemAdmin)] | ||
| public async Task<ActionResult<FeatureFlagResult>> SetGlobalEnabled([FromBody] SetGlobalEnabledInput input) |
| [HttpPost("SetRollout")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.SystemAdmin)] | ||
| public async Task<ActionResult<FeatureFlagResult>> SetRollout([FromBody] SetRolloutInput input) |
| [HttpPost("SaveTargetingRule")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.SystemAdmin)] | ||
| public async Task<ActionResult<FeatureFlagTargetingRulesResult>> SaveTargetingRule([FromBody] SaveTargetingRuleInput input) |
| [HttpPost("AddPrerequisite")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.SystemAdmin)] | ||
| public async Task<ActionResult<FeatureFlagPrerequisitesResult>> AddPrerequisite([FromBody] AddPrerequisiteInput input) |
| [HttpPost("SetOverride")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize] | ||
| public async Task<ActionResult<FeatureFlagOverrideResult>> SetOverride([FromBody] SetFeatureFlagOverrideInput input) |
| [Authorize] | ||
| public async Task<ActionResult<FeatureFlagOverrideResult>> SetOverride([FromBody] SetFeatureFlagOverrideInput input) | ||
| { | ||
| if (input == null || string.IsNullOrWhiteSpace(input.Key)) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Web/Resgrid.Web.Services/Controllers/TwilioController.cs (1)
58-90: ⚡ Quick winResolve
IFeatureToggleServiceper repo convention instead of extending constructor injection.This adds another constructor dependency to an already very large constructor. Please resolve
IFeatureToggleServicevia the repository’s constructor pattern instead of injecting it here.As per coding guidelines, "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection".🤖 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 `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 58 - 90, The TwilioController constructor is adding IFeatureToggleService to an already large constructor; change to resolve it inside the constructor via the repository pattern instead of constructor injection: remove IFeatureToggleService from the parameter list and stop assigning it from the constructor args, and instead obtain an instance by calling Bootstrapper.GetKernel().Resolve<IFeatureToggleService>() (assign to the existing _featureToggleService field) inside the TwilioController constructor; ensure you only modify the constructor signature and initialization, leaving other injected services (e.g., IDepartmentSettingsService, INumbersService, etc.) untouched.
🤖 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 `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Around line 481-509: The Stop branch uses profile.UserId without checking for
null; update the TextCommandTypes.Stop case in TwilioController (where profile
is obtained via _userProfileService.GetProfileByMobileNumberAsync and
DisableTextMessagesForUserAsync is called) to null-check profile first, and if
profile is null mark messageEvent.Processed appropriately and send a
deterministic SMS via response.Message (e.g. "Unable to locate your profile;
please log in to Resgrid to manage text settings" or similar) instead of calling
DisableTextMessagesForUserAsync, otherwise proceed to call
DisableTextMessagesForUserAsync(profile.UserId) and send the normal confirmation
message.
- Around line 141-183: After resolving departmentId (and userProfile) but before
the chatbotEnabled branch, ensure messageEvent.CustomerId is populated so
chatbot-routed events keep the resolved department context: add an assignment
such as if (departmentId.HasValue) messageEvent.CustomerId = departmentId.Value;
(or use departmentId.GetValueOrDefault() if appropriate) immediately after the
code that sets departmentId and userProfile and before calling
_featureToggleService.IsEnabledAsync/branching to ProcessTextCommandsAsync so
both Chatbot ingress handling and ProcessTextCommandsAsync see the same
CustomerId.
- Around line 441-474: Ensure the CallDetail branch validates the retrieved call
before using it: after calling _callsService.GetCallByIdAsync, check that the
call is not null and that its department id matches the current caller's
department (compare call.DepartmentId to department.DepartmentId or
department.Id), and if either check fails return a generic "Call not found" or
"Access denied" via response.Message and do not proceed to build callText; only
build and send call details when the call exists and belongs to the department
to avoid null dereference and cross-department data leakage.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Around line 58-90: The TwilioController constructor is adding
IFeatureToggleService to an already large constructor; change to resolve it
inside the constructor via the repository pattern instead of constructor
injection: remove IFeatureToggleService from the parameter list and stop
assigning it from the constructor args, and instead obtain an instance by
calling Bootstrapper.GetKernel().Resolve<IFeatureToggleService>() (assign to the
existing _featureToggleService field) inside the TwilioController constructor;
ensure you only modify the constructor signature and initialization, leaving
other injected services (e.g., IDepartmentSettingsService, INumbersService,
etc.) untouched.
🪄 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: 86f377a9-658e-42cf-afd9-babc3c8c6724
📒 Files selected for processing (4)
Core/Resgrid.Model/FeatureFlagKeys.csProviders/Resgrid.Providers.Migrations/Migrations/M0072_AddingChatbotTwilioFeatureFlag.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0072_AddingChatbotTwilioFeatureFlagPg.csWeb/Resgrid.Web.Services/Controllers/TwilioController.cs
✅ Files skipped from review due to trivial changes (1)
- Core/Resgrid.Model/FeatureFlagKeys.cs
Comment on lines
+141
to
+183
| // Resolve the department that owns the inbound number (falling back to the sender's | ||
| // linked profile) so the Chatbot Twilio integration flag can be evaluated per-department. | ||
| UserProfile userProfile = null; | ||
| var departmentId = await _departmentSettingsService.GetDepartmentIdByTextToCallNumberAsync(textMessage.To); | ||
| if (!departmentId.HasValue) | ||
| { | ||
| MessageId = request.MessageSid ?? Guid.NewGuid().ToString("N"), | ||
| From = request.From?.Replace("+", ""), | ||
| To = request.To?.Replace("+", ""), | ||
| Text = request.Body, | ||
| Platform = ChatbotPlatform.SmsTwilio, | ||
| Timestamp = DateTime.UtcNow | ||
| }; | ||
| userProfile = await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | ||
| if (userProfile != null) | ||
| { | ||
| var department = await _departmentsService.GetDepartmentByUserIdAsync(userProfile.UserId); | ||
| if (department != null) | ||
| departmentId = department.DepartmentId; | ||
| } | ||
| } | ||
|
|
||
| // Feature-flagged rollout: the chatbot ingress is the new path. When the flag is off | ||
| // (globally or for this department) fall back to the original text-command handling so | ||
| // existing behavior is preserved. | ||
| var chatbotEnabled = await _featureToggleService.IsEnabledAsync( | ||
| FeatureFlagKeys.ChatbotTwilioTextIntegration, departmentId ?? 0, false); | ||
|
|
||
| if (chatbotEnabled) | ||
| { | ||
| var chatbotMessage = new ChatbotMessage | ||
| { | ||
| MessageId = request.MessageSid ?? Guid.NewGuid().ToString("N"), | ||
| From = request.From?.Replace("+", ""), | ||
| To = request.To?.Replace("+", ""), | ||
| Text = request.Body, | ||
| Platform = ChatbotPlatform.SmsTwilio, | ||
| Timestamp = DateTime.UtcNow | ||
| }; | ||
|
|
||
| var chatbotResponse = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage); | ||
| var chatbotResponse = await _chatbotIngressService.ProcessMessageAsync(chatbotMessage); | ||
|
|
||
| if (chatbotResponse.Processed) | ||
| messageEvent.Processed = true; | ||
| if (chatbotResponse.Processed) | ||
| messageEvent.Processed = true; | ||
|
|
||
| response.Message(chatbotResponse.Text); | ||
| response.Message(chatbotResponse.Text); | ||
| } | ||
| else | ||
| { | ||
| await ProcessTextCommandsAsync(textMessage, messageEvent, response, departmentId, userProfile); |
Contributor
There was a problem hiding this comment.
Preserve CustomerId for chatbot-routed inbound events.
messageEvent.CustomerId is only assigned inside ProcessTextCommandsAsync, so flagged-on messages are saved without the resolved department context. Set it once after departmentId is known, before branching.
Suggested fix
if (!departmentId.HasValue)
{
userProfile = await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn);
if (userProfile != null)
{
var department = await _departmentsService.GetDepartmentByUserIdAsync(userProfile.UserId);
if (department != null)
departmentId = department.DepartmentId;
}
}
+
+ if (departmentId.HasValue)
+ messageEvent.CustomerId = departmentId.Value.ToString();
// Feature-flagged rollout: the chatbot ingress is the new path. When the flag is off🤖 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 `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 141 -
183, After resolving departmentId (and userProfile) but before the
chatbotEnabled branch, ensure messageEvent.CustomerId is populated so
chatbot-routed events keep the resolved department context: add an assignment
such as if (departmentId.HasValue) messageEvent.CustomerId = departmentId.Value;
(or use departmentId.GetValueOrDefault() if appropriate) immediately after the
code that sets departmentId and userProfile and before calling
_featureToggleService.IsEnabledAsync/branching to ProcessTextCommandsAsync so
both Chatbot ingress handling and ProcessTextCommandsAsync see the same
CustomerId.
Comment on lines
+441
to
+474
| case TextCommandTypes.CallDetail: | ||
| messageEvent.Processed = true; | ||
|
|
||
| var call = await _callsService.GetCallByIdAsync(int.Parse(payload.Data)); | ||
|
|
||
| var callText = new StringBuilder(); | ||
| callText.Append($"Call Information for {call.Name}" + Environment.NewLine); | ||
| callText.Append("---------------------" + Environment.NewLine); | ||
| callText.Append($"Id: {call.CallId}" + Environment.NewLine); | ||
| callText.Append($"Number: {call.Number}" + Environment.NewLine); | ||
| callText.Append($"Logged: {call.LoggedOn.TimeConverterToString(department)}" + Environment.NewLine); | ||
| callText.Append("-----Nature-----" + Environment.NewLine); | ||
| callText.Append(call.NatureOfCall + Environment.NewLine); | ||
| callText.Append("-----Address-----" + Environment.NewLine); | ||
|
|
||
| if (!String.IsNullOrWhiteSpace(call.Address)) | ||
| callText.Append(call.Address + Environment.NewLine); | ||
| else if (!string.IsNullOrEmpty(call.GeoLocationData) && call.GeoLocationData.Length > 1) | ||
| { | ||
| try | ||
| { | ||
| string[] points = call.GeoLocationData.Split(char.Parse(",")); | ||
|
|
||
| if (points != null && points.Length == 2) | ||
| { | ||
| callText.Append(_geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) + Environment.NewLine); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| } | ||
| } | ||
|
|
||
| response.Message(callText.ToString()); |
Contributor
There was a problem hiding this comment.
Validate call ownership before returning call details.
The CallDetail branch returns whatever GetCallByIdAsync(int.Parse(payload.Data)) finds, without checking that the call belongs to the sender’s department. That leaks cross-department incident data and also dereferences call without a null guard.
Suggested fix
case TextCommandTypes.CallDetail:
messageEvent.Processed = true;
var call = await _callsService.GetCallByIdAsync(int.Parse(payload.Data));
+ if (call == null || call.DepartmentId != department.DepartmentId)
+ {
+ response.Message("Unable to find that call.");
+ break;
+ }
var callText = new StringBuilder();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case TextCommandTypes.CallDetail: | |
| messageEvent.Processed = true; | |
| var call = await _callsService.GetCallByIdAsync(int.Parse(payload.Data)); | |
| var callText = new StringBuilder(); | |
| callText.Append($"Call Information for {call.Name}" + Environment.NewLine); | |
| callText.Append("---------------------" + Environment.NewLine); | |
| callText.Append($"Id: {call.CallId}" + Environment.NewLine); | |
| callText.Append($"Number: {call.Number}" + Environment.NewLine); | |
| callText.Append($"Logged: {call.LoggedOn.TimeConverterToString(department)}" + Environment.NewLine); | |
| callText.Append("-----Nature-----" + Environment.NewLine); | |
| callText.Append(call.NatureOfCall + Environment.NewLine); | |
| callText.Append("-----Address-----" + Environment.NewLine); | |
| if (!String.IsNullOrWhiteSpace(call.Address)) | |
| callText.Append(call.Address + Environment.NewLine); | |
| else if (!string.IsNullOrEmpty(call.GeoLocationData) && call.GeoLocationData.Length > 1) | |
| { | |
| try | |
| { | |
| string[] points = call.GeoLocationData.Split(char.Parse(",")); | |
| if (points != null && points.Length == 2) | |
| { | |
| callText.Append(_geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) + Environment.NewLine); | |
| } | |
| } | |
| catch | |
| { | |
| } | |
| } | |
| response.Message(callText.ToString()); | |
| case TextCommandTypes.CallDetail: | |
| messageEvent.Processed = true; | |
| var call = await _callsService.GetCallByIdAsync(int.Parse(payload.Data)); | |
| if (call == null || call.DepartmentId != department.DepartmentId) | |
| { | |
| response.Message("Unable to find that call."); | |
| break; | |
| } | |
| var callText = new StringBuilder(); | |
| callText.Append($"Call Information for {call.Name}" + Environment.NewLine); | |
| callText.Append("---------------------" + Environment.NewLine); | |
| callText.Append($"Id: {call.CallId}" + Environment.NewLine); | |
| callText.Append($"Number: {call.Number}" + Environment.NewLine); | |
| callText.Append($"Logged: {call.LoggedOn.TimeConverterToString(department)}" + Environment.NewLine); | |
| callText.Append("-----Nature-----" + Environment.NewLine); | |
| callText.Append(call.NatureOfCall + Environment.NewLine); | |
| callText.Append("-----Address-----" + Environment.NewLine); | |
| if (!String.IsNullOrWhiteSpace(call.Address)) | |
| callText.Append(call.Address + Environment.NewLine); | |
| else if (!string.IsNullOrEmpty(call.GeoLocationData) && call.GeoLocationData.Length > 1) | |
| { | |
| try | |
| { | |
| string[] points = call.GeoLocationData.Split(char.Parse(",")); | |
| if (points != null && points.Length == 2) | |
| { | |
| callText.Append(_geoLocationProvider.GetAproxAddressFromLatLong(double.Parse(points[0]), double.Parse(points[1])) + Environment.NewLine); | |
| } | |
| } | |
| catch | |
| { | |
| } | |
| } | |
| response.Message(callText.ToString()); |
🤖 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 `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 441 -
474, Ensure the CallDetail branch validates the retrieved call before using it:
after calling _callsService.GetCallByIdAsync, check that the call is not null
and that its department id matches the current caller's department (compare
call.DepartmentId to department.DepartmentId or department.Id), and if either
check fails return a generic "Call not found" or "Access denied" via
response.Message and do not proceed to build callText; only build and send call
details when the call exists and belongs to the department to avoid null
dereference and cross-department data leakage.
Comment on lines
+481
to
+509
| else if (textMessage.To == Config.NumberProviderConfig.TwilioResgridNumber) // Resgrid master text number | ||
| { | ||
| var profile = await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | ||
| var payload = _textCommandService.DetermineType(textMessage.Text); | ||
|
|
||
| switch (payload.Type) | ||
| { | ||
| case TextCommandTypes.None: | ||
| response.Message("Resgrid (https://resgrid.com) Automated Text System. Unknown command, text help for supported commands."); | ||
| break; | ||
| case TextCommandTypes.Help: | ||
| messageEvent.Processed = true; | ||
|
|
||
| var help = new StringBuilder(); | ||
| help.Append("Resgrid Text Commands" + Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("This is the Resgrid system for first responders (https://resgrid.com) automated text system. Your department isn't signed up for inbound text messages, but you can send the following commands." + | ||
| Environment.NewLine); | ||
| help.Append("---------------------" + Environment.NewLine); | ||
| help.Append("STOP: To turn off all text messages" + Environment.NewLine); | ||
| help.Append("HELP: This help text" + Environment.NewLine); | ||
|
|
||
| response.Message(help.ToString()); | ||
|
|
||
| break; | ||
| case TextCommandTypes.Stop: | ||
| messageEvent.Processed = true; | ||
| await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId); | ||
| response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile."); |
Contributor
There was a problem hiding this comment.
Null-check profile before processing STOP on the master number.
profile can be null here, but the Stop branch unconditionally uses profile.UserId. That turns an unrecognized sender into a generic error response instead of a deterministic SMS reply.
Suggested fix
case TextCommandTypes.Stop:
messageEvent.Processed = true;
- await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId);
- response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile.");
+ if (profile == null)
+ {
+ response.Message("We couldn't find a Resgrid user for this phone number.");
+ break;
+ }
+
+ await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId);
+ response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile.");
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| else if (textMessage.To == Config.NumberProviderConfig.TwilioResgridNumber) // Resgrid master text number | |
| { | |
| var profile = await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | |
| var payload = _textCommandService.DetermineType(textMessage.Text); | |
| switch (payload.Type) | |
| { | |
| case TextCommandTypes.None: | |
| response.Message("Resgrid (https://resgrid.com) Automated Text System. Unknown command, text help for supported commands."); | |
| break; | |
| case TextCommandTypes.Help: | |
| messageEvent.Processed = true; | |
| var help = new StringBuilder(); | |
| help.Append("Resgrid Text Commands" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("This is the Resgrid system for first responders (https://resgrid.com) automated text system. Your department isn't signed up for inbound text messages, but you can send the following commands." + | |
| Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("STOP: To turn off all text messages" + Environment.NewLine); | |
| help.Append("HELP: This help text" + Environment.NewLine); | |
| response.Message(help.ToString()); | |
| break; | |
| case TextCommandTypes.Stop: | |
| messageEvent.Processed = true; | |
| await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId); | |
| response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile."); | |
| else if (textMessage.To == Config.NumberProviderConfig.TwilioResgridNumber) // Resgrid master text number | |
| { | |
| var profile = await _userProfileService.GetProfileByMobileNumberAsync(textMessage.Msisdn); | |
| var payload = _textCommandService.DetermineType(textMessage.Text); | |
| switch (payload.Type) | |
| { | |
| case TextCommandTypes.None: | |
| response.Message("Resgrid (https://resgrid.com) Automated Text System. Unknown command, text help for supported commands."); | |
| break; | |
| case TextCommandTypes.Help: | |
| messageEvent.Processed = true; | |
| var help = new StringBuilder(); | |
| help.Append("Resgrid Text Commands" + Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("This is the Resgrid system for first responders (https://resgrid.com) automated text system. Your department isn't signed up for inbound text messages, but you can send the following commands." + | |
| Environment.NewLine); | |
| help.Append("---------------------" + Environment.NewLine); | |
| help.Append("STOP: To turn off all text messages" + Environment.NewLine); | |
| help.Append("HELP: This help text" + Environment.NewLine); | |
| response.Message(help.ToString()); | |
| break; | |
| case TextCommandTypes.Stop: | |
| messageEvent.Processed = true; | |
| if (profile == null) | |
| { | |
| response.Message("We couldn't find a Resgrid user for this phone number."); | |
| break; | |
| } | |
| await _userProfileService.DisableTextMessagesForUserAsync(profile.UserId); | |
| response.Message("Text messages are now turned off for this user, to enable again log in to Resgrid and update your profile."); | |
| break; |
🤖 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 `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 481 -
509, The Stop branch uses profile.UserId without checking for null; update the
TextCommandTypes.Stop case in TwilioController (where profile is obtained via
_userProfileService.GetProfileByMobileNumberAsync and
DisableTextMessagesForUserAsync is called) to null-check profile first, and if
profile is null mark messageEvent.Processed appropriately and send a
deterministic SMS via response.Message (e.g. "Unable to locate your profile;
please log in to Resgrid to manage text settings" or similar) instead of calling
DisableTextMessagesForUserAsync, otherwise proceed to call
DisableTextMessagesForUserAsync(profile.UserId) and send the normal confirmation
message.
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
2 participants
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
New Features
Documentation