Skip to content
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

Test/aggmds counts #1064

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Test/aggmds counts #1064

wants to merge 22 commits into from

Conversation

craigrbarnes
Copy link
Contributor

@craigrbarnes craigrbarnes commented Aug 22, 2022

Jira Ticket: BRH-169

New Features

  • Changes how the Discovery Page retrieves metadata from the aggregateMDS. Instead of pulling the __manifest field, it adds the count option to the aggregate MDS which returns the length of the array. If a study is selected the array contents of the __manifest field is pulled from the aggreateMDS.

Breaking Changes

Bug Fixes

  • if _subject_count is not a number, change the value to "N/A".

Improvements

Dependency updates

Deployment changes

@@ -89,7 +89,7 @@ export const renderFieldContent = (content: any, contentType: 'string'|'paragrap
if (Array.isArray(content)) {
return content.join(', ');
}
return content.toLocaleString();
return content ? content.toLocaleString() : 'N/A';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice I like this as this won't crash undef values

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions: does a missing content value need to be type matched? eg: if an int is missing the portal code should be looking for 0 as opposed to N/A? also how about __manifest? If there is none, that should be [] as opposed to N/A?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I have updated the expression

@@ -89,7 +89,7 @@ export const renderFieldContent = (content: any, contentType: 'string'|'paragrap
if (Array.isArray(content)) {
return content.join(', ');
}
return content.toLocaleString();
return content ? content.toLocaleString() : 'N/A';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use valueIfNotAvailable to handle missing values? Just like what we were doing in existing codes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works, however, it requires the configuration to ensure all fields are using valueIfNotAvailable
But that is cleaner solution

@ocshawn
Copy link
Contributor

ocshawn commented May 26, 2023

@craigrbarnes is this still needed?

@craigrbarnes
Copy link
Contributor Author

@craigrbarnes is this still needed?

Yes. I'm looking at it now. It is needed for BRH

Copy link
Contributor

@ocshawn ocshawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants