Skip to content

feat(adoption-insights): add time-saved backend enrichment and notifications - #3946

Open
rajin-kichannagari wants to merge 5 commits into
redhat-developer:mainfrom
rajin-kichannagari:feat/time-saved-backend
Open

feat(adoption-insights): add time-saved backend enrichment and notifications#3946
rajin-kichannagari wants to merge 5 commits into
redhat-developer:mainfrom
rajin-kichannagari:feat/time-saved-backend

Conversation

@rajin-kichannagari

Copy link
Copy Markdown
Contributor

Summary

Backend support for the Estimated Time Saved feature in Adoption Insights.

  • Enrich scaffolder create events with rhdh.redhat.com/time-saved annotation value from catalog entity at write time (RHIDP-15190)
  • Add time_saved_totals query aggregating by template with SUM/AVG/COUNT
  • Add periodic notification summary via NotificationService (AC3)
    • Scheduler runs daily at 9am, lookback per frequency tier
    • shouldSendForFrequency checks day-of-week/month
  • Add per-user notification frequency settings (AC4)
    • notification_preferences DB table with atomic upsert
    • GET/PUT /notification-preferences REST endpoints
  • Add admin config flag: adoptionInsights.notifications.enabled (defaults to true, set false to disable notifications entirely)

Hey, I just made a Pull Request!

Backend enrichment and notification scheduler for the Est. Time Saved feature. Reads the rhdh.redhat.com/time-saved annotation from template entities at event write time and stores the value. Adds a daily notification scheduler that summarizes time saved per user, respecting their frequency preference (daily/weekly/monthly/none). Admins can disable notifications entirely via config.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes) — N/A, backend only

Rajin Kichannagari and others added 2 commits July 23, 2026 18:18
…cations

- Enrich scaffolder create events with rhdh.redhat.com/time-saved
  annotation value from catalog entity at write time (RHIDP-15190)
- Add time_saved_totals query aggregating by template with SUM/AVG/COUNT
- Add periodic notification summary via NotificationService (AC3)
  - Scheduler runs daily at 9am, lookback per frequency tier
  - shouldSendForFrequency checks day-of-week/month
- Add per-user notification frequency settings (AC4)
  - notification_preferences DB table with atomic upsert
  - GET/PUT /notification-preferences REST endpoints
- Add admin config flag: adoptionInsights.notifications.enabled
  (defaults to true, set false to disable notifications entirely)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rhdh-gh-app

rhdh-gh-app Bot commented Jul 23, 2026

Copy link
Copy Markdown

Important

This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior.

Changed Packages

Package Name Package Path Changeset Bump Current Version
backend workspaces/adoption-insights/packages/backend none v0.0.0
@red-hat-developer-hub/backstage-plugin-adoption-insights-backend workspaces/adoption-insights/plugins/adoption-insights-backend minor v0.9.0

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Adoption Insights: enrich time-saved events and add notification preferences + scheduler

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Enrich scaffolder create events with template time-saved annotation at write time.
• Add aggregated time_saved_totals insights query (SUM/AVG/COUNT by template).
• Introduce time-saved notification summaries with per-user frequency preferences and admin toggle.
Diagram

