Skip to content

Add the member adoption report endpoint - #8316

Draft
maxkpower wants to merge 2 commits into
dirt/prototype/adoption-report/data-accessfrom
dirt/prototype/adoption-report/endpoint
Draft

Add the member adoption report endpoint#8316
maxkpower wants to merge 2 commits into
dirt/prototype/adoption-report/data-accessfrom
dirt/prototype/adoption-report/endpoint

Conversation

@maxkpower

Copy link
Copy Markdown

🎟️ Tracking

PM-35924

📔 Objective

Adds GET /reports/member-adoption/{organizationId}, returning per-member adoption detail plus organization level counts.

  • Authorized with [Authorize<AccessReportsRequirement>] rather than the imperative ICurrentContext.AccessReports check the sibling actions use. Same principals (Owner, Admin, Custom with accessReports, provider users), but it returns 403 where the neighbours return 404.
  • organizationId is bound [FromRoute]. Without it, MVC's Form-then-Route value provider order lets a form body shadow the route value, so the action could act on a different organization than the one authorized.
  • Gated on UseRiskInsights, matching the other org scoped actions on this controller.
  • HasRecentLogin uses an injected TimeProvider and a 30 day window, bounded at both ends so a client with a skewed clock cannot sit permanently active.
  • Counts are derived from one materialized list, so Total, Active and Inactive cannot disagree with the member list.

Top of a 3 PR stack, on data-access.

📸 Screenshots

No user-visible change.

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.99%. Comparing base (b529b9a) to head (7d9623b).

❗ There is a different number of reports uploaded between BASE (b529b9a) and HEAD (7d9623b). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (b529b9a) HEAD (7d9623b)
2 1
Additional details and impacted files
@@                              Coverage Diff                               @@
##           dirt/prototype/adoption-report/data-access    #8316      +/-   ##
==============================================================================
- Coverage                                       69.64%   63.99%   -5.65%     
==============================================================================
  Files                                            2476     2480       +4     
  Lines                                          106241   106350     +109     
  Branches                                         9636     9640       +4     
==============================================================================
- Hits                                            73991    68061    -5930     
- Misses                                          29783    35944    +6161     
+ Partials                                         2467     2345     -122     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maxkpower
maxkpower force-pushed the dirt/prototype/adoption-report/endpoint branch 2 times, most recently from eb9eba9 to 75087c9 Compare September 4, 2026 00:11
@maxkpower
maxkpower force-pushed the dirt/prototype/adoption-report/endpoint branch from 14e0df2 to b67ae5e Compare September 4, 2026 00:27
@maxkpower maxkpower added the ai-review Request a Claude code review label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new GET /reports/member-adoption/{organizationId} endpoint, the MemberAdoptionReportResult/response models, the MemberAdoptionReportQuery aggregation, the DI registration, the feature flag constant, and both test suites. Authorization was traced end to end: AccessReportsRequirement derives from BasePermissionRequirement (Owner, Admin, Custom with accessReports, confirmed provider users), OrganizationRequirementHandler resolves the org from route data via HttpContextExtensions.GetOrganizationId(), which accepts organizationId — the same route value the action binds with [FromRoute], so the authorized org and the queried org cannot diverge; the action-level attribute composes with the controller's [Authorize("Application")], matching the GroupsController precedent. The commit-2 switch to a deferred Members projection is safe: IMemberAdoptionReportRepository returns IReadOnlyList<MemberAdoptionReportDetail> and both implementations materialize (AsList() / ToListAsync()) before their connection or IServiceScope is disposed, so the post-return enumeration during serialization touches no live DB resource and cannot re-query; TimeProvider resolves via SharedWeb's TryAddSingleton(TimeProvider.System), UseFeatureFlagChecks() sits between UseAuthorization() and UseEndpoints() so the SDK RequireFeature attribute actually gates the route, and the log line emits only the organization ID and aggregate counts — no member PII.

One note outside the review: CI reports the Lint job (dotnet format --verify-no-changes) failing on the head commit. The job log was not retrievable from this review environment, so the cause is unconfirmed; Run tests and Analyze (csharp) were still pending.

Code Review Details

No findings met the bar for posting.

Items considered and deliberately not raised:

  • HasRecentLogin is evaluated twice per member (once in the counting loop, once in the deferred projection) but from the same captured now/activityCutoff via a shared helper, so the counts and the rows cannot drift.
  • Rejecting future-dated LastActivityDate as inactive is a deliberate bound against client clock skew and is covered by a test.
  • The unpaginated member list and the returned member Name/Email match the established shape of the neighbouring member-access endpoint on the same controller.
  • Returning 403 rather than the siblings' 404 is called out in the PR description; a non-member receives 403 whether or not the organization exists, so it is not an enumeration oracle.
  • AddScoped rather than TryAddScoped deviates from ADR-0026, but every sibling registration in AddReportingServices uses AddScoped.
  • The fully-qualified Bitwarden.Server.Sdk.Features.RequireFeature is correct — the unqualified RequireFeature reachable through using Bit.Core.Utilities is [Obsolete] with diagnostic BWA0002, and leaving the neighbouring passkey-directory action on it keeps this diff scoped.

@maxkpower
maxkpower force-pushed the dirt/prototype/adoption-report/endpoint branch from b67ae5e to 6f572cc Compare September 4, 2026 00:42
@maxkpower
maxkpower force-pushed the dirt/prototype/adoption-report/endpoint branch from 6f572cc to 7d9623b Compare September 4, 2026 23:49
Exposes GET /reports/member-adoption/{organizationId}, behind the
MemberAdoptionReport feature flag and the same plan and reports authorization
the other organization reports use.

MemberAdoptionReportQuery turns the repository details into the organization
totals and the member rows. The response model projects the members straight
to its own model as it writes rather than buffering them into a list.

The SDK RequireFeature attribute is applied by its fully qualified name so the
controller keeps importing Bit.Core.Utilities; resolving it unqualified through
the SDK namespace would also rebind the pre-existing passkey-directory
endpoint and change its response when its flag is off.
All four counts now come from a single foreach over the repository's
details, so the separate Count passes for active members and redeemed
sponsorships are gone, and Members is a deferred projection rather than
a second list. The response model already projects lazily as it writes,
so no member list is retained for the life of the request.

The activity cutoff and now are still captured once from TimeProvider,
and the recency test is shared by the counting loop and the projection
so the two cannot drift. The repository contract returns an
IReadOnlyList, materialized with its connection or scope already
disposed, which is what makes the deferred projection safe to enumerate
after the query returns.

At 17,001 members this holds about 4.5 MB less live heap per request
(a 0.13 MB details copy plus roughly 4.4 MB of member objects). That is
a small share of the measured per-request growth; the bulk is the detail
objects themselves, the access graph rows the repositories read, and
serialization buffers.
@maxkpower
maxkpower force-pushed the dirt/prototype/adoption-report/endpoint branch from 7d9623b to 2bfdf43 Compare September 8, 2026 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant