-
Notifications
You must be signed in to change notification settings - Fork 4
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
[DUOS-2452] DAC Dataset Acceptance => Public Visibility #2305
Conversation
src/pages/DatasetCatalog.js
Outdated
const openAccess = extractDatasetProp('Open Access', dataset); | ||
if(!isNil(openAccess)){ | ||
// if open Access is false, dac approval required | ||
return openAccess ? dataset.study.publicVisibility : (dataset.dacApproval && dataset.study.publicVisibility); |
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.
study
can be null, do we need null-safe traversal 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.
My thinking was since the openAccess is a property on a study, if openAccess
is not null then the study must exist.
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 think that's true today, but the code doesn't enforce that currently and could be incorrect in the near future.
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.
Good point. Do you think that a good approach would be to just add dataset.study?.publicVisibility
or do more thorough handling 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.
I think that's fine 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.
One suggestion inline, otherwise 👍🏽
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.
Glad we're simplifying this logic 👍
Addresses
https://broadworkbench.atlassian.net/jira/software/c/projects/DUOS/boards/123?modal=detail&selectedIssue=DUOS-2452
Summary
A dataset should only be visible if Public Visibility is YES and it is ACCEPTED by the DAC,
unless it is an open access or external access dataset in which case it only needs Public Visibility to be set to YES.
Unless/until we migrate legacy datasets to have studies we rely on the Active dataset prop
Have you read Terra's Contributing Guide lately? If not, do that first.