-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix(datasets): if a dataset fails we display non-failed datasets #933
Conversation
To make a dataset fail you can do this... Inside /api-client/project.js you choose a dataset that you want to make fail and change "NAME-OF-DATASET-THAT-WILL-FAIL" for the name of the dataset that will fail
|
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.
Code looks good. I just have a question about the change because it silently drops datasets that return an error when we try to retrieve the full information. Do you think this is the right behavior, or should we show the information we have for those datasets with a message the the full dataset could not be retrieved?
@@ -54,7 +54,7 @@ function ImportDataset(props) { | |||
|
|||
const findDatasetInKgAnRedirect = (oldDatasetsList) => { | |||
let waitForDatasetInKG = setInterval(() => { | |||
props.client.getProjectDatasetsFromKG(props.projectPathWithNamespace) | |||
props.client.getProjectDatasetsFromKG_short(props.projectPathWithNamespace) |
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.
Can you explain why you switched from getProjectDatasetsFromKG
to getProjectDatasetsFromKG_short
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.
The get projects from KG fetches the datasets list and also the details of the dataset (this means one call per dataset plus one call to get the datasets list). The dataset import fetches de dataset list to see if there was a dataset added and if there is it redirects the user. We don't need all the dataset details that the getProjectDatasetFromKG gives. Maybe i whould rename the functions to "getProjectDatasetsFromKgEnriched" and "getProjectDatasetFromKG"
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 like the name getProjectDatasetsFromKgEnriched
return Promise.all(fullDatasets); | ||
return Promise.all(fullDatasets).then(datasets => { | ||
//in case one of the dataset fetch fails we return the ones that didn't fail | ||
return datasets.filter(dataset => dataset !== "error"); |
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.
Would it make sense to put in a placeholder for the dataset that failed, showing the information we have and a message that the dataset could not be retrieved?
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 made a comment about this bellow... i think that we shouldn't overthink this because it will change soon since kuba will return datasets enriched from the KG.
What happens here is that i get from the KG a list of datasets that has an error, then i try to fetch details from this list and this is when the error makes things fail. This should not happen... i shouldn't be getting from the backend a list that has an error. This is why i decided to drop the error instead of displaying something. (What is causing this error to happen is that there are two datasets with the same ID if i understand correctly, this is why i say this error shouldn't happen). After kuba finishes this SwissDataScienceCenter/renku-graph#298 we will stop fetching dataset per dataset and will have to change this anyways... hopefully this should be solved also so i wouldn't put to many effort on fixing something that will change soon. |
d530c25
to
762d772
Compare
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.
👍
Closes #924
In case a dataset fetch fails all dataset fetch was failing.
This was causing a "no dataset inside this project" message.
Now in case a dataset can't be fetched we display all the datasets that where fetched with no problem.
I also replaced some calls to get dataset for simplified ones when we don't need to get all the dataset info (i.e redirect after dataset add).