-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MPDX-8478 - Add MPD-HI Card to Dashboard #1221
Conversation
[no-Jira] Refresh 14 month report after account list redirect
Preview branch generated at https://add-hi-graph-to-dashboard.d3dytjb8adxkk5.amplifyapp.com |
…rease-ask-date Allow user to remove next increase ask date
… tests to ensure the issue is caught.
…ne time import, unless the user re-imports.
Also allowing the user to delete the contact, and warning them
…ourced-contacts MPDX-8488 - Prevent users from deleting Siebel contacts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! Great work so far.
...components/Reports/HealthIndicatorReport/HealthIndicatorWidget/HealthIndicatorWidget.graphql
Outdated
Show resolved
Hide resolved
src/components/Reports/HealthIndicatorReport/HealthIndicatorWidget/HealthIndicatorWidget.tsx
Outdated
Show resolved
Hide resolved
...omponents/Reports/HealthIndicatorReport/HealthIndicatorWidget/HealthIndicatorWidget.test.tsx
Show resolved
Hide resolved
src/components/Reports/HealthIndicatorReport/HealthIndicatorWidget/HealthIndicatorWidget.tsx
Show resolved
Hide resolved
…e when we pass the data to the API since we get the data in lowercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e3940ad
to
31a2a39
Compare
@@ -85,15 +85,21 @@ const StyledProgress = ({ | |||
<> | |||
<ProgressBar | |||
style={{ | |||
width: percentageFormat(secondary, locale).replace('\xa0', ''), | |||
width: `calc(${percentageFormat(secondary, locale).replace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice this before, but I don't think we should be using a localized percentage format for the CSS width. Different locales use different separators or even put the percent sign first, which will result in invalid CSS syntax. I think we should just do (secondary / 100).toFixed() + '%'
since we want the percentage in a specific format. Thanks for the - 4px
fix though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, maybe I needed to do a hard refresh to flush the cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
MPDX-8491 - Fix Activity Type Case-Sensitivity Issue in Google Calendar Integration
[no-Jira] Update translations
* Sort tag list after they are joined * Make button content left aligned for short tag names
…ountList Id if they have an underscore as their accountList ID
MPDX-8492 - Enhance loadSession to Redirect Placeholder _ to Default Account List
…parameters to change the height of the process bar.
…ger using the localePercentage function
a18307b
to
0d3cbc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
Change in average Code Health of affected files: +0.09 (8.64 -> 8.73)
- Declining Code Health: 5 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
@@ -69,6 +69,7 @@ export const FourteenMonthReport: React.FC<Props> = ({ | |||
useEffect(() => { | |||
(async () => { | |||
try { | |||
setFourteenMonthReportError(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Getting worse: Complex Method
FourteenMonthReport:React.FC<Props> already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132
expect(mutationSpy.mock.lastCall).toMatchObject([ | ||
{ | ||
operation: { | ||
operationName: 'UpdateContactPartnership', | ||
variables: { | ||
attributes: { | ||
likelyToGive: 'MOST_LIKELY', | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ Getting worse: Code Duplication
introduced similar code in: 'should allow for user to remove the start date','should allow user to remove next ask date'
@@ -5,6 +5,7 @@ import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider'; | |||
import { render, waitFor } from '@testing-library/react'; | |||
import userEvent from '@testing-library/user-event'; | |||
import { SnackbarProvider } from 'notistack'; | |||
import TestRouter from '__tests__/util/TestRouter'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Getting better: Code Duplication
reduced similar code in: 'should handle closing modal | Cancel Button','should handle closing modal | Close Button','should render edit partnership info modal','should save when only status is inputted'
} | ||
|
||
export const DeleteContactModal: React.FC<DeleteContactModalProps> = ({ | ||
open, | ||
setOpen, | ||
deleting, | ||
deleteContact, | ||
contactId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Complex Method
DeleteContactModal:React.FC<DeleteContactModalProps> has a cyclomatic complexity of 18, threshold = 10
@@ -993,10 +993,22 @@ class MpdxRestApi extends RESTDataSource { | |||
googleIntegrationId, | |||
googleIntegration, | |||
) { | |||
const attributes = {}; | |||
interface GoogleIntegrationAttributes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ Getting worse: Lines of Code in a Single File
The lines of code increases from 1014 to 1024, improve code health by reducing it to 1000
@@ -63,15 +65,40 @@ const MonthlyGoal = ({ | |||
pledged = 0, | |||
totalGiftsNotStarted, | |||
currencyCode = 'USD', | |||
}: Props): ReactElement => { | |||
const { classes } = useStyles(); | |||
}: MonthlyGoalProps): ReactElement => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Getting worse: Complex Method
MonthlyGoal increases in cyclomatic complexity from 14 to 19, threshold = 10
it('should set to TRUE as machine goal is defined and the same as the monthly goal', async () => { | ||
const { findByText } = render( | ||
<Components | ||
healthIndicatorData={[healthIndicatorScore]} | ||
goal={healthIndicatorScore.machineCalculatedGoal} | ||
/>, | ||
); | ||
|
||
expect(await findByText('Ownership')).toBeInTheDocument(); | ||
expect(setUsingMachineCalculatedGoal).toHaveBeenCalledWith(true); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Code Duplication
The module contains 3 functions with similar structure: 'should set to FALSE as machine goal is different than the monthly goal','should set to FALSE as machine goal is not defined','should set to TRUE as machine goal is defined and the same as the monthly goal'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
Change in average Code Health of affected files: +0.09 (8.64 -> 8.73)
- Declining Code Health: 5 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
const { data, loading } = useHealthIndicatorWidgetQuery({ | ||
variables: { | ||
accountListId, | ||
month: DateTime.now().startOf('month').toISODate(), | ||
}, | ||
}); | ||
|
||
useEffect(() => { | ||
setShowHealthIndicator(!!data?.healthIndicatorData.length); | ||
const machineCalculatedGoal = | ||
data?.healthIndicatorData[0]?.machineCalculatedGoal; | ||
setUsingMachineCalculatedGoal( | ||
!!machineCalculatedGoal && goal === machineCalculatedGoal, | ||
); | ||
}, [data, goal]); | ||
|
||
if (!showHealthIndicator) { | ||
return null; | ||
} | ||
|
||
const currentStats = data?.healthIndicatorData[0]; | ||
|
||
return ( | ||
<AnimatedCard sx={{ height: '100%' }}> | ||
<CardHeader | ||
title={t('MPD Health Indicator')} | ||
titleTypographyProps={{ | ||
style: { fontSize: '1rem' }, | ||
}} | ||
/> | ||
<StyledCardContent> | ||
<Tooltip | ||
title={`${t('MPD Health')} = [(${t('Ownership')} * 3) + (${t( | ||
'Success', | ||
)} * 2) + (${t('Consistency')} * 1) + (${t('Depth')} * 1)] / 7`} | ||
arrow | ||
> | ||
<StyledBox> | ||
<Typography variant="h4" color="primary" width={'55px'}> | ||
{currentStats?.overallHi} | ||
</Typography> | ||
<Box width={'calc(100% - 55px)'}> | ||
<Typography | ||
component="div" | ||
color="primary" | ||
sx={{ marginBottom: 1 }} | ||
> | ||
{t('Overall Health Indicator')} | ||
</Typography> | ||
<StyledProgress | ||
loading={loading} | ||
primary={ | ||
currentStats?.overallHi ? currentStats.overallHi / 100 : 0 | ||
} | ||
barHeight={20} | ||
/> | ||
</Box> | ||
</StyledBox> | ||
</Tooltip> | ||
|
||
<Grid container> | ||
<WidgetStat | ||
loading={loading} | ||
stat={currentStats?.ownershipHi} | ||
statName={t('Ownership')} | ||
toolTip={t('% of Self-raised Funds over Total Funds')} | ||
/> | ||
<WidgetStat | ||
loading={loading} | ||
stat={currentStats?.consistencyHi} | ||
statName={t('Consistency')} | ||
toolTip={t('% of months with positive account balance')} | ||
/> | ||
<WidgetStat | ||
loading={loading} | ||
stat={currentStats?.successHi} | ||
statName={t('Success')} | ||
toolTip={t('% of Self-raised Funds over Support Goal')} | ||
/> | ||
<WidgetStat | ||
loading={loading} | ||
stat={currentStats?.depthHi} | ||
statName={t('Depth')} | ||
toolTip={t('Trend of local partners')} | ||
/> | ||
</Grid> | ||
</StyledCardContent> | ||
<CardActions> | ||
<Button | ||
LinkComponent={NextLink} | ||
href={`/accountLists/${accountListId}/reports/healthIndicator`} | ||
size="small" | ||
color="primary" | ||
> | ||
{t('View Details')} | ||
</Button> | ||
</CardActions> | ||
</AnimatedCard> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Complex Method
HealthIndicatorWidget:React.FC<HealthIndicatorWidgetProps> has a cyclomatic complexity of 14, threshold = 10
Due to the state of the commit history, I closed this PR and cherry-picked it to this PR - #1221 |
Description
In this PR, I have created the Health Indicator widget and add it to the dashboard. The data I use is the last item in the
healthIndicatorData
array, which I fetch using theHealthIndicatorWidget
GQL query. (I'm unsure if that is correct, but I'm going with it for now.)I've added tests for the HI widget and Monthly Goal Components.
Mentionable changes
Process Bar Component: Switched
makeStyles()
tostyled()
components to allow me to make the HI process bar thinner.Monthly Goal Component: If HI data is present, make it responsive with the HI widget.
Jira Task - MPDX-8478
Checklist: