feat(pci.savings-plan): view percentage consumption #15236
Conversation
Eric-ciccotti
requested review from
tibs245,
aboungnaseng-ovhcloud,
sidlynx,
seven-amid,
qpavy and
ghyenne
and removed request for
a team
Eric-ciccotti
changed the title
| const { t } = useTranslation('dashboard'); | ||
| const data = [ | ||
| const computedUsagePercent = calculateAverageUtilization(consumption); |
There was a problem hiding this comment.
should be wrapped in a useMemo
| import { SavingsPlanConsumption } from '@/types/savingsPlanConsumption.type'; | ||
|
|
||
| export const calculateAverageUtilization = ( | ||
| consumption: SavingsPlanConsumption | null, |
There was a problem hiding this comment.
I think the null value should be handled by the caller
There was a problem hiding this comment.
there ? const computedUsagePercent = calculateAverageUtilization(consumption);
| export const calculateAverageUtilization = ( | ||
| consumption: SavingsPlanConsumption | null, | ||
| ): number | null => { | ||
| if (!consumption?.flavors || consumption.flavors.length === 0) { |
There was a problem hiding this comment.
the test on flavors length is already handled line 19
Does consumption.flavors could be null ? If so I think you could handle this case line 10 with || []
This would simplifies the readability
There was a problem hiding this comment.
flavors couln't be null you right, I'll fix it
| @@ -0,0 +1,27 @@ | |||
| import { SavingsPlanConsumption } from '@/types/savingsPlanConsumption.type'; | |||
|
|
|||
| export const calculateAverageUtilization = ( | |||
There was a problem hiding this comment.
| export const calculateAverageUtilization = ( | |
| export const calculateAverageUsage = ( |
| utilizationValues.reduce((sum, value) => sum + value, 0) / | ||
| utilizationValues.length; | ||
|
|
||
| return Number(averageUtilization.toFixed(2)); |
There was a problem hiding this comment.
Fixing digits should be handled by the display layer and shouldn't be converted back to number or it could lead to errors due to IEE-754
There was a problem hiding this comment.
what do you suggest, it this should be fixed into the render ?
There was a problem hiding this comment.
Yes, the .toFixed should inside the react component
| import { SavingsPlanConsumption } from '@/types/savingsPlanConsumption.type'; | ||
|
|
||
| describe('calculateAverageUtilization', () => { | ||
| it('should return null when consumption is null', () => { |
There was a problem hiding this comment.
As a global improvement, you could iterate over an array of data
There was a problem hiding this comment.
you right, done
ref: TAPC-1779 Signed-off-by: Eric Ciccotti <[email protected]>
Eric-ciccotti
force-pushed
the
feat/TAPC-1779-view-percentage-consumption
branch
from
9121f2f to
1edad29
Compare
|
feat/TAPC-2358-svp-dashboardDescription
KPI view percentage consumption for savings plan