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

Austenem/CAT-793 unified datasets attribution #3551

Merged
merged 11 commits into from
Oct 2, 2024

Conversation

austenem
Copy link
Collaborator

Summary

EPICs and Lab Processed datasets should be attributed to the labs that uploaded the dataset. This update removes the "Contact" from the "Summary" section and adds an "Attribution" section for EPICs and Lab Processed datasets.

Design Documentation/Original Tickets

CAT-793 Jira ticket

Figma mockup

Testing

Checked unified views pages with centrally processed and lab processed datasets to ensure sections matched designs.

Lab processed: http://localhost:5001/browse/dataset/225280788b0c66d095d05c4e36a89b81?redirected=True#section-labprocessed-published
Lab & centrally processed: http://localhost:5001/browse/dataset/9046d09943f6a10b70544822de1d5752?redirected=True#section-labprocessed-published

Screenshots/Video

Screenshot 2024-09-26 at 11 38 26 AM

Checklist

  • Code follows the project's coding standards
    • Lint checks pass locally
    • New CHANGELOG-your-feature-name-here.md is present in the root directory, describing the change(s) in full sentences.
  • Unit tests covering the new feature have been added
  • All existing tests pass
  • Any relevant documentation in JIRA/Confluence has been updated to reflect the new feature
  • Any new functionalities have appropriate analytics functionalities added

analysis: Boolean(metadata?.dag_provenance_list),
attribution: creation_action !== 'Central Process' && Boolean(contributors),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's introduce constants for creation_action.

  • Create Dataset Activity
  • Central Process
  • Multi-Assay Split
  • Lab Process
  • Create Publication Activity
  • External Process

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea - I wrote up a type CreationAction that represents that set of strings. Is that what you had in mind, or would creating individually named constants to represent these strings/an enum be preferable?

} = useProcessedDatasetContext();

if (mapped_consortium !== 'HuBMAP') {
if (creation_action !== 'Central Process') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use a constant here as well.

dataset: { creation_action, contributors, contacts },
} = useProcessedDatasetContext();

if (creation_action === 'Central Process' || !contributors) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Constant here as well.

</LabelledSectionText>
<Stack spacing={1}>
<ProcessedDatasetDescription />
<LabelledSectionText label="Consortium">{dataset.group_name}</LabelledSectionText>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is 'Consortium' the right label for group name? Should group_name be mapped_consortium? Do we also want to display the group_name? @tsliaw

Copy link
Contributor

Choose a reason for hiding this comment

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

Consortium should be the label for mapped_consortium. I didn't include the group field initially since I assumed that the group should be the same for both the raw and processed, but that's probably not true. @austenem Let's add the group field before the consortium field in the summary section. Can you also replace the consortium field in the mini processed data detail section (the pop-up thing on the right) with group instead? That field should be more useful than consortium.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tsliaw Will do! Would it make sense to also replace the "Consortium" field in the summary view top bar, or to add a "Group" field?

context/app/static/js/pages/Dataset/hooks.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

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

Great, thanks for the updates!

@austenem austenem merged commit ea21ec1 into main Oct 2, 2024
8 checks passed
@austenem austenem deleted the austenem/cat-793-unified-datasets-attribution branch October 2, 2024 15:07
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.

3 participants