graph TD
A["Event ingest API"] --> B["EventApiController"] --> D[("Adoption Insights DB")]
B --> C["CatalogService"]
E["Insights API (time_saved_totals)"] --> D
H["Preferences API"] --> D
F["Daily notification job"] --> D --> G["NotificationService"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Compute time-saved at query time (no event enrichment)
  • ➕ Avoids persisting derived data on events
  • ➕ Automatically reflects catalog annotation changes retroactively
  • ➖ Adds runtime dependency on Catalog for every report/notification run
  • ➖ Harder to make historical reporting deterministic if templates change
2. Batch preferences lookup to avoid per-user reads
  • ➕ Removes N+1 DB queries in notification job
  • ➕ Simpler to reason about scheduler performance at scale
  • ➖ Requires either a join/CTE or an API in EventDatabase to fetch all prefs
  • ➖ Slightly more complex query/adapter logic
3. Event-driven notifications (emit on scaffolder create, aggregate async)
  • ➕ Near-real-time feedback and fewer scheduled scans
  • ➕ Can amortize aggregation work via streaming/queue
  • ➖ More infrastructure and operational complexity
  • ➖ Harder to align with weekly/monthly summary semantics without extra state

Recommendation: The chosen approach (enrich at write time + daily scheduler) is a good fit for deterministic reporting and simple operations. Consider a follow-up optimization to batch-fetch notification preferences (or join in the aggregation query) to avoid per-user preference lookups as usage grows.

Files changed (19) +737 / -8

Enhancement (8) +353 / -7
EventApiController.tsEnrich scaffolder create events and add time_saved_totals query type +53/-7

Enrich scaffolder create events and add time_saved_totals query type

• Introduces Catalog/Auth dependencies and enriches scaffolder create events by reading 'rhdh.redhat.com/time-saved' from the template entity and storing it as 'event.value' when valid. Makes event ingestion async and adds 'time_saved_totals' to the insights query dispatch map.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts

BaseAdapter.tsImplement time-saved aggregations and notification preference CRUD +85/-0

Implement time-saved aggregations and notification preference CRUD

• Adds 'getTimeSavedTotals' grouped by template entityRef with COUNT/AVG/SUM over stored event values and a computed grand total. Implements 'getTimeSavedPerUser' for notification summaries and adds atomic upsert-based 'get/setNotificationPreference' backed by 'notification_preferences'.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/database/adapters/BaseAdapter.ts

event-database.tsExtend EventDatabase interface for time-saved and preferences +10/-0

Extend EventDatabase interface for time-saved and preferences

• Adds interface methods for 'getTimeSavedTotals', per-user time-saved aggregation, and get/set notification frequency preferences.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/database/event-database.ts

scheduleTimeSavedNotifications.tsAdd daily time-saved notification scheduler +129/-0

Add daily time-saved notification scheduler

• Introduces a scheduled task (cron 9am daily) that aggregates time saved per user over daily/weekly/monthly lookbacks and sends summaries via NotificationService. Respects per-user frequency and supports a global config kill-switch 'adoptionInsights.notifications.enabled' (default true).

workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts

plugin.tsWire catalog/auth/notification services and schedule notifications +21/-0

Wire catalog/auth/notification services and schedule notifications

• Adds dependencies on Catalog, Auth, and NotificationService to the plugin init and passes Catalog/Auth into EventApiController for enrichment. Schedules the time-saved notification job and passes the EventDatabase into the router for preference endpoints.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/plugin.ts

router.tsAdd notification preference REST endpoints +34/-0

Add notification preference REST endpoints

• Adds GET/PUT '/notification-preferences' routes that read/write a per-user frequency setting, with validation against the allowed enum values. Uses the current user entity ref as the preference key.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/router.ts

event-request.tsAdd time_saved_totals to allowed insights query types +1/-0

Add time_saved_totals to allowed insights query types

• Extends 'QUERY_TYPES' to include 'time_saved_totals', enabling controller routing and request validation for the new aggregation endpoint.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/types/event-request.ts

event.tsAdd types for time-saved aggregates and notification frequency +20/-0

Add types for time-saved aggregates and notification frequency

• Introduces 'TimeSavedTotals', 'TimeSavedTemplate', 'UserTimeSaved', and 'NotificationFrequency' types to formalize the new API responses and DB operations.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/types/event.ts

Tests (5) +289 / -1
EventApiController.test.tsExpand controller tests for time-saved query and enrichment +171/-0

Expand controller tests for time-saved query and enrichment

• Adds mocks for CatalogService/AuthService and verifies 'time_saved_totals' routes to the new database query. Introduces a focused test suite for scaffolder event enrichment, including stringified context, missing/invalid annotation, and error handling.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.test.ts

BaseAdapter.test.tsAdd adapter tests for time-saved totals aggregation +105/-0

Add adapter tests for time-saved totals aggregation

• Extends the mock knex surface with 'whereNotNull' and adds tests covering query predicates and grand-total summation for the 'getTimeSavedTotals' aggregation.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/database/adapters/BaseAdapter.test.ts

EventBatchProcessor.test.tsUpdate batch processor test DB mock for new interface +4/-0

Update batch processor test DB mock for new interface

• Extends the mocked EventDatabase with the newly added time-saved and notification preference methods to keep processor tests compiling and representative.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/domain/EventBatchProcessor.test.ts

plugin.test.tsUpdate plugin validation error message for new query type +1/-1

Update plugin validation error message for new query type

• Extends the allowed insights query type list in test expectations to include 'time_saved_totals'.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/plugin.test.ts

router.test.tsUpdate router test wiring for db + controller dependencies +8/-0

Update router test wiring for db + controller dependencies

• Updates the EventApiController construction to include Catalog/Auth mocks and passes the DB instance into 'createRouter' to support the new preferences endpoints.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/router.test.ts

Other (6) +95 / -0
time-saved-backend.mdAdd changeset for time-saved backend feature +5/-0

Add changeset for time-saved backend feature

• Introduces a changeset bumping the adoption-insights-backend package as a minor release. Documents the new enrichment, scheduler, and preferences capabilities.

workspaces/adoption-insights/.changeset/time-saved-backend.md

package.jsonAdd notifications backend dependency to app backend +1/-0

Add notifications backend dependency to app backend

• Adds '@backstage/plugin-notifications-backend' to the backend app dependencies so the notifications system can be registered and used by the Adoption Insights backend plugin.

workspaces/adoption-insights/packages/backend/package.json

index.tsRegister notifications backend plugin in backend app +2/-0

Register notifications backend plugin in backend app

• Wires '@backstage/plugin-notifications-backend' into the backend initialization sequence. Enables NotificationService availability for downstream plugins.

workspaces/adoption-insights/packages/backend/src/index.ts

20260716000000_notification_preferences.jsAdd notification_preferences table migration +31/-0

Add notification_preferences table migration

• Creates 'notification_preferences' keyed by 'user_ref' with a 'frequency' field defaulting to 'weekly' and an 'updated_at' timestamp. Provides down migration to drop the table.

workspaces/adoption-insights/plugins/adoption-insights-backend/migrations/20260716000000_notification_preferences.js

package.jsonAdd notifications node dependency to plugin +1/-0

Add notifications node dependency to plugin

• Adds '@backstage/plugin-notifications-node' so the backend plugin can send notifications via the NotificationService interface.

workspaces/adoption-insights/plugins/adoption-insights-backend/package.json

yarn.lockLockfile updates for notifications plugins +55/-0

Lockfile updates for notifications plugins

• Adds resolved entries for Backstage notifications backend/node/common packages (and transitive signals dependency) to support the new notification functionality.

workspaces/adoption-insights/yarn.lock

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.56693% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.35%. Comparing base (766e4aa) to head (dcc0999).
⚠️ Report is 87 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #3946    +/-   ##
========================================
  Coverage   57.35%   57.35%            
========================================
  Files        2384     2385     +1     
  Lines       95616    95732   +116     
  Branches    26734    26770    +36     
========================================
+ Hits        54838    54909    +71     
- Misses      40544    40590    +46     
+ Partials      234      233     -1     
Flag Coverage Δ *Carryforward flag
adoption-insights 83.20% <64.56%> (-1.34%) ⬇️
ai-integrations 69.26% <ø> (ø) Carriedforward from d254b13
app-defaults 69.79% <ø> (ø) Carriedforward from d254b13
augment 46.67% <ø> (ø) Carriedforward from d254b13
boost 75.89% <ø> (ø) Carriedforward from d254b13
bulk-import 72.63% <ø> (ø) Carriedforward from d254b13
cost-management 13.55% <ø> (ø) Carriedforward from d254b13
dcm 60.72% <ø> (ø) Carriedforward from d254b13
extensions 56.28% <ø> (ø) Carriedforward from d254b13
global-floating-action-button 71.18% <ø> (ø) Carriedforward from d254b13
global-header 62.19% <ø> (ø) Carriedforward from d254b13
homepage 47.58% <ø> (ø) Carriedforward from d254b13
install-dynamic-plugins 56.77% <ø> (ø) Carriedforward from d254b13
intelligent-assistant 74.05% <ø> (ø) Carriedforward from d254b13
konflux 91.98% <ø> (ø) Carriedforward from d254b13
lightspeed 69.02% <ø> (ø) Carriedforward from d254b13
mcp-integrations 83.40% <ø> (ø) Carriedforward from d254b13
orchestrator 62.65% <ø> (ø) Carriedforward from d254b13
quickstart 65.18% <ø> (ø) Carriedforward from d254b13
sandbox 79.56% <ø> (ø) Carriedforward from d254b13
scorecard 82.66% <ø> (ø) Carriedforward from d254b13
theme 83.85% <ø> (ø) Carriedforward from d254b13
translations 5.12% <ø> (ø) Carriedforward from d254b13
x2a 79.31% <ø> (ø) Carriedforward from d254b13

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 766e4aa...dcc0999. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Jul 23, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 11 rules
✅ Cross-repo context
  Explored: repo: redhat-developer/rhdh (sha: d090bd1a)
  Explored: repo: redhat-developer/rhdh-operator (sha: cde4968e)
  Explored: repo: redhat-developer/rhdh-local (sha: 2ae9e8c8)
  Not relevant to this PR: redhat-developer/rhdh-chart

Grey Divider


Action required

1. Notifications backend not provided ✓ Resolved 🔗 Cross-repo conflict ≡ Correctness
Description
This PR makes adoption-insights-backend require Backstage notificationService during plugin init,
meaning hosts like redhat-developer/rhdh must also install/provide the notifications backend
plugin. RHDH’s backend entrypoint does not add @backstage/plugin-notifications-backend by default
and the adoption-insights backend dynamic wrapper doesn’t embed it, so enabling the upgraded plugin
alone can fail to start.
Code

workspaces/adoption-insights/plugins/adoption-insights-backend/src/plugin.ts[R20-32]

+import { catalogServiceRef } from '@backstage/plugin-catalog-node';
+import { notificationService } from '@backstage/plugin-notifications-node';
import { adoptionInsightsEventsReadPermission } from '@red-hat-developer-hub/backstage-plugin-adoption-insights-common';
import { createRouter } from './router';
import { migrate } from './database/migration';
import { DatabaseFactory } from './database/DatabaseFactory';
import { EventBatchProcessor } from './domain/EventBatchProcessor';
import EventApiController from './controllers/EventApiController';
import { schedulePartition } from './database/partition';
+import { scheduleTimeSavedNotifications } from './notifications/scheduleTimeSavedNotifications';
import { getConfigurationOptions } from './utils/config';

/**
Relevance

●● Moderate

Mixed precedent: they sometimes reject making init deps optional; unclear if they’ll relax
notifications dependency.

PR-#3347

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR makes notifications a required init-time dependency for adoption-insights-backend. In the
pinned RHDH snapshot, the backend entrypoint does not add the notifications backend plugin, and the
adoption-insights backend dynamic wrapper only embeds adoption-insights packages (not
notifications), while notifications backend is packaged as a separate dynamic wrapper—indicating
hosts must explicitly enable/install it for the new plugin to work.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/plugin.ts[16-104]
External repo: redhat-developer/rhdh, packages/backend/src/index.ts [121-177]
External repo: redhat-developer/rhdh, dynamic-plugins/wrappers/red-hat-developer-hub-backstage-plugin-adoption-insights-backend-dynamic/package.json [1-44]
External repo: redhat-developer/rhdh, dynamic-plugins/wrappers/backstage-plugin-notifications-backend-dynamic/package.json [1-45]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`@red-hat-developer-hub/backstage-plugin-adoption-insights-backend` now declares a **hard dependency** on Backstage Notifications (`notificationService`) at init-time. In RHDH, the backend does not register `@backstage/plugin-notifications-backend` by default, and the adoption-insights backend dynamic wrapper does not embed/declare the notifications backend plugin, so enabling the updated adoption-insights backend dynamic plugin alone can cause runtime init failure due to missing service binding.

### Issue Context
- The PR adds `notification: notificationService` as a required `deps` entry in the adoption-insights backend plugin.
- RHDH’s backend composition (pinned commit) doesn’t add the notifications backend plugin.
- RHDH ships a separate dynamic wrapper for `backstage-plugin-notifications-backend`, but the adoption-insights backend wrapper does not depend on it.

### Fix Focus Areas
- workspaces/adoption-insights/plugins/adoption-insights-backend/src/plugin.ts[20-104]
- /cross_repos/rhdh/packages/backend/src/index.ts[121-177]
- /cross_repos/rhdh/dynamic-plugins/wrappers/red-hat-developer-hub-backstage-plugin-adoption-insights-backend-dynamic/package.json[1-44]
- /cross_repos/rhdh/dynamic-plugins/wrappers/backstage-plugin-notifications-backend-dynamic/package.json[1-45]

### Recommended fix approaches (choose one)
1) **RHDH coordination (recommended):**
  - Update RHDH’s default dynamic plugin set / catalog-index `dynamic-plugins.default.yaml` to ensure `backstage-plugin-notifications-backend` (and any required companion like signals backend/module) is enabled whenever adoption-insights backend is enabled.
  - Optionally, document the new requirement in RHDH/operator docs for the adoption-insights plugin.

2) **Make notifications optional in this plugin:**
  - Refactor so the core adoption-insights backend plugin can run without Notifications installed (e.g., split the scheduler/notifications feature into a separate backend module/plugin that depends on `notificationService`, or use an optional service reference pattern if supported).
  - Ensure `adoptionInsights.notifications.enabled: false` can fully avoid requiring Notifications services.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. CSV breaks for totals ✓ Resolved 🐞 Bug ≡ Correctness
Description
getTimeSavedTotals returns data as an object with { total_time_saved_minutes, templates }, but
getInsights assumes result.data is an array for both audit resultsCount and CSV export. This
makes audit metadata incorrect (0 results) and can break or generate unusable CSV for
type=time_saved_totals.
Code

workspaces/adoption-insights/plugins/adoption-insights-backend/src/database/adapters/BaseAdapter.ts[R466-471]

+    return {
+      data: {
+        total_time_saved_minutes: grandTotal,
+        templates: rows,
+      },
+    } as any;
Relevance

●●● Strong

Clear response-shape mismatch causes wrong counts/CSV; team typically fixes these deterministic
contract bugs.

PR-#2913

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The adapter returns a non-array data payload, while the controller uses result.data?.length and
passes result.data into the CSV parser, which assumes array-like semantics.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/database/adapters/BaseAdapter.ts[434-472]
workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts[186-220]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`time_saved_totals` returns a nested object in `result.data`, but `EventApiController.getInsights` treats `result.data` as an array (for `resultsCount` and CSV export). This causes incorrect audit metadata and potentially broken CSV exports for the new query type.

### Issue Context
- `BaseDatabaseAdapter.getTimeSavedTotals()` currently returns `{ data: { total_time_saved_minutes, templates: [...] } }`.
- `EventApiController.getInsights()` computes `resultsCount` via `result.data?.length` and, when `format=csv`, runs `json2csv` over `result.data`.

### Fix Focus Areas
- workspaces/adoption-insights/plugins/adoption-insights-backend/src/database/adapters/BaseAdapter.ts[434-472]
- workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts[186-220]

### Suggested fix
Implement one of the following (pick one and make it consistent across types):
1) Special-case `time_saved_totals` in `getInsights`:
  - Set `resultsCount` to `result.data.templates.length`.
  - If `format=csv`, export `result.data.templates` (and optionally add the grand total as a separate header/row).
2) Change `getTimeSavedTotals` to return an array in `data` (templates), and put the grand total in a `meta` field (or similar), keeping `data` consistently array-shaped across endpoints.
Add/adjust tests for CSV export and audit metadata for `time_saved_totals`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Notification run aborts early ✓ Resolved 🐞 Bug ☼ Reliability
Description
The scheduled notification job does not isolate failures per user; an exception from
getNotificationPreference or notification.send will abort the entire run. This can prevent later
users from receiving notifications for that day/week/month.
Code

workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts[R106-123]

+        for (const user of users) {
+          if (user.total_time_saved_minutes <= 0) continue;
+
+          const pref = await db.getNotificationPreference(user.user_ref);
+          if (pref !== frequency) continue;
+
+          const timeSaved = formatTimeSaved(user.total_time_saved_minutes);
+
+          await notification.send({
+            recipients: { type: 'entity', entityRef: user.user_ref },
+            payload: {
+              title: `You completed ${user.execution_count} self-service actions ${period}, saving an estimated ${timeSaved} of time.`,
+              link: `${baseUrl}/adoption-insights`,
+              topic: 'time-saved-summary',
+            },
+          });
+          sentCount++;
+        }
Relevance

●●● Strong

They accept adding error isolation to prevent one failure aborting a larger job flow.

PR-#2323

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The job awaits per-user DB and notification operations in-line; without try/catch, any rejection
propagates out and ends the scheduled function early.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts[61-129]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`scheduleTimeSavedNotifications` awaits DB lookups and `notification.send` inside nested loops without per-user error handling. Any thrown error stops processing and prevents other notifications from being sent in the same run.

### Issue Context
- The job iterates frequencies, then users.
- Per user it calls `db.getNotificationPreference` and `notification.send` without try/catch.

### Fix Focus Areas
- workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts[98-127]

### Suggested fix
- Wrap the per-user block in `try/catch`:
 - On error: log `user_ref`, frequency, and error; continue to next user.
- Consider also isolating per-frequency failures.
- Optionally track `failedCount` and include it in the final log so operators can see partial failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

4. PUT body destructure crash ✓ Resolved 🐞 Bug ☼ Reliability
Description
PUT /notification-preferences destructures const { frequency } = req.body without guarding
against req.body being undefined or null. This can throw a TypeError and return 500 instead of
a 400 validation response for empty/malformed requests.
Code

workspaces/adoption-insights/plugins/adoption-insights-backend/src/router.ts[R97-100]

+  router.put('/notification-preferences', async (req, res) => {
+    const userRef = await getUserEntityRef(req);
+    const { frequency } = req.body;
+
Relevance

●●● Strong

Simple crash-prevention input hardening; aligns with prior accepted request-body validation
tightening.

PR-#3536

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The handler directly destructures req.body before any validation, which will throw when req.body
is not an object.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/router.ts[91-112]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The PUT `/notification-preferences` handler destructures `req.body` unconditionally. If the request has an empty body (or body parsing yields `undefined`/`null`), this throws and results in a 500.

### Issue Context
Current code:
- `const { frequency } = req.body;`
- Then validates with `VALID_FREQUENCIES.includes(frequency)`.

### Fix Focus Areas
- workspaces/adoption-insights/plugins/adoption-insights-backend/src/router.ts[97-112]

### Suggested fix
- Replace destructuring with a safe read, e.g.:
 - `const frequency = (req.body ?? {}).frequency;`
 - Or validate `req.body` is a non-null object before reading.
- If missing/invalid, return 400 with the existing error message.
- Add a test for PUT with empty body and with `null` body to ensure it returns 400 (not 500).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Ingestion blocks on catalog calls ✓ Resolved 🐞 Bug ➹ Performance
Description
trackEvents now performs sequential Catalog lookups during request handling via `await
enrichScaffolderEvent(event)` inside a loop. This adds external dependency latency to event
ingestion and can delay/timeout requests for large batches or a slow catalog.
Code

workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts[R110-113]

+    for (const event of processedEvents) {
      const result = EventSchema.safeParse(event);

      if (!result.success) {
Relevance

● Weak

Analogous rejection: avoid blocking request path on slow external refresh/work; same risk with
catalog calls.

PR-#2671

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
processIncomingEvents awaits enrichment for each processed event, and enrichment performs an
awaited catalog call; the queueing step is synchronous, so request latency is directly impacted by
catalog response time and event count.

workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts[68-123]
workspaces/adoption-insights/plugins/adoption-insights-backend/src/domain/EventBatchProcessor.ts[59-66]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Event ingestion now blocks on catalog enrichment: `processIncomingEvents` awaits `enrichScaffolderEvent` for each event in sequence, and `enrichScaffolderEvent` does a `CatalogService.getEntityByRef` call. This increases `/events` POST latency proportional to the number of scaffolder-create events.

### Issue Context
- `trackEvents` awaits `processIncomingEvents` before responding 200.
- `EventBatchProcessor.addEvent` is synchronous and just enqueues, so the new request latency is dominated by catalog calls.

### Fix Focus Areas
- workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts[68-123]
- workspaces/adoption-insights/plugins/adoption-insights-backend/src/domain/EventBatchProcessor.ts[59-66]

### Suggested fix
- Fetch credentials once per request (outside the loop) and reuse.
- Enrich only the subset of events that match scaffolder-create criteria.
- Use bounded parallelism for catalog lookups (e.g., `p-limit`) or `Promise.all` for small batches, then enqueue enriched events.
- Keep the same functional outcome (store time-saved at write time) while reducing cumulative latency.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@rhdh-qodo-merge rhdh-qodo-merge Bot added enhancement New feature or request Tests labels Jul 23, 2026
Rajin Kichannagari and others added 2 commits July 28, 2026 10:59
- Make notificationService optional so plugin doesn't crash when
  notifications backend isn't installed
- Fix CSV export for time_saved_totals (data is object, not array)
- Add per-user error isolation in notification scheduler
- Guard PUT /notification-preferences against null/undefined body
- Parallelize catalog enrichment in event ingestion

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The installed @backstage/backend-plugin-api version doesn't expose
.optional() on ServiceRef. Keep notificationService as a required dep;
the config flag adoptionInsights.notifications.enabled already allows
disabling the feature.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rajin-kichannagari

Copy link
Copy Markdown
Contributor Author

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 3:26 AM UTC · Completed 3:44 AM UTC
Commit: d254b13 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

High

  • [stale-api-reference] workspaces/adoption-insights/plugins/adoption-insights-backend/README.md:111 — The Events API documentation table lists the allowed values for the type query parameter but does not include the new time_saved_totals type. The validation error message (visible in plugin.test.ts) now includes time_saved_totals, confirming the API accepts it, but the docs omit it. Users consulting the docs will not know this query type exists.
    Remediation: Add time_saved_totals to the type parameter description in the backend README.

Medium

  • [logic-error] workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts:121 — The refactored processIncomingEvents splits validation and event addition into two separate phases. If a batch contains events [A, B, C] and event C fails validation, events A and B will have auditEvent.success() recorded but will never be added to the processor (the throw exits before the enrichment/addition phase). This creates misleading audit records.
    Remediation: Defer auditEvent.success() calls until after events are successfully enriched and added to the processor.

  • [logic-error] workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts:101 — N+1 query pattern: for each frequency tier, the scheduler fetches ALL users who saved time, then individually queries each user's notification preference via db.getNotificationPreference(user.user_ref). With 1,000 active users, this is 1,000+ DB queries per scheduler run, risking timeouts against the 5-minute task limit.
    Remediation: Batch-fetch notification preferences with a method like getNotificationPreferencesByFrequency(frequency) or getNotificationPreferences(userRefs[]).

  • [rbac-violation] workspaces/adoption-insights/plugins/adoption-insights-backend/src/router.ts:88 — The new /notification-preferences GET and PUT endpoints authenticate the user (via getUserEntityRef) but do NOT call authorizeUser() to check adoptionInsightsEventsReadPermission. Every other data-access endpoint (GET /events) performs this permission check. A user denied the adoption-insights permission can still read/write notification preferences, inconsistent with the existing RBAC model. Additionally, these endpoints bypass the controller layer used by all other routes.
    Remediation: Add await authorizeUser(req) at the start of both handlers. Consider routing through EventApiController for consistency.

  • [missing-config-documentation] workspaces/adoption-insights/plugins/adoption-insights-backend/README.md:26 — New config key adoptionInsights.notifications.enabled (boolean, defaults to true) controls the notification scheduler but is not documented in the README's Configuration section.
    Remediation: Add the config key to the Configuration section with a YAML snippet.

  • [missing-endpoint-documentation] workspaces/adoption-insights/plugins/adoption-insights-backend/README.md:101 — Two new REST endpoints (GET /notification-preferences, PUT /notification-preferences) are not documented in the README.
    Remediation: Add a section documenting these endpoints with paths, methods, request/response formats, and valid frequency values.

  • [missing-setup-documentation] workspaces/adoption-insights/plugins/adoption-insights-backend/README.md:14@backstage/plugin-notifications-backend is now a required backend dependency (hard dependency at plugin init) but the Installation section does not mention this prerequisite.
    Remediation: Add a note to Installation about requiring the notifications backend.

  • [missing-annotation-documentation] workspaces/adoption-insights/plugins/adoption-insights-backend/README.md:1 — The PR introduces a new catalog entity annotation rhdh.redhat.com/time-saved that template owners must add for time-saved enrichment to work. No documentation explains this annotation or its expected values (positive integer = minutes saved per execution).
    Remediation: Add documentation explaining the annotation, where to place it, and what values it accepts.

Low

  • [edge-case] workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.ts:211 — The csvData extraction uses Array.isArray(result.data) ? result.data : result.data?.templates, an implicit assumption that works for time_saved_totals today but would silently skip CSV export for any future non-array query type without .templates.

  • [edge-case] workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts:93 — Monthly lookback uses fixed 30 days instead of a calendar month. For 31-day months, one day of data is missed; for February, two extra days are included.

  • [test-inadequate] workspaces/adoption-insights/plugins/adoption-insights-backend/src/controllers/EventApiController.test.ts:402 — The test mocks processIncomingEvents with synchronous (() => {}), but the method is now async. While await undefined works, this weakens coverage of the async enrichment flow.

  • [fail-open] workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts:62 — Notifications default to enabled when config key is absent (?? true). Operators upgrading will have notifications unexpectedly activated without explicit opt-in.

  • [data-exposure] workspaces/adoption-insights/plugins/adoption-insights-backend/src/notifications/scheduleTimeSavedNotifications.ts:118 — Warning log interpolates the full error object via template literal ${err}, potentially exposing internal URLs or connection strings in log aggregation.

  • [missing-constraint] workspaces/adoption-insights/plugins/adoption-insights-backend/migrations/20260716000000_notification_preferences.js:21 — The frequency column has no CHECK constraint limiting values to the valid set. While the API layer validates, getNotificationPreference casts raw DB values to NotificationFrequency without re-validating.

  • [return-type-convention] workspaces/adoption-insights/plugins/adoption-insights-backend/src/database/adapters/BaseAdapter.tsgetTimeSavedTotals declares Promise<Knex.QueryBuilder> but returns a plain object cast with as any, unlike other methods that return builder chains.

  • [intent-alignment] workspaces/adoption-insights/plugins/adoption-insights-backend/src/plugin.ts:50notificationService is a hard (non-optional) plugin dependency. If @backstage/plugin-notifications-backend is not installed, the entire adoption-insights plugin fails to initialize. The config flag only controls the scheduler, not the dependency at init.

  • [error-response-convention] workspaces/adoption-insights/plugins/adoption-insights-backend/src/router.ts:97 — Validation error for invalid frequency uses { error: '...' } shape, while existing validation errors use { message: '...', errors: { ... } } with fieldErrors.

- Move auditEvent.success() after enrichment to prevent audit/processing
  mismatch when a later event in the batch fails validation
- Add RBAC authorization to notification-preferences endpoints
- Document time_saved_totals query type in Events API table
- Document adoptionInsights.notifications.enabled config option
- Document notification-preferences REST endpoints
- Document @backstage/plugin-notifications-backend prerequisite
- Document rhdh.redhat.com/time-saved catalog annotation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@rajin-kichannagari

Copy link
Copy Markdown
Contributor Author

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 1:50 PM UTC · Completed 2:27 PM UTC
Commit: dcc0999 · View workflow run →

rajin-kichannagari pushed a commit to rajin-kichannagari/rhdh-plugins that referenced this pull request Jul 29, 2026
…ormat

Remove useTimeSavedTotals backend API call (API doesn't exist without
PR redhat-developer#3946) and compute time saved client-side from entity annotations.
Display in compact "1d 6h" format instead of "1 d 6 h".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rajin-kichannagari pushed a commit to rajin-kichannagari/rhdh-plugins that referenced this pull request Aug 3, 2026
- Remove NotificationSettings component, useNotificationPreference hook,
  and notification-related API methods/types (moved to PR redhat-developer#3946)
- Delete dead useTimeSavedTotals.tsx hook
- Translate compact time units via i18n keys instead of hardcoded d/h/m
- Add unit tests for formatTimeSavedCompact
- Regenerate API reports

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant