Skip to content

fix(jans-fido2): use the fixed adoption rate in the executive summary - #14861

Merged
yurem merged 1 commit into
mainfrom
jans-fido2-executive-summary-adoption-rate
Aug 27, 2026
Merged

fix(jans-fido2): use the fixed adoption rate in the executive summary#14861
yurem merged 1 commit into
mainfrom
jans-fido2-executive-summary-adoption-rate

Conversation

@imran-ishaq

@imran-ishaq imran-ishaq commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

PR body

Prepare


Description

Target issue

closes #14859

Implementation Details

Fido2AnalyticsService.generateExecutiveSummary did not use Fido2MetricsService. getUserAdoptionMetrics's adoptionRate — it pulled totalUniqueUsers and newUsers back out of
that map and recomputed its own rate. Two defects from that:

  1. Same population bug as fix(jans-fido2): user-adoption metrics measure the wrong population #14830, independently. totalUsers is totalUniqueUsers — everyone
    active in the query window, not a user population — so the recompute fell into the same
    "adoptionRate falls as adoption succeeds" trap that fix(jans-fido2): user-adoption metrics measure the wrong population #14830 fixes in getUserAdoptionMetrics
    itself. Fixing the source method did not fix this call site, because this call site threw the
    fixed value away.
  2. (Long) cast threw on every non-empty window. getUserAdoptionMetrics stores
    totalUniqueUsers/newUsers as Set.size() — an int, autoboxed to Integer — not a Long.
    Casting an Integer to Long throws ClassCastException at runtime. generateExecutiveSummary
    is only ever called from generateComprehensiveReport, which wraps it in a try/catch, so the
    entire comprehensive report silently failed and returned only an error key — discarding
    performance, device, error and trends data along with it — whenever the window had at least one
    active user.

The fix. Read adoptionRate straight from userAdoption.get(Fido2MetricsConstants.ADOPTION_RATE)
as a Double and use it directly for the insight thresholds, instead of recomputing from
totalUniqueUsers/newUsers. Fixes both defects at once: no more cast mismatch, and the insight now
agrees with the corrected, population-scoped rate.

Currently unreachable from the REST API. Fido2AnalyticsService has no caller in
Fido2MetricsController or anywhere else in jans-fido2generateComprehensiveReport and
generateExecutiveSummary are dead code today. Fixing it now so it's correct before the service is
ever wired up, rather than rediscovered as a production incident afterward.

Relationship to #14830. This PR does not depend on #14830 landing first — adoptionRate already
exists as a key on getUserAdoptionMetrics's response regardless of which PR merges first, this PR
just stops bypassing it. Once #14830 also lands, the value this reads becomes the corrected one.


Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Tests — new Fido2AnalyticsServiceTest (3 tests, all green):

  • a window with active users no longer throws ClassCastException
  • the "strong adoption" insight reflects adoptionRate from the metrics service, not a window-activity
    ratio recomputed from totalUniqueUsers/newUsers
  • an unknown (null) adoption rate adds no adoption insight and does not throw

Docs — no docs page describes Fido2AnalyticsService's executive summary (it isn't wired to a
controller yet), so none needed updating.

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

Summary by CodeRabbit

  • Bug Fixes

    • Executive adoption summaries now use the configured adoption rate, improving the accuracy of “strong” and “low” adoption insights.
    • Summaries remain stable when adoption data is unavailable or active-user windows contain incomplete data.
  • Tests

    • Added coverage for configured adoption rates, missing adoption data, and active-user reporting scenarios.

Signed-off-by: imran <imranishaq7071@gmail.com>
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a28fa8cf-fa03-4bbd-8ec2-9736deceff45

📥 Commits

Reviewing files that changed from the base of the PR and between 7ee988a and a155f20.

📒 Files selected for processing (2)
  • jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.java
  • jans-fido2/server/src/test/java/io/jans/fido2/service/metric/Fido2AnalyticsServiceTest.java

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

generateExecutiveSummary now uses ADOPTION_RATE from getUserAdoptionMetrics. Tests cover configured rates, active-user windows, and null-rate handling.

Changes

Executive summary adoption insights

Layer / File(s) Summary
Adoption rate flow and regression coverage
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.java, jans-fido2/server/src/test/java/io/jans/fido2/service/metric/Fido2AnalyticsServiceTest.java
The service uses ADOPTION_RATE instead of recomputing a rate from user counts. Mockito and JUnit tests verify threshold insights, active-user windows, and null-rate behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to a155f

This PR corrects the executive summary to use the established adoption-rate metric and prevents the prior runtime failure, with no actionable merge-blocking risk remaining after normal checks and review.

Suggested reviewers: yurem

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: using the fixed adoption rate in the FIDO2 executive summary.
Description check ✅ Passed The description follows the repository template. It includes preparation checks, target issue #14859, implementation details, test coverage, and the documentation-impact confirmation.
Linked Issues check ✅ Passed The changes satisfy issue #14859. The implementation reads ADOPTION_RATE directly, removes the Integer-to-Long cast failure, preserves null handling, and adds relevant tests.
Out of Scope Changes check ✅ Passed The changes are limited to the requested analytics fix and its unit tests. No unrelated code or documentation changes are present.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jans-fido2-executive-summary-adoption-rate

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mo-auto

mo-auto commented Aug 25, 2026

Copy link
Copy Markdown
Member

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@mo-auto mo-auto added comp-jans-fido2 Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality labels Aug 25, 2026
@sonarqubecloud

Copy link
Copy Markdown

@imran-ishaq
imran-ishaq marked this pull request as ready for review August 27, 2026 08:42
@imran-ishaq
imran-ishaq requested a review from yurem as a code owner August 27, 2026 08:42
@yurem
yurem merged commit 8162b59 into main Aug 27, 2026
20 of 22 checks passed
@yurem
yurem deleted the jans-fido2-executive-summary-adoption-rate branch August 27, 2026 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-jans-fido2 Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(jans-fido2): executive summary recomputes adoption rate against window activity, and crashes when it runs

3 participants