fix: do not show flagsPerUser when calculation results to NaN #6639
Conversation
Signed-off-by: andreas-unleash <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
| condition={ | ||
| flagsPerUser !== undefined && | ||
| flagsPerUser !== '' && | ||
| flagsPerUser !== 'NaN' |
There was a problem hiding this comment.
Wait, are you saying that the value we store here the string "NaN"? Or is it the value NaN?
I would expect this comparison to be done using the Number.isNan function, but I don't know what the value we get from the API is in this case? If it is the string, then we should fix this on the API level, right?
There was a problem hiding this comment.
The value is coming from a calculation parsed to a string. So yes the value that gets here is 'NaN' and because of the other string comparisons here, I added it here for consistency
count={summary.total}
flagsPerUser={
showAllProjects
? (summary.total / users.total).toFixed(2)
: ''
}
/>```
frontend/src/component/executiveDashboard/componentsStat/FlagStats/FlagStats.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: andreas-unleash <[email protected]>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
| showAllProjects | ||
| ? (summary.total / users.total).toFixed(2) | ||
| : '' | ||
| showAllProjects ? summary.flagsPerUser : '' |
There was a problem hiding this comment.
✅ Getting better: Large Method
Charts:VFC<IChartsProps> decreases from 138 to 136 lines of code, threshold = 120
Why does this problem occur?
Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function. Read more.
Signed-off-by: andreas-unleash <[email protected]>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
Signed-off-by: andreas-unleash <[email protected]>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
frontend/src/component/executiveDashboard/componentsStat/FlagStats/FlagStats.tsx
Outdated
Show resolved
Hide resolved
| return { | ||
| ...sum, | ||
| flagsPerUser: | ||
| Number.isNaN(flagsPerUser) || flagsPerUser === 0 |
There was a problem hiding this comment.
Why are we using "N/A" if flags per user is 0? If there are 0 flags, then 0 flags per user is an entirely valid value, isn't it?
| @@ -124,9 +125,7 @@ export const Charts: VFC<IChartsProps> = ({ | |||
| <FlagStats | |||
| count={summary.total} | |||
| flagsPerUser={ | |||
| showAllProjects | |||
| ? (summary.total / users.total).toFixed(2) | |||
There was a problem hiding this comment.
Sorry, this has taken me a while to realize, but this is what caused us to store the string 'NaN', right?
Couldn't we have gotten around all the other changes here by instead doing something like:
const averageFlagsPerUserCalculation = summary.total / users.total
const averageFlagsPerUser = averageFlagsPerUserCalculation.isNan() ? 'N/A' : averageFlagsPerUserCalculation.toFixed(2)and then using that value as the prop instead?
That way, we don't need to update any other files, we don't need to touch the hook, and we don't need to touch any tests, etc.
There was a problem hiding this comment.
I think it makes more sense to move the calculation to the summary hook inline with the rest of the calculations.
There was a problem hiding this comment.
Sure. You know the system better than I do, so I'll trust you on that.
…tats/FlagStats.tsx Co-authored-by: Thomas Heartman <[email protected]>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
Signed-off-by: andreas-unleash <[email protected]>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
frontend/src/component/executiveDashboard/hooks/useFilteredFlagsSummary.ts
Outdated
Show resolved
Hide resolved
| @@ -124,9 +125,7 @@ export const Charts: VFC<IChartsProps> = ({ | |||
| <FlagStats | |||
| count={summary.total} | |||
| flagsPerUser={ | |||
| showAllProjects | |||
| ? (summary.total / users.total).toFixed(2) | |||
There was a problem hiding this comment.
Sure. You know the system better than I do, so I'll trust you on that.
…gsSummary.ts Co-authored-by: Thomas Heartman <[email protected]>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
Signed-off-by: andreas-unleash <[email protected]>
There was a problem hiding this comment.
✅ Code Health Quality Gates: OK
- Declining Code Health: 0 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
✅ Improving Code Health:
- Large Method Charts.tsx: Charts:VFC<IChartsProps>
What it says on the tin
Closes # 1-2209