360 - Refactor Dataset Page to handle dataset ID via URL query #364

nozomione · 2024-09-17T21:34:57Z

Issue Number

Epic: #358

Closing #360 and #362

Feature branch: feature/epic-refactor-dataset-page-data-handling

Purpose/Implementation Notes

I've made the following changes to the dataset page and its sub-components:

Managed dataset data via URL query
Removed states for various dataset types
Moved the dataset status variables to sub-component level and simplify JSX
Added check to redirect users if dataset/id matches their My Dataset ID

Types of changes

Refactor (addresses code organization and design mentioned in corresponding issue)

Functional tests

N/A

Checklist

Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in downstream modules

Screenshots

N/A

…data structure (i.e., 'dataset') to sub-components, define dataset state variables (i.e., 'isProcessed', 'isUnprocessed') in sub-components and clean up the JSX rendering condition, in pages/dataset/[datast_id] and pages/download

…ternary in JSX, and destructure 'dataset' in DatasetSummary

…et-id-via-url-query

vercel · 2024-09-17T21:35:01Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
refinebio-web	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 7, 2024 9:17pm

davidsmejia

Wrote a couple of commnets. I'd like to improve how we distinguish between a dataset from a prop and the myDataset and when / if it matters. Let me know your thoughts on my suggestion. Otherwise the bulk of the PR looks correct to me.

davidsmejia · 2024-09-24T20:40:53Z

src/components/Dataset/DatasetPageHeader.js

