-
Notifications
You must be signed in to change notification settings - Fork 2
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
SET-238 OurDNA Dashboard v2 prototype #1014
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1014 +/- ##
==========================================
- Coverage 82.94% 82.46% -0.48%
==========================================
Files 191 188 -3
Lines 16788 16369 -419
==========================================
- Hits 13924 13499 -425
- Misses 2864 2870 +6 ☔ View full report in Codecov by Sentry. |
4b4cfbe
to
d0e813c
Compare
3525ebd
to
a467018
Compare
9968397
to
b85b25c
Compare
b85b25c
to
405c74e
Compare
Looks good to me on the first look 👍 |
Ship it 🚢 |
meta={'consent': True, 'field': 1}, | ||
samples=[ | ||
|
||
ANCESTRIES = [ |
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 expect this list to be configurable by event/PM team ?
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 is just for generating test data for testing the dashboard locally, the values don't matter much other than trying to be roughly indicative of what the real data will look like.
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.
Reviewed with MH :)
This is actually quite impressive work, I love how it has turned out! There were a few comments that Milo left on our behalf, but otherwise looks great!
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.
We had reviewed with @nevoodoo , very impressed, especially. CTE, amazing job.
We had just a few little comments.
useEffect(() => { | ||
if (!data) return | ||
const plotOptions = getPlotOptions(data) | ||
console.log(width, height) |
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 log this here?
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.
Ah thanks, good pickup, that's a leftover debug comment. I'll add a ticket to add a no-console.log linting rule to Jira.
return { | ||
...qq, | ||
// remove any trailing semicolons from the query, in case they were added accidentally | ||
query: stripIndent(qq.query.replace(/;[\w\s]*$/g, '')), |
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.
It is a bit unclear for us why trailing commas would be in the passed SQL query.
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.
Yeah it's a bit of an edge case, but the flow for creating the queries for reports is often to first start by writing a query in the sql editor and then copy/paste it into a report definition. If that query ends up as a CTE as part of a larger query then it would cause errors.
This is more of a future proofing thing for when we want to allow authoring reports via the UI where it could be more likely to happen.
} | ||
|
||
export const reports: Record<string, Record<string, ReportDefinition>> = { | ||
ourdna: { |
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 ask for clarification, just put a comment here, this is the place where we link project to custom reports. We've got lost a bit on this one.
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.
Yeah that's right, this is sort of an interim thing while we have custom reports but don't have them stored in the database, we need somewhere to store a listing of reports. The structure here works with the code here https://github.com/populationgenomics/metamist/blob/SET-238-dashboard-v-2-prototype/web/src/pages/report/ProjectReport.tsx#L9 which matches the params in the url to load the correct report.
It is also imported in the project summary page to show a list of reports for each project, matched on the project name.
This PR adds the prototype v2 version of the OurDNA dashboard.
The reporting components are structured in such a way that it should be easy to add reports to other projects.
Reports are defined in a declarative way so that it will be possible in the future to make this sort of reporting a 1st class citizen within metamist where the reports can be created via the UI and stored in the database.
At the moment there are 3 types of report items
To generate data for testing the dashboard, you can run the
test/data/generate_ourdna_data.py
script.The dashboard link will show up on the project summary page:

and is split into 4 tabs