feat(adoption-insights): add Est. Time Saved column to Top Templates card - #3707
feat(adoption-insights): add Est. Time Saved column to Top Templates card#3707rajin-kichannagari wants to merge 11 commits into
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3707 +/- ##
==========================================
- Coverage 56.44% 56.41% -0.04%
==========================================
Files 2383 2387 +4
Lines 95609 95699 +90
Branches 26731 26735 +4
==========================================
+ Hits 53968 53989 +21
- Misses 41403 41473 +70
+ Partials 238 237 -1
*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:
|
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Tested Locally, /lgtm
Please do not merge this without approvals from @Eswaraiahsapram / @karthikjeeyar .
karthikjeeyar
left a comment
karthikjeeyar
left a comment
There was a problem hiding this comment.
Overall the changes looks good, just a couple of observations.
1.5hrs and 8hrs looks cramped especially in a table column where you 're scanning numbers quickly. IMO a space between the number and unit will improve readability.
| const totalMinutes = parsed * count; | ||
| if (totalMinutes < 60) { | ||
| return `${totalMinutes}min`; | ||
| } | ||
| const hours = totalMinutes / 60; | ||
| if (Number.isInteger(hours)) { | ||
| return `${hours}hrs`; | ||
| } | ||
| return `${hours.toFixed(1)}hrs`; |
There was a problem hiding this comment.
Units are hardcoded, can we translate them?
There was a problem hiding this comment.
@karthikjeeyar, what's your opinion on this? When the duration is more than 24 hours, for example, 28 hours - I think it would be better to display it as 1d 4h instead of 28h. What do you think?
cc - @ShiranHi
There was a problem hiding this comment.
How is this?
|
New changes are detected. LGTM label has been removed. |
|
Since the new translation keys are added, you must re-generate the api-reports by running |
| readonly 'filter.selectKind': string; | ||
| readonly 'units.days': string; | ||
| readonly 'units.hours': string; | ||
| readonly 'units.minutes': string; |
There was a problem hiding this comment.
I don't see this units.* strings defined in the ref.ts or in other translation files, can you check if you have missed something to commit?
c36d46f to
76106c5
Compare
…card - Add Est. Time Saved column showing per-template time savings (annotation value × execution count) on Top Templates card (RHIDP-15188) - Add useTimeSavedTotals hook to fetch aggregated time-saved data - Add formatTimeSaved utility for days/hours/minutes display - Add NotificationSettings component for frequency preferences - Add useNotificationPreference hook for GET/PUT preferences - Add translation keys for units, notifications, and table headers - Wire AdoptionInsightsNotificationSettings to Settings page Depends on backend PR for time_saved_totals API and notification preferences endpoints. RHDHPLAN-1553 / RHIDP-15188 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Karthik-provided report is for redhat-developer#3713, not this PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8db3df5 to
2a24e10
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Templates component now depends on useTimeSavedTotals hook which requires DateRangeProvider context. Add mock to prevent test crashes and update expected output to match translation format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extends the existing "New data shows in top templates" E2E test to assert the Est. Time Saved column header renders and the third cell is visible in each template row. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge duplicate imports from adoption-insights package in App.tsx, replace Promise.resolve with direct return in async getTimeSavedTotals, and use conventional useState destructuring in useNotificationPreference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| }, | ||
| ), | ||
| ); | ||
| }, [api, startDateRange, endDateRange]); |
There was a problem hiding this comment.
We do not have any API like time_saved_totals, I am not sure if you have changed something in recent commits. Few weeks ago, when I first reviewed this PR, it used to read the annotation for an entity and multiply it by the count to arrive at the timeSavedTotal, I think you may have to revert to that state.
Non-existing API is being called to get the data:
Due to the above issue, -- is displayed now.
There was a problem hiding this comment.
Fixed. Reverted to client-side computation using the entity annotation × count, like it was before. Also dropped the translation keys for the time display in favor of a compact formatter
There was a problem hiding this comment.
I believe you can keep the compact formatter and still translate. Given that this plugins support 5 languages and all the user-visible strings are translated, the time display should be too.
The units.days, units.hours, units.minutes keys are still present in all 5 language files (de/es/fr/it/ja) but were removed from ref.ts And formatTimeSavedCompact hardcodes English d/h/m, so a Japanese user sees 1d 6h instead of 1日 6時間. It would recommend passing the t function to the formatter and translate this as well.
…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>
|
karthikjeeyar
left a comment
karthikjeeyar
left a comment
There was a problem hiding this comment.
The scope of this PR should be the Est. Time Saved column only reading the rhdh.redhat.com/time-saved annotation and displaying it.
Suggestion is to move these Notification feature to a PR which brings in the backend support to store user's notification preferences.
| return { days, hours, minutes }; | ||
| } | ||
|
|
||
| export function formatTimeSavedCompact(result: TimeSavedResult): string { |
There was a problem hiding this comment.
Add unit test coverage for this function.
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ frequency }), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
UI breaks when you try changing the values in the dropdown:
This API endpoint /notification-preferences doesn't exist. I see code where GET and PUT request is made when you try to visit and modify the Time Saved notifcations in Notifications tab.
If you have follow up PR to add this API endpoint, then you should add these frontend components in that PR and I believe this PR should be scoped only to the Est. Time saved, let me know your thoughts.
There was a problem hiding this comment.
Dead code. please remove this file.
| getNotificationPreference(): Promise<NotificationPreferenceResponse>; | ||
| setNotificationPreference( | ||
| frequency: NotificationFrequency, | ||
| ): Promise<NotificationPreferenceResponse>; |
There was a problem hiding this comment.
These functions are basically calling Non-existing API endpoints, Please remove this usage in all the places.
| export type TimeSavedTemplate = { | ||
| entityref: string; | ||
| execution_count: number; | ||
| time_saved_per_execution: number; | ||
| total_time_saved_minutes: number; | ||
| }; | ||
|
|
||
| export type TimeSavedTotalsResponse = { | ||
| data: { | ||
| total_time_saved_minutes: number; | ||
| templates: TimeSavedTemplate[]; | ||
| }; | ||
| }; | ||
|
|
4 participants
Read the rhdh.redhat.com/time-saved annotation from template entities and display a calculated Est. Time Saved column (annotation × execution count) on the Top Templates card. Templates without the annotation show —.
RHDHPLAN-1553 / RHIDP 15188
✔️ Checklist