@@ -79,7 +81,7 @@ export const DatasetPageHeader = ({ dataset }) => {
  return (
    <FixedContainer pad="none">
      <Box>
-        {dataset?.data && (
+        {data && (


Do we still need this check here?

davidsmejia · 2024-09-24T20:46:02Z

src/components/Dataset/MoveToDatasetButton.js

Could you give me feedback on this suggestion?
I think I'd like to make the following changes for consistency.

Keep the prop as dataset since that is the dynamic part and would match convention.

Instead of renaming dataset.data to datasetData always use dataset.data

Instead of renaming dataset.data and dataset.id from to myDatasetid and myDatasetData just rename the whole dataset to myDataset so we will always access attributes on myDataset or dataset.

In the future we may consider makeing myDataset an export of useDatasetManager but I need to review that holistically before suggesting that change.

Thank you for these great suggestions 👍 I totally agree with keeping the prop as dataset and using dataset.data. Renaming the dataset context state to myDataset is an excellent idea for clarity and readability as well! Let’s discuss this further in our 1:1, and once we finalize the implementation strategy, we can file a new issue for it. Looking forward!

davidsmejia · 2024-09-24T20:59:01Z

src/components/Dataset/ShareDatasetButton.js

@@ -12,14 +12,14 @@ import { Modal } from 'components/shared/Modal'
 import { TextInput } from 'components/shared/TextInput'

 export const ShareDatasetButton = ({ dataset }) => {
+  const { id: datasetId } = dataset


Based on the outcome of my earlier comment we may want to keep dataset.id here so that it is clearer to future maintainers that any give datasetId is either from what is passed in vs myDataset and will indicate if it matters.

…mponents, and remove the conditional check for data (no longer required) in DatasetPageHader

nozomione · 2024-09-25T16:26:16Z

I've applied your feedback and this PR is ready for another look. Thank you David!

…'id' and 'data' (until we rename the user-defined 'dataset' context state to 'myDataset' in a separate issue)

nozomione · 2024-09-26T19:35:15Z

I've updated the naming of the user-defined dataset to myDataset at the component level. I'll file a new issue for the renaming of the dataset and datasetId context states (for user-saved data in LocalStorage) once we finalize the naming for datasetId etc.

I've also filed the issue for the getDatasetState helper which came up during our 1:1, please see #370.

This PR is now ready for your review. Thank you David!

davidsmejia · 2024-09-25T19:48:32Z

src/components/Dataset/MoveToDatasetButton.js

  const { push } = useRouter()
-  const { addSamples, getTotalSamples } = useDatasetManager()
+  const {
+    dataset: { id: myDatasetId, data: myDatasetData },


Suggested change

dataset: { id: myDatasetId, data: myDatasetData },

dataset: myDataset,

And then access the attributes in the same way so that it is consistent.

davidsmejia · 2024-09-25T20:39:37Z

src/components/Dataset/DatasetPageHeader.js

+  const {
+    expires_on: expiredOn,
+    is_available: isAvailable,
+    is_processed: isProcessed,
+    is_processing: isProcessing,
+    success
+  } = dataset
+  const isExpired = moment(expiredOn).isBefore(Date.now())
+  const isProcessingError = success === false // 'success' may be null


I think we should replace this with a helper that wraps the actual meaning of these state attributes. They are not the clearest names on the API. We can't really change them very easily there because external scripts might be using them so here we should just abstract away the specifics into the actionable states.

I think you can clean this up but the important part is that logically only one of these states can be true at one #time.

const getDatasetState = (dataset) => { // datasets go in the following states: // isNotProcessed -> isProcessing -> isReady -> isExpired // | -> isError // processing was attempted but an error occurred and no output was able to be created const isErrored = dataset.success === false // the dataset is still available to be edited by the token holder const isNotStarted = dataset.is_processing === false && dataset.is_processed === false && dataset.success === null // dataset is processing and has not encountered an error const isProcessing = dataset.is_processing && dataset.is_processed === false && dataset.success === null // processing completed but is no longer available const isExpired = dataset.is_processed && dataset.success === true && moment(dataset.expires_on).isBefore(Date.now()) // dataset is ready to download and has not expired const isReady = dataset.is_processed && !isExpired return { isNotStarted, isProcessing, isReady, isExpired, isErorred, } }

David, as chat on Slack (and noted in here as well #364 (comment)), I've filed a dedicated new issue (#370), for adding getDatasetState helper and refactoring the components that use dataset states.

This should make the review easier while keeping the file changes minimal and reducing the merge conflicts (the PR for it is also ready (#373). Thank you David!

…tate helper in a later PR

davidsmejia

One comment, otherwise everything else I would suggest should be handled in a future PR.

davidsmejia · 2024-10-07T19:06:41Z

src/pages/dataset/[dataset_id].js

-            <>
-              <DatasetPageHeader dataset={selectedDataset} />
+      <Box>
+        {dataset?.data && (


I think we can get rid of this check as well there should only be 2 states we care about. Loading and loaded. I would think that if dataset.data is undefined then loading would be true.

It has been cleaned up in 41e8260 👍

…nozomione/360-refactor-data-set-page-to-handle-dataset-id-via-url-query

nozomione added 3 commits September 17, 2024 14:35

(edit) make explicit return for 'loading' in [dataset_id] instead of …

978bc36

…ternary in JSX, and destructure 'dataset' in DatasetSummary

Merge 'dev' into nozomione/360-refactor-data-set-page-to-handle-datas…

1dab255

…et-id-via-url-query

nozomione self-assigned this Sep 17, 2024

nozomione requested a review from davidsmejia September 17, 2024 21:35

This was linked to issues Sep 17, 2024

Refactor Dataset Page to handle dataset ID via URL query #360

Closed

Discussion: UI presentation when /dataset/id matches user's My Dataset ID #362

Closed

davidsmejia reviewed Sep 24, 2024

View reviewed changes

(rename and cleanup) use the given props as is without renaming in co…

679e50c

…mponents, and remove the conditional check for data (no longer required) in DatasetPageHader

vercel bot deployed to Preview September 25, 2024 16:26 View deployment

nozomione requested a review from davidsmejia September 25, 2024 16:26

(minor) use the name myDataset for user-defined dataset, and get its …

cb76a18

…'id' and 'data' (until we rename the user-defined 'dataset' context state to 'myDataset' in a separate issue)

vercel bot deployed to Preview September 26, 2024 19:27 View deployment

nozomione requested review from davidsmejia and removed request for davidsmejia September 26, 2024 19:35

davidsmejia reviewed Sep 27, 2024

View reviewed changes

nozomione requested a review from davidsmejia October 2, 2024 22:46

(edit) revert the change - this will be handled using the getDatasetS…

6cdcb00

…tate helper in a later PR

vercel bot deployed to Preview October 3, 2024 15:32 View deployment

davidsmejia approved these changes Oct 7, 2024

View reviewed changes

nozomione deleted the branch feature/epic-refactor-dataset-page-data-handling October 7, 2024 20:53

nozomione closed this Oct 7, 2024

nozomione reopened this Oct 7, 2024

nozomione added 2 commits October 7, 2024 17:06

Merge branch 'feature/epic-refactor-dataset-page-data-handling' into …

4fba955

…nozomione/360-refactor-data-set-page-to-handle-dataset-id-via-url-query

(clean up) remove the data object check from JSX in [dataset_id]

41e8260

vercel bot deployed to Preview October 7, 2024 21:17 View deployment

nozomione merged commit f49dcc1 into feature/epic-refactor-dataset-page-data-handling Oct 7, 2024
2 checks passed

nozomione deleted the nozomione/360-refactor-data-set-page-to-handle-dataset-id-via-url-query branch October 7, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

360 - Refactor Dataset Page to handle dataset ID via URL query #364

360 - Refactor Dataset Page to handle dataset ID via URL query #364

nozomione commented Sep 17, 2024 •

edited

Loading

vercel bot commented Sep 17, 2024 •

edited

Loading

davidsmejia left a comment

davidsmejia Sep 24, 2024

davidsmejia Sep 24, 2024

nozomione Sep 25, 2024 •

edited

Loading

davidsmejia Sep 24, 2024

nozomione commented Sep 25, 2024

nozomione commented Sep 26, 2024

davidsmejia Sep 25, 2024

davidsmejia Sep 25, 2024

nozomione Oct 2, 2024 •

edited

Loading

davidsmejia left a comment

davidsmejia Oct 7, 2024

nozomione Oct 7, 2024

	dataset: { id: myDatasetId, data: myDatasetData },
	dataset: myDataset,

360 - Refactor Dataset Page to handle dataset ID via URL query #364

360 - Refactor Dataset Page to handle dataset ID via URL query #364

Conversation

nozomione commented Sep 17, 2024 • edited Loading

Issue Number

Purpose/Implementation Notes

Types of changes

Functional tests

Checklist

Screenshots

vercel bot commented Sep 17, 2024 • edited Loading

davidsmejia left a comment

Choose a reason for hiding this comment

davidsmejia Sep 24, 2024

Choose a reason for hiding this comment

davidsmejia Sep 24, 2024

Choose a reason for hiding this comment

nozomione Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

davidsmejia Sep 24, 2024

Choose a reason for hiding this comment

nozomione commented Sep 25, 2024

nozomione commented Sep 26, 2024

davidsmejia Sep 25, 2024

Choose a reason for hiding this comment

davidsmejia Sep 25, 2024

Choose a reason for hiding this comment

nozomione Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

davidsmejia left a comment

Choose a reason for hiding this comment

davidsmejia Oct 7, 2024

Choose a reason for hiding this comment

nozomione Oct 7, 2024

Choose a reason for hiding this comment

nozomione commented Sep 17, 2024 •

edited

Loading

vercel bot commented Sep 17, 2024 •

edited

Loading

nozomione Sep 25, 2024 •

edited

Loading

nozomione Oct 2, 2024 •

edited

Loading