CU-868f7n1u3 Adding in Countly for analytics. PW validation fix. #244
Conversation
WalkthroughAdds Countly telemetry configuration and web SDK initialization; extends analytics JS to forward events to Countly and Aptabase. Introduces password complexity validation utilities and attribute, applies to registration and user models. Tightens login/username length rules. Adjusts PushService registration guard to require DeviceId. Adds Dropzone and Countly assets via libman and bundles. Adds test examples. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser as Browser (View)
participant MVC as MVC Model Binder
participant Attr as PasswordComplexityAttribute
participant Helpers as StringHelpers
User->>Browser: Submit form with NewPassword/Password
Browser->>MVC: POST model data
MVC->>Attr: Validate(value, context)
Attr->>Helpers: VerifyPasswordComplexity(password, rules)
Helpers-->>Attr: PasswordComplexityResult(IsValid, Errors)
alt valid
Attr-->>MVC: ValidationResult.Success
MVC-->>Browser: Proceed (next action)
else invalid
Attr-->>MVC: ValidationResult(errors)
MVC-->>Browser: Return model errors
end
sequenceDiagram
autonumber
participant Page as _UserLayout.cshtml
participant CSDK as Countly SDK
participant App as resgrid.common.analytics
Note over Page: If CountlyUrl & CountlyWebKey set
Page->>CSDK: Load countly.min.js and init({app_key,url})
App->>CSDK: set_id(userId), user_details
App->>CSDK: track_sessions(), track_pageview()
App->>PostHog: capture(event, props)
App->>Aptabase: trackEvent(event, props)
App->>CSDK: add_event({key:event, segmentation:props})
sequenceDiagram
autonumber
actor Client
participant Service as PushService
Client->>Service: RegisterUnit(pushUri)
alt Missing DeviceId or UnitId or PushLocation
Service-->>Client: return false
else Valid input
Service->>Service: Continue registration flow
Service-->>Client: return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
Web/Resgrid.Web/libman.json (1)
289-291: Library additions LGTM; consider scoping files for Dropzone
- Countly entry is correctly pinned and file-scoped.
- Dropzone pulls the entire package; to reduce payload, consider adding a files array (e.g., dist/dropzone.min.js, dist/dropzone.css).
{ - "library": "@deltablot/[email protected]", - "destination": "wwwroot/lib/deltablot/dropzone/" + "library": "@deltablot/[email protected]", + "destination": "wwwroot/lib/deltablot/dropzone/", + "files": [ + "dist/dropzone.min.js", + "dist/dropzone.css" + ] }Also applies to: 293-299
Core/Resgrid.Services/PushService.cs (2)
38-39: Guarding on DeviceId is good; add trace for early returnsReturning false without logging makes prod issues hard to diagnose. Add a concise log for null/empty DeviceId/PushLocation.
- if (pushUri == null || string.IsNullOrWhiteSpace(pushUri.DeviceId) || string.IsNullOrWhiteSpace(pushUri.PushLocation)) - return false; + if (pushUri == null || string.IsNullOrWhiteSpace(pushUri.DeviceId) || string.IsNullOrWhiteSpace(pushUri.PushLocation)) + { + Framework.Logging.LogWarning("PushService.Register invalid args: " + + $"nullUri={pushUri == null}, deviceIdEmpty={string.IsNullOrWhiteSpace(pushUri?.DeviceId)}, pushLocationEmpty={string.IsNullOrWhiteSpace(pushUri?.PushLocation)}"); + return false; + }
64-65: Unit registration: same ask—log why the guard failsMirror logging here to aid ops visibility.
- if (pushUri == null || !pushUri.UnitId.HasValue || string.IsNullOrWhiteSpace(pushUri.DeviceId) || string.IsNullOrWhiteSpace(pushUri.PushLocation)) - return false; + if (pushUri == null || !pushUri.UnitId.HasValue || string.IsNullOrWhiteSpace(pushUri.DeviceId) || string.IsNullOrWhiteSpace(pushUri.PushLocation)) + { + Framework.Logging.LogWarning("PushService.RegisterUnit invalid args: " + + $"nullUri={pushUri == null}, unitIdHasValue={pushUri?.UnitId.HasValue}, deviceIdEmpty={string.IsNullOrWhiteSpace(pushUri?.DeviceId)}, pushLocationEmpty={string.IsNullOrWhiteSpace(pushUri?.PushLocation)}"); + return false; + }Core/Resgrid.Config/TelemetryConfig.cs (1)
19-21: Countly keys added; consider first-class exporter supportIf you later want to toggle providers uniformly, extend TelemetryExporters with Countly and wire GetAnalyticsKey or a GetEnabledExporters helper. Not blocking.
public enum TelemetryExporters { None = 0, PostHog = 1, - Aptabase = 2 + Aptabase = 2, + Countly = 3 }Web/Resgrid.Web/Areas/User/Views/Shared/_UserLayout.cshtml (1)
62-73: Initialize Countly safely and avoid render-blocking
- Add
deferto the script tag to avoid blocking head parsing.- Ensure
Countly.qexists before analytics code uses it.- <script type='text/javascript' src="~/lib/countly-sdk-web/lib/countly.min.js"></script> + <script defer src="~/lib/countly-sdk-web/lib/countly.min.js"></script> <script type='text/javascript'> if (Countly && window["Countly"]) { + Countly.q = Countly.q || []; Countly.init({ app_key: "@Resgrid.Config.TelemetryConfig.CountlyWebKey", url: "@Resgrid.Config.TelemetryConfig.CountlyUrl" }); } </script>Web/Resgrid.Web/wwwroot/js/app/common/analytics/resgrid.common.analytics.js (3)
16-28: Countly: prefer direct API when available; keep q-push as fallbackUse direct methods when the SDK is loaded, fall back to
q.pushif not. Also explicitly setcount: 1in events.- if (typeof Countly !== "undefined") { - Countly.q.push(['set_id', userId]); - Countly.q.push(['user_details', { - "name": name, - "email": email, - "organization": departmentName, - "custom": { - "createdOn": new Date(createdOn * 1000), - "departmentId": departmentId - }]); - Countly.q.push(['track_sessions']); - Countly.q.push(['track_pageview']); - } + if (typeof Countly !== "undefined") { + var C = Countly; + if (C.user_details) { + if (C.set_id) C.set_id(userId); + C.user_details({ + name: name, + email: email, + organization: departmentName, + custom: { + createdOn: new Date(createdOn * 1000), + departmentId: departmentId + } + }); + if (C.track_sessions) C.track_sessions(); + if (C.track_pageview) C.track_pageview(); + } else if (C.q && C.q.push) { + C.q.push(['set_id', userId]); + C.q.push(['user_details', { + name: name, + email: email, + organization: departmentName, + custom: { + createdOn: new Date(createdOn * 1000), + departmentId: departmentId + } + }]); + C.q.push(['track_sessions']); + C.q.push(['track_pageview']); + } + }
33-36: Minor: terminate statementAdd a semicolon to keep linters quiet.
- posthog.capture(event) + posthog.capture(event);
38-46: Forwarding to Countly: add explicit count and direct-call fallbackBe explicit with
count: 1and preferadd_eventdirectly when available.- if (typeof Countly !== 'undefined') { - Countly.q.push(['add_event', { - "key": event - }]); - } + if (typeof Countly !== 'undefined') { + var C = Countly; + if (C.add_event) { + C.add_event({ key: event, count: 1 }); + } else if (C.q && C.q.push) { + C.q.push(['add_event', { key: event, count: 1 }]); + } + }Core/Resgrid.Framework/StringHelpers.cs (1)
257-329: Password complexity helper looks solid; consider localizationError strings are hard-coded; consider pulling from Resgrid.Localization to localize validation messages and centralize wording.
Tests/Resgrid.Tests/Framework/PasswordComplexityExamples.cs (2)
20-28: Fix incorrect “valid” example against default rules (min length = 8).“Abc123” is 6 chars, so it fails the default 8-char rule. Update the sample to avoid misleading output/comments.
- "Abc123", // Valid according to default requirements + "Abcd1234", // Valid according to default requirements
45-55: Consider converting console examples into executable unit tests.Turn these into xUnit/NUnit/MSTest assertions so regressions are caught automatically and CI-visible.
I can draft a minimal test class mirroring these cases if you want.
Web/Resgrid.Web/Models/AccountViewModels/RegisterViewModel.cs (1)
40-46: Clarify length validation message to cover both min and max.Current StringLength message mentions only the minimum; it will also surface on >100 chars. Make the message accurate.
- [StringLength(100, ErrorMessage = "The password must be at least 8 characters long", MinimumLength = 8)] + [StringLength(100, MinimumLength = 8, ErrorMessage = "The password must be between 8 and 100 characters long.")]Web/Resgrid.Web/Areas/User/Models/AddPersonModel.cs (1)
46-52: Align length validation message with both bounds.Same issue as RegisterViewModel: message says “at least 8,” but will trigger for >100 too.
- [StringLength(100, ErrorMessage = "The password must be at least 8 characters long", MinimumLength = 8)] + [StringLength(100, MinimumLength = 8, ErrorMessage = "The password must be between 8 and 100 characters long.")]Core/Resgrid.Framework/PasswordComplexityAttribute.cs (2)
24-40: Remove duplicate override; keep a single source of truth.Overriding both IsValid methods duplicates logic and risks divergence. Rely on the ValidationResult override only.
- public override bool IsValid(object value) - { - if (value is not string password) - { - return false; - } - - var result = StringHelpers.VerifyPasswordComplexity( - password, - MinLength, - RequireUppercase, - RequireLowercase, - RequireDigit, - RequireSpecialChar); - - return result.IsValid; - } + // Remove boolean override to avoid logic drift; ValidationResult override below is authoritative.
42-64: Avoid duplicate “required” errors by skipping validation on null/empty.When combined with [Required], the current logic adds an extra “empty password” message from VerifyPasswordComplexity. Early-out keeps messages clean.
- protected override ValidationResult IsValid(object value, ValidationContext validationContext) + protected override ValidationResult IsValid(object value, ValidationContext validationContext) { - if (value is not string password) - { - return new ValidationResult("Invalid password format"); - } + // Let [Required] handle null/empty scenarios to avoid duplicate messages. + if (value is null) return ValidationResult.Success; + if (value is not string password) return ValidationResult.Success; + if (string.IsNullOrWhiteSpace(password)) return ValidationResult.Success; var result = StringHelpers.VerifyPasswordComplexity( password, MinLength, RequireUppercase, RequireLowercase, RequireDigit, RequireSpecialChar); if (result.IsValid) { return ValidationResult.Success; } var errorMessage = string.Join(", ", result.Errors); return new ValidationResult(errorMessage, new[] { validationContext.MemberName }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Core/Resgrid.Config/TelemetryConfig.cs(1 hunks)Core/Resgrid.Framework/PasswordComplexityAttribute.cs(1 hunks)Core/Resgrid.Framework/StringHelpers.cs(2 hunks)Core/Resgrid.Services/PushService.cs(2 hunks)Tests/Resgrid.Tests/Framework/PasswordComplexityExamples.cs(1 hunks)Web/Resgrid.Web/Areas/User/Models/AddPersonModel.cs(2 hunks)Web/Resgrid.Web/Areas/User/Views/Shared/_UserLayout.cshtml(1 hunks)Web/Resgrid.Web/Models/AccountViewModels/LoginViewModel.cs(1 hunks)Web/Resgrid.Web/Models/AccountViewModels/RegisterViewModel.cs(2 hunks)Web/Resgrid.Web/Startup.cs(1 hunks)Web/Resgrid.Web/libman.json(1 hunks)Web/Resgrid.Web/wwwroot/js/app/common/analytics/resgrid.common.analytics.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Prefer functional patterns and immutable data where appropriate in C#
Use modern C# features appropriately
Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations
Separate state from behavior (e.g., use records for state and place operations in separate static classes)
Prefer pure methods over methods with side effects
Use extension methods appropriately for domain-specific operations
Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions
Minimize constructor injection; keep the number of injected dependencies small
Prefer composition with interfaces to extend behavior
Files:
Core/Resgrid.Config/TelemetryConfig.csWeb/Resgrid.Web/Models/AccountViewModels/RegisterViewModel.csCore/Resgrid.Framework/StringHelpers.csTests/Resgrid.Tests/Framework/PasswordComplexityExamples.csCore/Resgrid.Services/PushService.csWeb/Resgrid.Web/Startup.csCore/Resgrid.Framework/PasswordComplexityAttribute.csWeb/Resgrid.Web/Models/AccountViewModels/LoginViewModel.csWeb/Resgrid.Web/Areas/User/Models/AddPersonModel.cs
🧬 Code graph analysis (3)
Core/Resgrid.Framework/StringHelpers.cs (1)
Core/Resgrid.Framework/PasswordComplexityAttribute.cs (1)
IsValid(24-40)
Tests/Resgrid.Tests/Framework/PasswordComplexityExamples.cs (2)
Core/Resgrid.Framework/StringHelpers.cs (1)
StringHelpers(22-331)Core/Resgrid.Framework/PasswordComplexityAttribute.cs (1)
IsValid(24-40)
Core/Resgrid.Framework/PasswordComplexityAttribute.cs (1)
Core/Resgrid.Framework/StringHelpers.cs (1)
StringHelpers(22-331)
🪛 Biome (2.1.2)
Web/Resgrid.Web/wwwroot/js/app/common/analytics/resgrid.common.analytics.js
[error] 25-25: expected , but instead found ]
Remove ]
(parse)
[error] 25-25: expected , but instead found ;
Remove ;
(parse)
[error] 26-26: expected , but instead found (
Remove (
(parse)
[error] 26-26: expected , but instead found ;
Remove ;
(parse)
[error] 27-27: expected , but instead found (
Remove (
(parse)
[error] 27-27: expected , but instead found ;
Remove ;
(parse)
[error] 38-38: expected , but instead found if
Remove if
(parse)
[error] 38-38: Expected a parameter but instead found 'typeof'.
Expected a parameter here.
(parse)
[error] 38-38: expected , but instead found Aptabase
Remove Aptabase
(parse)
[error] 38-38: expected , but instead found !==
Remove !==
(parse)
[error] 42-42: expected , but instead found if
Remove if
(parse)
[error] 42-42: Expected a parameter but instead found 'typeof'.
Expected a parameter here.
(parse)
[error] 42-42: expected , but instead found Countly
Remove Countly
(parse)
[error] 42-42: expected , but instead found !==
Remove !==
(parse)
[error] 38-40: This method is later overwritten by an object member with the same name.
Overwritten with this method.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this method.
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
Web/Resgrid.Web/Startup.cs (4)
353-355: Formatting-only changes in public bundles look goodNo functional changes; safe to merge.
357-359: Formatting-only changes in Angular bundle look goodNo functional changes; safe to merge.
361-370: Add Dropzone CSS is correct; order is appropriateThe Dropzone CSS path matches libman destination and is placed before site style.css, allowing site overrides. LGTM.
372-382: Add Dropzone JS is correct; bundle order is fineThe JS path matches libman destination. Placing Dropzone before site.min.js is sensible.
| [StringLength(250, ErrorMessage = "The username must be at least 2 characters long and contain only alphanumeric characters.", MinimumLength = 2)] | ||
| public string Username { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Username error message contradicts actual allowed characters
Identity allows spaces and -._@ (see Startup configuration). Either adjust the message or add a regex to enforce the identity-allowed set. Suggested: simplify the message.
- [StringLength(250, ErrorMessage = "The username must be at least 2 characters long and contain only alphanumeric characters.", MinimumLength = 2)]
+ [StringLength(250, ErrorMessage = "The username must be at least 2 characters long.", MinimumLength = 2)]📝 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.
| [StringLength(250, ErrorMessage = "The username must be at least 2 characters long and contain only alphanumeric characters.", MinimumLength = 2)] | |
| public string Username { get; set; } | |
| [StringLength(250, ErrorMessage = "The username must be at least 2 characters long.", MinimumLength = 2)] | |
| public string Username { get; set; } |
🤖 Prompt for AI Agents
In Web/Resgrid.Web/Models/AccountViewModels/LoginViewModel.cs around lines 8-9,
the StringLength error message incorrectly states allowed characters
(alphanumeric only) while Identity permits spaces and -._@; either simplify the
message to only describe length (e.g., "The username must be between 2 and 250
characters.") or add a RegularExpression attribute that matches the exact
allowed character set used in Startup (include spaces and - . _ @ plus
alphanumerics) so the validation message and enforcement align.
| [StringLength(100, ErrorMessage = "The password must be at least 8 characters long, include a number (digit) and an uppercase letter", MinimumLength = 4)] | ||
| [DataType(DataType.Password)] | ||
| public string Password { get; set; } |
There was a problem hiding this comment.
Login password length constraint may block valid users
Enforcing min length here can prevent sign-in for legacy accounts whose passwords predate current policy. Recommend removing StringLength (validation occurs on change/reset), or align MinimumLength with current policy (8) if you’re certain no existing accounts fall below.
- [StringLength(100, ErrorMessage = "The password must be at least 8 characters long, include a number (digit) and an uppercase letter", MinimumLength = 4)]
- [DataType(DataType.Password)]
+ [DataType(DataType.Password)]📝 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.
| [StringLength(100, ErrorMessage = "The password must be at least 8 characters long, include a number (digit) and an uppercase letter", MinimumLength = 4)] | |
| [DataType(DataType.Password)] | |
| public string Password { get; set; } | |
| [DataType(DataType.Password)] | |
| public string Password { get; set; } |
🤖 Prompt for AI Agents
In Web/Resgrid.Web/Models/AccountViewModels/LoginViewModel.cs around lines
12-14, the StringLength attribute on Password enforces a minimum length (4) at
sign-in which can block legacy accounts; remove the StringLength attribute
entirely so login does not validate password length (perform length validation
only on password change/reset), or if you are sure no existing accounts use
shorter passwords, update MinimumLength to 8 to match current policy.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests