-
Notifications
You must be signed in to change notification settings - Fork 7
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
[TTAHUB-3402] Hook up QA widgets to BE #2394
base: al-ttahub-3316-3317-overview-widgets
Are you sure you want to change the base?
[TTAHUB-3402] Hook up QA widgets to BE #2394
Conversation
…02-connect-be-overview
…02-connect-be-overview
…02-connect-be-overview
…om/HHS/Head-Start-TTADP into al-ttahub-3402-connect-be-overview
…02-connect-be-overview
…om/HHS/Head-Start-TTADP into al-ttahub-3402-connect-be-overview
…om/HHS/Head-Start-TTADP into al-ttahub-3402-connect-be-overview
…02-connect-be-overview
Looked at this branch on dev, on the main QA dash landing page the filters don't seem to update the page |
The new dashboard routes 404 and I also don't see them in your code so I expect you have a bad merge w/main |
Bad merge should now be fixed. |
frontend/src/fetchers/ssdi.js
Outdated
'region', | ||
'regionIds', |
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.
why do we need both?
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 was waiting till Garrett updated the BE, I have removed it.
frontend/src/fetchers/ssdi.js
Outdated
const url = getSelfServiceUrl(filterName, filters); | ||
|
||
const response = await get(url); | ||
const urlToUse = url + (dataSetSelection && dataSetSelection.length ? dataSetSelection.map((s) => `&dataSetSelection[]=${s}`).join('') : ''); |
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.
do we need to account for this case (dataSetSelection is falsy?) If so, did we add a test?
Note also there is no need to check for length: if dataSetSelection is empty, the map will be an empty array and will join out to be the same as your else statement
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.
Removed checks.
const requestSort = useCallback((sortBy, passedDirection = null) => { | ||
// Get sort direction. | ||
let direction = 'asc'; | ||
if ( | ||
if (passedDirection) { | ||
// If the direction is passed, use it. | ||
direction = passedDirection; | ||
} else if ( |
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.
Can you leave a comment explaining why we need to optionally override the sortConfig direction?
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.
Just seems like you could mutate the object that already has the direction in it instead of adding another parameter and all this additional logic
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 added a comment.
This seems to just take the opposite of whats currently set in the sortConfig (arrow sort).
@@ -15,7 +17,7 @@ function GoalCard({ | |||
<DataRow | |||
label="Goal number" | |||
value={( | |||
<Link to="recipient-tta-records/376/region/1/goals?id[]=83697"> | |||
<Link to={`../../recipient-tta-records/${recipientId}/region/${regionId}/goals?id[]=${goal.id}`}> |
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.
minor but I don't think you need the ../../, just starting with "/recipient-tta-records" should do it
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.
You are correct.
}); | ||
updateError(''); | ||
} catch (e) { | ||
updateError('Unable to fetch QA data'); |
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.
No test for this case I don't think
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.
added
@@ -24,7 +24,7 @@ const DEFAULT_SORT_CONFIG = { | |||
activePage: 1, | |||
}; | |||
|
|||
export default function PercentageActivityReportByRole({ data }) { | |||
export default function PercentageActivityReportByRole({ data, loading }) { |
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 left the loading attribute off these graphs on purpose; we don't really want all those announcements/mini "loading" spinners since the whole page is wrapped in a loader. There's an outstanding accessibility ticket to fix it. everywhere.
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.
Ok I removed loaders from all of the individual widgets and just added one loader on the dashboard itself.
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 has been cleaned up.
} | ||
} | ||
// Call resources fetch. | ||
fetchQaDat(); |
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.
can we fix the spelling of this method?
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.
yes
}); | ||
updateError(''); | ||
} catch (e) { | ||
updateError('Unable to fetch QA data'); |
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.
no test for this case
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.
added
}); | ||
updateError(''); | ||
} catch (e) { | ||
updateError('Unable to fetch QA data'); |
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.
same comments as before, no test for this case that I can see, and the function has a typo
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.
added test fixed typo
FYI @AdamAdHocTeam, @GarrettEHill Dashboard no longer loads any data at all |
@thewatermethod this should be good now @GarrettEHill resolved the BE issues. |
@AdamAdHocTeam @GarrettEHill Filters that error:
Filters that do not seem to update the data (none of the charts, even the ones that would make sense to update, lke the root cause chart)Please double check these are being applied
These only apply to the landing page; when I started picking through the sub pages I found similar issues but I'm not going to document them rn |
@GarrettEHill can you please have a look at this on Monday seems we are missing data points and filters. |
const totalRecipients = widgetData ? widgetData.total : 0; | ||
const grants = widgetData ? widgetData['grants with class'] : 0; | ||
const pct = widgetData ? widgetData['% recipients with class'] : 0; | ||
const recipoientsWithClass = widgetData ? widgetData['recipients with class'] : 0; |
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.
spelling
This is a frontend change, they were hardcoded in |
Description of change
This hooks up the new QA dashboard to the SSDI.
How to test
Issue(s)
Checklists
Every PR
Before merge to main
Production Deploy
After merge/deploy