[ENH] Modified the result tables for humans and machines #357
Conversation
- Updated `generateTSVString` function - Updated relevant components
rmanaem
changed the title
✅ Deploy Preview for neurobagel-query ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rmanaem
added
the
pr-minor
There was a problem hiding this comment.
Thanks @rmanaem for putting this together so quickly and beautifully. The UI looks really awesome!
I left a couple of clarity comments for you to look at, mostly minor, but some of the new label matching functionality might need another look.
I think your e2e test of the generated TSV fails because during the e2e test you don't actually have a live n-API to talk to, and so the list of diagnoses or attributes with their labels is not "gotten".
I think we can either mock these requests and then just assert over a smaller set of diagnoses being correctly relabeled - or we go the long route and have a full on e2e test with live n-API and f-API.
I strongly prefer mocking because these kinds of expensive e2e tests should probably live in a different test suite.
Reviewer's Guide by SourceryThis PR modifies the result tables to improve readability and usability for both humans and machines. The changes include converting URIs to human-readable labels, restructuring the TSV output format, and updating the UI components to support these changes. The implementation focuses on maintaining data integrity while making the output more accessible. ER diagram for TSV output format changeserDiagram
DATASET {
string DatasetName
string PortalURI
int NumMatchingSubjects
string[] DatasetImagingModalities
string[] DatasetPipelines
}
SUBJECT {
string SubjectID
string SessionID
string SessionFilePath
string SessionType
int NumMatchingPhenotypicSessions
int NumMatchingImagingSessions
string[] SessionImagingModalities
string[] SessionCompletedPipelines
}
DATASET ||--o{ SUBJECT : contains
DATASET {
string DatasetName
string PortalURI
int NumMatchingSubjects
string[] DatasetImagingModalities
string[] DatasetPipelines
}
SUBJECT {
string SubjectID
string SessionID
string SessionFilePath
string SessionType
int NumMatchingPhenotypicSessions
int NumMatchingImagingSessions
string[] SessionImagingModalities
string[] SessionCompletedPipelines
}
DATASET ||--o{ SUBJECT : contains
Class diagram for GetDataDialog component changesclassDiagram
class GetDataDialog {
+ boolean open
+ void onClose()
+ boolean disableDownloadResultsButton
+ void handleDownloadResultButtonClick(identifier: string)
}
GetDataDialog --> DownloadResultButton
class DownloadResultButton {
+ string identifier
+ boolean disabled
+ void handleClick(identifier: string)
}
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rmanaem - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Hi @rmanaem! 👋
@sourcery-ai is now installed on this repository.
We found this recent PR of yours and reviewed it to show you what Sourcery can do.
If you want to review another PR, just comment with @sourcery-ai review ✨
|
Oh and I suggest |
There was a problem hiding this comment.
Hey @rmanaem, I missed that our downloaded files now also have spaces in them by default because button identifier, button text, and file name are all the same.
Spaces in the file names are going to be more of an issue than in the identifier, so let's fix that before merging.
Specifically make downloaded file names not have any spaces
rmanaem
changed the title
|
🚀 PR was released in |
Num{Phenotypic,Imaging}SessionstoNumMatching{Phenotypic,Imaging}Sessions#136Checklist
This section is for the PR reviewer
[ENH],[FIX],[REF],[TST],[CI],[MNT],[INF],[MODEL],[DOC]) (see our Contributing Guidelines for more info)skip-release(to be applied by maintainers only)Closes #XXXXquery-tool-resultsfiles in the neurobagel_examples repo have been regeneratedFor new features:
For bug fixes:
Summary by Sourcery
Modify the result tables to improve readability for both humans and machines by updating the ResultContainer component and adjusting the TSV download format. Enhance the Cypress tests to align with these changes.
Enhancements:
Tests: