Skip to content

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

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

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.

5 participants