Add template properties for Datadog #6528
Conversation
brant-livefront
changed the title
|
Claude finished @brant-livefront's task —— View job Code Review Complete
SummaryThis PR adds new template properties ( Status of Previous Findings:
Findings❌ Finding 1: Missing integration tests for new repository method Per CLAUDE.md: "ALWAYS add unit tests (with mocking) for any new feature development" The new
Required test coverageAdd integration tests covering:
Test files:
The migration script (util/Migrator/DbScripts/2025-11-05_00_OrganizationUserUserDetails_ReadByOrganizationIdUserId.sql:17) has a GO statement on line 17, but this file does not end with a trailing newline. SQL Server migration scripts should end with a newline after the final GO statement to prevent potential concatenation issues with subsequent scripts. 💭 Finding 3: User role information exposure to third-party integrations The addition of While this appears intentional for enriching integration data, has this been reviewed against:
If approved, consider documenting this in integration configuration UI so administrators understand what data is being shared. Good Practices Observed
|
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6528 +/- ##
==========================================
- Coverage 56.83% 56.82% -0.01%
==========================================
Files 1909 1909
Lines 84810 84859 +49
Branches 7633 7639 +6
==========================================
+ Hits 48199 48223 +24
- Misses 34785 34810 +25
Partials 1826 1826 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
I assume my review is not required because this is merging into a feature branch (so codeowner requirements don't apply) and Architecture Team are handling this work. Please re-request my review if this is not the case.
…ly the Type is referenced
| SET NOCOUNT ON | ||
|
|
||
| SELECT | ||
| TOP 1 * |
There was a problem hiding this comment.
❓ Curious the purpose of the TOP 1 here? With no ORDER BY clause, this query is non-deterministic and can return inconsistent results any time it's run.
There was a problem hiding this comment.
I think I was being a bit belt + suspenders. There's really no way this query should return more than one. I can remove it so it doesn't cause confusion.
There was a problem hiding this comment.
@brant-livefront FWIW, I checked the database and there is one (and only one) instance with multiple records with the same UserId and OrgId. The data in both rows are identical (except for the Id), so that leads me to believe that the duplicate row was entered inadvertently. If you leave the TOP 1 in there, it should be fine assuming you don't care about the Id for the OrganizationUser table, since in this case they will be different. Every other User/Org combination has only one row (currently). I was initially concerned about performance but compared execution plans and that doesn't appear to be an issue so it's fine if you want to leave it in. Just be aware that it is currently possible that more than one row can be returned for a given User/Org, so you'd either need the TOP 1 or filter it out in code.
There was a problem hiding this comment.
@mkincaid-bw Thanks for checking on this! I think the best course is to put the TOP 1 back in - we don't care about order (or which id that one row returns) because we're only looking up things like Name, Email and Type here.
Our code currently would fail if it returned more than 1, so given that there's a possible edge case, the TOP 1 is probably a good defense against that rather than code around a case that really shouldn't be there. I'll add it back in.
There was a problem hiding this comment.
Mark and team are discussing the strange backstory with this table, so this may need to stay open for a bit as they research why there's not a unique index.
There was a problem hiding this comment.
I discussed this with Matt in Slack and I moved back (in latest commit) to dropping the TOP 1 and assuming uniqueness. We won't merge until that's a safe assumption.
There was a problem hiding this comment.
There was only one instance of duplicate data from 2017. Robert did some analysis on code changes and found that this occurred before some additional checks were put in place to handle the unique check.
I just cleaned up the duplicate data so there are currently no dups. After discussion, we are opting to NOT add a unique index at this time, primarily because we can't account for possible duplicates in self-hosted clients. We feel this is pretty safe since there was only one instance of duplicate data in 8 years, before additional checks in code were added.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs
Outdated
Show resolved
Hide resolved
src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs
Show resolved
Hide resolved
| public async Task<OrganizationUserUserDetails?> GetDetailsByOrganizationIdUserIdAsync(Guid organizationId, Guid userId) | ||
| { | ||
| using (var connection = new SqlConnection(ConnectionString)) | ||
| { | ||
| var result = await connection.QuerySingleOrDefaultAsync<OrganizationUserUserDetails>( | ||
| "[dbo].[OrganizationUserUserDetails_ReadByOrganizationIdUserId]", | ||
| new | ||
| { | ||
| OrganizationId = organizationId, | ||
| UserId = userId | ||
| }, | ||
| commandType: CommandType.StoredProcedure); | ||
|
|
||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ Per CLAUDE.md guidelines: "ALWAYS add unit tests (with mocking) for any new feature development." This new repository method lacks unit test coverage. Consider adding tests for:
- Successful retrieval of user details
- Null return when user not found
- Proper parameter handling
Note: The EF implementation of this same method also needs tests.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
.../Migrator/DbScripts/2025-11-05_00_OrganizationUserUserDetails_ReadByOrganizationIdUserId.sql
Outdated
Show resolved
Hide resolved
brant-livefront
dismissed
withinfocus’s stale review
The base branch was changed.
src/Sql/dbo/Stored Procedures/OrganizationUserUserDetails_ReadByOrganizationIdUserId.sql
Outdated
Show resolved
Hide resolved
| /// <remarks> | ||
| /// Similar to GetByOrganizationAsync, but returns the user details. | ||
| /// </remarks> | ||
| Task<OrganizationUserUserDetails?> GetDetailsByOrganizationIdUserIdAsync(Guid organizationId, Guid userId); |
There was a problem hiding this comment.
❌ Finding 2: New repository method lacks unit test coverage
Per CLAUDE.md: "ALWAYS add unit tests (with mocking) for any new feature development"
The new GetDetailsByOrganizationIdUserIdAsync method has no unit tests in either the Dapper or EntityFramework implementations. This violates the project's testing requirements and leaves 0% coverage on this new code path.
Required test coverage
Add tests to verify:
- Successful retrieval when user exists in organization
- Null return when user not found
- Null return when organization not found
- Proper parameter handling (organizationId, userId)
- Behavior with edge cases (null parameters, invalid GUIDs)
Tests should be added to:
test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs(for Dapper)test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs(for EF)
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Conditionally. Wait for DBOps.
| SET NOCOUNT ON | ||
|
|
||
| SELECT | ||
| TOP 1 * |
There was a problem hiding this comment.
Mark and team are discussing the strange backstory with this table, so this may need to stay open for a bit as they research why there's not a unique index.
src/Sql/dbo/Stored Procedures/OrganizationUserUserDetails_ReadByOrganizationIdUserId.sql
Outdated
Show resolved
Hide resolved
| return rowCount > 0; | ||
| } | ||
|
|
||
| public async Task<OrganizationUserUserDetails?> GetDetailsByOrganizationIdUserIdAsync(Guid organizationId, Guid userId) |
There was a problem hiding this comment.
❌ Finding 2: Missing integration tests for new repository method
Per CLAUDE.md: "ALWAYS add unit tests (with mocking) for any new feature development"
The new GetDetailsByOrganizationIdUserIdAsync method lacks integration test coverage in both Dapper and EntityFramework implementations. Codecov reports 0% coverage on this new code (13 lines missing in Dapper, 8 in EF).
Required test coverage
Add integration tests to verify:
- Successful retrieval when user exists in organization
- Null return when user not found
- Null return when organization not found
- Proper parameter handling (organizationId, userId)
- Behavior with invalid GUIDs
Tests should be added to:
test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs(for Dapper)test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs(for EF)
| public OrganizationUserUserDetails? User { get; set; } | ||
| public string? UserName => User?.Name; | ||
| public string? UserEmail => User?.Email; | ||
| public OrganizationUserType? UserType => User?.Type; |
There was a problem hiding this comment.
💭 Finding 3: New UserType and ActingUserType fields expose organizational role information
The addition of UserType and ActingUserType fields will expose OrganizationUserType enum values (Owner, Admin, User, etc.) to external integrations like Datadog.
Security consideration
While this appears intentional for enriching integration data, verify that exposing user role information to third-party integrations aligns with:
- Zero-knowledge security principles (CLAUDE.md)
- Data minimization requirements (GDPR/CCPA compliance)
- Customer expectations for what data is shared with integrations
If approved, consider documenting this in the integration configuration UI so administrators understand what organizational data is being shared.
| public async Task<OrganizationUserUserDetails?> GetDetailsByOrganizationIdUserIdAsync(Guid organizationId, Guid userId) | ||
| { | ||
| using (var connection = new SqlConnection(ConnectionString)) | ||
| { | ||
| var result = await connection.QuerySingleOrDefaultAsync<OrganizationUserUserDetails>( | ||
| "[dbo].[OrganizationUserUserDetails_ReadByOrganizationIdUserId]", | ||
| new | ||
| { | ||
| OrganizationId = organizationId, | ||
| UserId = userId | ||
| }, | ||
| commandType: CommandType.StoredProcedure); | ||
|
|
||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
❌ Finding 1: This new repository method lacks integration test coverage.
Per CLAUDE.md: "ALWAYS add unit tests (with mocking) for any new feature development"
Codecov shows 13 missing lines here (0% coverage). Add integration tests to test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs covering:
- Successful retrieval when user exists in organization
- Null return when user/organization not found
- Edge cases with invalid GUIDs
| AND | ||
| [UserId] = @UserId | ||
| END | ||
| GO |
There was a problem hiding this comment.
| GO | |
| GO | |
| public OrganizationUserType? UserType => User?.Type; | ||
|
|
||
| public User? ActingUser { get; set; } | ||
| public OrganizationUserUserDetails? ActingUser { get; set; } | ||
| public string? ActingUserName => ActingUser?.Name; | ||
| public string? ActingUserEmail => ActingUser?.Email; | ||
| public OrganizationUserType? ActingUserType => ActingUser?.Type; |
There was a problem hiding this comment.
💭 Finding 3: These new fields expose user role information (OrganizationUserType enum values like Owner, Admin, User) to external integrations like Datadog.
While this appears intentional for enriching integration data, verify this aligns with:
- Zero-knowledge security principles (per CLAUDE.md)
- Data minimization requirements (GDPR/CCPA)
- Customer expectations for data sharing with integrations
Consider documenting this in integration configuration UI so administrators understand what organizational data is being shared.
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
This is good to go. We know AC team is working on data discrepancies but the SQL change here should return a single result.
📔 Objective
We recently got a few more specifics from the Datadog team on the existing pull-based integration. In order to support identical data being sent as part of our new push-based integration, we need a few more dynamic fields to enrich the data.
This PR adds these new dynamic fields.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes