Read member adoption details for an organization - #8315
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dirt/prototype/adoption-report/device-index #8315 +/- ##
===============================================================================
+ Coverage 69.56% 69.64% +0.07%
===============================================================================
Files 2471 2476 +5
Lines 105935 106241 +306
Branches 9601 9636 +35
===============================================================================
+ Hits 73695 73991 +296
- Misses 29777 29783 +6
- Partials 2463 2467 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0442193 to
3a26e37
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Re-reviewed after the three commits that moved the shared item count out of SQL and into Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| Guid organizationId) | ||
| { | ||
| await using var scope = ServiceScopeFactory.CreateAsyncScope(); | ||
| var dbContext = GetDatabaseContext(scope); |
There was a problem hiding this comment.
❓ QUESTION: The Dapper path sets commandTimeout: 120, but this EF path keeps the provider default (30s).
Details
MemberAdoptionReportRepository (Dapper) passes commandTimeout: 120 — signalling that the report is expected to exceed the 30s default on a large organization. The EF translation runs the same two aggregations with no timeout override, and I couldn't find a global CommandTimeout / SetCommandTimeout anywhere in src/Infrastructure.EntityFramework or src/SharedWeb, so MySQL/PostgreSQL/SQLite fall back to the 30s provider default.
If the intent is parity, dbContext.Database.SetCommandTimeout(120) before the two queries would match. If self-hosted EF deployments are assumed small enough that 30s is fine, no change needed — just wanted to confirm the asymmetry is deliberate.
| WITH [OrganizationCollection] AS ( | ||
| SELECT | ||
| COL.[Id] | ||
| FROM | ||
| [dbo].[Collection] COL | ||
| WHERE | ||
| COL.[OrganizationId] = @OrganizationId | ||
| ), |
There was a problem hiding this comment.
❓ QUESTION: SharedItemCount includes each member's own default (My Items) collection — should Collection.Type = 1 be excluded?
Details
[OrganizationCollection] takes every collection in the org, including Type = 1 (CollectionType.DefaultUserCollection). Collection_CreateDefaultCollections links that collection to its owner through CollectionUser, and the ciphers inside it are org-owned (Cipher.OrganizationId = @OrganizationId, UserId IS NULL), so they satisfy [OrganizationCollectionCipher] and land in the member's SharedItemCount.
For an org with UseMyItems enabled, the effect on a member who has been migrated is that their own items count as shared items while VaultItemCount (which requires Cipher.OrganizationId IS NULL) drops to 0 — so the two columns swap meaning rather than describing personal vs shared usage. The codebase already has a precedent for drawing this line: CipherOrganizationDetails_ReadByOrganizationIdExcludingDefaultCollections filters [CollectionType] <> 1, and CollectionCipher_UpdateCollectionsAdmin uses C.[Type] <> 1.
If the report is meant to measure genuinely shared items, adding AND COL.[Type] <> 1 here (and the matching collection.Type != CollectionType.DefaultUserCollection in the EF directCollectionAccess/groupCollectionAccess queries) would keep the two counts distinct. If counting them is deliberate for adoption purposes, no change needed — the test collections are all built as CollectionType.SharedCollection, so the current behaviour isn't exercised either way.
f4db932 to
b529b9a
Compare
| @@ -0,0 +1,264 @@ | |||
| using System.Runtime.InteropServices; | |||
There was a problem hiding this comment.
.editorconfig requires for *.cs, which fails the Lint job.
Details and fix
.editorconfig sets charset = utf-8-bom for [*.{cs,csx,vb,vbx}], and dotnet format enforces it. Three of the new .cs files in this PR are plain ASCII with no BOM, while every other new file in the same diff (MemberAdoptionReportDetail.cs, both repositories, the test files) has one:
src/Core/Dirt/Models/Data/MemberAdoptionReportAccessGraph.cssrc/Core/Dirt/Reports/ReportFeatures/SharedItemCountCalculator.cstest/Core.Test/Dirt/ReportFeatures/SharedItemCountCalculatorTests.cs
Lint (dotnet format --verify-no-changes) is currently failing on this PR and passing on the base PR, and build-artifacts plus build-mssqlmigratorutility both declare needs: lint, so the Docker image and migrator builds are skipped.
Running dotnet format over the three projects will rewrite them with the BOM. I have not seen the job log, so I can only say these three definitely violate the configured charset — there may be another fixer firing as well, which the same command would also settle.
Reference: .editorconfig:17-19, .github/workflows/build.yml:36-37
Adds MemberAdoptionReport_ReadByOrganizationId and its Entity Framework translation, returning one row per confirmed member with the last activity date, whether a browser extension is in use, the personal vault item count, the number of shared items the member can reach and whether a sponsorship has been redeemed. The report is unpaged over an entire organization, so its runtime grows with member and cipher counts and the implicit 30 second command timeouts are not a deliberate bound. Both backends read on 120 seconds instead, matching the other whole-organization Dapper reads, and the Dapper read goes to the read replica. The repository contract hands back an IReadOnlyList rather than an IEnumerable, so a caller can walk the details more than once without the read being repeated; both implementations already materialize before they return.
Adds the access-graph edge types and SharedItemCountCalculator, which unions the collections a member reaches directly or through a group and counts the distinct organization-owned ciphers behind them. Nothing calls it yet. It exists so that the Dapper and Entity Framework repositories can share one implementation of the per-member set union, rather than each expressing that union relationally, which at organization scale neither backend completes inside its command timeout.
The detail procedure no longer computes SharedItemCount. A new procedure returns the member-to-collection and collection-to-cipher edges in one QueryMultiple round trip, and the Dapper repository folds the counts in through SharedItemCountCalculator so both backends can share one algorithm. Removing the serial per-member set union from the procedure is what lets the report finish; the aggregation now happens over the edges in memory. What is left of the detail read is also cut to one pass per member. It hit Device twice per member and drove VaultItemCount through a per-member OUTER APPLY that the optimizer served from an eager index spool, seeked once per member. The two Device applies fold into a single aggregate that computes the last activity date and the browser-extension flag from one seek, and personal vault items are pre-aggregated set-wise, scoped to the organization's confirmed members so the count never widens to every account's personal vault. Against the 17,001-member org that pass drops the detail read from 319,331 logical reads to 80,623 and warm CPU from roughly 400ms to 180ms. The sponsorship apply is unchanged: the table is empty in the dev database, so no improvement could be measured. Output is byte-identical, verified with EXCEPT in both directions including row position, plus a scratch case covering members with no devices, only an extension device, only a non-extension device, a null UserId, and a redeemed sponsorship.
Reads the access graph as flat projections and defers counting to SharedItemCountCalculator, so the provider translations no longer materialise the member by cipher cross product. Covers the three reads the report is built from with SQLite integration tests against a real, throwaway database.
b529b9a to
dfe1bf2
Compare
1 participant
🎟️ Tracking
PM-35924
📔 Objective
Adds the data access for the member adoption report: one row per confirmed organization member with login recency, browser extension use, vault item count, reachable shared item count, and sponsorship redemption.
FromSqlRaw/EXECpassthrough, which is what makes the sibling Risk Insights and Member Access reports MSSQL only.SharedItemCountcounts organization ciphers reachable directly viaCollectionUserand through a group viaGroupUser/CollectionGroup. Both de-duplication steps are load bearing: the union collapses a collection reachable by both paths, and the separate distinct collapses a cipher sitting in more than one reachable collection. Dropping either inflates the count.OrganizationUser.Status = 2is counted. Invited and Accepted members are deliberately excluded.MemberAdoptionReportRepositorySqliteTestsruns against a real in-memory SQLite context with no configuration, so it executes on every PR. The sibling[CiSkippedTheory]harness never runs in CI.Middle of a 3 PR stack, on
device-index.endpointsits on this.📸 Screenshots
No user-visible change.