RHIDP-14172 Refactor average aggregation type for Scorecard card#3417
RHIDP-14172 Refactor average aggregation type for Scorecard card#3417imykhno wants to merge 5 commits into
average aggregation type for Scorecard card#3417Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
|
🤖 Finished Review · ✅ Success · Started 2:19 PM UTC · Completed 2:35 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3417 +/- ##
=======================================
Coverage 54.12% 54.12%
=======================================
Files 2344 2344
Lines 89539 89546 +7
Branches 25076 25078 +2
=======================================
+ Hits 48460 48467 +7
Misses 39524 39524
Partials 1555 1555
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Previous run (4)ReviewFindingsLow
Info
Previous run (5)ReviewFindingsCritical
High
Medium
Low
Previous run (6)ReviewFindingsHigh
Medium
Low
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
imykhno
force-pushed
the
refactor/scorecard-aggregation-type
branch
from
4831a52 to
60f1f11
Compare
|
🤖 Review · Started 8:35 AM UTC |
|
🤖 Finished Review · ❌ Failure · Started 8:35 AM UTC · Completed 8:43 AM UTC |
|
/fs-review |
imykhno
requested review from
a team,
Eswaraiahsapram,
christoph-jerolimov and
its-mitesh-kumar
as code owners
|
🤖 Review · Started 9:29 AM UTC |
PR Summary by QodoRefactor Scorecard aggregation type: average → weightedStatusScore Description
Diagram
High-Level Assessment
Files changed (53)
|
Code Review by Qodo
Context used✅ Tickets:
RHIDP-14172 1.
|
rhdh-qodo-merge
Bot
added
documentation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 9:29 AM UTC · Completed 9:46 AM UTC |
|
🤖 Finished Review · ✅ Success · Started 11:55 AM UTC · Completed 12:06 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
dzemanov
left a comment
dzemanov
left a comment
There was a problem hiding this comment.
Thank you!
Tested that the changes work fine with weightedStatusScore set as type, the API response has also changed correctly.
The question is if we want to merge this now?
It will block some opened PRs (like #3282 and #3284) from releasing since other PRs with breaking changes will need to be included in the same scorecard release.
imykhno
force-pushed
the
refactor/scorecard-aggregation-type
branch
from
8ce2bdf to
53e158f
Compare
|
This pull request adds a new top-level directory under |
|
🤖 Finished Review · ✅ Success · Started 10:14 AM UTC · Completed 10:25 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
imykhno
force-pushed
the
refactor/scorecard-aggregation-type
branch
from
53e158f to
132f773
Compare
|
🤖 Finished Review · ✅ Success · Started 1:00 PM UTC · Completed 1:15 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
…dStatusScore` Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
… safety and improved structure Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…dKpi` across tests and configurations Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
imykhno
force-pushed
the
refactor/scorecard-aggregation-type
branch
from
132f773 to
ad8ef98
Compare
|
🤖 Finished Review · ✅ Success · Started 10:38 AM UTC · Completed 10:54 AM UTC |
fullsend-ai-review
Bot
dismissed stale reviews from themself
Superseded by updated review
fullsend-ai-review
Bot
added
requires-manual-review
| | `description` | Display description for this aggregation. | | ||
| | `type` | Aggregation algorithm: `statusGrouped` (counts per threshold status) or `weightedStatusScore` (normalized weighted score). | | ||
| | `metricId` | Metric provider id used to load thresholds and compute counts. | | ||
| | `options` | Optional for `statusGrouped`. **Required** for `weightedStatusScore`: must include **`options.statusScores`** — map status keys to numeric weights (typically one entry per **metric threshold rule key**). Optionally **`options.thresholds`** (same shape as metric thresholds; see [thresholds.md — Aggregation KPI result thresholds](./docs/thresholds.md#4-aggregation-kpi-result-thresholds-weighted-status-score-type)); evaluated on **`weightedStatusScore`** (**0–100** portfolio percentage, **one decimal**); first match sets **`aggregationChartDisplayColor`**. The API includes **`weightedStatusScore`**, **`weightedStatusSum`**, **`weightedStatusMaxPossible`**, and **`aggregationChartDisplayColor`** (from configured or default result thresholds). | |
There was a problem hiding this comment.
This anchor is broken: GitHub slugs camelCase as a single word, so the heading ### 4. Aggregation KPI result thresholds (weightedStatusScore type) resolves to #4-aggregation-kpi-result-thresholds-weightedstatusscore-type, not ...-weighted-status-score-type. The old average-type anchor worked because it's one word. Same broken link in docs/aggregation.md line 50.
|
|
||
| - `scorecard.aggregationKPIs.*.type`: `average` → `weightedStatusScore` | ||
|
|
||
| ### `GET /aggregations/:aggregationId` API |
There was a problem hiding this comment.
Since this changeset is the migration guide for the major bump, should it also list the translation key renames (metric.averageCenterTooltip* / metric.averageLegendTooltip* -> metric.weightedStatusScore*) and the data-testid renames (average-card-center-percent -> weighted-status-score-card-center-percent)? Anyone overriding scorecard translations or targeting these test ids in downstream e2e suites breaks silently otherwise.
| const weightedStatusScoreAggregationConfigSchema = z.object({ | ||
| ...baseAggregationConfigSchema.shape, | ||
| type: z.literal(aggregationTypes.average), | ||
| type: z.literal(aggregationTypes.weightedStatusScore), |
There was a problem hiding this comment.
With the discriminated union, an existing deployment still configured with type: average will fail startup with a generic zod invalid-discriminator error that doesn't hint at the rename. Did we consider either accepting average as a deprecated alias for one release, or at least a targeted error in validateAggregationConfig.ts like "average has been renamed to weightedStatusScore"? That would save operators a trip to the changelog.
| <Stack direction="column"> | ||
| <TooltipContent | ||
| label={t('metric.averageLegendTooltipEntitiesEach', { | ||
| label={t('metric.weightedStatusScoreLegendTooltipEntitiesEach', { |
There was a problem hiding this comment.
LegendTooltipContent isn't imported anywhere (on main or this branch), and the docs updated in this PR say the side legend was replaced by the center tooltip. Should we delete it instead of renaming? That would also drop the three weightedStatusScoreLegendTooltip* keys from ref.ts, all five locale files, and both report.api.md files - they only exist to serve this dead component.
| }; | ||
| }; | ||
|
|
||
| export type TooltipPosition = { left: number; top: number }; |
There was a problem hiding this comment.
nit: TooltipPosition is shared - components/CardTooltip.tsx and components/CardLegendContent.tsx both import it from this card-specific folder, same smell we had with AverageCard/types.ts. Since this PR already created the shared AggregatedMetricCards/types.ts for AggregatedMetricCardBaseProps, how about moving it there?
|
|
||
| const isStatusGroupedCardProps = ( | ||
| props: AggregatedMetricCardProps, | ||
| ): props is StatusGroupedCardComponentProps => |
There was a problem hiding this comment.
These guards only check metadata.aggregationType but assert the whole props shape including the result variant, so the compiler takes the result type on faith - functionally the same as the old casts. Fine as-is since the backend guarantees the pairing, but making AggregatedMetricResult a discriminated union in scorecard-common would let a plain switch narrow both sides without custom guards. Worth a follow-up? Also, the new type-only imports here could use import type like the old code did.
| title: GitHub Open PRs (weighted health) | ||
| type: average | ||
| description: Weighted health average for open PRs by threshold status across your entities. | ||
| type: weightedStatusScore |
There was a problem hiding this comment.
nit: pre-existing, but since this block was touched anyway - it sits at scorecard.openPrsWeightedKpi, while the backend reads scorecard.aggregationKPIs.<id>, so this example override does nothing as written. Nesting it under aggregationKPIs: would make the example actually work.
The e2e suite fails on CI because the click on the 'All' user picker times out: the picker item's container <li> (MUI ListItem container with the count badge as secondary action) intercepts pointer events, so Playwright retries until timeout. This breaks every entity test (60 failures across all locales) while passing locally, where font rendering keeps the click point inside the clickable area. Apply the 'all' user filter through the catalog URL query parameter instead of clicking the picker, removing the hit-testing dependency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Gustavo Lira <guga.java@gmail.com>
|
|
🤖 Finished Review · ✅ Success · Started 5:07 PM UTC · Completed 5:20 PM UTC |
| | `description` | Display description for this aggregation. | | ||
| | `type` | Aggregation algorithm: `statusGrouped` (counts per threshold status) or `weightedStatusScore` (normalized weighted score). | | ||
| | `metricId` | Metric provider id used to load thresholds and compute counts. | | ||
| | `options` | Optional for `statusGrouped`. **Required** for `weightedStatusScore`: must include **`options.statusScores`** — map status keys to numeric weights (typically one entry per **metric threshold rule key**). Optionally **`options.thresholds`** (same shape as metric thresholds; see [thresholds.md — Aggregation KPI result thresholds](./docs/thresholds.md#4-aggregation-kpi-result-thresholds-weighted-status-score-type)); evaluated on **`weightedStatusScore`** (**0–100** portfolio percentage, **one decimal**); first match sets **`aggregationChartDisplayColor`**. The API includes **`weightedStatusScore`**, **`weightedStatusSum`**, **`weightedStatusMaxPossible`**, and **`aggregationChartDisplayColor`** (from configured or default result thresholds). | |
There was a problem hiding this comment.
[medium] logic-error
Multiple markdown links use anchor #4-aggregation-kpi-result-thresholds-weighted-status-score-type, but the heading in thresholds.md generates anchor #4-aggregation-kpi-result-thresholds-weightedstatusscore-type. GitHub lowercases and strips backticks without inserting hyphens within camelCase words. The old anchor worked because average had no camelCase. Same broken anchor in aggregation.md.
Suggested fix: Change the anchor fragment in README.md and aggregation.md to #4-aggregation-kpi-result-thresholds-weightedstatusscore-type, or change the heading in thresholds.md to use a hyphenated form.
fullsend-ai-review
Bot
added
requires-manual-review
3 participants
Hey, I just made a Pull Request!
Based on our discussion regarding the new scorecard aggregation types, we decided that the currently implemented
averageis actually a weighted status score — not an average of raw values. Because of this, we have decided to refactor the name of the average type.For:
✔️ Checklist
How to test:
Confirm that
weightedStatusScoreaggregation type works correctly:app-config.yaml, for example: